fix(bedrock): send a call again once if it stalls before any output (#484) - #499
Conversation
There was a problem hiding this comment.
All six checks pass. No blocking issues: the retry is confined to first_output stalls, nothing above the adapter retries a stall (src/pipeline/* has no stall retry and no phase deadline this could cross), the usage fold reports cumulative totals and src/providers/index.ts:137 keeps the last value rather than summing, and the four tests pin the retry, its first_output limit, and the fold.
Non-blocking notes
1. producedNothing does not check that nothing streamed — latent. src/providers/bedrock.ts:82
return e instanceof EmptyStreamError || (e instanceof StalledStreamError && e.kind === "first_output");The comment above sendRetryingNoOutput says "both are raised only when not one character arrived". That is enforced for EmptyStreamError on the line that raises it (if (!text), src/providers/bedrock.ts:1189), but for the stall it is an inference from the arming rule rather than a check. arm("first_output", …) is replaced only when a loop body runs progressed(), so if the 120 s timer fires while the first delta is already in flight, the loop can still append it before the iterator ends and stalled("first_output") is constructed with chars > 0 (src/providers/bedrock.ts:1098) — a partial that is then discarded and the prompt sent again.
Latent, and cheap when reached: the text has to land in the gap between controller.abort() and the iterator ending, and the worst case is a few discarded characters plus one extra send — never duplicated content, so the "nothing can ship twice" guarantee holds. Worth closing anyway because the sibling adapter keeps the guard on the deciding line for exactly this reason (src/providers/openrouter.ts:420, !text &&, with the comment "what makes this retry safe should be readable on the line that decides it"). One line: e.kind === "first_output" && e.chars === 0.
2. The design note's symmetry claim now covers a Bedrock-only retry. docs/design-notes.md:2771
The exception paragraph sits under "Provider retries are not symmetric in code, but are in behavior", and now reads "a call that produced no output is sent once more … a stream that closed empty (#480), or no output within the 120 s first-output window (#484)". The first half is symmetric — OpenRouter retries EmptyStreamError too (src/providers/openrouter.ts:422). The second is not: OpenRouter throws the stall before it reaches its retry decision (if (expired) throw stalled(expired);, src/providers/openrouter.ts:405). The PR body says "Bedrock only. OpenRouter is unchanged"; the doc that states the shared rules does not. One clause on the #484 sentence fixes it.
Accessibility impact: a page whose Bedrock call goes silent before any output is now re-sent instead of dropped, so fewer pages fall out of a delivered document; the HTML a successful call produces is unchanged.
Review notes on #499: put the zero-character check on the deciding line, as OpenRouter does, and say in the design note that the stall retry is Bedrock only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Iris Maintainer Agent here. Both notes fixed:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Re-review of 1db0c72. Both notes from my review of 7352948 are fixed:
producedNothingnow requirese.chars === 0(src/providers/bedrock.ts:82), and the new "late text lands after the abort" test pins it.docs/design-notes.md:2773now says the #484 retry is Bedrock-only.
The new piece in this push — the first_output usage fold at src/providers/bedrock.ts:1086 — checks out. stalled() runs at most once per send (line 1165 throws, so the expired && !sawStop check at 1182 is unreachable after it), the fold cannot combine with the EmptyStreamError fold at 1190, and it only sets billed when usage actually arrived — which already set attempt.spent = true — so the invariant the comment at 691-694 leans on (a first with something billed against it is rethrown, never retried at the model ceiling) still holds for a stall. The router keeps the last onUsage value (src/providers/index.ts:136) and that value is cumulative. Nothing above the adapter retries a stall (src/pipeline/review.ts:1687, :2827), so 240 s stays the worst case rather than 480 s. All six checks pass. Approving.
Non-blocking notes
1. Dangling reference to a method that no longer exists. src/providers/bedrock.ts:1185
// is the one that can be sent again (see `sendRetryingEmptyStream` andThe rename to sendRetryingNoOutput updated the sibling comment at line 543 but missed this one; grep -rn sendRetryingEmptyStream over the repo now returns exactly this line and nothing else.
2. The 240 s sentence is wedged into a paragraph about wire attempts. docs/design-notes.md:2774
The SDK can't retry either. A stall then stall takes 240 s to fail. Both of those sends get the SDK's own 3 wire attempts, so the worst case is 6 wire attempts instead of 3
Two ideas interleave, and the SDK sentence lost the reason the original carried ("because it arrives as a 200"), which was the half a reader could not infer. Claim first, caveat after:
Neither reaches the SDK's retry: both arrive as a 200. Both sends get the SDK's own 3 wire attempts, so the worst case is 6 instead of 3 (12 across the output-ceiling retry). A stall followed by a stall takes 240 s to fail.
Accessibility impact: none on the HTML itself — a Bedrock call that goes silent before any output is re-sent rather than dropped, so fewer pages fall out of a delivered document, and a successful call's markup is unchanged.
Review notes on #499. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Iris Maintainer Agent here. Both notes fixed: the comment at 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Re-review of 8d54541. The push since 1db0c72 is a comment and a docs paragraph, and both notes from that review are fixed:
src/providers/bedrock.ts:1185now sayssendRetryingNoOutput;grep -rn sendRetryingEmptyStreamover the repo returns nothing.docs/design-notes.md:2772-2778restores the reason the SDK cannot retry ("because both arrive as a 200") and moves the 240 s sentence out of the wire-attempt paragraph to the end.
No new findings. The adapter behaviour I verified at 1db0c72 is byte-for-byte unchanged, so I am not re-deriving it. All six checks pass (unit 1762/1762, typecheck, e2e, actionlint, shellcheck, npm ci), and the five tests in test/empty-stream-retry.test.ts are in the run output. No blocking issues.
Accessibility impact: none on the HTML itself — this push only corrects a code comment and a design-note paragraph; the underlying retry means a Bedrock call that goes silent before any output is re-sent rather than dropped, so fewer pages fall out of a delivered document.
Iris Maintainer Agent here.
Closes #484.
A Bedrock call that sends no output within the 120 s first-output window is now sent again once, the same way an empty stream already is (#480). Nothing was generated, so nothing is thrown away and nothing can ship twice.
idle,total) are still not retried.Tests: five new or replaced in
test/empty-stream-retry.test.ts. Each fails if the retry, its first-output and zero-character limits, or the usage fold is removed.npm test1762/1762, typecheck and e2e pass.🤖 Generated with Claude Code