From 8e4be34a27cd6255f67787ad350f46bf2a66d2e1 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 22 Aug 2026 13:23:37 -0700 Subject: [PATCH] Never auto-abort a healthy wait for the model's next token awaitingResponse flips true with no signal to tell "still coming" from "never coming" apart, both right after submit and the instant a tool batch resolves. Auto-abort now only fires on a stream that had already started producing tokens and then went dead mid-flight; the awaiting case still surfaces via the notice instead of killing the turn. Adds regression coverage for a healthy post-tool-batch wait and for live sub-agent progress under an outstanding task call, both of which must never auto-abort. --- docs/TUI.md | 8 ++ src/tui/stall-watchdog.test.ts | 166 ++++++++++++++++++++++----------- src/tui/stall-watchdog.ts | 24 +++++ src/tui/turn-monitor.test.ts | 82 +++++++++++++++- 4 files changed, 226 insertions(+), 54 deletions(-) diff --git a/docs/TUI.md b/docs/TUI.md index af7429032..717a1ac00 100644 --- a/docs/TUI.md +++ b/docs/TUI.md @@ -133,6 +133,14 @@ threshold, so a resumed session with already-stale activity shows the settled glyph immediately rather than alarming about silence the operator missed, and a stall that breaks and re-arms bursts again. +Auto-abort (`shouldAbortForStall`) is reserved for a stream that had already +started producing tokens and then went dead mid-flight — not for a run that +is merely *awaiting* the model's next response (right after submit, or the +instant a tool batch resolves and `awaitingResponse` flips back to true). +That wait has no signal to tell "still coming" from "never coming" apart, so +it is never auto-aborted no matter how long it runs; it still surfaces via +the notice, keeping the operator in control of whether to give up on it. + An idle session animates nothing at all: the monitor tick stops entirely rather than repainting an unchanging frame. diff --git a/src/tui/stall-watchdog.test.ts b/src/tui/stall-watchdog.test.ts index 7b0f007de..7d41fe2ac 100644 --- a/src/tui/stall-watchdog.test.ts +++ b/src/tui/stall-watchdog.test.ts @@ -14,28 +14,21 @@ import { } from "./stall-watchdog.js" describe("shouldAbortForStall", () => { + // Mid-stream hang: tokens already flowed, then everything went silent — + // the one shape auto-abort is willing to act on. The generic guards + // (status, threshold, exemptions) are exercised against this base. const base = { status: "running" as const, - awaitingResponse: true, + awaitingResponse: false, lastActivityAt: 0, nowMs: STALL_TIMEOUT_MS, stallTimeoutMs: STALL_TIMEOUT_MS, isProcessing: true, - streamingType: null, + streamingType: "text" as const, activeToolCalls: [], } - test("a parallel fan-out with sibling tools still running is not a stall", () => { - const args = { - ...base, - activeToolCalls: ["call-2"], - lastActivityAt: 0, - nowMs: 20 * 60_000, - } - expect(shouldAbortForStall(args)).toBe(false) - }) - - test("aborts an awaiting run past the timeout", () => { + test("aborts a mid-stream hang past the timeout", () => { expect(shouldAbortForStall(base)).toBe(true) }) @@ -52,35 +45,18 @@ describe("shouldAbortForStall", () => { expect(shouldAbortForStall({ ...base, status: "stopping" })).toBe(false) }) - // Two independent exemptions (a gate open on the operator, a sibling tool - // call still outstanding) must both keep exempting when combined — neither - // one's guard may accidentally require the other's condition to also hold. - test("a gate open and a sibling tool call each exempt alone, and together", () => { - const gateOnly = { ...base, status: "blocked" as const } - const toolCallOnly = { ...base, activeToolCalls: ["call-2"] } - const both = { ...base, status: "blocked" as const, activeToolCalls: ["call-2"] } - - expect(shouldAbortForStall(gateOnly)).toBe(false) - expect(shouldAbortForStall(toolCallOnly)).toBe(false) - expect(shouldAbortForStall(both)).toBe(false) - }) - test("a settled turn with nothing in flight is not a stall", () => { expect( shouldAbortForStall({ ...base, - awaitingResponse: false, + streamingType: null, isProcessing: false, }), ).toBe(false) }) test("mid-thinking silence fires, recent thinking tokens do not", () => { - const thinking = { - ...base, - awaitingResponse: false, - streamingType: "thinking" as const, - } + const thinking = { ...base, streamingType: "thinking" as const } expect(shouldAbortForStall(thinking)).toBe(true) expect( shouldAbortForStall({ ...thinking, lastActivityAt: STALL_TIMEOUT_MS - 1 }), @@ -88,24 +64,70 @@ describe("shouldAbortForStall", () => { }) test("mid-stream text hang aborts", () => { - expect( - shouldAbortForStall({ - ...base, - awaitingResponse: false, - streamingType: "text", - }), - ).toBe(true) + expect(shouldAbortForStall(base)).toBe(true) }) test("long tool runs are not stalls", () => { + expect(shouldAbortForStall({ ...base, streamingType: "tool" })).toBe(false) + }) +}) + +// The other shape silence can take: awaiting the model's next response, with +// no tokens yet — set right after submit and again the instant the last +// outstanding tool call resolves (`turnStateOnSubmit`, the `tool.done` +// handler). A slow model produces exactly this state for as long as it takes +// to reply, so it is never auto-aborted, however long the silence — only the +// notice may surface. This is the regression coverage for CL-5640. +describe("shouldAbortForStall — awaiting the model's next token is never auto-aborted", () => { + const awaiting = { + status: "running" as const, + awaitingResponse: true, + lastActivityAt: 0, + nowMs: STALL_TIMEOUT_MS, + stallTimeoutMs: STALL_TIMEOUT_MS, + isProcessing: true, + streamingType: null, + activeToolCalls: [], + } + + test("does not abort a run merely awaiting a response, however long", () => { + expect(shouldAbortForStall(awaiting)).toBe(false) expect( - shouldAbortForStall({ - ...base, - awaitingResponse: false, - streamingType: "tool", - }), + shouldAbortForStall({ ...awaiting, nowMs: STALL_TIMEOUT_MS * 10 }), + ).toBe(false) + }) + + // Mirrors the tool.done handler: the last outstanding call just resolved, + // awaitingResponse flips true and streamingType resets to null, then the + // model itself takes a long-but-healthy while to start its next reply. + test("healthy post-tool-batch wait never auto-aborts", () => { + expect(shouldAbortForStall({ ...awaiting, activeToolCalls: [] })).toBe( + false, + ) + }) + + test("a parallel fan-out with sibling tools still running is not a stall", () => { + expect( + shouldAbortForStall({ ...awaiting, activeToolCalls: ["call-2"] }), ).toBe(false) }) + + // Two independent exemptions (a gate open on the operator, a sibling tool + // call still outstanding) must both keep exempting when combined — neither + // one's guard may accidentally require the other's condition to also hold. + test("a gate open and a sibling tool call each exempt alone, and together", () => { + const gateOnly = { ...awaiting, status: "blocked" as const } + const toolCallOnly = { ...awaiting, activeToolCalls: ["call-2"] } + const both = { + ...awaiting, + status: "blocked" as const, + activeToolCalls: ["call-2"], + } + + expect(shouldAbortForStall(gateOnly)).toBe(false) + expect(shouldAbortForStall(toolCallOnly)).toBe(false) + expect(shouldAbortForStall(both)).toBe(false) + }) }) describe("applyStallRecovery", () => { @@ -232,8 +254,26 @@ describe("shouldNoticeStall", () => { expect(shouldNoticeStall({ ...base, nowMs: STALL_NOTICE_MS - 1 })).toBe(false) }) - test("hands over to the abort once the run is aborted", () => { - expect(shouldNoticeStall({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe(false) + test("hands over to the abort once a mid-stream hang is aborted", () => { + const midStream = { + ...base, + awaitingResponse: false, + streamingType: "text" as const, + } + expect(shouldNoticeStall({ ...midStream, nowMs: STALL_TIMEOUT_MS })).toBe( + false, + ) + }) + + test("a healthy wait for the model's next token keeps noticing rather than handing over to an abort", () => { + // Unlike the mid-stream case above, this shape never reaches "abort" — + // see the shouldAbortForStall describe block above — so the notice keeps + // surfacing indefinitely instead of going silent once the old timeout + // would have fired. + expect(shouldNoticeStall({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe(true) + expect( + shouldNoticeStall({ ...base, nowMs: STALL_TIMEOUT_MS * 10 }), + ).toBe(true) }) test("a long tool run is not stuck", () => { @@ -261,21 +301,41 @@ describe("the stall level the indicator reads", () => { repeating: false, } - test("quiet, notice and abort partition the same silence clock", () => { - expect(stallLevel({ ...base, nowMs: STALL_NOTICE_MS - 1 })).toBe("quiet") + test("quiet, notice and abort partition the same silence clock for a mid-stream hang", () => { + const midStream = { ...base, awaitingResponse: false, streamingType: "text" as const } + expect(stallLevel({ ...midStream, nowMs: STALL_NOTICE_MS - 1 })).toBe( + "quiet", + ) + expect(stallLevel({ ...midStream, nowMs: STALL_NOTICE_MS })).toBe("notice") + expect(stallLevel({ ...midStream, nowMs: STALL_TIMEOUT_MS })).toBe("abort") + }) + + // Awaiting the model's next token (right after submit, or right after a + // tool batch resolves) never escalates to "abort" — see + // shouldAbortForStall's dedicated describe block — so this shape stays at + // "notice" indefinitely instead of handing over. + test("a healthy wait for the model's next token stays at notice, never abort", () => { expect(stallLevel({ ...base, nowMs: STALL_NOTICE_MS })).toBe("notice") - expect(stallLevel({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe("abort") + expect(stallLevel({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe("notice") + expect(stallLevel({ ...base, nowMs: STALL_TIMEOUT_MS * 10 })).toBe( + "notice", + ) }) test("the indicator keeps reading stalled across the abort threshold", () => { // The notice hands over to the abort so the two never speak at once, but // the phase must not flip back to healthy at the exact moment the run is // most stuck — that was the whole complaint the indicator answers. - expect(shouldNoticeStall({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe(false) - expect(isStalledForDisplay({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe(true) - expect(isStalledForDisplay({ ...base, nowMs: STALL_TIMEOUT_MS * 3 })).toBe( - true, + const midStream = { ...base, awaitingResponse: false, streamingType: "text" as const } + expect(shouldNoticeStall({ ...midStream, nowMs: STALL_TIMEOUT_MS })).toBe( + false, ) + expect( + isStalledForDisplay({ ...midStream, nowMs: STALL_TIMEOUT_MS }), + ).toBe(true) + expect( + isStalledForDisplay({ ...midStream, nowMs: STALL_TIMEOUT_MS * 3 }), + ).toBe(true) }) test("a repeating run is not a stall on any surface", () => { diff --git a/src/tui/stall-watchdog.ts b/src/tui/stall-watchdog.ts index e4c8009cd..ffed304d5 100644 --- a/src/tui/stall-watchdog.ts +++ b/src/tui/stall-watchdog.ts @@ -104,9 +104,33 @@ function silentPastThreshold( ) } +/** + * Whether the run has gone silent while merely *awaiting* the model's next + * response — right after submit or the instant a tool batch resolves, before + * any token of the reply has arrived. `turnStateOnSubmit` and the `tool.done` + * handler both reset `streamingType` to null exactly when they flip + * `awaitingResponse` true, so this state can persist for as long as the model + * takes to start replying: a slow model or a long thinking pass, not + * necessarily a dead one. There is no signal available here to tell "still + * coming" from "never coming" apart, so this case is deliberately excluded + * from auto-abort and left to the notice instead — see `shouldAbortForStall`. + */ +function awaitingFirstToken(args: ShouldAbortForStallArgs): boolean { + return args.awaitingResponse && args.streamingType === null +} + // Pure decision helper: returns true when the run is genuinely stuck and should // be aborted. Extracted so the timeout logic is unit-testable without timers. +// +// Auto-abort is reserved for a stream that had already started producing +// tokens and then went dead mid-flight — the one case silence cannot be +// explained by "still waiting on the model." A long-but-healthy wait for the +// model to start (right after submit, or right after a tool batch resolves) +// is exempted here even past the timeout: it still surfaces via the notice +// (`stallLevel` / `shouldNoticeStall`), but the operator stays in control of +// whether to give up on it rather than having the turn discarded for them. export function shouldAbortForStall(args: ShouldAbortForStallArgs): boolean { + if (awaitingFirstToken(args)) return false return silentPastThreshold(args, args.stallTimeoutMs) } diff --git a/src/tui/turn-monitor.test.ts b/src/tui/turn-monitor.test.ts index 7ef061c1f..110adb1ce 100644 --- a/src/tui/turn-monitor.test.ts +++ b/src/tui/turn-monitor.test.ts @@ -326,11 +326,14 @@ describe("stall watchdog", () => { }) }) - test("aborts and flashes after the stall timeout", async () => { + test("aborts and flashes once a mid-stream hang crosses the stall timeout", async () => { await withTestRenderer(async (h) => { const t: Harness = await setup(h) try { t.bridge.submit("build it", "immediate") + // Tokens actually started flowing, then everything went silent — + // the one shape auto-abort still acts on. + t.bridge.handle({ type: "inference.text.delta", data: { token: "ok" } }) t.port.clear() t.advance(500) @@ -352,6 +355,83 @@ describe("stall watchdog", () => { }) }) + // CL-5640: a healthy wait for the model to start its next reply — right + // after submit, or right after the last outstanding tool call resolves — + // must never be auto-aborted just because the parent stream is quiet. Only + // a stream that had already started producing tokens and then went dead + // (covered above) earns the abort; this shape gets the notice at most. + test("a long-but-healthy wait right after submit is never auto-aborted, only noticed", async () => { + await withTestRenderer(async (h) => { + const t: Harness = await setup(h) + try { + t.bridge.submit("build it", "immediate") + t.port.clear() + + // No delta ever arrives — the model is just slow to start — held + // far past the stall timeout. + t.advance(20 * 60_000) + t.tick() + expect(t.port.calls).toEqual([]) + expect(t.shell.statusFlash).toBe(STALL_NOTICE_MESSAGE) + } finally { + t.bridge.dispose() + } + }) + }) + + test("a long-but-healthy wait right after a tool batch resolves is never auto-aborted", async () => { + await withTestRenderer(async (h) => { + const t: Harness = await setup(h) + try { + t.bridge.submit("build it", "immediate") + t.bridge.handle({ + type: "inference.tool_call.end", + data: { name: "bash", callId: "c1" }, + }) + t.bridge.handle({ + type: "tool.done", + data: { result: { callId: "c1" } }, + }) + t.port.clear() + + // The last outstanding call resolved; the model just takes a long + // while to start its next reply. Held far past the stall timeout. + t.advance(20 * 60_000) + t.tick() + expect(t.port.calls).toEqual([]) + expect(t.shell.statusFlash).toBe(STALL_NOTICE_MESSAGE) + } finally { + t.bridge.dispose() + } + }) + }) + + // CL-5640: live sub-agent progress must keep the parent stream's silence + // exempt from abort even though the parent's own `task` call is the only + // thing in `activeToolCalls` — a future change to task-lifecycle handling + // must not silently drop this exemption. + test("live sub-agent progress under an outstanding task call is never auto-aborted", async () => { + await withTestRenderer(async (h) => { + const t: Harness = await setup(h) + try { + t.bridge.submit("build it", "immediate") + t.bridge.handle({ + type: "inference.tool_call.end", + data: { name: "task", callId: "c1" }, + }) + t.port.clear() + + // The parent stream stays quiet while the sub-agent works — far past + // the stall timeout. + t.advance(20 * 60_000) + t.tick() + expect(t.port.calls).toEqual([]) + } finally { + t.bridge.dispose() + } + }) + }) + test("an open gate is exempt no matter how long the operator takes", async () => { await withTestRenderer(async (h) => { const t: Harness = await setup(h)