fix(gateway): emit error event when a turn is cancelled before agent ready - #65567
fix(gateway): emit error event when a turn is cancelled before agent ready#65567jinglun010-cpu wants to merge 1 commit into
Conversation
…ready
run_after_agent_ready() silently returned when _turn_cancel_requested or
running=False was set during lazy agent startup, leaving the Desktop with
a {"status":"streaming"} reply that never produced a message.start or
error event. The _wait_agent error branch 6 lines above already emits;
this mirrors it so the client can surface feedback instead of hanging.
This is the server-side half of NousResearch#63078 — the client-side drift guard is
addressed by NousResearch#64327, but even with that fix the server-side cancel race
still silently dropped the turn.
Adds two regression tests that capture _emit (the existing sibling test
mocked it to a no-op, so it could not catch the silent drop).
Closes NousResearch#63078
alt-glitch
added
type/bug
|
Thanks for isolating the deferred-start cancellation path. Current The proposed This is an automated hermes-sweeper review. |
teknium1
added
sweeper:risk-message-delivery
|
suggesting changes Please keep an explicit Split the two conditions: the unexpected Security evidence:
Review setup: I reviewed a run-owned local rebase or patch replay against current GitHub Signed: GPT-5.6-sol-xhigh in Codex |
alt-glitch
added
the
needs-decision
… outlives 30s (#63078) Leg 2 of #63078: prompt.submit returns {"status":"streaming"} immediately and runs _start_agent_build + _wait_agent(timeout=30s) behind it. The deferred build (MCP discovery with per-server retry backoff, synchronous model-metadata HTTP, skills scanning) routinely outlives 30s on cold starts; on timeout run_after_agent_ready emitted an error EVENT and returned without ever calling _run_prompt_submit — the user's first message was permanently discarded while the build finished successfully in the background. The desktop's optimistic row eventually cleared with no visible error: the blank first session. New _wait_agent_for_prompt replaces the flat cliff for the deferred prompt path only (_sess()'s RPC-blocking _wait_agent keeps its 30s contract): - The pending prompt stays attached to the (already off-RPC) run thread and is delivered the moment the still-running build completes — a slow build is no longer message loss. - The wait runs in 5s slices so a cancel (session.interrupt / churn) is honored promptly; the cancelled path returns None and defers to the caller's cancel branch (the #65567 emit) for user-visible messaging. - Past 30s the client gets ONE keyed notification.show ('Still starting the agent…', key=agent-build-slow, desktop toast / TUI status bar), cleared on delivery — patient, never silent. - Permanent failure only when the build itself fails: agent_error set at ready, the build thread died without signalling ready (fail fast via the new _agent_build_thread handle instead of sitting out the cap on a corpse), or the bounded cap expired on a genuinely hung build. The cap defaults to 600s and is tunable via agent.build_wait_timeout in config.yaml (no new env vars); the error message states the message was not sent. Tests: slow-build delivery with zero error events; the keyed progress notice shown once and cleared; build-failure surfacing exactly one error event with the real reason; dead-thread fail-fast; cancel honored mid-wait; config override + fallback semantics; cap expiry message. The compute-host fallback test stubs the new waiter alongside _wait_agent.
…message survival (#63078) Client half of leg 2. The issue's failure mode was the desktop clearing the optimistic first message and showing nothing when the backend dropped the turn. With the gateway now preserving the message across slow builds and emitting a real error event on genuine failure, these tests pin the desktop contract that makes that visible: - an agent-init error event renders an in-transcript assistant error bubble, keeps the user's optimistic first message (never silently cleared), fires the global error toast, and releases busy/awaitingResponse so the composer is usable again; - the #65567 pre-ready cancel emit renders the same way. Covers failAssistantMessage + the gateway-event error branch, which no test exercised at the session-state level (todo-cleanup only asserted todo eviction).
SummaryThree PRs address or reference #63078: #65567 and #66545 expose the deferred-start cancellation/not-running silent return as an error, while merged #71140 records the broader first-message fix by extending the agent-build wait and surfacing initialization failures. The two open diffs make a dropped turn visible but retain generic-error behavior for an intentional Stop and do not themselves replace the timeout-driven discard path. Related pull requests
Duplicates#66545 is substantively a duplicate of #65567; merged #71140 records incorporation of #65567's server-side path, giving the consolidation chain #66545 → #65567 → #71140. Suggested consolidationClose #66545 as duplicate of #65567, then close #65567 as duplicate of merged #71140. Despite the keep_open reviews and #65567's earlier best-focused-fix designation, #71140 subsequently merged with the broader build-wait fix and explicitly incorporated #65567's path; moreover, the contributor's suggesting-changes review shows that both open diffs should not land unchanged because they represent an intentional Stop as a generic error, a concern that may warrant a separate follow-up against the merged implementation. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I63078(["issue #63078 (closed)"])
subgraph Dup65567 ["PRs duplicating each other"]
P65567["PR #65567 (open)"]
P66545["PR #66545 (open)"]
end
P65567 -->|best fix| I63078
class I63078 closed
class P65567 open
class P66545 open
class P65567 best
class P65567 target
click I63078 "https://github.com/NousResearch/hermes-agent/issues/63078"
click P65567 "https://github.com/NousResearch/hermes-agent/pull/65567"
click P66545 "https://github.com/NousResearch/hermes-agent/pull/66545"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 3 pull requests and 1 issue in this complex. Diffs were read for 2 of 3 PRs (rest unavailable); Assessment working set: 11 kB of PR diffs, 13 kB of issue/PR text, 13 kB of discussion (15 comments), 5 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
alt-glitch
added
duplicate
Duplicate of merged #71140, which incorporated the pre-ready server-side visibility path as part of the broader first-message build-wait fix. This branch also reports an intentional Stop as a generic error, so any remaining cancellation-semantics work should be a focused follow-up against current main. |
… outlives 30s (NousResearch#63078) Leg 2 of NousResearch#63078: prompt.submit returns {"status":"streaming"} immediately and runs _start_agent_build + _wait_agent(timeout=30s) behind it. The deferred build (MCP discovery with per-server retry backoff, synchronous model-metadata HTTP, skills scanning) routinely outlives 30s on cold starts; on timeout run_after_agent_ready emitted an error EVENT and returned without ever calling _run_prompt_submit — the user's first message was permanently discarded while the build finished successfully in the background. The desktop's optimistic row eventually cleared with no visible error: the blank first session. New _wait_agent_for_prompt replaces the flat cliff for the deferred prompt path only (_sess()'s RPC-blocking _wait_agent keeps its 30s contract): - The pending prompt stays attached to the (already off-RPC) run thread and is delivered the moment the still-running build completes — a slow build is no longer message loss. - The wait runs in 5s slices so a cancel (session.interrupt / churn) is honored promptly; the cancelled path returns None and defers to the caller's cancel branch (the NousResearch#65567 emit) for user-visible messaging. - Past 30s the client gets ONE keyed notification.show ('Still starting the agent…', key=agent-build-slow, desktop toast / TUI status bar), cleared on delivery — patient, never silent. - Permanent failure only when the build itself fails: agent_error set at ready, the build thread died without signalling ready (fail fast via the new _agent_build_thread handle instead of sitting out the cap on a corpse), or the bounded cap expired on a genuinely hung build. The cap defaults to 600s and is tunable via agent.build_wait_timeout in config.yaml (no new env vars); the error message states the message was not sent. Tests: slow-build delivery with zero error events; the keyed progress notice shown once and cleared; build-failure surfacing exactly one error event with the real reason; dead-thread fail-fast; cancel honored mid-wait; config override + fallback semantics; cap expiry message. The compute-host fallback test stubs the new waiter alongside _wait_agent.
…message survival (NousResearch#63078) Client half of leg 2. The issue's failure mode was the desktop clearing the optimistic first message and showing nothing when the backend dropped the turn. With the gateway now preserving the message across slow builds and emitting a real error event on genuine failure, these tests pin the desktop contract that makes that visible: - an agent-init error event renders an in-transcript assistant error bubble, keeps the user's optimistic first message (never silently cleared), fires the global error toast, and releases busy/awaitingResponse so the composer is usable again; - the NousResearch#65567 pre-ready cancel emit renders the same way. Covers failAssistantMessage + the gateway-event error branch, which no test exercised at the session-state level (todo-cleanup only asserted todo eviction).
5 participants
Summary
When a turn is cancelled during lazy agent startup (e.g. the user hits Stop, or a concurrent
session.createrace clearsrunning), the deferred run thread inrun_after_agent_ready()bails silently — it clears the inflight turn and returns without emitting any event. The Desktop seesprompt.submitreturn{"status": "streaming"}but never receives amessage.startorerrorevent, so the composer shows no feedback and the user's message appears to vanish.This is the server-side half of #63078. The existing PRs targeting that issue (#62805, #63045, #64327) all fix the client-side drift guard that self-aborts the first message of a new chat. But even with the drift guard fixed, the server-side silent-return path here remains: any turn cancelled between
_start_agent_buildand_run_prompt_submitdisappears without a trace. @Kenmege called this out explicitly in #63078 (comment): "The server-side_wait_agent30 s ceiling described there still deserves its own fix."The sibling test
test_interrupt_before_agent_ready_prevents_late_turn_startalready asserts_run_prompt_submitis skipped on this path, but it mocks_emitto a no-op — so it cannot catch the silent drop. This PR adds the missing emit and two regression tests that capture_emitto assert the client receives anerrorevent.Changes
tui_gateway/server.pyrun_after_agent_ready(), emit anerrorevent before the silent return when_turn_cancel_requestedornot running. Message distinguishes the two causes. Mirrors the_wait_agenterror branch 6 lines above, which already emits.tests/test_tui_gateway_server.pytest_cancelled_turn_before_agent_ready_emits_error_event(interrupt path) andtest_session_not_running_before_agent_ready_emits_error_event(concurrent-clear path). Both assert_emitreceived anerrorevent addressed to the session.+15 / -1 in source, +144 / -1 in tests. 2 files.
Test Plan
test_cancelled_turn_before_agent_ready_emits_error_event— interrupt during agent build → error event emitted with "cancelled" in messagetest_session_not_running_before_agent_ready_emits_error_event— concurrentrunning=False→ error event emitted with "no longer running" in messagetest_interrupt_before_agent_ready_prevents_late_turn_start— existing test still passes (no regression)expected one error event, got: []→ restored fix → all greenCloses #63078