Repository navigation
fix(sidecar): map provider input overflow to context_length_exceeded - #7352
Conversation
… loops When an image-bridge or web-search loop iteration hit a full request-pacing queue (or an exhausted key pool), the header-phase catch wrapped the RequestPacingQueueOverloadError into LoopError(502, "Provider unreachable"). The caller saw a 502 upstream failure instead of the 429 overload answer with Retry-After that every other route returns. Rethrow the overload like the shared transport classifier already does, so the handler's requestPacingOverloadResponse maps it. Co-Authored-By: Claude Code <noreply@anthropic.com>
When an adapter raised `translation_buffer_limit` as an error event, the bridge handed it to `terminateForTranslatorOverflow`, which ignored its argument and synthesized a usage-less `response.failed`. A turn the provider had already billed was therefore logged with zero usage and cost. The overflow terminal now reports the event's usage through `onUsage` and on the `response.failed` frame, as the ordinary error terminal already does. The client-facing error is unchanged. Co-Authored-By: Claude Code <noreply@anthropic.com>
An upstream 413 (or a 400/422 carrying a definite context-overflow message, or an adapter's own context_length_exceeded event) behind the web-search or image sidecar leaked out as a generic upstream failure, so Codex resent the identical oversized payload through its retry budget instead of compacting. One classifier in context-overflow.ts now serves both loops: the sidecar boundary maps a first-iteration failure to the main path's streaming or JSON overflow response, and both loops end a later iteration's overflow with the same terminal event. Combo attempts keep their raw failure for the combo layer, which already maps a final 413 itself. Closes #7316
…flow Review follow-up for #7316: - A worded in-stream overflow keeps its status and provider wording beside context_length_exceeded, so combo stream preflight can still hop to a larger-context target; only a bare 413 gets the proxy-owned message. - Rate-window wording ("too many tokens per minute") and an explicit rate_limit_exceeded code are never classified as an overflow. - A first-iteration failure marked x-should-retry: false keeps its status and header instead of inviting a compacted resend. - Document the sidecar overflow contract in structure/transports/responses.md.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 15
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
Round-2 review follow-up for #7316: - The loops classify each failure with its status, wording, upstream error.code and non-replayable mark, and tag a qualifying first-iteration JSON failure; the sidecar boundary maps only tagged responses instead of re-parsing the loop's envelope. - A non-replayable failure (HTTP or adapter event) keeps its own verdict mid-stream too, so it never invites a compacted resend. - An upstream rate_limit_exceeded code survives into classification, so a "too many tokens" rate refusal is not read as an overflow. - A 413 with definite overflow wording keeps that wording for combo failover; only a status-only 413 gets the proxy message.
Round-3 review follow-up for #7316: when the sidecar classifier keeps a failure (non-replayable, or an upstream rate_limit_exceeded) but its wording would match the bridge's context-window heuristics, the in-stream event now carries an explicit code, so classifyError cannot rename it context_length_exceeded on the wire.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: d1026ce9f3
ℹ️ 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".
| export function sidecarLoopOverflowEvent( | ||
| failure: SidecarLoopFailure, | ||
| ): Extract<AdapterEvent, { type: "error" }> | undefined { | ||
| if (failure.nonReplayable) return undefined; |
There was a problem hiding this comment.
When a sidecar's first response is marked non-replayable, returning here prevents overflow mapping but does not preserve that verdict on the new jsonError response subsequently created by either loop. That replacement has neither the non-replayable WeakSet marker nor x-should-retry: false, so a marked 413/400/422 (or a marked transport failure carrying an upstream context_length_exceeded code) reaches the client as an ordinary retryable HTTP error and Codex can resend a turn the provider may already be executing. Propagate the replay-refusal marker/header when rebuilding this first-iteration response, rather than only declining the overflow conversion.
Useful? React with 👍 / 👎.
Summary
When an upstream answered HTTP 413 behind the web-search or image sidecar, the turn failed with a generic
upstream_error(Provider error 413). Codex treats that as a retryable transport failure, so it resent the same oversized payload four times and never compacted. The main path has long mapped the same 413 to a terminalcontext_length_exceeded. This PR applies that mapping to both sidecars.Before (streaming turn with
web_search, relay answers 413 with a plain-text body): HTTP 413{"error":{"message":"Provider error 413","type":"upstream_error","code":null}}, which Codex retries.After: HTTP 200 SSE ending in
response.failedwith{"type":"invalid_request_error","code":"context_length_exceeded"}andretryable: false, the same frame the main path emits. Codex then compacts.One classifier, two call sites.
isSidecarContextOverflowinsrc/server/responses/context-overflow.tsdecides for both loops:isDefiniteContextOverflow, now exported fromsrc/combos/failover.ts, so the sidecar and combo failover agree on what an overflow looks like. Rate-window wording such as "too many tokens per minute" and an upstreamrate_limit_exceededcode are excluded. The loops read that code from the bounded, display-safe error body.context_length_exceededevent keeps its code. The web-search progress stream and both loops' adapter-terminal paths previously dropped it to a generic 502.translation_buffer_limitis excluded, because compacting cannot shrink a buffer the proxy owns.Where it applies:
sidecar-execution.tsmaps only a tagged response tostreamingContextOverflowResponseorjsonContextOverflowResponse, with the proxy-owned message so upstream body text is never echoed.sidecarContextOverflowEvent.Decisions and their reasons:
core-comboreads it forcomboFailureDecisionand already maps a final 413 itself, so combo routing is unchanged.x-should-retry: false, keeps its own failure both before and during streaming, because the provider may already have run that turn. This keeps the existing contract inresponses-web-search-thinking-signature-recovery.test.ts. An adapter event markedretryable: falseunder a different code is likewise left as the adapter reported it. When such a kept failure's wording would match the bridge's context-window heuristics, the in-stream event carries an explicit code so the wire never renames itcontext_length_exceeded.onUsagebefore ending the turn, and attaching it to the event would count it twice. This matches the previous behavior.Absorbed community PRs (cherry-picked with original authorship):
queue_full,queue_expired,keys_exhausted) on a sidecar's first send is now rethrown, so it becomes the usual 429rate_limit_errorwithRetry-Afterinstead of502 Provider unreachable. Verified: both loops' eager-drain catches rethrow it, andpolicy-fallback.tsconverts it throughrequestPacingOverloadResponse.translation_buffer_limitterminal now keeps the usage the provider already billed, onresponse.failedand once throughonUsage. Verified that it does not double-report: that branch returns before the ordinary error terminal'sonUsage.Related, already closed: #4112 (the main-path non-streaming 413) and #3675. I searched open issues for 413, Payload Too Large, sidecar and
context_length_exceededand found no open duplicates of #7316.Closes #7316
Co-authored-by: Vadym O bolein95@gmail.com
Verification
tests/responses/responses-sidecar-context-overflow.test.tsruns end to end throughstartServerwith anopenai-chatrelay and covers both sidecars. Cases: streaming plain-text 413 and streaming worded 400 both become a terminalcontext_length_exceeded; an unrelated 400 and arate_limit_exceeded400 keep their own failure; non-streaming 413 keeps HTTP 413 with the classification. It also includes classifier unit cases, with negatives for 429, rate-window wording on a 400,rate_limit_exceeded, andtranslation_buffer_limit. Run against the unmodified source, every positive case failed; with the fix, all pass.tests/web-search/web-search-loop-error-usage.test.ts: a later iteration's 413, an adapter-coded overflow, a worded 400 or 413 that keeps its evidence, a non-replayable 413 (bare or worded) that is left alone, and arate_limit_exceeded400 that keeps its rate verdict.tests/images/loop.test.tsalso covers a worded non-replayable 413.tests/images/loop.test.ts: a later iteration's 413, and a runTurn adapter's own overflow verdict.bun run typecheck: passes.bun run structure:check,bun run privacy:scan,bun scripts/file-size-ratchet.ts: pass.bun teston the touched and neighboring files (sidecar overflow, web-search loop/pacing/thinking recovery, images loop, translator budget, main-path context overflow, lab boundary): all pass.bun run test:changedwith the pinned Bun 1.4.3 runner: 34662 pass / 43 skip / 2 fail across 1544 files.resolveCodexRuntime > treats missing persisted and resolved versions as the same selection, fails identically on a cleanorigin/devcheckout on this machine. It depends on the local environment and is unrelated.bun run test.test:changedalready covered 1544 files through the changed modules' import graph, and the rest is left to CI.test-layout-tooling"launches isolate tests through the pinned runner" fails identically on the base commit here because of a local runner-path detail. It is unrelated.Checklist
structure/transports/responses.mddocuments the sidecar overflow contract; there is no user-facing setting to document.)