feat: opt-in pre-content stall retries (DEV-723 4/5)

[LukasParke]

Stack layer 4/5 — opt-in pre-content stall retries (maxStallRetries)

Base: #774 (StreamFailedError). Top of stack: #776 (raw-stream helpers + docs). Linear: DEV-723.

Changes

timeout: { firstContentMs: 15_000, maxStallRetries: 1 }

Transparently re-issues a turn's request when it stalls before producing any content.

Safety model: with retries enabled, the turn's stream is held back (awaitFirstContent) until its first content-bearing or terminal event arrives; metadata events are buffered and replayed. A discarded stalled attempt therefore never leaked anything downstream — the retry is provably free of duplicated output. Stalls after content started are never retried and surface as StreamStalledError { retryable: false }.

With maxStallRetries unset/0 the stream passes through in real time exactly as before — no buffering, no behavior change.

Implementation: sendTurnRequest() in ModelResult unifies all three request sites; awaitFirstContent / replayThenPipe / normalizeStallRetries helpers in stream-watchdog.ts.

Tests

4 new integration tests: retry-then-succeed (stalled request torn down, second clean), budget exhaustion rethrows the final stall after exactly one retry, mid-content stalls bypass retries entirely (single request asserted), and streaming consumers see the winning attempt's deltas exactly once.

Verification

234 unit tests passing; lint / typecheck clean.

[devin-ai-integration]

Devin Review found 1 potential issue.

View 2 additional findings in Devin Review.

[devin-ai-integration]

🟡 Setting a retry count without a stall deadline silently delays streamed events and never actually retries

The retry budget is read without checking that any stall deadline is enabled (normalizeStallRetries(this.options.timeout) at src/lib/model-result.ts:966), so a caller who sets only the retry count gets their events held back until the model's first real output while no retry can ever trigger.
Impact: Users who configure retries without also configuring a stall timeout see streamed status events arrive late (or not at all while the connection hangs) and gain no retry protection.

Why buffering is enabled without any watchdog

createTurnWatchContext (src/lib/model-result.ts:971-1010) returns the identity watch when hasActiveStreamTimeouts(timeouts) is false, which only considers firstContentMs/contentIntervalMs (src/lib/stream-watchdog.ts:438-446). normalizeStallRetries (src/lib/stream-watchdog.ts:425-431) ignores those fields, so with timeout: { maxStallRetries: 2 } the code takes the buffering branch at src/lib/model-result.ts:999-1002: awaitFirstContent drains the unwatched stream and withholds every pre-content event (e.g. response.created) until a content-bearing or terminal event arrives. Since no StreamStalledError can ever be raised (no deadline armed), the stalled branch is dead and the only effect is delayed delivery.

Suggested change

	const maxStallRetries = normalizeStallRetries(this.options.timeout);
	const maxStallRetries = hasActiveStreamTimeouts(this.options.timeout)
	? normalizeStallRetries(this.options.timeout)
	: 0;

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[perry-the-pr-reviewer]

Perry's Review

Verdict: 💬 Comments / questions

Full review

This PR adds opt-in pre-content stall retries (maxStallRetries) to callModel, unifying the three duplicated request-send sites into a single sendTurnRequest method. The safety model is sound: with retries enabled, the stream is buffered via awaitFirstContent until a content-bearing or terminal event arrives, so a discarded stalled attempt provably never leaks events downstream. The four new integration tests cover the key scenarios well (retry-then-succeed, budget exhaustion, mid-content stall bypass, no-duplicate-events).

Findings

Suggestion: maxStallRetries without a stall deadline silently delays streamed events

sendTurnRequest reads maxStallRetries via normalizeStallRetries (line 966) without checking that any stall deadline is actually armed. hasActiveStreamTimeouts (which gates watchdog activation in createTurnWatchContext) only considers firstContentMs and contentIntervalMs — not maxStallRetries. So a caller who sets only timeout: { maxStallRetries: 1 } gets:

