Skip to content

fix(runtime): seal partial thinking before retrying mid-stream network cuts - #4393

Merged
Astro-Han merged 1 commit into
apache:mainfrom
chinawch007:fix-4284-sealed-thinking-network-retry
Sep 3, 2026
Merged

fix(runtime): seal partial thinking before retrying mid-stream network cuts#4393
Astro-Han merged 1 commit into
apache:mainfrom
chinawch007:fix-4284-sealed-thinking-network-retry

Conversation

@chinawch007

Copy link
Copy Markdown
Contributor

Summary

A thinking-only streaming attempt that died from a retryable network error
ended the turn, while the identical attempt state recovered when the 120s
idle watchdog fired first — in the #4284 incident an ECONNRESET landed
seconds before the watchdog would have, so the same fault got opposite
outcomes depending on which detector noticed it. Recovery safety depends on
what the attempt emitted, not on which side detected the cut.

The plain retryable path now accepts thinking-only attempts under the seal
contract the watchdog path already uses: flushStep() closes the partial
thinking as a message under its own id, the retry streams into a fresh id,
and the sealed fragment stays out of the retried request's provider context.
It carries its own budget (MAX_SEALED_THINKING_RETRIES_PER_STEP = 1) so a
gateway that systematically cuts long thinking streams fails fast instead of
accumulating fragments across the full attempt budget. The new
sealedThinkingRecovery flag is mutually exclusive with the other three
retry paths by construction.

Answer text, tool activity, and provider continuation metadata remain
non-retryable; the watchdog and truncated-stream paths are unchanged.

Fixes #4284

Verification

  • npm run format:check — clean (8 formatting nits auto-fixed and folded in)
  • npm run lint — 2304 files, no issues
  • npm run typecheck
  • npm --workspace @maka/runtime run build — clean
  • npm --workspace @maka/runtime run test:dist — 3107 tests, 3094 pass, 13 skip

Not run: the full root-level battery (npm run lint, format:check,
workspace-wide tests, knip).

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: ZCode — analyzed the issue, authored the implementation
and tests from that analysis under a plan reviewed by the contributor; every
step was human-reviewed and verified locally.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Aug 31, 2026

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I did not find a P0-P3 correctness issue in the exact-head change. The new path permits only retryable failures whose physical request emitted thinking but no answer text, tool activity, continuation metadata, or completed step boundary; it seals that fragment under the current message id, rotates the id, and allows one such recovery per provider step. The added tests cover the successful retry, no-output retry, the one-fragment budget, and the continuation-metadata exclusion.

Validation on this exact head passed: clean install, npm run build:test, full repository typecheck, lint, format, git diff --check, and the complete Runtime suite (3081 passed, 13 skipped). The PR is currently CONFLICTING/DIRTY against main and has no hosted test result. I also performed a local semantic conflict resolution that preserves current main’s ordered reasoning-parts accumulator; the merged tree passed build:test, full typecheck, and the complete Runtime suite (3105 passed, 13 skipped). This is therefore a technical code-review GO for the reviewed head, not a merge-ready decision; the author still needs to rebase and obtain current-head hosted checks.

I did not exercise a real paid provider or reproduce the original macOS gateway incident.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@chinawch007
chinawch007 force-pushed the fix-4284-sealed-thinking-network-retry branch from d046ea5 to 304e6cd Compare September 1, 2026 04:05
@chinawch007

Copy link
Copy Markdown
Contributor Author

Thanks for your attention, resolved the conflicts.

…k cuts

A thinking-only attempt that failed with a retryable provider/network
error (the 2026-08-28 ECONNRESET incident: reset landed seconds before
the 120s idle watchdog would have) ended the turn, while the identical
attempt state recovered when the watchdog noticed the failure first.
Recovery safety depends on what the attempt emitted, not on which side
detected the cut: thinking is sealable, so the plain retryable path now
accepts thinking-only attempts with the same flushStep() +
fresh-message-id contract as the watchdog path, under its own budget of
one recovery per step so a systematically cutting gateway fails fast
instead of accumulating sealed fragments across the full attempt budget.

The sealed fragment stays out of the retried request's provider
context. Answer text, tool activity, and provider continuation metadata
remain non-retryable; the watchdog and truncated-stream paths are
unchanged.

Fixes apache#4284

Generated-by: ZCode
@chinawch007
chinawch007 force-pushed the fix-4284-sealed-thinking-network-retry branch from 304e6cd to 99720ef Compare September 1, 2026 04:13
@chinawch007
chinawch007 requested a review from hqhq1025 September 3, 2026 13:31

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving: the change lands on the existing retry gate, keeps a single recoverability predicate, and I found no P0/P1/P2 correctness issue at 99720ef.

I verified the mutual exclusivity claim rather than taking it on faith, and it holds by construction for a reason worth recording here:

  • Watchdog path: the timeout error is a plain Model stream idle timeout after Xms (stream-watchdog.ts), which classifies as Timeout, and Timeout is not in the retryable set in providerRetryMetadata (provider-error-classification.ts). So failure.retryable is false on every watchdog settlement and sealedThinkingRecovery cannot be entered there. That matters beyond tidiness: it is what stops the new budget from silently widening MAX_IDLE_WATCHDOG_RETRIES_PER_STEP from 1 to 2 per step. !idleWatchdogRecovery is a second lock on the same door.
  • Truncated path: truncated outcomes carry modelStepFailure(...), which hardcodes retryable: false (model-adapter.ts), and incompleteStreamHasNoObservableOutput requires !attemptSawThinking while the new path requires it to be true. Exclusive on both counts.

On transcript/projection consistency: the sealed fragment is persisted unsigned and with no provider metadata, and reasoningReplay in materializeRuntimeReplayPlan drops exactly that shape for signed-thinking and Responses transports, so it does not re-enter provider context on a later turn or on overflow recovery either. The one transport that does replay it is openai-chat-plaintext, which is the same contract the watchdog seal path already carries, not new exposure from this PR.

The four new tests drive the real backend.send() path with only providerRetrySleep stubbed, and the two that matter (seal-and-retry, one-fragment budget) fail on current main. One P3 on test strength is inline; it does not block merge.

Mechanics, not review points: the head is 4 commits behind main and all 4 touched ai-sdk-backend.ts, but none of them added or removed a line in the attempt-flag or retry-gate region, and git merge-tree is clean, so this is a rebase for fresh hosted checks rather than a conflict to resolve.

assert.deepEqual(
events
.filter((event) => event.type === 'provider_retry')
.map(({ phase, reason }) => ({ phase, reason })),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P3: this test is the guard for the !attemptHasNoObservableOutput() clause in sealedThinkingRecovery, but it does not actually pin it. Delete that clause and a no-output retryable failure becomes a sealed-thinking recovery: maxAttempts collapses from MAX_PROVIDER_ATTEMPTS_PER_STEP (10) to nextAttempt (2), and the sealed budget is spent before any fragment exists, so a later real thinking cut in the same step can no longer recover. None of that turns this test red, because the projection here is only { phase, reason } and calls === 2, both retry events, and stopReason === 'end_turn' all still hold.

Smallest fix: include maxAttempts in the mapped shape and assert it is 10 in the expected array.

@Astro-Han
Astro-Han merged commit bf5ca67 into apache:main Sep 3, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(runtime): Mid-stream ECONNRESET after thinking output is terminal, while idle-watchdog timeout on the same state recovers

3 participants