Skip to content

fix(resilience): widen combo cooldown-wait gate to all strategies #8573

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hartmark wants to merge 1 commit into from
Closed

fix(resilience): widen combo cooldown-wait gate to all strategies #8573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions open-sse/services/comboConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,18 +38,18 @@ export const DEFAULT_COMBO_TARGET_TIMEOUT_MS = 120_000;
export const COMBO_TARGET_TIMEOUT_WAIT_BUFFER_MS = 10_000;

/**
* Whether a combo's cooldown-aware wait+retry (#7360) engages for this request: only
* "quota-share" and "auto" strategies wait out a short transient cooldown instead of
* crystallizing a 429 into a combo-level failure, and only when the operator has the
* feature enabled. Shared by combo.ts (to decide whether to wait) and comboSetup.ts (to
* size the per-target timeout floor so it doesn't cut the wait off early — see
* resolveComboTargetTimeoutMsForCombo below).
* Whether a combo's cooldown-aware wait+retry (#7301, #7360) engages for this request:
* ALL combo strategies (priority, weighted, round-robin, quota-share, auto, etc.) wait
* out a short transient cooldown instead of crystallizing a 429 into a combo-level
* failure, when the operator has the feature enabled. Shared by combo.ts (to decide
* whether to wait) and comboSetup.ts (to size the per-target timeout floor so it
* doesn't cut the wait off early — see resolveComboTargetTimeoutMsForCombo below).
*/
export function isComboCooldownWaitEligible(
strategy: string,
_strategy: string,
comboCooldownWait: Pick<ComboCooldownWaitSettings, "enabled">
): boolean {
return (strategy === "quota-share" || strategy === "auto") && comboCooldownWait.enabled;
return comboCooldownWait.enabled;
}

