Skip to content

Commit 7e69ccc

Browse files
committed
fix(tui): abort stall after post-tool and post-compact silence
Awaiting-response with a null stream is no longer a permanent stall exemption. Execution-watchdog-exempt tools stay bounded by stall timeout instead of pinning forever. Verification: bun run check (7664 pass).
1 parent 445886d commit 7e69ccc

7 files changed

Lines changed: 184 additions & 88 deletions

File tree

‎docs/ARCHITECTURE.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -419,7 +419,7 @@ tool call
419419

420420
**Approval log** (`src/permission/approval-log.ts`, CL-5666): every consequential decision the gate makes — auto-mode allow/deny or an interactive prompt's allow-once/allow-with-scope/deny/timeout/abort — is appended as one JSONL record to `approvals.jsonl` in the session dir, carrying the classifier/auto-shell rule name that fired (the existing `auto-shell-policy.ts`/`classify.ts` rule names, plus a small closed set of additional fixed literals the log itself defines for decisions those modules don't otherwise name — `auto-allowed-tool`, `non-interactive`, `mega-chain` — never model- or user-authored text), whether the decision was `auto` or `interactive`, a shell chain's segment count, and queued/displayed/settled timestamps. `displayedAt` is set by `PermissionRequest.markDisplayed`, called from `gate-wire.ts`'s `open()` the moment a request actually reaches the overlay host — distinct from when it was raised, so the gap it exposes is the CL-5664 signal (a queued gate arming its timeout before the operator could see it). No command text, file content, path, credential, or other free text is ever recorded — only tool name, rule, mode, segment count, and timing; a sub-agent's free-text dispatch label is deliberately left out, even though it would enable a per-agent breakdown, because nothing constrains what a model puts in it. A hard size cap on the serialized line is defense in depth against a future field reintroducing free text. Writes are fire-and-forget and swallow their own errors; the log defaults to a no-op so nothing depends on it being wired. `scripts/approval-forensics.ts` aggregates across local sessions the same way `intervention-forensics.ts` does for stop/nudge events: per-tool counts by outcome and mode, duration/display-delay percentiles, mega-chain counts, and a duplicate-rate proxy (sessions that hit the same rule more than once).
421421

422-
**Tool wall-clock budget vs. permission prompts.** Each tool `run()` is wrapped by an outer execution watchdog (`src/tui/tool-execution-watchdog.ts`). The watchdog arms when Settings set `tools.timeoutMs` / `tools.maxTimeoutMs`, when foreground `run_shell` has an effective timeout (120s default or per-call, plus 1000ms slack so this layer cannot beat shell-guard), or for `mcp__*` calls. Fleet wait tools are exempt, regardless of Settings: the generic per-tool budget never aborts a sub-agent run while the parent waits for it. That exemption is unconditional, not because the worker is otherwise bounded — there is no turn budget; `deadlineMs` is opt-in, and there is no no-progress or thrash stop. A stuck worker that never trips stall or deadline runs until the parent cancels it or, in eval mode, `--agent-timeout-ms` bounds it. `spawn_agent`, `wait_agents`, `ask_director`, `shell_collect`, and `run_shell` with `background: true` stay exempt. By default (`tools.waitForApproval`, Settings → Tools, **On**), an armed budget freezes while the operator is deciding on a permission prompt, so a late approve still runs the tool and the agent waits for the decision instead of timing out under the modal. When **Off**, the budget keeps ticking during the prompt; if it expires first the tool is skipped and the permission modal is dismissed via the budget AbortSignal (auto-deny with a timeout message). The TUI permission queue (`src/tui/gate-wire.ts`, backed by `src/permission/queue.ts`) attaches that signal so ghost prompts cannot outlive an already-aborted tool.
422+
**Tool wall-clock budget vs. permission prompts.** Each tool `run()` is wrapped by an outer execution watchdog (`src/tui/tool-execution-watchdog.ts`). The watchdog arms when Settings set `tools.timeoutMs` / `tools.maxTimeoutMs`, when foreground `run_shell` has an effective timeout (120s default or per-call, plus 1000ms slack so this layer cannot beat shell-guard), or for `mcp__*` calls. Fleet wait tools are exempt, regardless of Settings: the generic per-tool budget never aborts a sub-agent run while the parent waits for it. That exemption is unconditional, not because the worker is otherwise bounded — there is no turn budget; `deadlineMs` is opt-in, and there is no no-progress or thrash stop. A stuck worker that never trips stall or deadline still runs until the parent turn's stall budget fires, the operator cancels, or, in eval mode, `--agent-timeout-ms` bounds it. `spawn_agent`, `wait_agents`, `ask_director`, `shell_collect`, and `run_shell` with `background: true` stay exempt from the per-tool watchdog; `wait_agents`, `ask_director`, and `shell_collect` are still bounded by the TUI stall watchdog so an in-flight poll cannot pin the turn forever. By default (`tools.waitForApproval`, Settings → Tools, **On**), an armed budget freezes while the operator is deciding on a permission prompt, so a late approve still runs the tool and the agent waits for the decision instead of timing out under the modal. When **Off**, the budget keeps ticking during the prompt; if it expires first the tool is skipped and the permission modal is dismissed via the budget AbortSignal (auto-deny with a timeout message). The TUI permission queue (`src/tui/gate-wire.ts`, backed by `src/permission/queue.ts`) attaches that signal so ghost prompts cannot outlive an already-aborted tool.
423423

