Skip to content

fix: run conversation deletion off event loop - #3996

Open
SabhyaC26 wants to merge 1 commit into
mainfrom
codex/delete-conversation-off-event-loop
Open

fix: run conversation deletion off event loop#3996
SabhyaC26 wants to merge 1 commit into
mainfrom
codex/delete-conversation-off-event-loop

Conversation

@SabhyaC26

Copy link
Copy Markdown
Contributor

Related issue

N/A (reported directly)

Summary

  • Offloads SqlAlchemyConversationStore.delete_conversation to a worker thread so its recursive CTE and cross-database deletes cannot block the server event loop.
  • Keeps the existing delete transaction body unchanged behind a synchronous helper and adds a regression test that proves the loop advances while that body is blocked.

ELI5: the server now hands the slow database cleanup to a worker instead of making every live request wait behind it.

async request -> asyncio.to_thread -> existing recursive delete

Test Plan

  • pytest -q tests/stores/test_conversation_store.py tests/stores/test_conversation_store_split_db.py -k 'delete_conversation' — 7 passed.
  • Ruff check and format-check on both changed files.
  • Pyrefly on the changed store implementation — 0 errors.
  • Live Browser verification against a local server + connected host: created a real Claude workflow, sent Reply with exactly DELETE-E2E-READY. Do not use tools., and received DELETE-E2E-READY.
  • Seeded that live parent with 6,000 recursive sub-agent conversations and 6,000 item/FTS rows, then deleted it through the UI. The 4.739-second delete removed all conversation, metadata, item, and FTS rows while 800/800 health probes and 400/400 concurrent session-list requests succeeded (health max 63 ms; session-list max 12 ms).

Demo

N/A — backend concurrency fix; verified through the live UI and Browser workflow above.

Type of change

  • Bug fix
  • Feature
  • UI / frontend change
  • Refactor / chore
  • Docs
  • Test / CI
  • Breaking change

Test coverage

  • Unit tests added / updated
  • Integration tests added / updated
  • E2E tests added / updated
  • Manual verification completed
  • Existing tests cover this change
  • Not applicable

Coverage notes

Manual verification used the current branch's live backend, a real connected local host, and the Browser UI. The recursive delete ran long enough to expose the original freeze while concurrent probes verified that the event loop remained responsive.

Changelog

Deleting large agent conversation trees no longer freezes other server activity.

@github-actions github-actions Bot added the size/S Pull request size: S label Aug 3, 2026
@omnigent-ci

omnigent-ci Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Missing visual demonstration

This PR fixes a backend bug where a large recursive delete froze the server event loop — a "stuck/broken → fixed" behavior change. The description reports a live load test with concrete latency numbers (4.739 s delete while 800/800 health probes and 400/400 session-list requests stayed responsive), but no screenshot or video is attached ("Attached images/videos" = none found). Please attach a short capture or graph from that load test (e.g. probe latency staying flat during the delete) so reviewers can see the responsiveness before/after without reproducing the 6,000-node setup. This is a nudge, not a blocker.

Blocking issues

None. The change is a minimal, correct offload of the existing synchronous delete body to a worker thread. Verified:

  • The transaction body is moved verbatim into _delete_conversation_sync; no delete logic changed.
  • current_workspace_id() reads a ContextVar, and asyncio.to_thread copies the current contextvars.Context into the worker thread, so workspace scoping (used throughout the delete) still resolves correctly off-thread — no scoping regression.
  • SQLite engines are created with check_same_thread=False and non-SQLite engines use a thread-sized pool (pool_size=200) explicitly aligned with the AnyIO to_thread limiter, so running this body on a worker thread is consistent with the existing engine design and how sibling calls (get_conversation, etc.) are already dispatched via asyncio.to_thread at call sites.

Security vulnerabilities

None. No new inputs, query construction, deserialization, or trust-boundary changes — same parameterized SQLAlchemy deletes, now executed on a worker thread.

Non-blocking notes

  • The interface docstring in conversation_store/__init__.py still says the method is "Async because it may need to cancel in-flight responses" — that rationale predates this PR and the impl still doesn't cancel in-flight responses. Now that async is also justified by the thread offload, consider updating that docstring for accuracy (out of scope here).
  • The regression test asserts loop non-blocking via started/release events with a 1s timeout. It's sound and deterministic in practice; just note the 1s bounds could theoretically flake on a severely overloaded CI host — acceptable.

Summary

A tight, well-scoped concurrency fix: it wraps the existing recursive-delete transaction in asyncio.to_thread without altering delete semantics, ContextVar-based workspace scoping is preserved across the thread boundary, and the engine/pool config already supports off-loop DB access. The added regression test meaningfully guards the "loop advances while delete blocks" property. No correctness or security concerns — the only ask is attaching the visual/latency evidence from the load test that's described but not embedded.


Automated review by Polly · workflow run

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Benchmark results (SQLite, PR #3996)

Commit: 85ebf70456eb8f03bf9d3526fc6bd4955fc6b277

Benchmark comparison

Regression threshold: 100% on avg P50 or avg P95.

Journey Status Base P50 ms Cand P50 ms Δ P50 Base P95 ms Cand P95 ms Δ P95 Req/op
list_sessions ✅ ok 36.2 38.2 +5.6% 144.5 142.6 -1.3% 1.0
create_session ✅ ok 304.8 325.5 +6.8% 323.7 337.3 +4.2% 2.0
get_session ✅ ok 16.4 17.7 +7.7% 17.7 18.9 +6.6% 1.0
load_conversation_history ✅ ok 6.0 6.4 +6.3% 6.7 7.4 +9.7% 1.0
search_sessions ✅ ok 219.8 213.2 -3.0% 342.6 319.3 -6.8% 1.0
list_projects ✅ ok 33.1 31.8 -4.1% 165.1 136.6 -17.3% 1.0
list_project_sessions ✅ ok 56.8 53.7 -5.5% 191.3 162.4 -15.1% 1.0
fork_session ✅ ok 20.7 19.5 -5.9% 23.3 21.6 -7.0% 1.0
add_comment ✅ ok 5.8 5.3 -8.4% 7.2 6.1 -15.6% 1.0
policy_evaluate ⚠️ skipped
session_cold_start ✅ ok 3748.8 3991.2 +6.5% 3816.1 4034.2 +5.7% 13.0
session_cold_restart ✅ ok 3946.6 4218.3 +6.9% 4019.7 4308.9 +7.2% 12.0
warm_turn ✅ ok 144.0 145.8 +1.2% 208.6 209.3 +0.4% 3.0
time_to_first_token ✅ ok 381.8 378.1 -1.0% 398.3 395.1 -0.8% 4.8
interrupt ✅ ok 129.0 129.2 +0.2% 138.5 138.4 -0.1% 5.0
read_runner_file ✅ ok 15.4 13.8 -10.6% 20.5 18.0 -12.1% 1.0

PASS — no regressions detected.

Signed-off-by: SabhyaC26 <sabhyachhabria@gmail.com>
@SabhyaC26
SabhyaC26 force-pushed the codex/delete-conversation-off-event-loop branch from 8fabf67 to 85ebf70 Compare August 11, 2026 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Pull request size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant