fix(orchestrator): prevent double cancel() from killing start() generator - #183
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
/gemini review |
…ator In OpenCode mode, abort_session called cancel() twice — once via interrupt() → cancel_run_for_session() and again via session_pool.cancel_run(). The second cancel() threw CancelledError at _idle_event.wait() inside _idle_loop(), which was OUTSIDE the except CancelledError handler in start(). This killed the generator and all subsequent messages got stuck. Three-layer defense: 1. abort_session: skip redundant cancel_run() for per-session agents (interrupt() already calls cancel_run_for_session() internally) 2. start(): widen except CancelledError to wrap entire while-loop body including _idle_loop(), so stray CancelledError during idle is caught 3. cancel(): make idempotent — if _force_cancelling is already True, don't call task.cancel() again Additional fixes: - followup() now checks _closed in addition to _closing (defense-in-depth) - _start_run_handle() resets agent._cancelled to prevent stale flag from a previous aborted run persisting into new runs Closes #182 Co-authored-by: Oracle <oracle@agentpool>
Exercises the full pydantic-ai execution path (Agent → NativeTurn → agentlet.iter() → TestModel) to verify the RunHandle survives a double cancel during idle and successfully processes a followup message through the real pydantic-ai turn execution path. This complements the mock-based unit tests which verify RunHandle control flow (cancel/idle/steer mechanics) but don't exercise the real pydantic-ai code path.
There was a problem hiding this comment.
Code Review
This pull request fixes a critical bug where double-cancelling a session kills the orchestrator's start generator, leaving subsequent messages stuck. The fix introduces a three-layer defense: making the cancel operation idempotent, widening the CancelledError exception handling in the start loop, and preventing redundant cancel calls during session abortion. Stale agent cancellation flags are also reset on new runs. The review feedback focuses on improving the newly added regression tests by replacing flaky, hardcoded asyncio.sleep calls with deterministic waits on the turn completion event.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
- turn.py: check if prompt item is a list before extend(); single content items like ImageUrl must be appended, not extended - test_session_lifecycle: update test to match new abort behavior — per-session agents call interrupt() only, cancel_run() is NOT called separately (fixes #182 double cancel bug)
91fce4d to
3a54beb
Compare
Address review feedback: use _turn_complete_event.wait() instead of hardcoded asyncio.sleep() for deterministic, faster tests. - First cancel + followup: clear + wait_for(_turn_complete_event) - Second cancel (during idle): sleep(0) to yield to event loop only, since outer handler doesn't set _turn_complete_event
Problem
In OpenCode mode, after canceling (aborting) a running session, both queued messages and new user messages get stuck — the run never resumes.
Root Cause
The
abort_sessionhandler calledrun_handle.cancel()twice:session_agent.interrupt()→cancel_run_for_session()→run_handle.cancel()session_pool.cancel_run()→run_handle.cancel()The second
cancel()threwCancelledErrorat_idle_event.wait()inside_idle_loop(), which was OUTSIDE theexcept asyncio.CancelledErrorhandler instart(). This killed thestart()generator, and all subsequent messages got stuck because the RunLoop was dead.The design flaw in
start():The
except CancelledErrorhandler only wrapped_execute_turn/_handle_turn_result. CancelledError thrown during_idle_loop()was not caught, propagating through thewhileloop →finallyblock → out ofstart(), killing the generator.Fix — Three-Layer Defense
Fix #1 (Primary): Remove redundant
cancel_run()inabort_sessionFor per-session native agents,
interrupt()already callscancel_run_for_session()internally. The explicitsession_pool.cancel_run()was redundant and lethal. Now only called for non-per-session (shared) agents whereinterrupt()isn't used.Fix #2: Widen
except CancelledErrorinstart()Moved the
try/except asyncio.CancelledErrorto wrap the entirewhileloop body (including_idle_loop()). Any strayCancelledErrorduring idle is now caught, and the loop continues to idle waiting.Fix #3: Make
cancel()idempotentIf
_force_cancellingis alreadyTrue(from a previouscancel()that hasn't been processed yet),cancel()now returns without callingtask.cancel()again. This prevents a secondCancelledErrorfrom being thrown.Additional Fixes
followup()defense: Now checks_closedin addition to_closing— messages to dead runs are rejected instead of silently lost._start_run_handle()cleanup: Resetsagent._cancelled = Falsebefore creating a newRunHandleto prevent a stale flag from a previous aborted run from persisting into new runs (latent ACP bug).Test Coverage
New regression tests in
tests/orchestrator/test_double_cancel.py:test_double_cancel_does_not_kill_generator— generator survives double canceltest_double_cancel_then_followup_processes_message— messages processed after double canceltest_cancel_idempotent_when_force_cancelling_already_true— idempotent canceltest_followup_rejected_after_close— followup rejected for dead runstest_cancelled_event_loop_survives_idle_cancel— CancelledError in _idle_loop() caughtAll 51 existing cancel/run_handle tests also pass.
Closes #182