fix(responses): reject expired forward continuations - #749
Conversation
📝 WalkthroughWalkthroughCanonical OpenAI forward requests now reject unavailable local replay state instead of forwarding incomplete context. Quota priming is skipped for these misses, while fresh replay, ordinary requests, and API-key provider behavior are covered by end-to-end tests. ChangesExpired forward replay handling
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8db38a2281
ℹ️ 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".
| if ( | ||
| isThreadSpawnRequest(req.headers) | ||
| && !(hasUnexpandedPreviousResponse && isCanonicalOpenAiForwardProvider(route.provider)) | ||
| ) { | ||
| await maybePrimeSubagentQuota(config); |
There was a problem hiding this comment.
When a thread_spawn continuation was previously served by an API-key fallback, then local replay and quota caches become stale (for example after a proxy restart), this condition suppresses maybePrimeSubagentQuota. Unknown quota is treated as available by fallback selection, so the request remains on the canonical route and is rejected below, even though priming would identify the exhausted primary and return the continuation to the API-key provider that owns its upstream response ID. Preserve quota-aware fallback selection, or otherwise evaluate continuation-capable fallbacks, before rejecting.
Useful? React with 👍 / 👎.
| if ( | ||
| hasUnexpandedPreviousResponse | ||
| && isCanonicalOpenAiForwardProvider(route.provider) | ||
| ) { | ||
| return formatErrorResponse( |
There was a problem hiding this comment.
When a combo continuation's local replay has expired and the current target is canonical OpenAI, this local 400 invalid_request_error is classified as stop by comboFailureDecision, so no later target is attempted. This breaks a round-robin/failover chain when the response ID was created by a later API-key Responses target, which could natively resume it. Skip incapable canonical targets for replay misses, or make this target-specific failure hop so the owning provider can be tried.
Useful? React with 👍 / 👎.
| hasUnexpandedPreviousResponse | ||
| && isCanonicalOpenAiForwardProvider(route.provider) | ||
| ) { |
There was a problem hiding this comment.
When a custom or noncanonical openai-responses provider uses authMode: "forward", this canonical-URL check is false, so an expired continuation still proceeds. The adapter strips previous_response_id for every forward provider, not only the canonical URL, and consequently sends only the current delta upstream—the same silent context loss this change is intended to prevent. Align this fail-closed predicate with the adapter's forward-mode stripping predicate, or preserve native continuation IDs for forward providers that actually support them.
Useful? React with 👍 / 👎.
| return formatErrorResponse( | ||
| 400, | ||
| "invalid_request_error", | ||
| "OpenAI forward continuation state is unavailable or expired; start a new session instead of reusing this previous_response_id.", |
There was a problem hiding this comment.
This adds a user-visible terminal condition: after replay TTL expiry, eviction, or loss of the local snapshot, canonical continuations now return 400 and require a new session, but docs-site/src/content/docs/reference/architecture.md only says remembered previous_response_id input is expanded when available and no guide describes this failure or recovery. Update the English documentation and keep its translated counterparts consistent so users can diagnose the new response.
AGENTS.md reference: AGENTS.md:L152-L153
Useful? React with 👍 / 👎.
8db38a2 to
6521518
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/responses/core.ts`:
- Around line 1157-1166: The prime decision currently occurs before fallback
resolution, allowing subagent selection to use stale quota data when the
fallback rewrites route. Move or defer maybePrimeSubagentQuota() until after the
fallback logic, and key the skip condition against the final route so canonical
replay misses still avoid priming while fallback selection can use refreshed
quota scores.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dea9634e-ef48-4c69-ac1d-59b6ec066645
📒 Files selected for processing (2)
src/server/responses/core.tstests/issue-702-expired-replay-state.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/server/responses/core.ts (1)
1157-1167: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftQuota-prime skip is still keyed off the pre-fallback route.
hasUnexpandedPreviousResponse && isCanonicalOpenAiForwardProvider(route.provider)is evaluated here using the route fromrouteModel(config, parsed.modelId)at Line 1149 — before the thread-spawn fallback block (Lines 1177-1208) can rewriterouteviaapplySubagentModelFallback/routeModel(config, fallback.to). This is the exact ordering a previous review flagged:applySubagentModelFallbackselects its target model from cached quota scores, andmaybePrimeSubagentQuota→primeCodexPoolQuotasis what refreshes those scores. If a replay-miss on the canonical forward provider causes priming to be skipped here, but the fallback logic then rewritesrouteto a non-canonical provider (so the Line 1216-1229 fail-closed check no longer fires and the request proceeds), that fallback decision is made on stale/un-refreshed quota data.Key the skip decision off the FINAL route (after fallback resolves), or defer
maybePrimeSubagentQuotauntil fallback has settled, so only requests that actually end up fail-closing skip the prime call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/responses/core.ts` around lines 1157 - 1167, Move the quota-prime skip decision in the thread-spawn flow until after the fallback block has finalized route, or otherwise recompute it from the final route returned by applySubagentModelFallback/routeModel. Ensure maybePrimeSubagentQuota is skipped only when hasUnexpandedPreviousResponse remains true and the final route provider is canonical OpenAI; fallback requests proceeding with a non-canonical provider must still prime quota data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/server/responses/core.ts`:
- Around line 1157-1167: Move the quota-prime skip decision in the thread-spawn
flow until after the fallback block has finalized route, or otherwise recompute
it from the final route returned by applySubagentModelFallback/routeModel.
Ensure maybePrimeSubagentQuota is skipped only when
hasUnexpandedPreviousResponse remains true and the final route provider is
canonical OpenAI; fallback requests proceeding with a non-canonical provider
must still prime quota data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a97496f1-b90b-4b2c-8777-190941e46fa2
📒 Files selected for processing (2)
src/server/responses/core.tstests/issue-702-expired-replay-state.test.ts
|
Merged into Verified locally on the merged tree before pushing, not from the PR description:
One thing worth naming for anyone reading this later: the scope is wider than the issue title. Every replay miss now returns the same 400, not just TTL expiry — restart and capacity eviction included. That is the right call, and the message ("unavailable or expired") already says so, but it is a behaviour change beyond the reported symptom. Thanks for the harness hardening too; restoring Closing as merged. Issue #702 is closed with the same evidence. |
Summary
previous_response_idbut its local replay state is unavailable or expired400 invalid_request_errorbefore authentication, adapter construction, or model-upstream dispatch; canonical replay misses also avoid unnecessary quota primingprevious_response_idsupport for API-key Responses providersfetch, andDate.noware always restoredFixes #702
Root cause
The canonical ChatGPT Responses backend rejects native
previous_response_id. OpenCodex therefore depends on its local replay state to expand the omitted history before forwarding. When that state expired, the prior path logged the replay miss, strippedprevious_response_id, and still forwarded only the continuation delta, silently losing conversation context.Fail-closed conditions
The request is rejected only when all of the following are true:
previous_response_id;The rejection happens before any upstream request. Expired continuations return HTTP 400 with
invalid_request_error.Compatibility controls
previous_response_id: succeeds normally.previous_response_idis preserved and succeeds normally.Validation
bun run typecheck: passedbun run privacy:scan: passedgit diff --check: passedThe local full-suite single-command run did not complete within the available execution time, so this PR does not claim that the full suite passed locally. Repository CI is the complete cross-platform validation authority for Linux, Windows, and macOS.
Summary by CodeRabbit
invalid_request_error, instructing users to start a new session.