424424
`mcp__*` tool calls are the exception to "arms only when Settings set it": they arm unconditionally with a 5-minute default (`DEFAULT_MCP_TOOL_TIMEOUT_MS`), overridable via `mcp.timeoutMs` and still capped by `tools.maxTimeoutMs` (CL-6895). Nothing else bounds an MCP call — the stall watchdog treats an in-flight tool as activity by design, so a wedged MCP server previously hung a tool call, and the turn, forever. On expiry the call returns a normal tool-error result ("MCP tool `<name>` timed out after `<n>`s — the server may be wedged; retry or continue without it"); the turn is never aborted. The MCP client itself (`src/mcp/client.ts`, wrapping `@modelcontextprotocol/sdk`) multiplexes concurrent requests over one connection by JSON-RPC message id with no serial queue or mutex in our code or in the vendored SDK's `Protocol.request()` — so concurrent calls to the same server are not expected to deadlock each other. Live forensics for CL-6895 showed multi-minute MCP calls that eventually completed successfully, consistent with a slow server response rather than a client-side deadlock.
425425

‎docs/IMPLEMENTATION.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,7 @@ Provider and model configuration lives in JSON settings files. The global file h
286286
}
287287
```
288288

289-
- `timeoutMs` / `maxTimeoutMs` — outer execution watchdog around each tool `run()`. Unset leaves the generic watchdog unarmed; set these to arm it. `maxTimeoutMs` clamps non-shell tools when set and does not cap a longer requested `run_shell`. Foreground `run_shell` always arms at the effective shell timeout (120s default, `settings.shell.timeoutMs` override, or per-call) plus 1000ms slack. Fleet wait tools are exempt: a dispatched sub-agent is bounded by stall, opt-in `deadlineMs`, and operator cancel, not the generic per-tool budget. Background shell is exempt too: a `run_shell` with `background: true` arms nothing (only a per-call timeout bounds the process; there is no 120s default) and `shell_collect` never arms (a bounded poll over a process that outlives the turn).
289+
- `timeoutMs` / `maxTimeoutMs` — outer execution watchdog around each tool `run()`. Unset leaves the generic watchdog unarmed; set these to arm it. `maxTimeoutMs` clamps non-shell tools when set and does not cap a longer requested `run_shell`. Foreground `run_shell` always arms at the effective shell timeout (120s default, `settings.shell.timeoutMs` override, or per-call) plus 1000ms slack. Fleet wait tools are exempt: a dispatched sub-agent is bounded by stall, opt-in `deadlineMs`, and operator cancel, not the generic per-tool budget. Background shell is exempt too: a `run_shell` with `background: true` arms nothing (only a per-call timeout bounds the process; there is no 120s default) and `shell_collect` never arms the per-tool watchdog (a bounded poll over a process that outlives the turn). An in-flight `shell_collect`, `wait_agents`, or `ask_director` is still bounded by the TUI stall watchdog so it cannot pin the turn forever.
290290
- `waitForApproval` (default **true** when unset) — freeze that budget while a permission prompt is open so a late approve still runs the tool. **Settings → Tools** toggles this live for the next tool call and persists it here. When **false**, the budget keeps ticking during the prompt; on expiry the tool is skipped and the modal is auto-dismissed. The freeze is bounded: after **30 minutes** with the prompt still unanswered the budget resumes ticking on its own, so a prompt that never becomes visible (overlay open, UI gone) cannot hang a tool run indefinitely.
291291

292292
Optional `mcp` block bounds MCP tool calls (`mcp__*` names) specifically — unlike `tools.*`, this arms **unconditionally** even with no settings at all, defaulting to **5 minutes**, since a wedged MCP server otherwise hangs a call forever with nothing to bound it (CL-6895):

‎docs/TUI.md‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -201,13 +201,16 @@ threshold, so a resumed session with already-stale activity shows the settled
201201
glyph immediately rather than alarming about silence the operator missed, and
202202
a stall that breaks and re-arms bursts again.
203203

204-
Auto-abort (`shouldAbortForStall`) is reserved for a stream that had already
205-
started producing tokens and then went dead mid-flight — not for a run that
206-
is merely _awaiting_ the model's next response (right after submit, or the
207-
instant a tool batch resolves and `awaitingResponse` flips back to true).
208-
That wait has no signal to tell "still coming" from "never coming" apart, so
209-
it is never auto-aborted no matter how long it runs; it still surfaces via
210-
the notice, keeping the operator in control of whether to give up on it.
204+
Auto-abort (`shouldAbortForStall`) fires when a live turn has been silent
205+
past `STALL_TIMEOUT_MS`. That includes a stream that started producing
206+
tokens and then went dead, a wait for the model's next response that never
207+
arrives (right after submit, after a tool batch resolves, or after compact
208+
continuation re-entry), and an in-flight poll the per-tool execution
209+
watchdog leaves unarmed (`shell_collect`, `wait_agents`, `ask_director`).
210+
Ordinary in-flight tools stay exempt here — they have their own execution
211+
budget — and an open operator gate still blocks the clock. The notice
212+
still arms first at `STALL_NOTICE_MS` and hands over to the abort at the
213+
budget so the two never speak at once.
211214
The notice is a live diagnosis, not a sticky banner: it comes down on the
212215
same paint as the activity that ends the silence, including when the turn
213216
settles before the next monitor tick.

‎src/tui/runtime-bridge.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1461,6 +1461,7 @@ export function attachSessionBridge(
14611461
stallTimeoutMs,
14621462
isProcessing: bag.turn.isProcessing,
14631463
streamingType: bag.turn.streamingType,
1464+
currentToolName: bag.turn.currentToolName,
14641465
activeToolCalls: bag.turn.activeToolCalls,
14651466
stallNoticeMs,
14661467
repeating: bag.turn.repeating,

‎src/tui/stall-watchdog.test.ts‎

Lines changed: 91 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,7 @@ import {
1313
} from "./stall-watchdog.js";
1414

1515
describe("shouldAbortForStall", () => {
16-
// Mid-stream hang: tokens already flowed, then everything went silent —
17-
// the one shape auto-abort is willing to act on. The generic guards
18-
// (status, threshold, exemptions) are exercised against this base.
16+
// Mid-stream hang: tokens already flowed, then everything went silent.
1917
const base = {
2018
status: "running" as const,
2119
awaitingResponse: false,
@@ -24,6 +22,7 @@ describe("shouldAbortForStall", () => {
2422
stallTimeoutMs: STALL_TIMEOUT_MS,
2523
isProcessing: true,
2624
streamingType: "text" as const,
25+
currentToolName: null,
2726
activeToolCalls: [],
2827
};
2928

@@ -70,17 +69,23 @@ describe("shouldAbortForStall", () => {
7069
});
7170

7271
test("long tool runs are not stalls", () => {
73-
expect(shouldAbortForStall({ ...base, streamingType: "tool" })).toBe(false);
72+
expect(
73+
shouldAbortForStall({
74+
...base,
75+
streamingType: "tool",
76+
currentToolName: "bash",
77+
}),
78+
).toBe(false);
7479
});
7580
});
7681

77-
// The other shape silence can take: awaiting the model's next response, with
78-
// no tokens yet — set right after submit and again the instant the last
79-
// outstanding tool call resolves (`turnStateOnSubmit`, the `tool.done`
80-
// handler). A slow model produces exactly this state for as long as it takes
81-
// to reply, so it is never auto-aborted, however long the silence — only the
82-
// notice may surface. This is the regression coverage for CL-5640.
83-
describe("shouldAbortForStall — awaiting the model's next token is never auto-aborted", () => {
82+
// Awaiting the model's next response with no tokens yet — set right after
83+
// submit, after the last tool call resolves, and after compact continuation
84+
// re-entry (`turnStateOnSubmit`, `tool.done`, `beginSystemContinuation`).
85+
// Cadence still ticks and the notice still arms at STALL_NOTICE_MS; past
86+
// STALL_TIMEOUT_MS this shape auto-aborts so a continuation that never lands
87+
// cannot freeze the turn.
88+
describe("shouldAbortForStall — awaiting-response with a null stream eventually aborts", () => {
8489
const awaiting = {
8590
status: "running" as const,
8691
awaitingResponse: true,
@@ -89,25 +94,35 @@ describe("shouldAbortForStall — awaiting the model's next token is never auto-
8994
stallTimeoutMs: STALL_TIMEOUT_MS,
9095
isProcessing: true,
9196
streamingType: null,
97+
currentToolName: null,
9298
activeToolCalls: [],
9399
};
94100

95-
test("does not abort a run merely awaiting a response, however long", () => {
96-
expect(shouldAbortForStall(awaiting)).toBe(false);
101+
test("aborts a run awaiting a response once the stall budget elapses", () => {
97102
expect(
98-
shouldAbortForStall({ ...awaiting, nowMs: STALL_TIMEOUT_MS * 10 }),
103+
shouldAbortForStall({ ...awaiting, nowMs: STALL_TIMEOUT_MS - 1 }),
99104
).toBe(false);
105+
expect(shouldAbortForStall(awaiting)).toBe(true);
106+
expect(
107+
shouldAbortForStall({ ...awaiting, nowMs: STALL_TIMEOUT_MS * 10 }),
108+
).toBe(true);
100109
});
101110

102111
// Mirrors the tool.done handler: the last outstanding call just resolved,
103-
// awaitingResponse flips true and streamingType resets to null, then the
104-
// model itself takes a long-but-healthy while to start its next reply.
105-
test("healthy post-tool-batch wait never auto-aborts", () => {
112+
// awaitingResponse flips true and streamingType resets to null, then nothing
113+
// else arrives.
114+
test("post-tool-batch silence auto-aborts after the stall budget", () => {
106115
expect(shouldAbortForStall({ ...awaiting, activeToolCalls: [] })).toBe(
107-
false,
116+
true,
108117
);
109118
});
110119

120+
// Same turn shape as compact continuation: beginSystemContinuation calls
121+
// turnStateOnSubmit, which is awaitingResponse + null streamingType.
122+
test("post-compact continuation silence auto-aborts after the stall budget", () => {
123+
expect(shouldAbortForStall(awaiting)).toBe(true);
124+
});
125+
111126
test("a parallel fan-out with sibling tools still running is not a stall", () => {
112127
expect(
113128
shouldAbortForStall({ ...awaiting, activeToolCalls: ["call-2"] }),
@@ -132,6 +147,37 @@ describe("shouldAbortForStall — awaiting the model's next token is never auto-
132147
});
133148
});
134149

150+
describe("shouldAbortForStall — execution-watchdog-exempt tools do not pin forever", () => {
151+
const collect = {
152+
status: "running" as const,
153+
awaitingResponse: false,
154+
lastActivityAt: 0,
155+
nowMs: STALL_TIMEOUT_MS,
156+
stallTimeoutMs: STALL_TIMEOUT_MS,
157+
isProcessing: true,
158+
streamingType: "tool" as const,
159+
currentToolName: "shell_collect",
160+
activeToolCalls: ["collect-1"],
161+
};
162+
163+
test("in-flight collect auto-aborts after the stall budget", () => {
164+
expect(
165+
shouldAbortForStall({ ...collect, nowMs: STALL_TIMEOUT_MS - 1 }),
166+
).toBe(false);
167+
expect(shouldAbortForStall(collect)).toBe(true);
168+
});
169+
170+
test("wait_agents is bounded by the same stall budget", () => {
171+
expect(
172+
shouldAbortForStall({
173+
...collect,
174+
currentToolName: "wait_agents",
175+
activeToolCalls: ["wait-1"],
176+
}),
177+
).toBe(true);
178+
});
179+
});
180+
135181
describe("applyStallRecovery", () => {
136182
test("aborts then notifies with the default message", () => {
137183
const calls: string[] = [];
@@ -170,6 +216,7 @@ describe("shouldNoticeStall", () => {
170216
stallNoticeMs: STALL_NOTICE_MS,
171217
isProcessing: true,
172218
streamingType: null,
219+
currentToolName: null,
173220
repeating: false,
174221
activeToolCalls: [],
175222
};
@@ -207,15 +254,12 @@ describe("shouldNoticeStall", () => {
207254
);
208255
});
209256

210-
test("a healthy wait for the model's next token keeps noticing rather than handing over to an abort", () => {
211-
// Unlike the mid-stream case above, this shape never reaches "abort" —
212-
// see the shouldAbortForStall describe block above — so the notice keeps
213-
// surfacing indefinitely instead of going silent once the old timeout
214-
// would have fired.
215-
expect(shouldNoticeStall({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe(true);
216-
expect(shouldNoticeStall({ ...base, nowMs: STALL_TIMEOUT_MS * 10 })).toBe(
257+
test("an awaiting-response wait hands over to abort at the stall budget", () => {
258+
expect(shouldNoticeStall({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe(false);
259+
expect(shouldAbortForStall({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe(
217260
true,
218261
);
262+
expect(stallLevel({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe("abort");
219263
});
220264

221265
test("a long tool run is not stuck", () => {
@@ -224,9 +268,27 @@ describe("shouldNoticeStall", () => {
224268
...base,
225269
awaitingResponse: false,
226270
streamingType: "tool",
271+
currentToolName: "bash",
227272
}),
228273
).toBe(false);
229274
});
275+
276+
test("in-flight collect notices, then aborts at the stall budget", () => {
277+
const collect = {
278+
...base,
279+
awaitingResponse: false,
280+
streamingType: "tool" as const,
281+
currentToolName: "shell_collect",
282+
activeToolCalls: ["collect-1"],
283+
};
284+
expect(shouldNoticeStall(collect)).toBe(true);
285+
expect(shouldNoticeStall({ ...collect, nowMs: STALL_TIMEOUT_MS })).toBe(
286+
false,
287+
);
288+
expect(shouldAbortForStall({ ...collect, nowMs: STALL_TIMEOUT_MS })).toBe(
289+
true,
290+
);
291+
});
230292
});
231293

232294
describe("the stall level the indicator reads", () => {
@@ -239,6 +301,7 @@ describe("the stall level the indicator reads", () => {
239301
stallNoticeMs: STALL_NOTICE_MS,
240302
isProcessing: true,
241303
streamingType: null,
304+
currentToolName: null,
242305
activeToolCalls: [],
243306
repeating: false,
244307
};
@@ -256,16 +319,10 @@ describe("the stall level the indicator reads", () => {
256319
expect(stallLevel({ ...midStream, nowMs: STALL_TIMEOUT_MS })).toBe("abort");
257320
});
258321

259-
// Awaiting the model's next token (right after submit, or right after a
260-
// tool batch resolves) never escalates to "abort" — see
261-
// shouldAbortForStall's dedicated describe block — so this shape stays at
262-
// "notice" indefinitely instead of handing over.
263-
test("a healthy wait for the model's next token stays at notice, never abort", () => {
322+
test("an awaiting-response wait notices, then aborts at the stall budget", () => {
264323
expect(stallLevel({ ...base, nowMs: STALL_NOTICE_MS })).toBe("notice");
265-
expect(stallLevel({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe("notice");
266-
expect(stallLevel({ ...base, nowMs: STALL_TIMEOUT_MS * 10 })).toBe(
267-
"notice",
268-
);
324+
expect(stallLevel({ ...base, nowMs: STALL_TIMEOUT_MS })).toBe("abort");
325+
expect(stallLevel({ ...base, nowMs: STALL_TIMEOUT_MS * 10 })).toBe("abort");
269326
});
270327

271328
test("the indicator keeps reading stalled across the abort threshold", () => {

0 commit comments

Comments
 (0)