1. Unnecessary buffering: awaitFirstContent holds back all pre-content events (e.g. response.created) until the first content-bearing or terminal event arrives, even though no watchdog is wrapping the stream.
2. No stall detection: Without firstContentMs, no StreamStalledError can ever be raised, so the stalled outcome branch is dead code — retries never fire.
3. Hang forever: If the stream stalls before content, awaitFirstContent blocks indefinitely (no deadline to break it).

This is a misconfiguration trap — the feature is designed to be used with firstContentMs, but nothing enforces or documents that dependency. The fix is straightforward:

const maxStallRetries = hasActiveStreamTimeouts(this.options.timeout)
  ? normalizeStallRetries(this.options.timeout)
  : 0;

This gates the retry+buffering path on an active watchdog, so maxStallRetries without a deadline is a no-op (stream passes through in real time, same as maxStallRetries: 0).

(Devin's review flagged the same issue — this independent analysis confirms it.)

Note: Chat-completions helpers exported but unused/untested

isContentBearingChatChunk, isTerminalChatChunk, and applyChatStreamWatchdog are new exports in stream-watchdog.ts with no call sites or tests in this PR. They're presumably consumed by #776 (top of stack), but adding untested public exports means a regression in them wouldn't be caught until #776 lands. Consider adding unit tests for the classification predicates here, or moving the exports to #776.

Test coverage

The four new integration tests are well-constructed:

• retry-then-succeed: validates the happy path — stalled attempt is torn down (signal aborted), retry succeeds.
• budget exhaustion: validates exactly one retry, then the final stall is rethrown.
• mid-content stall bypass: validates that stalls after content are never retried (single request asserted).
• no-duplicate-events: validates that streaming consumers see each delta exactly once.

One gap: there's no test for the misconfiguration case (maxStallRetries > 0 without firstContentMs). A test asserting that the stream passes through without buffering in that case would prevent a regression of the suggested fix.

Risk assessment

Risk: 🟡 Medium

Dimension	Severity	Risk	Reasoning
Implementation risk	🟨🟨	Medium	The core logic is correct, but the misconfiguration trap is a real behavior change that could surprise users.
Premise risk	🟩	Low	Pre-content stall retries are a well-understood pattern; the safety model (buffer until first content) is sound.
Estimated impact	🟩	Low	The feature is opt-in (default off); the issue only manifests with a specific misconfiguration and causes delayed events, not data loss.

Risk Factor	Severity	Risk	Reasoning
Reversibility	🟩	Low	Fully reversible — removing the config option restores prior behavior.
Detectability	🟨🟨	Medium	The misconfiguration trap produces no error signal — events are silently delayed.
Blast radius	🟩	Low	Only affects callers who opt into maxStallRetries without firstContentMs.
Data integrity	🟩	Low	No persisted state is touched; no output duplication risk (proven by the safety model).
Financial exposure		None	No billing or payment paths are affected.
Security and privacy exposure		None	No credentials, auth, or tenant-isolation paths are touched.
Propagation	🟩	Low	The retry loop is self-contained within sendTurnRequest; no downstream systems absorb errors.
Availability	🟩	Low	A hung stream without firstContentMs already hangs without this PR; the feature doesn't make it worse.
Recovery cost	🟩	Low	Fix is a one-line gate; no data reconciliation needed.
Time to correct	🟩	Low	One-line change, no migration or coordination required.

[perry-the-pr-reviewer]

▶ Prompt for agents: This is a suggestion, not a blocker.

normalizeStallRetries reads the retry count without checking that any stall deadline is armed. hasActiveStreamTimeouts (which gates watchdog activation) only considers firstContentMs/contentIntervalMs, not maxStallRetries. A caller who sets only timeout: { maxStallRetries: 1 } gets unnecessary event buffering via awaitFirstContent (all pre-content events withheld until first content) with no stall detection — the stalled branch is dead code and a hung stream blocks forever.

Suggested change

	const maxStallRetries = normalizeStallRetries(this.options.timeout);
	const maxStallRetries = hasActiveStreamTimeouts(this.options.timeout)
	? normalizeStallRetries(this.options.timeout)
	: 0;

This gates the retry+buffering path on an active watchdog, so maxStallRetries without a deadline is a no-op (stream passes through in real time). Consider also adding a test for the misconfiguration case (maxStallRetries > 0 without firstContentMs).