Repository navigation
fix(retry): stop ambiguous reset replay across recovery boundaries #4741
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
095814d
7c88136
f1264e3
cd3f656
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import { isNonReplayableResponse } from "../../lib/upstream-retry"; | ||
| import type { ResponsesRequestContext, ResponsesAdmissionState } from "./core-options"; | ||
| import type { PreparedResponsesRequest } from "./request-prepare"; | ||
| import type { ResponsesTransport } from "./request-transport"; | ||
|
|
@@ -505,6 +506,12 @@ export async function prepareAdapterExchange( | |
| }; | ||
| // Keep recovery kinds in sync with the native Responses `passthroughRecovery:` loop above. | ||
| recovery: for (;;) { | ||
| // Preserve the terminal verdict through adapter and combo error formatting. | ||
| // This also covers a reset reached by a 401/429/413 recovery refetch. | ||
| if (isNonReplayableResponse(upstreamResponse)) { | ||
| cleanupUpstreamAbort(); | ||
| return upstreamResponse; | ||
| } | ||
|
Comment on lines
+532
to
+535
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Re-check the non-replayable verdict after recovery loops. This guard runs only when control enters That formatter receives no upstream code. It replaces Add the same 🤖 Prompt for AI Agents |
||
| if ( | ||
| upstreamResponse.status === 401 | ||
| && isOAuth401ReplayProvider | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -512,7 +512,7 @@ export async function runWithWebSearch(deps: WebSearchLoopDeps): Promise<Respons | |
| signal: headerDeadline.signal, | ||
| }, retryRecovery)); | ||
| }, | ||
| { abortSignal: headerDeadline.signal, label: "web-search-loop" }, | ||
| { replaySafe: true, abortSignal: headerDeadline.signal, label: "web-search-loop" }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Keep the routed model POST non-replayable. When Remove Based on the PR objective, production model POST callers must not opt into reset replay. 🤖 Prompt for AI Agents |
||
| ); | ||
| } | ||
| } finally { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,7 +110,7 @@ export async function runXaiWebSearch( | |
| // Credential-bearing: never follow a redirect off the pinned origin. | ||
| redirect: "manual", | ||
| }, recovery)), | ||
| { abortSignal: linkedSignal.signal, label: "xai-web-search-sidecar" }, | ||
| { replaySafe: true, abortSignal: linkedSignal.signal, label: "xai-web-search-sidecar" }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- xai executor ---'
cat -n src/web-search/xai-executor.ts
printf '%s\n' '--- replaySafe definitions and callers ---'
rg -n -C 4 'replaySafe|ambiguous|reset result|sidecar failure' src tests structure 2>/dev/null | head -240
printf '%s\n' '--- xAI endpoint and idempotency references ---'
rg -n -i -C 3 'api\.x\.ai|/responses|idempotency|xai' src tests structure docs-site README.md 2>/dev/null | head -240Repository: lidge-jun/opencodex Length of output: 618 🤖 get_repo_knowledge executed:
Length of output: 10134 🌐 Web query:
💡 Result: <search_synthesis> <source_evidence> Citations:
🌐 Web query:
💡 Result: <search_synthesis> <source_evidence> Citations:
🌐 Web query:
💡 Result: <search_synthesis> <source_evidence> Citations:
Do not mark the xAI 🤖 Prompt for AI Agents |
||
| ); | ||
| const detachBodyGuard = cancelBodyOnAbort(res.body, linkedSignal.signal); | ||
| if (!res.ok) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -756,6 +756,25 @@ nor releases. That is not a lost send; it is a send the request never made, spen | |
| later recovery in the same request then cannot have. `tests/lib/execution-budget-permits.test.ts` | ||
| pins both ladder shapes against exactly that. | ||
|
|
||
| ## Ambiguous connection-reset replay boundary | ||
|
|
||
| `src/lib/upstream-retry.ts` returns a marked 502 with | ||
| `upstream_closed_before_response` when a fetch rejects with an ambiguous connection | ||
| reset. No response headers is not evidence that a model POST was never processed. | ||
| Only an explicitly replay-safe operation opts into reset retries. The existing | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Update the earlier reset-retry description. Lines 592-600 still state that 🤖 Prompt for AI Agents |
||
| provider HTTP-status policy and the shared physical-send budget remain independent: | ||
| zero refuses dispatch, invalid counts fail, and a stopped send is counted once. | ||
| `src/server/responses/adapter-dispatch.ts` preserves this verdict instead of formatting | ||
| it as a generic replayable upstream error. The existing account and combo guards stop | ||
| on the marker or structured code. The helper and public Responses count regressions | ||
| live in `tests/lib/upstream-retry.test.ts` and | ||
| `tests/responses/responses-send-budget-counts.test.ts`. | ||
|
|
||
| `src/bridge/errors.ts` retains only the two allowlisted non-replayable transport | ||
| codes, reapplies the in-process marker, and does not attach `Retry-After` to them. | ||
| Other upstream codes keep the existing classification; cyber-policy hard blocks | ||
| retain precedence. The combo, 429-refetch, and account-guard tests cover this boundary. | ||
|
|
||
| ## Combo output headroom | ||
|
|
||
| A combo child is admitted against two budgets, not one. `resolveInputCeiling` in | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not mark the routed model request as replay-safe.
This
fetchWithResetRetrycall sends anAdapterRequestcreated fromiterParsedthrough the routed-model dispatch path. WithreplaySafe: true, a connection reset after the provider accepts the POST can trigger a second model request. That can duplicate provider charges or model-side effects.Remove this opt-in unless every image-bridge adapter provides an idempotency guarantee.
Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents