fix(subagents): bound a non-terminating foreground child turn with a content-progress stream guard - #2452
Conversation
…content-progress stream guard A non-terminating foreground in-process subagent model turn could block its parent indefinitely and prevent a configured fallback. The reporter observed 32749 consecutive whitespace-only toolcall_delta events, message_end never observed, the foreground child pending, fallback attempt count 0, and the parent blocked ~6.5 hours. pi-agent-core's consumption loop (`for await (const event of response)`) has no inter-event timeout and no signal.aborted check, so a stream that floods contentless deltas — or goes silent — never reaches a terminal event and prompt() never settles. Fallback is throw-driven, so a stream that throws nothing never advances a candidate. Wrap the AssistantMessageEventStream returned by createAgentSession's streamFn factory with a content-progress liveness guard. It tracks time since the last content-bearing event; if none arrives within streamStallMs it throws a retryable failure whose message matches the classifier's `stream ended before a terminal response event` pattern and normalizes to provider_unavailable, so same-model retry and configured fallbackModels advance within the same prompt() call — or, with no fallback, prompt() resolves with an error and the runner returns control. Classification stays pi-agent-core's job: an abort during a stall is stamped "aborted" -> cancelled -> no fallback. The guard is inert unless streamStallMs > 0. Only unattended in-process subagent sessions set it; the interactive main session is byte-for-byte unchanged. Subagent control config gains streamStallMs (default 300000 ms, 0 disables), resolved from per-run/global config and threaded through the child spec into the SDK session. Real reasoning and slow in-flight tools keep resetting the window and are never affected. Fixes bastani-inc#2446
| override result() { | ||
| return this.source.result(); | ||
| } |
There was a problem hiding this comment.
Direct result calls bypass the liveness guard
result() delegates directly to the underlying provider stream, while the stall timer is only created when the guarded async iterator is consumed. Compaction and summarization paths that await stream.result() without iteration therefore never arm the configured timeout; a provider stream that never produces a terminal result leaves the child and its parent waiting indefinitely rather than surfacing a retryable failure for retry or model fallback.
Artifacts
Focused reproduction source for raw and guarded direct-result calls
- Authored Bun script constructs a nonterminating provider stream and compares raw versus liveness-wrapped direct result calls, showing the guard is not entered by direct result().
Raw direct-result reproduction output
- Executed the raw nonterminating provider stream for 80ms; direct result() timed out with no iterator pull, establishing the baseline hang.
Guarded direct-result reproduction output
- Executed the same stream wrapped with a 10ms liveness guard; direct result() still timed out with zero stall callbacks and zero iterator pulls, proving the bypass.
Existing stream liveness guard unit-suite output
- Executed the repository's focused guard tests; all 28 tests passed, showing existing coverage does not exercise the direct-result path.
Ran code and verified through T-Rex
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/coding-agent/src/core/stream-liveness-guard.ts
Line: 137-139
Comment:
**Direct result calls bypass the liveness guard**
`result()` delegates directly to the underlying provider stream, while the stall timer is only created when the guarded async iterator is consumed. Compaction and summarization paths that await `stream.result()` without iteration therefore never arm the configured timeout; a provider stream that never produces a terminal result leaves the child and its parent waiting indefinitely rather than surfacing a retryable failure for retry or model fallback.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| @@ -0,0 +1,226 @@ | |||
| import assert from "node:assert/strict"; | |||
| import { mkdirSync, mkdtempSync, rmSync } from "node:fs"; | |||
There was a problem hiding this comment.
Test bypasses runtime filesystem helpers
This root integration test imports and invokes node:fs operations directly instead of using test/helpers/runtime.ts, bypassing the repository's centralized cross-runtime filesystem behavior and platform conventions.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/integration/subagent-stream-stall-fallback.test.ts
Line: 2
Comment:
**Test bypasses runtime filesystem helpers**
This root integration test imports and invokes `node:fs` operations directly instead of using `test/helpers/runtime.ts`, bypassing the repository's centralized cross-runtime filesystem behavior and platform conventions.
**Context Used:** AGENTS.md ([source](https://github.com/bastani-inc/atomic/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
Fixes #2446. A non-terminating foreground in-process subagent model turn could block its parent indefinitely and prevent a configured fallback from ever being reached. The reporter observed 32749 consecutive whitespace-only
toolcall_deltaevents,message_endnever observed, the foreground child pending, fallback attempt count 0, and the parent blocked ~6.5 hours.Root cause
pi-agent-core's consumption loop (
for await (const event of response)) has no inter-event timeout and nosignal.abortedcheck. A stream that floods contentless deltas — or goes silent — never reaches a terminal event, soprompt()never settles. Fallback is throw-driven: it advances only when a throw during consumption is stampedstopReason:"error". A stream that emits no terminal error throws nothing, so fallback is never consulted.Fix — a content-progress liveness guard at the stream layer
Wrap the
AssistantMessageEventStreamreturned bycreateAgentSession's streamFn factory with a guard that tracks time since the last content-bearing event. If none arrives withinstreamStallMs, the guard throws a retryable failure whose message matches the classifier'sstream ended before a terminal response eventpattern and normalizes toprovider_unavailable. Same-model retry and configuredfallbackModelsthen advance within the sameprompt()call — or, with no fallback,prompt()resolves with an error and the runner returns control instead of hanging.Key properties:
"aborted"→cancelled→ no fallback; a stall while not aborted is"error"→ fallbackable. The abort/interrupt race is handled for free.await response.result(), which would hang forever without a terminal event — so on stall/abort the guarded iterator throws.streamStallMs > 0. Only unattended in-process subagent sessions set it; the interactive main session is byte-for-byte unchanged.Config surface
Subagent control config gains
streamStallMs(default 300000 ms,0disables), resolved from per-runcontrol.streamStallMsor global config and threaded through the child spec into the SDK session. With defaults it trips after the observational needs-attention (60000 ms) and long-running (240000 ms) notices, which surface a stalled child but never terminate it.Files
packages/coding-agent/src/core/stream-liveness-guard.ts— the guard (isProgressEvent,guardStreamLiveness,GuardedAssistantMessageEventStream).packages/coding-agent/src/core/sdk-types.ts+sdk.ts—streamStallMsoption; all three streamFn return paths wrapped.packages/subagents/src/{shared/types-results,extension/schemas,runs/shared/subagent-control,runs/inprocess/runner,runs/foreground/inprocess-run-sync}.ts— config + spec threading (parseStallMsadmits the0disable sentinel thatparsePositiveIntwould reject).packages/coding-agent/docs/subagents.md+ bothpackages/*/CHANGELOG.md.Tests
Maps to the issue's required cases — long legitimate turns, in-flight tools, contentless deltas, abort/interrupt races, cleanup after recovery, and fallback becoming reachable.
test/unit/stream-liveness-guard.test.ts(deterministic injected clock/timer): long turn spanning many windows never trips; non-empty tool-call deltas keep it alive; whitespace flood trips bounded with one stall; silent hang trips via the timer with no unhandled rejection; already-aborted and abort-during-stall both outrank the stall; cleanup removes the listener and cancels timers; terminaldonecompletes; terminalless close ends retryably; a 16-caseisProgressEventtable; and a classification bridge asserting the real stall error normalizes to a same-model-retryable, fallbackableprovider_unavailable.test/integration/subagent-stream-stall-fallback.test.ts: drives the realcreateAgentSessionstreamFn factory — a stalled provider stream ends with a fallbackable failure the classifier advances on, and an abort during a stall surfaces as an abort (not a fallbackable failure).Verification
npm run check(biome +tsc --noEmit+ coding-agenttsgo -p tsconfig.build.json --noEmit+ shrinkwrap) is green on the rebased tree. New unit (28) and integration (2) tests pass. Remaining full-suite failures on my Windows machine are the pre-existing symlink-EPERM / load-sensitive / tar-C: environmental cases documented in AGENTS.md, unrelated to this change.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Greptile Summary
This change adds a configurable content-progress timeout for unattended in-process subagent streams and connects stalled streams to retry and model fallback handling. Direct final-result consumers in compaction and summarization do not enter the guarded iterator, so a provider that never produces a terminal result can still leave those flows waiting indefinitely. The integration test should also use the repository runtime filesystem helper to retain cross-runtime filesystem behavior.
Confidence Score: 4/5
Do not merge until direct final-result consumers receive the same stall protection as iterated streams.
A nonterminating-stream reproduction showed that direct result calls did not pull the guarded iterator or invoke its stall callback, while the focused existing guard tests passed without covering that path.
Files Needing Attention: packages/coding-agent/src/core/stream-liveness-guard.ts needs a direct-result liveness policy; test/integration/subagent-stream-stall-fallback.test.ts needs the runtime filesystem helper.
What T-Rex did
Comments Outside Diff (1)
General comment
GuardedAssistantMessageEventStream.result()delegates to the underlying nonterminating provider stream. The liveness timeout exists only inside[Symbol.asyncIterator], so a caller that usesawait stream.result()without iterating never starts the timeout and can hang indefinitely. This is reachable from the compaction session-summary, branch-summary, and range-planner request paths.packages/coding-agent/src/core/stream-liveness-guard.ts:137-139returnsthis.source.result()directly, while timer setup and the failure race occur only after the guarded async iterator is consumed at lines 141-227.result()observe the same liveness policy as iteration—for example, have it consume the guarded iterator through terminal completion and return/throw the corresponding final result, with safe single-consumer coordination—or change every direct-result compaction/summarization caller to consume the guarded stream before awaiting its result. Add focused tests for silent and whitespace-flooding sources invoked through directresult().Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(subagents): bound a non-terminating ..." | Re-trigger Greptile
Context used: