Repository navigation
fix(subagents): drain owned batch stop across cancellation - #5525
WillemJiang merged 2 commits into
Conversation
7522273 to
66cd99b
Compare
willem-bd
left a comment
There was a problem hiding this comment.
Verified empirically at head 66cd99b: the new regression test fails on base 16f154f with exactly the reported message and passes on this branch; the focused suites (test_subagent_runtime.py, test_subagent_batch_service.py) plus the broader subagent lifecycle/executor suites pass (160 passed; 5 pre-existing env-only collection errors in test_extension_subagent_lifecycle.py also occur on base), and ruff check/format are clean. The fix itself looks correct: the lifecycle lock is held for the whole drain, so concurrent start()/stop() callers still serialize and a second stop() still returns only after the service reaches a terminal state; hiding the submitter immediately matches the stated intent, and the first caller cancellation is preserved. Two minor points below on the unbounded drain and an exception-guard asymmetry — neither blocks.
| cancellation: asyncio.CancelledError | None = None | ||
| while not stop_task.done(): | ||
| try: | ||
| await asyncio.shield(stop_task) |
There was a problem hiding this comment.
Nit/suggestion on robustness: this drain is intentionally unbounded — once the first cancellation is recorded, stop() can never be abandoned until service.stop() reaches a terminal state. That holds for today's SubagentBatchService.stop() (it cancels the poller/children and gathers with return_exceptions=True), but one pathological await inside the service — a repository call that ignores cancellation, or a gathered child task that suppresses CancelledError without exiting — would now make runtime (and by extension Gateway) shutdown permanently un-cancellable, where old main would at least escape (buggily). Consider either bounding the drain (e.g., asyncio.wait({stop_task}, timeout=...), then re-raising the recorded cancellation regardless) or documenting the invariant that the owned service's stop() must always terminate.
| if cancellation is not None: | ||
| try: | ||
| stop_task.result() | ||
| except Exception as exc: |
There was a problem hiding this comment.
Nit on an asymmetry: if stop_task itself ends cancelled, stop_task.result() re-raises CancelledError as a BaseException, which bypasses this except Exception guard — so the recorded first caller cancellation is silently dropped and the raise cancellation from exc chaining never happens. With the current SubagentBatchService.stop() that state is unreachable (the shield keeps outer cancellation off the inner task, and the service gathers with return_exceptions=True), so it's theoretical today, but checking stop_task.cancelled() (or catching BaseException) would make the "preserve the first caller CancelledError" contract hold unconditionally.
Why
A caller can cancel
SubagentRuntime.stop()after the runtime has already hidden its owned batch submitter but before the ownedSubagentBatchService.stop()has actually finished.Current main sets
_batch_started = Falsebefore directly awaiting the service stop. If that await is interrupted, the runtime now reports no batch submitter and any laterstop()call returns immediately, while the owned service may still be part-way through stopping. The real service has cancellable awaits before active executions are cancelled and its ownership maps are cleared, so this can leave runtime-owned batch work alive after the runtime itself has released lifecycle ownership.What changed
Task.cancel().CancelledError, but only propagate it after the owned service stop reaches a terminal state.Surface area
frontend/backend/appSubagentRuntimelifecycle ownershipdocker/or sandboxed executionskills/backend/pyproject.tomlorfrontend/package.jsonScreenshots / Recording
N/A — backend lifecycle/cancellation fix.
Bug fix verification
Test:
backend/tests/test_subagent_runtime.py::test_runtime_stop_drains_owned_batch_worker_across_repeated_cancellation.Deterministic schedule:
SubagentRuntimewith an owned batch service.service.stop()after it starts.runtime.stop()once, then again while cleanup is still blocked.Red on current main
16f154f32b6104ddeea525c713445565e648aeee: yes. The task becomes cancelled/done after the first cancellation while the service stop is still blocked.Green on this branch: yes. The runtime keeps lifecycle ownership through repeated cancellation until the owned stop completes.
Collision audit
Re-checked immediately before submission:
SubagentRuntime.stop();subagents/runtime.py, and explicitly scopes hardTask.cancel(), batch persistence, and Gateway shutdown outside that PR;SubagentRuntime.stop();Validation
main16f154f32b6104ddeea525c713445565e648aeee; the two touched file blobs are unchanged from the validated head, so the rebase only changed the parent commit.66cd99b38bb73b9077a178252726b5f965845b02, 1 commit ahead / 0 behind; diff is 2 files, production change +21/-3.runtime stop released ownership after the first cancellation.tests/test_subagent_runtime.py tests/test_subagent_batch_service.py— 10 passed.ruff checkon both changed files — clean.ruff format --checkon both changed files — clean.python scripts/check_agent_guidance.py— 28 AGENTS.md, 0 errors, 0 warnings.git diff --check— clean.AI assistance
Tool(s) used: ChatGPT
How you used it: Audited the current runtime lifecycle, constructed and ran the deterministic cancellation schedule on current main, performed collision checks against active subagent work, implemented the minimal shield/drain fix, rebased it onto current main, and ran focused tests/lint.