Skip to content

fix(sse): reconcile combo cooldown-wait contract with its gate (#8541) - #8614

Closed
MumuTW wants to merge 1 commit into
cheaper-inference:release/v3.8.49from
MumuTW:fix/8541-combo-cooldown-wait-contract
Closed

MumuTW wants to merge 1 commit into
cheaper-inference:release/v3.8.49from
MumuTW:fix/8541-combo-cooldown-wait-contract

Conversation

@MumuTW

@MumuTW MumuTW commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Closes #8541.

The contradiction

combo.ts documented the cooldown-aware 429 retry as available to every combo strategy; the gate that reaches it admits only two. The statements sit ~1200 lines apart in the same file.

Source Says
combo.ts:2717 comment "Available to ALL combo strategies (not just quota-share)"
combo.ts:2719 comment "ONE decision path for EVERY strategy"
comboConfig.ts:52 quota-share or auto only

Two tests in tests/unit/serial/combo-quota-share-cooldown-wait-timing.test.ts had been rewritten to match the comment, so they asserted behavior no code path could produce. They are red on the base branch and surface on Unit Tests fast-path, making every PR→release/** that selects them red.

Which side won, and why

#8541 left A (widen the gate) vs B (narrow the contract) open. B. The issue framed this as "comments + test vs implementation", but it is actually test vs test — a third source already asserts the narrow contract directly, and a fourth documents it:

// tests/unit/combo-config.test.ts:328  (carries its own #7360 rationale)
assert.equal(isComboCooldownWaitEligible("priority",   { enabled: true }), false);
assert.equal(isComboCooldownWaitEligible("fill-first", { enabled: true }), false);
// src/lib/resilience/settings.ts:99
// quota-share and auto combos, and only for transient (non quota_exhausted) reasons.

So three sources agree with the gate and only the prose disagreed.

Widening would also not have been a comment fix. resolveComboTargetTimeoutMsForCombo uses the same predicate to raise the per-target timeout floor, so option A would lift the default per-target timeout from 120s to 150s for every strategy, and require deleting the assertions above. That is a deliberate feature change; if the generalization is still wanted it belongs in its own PR, not in base-red cleanup.

Changes

open-sse/services/combo.ts — correct the two scope comments.

Comment-only: git diff filtered to non-comment lines is empty. The file stays at exactly 3640 lines, so the frozen file-size baseline is untouched (an earlier, wordier draft pushed it to 3651 > 3642 and was tightened rather than rebaselined — this file is mid-decomposition under #3501 and should not grow).

Timing test — "short 429 cooldown" — assert priority propagates the 429 without redispatching.

The stub still returns 200 on a hypothetical 2nd dispatch, so asserting 429 is positive proof that no wait fired, not merely an absence check.

Timing test — "quota_exhausted lock" — moved from priority to quota-share, model order flipped.

Two separate defects, neither covered by #8541:

  1. On a non-eligible strategy the branch it guards never opens, so on priority it was asserting against unreachable code — vacuous regardless of A or B.
  2. Since fix(sse): stop combo's aggregated failure response from mixing fields across targets (#8486) #8508 (30709255c) lastStatus is last-write-wins, so the surfaced status and message always come from the same target. The branch requires status === 429; with the 403 target dispatched last, the response crystallizes 403 and the wait is never even considered. That is why this test reports 403 !== 429 — a precondition failure, so its actual subject never ran. Putting the 429 target last makes the allow-list the thing genuinely under test.

Non-vacuousness

The rewritten quota_exhausted test was checked against a positive control before being trusted: swap the lock reason to rate_limit (both targets 429, errorCodes: [429]) and the same shape waits and re-dispatches to 12 upstream calls instead of 2. So the 2 asserted calls are barrier 1 (the allow-list) doing its job, not an absence of wait machinery.

Verification

Command Result
combo-quota-share-cooldown-wait-timing.test.ts 4/4, green on 3 consecutive runs (timing-sensitive, so repeated)
combo-config.test.ts 44/44 — untouched by this diff
combo-cooldown-retry + cooldown-aware-retry-budget + resilience-settings-combo-cooldown-wait + quota-share-strategy + chat-cooldown-aware-retry 77/77
combo-antigravity-missing-project-reset-8486.test.ts green (the #8508 regression guard)
npm run typecheck:core clean
npm run lint clean
npm run check:file-size OK
npm run check:known-symbols OK — 20/20 strategies
npm run check:complexity-ratchets 2169 / 956

Base

Branched off 4053e2314 rather than the release/v3.8.49 tip, matching the other in-flight combo PRs.

check:complexity-ratchets reports 2169/956 vs frozen 2130/951 — byte-identical to the base, so this PR contributes zero. That is the pre-existing base-red owned by #8580. Fast Quality Gates will stay red until #8580 merges; see #8542 for why the fail-fast ordering means a later gate cannot demonstrate its own fix.

Scope note

This closes the Unit Tests fast-path half of the base-red. It does not touch isComboCooldownWaitEligible — no runtime behavior changes in this PR.

…er-inference#8541)

combo.ts documented the cooldown-aware 429 retry as "available to ALL combo
strategies", but the gate feeding it — isComboCooldownWaitEligible() — admits
only quota-share and auto. The two statements sit ~1200 lines apart in the same
file. Two tests in combo-quota-share-cooldown-wait-timing.test.ts had been
rewritten to match the comment, so they asserted behavior no code path could
produce and were red on the base branch.

Resolved toward the NARROW contract (comments + tests follow the gate), because
three sources already agree with it and only the prose disagreed:

- comboConfig.ts:52 — the predicate itself
- tests/unit/combo-config.test.ts:328 — asserts priority/fill-first are NOT
  eligible, with its own cheaper-inference#7360 rationale
- src/lib/resilience/settings.ts:99 — "quota-share and auto combos, and only
  for transient (non quota_exhausted) reasons"

Widening instead would not have been a comment fix: the same predicate raises
the per-target timeout floor via resolveComboTargetTimeoutMsForCombo, so it
would also lift the default per-target timeout from 120s to 150s for every
strategy, and would require deleting the combo-config.test.ts assertions above.
That is a deliberate feature change, not base-red cleanup.

Changes:

- combo.ts: correct the two scope comments. Comment-only — no non-comment line
  is touched, and the file stays at exactly 3640 lines so the frozen file-size
  baseline is unaffected.
- timing test "short 429 cooldown": assert priority propagates the 429 without
  redispatching. The stubbed 2nd dispatch still returns 200, so asserting 429
  is positive proof no wait fired, not merely an absence check.
- timing test "quota_exhausted lock": moved from priority to quota-share and
  the model order flipped. On a non-eligible strategy the branch it guards
  never opens, so it was asserting against unreachable code. The flip is
  required because cheaper-inference#8508 (3070925) made lastStatus last-write-wins so status
  and message always come from the same target — with the 403 target last the
  response crystallizes 403 and the `status === 429` guard never opens. Putting
  the 429 target last makes the allow-list the thing actually under test.

Verified the rewritten quota_exhausted test is non-vacuous with a positive
control: swapping the lock reason to rate_limit (both targets 429, errorCodes
[429]) turns the same shape into 12 upstream dispatches instead of 2.

Local: timing file 4/4 green on 3 consecutive runs; combo-config 44/44
unchanged; combo-cooldown-retry + cooldown-aware-retry-budget +
resilience-settings-combo-cooldown-wait + quota-share-strategy +
chat-cooldown-aware-retry 77/77; 8486 regression guard green. typecheck, lint,
check:file-size, check:known-symbols clean. Complexity 2169/956 — identical to
the base, so the ratchet red is the pre-existing base-red tracked by cheaper-inference#8580.
@MumuTW
MumuTW requested a review from diegosouzapw as a code owner July 25, 2026 19:33
@MumuTW

MumuTW commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Repositioning this PR: it is not an alternative to #8559

I opened this without noticing that @DinonowDev had already sent #8559 for the same issue, five hours earlier. My mistake — I searched the issue tracker for #8541 and never searched open PRs. #8559 is the better piece of work on its own terms: it carries the gate, the settings type, the ResilienceTab UI, i18n and RESILIENCE_GUIDE.md, and it rewrites combo-config.test.ts to iterate ALL_COMBO_STRATEGIES instead of just deleting the assertions that contradicted it.

I framed #8541 as an either/or (widen the gate vs narrow the contract). That framing was wrong. These two PRs are not competing answers to one question — they are two different kinds of change that happen to touch the same lines:

this PR #8559
Kind CI hygiene — closes the Unit Tests fast-path half of the base-red Behavior change — every strategy waits out transient 429s
Runtime effect none (comment-only in combo.ts; git diff filtered to non-comment lines is empty) on by default: comboCooldownWait.enabled stays true, so eligibility becoming strategy-agnostic changes behavior for every combo user, and resolveComboTargetTimeoutMsForCombo lifts the default per-target timeout 120s → 150s along with it
Revert blast radius comments + two tests would also revert the base-red fix bundled with it

The base-red is blocking #8548, #8580, #8582 and #8592 now; whether cooldown-wait should generalize to all strategies is a deliberate product decision. Bundling them means the second decision inherits the first one's time pressure.

Suggested sequencing

this PR lands  →  base-red closed, zero behavior change, blocked PRs unblocked
      ↓
#8559 rebases  →  ships the generalization as its own reviewable behavior change

Nothing here rejects #8559's direction. If the generalization is what the project wants, this PR costs it only a few comment lines and two test assertions to re-flip on rebase. The reverse order puts an on-by-default routing change onto the release branch as a side effect of a red-CI cleanup.

If the maintainers would rather take #8559 first and have it absorb the base-red, I'll close this — say the word and it's gone, no attachment.

One practical note on #8559, offered as help

Its check rollup is 3 entries — Mergify Merge Protections (skipping), Mergify Merge Queue (skipping), semgrep-cloud-platform/scan — with no Fast Quality Gates, no Unit Tests, no Vitest, no ESLint or Docs job. This PR's rollup has 15. So #8559 reads as green only because the suite never ran on it, most likely pending first-time-contributor workflow approval. That is the same hole #8542 records as Evidence 3. Someone with the button should approve its workflows so it gets a real verdict either way — in particular whether the two timing tests in tests/unit/serial/combo-quota-share-cooldown-wait-timing.test.ts pass under the widened gate.

For reference, this PR's own red is Fast Quality Gates failing check:complexity-ratchets at 2169/956 — byte-identical to the base measurement, so it contributes nothing. That is the pre-existing base-red owned by #8580, and #8580 and this PR unblock each other: #8580 fixes the complexity half but is red on the unit tests this PR repairs.

@MumuTW

MumuTW commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #8559 (option A), not option B.

Why this PR cannot land

#8541 left two resolutions open:

This PR (#8614) Merged #8559
Choice B — narrow comments + tests to match the old gate A — widen the gate to match the comments
Runtime no change to isComboCooldownWaitEligible strategy-agnostic: return comboCooldownWait.enabled

#8559 merged at 2026-07-26T06:51:38Z and auto-closed #8541 one second later. On current release/v3.8.49 tip:

  1. Gate is wide — comboConfig.ts keeps _strategy (intentionally unused) and admits every strategy when the feature is enabled.
  2. The narrow-contract premise is gone — combo-config.test.ts now asserts “engages for every strategy when the feature is enabled”, the opposite of what this PR relied on.
  3. The only combo.ts hunk is dead — this PR annotated if (comboCooldownWaitEnabled && status === 429); tip is now if (comboCooldownWaitEnabled && earliestRetryAfter). The conflict cannot resolve toward this PR without rolling back fix(resilience): honor comboCooldownWait for every combo strategy (#8541) #8559’s runtime change.
  4. Timing tests would re-red — tip still has priority: short 429 → waits and re-dispatches (2nd pass 200); this PR rewrites that case to NO wait, 429 propagated.

Salvage: none

The quota_exhausted / model-order concern this PR fixed by switching to quota-share is already addressed upstream the other way: with the gate open for every strategy, the priority branch is reachable and #8559’s timing assertions cover the allow-list path.

No further rebase planned.

@MumuTW MumuTW closed this Jul 26, 2026
@MumuTW
MumuTW deleted the fix/8541-combo-cooldown-wait-contract branch September 5, 2026 10:19
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.

fix(resilience): combo cooldown-wait gate still quota-share/auto only, contradicting its own "ALL strategies" contract (#7301)

1 participant