fix(tui_gateway): keep Stop from stranding a session on a dead compute host or an in-flight agent build (supersedes #71825) - #75609
Conversation
|
Thanks for carrying the fix through the handler split. The premise is present on current main: The proposed accessor is synchronized with the existing pending-turn mutations, and the recovery guards avoid clearing a pending or replacement turn. The added focused tests cover the dead-host, pending-completion, post-teardown, and replacement-inflight cases. Automated hermes-sweeper review. |
28af523 to
a014d72
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a TUI gateway turn-isolation failure mode where session.interrupt could leave a session permanently “busy” (forever queueing prompt.submit) if the compute-host child was already dead and HostSupervisor.interrupt() raised.
Changes:
- Add a
HostSupervisor.has_pending_turn(sid)probe to determine whether a compute-host completion callback is still pending for a session. - Update
session.interrupt(compute-host branch) to avoid early-return on interrupt failure, and to safely force-clearsession["running"]+inflight_turnonly when guarded conditions indicate no completion will ever arrive. - Add regression tests covering interrupt-failure recovery and its guarded “do not clobber” cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tui_gateway/methods_session.py | Ensures Stop recovers local busy state even when compute-host interrupt fails, with race-safe guards. |
| tui_gateway/host_supervisor.py | Adds a lock-protected accessor to check whether a session still has a pending host completion. |
| tests/test_tui_gateway_server.py | Adds regression coverage for dead-host interrupt recovery and negative/race cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… fails Stop
`session.interrupt` on a turn-isolation session called
`HostSupervisor.interrupt()` and, on failure, returned 5019 immediately. That
early return skipped every recovery step the same handler performs a few lines
below: `_turn_cancel_requested` was never latched, `queued_prompt` was never
cleared, `_clear_pending(sid)` never ran, pending approvals were never denied,
and `session["running"]` was left True.
A compute host that is gone (broken pipe, respawn exhausted) can never deliver
the `turn.end` that clears `running`, so the session stayed busy permanently:
every later `prompt.submit` fell into `_handle_busy_submit` and queued forever.
The user pressed Stop, saw an error, and had no way back short of restarting the
backend.
Recover instead of bailing out, mirroring the in-process branch's own safety net
("force-clear it so the session can't be permanently bricked at 4009 'session
busy'"). The only difference is the liveness probe: `not run_thread_alive`
becomes `not supervisor.has_pending_turn(sid)`.
The force-clear is guarded so it can never clobber a live turn. It requires the
interrupt to have actually failed, the supervisor to hold no pending completion
for this sid, `running` to still be set, and `inflight_turn` to be the same
object observed before `interrupt()` was called — a replaced inflight means a
successor submit or drain already owns the session. The probe fails closed: an
unreadable supervisor is treated as "still pending", so a probe error prefers
leaving teardown to the waiter.
`has_pending_turn()` is added to `HostSupervisor`; `_pending_turns` already
existed but had no accessor. It reads under the same `self._lock` that guards
every other mutation of that dict. The probe is issued before `history_lock` is
taken, honoring the invariant stated at the `_interrupt_busy_session` call site:
never call a compute-host method while holding `history_lock`, since an
interrupt can wait behind the very operation it is cancelling.
Stop still reports success — from the user's point of view the turn is over
either way — and the failure is logged instead of swallowed.
…ent build `session.interrupt` resolved its session through `_sess`, whose `_wait_agent` blocks up to 30 seconds on the very `agent_ready` Event the deferred build it is cancelling still holds. Stop waited on its own target. This is the default path, not an edge case. `session.create` unconditionally seeds `"agent": None` alongside an unset `agent_ready`, and `prompt.submit` marks the session running and returns before the build completes. The compute-host branch above returns early and was never affected, and `turn_isolation` defaults to False in `config_defaults`, so the in-process branch is the one almost every session takes. Two failure modes followed from it: * Build finishes inside 30s — Stop is merely late. But `session.interrupt` is not in `_LONG_HANDLERS`, whose own comment names it as an RPC that must not be left "unread in the stdin pipe", so it runs inline on the socket reader thread and stalls every RPC queued behind it as well. * Build outlives 30s — `_wait_agent` returns 5032 and the handler returns that error *before* `_turn_cancel_requested` is ever set. `_wait_agent_for_prompt` then polls a flag nobody set, for as long as the `agent.build_wait_timeout` cap (600s), and goes on to call `_run_prompt_submit`. The turn the user cancelled runs anyway. Nothing in this handler needs a built agent, so resolve through `_sess_building` — the helper `_sess` itself delegates to, whose docstring makes precisely this argument for the attach RPCs: "`_sess`'s `_wait_agent` was buying nothing and charging up to 30 seconds for it". The build is still warmed; we simply stop blocking on it. When the agent already exists `_sess_building` returns immediately and behaviour is unchanged. The branch's single agent use is now guarded, since `agent` is None while the build is in flight and this change makes that state newly reachable there.
…st to rule out `test_interrupt_before_agent_ready_prevents_late_turn_start` and its documented sibling `test_cancelled_turn_before_agent_ready_emits_error_event` both pin Stop behaviour "during lazy agent startup", but each monkeypatched `_wait_agent` to a no-op and built its session with `_session()`, which never seeds `agent_ready`. Either alone is enough to hide the defect: with the stub the real resolver never runs, and without `agent_ready` the real `_wait_agent` returns immediately on its `ready is not None` guard. So neither test could fail no matter what `session.interrupt` did with the deferred build. Seed `agent_ready` with a real, never-set `threading.Event` — exactly what `session.create` installs while the build is in flight — and drop the stub, so the handler is exercised through its actual resolution path. The Event is then set just before the deferred run thread is stepped, which is the "after init finishes" the first test's own docstring describes, and it releases `_wait_agent_for_prompt` on its first slice instead of burning one. Both now fail against the previous `_sess` resolution with `5032 agent initialization timed out` after a real 30s wait each, and pass in under a second against `_sess_building`. `test_session_not_running_before_agent_ready_emits_error_event` carries the same stub but is left alone deliberately: it never calls `session.interrupt`, it clears `running` directly, so `_wait_agent` is unreachable in it and the stub is inert. It stays green across both sides of the change.
…ight or failed Two invariants the end-to-end tests above cannot state directly. `test_interrupt_never_waits_on_the_deferred_agent_build` counts `_wait_agent` and asserts zero, following the same idiom `test_config_set_model_explicit_provider_skips_broken_default_init` already uses for this. The end-to-end tests can only catch a regression here by really spending the 30 seconds; this one fails in milliseconds and names the invariant — Stop resolves its session without blocking on the build it is cancelling — rather than one of its symptoms. It also asserts `agent_ready` is still unset when Stop returns, so the flag cannot be reached by waiting. `test_interrupt_recovers_a_session_whose_agent_build_failed` covers a second, independent way `_sess` blocked Stop: `_wait_agent` also returns 5032 when `agent_error` is set, even for an `agent_ready` that IS set. Once a build had failed, `session.interrupt` returned that error before reaching the cancel and `running` cleanup — and nothing else clears `running` on that path, so the session stayed permanently busy and every later prompt.submit queued behind it. That is the same "session bricked at 4009" trap the compute-host branch already guards, arrived at through the in-process branch instead. Both fail against the previous `_sess` resolution (5032 "agent initialization timed out" and 5032 "provider metadata fetch failed" respectively) and pass against `_sess_building`.
a014d72 to
9bdac35
Compare
This supersedes #71825 (same bug, current code)
#71825 reported this bug correctly, and its shape is the right one — this PR carries it over. It cannot land as filed, for a structural reason rather than a staleness one:
tui_gateway/server.py @@ -10252,14 +10252,44, but commitf67ca22moved the session RPC bodies out ofserver.pyintotui_gateway/methods_session.py. Onmaintoday,git show origin/main:tui_gateway/server.py | grep -c 'method("session.interrupt")'→ 0. The handler it patches is not the handler that runs;server.py:10252is now unrelateddisplay.tool_progressfocus-config code.mergeable: CONFLICTING/mergeStateStatus: DIRTY.Its
HostSupervisor.has_pending_turn()addition still applies cleanly and is kept here essentially verbatim. Its four regression cases are ported by hand (its test hunk anchors at@@ -322of a file that is now 15,282 lines).What does this PR do?
session.interrupton a turn-isolation session callsHostSupervisor.interrupt()and, on failure, returns5019immediately:HostSupervisor.interrupt()callsstart()and then_send_frame(), so a host that is gone — broken pipe, respawn exhausted — makes it raise. The early return then skips every recovery step below it:_turn_cancel_requestedis never latched,queued_promptis never cleared,_clear_pending(sid)never runs, pending approvals are never denied, andsession["running"]is leftTrue.Because the host is dead, no
turn.endwill ever arrive to clear busy. Every laterprompt.submittherefore falls into_handle_busy_submitand queues forever. The user pressed Stop, got an error, and the session is bricked until the backend is restarted.The repo already documents the expectation this branch fails to honour
The in-process branch of the same function, ~15 lines below, carries this exact safety net:
The fix is derived from that idiom. The only thing that differs is the liveness probe:
not run_thread_alivebecomesnot supervisor.has_pending_turn(sid).The same handler's in-process branch had a second way to strand Stop
Everything above is the
turn_isolationpath.turn_isolationdefaults toFalseinhermes_cli/config_defaults.py, so the branch almost every session actually takes is the in-process one directly below it — and it stranded Stop too, for an unrelated reason.It resolved its session through
_sess:_sessis_sess_buildingplus_wait_agent, and_wait_agentblocks up to 30 seconds onsession["agent_ready"]:That is the same Event the deferred build has not set yet. Stop waited on the build it was cancelling.
This is the default path, not an edge case:
session.createunconditionally seeds"agent": Nonealongside an unsetagent_ready, andprompt.submitmarks the session running and returns before the build completes.Three consequences, all user-visible:
session.interruptis not in_LONG_HANDLERS, so it runs inline on the socket reader thread and every RPC queued behind it is stalled with it._wait_agentreturns5032and the handler returns before_turn_cancel_requestedis ever set._wait_agent_for_promptthen polls a flag nobody set, for up toagent.build_wait_timeout(default 600s), and goes on to call_run_prompt_submit. The turn the user cancelled runs anyway._wait_agentreturns5032from itsagent_errorbranch even for a setagent_ready. Stop errors out before the cleanup below it, and nothing else clearsrunning— the session is permanently busy, which is exactly the trap the compute-host half of this PR guards against, reached through the other branch.The repo states this remedy against itself
_sess_buildingexists precisely for handlers in this position, and its own docstring makes the argument:_LONG_HANDLERS' own comment names this RPC as one that must not be left waiting:And
_wait_agent_for_prompt's docstring records that the flat 30s_wait_agentceiling was "a message-eating cliff (#63078)" — which was fixed forprompt.submitand left in place forsession.interrupt.The commit that introduced
_sess_building("fix: attach RPCs no longer wait on the deferred agent build") converted the six attach RPCs —clipboard.paste,image.attach,image.attach_bytes,pdf.attach,file.attach,image.detach— and did not sweep the control RPCs. This is that sweep, for the one control RPC where the wait is self-defeating.Note this is not fixed by adding
session.interruptto_LONG_HANDLERS. That would unblock the reader thread but leave the 30s delay and the dropped_turn_cancel_requestedexactly as they are.Scope
Deliberately narrow:
session.interrupt's in-process branch only. Nothing in that branch needs a built agent — its single agent use,request_hard_interrupt(session["agent"]), is now guarded, becauseagent is Nonewhile the build is in flight becomes newly reachable once we stop waiting for it. When the agent already exists,_sess_buildingreturns immediately and behaviour is byte-identical.Other handlers share the "needs the session record, not the agent" shape (
approval.*,rollback.list/rollback.diff,process.kill/process.list). They are not included here: they are a different concern from Stop must not leave the session stuck, and they belong in their own change.Related Issue
No issue is filed for this — an issue search across four phrasings returned nothing. Supersedes #71825.
Type of Change
Changes Made
tui_gateway/host_supervisor.py— addHostSupervisor.has_pending_turn(sid)._pending_turnsalready existed but had no accessor; the new method reads it under the sameself._lockthat guards every mutation of that dict (submit_turn,_complete_turn,_fail_pending_turns).tui_gateway/methods_session.py—session.interrupt's compute-host branch no longer returns early on interrupt failure. It records the inflight snapshot, logs the failure, falls through to the shared recovery, and force-clears the stuck busy flag under four guards (below). Stop still returns{"status": "interrupted", "turn_isolation": True}— from the user's point of view the turn is over either way.tests/test_tui_gateway_server.py— four regression cases ported from fix(tui_gateway): stop dead compute-host Stop from leaving the session forever queued #71825, plus one direct unit test for the new probe. Placed beside the existing compute-host cluster (test_prompt_submit_dispatches_to_compute_host_when_turn_isolation_enabled,..._fails_open_inline_when_compute_host_dispatch_breaks,test_compute_host_turn_end_updates_metadata_mirror) and reusing itsmonkeypatch.setattr(server, "_get_compute_host_supervisor", ...)scaffolding.For the in-process branch:
tui_gateway/methods_session.py—session.interrupt's in-process branch resolves through_sess_buildingrather than_sess, and guards its single agent use for the now-reachableagent is Nonestate.tests/test_tui_gateway_server.py—test_interrupt_before_agent_ready_prevents_late_turn_startandtest_cancelled_turn_before_agent_ready_emits_error_eventstop monkeypatching_wait_agentand now run against a real, never-setagent_ready, so they can actually fail; plus two new cases,test_interrupt_never_waits_on_the_deferred_agent_buildandtest_interrupt_recovers_a_session_whose_agent_build_failed.The force-clear cannot clobber a live turn
All four must hold:
host_unreachablenot pending_for_sidsession.get("running")inflight_now is inflight_at_interrupt(and notNone)prompt.submit/ drain replaced the inflight turn and ownsrunningnowThe probe fails closed: an exception from
has_pending_turnsetspending_for_sid = True, so an unreadable supervisor leaves teardown to the waiter rather than guessing.The probe is issued before
history_lockis taken. That honours the invariant stated at the other supervisor-interrupt call site: "never call a provider or compute-host method while holding history_lock: an interrupt can wait behind the very operation it is trying to cancel." The guards are then re-evaluated inside the lock, which is what closes the race the probe cannot.Sibling sweep — resolved in both directions
Two supervisor-interrupt call sites exist in the corpus (
grep -rn '\.interrupt(' tui_gateway/ hermes_cli/ apps/), plus the other two_session_uses_compute_hostbranches that return_erron host failure:methods_session.py,session.interruptserver.py,_interrupt_busy_sessionmethods_session.py,session.compress_erron host failure, but does not own therunningbusy flag, so it cannot strand the session.methods_session.py,session.saverunningownership._interrupt_busy_sessionswallows the same failure with a bareexcept Exception: pass, so it looks like the same bug from the other direction. It is deliberately not changed:_enqueue_prompt(session, text, transport)underhistory_lockand returns_ok(rid, {"status": "queued"}); teardown there is owned by_drain_queued_prompt, not by the interrupt.finallyonly resets_busy_interrupt_pending. There is norid/ RPC context in that thread to report a recovery from.runningfrom it could clobber a live successor turn —_drain_queued_promptclaimsrunning = Trueunder the lock, and by the time the daemon thread runs the drain may already have started the queued turn.session.interruptis the opposite case in every respect: an explicit user Stop, on the RPC thread, with anrid, where ending the turn is the intent.Related work that is not superseded
#45001 (
fix(desktop+gateway): make "session busy" lock recoverable — AbortSignal + server watchdog) is complementary, not overlapping. It adds a time-based stall watchdog that force-clearsrunning+inflight_turnaftersession.inflight_watchdog_seconds(default 120s) of silence. It is strictly weaker on this path and does not replace this fix:running/inflight_turn, so_turn_cancel_requested,queued_prompt = None,_clear_pending(sid)and the approval resolution are still skipped by the dead-host early return;Its hunks (
server.py@3648/3663/5311/5537) are pre-refactor and never touch the compute-host branch. Both changes can land independently.How to Test
Compute-host half:
In-process half:
Red-before / green-after, measured rather than predicted. Each baseline was restored with
git checkout <parent-commit> -- <path>— notgit stash, which is a no-op on an already-committed file and yields a silent false pass:running5019returned,runningstaysTruerunninghas_pending_turnprobe semanticsAttributeErrortest_interrupt_before_agent_ready_prevents_late_turn_start5032 agent initialization timed outtest_cancelled_turn_before_agent_ready_emits_error_event5032 agent initialization timed outtest_interrupt_never_waits_on_the_deferred_agent_build_wait_agentcalledtest_interrupt_recovers_a_session_whose_agent_build_failed5032 provider metadata fetch failedThe two end-to-end interrupt cases each spend a real 30 seconds before failing against the reverted file (60.79s for the pair) and pass in under a second with the fix — the wait is the defect, so the clock is part of the evidence. The two new cases fail in 0.43s, because they assert the invariant directly instead of one of its symptoms.
test_session_not_running_before_agent_ready_emits_error_eventcarries the same_wait_agentstub but is deliberately left untouched: it never callssession.interrupt(it clearsrunningdirectly), so the stub is unreachable in it. It stays green on both sides of the change, which makes it the control for the two that flip.Full file: 580 passed.
tests/tui_gateway/: 472 passed.ruff checkclean on both touched files.Manual reproduction, compute-host half: enable
dashboard.turn_isolation, start a turn, kill thepython -m tui_gateway.compute_hostchild, press Stop. Before:compute-host interrupt failed, and every subsequent message is silently queued forever. After: Stop succeeds, the composer is usable again, and the failure is logged.Manual reproduction, in-process half: leave
turn_isolationat its default, start a fresh session so the deferred build is still running (a cold MCP discovery makes this easy to hit), send the first message, and press Stop immediately. Before: the UI is unresponsive for up to 30 seconds, Stop reportsagent initialization timed out, and the message you cancelled is then sent anyway once the build lands. After: Stop returns immediately and the turn never starts.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AContract Protected
Invariant: a compute-host
session.interruptnever leavessession["running"]set when no host completion can ever clear it.HostSupervisor.interrupt()raises (dead child, respawn exhausted, broken pipe) whilerunning is True— previously returned5019and stranded the session.start()/_send_frame()is covered without a code change.runninguntouched; a failed probe fails closed to the same outcome.Second invariant (in-process branch):
session.interruptnever blocks on the deferred agent build, and records_turn_cancel_requestedregardless of the build's state.agent_ready(build in flight), which cost up to 30 seconds and then dropped the cancel flag entirely on timeout; and a setagent_readywithagent_errorpopulated (build failed), which dropped it immediately.test_interrupt_never_waits_on_the_deferred_agent_buildasserts_wait_agentis not called, which holds independently of whatever_wait_agentitself later does._sess_buildingreturns immediately and this path is byte-identical to before — covered by the existing in-process interrupt tests, which are unchanged and still green.