fix(session): retry message-only first-event timeouts under bare defaults - #3908
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9487fd1a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return ( | ||
| !hasBareDefaultRetryDisqualifyingFacts(message) && | ||
| (message.errorMessage === WRAPPED_PROVIDER_FIRST_EVENT_TIMEOUT_ERROR || | ||
| BARE_DEFAULT_WATCHDOG_ERROR.test(message.errorMessage ?? "")) |
There was a problem hiding this comment.
When a bare-default run has already dirtied its replay epoch—for example, after an earlier tool execution—and the next request reports a content-free Anthropic stream stalled while waiting for the next event, this regex branch also sets messageOnlyWatchdogTimeout, bypassing the clean-epoch guard at lines 15115–15118. An idle stall proves that at least one stream event occurred, unlike the replay-safe first-event timeout described here, and because it remains classified as an unbounded transient failure the session can repeatedly reissue a billable request where the previous code surfaced the error; match only the timed out while waiting for the first event alternative.
Useful? React with 👍 / 👎.
| // Content-free message-only watchdog prose (wrapped canonical or bare | ||
| // per-provider variants) is admitted like the typed path: it is | ||
| // replay-safe, so a bare-default retry may re-issue the request even | ||
| // after earlier observable activity in the same run. | ||
| const messageOnlyWatchdogTimeout = isBareDefaultMessageOnlyFirstEventTimeout(message); |
There was a problem hiding this comment.
This changes externally observable retry behavior in packages/coding-agent, but the commit leaves packages/coding-agent/CHANGELOG.md unchanged; add the fix under that package's ## [Unreleased] section so it is included in the release record.
AGENTS.md reference: AGENTS.md:L178-L178
Useful? React with 👍 / 👎.
yazzang-homelab
left a comment
yazzang-homelab
left a comment
There was a problem hiding this comment.
Independent architect review.
I went in expecting to find the replay-safety claim unbacked, and traced it before writing. It holds — but not where the comment says it does, and that gap is worth closing.
The claim and where it is actually enforced. isBareDefaultMessageOnlyFirstEventTimeout (agent-session.ts:942-956) admits on two conditions only:
!hasBareDefaultRetryDisqualifyingFacts(message) &&
(message.errorMessage === WRAPPED_PROVIDER_FIRST_EVENT_TIMEOUT_ERROR ||
BARE_DEFAULT_WATCHDOG_ERROR.test(message.errorMessage ?? ""))and hasBareDefaultRetryDisqualifyingFacts (921-927) checks only structured transport facts — status, providerCode, anthropicErrorType, openaiErrorCode, headers. No content check. So the predicate's own doc comment —
replay-safe: the watchdog fired before the stream produced any observable output
— is a property the predicate does not establish. It is established by the callers:
15050-15062:(firstEventTimeout || messageOnlyWatchdogTimeout) && (assistantMessageHasVisibleOrToolContent(message) || (this.#retryAttempt > 0 && !this.#hasCleanRetryReplaySafety) || ...)→ terminal /false.15092:if (!managedFallback && assistantMessageHasVisibleOrToolContent(message)) return false;before the bare-default admissions.
Both gates fire before any retry, so a watchdog message that also carries visible or tool content cannot be replayed. The fail-closed protection the commit message advertises is real.
The maintainability problem is the asymmetry with its immediate neighbour. isBareDefaultCodexOverload (957-963), twelve lines below, bundles !assistantMessageHasVisibleOrToolContent(message) into the predicate itself. So two adjacent admission predicates for the same gate carry the content requirement at different layers, and the one that does not carry it is the one whose comment asserts it. A future caller reaching for isBareDefaultMessageOnlyFirstEventTimeout outside this method gets an unguarded replay admission and a comment telling them it is safe.
Either move !assistantMessageHasVisibleOrToolContent(message) into the predicate — matching the Codex sibling, and cheap since the callers already short-circuit — or reword the comment to say the caller enforces content-freedom. The first is better; it makes the invariant local to the thing that claims it.
What is well done:
- Structured facts fail closed. A message carrying
status: 503alongside watchdog prose is not "provably a content-free watchdog abort", and the code says exactly that. This is the right default for prose-based classification. - The near-miss coverage is the part that makes prose matching defensible — no-article, trailing-period, and
TypeErrorprefix variants explicitly excluded. Regex classification of provider errors rots silently without adversarial cases pinning the boundary. - The typed and message-only paths are now symmetric. The original bug is a good catch: typed first-event timeouts already retried after prior tool activity, message-only prose did not, purely because
!isTypedFirstEventTimeoutgated it. Two representations of the same provider condition behaving differently is the kind of thing that produces irreproducible bug reports. #hasCleanRetryReplaySafetygating re-retry at15053means the second attempt is not admitted on the same reasoning as the first.- The test matrix runs both Anthropic and OpenAI models through a real tool call before the failure, which is precisely the "observable activity in the same turn" precondition from the bug report — not a synthetic first-message timeout that would pass regardless.
Non-blocking overall; the content check is enforced today. Please close the layering gap so it stays enforced.
gajae.pr-review-verdict.v1 merge-approved sha256:c52eaed0331453ed3a99d1d7377a10f0e3057617 reviewer:architect evidence:read of agent-session.ts:921-956 and callers at 15050-15062, 15076, 15092-15110 at this head
c52eaed to
eea8481
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eea84810a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1,1717 +1 @@ | |||
| # Changelog | |||
|
|
|||
There was a problem hiding this comment.
The target diff does not merely omit this fix's entry: it deletes the entire 1,716-line changelog, including every released section, and leaves the packaged CHANGELOG.md empty. Consequently, scripts/release.ts skips this package because it can no longer find ## [Unreleased], and published packages lose all historical release notes. This is fresh evidence beyond the earlier missing-entry comment: restore the existing history and add this change only under ## [Unreleased].
AGENTS.md reference: AGENTS.md:L178-L178
Useful? React with 👍 / 👎.
경고 — 이 PR의 현재 head가 CHANGELOG 전체를 삭제한다머지하면 안 된다. 확인된 사실: 1바이트 — 개행 하나만 남았다. 원인은 내 쪽이다#3932(11:25:32Z 머지)가 그런데 그 결과 리베이스에서 CHANGELOG가 처음으로 진짜 충돌을 내기 시작했고, 그 충돌을 해소하는 과정에서 파일이 비워졌다. 시간대가 명확하다:
전환 비용을 예고하지 못한 건 내 잘못이다. 미안하다. 복구git fetch origin
git checkout origin/dev -- packages/coding-agent/CHANGELOG.md # 해당 패키지 경로로
# 그 다음 ## [Unreleased] 아래에 이 PR의 항목만 다시 추가
git add packages/coding-agent/CHANGELOG.md
git commit --amend --no-edit # 또는 새 커밋앞으로 리베이스에서 CHANGELOG 충돌이 나면 양쪽 항목을 모두 푸시 전에 다음으로 자가 점검할 수 있다: git cat-file -s HEAD:packages/coding-agent/CHANGELOG.md # 30만 바이트 근처여야 정상 |
yazzang-homelab
left a comment
yazzang-homelab
left a comment
There was a problem hiding this comment.
앞선 내 승인을 철회한다. 이 head는 머지하면 안 된다.
내가 승인한 시점 이후 head가 force-push됐고, 새 head에서 CHANGELOG가 파괴됐다. 승인은 그 이전 커밋에 대한 것이었으므로 현재 head에는 유효하지 않다.
코드 리뷰 내용 자체는 그대로 유효하다 — 로직에 대한 판단은 바뀌지 않았다. 되돌리는 것은 머지 가능 판정뿐이다.
원인은 내가 머지한 #3932다(union 드라이버 제거). 상세와 복구 명령은 이 PR에 이미 남긴 코멘트와 #3942 에 있다. CHANGELOG를 origin/dev에서 복원하고 이 PR의 항목만 다시 넣은 뒤 푸시하면, 그 head에 대해 즉시 재승인하겠다.
#3941(CI 가드)이 머지되면 이 상태는 CI에서 자동으로 걸린다.
Removing `packages/*/CHANGELOG.md merge=union` in #3932 was correct -- union never conflicts, it concatenates both sides of an overlapping hunk, which silently filed entries into versions that had already shipped (35 such entries audited on dev, #3929). What it did not account for is the transition: these files now conflict on rebase for the first time, and a bad resolution drops the whole history with no marker. That is not hypothetical. #3932 merged at 11:25:32Z. Between 11:29:29Z and 11:35:02Z, ten open pull requests across six authors force-pushed heads whose CHANGELOG was a single newline -- every released section gone. #3920 #3697 #3870 #3908 #3887 #3864 #3729 #3869 #3866 #3873. Nothing caught it: the files still parse, no test reads them, and the loss looks like a large deletion inside an otherwise legitimate diff. The guard asserts the one property that matters and nothing more: every `## [X.Y.Z]` heading present at the merge base must still be present at the head. Additions pass, rewording passes, and a release commit that consumes `## [Unreleased]` into a new version passes. Only losing a released section fails, and the message names the recovery command. Runs in `affected-plan`, which already checks out full history and carries the immutable event base sha, so it costs one bun invocation and needs no new job. Constraint: a release bump must still be able to add a version heading Constraint: must not depend on byte-size heuristics -- a legitimately small changelog is not a violation Rejected: threshold on deleted line count | fires on large legitimate edits and misses a small changelog emptied completely Rejected: restore merge=union | reinstates the silent misfiling this replaced, and GitHub ignores the driver anyway Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test scripts/changelog-history-guard.test.ts (11 pass); guard run against the three real broken heads (#3873 #3920 #3869) exits 1 and names the lost sections; clean range exits 0; bun run check:tools exit 0 Not-tested: a real release-bump PR end to end
Removing `packages/*/CHANGELOG.md merge=union` in #3932 was correct -- union never conflicts, it concatenates both sides of an overlapping hunk, which silently filed entries into versions that had already shipped (35 such entries audited on dev, #3929). What it did not account for is the transition: these files now conflict on rebase for the first time, and a bad resolution drops the whole history with no marker. That is not hypothetical. #3932 merged at 11:25:32Z. Between 11:29:29Z and 11:35:02Z, ten open pull requests across six authors force-pushed heads whose CHANGELOG was a single newline -- every released section gone. #3920 #3697 #3870 #3908 #3887 #3864 #3729 #3869 #3866 #3873. Nothing caught it: the files still parse, no test reads them, and the loss looks like a large deletion inside an otherwise legitimate diff. The guard asserts the one property that matters and nothing more: every `## [X.Y.Z]` heading present at the merge base must still be present at the head. Additions pass, rewording passes, and a release commit that consumes `## [Unreleased]` into a new version passes. Only losing a released section fails, and the message names the recovery command. Runs in `affected-plan`, which already checks out full history and carries the immutable event base sha, so it costs one bun invocation and needs no new job. Constraint: a release bump must still be able to add a version heading Constraint: must not depend on byte-size heuristics -- a legitimately small changelog is not a violation Rejected: threshold on deleted line count | fires on large legitimate edits and misses a small changelog emptied completely Rejected: restore merge=union | reinstates the silent misfiling this replaced, and GitHub ignores the driver anyway Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test scripts/changelog-history-guard.test.ts (11 pass); guard run against the three real broken heads (#3873 #3920 #3869) exits 1 and names the lost sections; clean range exits 0; bun run check:tools exit 0 Not-tested: a real release-bump PR end to end
5e0c1de to
872b6aa
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 872b6aa82a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const requiresScopedFirstEventTimeout = managedFallback || !legacyRetryConfigured; | ||
| if ( | ||
| firstEventTimeout && | ||
| (firstEventTimeout || messageOnlyWatchdogTimeout) && |
There was a problem hiding this comment.
When a prior tool execution has dirtied the run epoch and the provider returns this message-only first-event timeout twice in succession, the first failure is now retried, but the second enters this expanded branch with #retryAttempt > 0 and #hasCleanRetryReplaySafety === false, so it is surfaced even though the second attempt's scope is clean. This makes the new behavior tolerate only one watchdog failure after earlier activity and prematurely ends transient Anthropic/OpenAI outages that otherwise use unbounded retries; use the clean current attempt scope for every admitted message-only first-event timeout rather than reapplying the run-wide dirty-epoch check after retry one.
Useful? React with 👍 / 👎.
92177aa to
e2d4dcf
Compare
|
Re-open to re-trigger exact-head Dev CI after rebase onto origin/dev 3832188 (provider baselines). |
e08c209 to
2ed0a9c
Compare
…ults Under the default config (no explicit retry.* keys), the scoped first-event-timeout gate in #handleRetryableError required typed transport facts (providerCode=stream_first_event_timeout). Message-only watchdog prose — the wrapped "Error: Provider stream timed out while waiting for the first event", the bare canonical form, and the per-provider "Anthropic stream timed out..." / "OpenAI responses stream timed out..." variants — was blocked by the !isTypedFirstEventTimeout term once the run had observable activity (a prior tool execution in the same turn), so the turn died with the surfaced timeout instead of retrying like other provider non-critical errors. Typed first-event timeouts already retried in this situation. Admit content-free message-only watchdog prose in the bare-default scoped gate and replay-safety disjunct, mirroring the typed admission. The prose is replay-safe: the watchdog fired before any observable output, so re-issuing cannot duplicate side effects. Fail-closed protections are preserved: visible content, conflicting structured facts (status 503), near-miss prose (no article / trailing period / TypeError prefix), extension-hook scope participation, Alibaba/Kimi terminal policy, ollama-cloud bounded retry, and legacy-config unbounded first-party transient behavior (#713). Constraint: preserve typed-only admission when scope tracking is unavailable Constraint: never retry when visible content or extension hooks participated Rejected: reclassify per-provider variants to first_event_timeout | flips legacy unbounded retry to bounded (#713 scope guard) Rejected: blanket hasCleanRetryReplaySafety for prose | over-blocks after prior tool results (the very case being fixed) Confidence: high Scope-risk: narrow Reversibility: easy Tested: agent-session-resilient-retry (54) + fallback-upstream-count e2e + provider-retry-status + fallback-attempt-transaction + auto-compaction-oversized (91 total) + coding-agent check Not-tested: live provider first-event timeout e2e against a real API
…out retry Adds the Unreleased changelog entry for the bare-default first-event timeout retry fix. Confidence: high Scope-risk: none Reversibility: easy
isBareDefaultWrappedFirstEventTimeout is subsumed by isBareDefaultMessageOnlyFirstEventTimeout (wrapped canonical prose plus the BARE_DEFAULT_WATCHDOG_ERROR variants); the bare-default admission disjunct now reads the shared messageOnlyWatchdogTimeout predicate. Confidence: high Scope-risk: narrow Reversibility: easy Tested: resilient-retry + fallback-upstream-count e2e + provider-retry-status + fallback-attempt-transaction + auto-compaction-oversized (91) + coding-agent check
…treamingSession The OpenAI test case in the message-only first-event timeout regression test resolves credentials through AgentSession's modelRegistry/authStorage path, which only had anthropic set in beforeEach. buildBareStreamingSession now sets the runtime key for the model's provider like buildStatusErrorSession and buildModelSession already do. Lore-id: test-credential-isolation-3908 Confidence: high Scope-risk: narrow Reversibility: trivial Tested: targeted test suite will run in CI Not-tested: local bun test (node-pty native build unavailable in this environment)
Exact-head CI must re-run after the post-provider-baseline rebase.
2ed0a9c to
58913bb
Compare
2 participants
Summary
Fixes: "Error: Provider stream timed out while waiting for the first event" should be retryable and retried like other provider non-critical errors — including in
anthropicandopenai-responses.Under the default configuration (no explicit
retry.*keys),AgentSession.#handleRetryableErrorgated first-event timeout retries behind typed transport facts (providerCode=stream_first_event_timeout). Message-only watchdog prose — the wrapped canonical"Error: Provider stream timed out while waiting for the first event", the bare canonical form, and the per-provider"Anthropic stream timed out…"/"OpenAI responses stream timed out…"variants — was rejected by the!isTypedFirstEventTimeoutterm in the scoped bare-default gate once the run had observable activity (e.g. a prior tool execution in the same turn), so the turn died with the surfaced timeout instead of retrying. Typed first-event timeouts already retried in this situation.Change
isBareDefaultMessageOnlyFirstEventTimeout(message)— admits content-free, canonical message-only watchdog prose (wrapped canonical form or a bare per-provider variant matchingBARE_DEFAULT_WATCHDOG_ERROR) with no conflicting structured facts.#handleRetryableError, mirroring the existing typed admission.This is replay-safe: the watchdog fired before any observable output, so re-issuing the request cannot duplicate side effects.
Fail-closed protections preserved (existing tests stay green)
status: 503) → no retryTypeError:prefix) → no retryTests
devand pass here.devand pass here.onPayloadinvocation to pass the attempt scope, matching production wiring (sdk/session.ts); this exercises the real scope-dirtying mechanism instead of the blanket!typedgate.bun test: resilient-retry (54) + fallback-upstream-count e2e + provider-retry-status + fallback-attempt-transaction + auto-compaction-oversized (91 total) all pass.bun --cwd=packages/coding-agent run check(biome + tsc) clean.