fix(desktop): prevent false runtime-not-ready under gateway load - #66174
Conversation
Related to #65153 and #65388. The live branches now have a documented ownership split: this PR owns Desktop readiness-state polling plus |
|
Thanks for flagging this. I compared the actual diffs in #65388 and the runtime-probe work described by #65151 / #65153. Proposed ownership split:
The Desktop implementations are not quite equivalent: #66174 independently settles REST status and readiness with The backend delta here is also unique to the live reproduction: while the warning was visible, All required CI on #66174 is green. I am happy to coordinate with @yingliang-zhang and rebase/drop any truly duplicate slice once the ownership split is agreed; the goal is one canonical implementation, not competing readiness patches. |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Desktop fix preventing false runtime-not-ready detection under gateway load. The diff looks like a state management fix in the React renderer. No security concerns.
Reviewed by Hermes Agent
|
Thanks @c3llus — I agree with the ownership split. I've narrowed #65388 to its non-overlapping pieces only: One behavior from the former #65388 polling tests is worth preserving here: when Otherwise, the #65388 narrowed head: |
008474f to
8f15a54
Compare
|
Thanks @yingliang-zhang — implemented the disconnect-state regression you described in 8f15a5408. The hook now clears I also rebased #66174 onto current |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused readiness and reader-path fix. The current-main premise is confirmed: apps/desktop/src/app/shell/hooks/use-status-snapshot.ts:20-44 couples the legs and permits overlapping polls, while tui_gateway/server.py:1451-1453 still dispatches session.active_list inline because it is absent from _LONG_HANDLERS.
Problems
scripts/release.py:45adds a new email to the legacy map. Current main marks that map frozen atscripts/release.py:46-50;contributors/README.md:1-24requires new mappings to be one-file-per-email entries undercontributors/emails/. This portion will conflict on salvage and is no longer the supported attribution mechanism.
Suggested changes
- Replace that edit with
contributors/emails/theunathi@gmail.comcontainingUnathiCodexas its first non-comment line.
Automated hermes-sweeper review.
| @@ -45,6 +45,7 @@ | |||
|
|
|||
There was a problem hiding this comment.
Current main freezes the legacy map and requires new mappings under contributors/emails/ (contributors/README.md). Please replace this with contributors/emails/theunathi@gmail.com containing UnathiCodex; otherwise this will conflict on salvage and bypass the conflict-free mapping mechanism.
8f15a54 to
2fd41cb
Compare
2fd41cb to
48f01e2
Compare
austinpickett
left a comment
There was a problem hiding this comment.
Review
Verified locally on Windows 11 (worktree at PR head 48f01e2ef, plus a trial merge with current main).
What I checked
Premise verification — traced the failure mode against the actual code:
- On the merge-base,
use-status-snapshot.tsusedsetInterval+ a singlePromise.allwith a.catchthat converted any readiness rejection into{ready: false, source: 'fallback'}and published it unconditionally. So a queued/timed-out RPC really did surface as "inference not ready", andsetIntervalreally did allow overlapping refreshes where a stale failure could land after a newer healthy result. Premise holds. session.active_listwas indeed absent from_LONG_HANDLERSintui_gateway/server.py, so it ran inline on the WS reader thread — consistent with the reported 23s stall blocking readiness RPCs queued behind it on the same socket. Adding it follows the exact precedent of thesetup.runtime_check/setup.statusentries (#50005 family) directly above it.
Design of the fix:
Promise.allSettled+ independent settling is right: a hunggetStatus()no longer discards a good readiness result and vice versa.- The
source !== 'fallback'gate is the correct authoritative/transient discriminator —runtime-readiness.tsonly returnssource: 'fallback'when neither RPC produced a boolean, so suppressing it preserves the last authoritative state without masking a realok: false(which arrives assource: 'runtime_check'and is still published immediately, covered by a test). - Chained
setTimeoutscheduled infinallygives single-flight polling with no overlap and no dropped schedule on error paths; cleanup handles both the interval→timeout switch and thecancelledflag. - Clearing readiness when
gatewayState !== 'open'closes the complementary staleness hole (stale "ready" surviving a disconnect) — good catch to handle both directions.
Tests: the 5 new hook tests cover transient failure retention, initial-failure → null (no false negative on first paint), authoritative failure propagation, disconnect clearing, and non-overlap of slow refreshes. These are behavior contracts, not change-detectors. The GIL-starvation matrix addition means the _LONG_HANDLERS entry is regression-guarded, not just asserted once.
Local results (Windows 11)
npx vitest run src/app/shell/hooks/use-status-snapshot.test.ts src/lib/runtime-readiness.test.ts— 11 passednpm run typecheck(apps/desktop, both tsconfigs) — cleanpytest tests/tui_gateway/test_inline_rpc_gil_starvation.py— 9 passed- Trial merge with current
main— clean auto-merge, no semantic conflict with the recent_reset_session_agentchanges intui_gateway/server.py
CI is fully green and the branch merges cleanly. Nice, tightly-scoped fix that addresses both sides of the failure mode (frontend polling semantics + backend reader-thread starvation) with a clear line-level account of each. LGTM — approving.
…sResearch#66174) * fix(desktop): stabilize runtime readiness polling * fix(tui_gateway): pool live-session status polling * fix(desktop): clear readiness when gateway disconnects
…sResearch#66174) * fix(desktop): stabilize runtime readiness polling * fix(tui_gateway): pool live-session status polling * fix(desktop): clear readiness when gateway disconnects
…sResearch#66174) * fix(desktop): stabilize runtime readiness polling * fix(tui_gateway): pool live-session status polling * fix(desktop): clear readiness when gateway disconnects
What does this PR do?
Hermes Desktop polls REST status and gateway runtime readiness every 15 seconds. Under heavy concurrent agent load,
session.active_listcould occupy the shared WebSocket reader for tens of seconds. Readiness requests queued behind it could then time out, and the Desktop hook published the transport fallback (ready: false) as if it were an authoritative credential failure. The interval could also start overlapping refreshes, allowing a stale failure to race a newer healthy result.This PR fixes both sides of that failure mode:
ok: falseis still surfaced immediately.session.active_listruns through the gateway worker pool so a slow live-session rehydrate cannot block readiness, prompts, or interrupts on the same socket.Related Issue
No linked issue. Reproduced against a live remote Desktop/backend connection under concurrent agent load.
Type of Change
Changes Made
apps/desktop/src/app/shell/hooks/use-status-snapshot.tsto use non-overlapping, independently settled polling.apps/desktop/src/app/shell/hooks/use-status-snapshot.test.tscovering transient failures, real credential failures, initial unknown state, and overlapping polls.session.active_listtotui_gateway.server._LONG_HANDLERS.How to Test
Automated checks run:
npm test -- src/app/shell/hooks/use-status-snapshot.test.ts src/lib/runtime-readiness.test.ts— 10 passednpm run typecheck— passedscripts/run_tests.sh tests/tui_gateway/test_inline_rpc_gil_starvation.py— 9 passed on UbuntuChecklist
Code
Documentation & Housekeeping
Screenshots / Logs
During the live failure,
setup.runtime_checkstill returnedok: truefor the configured provider whilesession.active_listtook 23.2 seconds on the same connection. After applying both fixes, live-session polling completed in 209 ms and authoritative readiness returnedok: truein 337 ms.