fix(api_server): decouple session SSE from agent execution on client disconnect - #96507
fix(api_server): decouple session SSE from agent execution on client disconnect#96507antonysit wants to merge 2 commits into
Conversation
…disconnect
POST /api/sessions/{id}/chat/stream always persists to state.db, so a
dropped SSE connection is only a dead transport, not a stop signal.
Detach on disconnect: leave the agent running server-side and drain the
now-unread queue until the end sentinel instead of interrupting. The Stop
button still interrupts via POST /v1/runs/{run_id}/stop (the run stays
registered in _active_run_agents until the executor turn exits).
Fixes the mobile backgrounding case where switching apps / locking the
screen killed the SSE stream and cancelled the agent mid-task.
Refs NousResearch#15026
Related: #15492 pursues durable response-run/subscriber separation, while this PR narrowly detaches the session SSE handler and drains its unread queue. #85016 records the corresponding lifecycle-policy decision. Please consider the desired disconnect semantics across this cluster. |
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head e025fe01df86f1969da38f7191b769ec29fa3256 against live main@39f1e1881a027cead71c81b7b747be1c7ad6853a. The actual live-main merge base is 46f091b93e8fbe04f02d9200950145dcf5e3b031, so this head is 1 commit ahead / 18 behind; the main-only drift I checked is path-disjoint from this PR's two files. There were no submitted reviews on this head when I started.
The core lifecycle correction is right: a session stream transport disappearing should not automatically mean the persisted agent turn was abandoned. I specifically like that shutdown cancellation remains separate, _active_run_agents stays live until _run_agent() actually exits, and the detached consumer drains the now-unread queue rather than letting SSE events accumulate for the rest of a long tool turn. The new regression also proves the old transport-disconnect interrupt is gone while preserving the run control ref. That is a useful, narrowly scoped repair.
I do have one P1 lifecycle/control-plane blocker, plus the repository's hard decomposition gate.
P1 — a detached run can outlive the only public coordinate that can control or reconcile it
The new contract says the turn keeps running and can still be stopped through POST /v1/runs/{run_id}/stop. That is only true if the client successfully learned run_id before its transport died.
On this exact head, _handle_session_chat_stream() mints a fresh run_<uuid> server-side. The session SSE response headers expose X-Hermes-Session-Id (and optionally X-Hermes-Session-Key), but not the run id. The run id is projected into the event payload by _event_payload() and first becomes client-visible in run.started. There is no list-runs route, and the client-safe session resource does not expose an active run id; the public run control routes are all keyed by a run id the caller must already know.
The new disconnect test does not exercise that boundary: its fake response fails on the third write. So the test first lets the initial control events be written, then proves that a client which already received the run coordinate can detach safely. The adversarial sibling is a disconnect on the first run.started write (or before those bytes reach the client): this PR deliberately keeps the turn alive, but the client retains only the session id and has no supported way to rediscover the exact run for status, stop, steer, or approval.
There is a second manifestation of the same ownership gap on reconnect. Every new POST /api/sessions/{session_id}/chat/stream creates another fresh run id/task; this handler has no durable request idempotency/reattach contract or session→active-run reconciliation gate. After an ambiguous disconnect, a client retry can therefore create a second execution while the first detached execution is still live. For an agentic turn, that is not just duplicate text: it can repeat external tool side effects.
This is exactly the other side of "transport != lifecycle": once execution is independent of the subscriber, execution identity must also be independent of the subscriber. Keeping the task alive without preserving a recoverable control coordinate turns a dropped socket from an implicit Stop into potentially ownerless/duplicated background work.
Required repair
Please make the session-stream run a durable/recoverable control object, not only an in-memory queue producer. A clean contract would be one of:
- persist/index the current
active_run_id+ immutable request fingerprint/idempotency key on the session (or equivalent run record), and expose that exact run independently of the SSE body; and - on reconnect/retry, reattach/replay the same execution or return a typed deterministic
run_already_active/idempotency conflict rather than starting another agent for the same admitted request.
A response header is useful but not sufficient as the sole proof because the connection can fail before the client receives the header. The recovery coordinate has to be queryable from durable/session state or another authenticated stable key.
Please add vertical regressions that prove:
- the first SSE write fails, the agent keeps running, and the same authenticated client can rediscover the exact run and stop it;
- disconnect + retry of the same session/request produces exactly one underlying agent execution / side-effect, with the retry reattaching/replaying or refusing deterministically; and
- a detached run that enters approval remains discoverable and addressable after the original SSE subscriber is gone.
The neighboring work makes the ownership boundary pretty clear. #85016 is the open API-server lifecycle-policy issue for detach-vs-interrupt. #15026 is the stored /v1/responses recovery incident. Open #15492 is complementary, not duplicate: it is building the broader ResponseRun / subscriber separation, active-run recovery, idempotency and reattach semantics for /v1/responses. This PR owns the narrower persisted-session stream path, but these two paths should converge on one run/subscriber lifecycle vocabulary rather than ship two different meanings of "background run". #94697 is another complementary instance of the same defect class for Bot/TUI viewer detach: losing presentation ownership must not silently become mutation/lifecycle authority.
Credit-wise, Antony Sit owns this narrow session-stream repair. The older reports/proposals in #85016 and #15026 are incident/design provenance, not code ownership to flatten into this PR.
Hard architecture gate — do not regrow the killed API-server godfile
This head adds 45 lines of lifecycle authority directly around line ~4960 of gateway/platforms/api_server.py. #78643 is the explicit full-file decomposition owner, and completed epic #78647 records gateway/platforms/api_server.py as KILLED under the standing shard-never-regrow rule. The repository acceptance contract is hard here: modified files must remain under the 2K ceiling; new behavior belongs behind a sub-2K owner with the legacy surface left as a thin delegate/re-export.
The new detach/drain logic is already coherent enough to be a small lifecycle/stream helper. Please land the behavior through the accepted API-server decomposition topology rather than adding another authority cluster back to the monolith. #83546 is useful precedent for preserving the public import/seam while moving ownership out of the file.
Exact-head evidence
Hosted exact-head acceptance has not executed yet. CI 33093448609, Docker 33093447514, and Nix 33093447639 are all action_required, and the exact commit currently exposes zero check-runs. That is not a code-failure finding by itself—it is simply not a green receipt, so local/targeted evidence cannot be promoted into exact-object acceptance yet.
The detach direction is good, and the queue/control-ref details are careful. Once the run identity survives the subscriber lifecycle just as strongly as the execution does—and this is composed through the API-server shard boundary—the mobile/backgrounding fix becomes much safer to land. 🚀
|
@andrexibiza thanks for the thorough review — the transport/lifecycle framing is right, and the concrete points are fair. Responding point by point. P1 (ownerless run). Agreed, this is real. To be precise about the exposure in the current deployment: the primary client captures Your required repair is the right shape and I agree with it: persist the active Godfile gate. Fair, and I'll honor it: the detach/drain helper is self-contained enough to extract into a sub-2K module with Where this lands. #15492 is building the broader ResponseRun/subscriber separation with reattach + idempotency semantics; this PR owns the narrower persisted-session-stream path, and I agree the two should converge on one run/subscriber lifecycle vocabulary. I'd like to land this narrow detach first and implement the durable run coordinate as the next iteration — either in this PR or a sibling, whichever boundary maintainers prefer — with #85016 tracking the policy decision. Happy to proceed either way. |
…ousResearch#96507 P1) Persist the session-stream run as a durable control object instead of a transport-scoped handle: - sessions row gains active_run_id + active_run_key + active_run_status, auto-reconciled by _reconcile_columns (no version bump). - SessionDB gains claim_session_active_run (atomic conditional claim), set_session_active_run_status, clear_session_active_run (run-id-guarded). - GET /api/sessions/{id} exposes active_run_id + active_run_status so any authenticated client can rediscover a detached run. - Retry guard: a second start for a session with a live run returns a deterministic 409 run_already_active conflict (carrying the existing run_id) instead of starting a second execution. Idempotency-Key header honored; deterministic (session, prompt, message) fingerprint fallback. - Extract detach/drain helpers into gateway/platforms/api_server_session_stream.py (sub-2K, godfile gate epic NousResearch#78647 / precedent NousResearch#83546); api_server.py keeps thin delegates. Regressions: first-SSE-write-fails-then-rediscover-and-stop; disconnect+retry yields exactly one execution; detached approval run stays discoverable and addressable; completion clears the active-run slot. Refs NousResearch#15026, NousResearch#85016
|
Implemented the P1 repair on this branch. The session-stream run is now a durable control object: Added the three vertical regressions:
Keeping this in this PR rather than a sibling, since it directly resolves the review blocker. |
POST /api/sessions/{id}/chat/stream always persists to state.db, so a dropped SSE connection is only a dead transport, not a stop signal. Detach on disconnect: leave the agent running server-side and drain the now-unread queue until the end sentinel instead of interrupting. The Stop button still interrupts via POST /v1/runs/{run_id}/stop (the run stays registered in _active_run_agents until the executor turn exits). Fixes the mobile backgrounding case where switching apps / locking the screen killed the SSE stream and cancelled the agent mid-task. Refs #15026