fix(tui): close busy-flag race that stuck queue-mode back-to-back sends - #180
fix(tui): close busy-flag race that stuck queue-mode back-to-back sends#180hashbender wants to merge 1 commit into
Conversation
|
Review Complete Files Reviewed: 3 By Severity:
Three findings in the TUI submission flow: a latent stale busy flag when session clears mid-submission, a test mock mismatch with the gateway's response type, and a redundant state call. One medium-severity bug, two low-severity improvements. Files Reviewed (3 files) |
There was a problem hiding this comment.
Risk: 🟡 Medium (38/100) — 1 medium finding, 2 low · 323 LOC across 3 files
Summary
This PR modifies submissionCore.ts to close an async race window between the busy-flag set and the detect_drop RPC. The fix is sound, but review found one real bug (the busy flag can leak permanently if the session is cleared during the RPC round-trip) and two low-severity maintenance items.
Findings
finding-001 (low) — Mock mismatch in submissionCore.test.ts
File: ui-tui/src/__tests__/submissionCore.test.ts:28 — The test mock resolves prompt.submit with { status: 'streaming' }, but the real PromptSubmitResponse type only has { ok?: boolean }. Currently harmless (the code only uses .catch()), but a latent false-positive risk for future code.
finding-002 (medium) — Stale busy flag on session clear
File: ui-tui/src/app/submissionCore.ts:59-61 — markSubmitting() sets busy=true synchronously, then the closure startSubmit() re-checks the session ID after an async detect_drop RPC resolves. If the session was cleared during that gap (e.g. user switches sessions), startSubmit bails early but never resets busy, leaving the UI permanently stuck in 'running…' with no self-recovery path.
finding-003 (low) — Redundant patchUiState call
File: ui-tui/src/app/submissionCore.ts:71 — The same busy=true state is set twice: once synchronously by markSubmitting() at line 54 (added to close the async gap), and again inside startSubmit at line 71 (a vestigial duplicate).
| } | ||
|
|
||
| // prompt.submit et al: resolve immediately with a success shape. | ||
| return Promise.resolve({ status: 'streaming' }) |
There was a problem hiding this comment.
🟢 Mock response shape for prompt.submit does not match real PromptSubmitResponse type (suggestion)
At submissionCore.test.ts line 28, the mock resolves request() calls with { status: 'streaming' }. The actual PromptSubmitResponse interface (gatewayTypes.ts:377) only has { ok?: boolean } — no 'status' field exists. While submissionCore.ts currently ignores the resolved value (only using .catch()), this is a latent maintenance risk: any future code reading the prompt.submit response would pass tests with a wrong shape, producing false-positive test results.
💡 Suggestion: Change the mock's resolved value to match the real type: Promise.resolve({ ok: true }). This makes the mock truthful to the actual gateway contract and prevents false positives if submissionCore.ts ever inspects the prompt.submit response.
| return Promise.resolve({ status: 'streaming' }) | |
| return Promise.resolve({ ok: true }) |
📋 Prompt for AI Agents
In ui-tui/src/tests/submissionCore.test.ts line 28, change Promise.resolve({ status: 'streaming' }) to Promise.resolve({ ok: true }) to match the PromptSubmitResponse type from gatewayTypes.ts (which only has ok?: boolean).
| if (!liveSid) { | ||
| return deps.sys('session not ready yet') | ||
| } |
There was a problem hiding this comment.
🟡 Busy flag leaks permanently if session is cleared between markSubmitting and startSubmit (bug)
In submissionCore.ts, submitPrompt() calls markSubmitting() at line 54, which sets uiState.busy=true synchronously to close the queue-mode race. After this, the function fires an async input.detect_drop RPC. The startSubmit closure, which only runs after the RPC resolves, re-checks the session ID at line 57-61. If the session disappeared during the RPC round-trip — for example, the user switched sessions via resumeById() which calls resetSession() and clears sid without touching busy — startSubmit returns early with 'session not ready yet' but never resets busy. This leaves the UI permanently stuck in the 'running…' state. The user cannot submit new messages (busy prevents the mainline send path) and there is no self-recovery mechanism; the user must reload or restart.
💡 Suggestion: Reset the busy flag in the startSubmit early-return path when the session is gone. Before returning at line 60, call patchUiState({ busy: false, status: 'ready' }) so the UI recovers when the session disappears mid-submission.
📋 Prompt for AI Agents
In ui-tui/src/app/submissionCore.ts, inside the startSubmit closure, change the early-return guard at lines 59-61 from:
if (!liveSid) {
return deps.sys('session not ready yet')
}to:
if (!liveSid) {
patchUiState({ busy: false, status: 'ready' })
return deps.sys('session not ready yet')
}This ensures that if submitPrompt's earlier markSubmitting() call set busy=true but the session was cleared before startSubmit executes (e.g. user switched sessions during the detect_drop RPC), the UI is not left permanently stuck in a busy state.
| deps.appendMessage({ role: 'user', text: displayText }) | ||
| } | ||
|
|
||
| patchUiState({ busy: true, status: 'running…' }) |
There was a problem hiding this comment.
🟢 Redundant patchUiState call in submission flow (style)
In submissionCore.ts, submitPrompt() calls markSubmitting() at line 54 (which sets busy=true, status='running…'), then later inside the startSubmit closure at line 71 calls patchUiState with the identical parameters. The synchronous marking at line 54 was added specifically to close an async gap before the detect_drop RPC; the duplicate call at line 71 inside startSubmit is unnecessary and violates DRY. The state is already set synchronously per the design intent documented in the comments at lines 23-38.
💡 Suggestion: Remove the redundant patchUiState({ busy: true, status: 'running…' }) call at line 71. The state is already set synchronously by markSubmitting() at line 54 before any async work begins.
📋 Prompt for AI Agents
In ui-tui/src/app/submissionCore.ts at line 71, remove the redundant patchUiState({ busy: true, status: 'running…' }) call inside the startSubmit closure. The state was already set synchronously by markSubmitting() at line 54, which was explicitly added to close the async-busy gap before the detect_drop RPC. Delete only line 71 and rejoin the surrounding lines so the file parses cleanly.
Summary
Closes the queue-mode busy-flag race that stuck the TUI on "Analyzing…" forever after two rapid back-to-back sends. Salvage of NousResearch#56046 by @benbarclay onto current
main.Root cause: the submit path set the session
busyflag asynchronously, only inside the.thenof aninput.detect_dropRPC.dispatchSubmissionroutes queue-vs-send ongetUiState().busy, so a second Enter pressed inside that RPC window readbusy === falseand raced a secondprompt.submitdown the send path. The gateway accepts a mid-turn submit as a success ({status:"queued"}, not an error), so the client'sisSessionBusyErrorre-queue path never fired and the message vanished from the local drain effect — UI stuck busy until Ctrl+C.Changes
ui-tui/src/app/submissionCore.ts(new): pure, testable submit core;markSubmitting()flips busy synchronously at the choke point, before thedetect_dropround-trip. LegacyisSessionBusyErrorre-queue kept as a defensive net.ui-tui/src/app/useSubmission.ts:sendnow funnels throughsubmitPrompt; dead imports removed.ui-tui/src/__tests__/submissionCore.test.ts(new): asserts busy flips synchronously before the RPC resolves.Validation
false(races a 2nd submit)true(local enqueue)markSubmitting()Infographic
Mirror-of: NousResearch#56100
NousResearch#56100