Repository navigation
fix(gateway): bound subagent batch shutdown - #6434
Conversation
willem-bd
left a comment
There was a problem hiding this comment.
Verified at head a9a2892 (static review; no PR code executed). This is the follow-up #6409 left scoped, and it follows the established pattern correctly: dedicated helper reusing _SHUTDOWN_HOOK_TIMEOUT_SECONDS, preserved ordering (subagent_batches_available = False before stop, submitter cleared in finally), and the #6409-style tests (bounded-stop regression plus lifespan wiring assertion). One blocking gap: the new bounded site is not modeled in backend/tests/_gateway_shutdown_budget.py, so test_shutdown_budget_models_every_bounded_lifespan_hook fails deterministically (7 sites vs 6 modeled) and the deployment grace arithmetic under-counts the new sequential 5s hook. The deeper timeout-semantics concern (wait_for cancels stop() mid-orchestration, skipping native/item cancellation) was already flagged in the prior review thread and is not repeated here.
| # scheduled task service, subagent batch service, browser sessions, | ||
| # MCP session pool), the 1s retrieval-warm wait, the 5s | ||
| # run drain and the 30s memory flush (memory.shutdown_flush_timeout_seconds) | ||
| # -- about 61s -- plus slack. backend/tests/_gateway_shutdown_budget.py |
There was a problem hiding this comment.
[P3] The refreshed total is stale: "seven hooks bounded at 5s each" (35s) + 1s retrieval-warm + 5s run drain + 30s memory flush = about 71s, not 61s. The helm README was updated to "about 71s" in 34da587, so the two docs now disagree; 61s was only correct for the old five-hook list. stop_grace_period: 90s still covers 71s + the 10s --timeout-graceful-shutdown + slack, so this is comment-only - but the very next line claims backend/tests/_gateway_shutdown_budget.py pins the arithmetic, so the number should track the model.
Why
Gateway shutdown can still hang indefinitely in the durable subagent-batch path. Current main calls
await app.state.subagent_batch_service.stop()without a deadline.SubagentBatchService.stop()cancels and then gathers its poller and active execution tasks, so a cancellation-resistant task can hold the Gateway lifespan open forever.This is the follow-up explicitly left out of #6409: MCP-task shutdown is internally bounded, but the subagent-batch service is not.
What changed
SubagentBatchService._SHUTDOWN_HOOK_TIMEOUT_SECONDSpolicy introduced for other lifecycle hooks.stop()plus a lightweight lifespan wiring assertion.Surface area
Screenshots / Recording
Not applicable.
Bug fix verification
backend/tests/test_subagent_batch_shutdown_timeout.py::test_subagent_batch_service_shutdown_is_boundedlifespandirectly awaitssubagent_batch_service.stop()with no deadline, while that method awaits cancelled poller/execution tasks viaasyncio.gather(..., return_exceptions=True).Collision audit
Repeated immediately before publication against current upstream
main@1d3e68363c38f5c55fbb47a87400c8e3ba7d1fa1.SubagentRuntime.stop(); it does not bound Gateway waiting onSubagentBatchService.stop().Validation
fix/subagent-batch-shutdown-timeoutf930911c54ec6c3cb2a8b6d2972e94d16e5a26edmain@1d3e68363c38f5c55fbb47a87400c8e3ba7d1fa1SubagentBatchService.stop()(all native/item cancellation requests are issued before the first await), a regression that pins that ordering, and synchronized shutdown-budget accounting/docs. The branch remains architecture-aligned with the Gateway bounded-hook policy.git diff --check: not run / blocked — connected validation device is offline.AI assistance
Tool(s) used: ChatGPT
How you used it: Audited Gateway and subagent-batch shutdown ownership, confirmed the unbounded await on current main, implemented the minimal architecture-matching timeout helper and regression, and performed collision checks.