Skip to content

fix(subagents): bound a non-terminating foreground child turn with a content-progress stream guard - #2452

Open
yangjj-iso wants to merge 1 commit into
bastani-inc:mainfrom
yangjj-iso:fix/2446-subagent-stream-liveness
Open

fix(subagents): bound a non-terminating foreground child turn with a content-progress stream guard#2452
yangjj-iso wants to merge 1 commit into
bastani-inc:mainfrom
yangjj-iso:fix/2446-subagent-stream-liveness

Conversation

@yangjj-iso

@yangjj-iso yangjj-iso commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

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_delta events, message_end never 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 no signal.aborted check. A stream that floods contentless deltas — or goes silent — never reaches a terminal event, so prompt() never settles. Fallback is throw-driven: it advances only when a throw during consumption is stamped stopReason:"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 AssistantMessageEventStream returned by createAgentSession's streamFn factory with a guard that tracks time since the last content-bearing event. If none arrives within streamStallMs, the guard throws a retryable failure whose message matches the classifier's stream ended before a terminal response event pattern and normalizes to provider_unavailable. Same-model retry and configured fallbackModels then advance within the same prompt() call — or, with no fallback, prompt() resolves with an error and the runner returns control instead of hanging.

Key properties:

  • Classification stays pi-agent-core's job. The guard only decides when to end the stream. An abort during a stall is stamped "aborted"cancelledno fallback; a stall while not aborted is "error" → fallbackable. The abort/interrupt race is handled for free.
  • Content-progress, not wall-clock. The window measures time since the last content-bearing event (any non-empty text/thinking/tool-call delta, plus every structural and terminal event). Long reasoning passes and slow in-flight tools keep resetting it and are never affected. It bounds forward progress within a single stream — not socket idleness, not total turn/tool duration.
  • Throw, never return-done. After the for-await loop, agent-loop calls await response.result(), which would hang forever without a terminal event — so on stall/abort the guarded iterator throws.
  • Inert by default. The guard is off unless 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, 0 disables), resolved from per-run control.streamStallMs or 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

  • New packages/coding-agent/src/core/stream-liveness-guard.ts — the guard (isProgressEvent, guardStreamLiveness, GuardedAssistantMessageEventStream).
  • packages/coding-agent/src/core/sdk-types.ts + sdk.tsstreamStallMs option; 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 (parseStallMs admits the 0 disable sentinel that parsePositiveInt would reject).
  • packages/coding-agent/docs/subagents.md + both packages/*/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; terminal done completes; terminalless close ends retryably; a 16-case isProgressEvent table; and a classification bridge asserting the real stall error normalizes to a same-model-retryable, fallbackable provider_unavailable.
  • test/integration/subagent-stream-stall-fallback.test.ts: drives the real createAgentSession streamFn 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-agent tsgo -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.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with 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.

T-Rex T-Rex Logs

What T-Rex did

  • Ran a focused reproduction of the raw and guarded direct-result calls to address the posted P1 finding.
  • Compared the raw direct-result reproduction output, the guarded direct-result reproduction output, and the existing stream liveness guard unit-suite results to validate behavior across paths.
  • Validated contract behavior by wrapping the same stream with guardStreamLiveness(..., { stallMs: 10 }) and observed that it still times out when no iterator is pulled.
  • Summarized the plain-language takeaway that compaction/summarization paths requesting only the final result can wait forever instead of yielding the configured retryable liveness failure.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (1)

  1. General comment

    P1 Direct result bypasses the stream liveness guard

    • Bug
      • GuardedAssistantMessageEventStream.result() delegates to the underlying nonterminating provider stream. The liveness timeout exists only inside [Symbol.asyncIterator], so a caller that uses await 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.
    • Cause
      • packages/coding-agent/src/core/stream-liveness-guard.ts:137-139 returns this.source.result() directly, while timer setup and the failure race occur only after the guarded async iterator is consumed at lines 141-227.
    • Fix
      • Make 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 direct result().

    T-Rex Ran code and verified through T-Rex

Prompt To Fix All With AI
### Issue 1
packages/coding-agent/src/core/stream-liveness-guard.ts:137-139
**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.

### Issue 2
test/integration/subagent-stream-stall-fallback.test.ts:2
**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.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "fix(subagents): bound a non-terminating ..." | Re-trigger Greptile

Greptile also left 2 inline comments on this PR.

Context used:

  • Context used - AGENTS.md (source)

…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
Comment on lines +137 to +139
override result() {
return this.source.result();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

View artifacts

T-Rex 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";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

subagents: non-terminating model turn can block foreground child indefinitely and prevent fallback

1 participant