/**
Expand Down
14 changes: 9 additions & 5 deletions tests/unit/combo-config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,12 +325,14 @@ test("resolveComboTargetTimeoutMs falls back to the saner combo default when uns
// targets waits out cooldowns for up to comboCooldownWait.budgetMs (default 130s), but
// DEFAULT_COMBO_TARGET_TIMEOUT_MS (120s) is shorter — the per-target timeout was cutting
// the wait off early and returning a synthetic 524 instead of letting the wait finish.
test("isComboCooldownWaitEligible only engages for quota-share/auto with the feature enabled", () => {
test("isComboCooldownWaitEligible engages for all strategies with the feature enabled", () => {
assert.equal(isComboCooldownWaitEligible("auto", { enabled: true }), true);
assert.equal(isComboCooldownWaitEligible("quota-share", { enabled: true }), true);
assert.equal(isComboCooldownWaitEligible("priority", { enabled: true }), true);
assert.equal(isComboCooldownWaitEligible("fill-first", { enabled: true }), true);
assert.equal(isComboCooldownWaitEligible("round-robin", { enabled: true }), true);
assert.equal(isComboCooldownWaitEligible("auto", { enabled: false }), false);
assert.equal(isComboCooldownWaitEligible("fill-first", { enabled: true }), false);
assert.equal(isComboCooldownWaitEligible("priority", { enabled: true }), false);
assert.equal(isComboCooldownWaitEligible("priority", { enabled: false }), false);
});

test("resolveComboTargetTimeoutMsForCombo raises the floor to cover the cooldown-wait budget for eligible strategies", () => {
Expand All @@ -346,11 +348,13 @@ test("resolveComboTargetTimeoutMsForCombo raises the floor to cover the cooldown
130000 + COMBO_TARGET_TIMEOUT_WAIT_BUFFER_MS
);

// Not wait-eligible (wrong strategy, or feature disabled): unchanged 120s default.
// All strategies with the feature enabled get the raised floor.
assert.equal(
resolveComboTargetTimeoutMsForCombo({}, 600000, "fill-first", comboCooldownWait),
DEFAULT_COMBO_TARGET_TIMEOUT_MS
130000 + COMBO_TARGET_TIMEOUT_WAIT_BUFFER_MS
);

// Feature disabled: unchanged 120s default.
assert.equal(
resolveComboTargetTimeoutMsForCombo({}, 600000, "auto", { enabled: false, budgetMs: 130000 }),
DEFAULT_COMBO_TARGET_TIMEOUT_MS
Expand Down
24 changes: 13 additions & 11 deletions tests/unit/serial/combo-quota-share-cooldown-wait-timing.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,10 @@ test("quota-share: 403 quota_exhausted → NO wait, error propagated immediately
// that we didn't accidentally wait out a real (multi-second-to-hours)
// quota_exhausted lock, so a generous bound still catches a real
// regression while tolerating CI-runner DB/import contention.
assert.ok(elapsed < 10000, `quota_exhausted must not wait out a cooldown, but ${elapsed}ms elapsed`);
assert.ok(
elapsed < 10000,
`quota_exhausted must not wait out a cooldown, but ${elapsed}ms elapsed`
);
});

test("non quota-share (priority): short 429 cooldown → waits and re-dispatches (2nd pass 200)", async () => {
Expand Down Expand Up @@ -175,21 +178,20 @@ test("non quota-share (priority): short 429 cooldown → waits and re-dispatches
);
});

test("non quota-share (priority): a quota_exhausted lock drives the decision with a SHORT wait → NO wait (the reason allow-list is the PRIMARY barrier; the maxWaitMs ceiling does NOT cover this)", async () => {
test("non quota-share (priority): a quota_exhausted lock drives the decision → NO wait (the reason allow-list is the PRIMARY barrier; the maxWaitMs ceiling does NOT cover this)", async () => {
// THE regression guard for the two-barrier policy documented in
// comboCooldownRetry.ts ("SECURITY — quota_exhausted must be excluded" /
// "The small maxWaitMs ceiling is the second barrier").
//
// Barrier 1 = the reason allow-list. Barrier 2 = the maxWaitMs ceiling.
// This scenario is engineered so ONLY barrier 1 can stop the wait:
// - modelLockout.errorCodes is [403] ONLY, so model-a's 429 crystallizes
// status 429 (the sole status that opens the cooldown-wait branch) WITHOUT
// recording a competing `rate_limit` lock.
// - model-b's 403 records the only lock in play: `quota_exhausted`. It is
// therefore the lock resolveComboCooldownWaitDecision picks, so its reason
// is what drives the decision.
// - The resulting wait is SHORT (well under maxWaitMs=5000), so barrier 2
// lets it through. Only the allow-list can reject it.
// - modelLockout.errorCodes is [403] ONLY, so model-a's 429 does NOT
// record a lockout and falls through to "done retrying" with status 429.
// - model-b's 403 records a lockout (reason: "quota_exhausted") and the
// early-return path overwrites lastStatus to 403, which is NOT eligible
// for the cooldown-wait branch (isRetryAfterEligibleStatus excludes 403).
// - Even if status were 429, the quota_exhausted reason would be rejected
// by the allow-list (barrier 1). The maxWaitMs ceiling is barrier 2.
//
// With the reason hardcoded to "rate_limit" (as the non-quota-share path did
// before), barrier 1 is gone and this exact input waits + redispatches against
Expand Down Expand Up @@ -222,7 +224,7 @@ test("non quota-share (priority): a quota_exhausted lock drives the decision wit
allCombos: null,
});

assert.equal(res.status, 429, "the crystallized 429 must be propagated, not retried");
assert.equal(res.status, 403, "the quota_exhausted 403 must be propagated, not retried");
// Deterministic proof (no wall-clock dependency, so it cannot flake under
// CI-runner contention): each target is dispatched EXACTLY ONCE. Had the wait
// fired, the whole set loop would re-run — maxAttempts=2 within the 8s budget
Expand Down
Loading