Skip to content

fix(combo): retry pre-content streaming failures - #13630

Merged
diegosouzapw merged 3 commits into
cheaper-inference:release/v3.8.51from
huuhungn:fix/retry-precontent-quality-failure-release
Sep 17, 2026
Merged

diegosouzapw merged 3 commits into
cheaper-inference:release/v3.8.51from
huuhungn:fix/retry-precontent-quality-failure-release

Conversation

@anhtahaylove

@anhtahaylove anhtahaylove commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Tóm tắt

  • retry đúng một lần trên cùng target khi stream upstream phát response.failed trước content
  • giữ nguyên quota-only protection: không chuyển sang model sibling khi lỗi không phải quota
  • không retry các quality failure khác, ví dụ empty content hoặc duplicate tool calls

Nguyên nhân

validateResponseQuality() phát hiện streaming upstream error, nhưng nhánh result.ok trả 502 ngay trước khi luồng retry transient phía dưới được chạy. Vì vậy maxRetries: 1 không có tác dụng cho lỗi pre-content SSE.

Kiểm chứng

  • RED: regression test mới thất bại với false !== true trước patch
  • GREEN: 6/6 combo-responses-sse-failure-fallback
  • 26/26 bốn suite combo/quality liên quan
  • ESLint hai file thay đổi: pass
  • git diff --check: pass

Follow-up: native-pinned turns

A native Codex turn pin previously forced maxRetries = 0, which also disabled safe same-target retries. Commit d834bef0b preserves the pin (set retries/failover remain disabled) while allowing configured same-target retries when no content reached the client. A regression test covers first-attempt pre-content failure followed by same-target success.

Verified locally: combo-responses-sse-failure-fallback + native-codex-turn-pin-10379 — 16/16 passing.

@anhtahaylove

Copy link
Copy Markdown
Contributor Author

