fix(compact): route combo compact requests through the failover path (rebase of #3109) - #3184
Conversation
When a compact request resolved through a combo, the native-compact fast path sent the request directly to the picked provider without failover. A 429 or 5xx from that target surfaced as an exhausted-retry error to the client instead of advancing to the next combo target. Skip native compact when route.combo is set so the request falls through to the synthetic compaction path, which dispatches through handleResponses → handleComboResponses with full combo failover (cooldown + advanceToNext). (cherry picked from commit df4c544)
(cherry picked from commit 78855ed)
(cherry picked from commit 9582fc3)
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. |
|
✅ Deterministic PR hygiene checks passed. |
![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: a0f036f875
ℹ️ 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 responsePayload = isPlainObject(payload.response) ? payload.response : undefined; | ||
| const output = Array.isArray(responsePayload?.output) ? responsePayload.output : []; | ||
| const compaction = output.find(item => isPlainObject(item) && item.type === "compaction"); |
There was a problem hiding this comment.
When a streaming Responses backend emits the native compaction blob in response.output_item.done and follows it with response.completed whose output is empty—a protocol shape already exercised in tests/server-auth.test.ts—this parser ignores the item because it only searches the terminal snapshot. A combo child using this parsed adapter path therefore emits done without compactionEncryptedContent, causing the bridge to synthesize an empty ocx1: envelope and /responses/compact to return 502 instead of the valid native compaction. Retain compaction items from response.output_item.done as well, with the terminal's non-empty output remaining authoritative.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
2 participants
Summary
Maintainer rebase of #3109 onto current
dev— all five commits cherry-picked with author credit preserved, no conflicts.Routes combo compaction requests through the failover path, so a combo whose first target returns 429/5xx during
/responses/compactreaches a backup instead of failing the compaction outright.Review threads — both P1s already resolved on the branch
CHANGES_REQUESTEDcame from bot reviews against399726aae0, four commits behind this head. Both substantive findings were fixed by later commits on the same branch, which is why the threads read unresolved rather than addressed:P1 — "Decode
ocx1after account-gated combo failover." The concern: when a combo starts on an account-gated model, fails over, and a synthetic-compaction backup wins, the code decided whether to decode based on the pre-failover child, so anocx1:envelope could be returned raw to a v1 caller expecting replacement-history messages.src/server/responses/compact.tsnow keys that decision on the returned prefix rather than the originally selected target:Whichever target actually won, an
ocx1:envelope is decoded. "test(compact): cover combo failover and streaming" adds the mixed-target regression the review asked for.Minor — "Reject ciphertext-free streaming completions." The SSE path could emit
{ type: "done" }with no compaction payload where the buffered path already rejected the same condition. Fixed by "fix(compact): reject empty native ciphertext", and the guard handles the subtle case the reviewer implied: an emptyocx1:envelope decodes to""rather thannull, so the check is on decoded length, not nullness.Verification
Exact head
a0f036f87:bun test ./tests/server-combo-failover-e2e.test.ts— 78 pass, 0 fail, 475 expect() callsbun test ./tests/responses-compaction-routing.test.ts— 47 pass, 0 fail, 213 expect() callsbun test ./tests/responses-compaction.test.ts— 25 pass, 0 failThe failover e2e file is the one that matters here — it also contains the assertion #3175 corrected for the combo effort fix, and both changes coexist cleanly on this head.
Full-suite and typecheck coverage is left to CI on this exact head.
Checklist
dev