fix(tui_gateway): control RPCs no longer wait on the deferred agent build - #87429
fix(tui_gateway): control RPCs no longer wait on the deferred agent build#87429briandevans wants to merge 5 commits into
Conversation
…ferred build approval.pending, approval.received and approval.respond need the session RECORD and not the agent: all three reach tools.approval's module-global _gateway_queues, which is keyed by session_key alone and has no relationship to the session's agent_ready event. _sess's _wait_agent therefore bought nothing at these three sites and charged up to 30 seconds for it — the same argument _sess_building's own docstring makes for the attach RPCs. The charge landed in a pointed place. None of the three is in _LONG_HANDLERS, so the wait ran inline on the socket reader thread and stalled every RPC queued behind it — and _LONG_HANDLERS' own comment names approval.respond verbatim as the RPC the pool exists to keep unblocked. The pool protects approval.respond from slow billing handlers while approval.respond blocked itself on the agent build. The wait is on the common path rather than a corner: the desktop replays pending approvals on gateway.ready and session.info, i.e. exactly while a cold resume's deferred build is still warming. Worse, _start_agent_build deliberately early-returns for a lazy watch session spectating an in-flight child, so agent_ready stays unset for that child's whole run and these RPCs did not merely stall — they ran out the full timeout and returned a hard 5032 "agent initialization timed out", repeatedly. The six sibling *.respond RPCs in this module route through _respond() and never call _sess at all, so approval.respond was already the odd one out. _sess_building still kicks the build off; it just stops blocking on it.
…ilding Covers the three approval handlers against a session record whose deferred build has not finished: agent_ready is a threading.Event that is deliberately never set, and _start_agent_build is monkeypatched to a no-op, so any wait on the build is a real stall rather than a race. The seam mirrors the one already on main in test_attach_does_not_wait_for_agent.py. These assert behaviour, not timing. approval.pending must replay the queued approval, approval.received must actually flip the entry's acknowledged flag, and approval.respond must resolve the entry AND set its event so the blocked agent thread is genuinely released. Each then asserts agent_ready is still unset, which is what makes the result a fix rather than a coincidence. Two guards keep the change honest in the other direction: an unknown session_id must still return 4001 at every one of the three handlers, since _sess_building delegates to _sess_nowait and validation was never the wait, and approval.received must still reject a missing request_id with 4006. tools.approval exposes no public enqueue helper — the only producer is _await_gateway_decision on the agent thread — so the fixture mirrors its three lines under the module lock and pops the queue again on teardown. Verified against the unfixed resolver: the three behavioural tests and the 4006 guard each stall the full 30s _wait_agent cap and fail with 5032. The 4001 guards pass either way by design; they short-circuit before the wait.
…d build process.kill's ownership check reads session_key off the session record and compares it against the registry's ProcessSession.session_key; nothing on the path touches session["agent"]. The agent wait was therefore pure cost. This handler is not in _LONG_HANDLERS, so the wait ran inline on the socket reader thread. That is the worst placement available for this particular RPC: the user has already pressed Stop, and the handler that is supposed to act on it instead sat for up to 30 seconds holding the reader thread, blocking every RPC queued behind it on the same socket — with no spinner to explain it. On a lazy watch session spectating an in-flight child, _start_agent_build early-returns and agent_ready is never set at all, so the call did not just arrive late: it returned 5032 "agent initialization timed out" and the process was never killed. The scoping rule that makes this handler safe was not even consulted, because the wait fired ahead of it.
…d build _session_processes reads session_key off the record and matches it against the global process registry; it never dereferences session["agent"]. Same root cause as the sibling sites, so the same resolver applies. The symptom differs, which is why it is a separate change. process.list IS in _LONG_HANDLERS, so it runs on the RPC pool and never held the socket reader thread — there is no queue-behind stall here. What it cost instead is the desktop status stack: the pane that lists a session's background processes resolved empty for up to 30 seconds after a cold resume, or failed outright with 5032 on a lazy watch session whose build never starts, so a running dev server or preview looked like it had gone away.
…lding Extends the same building-session seam to process.list and process.kill, and closes the sweep with a resolver-level control. process.list seeds two entries in the global process registry — one owned by this session, one owned by another — and asserts the handler returns only the caller's while agent_ready is still unset, so the fix cannot be mistaken for having widened the session scoping. process.kill asserts 4044, not 5032. That is the discriminator for the whole change: before the fix the agent wait fired first and every one of these calls came back "agent initialization timed out" with the ownership rule never consulted at all. The missing-process_id guard likewise still returns 4012. The last test is the boundary check. It monkeypatches _wait_agent to record its calls, drives all five converted RPCs and asserts none of them waits, then drives rollback.list and asserts it still does — and that it fails on the _checkpoint_mgr dereference. That failure is precisely why rollback.list, rollback.diff and rollback.restore are excluded from this sweep: they resolve through _with_checkpoints, which reads session["agent"]._checkpoint_mgr, so converting them would fault mid-build instead of merely stalling. If either half of that assertion ever flips, the boundary this sweep draws has gone stale and the test says so. Verified against the unfixed resolver: process.list and the three process.kill tests each stall the full 30s cap and fail with 5032; the resolver control fails immediately with five recorded waits instead of none.
There was a problem hiding this comment.
Pull request overview
This PR fixes a TUI gateway responsiveness bug where several “control” JSON-RPC handlers unnecessarily waited for the deferred agent build (up to the 30s _wait_agent cap), causing stalls/timeouts for actions that only require the session record (not the agent). It aligns these handlers with the existing _sess_building pattern introduced for attach RPCs, ensuring approval/process control calls stay responsive during cold starts and lazy-watch sessions.
Changes:
- Switch
approval.pending,approval.received, andapproval.respondto resolve sessions via_sess_building(skip_wait_agent). - Switch
process.listandprocess.killto resolve sessions via_sess_building(skip_wait_agent), preventing reader-thread stalls and empty status behavior during cold resumes. - Add a focused regression test suite asserting these control RPCs complete and perform their work while
agent_readyremains unset, and that rollback RPCs remain correctly bounded to_sess(still waiting / still failing mid-build as designed).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tui_gateway/methods_tools.py | Updates process.list / process.kill to use _sess_building so control tooling doesn’t block on agent startup. |
| tui_gateway/methods_prompt.py | Updates approval RPCs to use _sess_building, avoiding unnecessary waits on deferred builds. |
| tests/tui_gateway/test_control_rpcs_do_not_wait_for_agent.py | Adds regression tests proving the converted RPCs don’t call _wait_agent and still do real work mid-build, while rollback remains intentionally gated. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
fix(tui_gateway): control RPCs no longer wait on the deferred agent build
The resolver-level sweep test ( |
alt-glitch
added
type/bug
What does this PR do?
_LONG_HANDLERSexists, in its own words, to "keep [slow handlers] off the main stdin loop so a slow portal can't stallapproval.respond/session.interrupt/ other RPCs." The repo pools the billing RPCs specifically to protectapproval.respond— and thenapproval.respondblocked itself for up to 30 seconds on the deferred agent build.This finishes the
_sess_buildingconversion across the five control RPCs that need the session record and never the agent._sess()is exactly_sess_building()+_wait_agent(timeout=30.0), and_wait_agentblocks onagent_ready, athreading.Eventthat stays unset until the deferred build completes (MCP discovery, model metadata, skills scan — "routinely tens of seconds on a cold start", per_sess_building's own docstring).At all five sites that wait bought nothing:
approval.pending/approval.received/approval.respondreachtools.approval's module-global_gateway_queues, keyed bysession_keyalone. It has no relationship to the tui_gateway session record or itsagent_readyevent.process.list/process.killmatchsession_keyagainst the global process registry (_session_processes,process_registry).None of the five dereferences
session["agent"].Four of the five are not in
_LONG_HANDLERS, so the wait ran inline on the socket reader thread and stalled every RPC queued behind it on the same socket — the precise failure mode_sess_buildingwas introduced to fix for the attach RPCs.And this is a hard failure, not just latency.
_start_agent_builddeliberately early-returns for a lazy watch session spectating an in-flight child:agent_readyis therefore never set for that child's entire run, so these RPCs ran out the full cap and returned5032 "agent initialization timed out"— repeatedly. Aprocess.killthe user asked for simply did not happen, and the ownership check that makes it safe was never even consulted.The path is the common one rather than a corner case: the desktop replays pending approvals on
gateway.readyandsession.info(approvalReplaySessionId), i.e. on reconnect and session attach — exactly when a cold resume's deferred build is in flight.get_pending_gateway_approval's docstring says the same thing: "Reconnectable clients use this to restore an approval prompt…"Corroborating that
approval.respondwas the odd one out: the six sibling*.respondRPCs inmethods_prompt.py(clarify.respond,sudo.respond,secret.respond, …) all route through_respond()and never call_sessat all.Sites deliberately NOT converted
This is the complete set for this root cause, and the exclusions are load-bearing:
rollback.list/rollback.diff/rollback.restore_with_checkpoints, which dereferencessession["agent"]._checkpoint_mgr. Converting these would fault mid-build instead of merely stalling.prompt.background,preview.restart,session.undo/compress/save/branchsession.interruptThe last test in this PR pins that boundary at runtime: it asserts the five converted RPCs skip
_wait_agentand thatrollback.liststill takes it and still fails on the_checkpoint_mgrdereference. If either half flips, the exclusion has gone stale and the suite says so.Related Issue
N/A — no filed issue; this completes the sweep merged in #86302, which converted the attach RPCs.
Type of Change
Changes Made
tui_gateway/methods_prompt.py—approval.pending,approval.received,approval.respondresolve via_sess_building; one comment above the trio explains why.tui_gateway/methods_tools.py—process.kill(inline: reader-thread stall) andprocess.list(pooled: empty desktop status stack) resolve via_sess_building; rationale in each docstring.tests/tui_gateway/test_control_rpcs_do_not_wait_for_agent.py— new file, 12 tests. Deliberately a new file rather than an append to the 15k-linetests/test_tui_gateway_server.py, mirroring main's own layout for this contract.Five commits: the approval sites, their tests,
process.kill,process.list, and the process tests + boundary control.process.killandprocess.listare separate commits because the defect they produce is different — a reader-thread stall versus a silently empty status pane.How to Test
Red before / green after — verified, not asserted. The new test file was applied alone on top of unmodified
mainin a clean worktree:Every converted site stalled the full cap, one test per site:
test_approval_pending_replays_the_queue_while_the_agent_is_buildingtest_approval_received_acknowledges_while_the_agent_is_buildingtest_approval_respond_resolves_while_the_agent_is_buildingtest_process_list_reports_this_sessions_processes_while_buildingtest_process_kill_still_refuses_another_sessions_process_while_buildingtest_process_kill_still_reports_an_unknown_process_while_buildingtest_process_kill_still_requires_a_process_idtest_approval_received_still_requires_a_request_idtest_converted_control_rpcs_skip_the_agent_wait_and_rollback_still_takes_it_wait_agentcalls instead of 0The 3 that pass both before and after are the
4001unknown-session guards — by design, they short-circuit ahead of the wait, which is what proves_sess_buildingkeeps_sess_nowait's validation.After the fix: 12 passed in 0.21s — the same suite, with the stalls gone. Each of the five commits was also checked out individually and the touched suite run at each (
59–71 passed).The tests assert behaviour, not timing:
approval.pendingmust replay the queued approval,approval.receivedmust flipacknowledged,approval.respondmust resolve the entry and set its event so the blocked agent thread is released, andprocess.listmust return only the calling session's processes.Related / Positioning
Of the 81 open PRs that touch either file, #68375 ("feat(tui): detach running turns into background") is the only one that rewrites any of these five handlers: it replaces
approval.respond's body wholesale and removes the_sess(params, rid)line this PR changes. It is currentlyCONFLICTINGwith main and was last updated 2026-08-02. If it lands first, this PR'sapproval.respondline becomes moot at that one site only —approval.pending,approval.received,process.listandprocess.killare untouched by it. I keptapproval.respondin scope because it is the site_LONG_HANDLERS' own comment names verbatim, and it is trivially droppable if you would rather take #68375's version there.#75707 and #74376 carry the same line as diff context but do not modify it.
Adjacent prior art on the same family of problem (unblocking the RPC reader thread), neither overlapping: #67789 pool-routes
subprocess.runexec commands inserver.py'sdispatch; #41510 proposes pooling the attach handlers, the alternative remedy for the surface main already fixed with_sess_building.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/A