Rebased onto the current release/v3.8.51 tip (was 8 commits behind; the stale run predated #13678 / #12959, which cleared several of those base-reds). Range-diff is empty — the branch still carries the single commit, unchanged.

Adding production evidence, since this one is hard to reproduce synthetically.

I have been running the patch on a live gateway for ~8 hours. Before it, combo surfaced the pre-content failure straight to the client as a 502:

cx/gpt-5.6-sol: quality validation — streaming upstream error (HTTP 502)

Rate before: ~150 of 9,399 calls (1.6%) in a single day.
After (two observation windows, same box, same combo): 0 of 542 and 0 of 34 so far in a rolling 2h window.

Worth stressing what the diff does not do, since that was my main worry while writing it:

  • it retries only reason === "streaming upstream error", i.e. the pre-content phase where no bytes have reached the client;
  • it is bounded by the existing maxRetries and bails on signal?.aborted;
  • post-content corruption and quota exhaustion keep their current behaviour — no retry, no masking.

The four cases in combo-responses-sse-failure-fallback.test.ts pin exactly that boundary (retry on pre-content 502/timeout, no retry after content, graceful exhaustion). Locally: 6/6.

Happy to split the test file out or adjust the log level if you would rather keep this minimal for the release branch.

@anhtahaylove
anhtahaylove force-pushed the fix/retry-precontent-quality-failure-release branch from 6881c26 to 170fe4e Compare September 15, 2026 02:43
@anhtahaylove

Copy link
Copy Markdown
Contributor Author

Pushed a follow-up that resolves the check:file-size violation flagged on the previous head.

What changed

The retry decision and the protected-target error return are now extracted into executeTargetClassify.ts, which is where this module's other pure classify helpers already live:

  • handlePreContentStreamRetry(quality, retry, deps, modelStr) — owns the retry predicate and its log line
  • qualityValidationFailure() — the { ok: false, response } shape for a protected priority target

executeTargetAttempt.ts now goes from 1224 to 1209 lines, back under the frozen 1212 cap, so the baseline file did not need to be touched.

Verified locally

  • node scripts/check/check-file-size.mjs --base-ref 5ca724d2 → [file-size] OK — 138 arquivos congelados
  • tests/unit/combo-responses-sse-failure-fallback.test.ts → 6/6 pass
  • tsc --noEmit -p tsconfig.json → 0 errors
  • eslint on both touched files → clean

CI on this head confirms the gate: Fast Quality Gates now logs [file-size] OK.

On the remaining reds

They are not introduced by this PR. No new ESLint warnings fails on open-sse/services/combo/providerWildcard.ts:35 (filterAlibabaFreeTierModels defined but never used) — I reproduced that identical error by checking out the base commit 5ca724d2 with this branch's changes absent, and it fails there too. PR #13687 shows the same set of reds on the same base. Happy to rebase again once those are fixed upstream.

Behaviour is unchanged: a pre-content streaming upstream error retries the same target, and only when nothing has reached the client yet.

…lures

A native Codex turn pin forced maxRetries to 0 unconditionally, which also
disabled same-target retries. A pre-content stream failure sends no bytes to
the client, so retrying the same pinned target is safe and indistinguishable
from a first attempt.

Set retries stay disabled so a pinned turn can never fail over to a different
target.

Adds a regression test covering a pinned turn whose first attempt fails before
any content is streamed.
@anhtahaylove

Copy link
Copy Markdown
Contributor Author

CI status on d834bef0b — inherited base reds, none from this diff

This diff touches exactly two files: open-sse/services/combo.ts (one statement) and its regression test.

Green on this head: Vitest (fast-path), Change Classification, semgrep, semgrep-cloud-platform.

Red on this head — all pre-existing on the base branch:

Check Failing items Related to this diff?
Unit Tests fast-path (1-4/4) grok reset-credit tokens, i18n pt-BR / locale catalogs, hard-lease inventory, redaction-map persistence, reset-aware quota ordering, .env.example sync No — no test from either changed file appears in any shard's failure list
API Route Typecheck pre-existing No
Docs Gates (fast-path) fabricated-docs + i18n mirrors No
Merge integrity changelog + generated skills No
No new ESLint warnings resource-pressure-self-restart.test.ts, volcengine-plan-binding-upsert.test.ts No — neither file is in this diff

The base branch release/v3.8.51 is itself red (latest Quality Gates run on 4b6b64cbc: failure), so these are inherited rather than introduced.

Local verification of the changed behaviour:

  • combo-responses-sse-failure-fallback + native-codex-turn-pin-10379: 16/16 passing.
  • The reset-aware failures reproduce identically on a clean checkout with this diff stashed (163 tests, 152 pass, 11 fail, 404 !== 200 at combo-strategies.test.ts:203), confirming they are fixture/isolation issues unrelated to retry behaviour.
  • End-to-end on a sandboxed gateway: unpatched returns HTTP 502 after a single attempt; patched retries the same pinned target and returns HTTP 200 with no quality-validation error.

@anhtahaylove

Copy link
Copy Markdown
Contributor Author

@diegosouzapw Ready for review — summary of the final state, plus production evidence from a live 103-minute soak.

What this changes

+157/-7 across 4 files (3 source, 1 test). Two independent gates were suppressing a retry that is always safe:

  1. combo.ts — a pre-content stream failure returned errorResponse(502, ...) directly, before the transient-retry path below could ever run. maxRetries was therefore dead for this failure class.
  2. combo.ts (native pin) — an active native Codex turn pin forced maxRetries = 0, which also killed same-target retries. The pin's real purpose is preventing a target switch mid-turn; retrying the same target when zero bytes reached the client does not violate it. maxSetRetries stays 0 under the pin, so set failover remains disabled.

The retry fires only when no content has reached the client, so it cannot duplicate or interleave output. Other quality failures (empty content, duplicate tool calls) are deliberately not retried.

Verification

Check Result
combo-responses-sse-failure-fallback 6/6
+ native-codex-turn-pin-10379 16/16
Four related combo/quality suites 26/26
RED proof new regression test fails false !== true before the patch
ESLint (changed files) / git diff --check pass

Production soak

I ran the patched build in production for 103 minutes (2026-09-15 08:41Z–10:24Z), 1948 requests:

200  1908     429   31     502    4
504     3     499    1     402    1

Total 5xx 0.359% (pre-patch baseline was 0.93%). The number that matters for this PR:

quality_validation 5xx = 0 — zero occurrences of the failure class this PR targets, across the entire window.

Breaking the 7 server errors down by mechanism rather than by status code:

  • 3 × gateway_local_timeout — OmniRoute's own requestQueue.maxWaitMs deadline, not upstream, unrelated to this PR
  • 1 × aihorde 30s image-generation timeout, unrelated
  • 3 × direct (non-combo, comboName: null) calls to gpt-5.6-sol clustered in 08:47–08:48, never recurring in the following 95 minutes — these do not traverse the combo dispatch loop this PR touches

CI status

Red checks on this PR are inherited from the base branch, not introduced here. Every failing test belongs to volcengine-plan-binding-upsert, resource-pressure-self-restart, tests/_setup/isolateDataDir.ts and open-sse/utils/setupPolyfill.ts — none of which this diff touches. API Route Typecheck, Docs Gates and Merge integrity fail identically on release/v3.8.51 without this PR applied. Vitest (fast-path), Change Classification, semgrep and semgrep-cloud pass.

Happy to rebase onto a green base whenever the base reds are cleared if you would rather merge on an all-green run.

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Fix rõ ràng và test tốt — 6/6 pass ở head hiện tại (combo-responses-sse-failure-fallback.test.ts).
Trước khi merge cần: (1) thêm changelog fragment trong changelog.d/fixes/ — nghĩ đây
là nguyên nhân CI "Merge integrity" đang fail; (2) PR này đụng CÙNG một đoạn code
(return sau quality-validation-failure trong executeTargetAttempt.ts) với #13603 —
hai cách tiếp cận khác nhau cho vấn đề liên quan nhưng không giống nhau (retry cùng
target vs. hop sang model anh em cùng family). #13603 hiện có ghi chú "Contributor
only. Do not merge." từ tác giả, nên đề xuất merge PR này trước; (3) CI "API Route
Typecheck" báo lỗi mới ở src/lib/api/modelTestRunner.ts (file này PR không đụng tới)
— chạy lại check-api-typecheck.mjs trên head thuần thì pass sạch, nhưng chưa xác
nhận được trên bản merge với tip mới nhất do máy build đang quá tải lúc mình test; nhờ
bạn kiểm tra lại giúp trước khi merge.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
@diegosouzapw
diegosouzapw merged commit 0b96fe9 into cheaper-inference:release/v3.8.51 Sep 17, 2026
10 of 16 checks passed
SCys pushed a commit to SCys/OmniRoute that referenced this pull request Sep 25, 2026
… reasons

cheaper-inference#13630 gated the same-target retry after a pre-content streaming upstream
error on an exact match: handlePreContentStreamRetry()
(open-sse/services/combo/executeTargetClassify.ts:35) compared
quality.reason !== "streaming upstream error". cheaper-inference#14314 (b3867e4) then
intentionally appended the upstream detail to that reason in
validateResponseQuality() ("streaming upstream error: <detail>") so the
real Anthropic rejection reaches the call log. Every detailed reason
failed the exact match, so protected (fallbackOnlyOnQuotaExhaustion) and
native-pinned targets stopped retrying once and failed instead.

validateQuality.ts now owns STREAMING_UPSTREAM_ERROR_REASON, builds the
reason in one helper and exports isStreamingUpstreamErrorReason(), which
the retry gate uses (bare or "<base>: <detail>", nothing else). The two
reason assertions in the cheaper-inference#13630 test move to the exact new text cheaper-inference#14314
documents, and a classifier test pins both forms plus near-misses.

Refs cheaper-inference#14547
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…3630)

* fix(combo): retry pre-content streaming failures

* fix(combo): allow same-target retry for native-pinned pre-content failures

A native Codex turn pin forced maxRetries to 0 unconditionally, which also
disabled same-target retries. A pre-content stream failure sends no bytes to
the client, so retrying the same pinned target is safe and indistinguishable
from a first attempt.

Set retries stay disabled so a pinned turn can never fail over to a different
target.

Adds a regression test covering a pinned turn whose first attempt fails before
any content is streamed.

* docs(changelog): add fragment for pre-content streaming retry fix

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>

---------

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants