fix: re-arm Responses request-body timeout recovery on turn output - #12654
hellofrommorgan wants to merge 3 commits into
Conversation
…n turn output The one-shot full-replay request-body-timeout recovery was keyed to the logical prompt sequence and never cleared, so a single long-running prompt got exactly one shake-and-retry for its entire lifetime. A second exact 408 later in the same prompt - over history that grew after the first recovery - terminated the turn even with eligible tool-result bulk and retry budget left. Clearing the marker in onAssistantSettledSuccessfully re-arms the recovery on any turn that produced output. Back-to-back timeouts never reach that path, so an unchanged-request loop is still bounded to one changed request.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
roboomp
left a comment
roboomp
left a comment
There was a problem hiding this comment.
P0 — clean root-cause fix. onAssistantSettledSuccessfully (turn-recovery.ts:435) clears #requestBodyReadTimeoutRecoveryPromptSequence only after the assistantTurnProducedOutput filter, so the one-shot full-replay recovery re-arms on genuine forward progress while the guard at line 1267 still short-circuits back-to-back timeouts to handled-terminal — the original loop bound is preserved. Callsite (agent-session.ts:3328) fires on every settled assistant message; the internal filter correctly excludes error/aborted/empty turns.
Tests are strong: the integration case drives a real AgentSession through 408 -> shake+retry -> tool turn -> assistant turn -> 408 -> recovery -> completion and asserts observable request bytes + final message; the three lifecycle units pin re-arm, no-progress-bound, and empty-output-no-rearm. CHANGELOG (Unreleased/Fixed) and docs both updated.
Only a test-only nit inline (AgentTool<any, any, any> vs the no-any convention). Nothing blocking. Thanks for the thorough repro write-up and disclosure, @hellofrommorgan.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2 participants
What
Follow-up to #11878. The one-shot full-replay request-body-timeout recovery is now re-armed by any turn that produced output, instead of staying spent for the entire user prompt.
In the contributor's own words: "should we submit a follow-up PR for…infinite 408s" — "implement … the reset guard on successful turn".
TurnRecovery.#requestBodyReadTimeoutRecoveryPromptSequencewas set to the logical prompt sequence before the shake and never cleared.onAssistantSettledSuccessfullynow clears it, so the guard bounds unchanged-request loops rather than the lifetime of a prompt.Why
#11878 correctly stopped the 5×61s identical-replay loop, and #11878's review moved the marker from
promptGeneration()topromptSequence()so a later user prompt could still recover. But one user prompt in an orchestrator/agentic run is hundreds of assistant turns, and history keeps growing after a successful recovery.Observed in a real session (
gpt-6-astraviagithub-copilot, omp 18.2.6):20:42:37Z— exact 408user_request_timeout/ "Timed out reading request body",requestBodyReadTimeoutFullReplay: true, one 62.5s attempt. Recovery fired, shook tool-result bulk into an artifact, retried once, succeeded (retryRecovery.status: "recovered", superseded by a real assistant response).21:12:36Z— second exact 408, again one 62.2s attempt,requestBodyReadTimeoutFullReplay: true, noretryRecoveryfield: the guard short-circuited tohandled-terminalwhile 72 un-elided tool results and full retry budget (#retryAttemptwas back to 0) were still available. The session stopped until the operator ran/compactand re-prompted.Only the first condition in that
ifcan explain it:retrySettings.enabledwas true,maxRetries (10) <= #retryAttempt (0)was false (the success path resets#retryAttempt), the session was neither disposed, aborting, nor compacting, and the failing turn had empty content so it was not replay-unsafe.Back-to-back timeouts (no intervening output) never reach
onAssistantSettledSuccessfully, so the original loop bound is unchanged.Testing
packages/coding-agent/test/agent-session-responses-body-read-timeout.test.ts: "recovers a second full-replay timeout in the same prompt after an intervening successful turn". Drives a realAgentSessionagainst a local Responses server through408 → shake+retry → tool turn → assistant turn → 408 → shake+retry → completioninside one prompt. Red before the fix (4 requests, second 408 terminal), green after (5 requests, second recovery elides only the new tool-result bulk intoartifact://, assistant text preserved).packages/coding-agent/test/turn-recovery-replay-unsafe.test.ts, newfull-replay timeout one-shot lifecycleblock: re-arms after a turn that produced output (red before the fix); stayshandled-terminalon back-to-back timeouts; does not re-arm on a settled turn that produced no output. The two bounding cases pass both before and after, so the loop protection is pinned, not loosened.bun teston the two touched files plusshake.test.ts: 123 pass, 0 fail.agent-session-retry-recovery,-retry-cap,-retry-fallback,-empty-stop-guard,-terminal-error-persistence,-thinking-loop-retry,-yield-empty-stop-suppression,-auto-compaction-queue,event-controller-error-banner,repro-issue-6879-tool-double-render-retry,issue-2750-subagent-runtime-fallback): 269 pass, 0 fail.bun run ci:check:full: exit 0 (types + oxlint + oxfmt clean).bun check:check:tspasses;check:rsfails onrustfmtdiffs incrates/pi-nativesthat reproduce on a pristine checkout ofmainat936f9762d2with no local changes, so it is pre-existing and unrelated.Disclosure
This pull request transparently discloses AI-assisted implementation and testing, submitted with the author's authorization. No separate human-review, human-exercise, or human-authored explanation claim is made beyond the contributor sentence quoted above, which is the contributor's own wording.
bun checkpasses — not claimed:check:tspasses, butcheck:rsexits 1 onrustfmtdrift incrates/pi-nativesthat reproduces identically on a pristinemainat936f9762d2with no local changes.bun run ci:check:full(types + oxlint + oxfmt) is exit 0. See Testing.