fix(tui): keep long slash commands (/hatch) alive with worker heartbeats - #99840
liuhao1024 wants to merge 1 commit into
Conversation
/hatch and other minutes-long slash commands were killed by the gateway's 45s slash-worker timeout while the worker subprocess kept running orphaned, and the TUI's blanket slash.exec fallback then retried command.dispatch, surfacing the misleading 'not a quick/plugin/bundle/skill command' error instead of the real timeout (NousResearch#99831). - slash_worker: heartbeat at half the configured slash timeout (capped 30s, HERMES_SLASH_HEARTBEAT_S overrides) while a command runs; protocol writes are lock-guarded against interleaving - server: treat a heartbeat as liveness in _SlashWorker.run() and restart the timeout window instead of failing - createSlashHandler: only retry command.dispatch when slash.exec did not fail with a worker-infrastructure error; real timeouts now surface verbatim
The fix correctly addresses the watchdog false-positive (#99831): the worker now emits The worker-side lifecycle is careful: Non-blocking: heartbeats prove the worker process's heartbeat thread is alive, not that the command is making progress — a worker whose main Verdict: LGTM |
|
Thanks for the review! The thread-vs-progress trade-off is acknowledged — a stall counter is a reasonable follow-up but beyond this fix's scope; the blocked-pipe lock risk is pre-existing, as noted. |
What does this PR do?
/hatchand other minutes-long TUI slash commands were killed by the gateway's 45s slash-worker timeout while the worker subprocess kept running orphaned, burning image credits with no way to deliver the result. On top of that, the TUI's blanketslash.execfallback retriedcommand.dispatchon any rejection, so the user saw the misleadingnot a quick/plugin/bundle/skill command: hatchinstead of the real timeout error (#99831).This PR implements the issue's minimal-mitigation direction:
tui_gateway/slash_worker.py): while a command executes, the worker emits{"id": rid, "heartbeat": true}lines at half the configured slash timeout (capped at 30s;HERMES_SLASH_HEARTBEAT_Soverrides). Protocol writes are guarded by a module lock so heartbeat and result lines can never interleave into corrupt JSON.tui_gateway/server.py):_SlashWorker.run()restarts its timeout window on a heartbeat instead of failing, so a live-but-slow command is never declared timed out. A wedged worker — one whose heartbeat thread is dead too — still hits the timeout, preserving the watchdog semantics.ui-tui/src/app/createSlashHandler.ts): worker-infrastructure failures (slash worker timed out,exited,closed pipe,start failed,failed) no longer retrycommand.dispatch; they surface verbatim. Route signals (use command.dispatch) and other failures still fall through to the dispatch retry, exactly as before.Related Issue
Fixes #99831
Type of Change
Changes Made
tui_gateway/slash_worker.py: added_resolve_heartbeat_s()+_emit_heartbeats()and a_stdout_lock; the command loop now runs the heartbeat thread while_run()executes and writes its result line under the lock.tui_gateway/server.py:_SlashWorker.run()skipsheartbeatmessages so they reset the wait window instead of trippingif not msg.get("ok").ui-tui/src/app/createSlashHandler.ts: theslash.execcatch only falls through tocommand.dispatchwhen the error is not a worker-infrastructure failure.tests/test_slash_worker_watchdog.py(heartbeat interval derivation, emitter stop semantics, broken-pipe quietness),tests/test_tui_gateway_server.py(heartbeat resets the window + stale-response skipping; silent worker still times out),ui-tui/src/__tests__/createSlashHandler.test.ts(timeout surfaces verbatim, dispatch retry must not fire).How to Test
pytest tests/test_slash_worker_watchdog.py -q— Observed result: 6 passed.pytest tests/test_tui_gateway_server.py -q -k "slash"— Observed result: 25 passed (incl. the 2 new heartbeat tests).pytest tests/tui_gateway/ -q— Observed result: 915 passed, 2 pre-existing order-dependent failures (test_gui_surface_toolsets.py::test_holds_exactly_the_gui_affordances,test_stale_provider_resume_live.py::test_renamed_provider_heals_to_new_identity) that reproduce identically on a cleanupstream/maincheckout of this worktree (verified via stash-diff baseline).cd ui-tui && ../node_modules/.bin/vitest run src/__tests__/createSlashHandler.test.ts— Observed result: 82 passed.hermes --tui, invoke/hatch <description>, and confirm the job no longer aborts at 45s with the misleading error; the worker keeps heartbeating and the result is delivered when generation completes.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/AScreenshots / Logs
Not applicable (backend behavior + unit tests; the issue's repro log lines are quoted in #99831).