From 0be9545312f8b305df9a56ae875019fab403f257 Mon Sep 17 00:00:00 2001 From: Xiangzhe Date: Fri, 14 Aug 2026 14:41:19 -0300 Subject: [PATCH] docs(sse): pin transient-rate-limit suppression and correct combo skip claims Fix round 1 from review of the agentrouter same-request combo skip (#10334): document and test that the new branch deliberately never populates transientRateLimitedProviders (it would re-open the connection the branch just exhausted via combo.ts's allowRateLimitedConnection force-allow), correct a code comment claiming the branch is 429-only (the quota rule also matches a raw 403, which lands in the same set via the same guard), and fix two RESILIENCE_GUIDE.md claims about the same-request skip: it only matches targets that already carry the exhausted connectionId, and the persisted cooldown was never gated on "next request" timing. --- docs/architecture/RESILIENCE_GUIDE.md | 28 ++++++++-- open-sse/services/combo/targetExhaustion.ts | 56 ++++++++++++++----- .../unit/agentrouter-lock-scope-10334.test.ts | 45 +++++++++++++++ 3 files changed, 110 insertions(+), 19 deletions(-) diff --git a/docs/architecture/RESILIENCE_GUIDE.md b/docs/architecture/RESILIENCE_GUIDE.md index 325058940f..da4095e2fc 100644 --- a/docs/architecture/RESILIENCE_GUIDE.md +++ b/docs/architecture/RESILIENCE_GUIDE.md @@ -377,12 +377,28 @@ never `creditsExhausted` — a defense against a future rule pairing scope see the code comment above the branch). - **Same-request combo routing** (`applyComboTargetExhaustion()`, `open-sse/services/combo/targetExhaustion.ts`): the same guard marks the - connection into the in-memory `exhaustedConnections` set so remaining - same-connection targets are skipped for the rest of the *current* request — - not just future requests. Without this, a combo with several legs on the - same exhausted agentrouter account would burn one upstream call per - remaining leg before the persisted cooldown above ever took effect on the - next request. + connection into the in-memory `exhaustedConnections` set, keyed + `${provider}:${connectionId}`. This only skips a remaining SAME-REQUEST + target that *itself already carries that exact `connectionId`* on its own + target object (`getExhaustedTargetSkipReason()`, + `open-sse/services/combo/comboPredicates.ts`, `if (provider && +connectionId)` before the `exhaustedConnections` lookup) — a plain + model-list combo, where sibling targets carry no pinned `connectionId` of + their own and one is only resolved per-dispatch from the response's + `X-OmniRoute-Selected-Connection-Id` header, never hits that key match. For + that common case, the real protection against a remaining leg reusing the + just-exhausted account is NOT this Set — it is the persistence layer above + (the connection's `rateLimitedUntil` is now in the future) combined with + this same guard suppressing `transientRateLimitedProviders` for the + failure (see "Two-stage design" and the code comment on the + `isAgentrouterConnectionQuotaScope` branch in `targetExhaustion.ts`): with + that Set left unmarked, `combo.ts`'s `allowRateLimitedConnection` force-allow + (`open-sse/services/combo.ts:1005-1013`, `:2734-2738`) does NOT kick in for + the provider's remaining legs, so credential selection's `rateLimitedUntil` + filter (`src/sse/services/auth.ts:1238`) is honored normally and a + remaining leg either picks a different, still-eligible agentrouter + connection or fails with no credentials available — it does not force its + way back onto the connection this branch just cooled down. ### Two-stage design: status restatement, then classification diff --git a/open-sse/services/combo/targetExhaustion.ts b/open-sse/services/combo/targetExhaustion.ts index 3d7af3afd7..0325b64b97 100644 --- a/open-sse/services/combo/targetExhaustion.ts +++ b/open-sse/services/combo/targetExhaustion.ts @@ -96,21 +96,51 @@ export function applyComboTargetExhaustion( const { result, sets, log, tag, errorText, structuredError } = opts; const provider = target.provider; - // #10334: agentrouter-exclusive account-wide quota exhaustion ("额度不足", - // restated to 429) must skip remaining SAME-CONNECTION targets within THIS - // request too, not just via the persisted cooldown markAccountUnavailable - // applies for the NEXT request. agentrouter is a passthroughModels provider + // #10334: agentrouter-exclusive account-wide quota exhaustion ("额度不足") + // must skip remaining SAME-CONNECTION targets within THIS request too, not + // just via the persisted cooldown markAccountUnavailable applies for + // whichever leg runs next. agentrouter is a passthroughModels provider // (hasPerModelQuota() === true), so without this branch the classification // below would fall straight through isProviderQuotaExhausted's - // !hasPerModelQuota() guard and markConnectionLevelExhaustion's 429-is-not- - // connection-level guard (CONNECTION_LEVEL_ERROR_STATUSES excludes 429), - // marking nothing — combo would keep burning one upstream call per - // remaining model of the same exhausted account. isAgentrouterConnectionQuotaScope - // is the same guard markAccountUnavailable uses, so both consumers agree on - // exactly which fallbackResult shapes qualify (never a permanent/credits- - // exhausted result, even one carrying ruleScope "connection"). Runs before - // the auth-level (401/403) branch below since it is unaffected by it (this - // path is 429-only) but keeps the diff to a single early return. + // !hasPerModelQuota() guard, and — for the restated-429 case — + // markConnectionLevelExhaustion's connection-level guard (429 is not in + // CONNECTION_LEVEL_ERROR_STATUSES), marking nothing: combo would keep + // burning one upstream call per remaining model of the same exhausted + // account. isAgentrouterConnectionQuotaScope is the same guard + // markAccountUnavailable uses, so both consumers agree on exactly which + // fallbackResult shapes qualify (never a permanent/credits-exhausted + // result, even one carrying ruleScope "connection"). + // + // Runs BEFORE the auth-level (401/403) branch below. This is deliberate, + // not incidental: the "额度不足" rule matches statuses {400, 403, 429} + // (buildAgentrouterRules, providerErrorRules.ts), and Task 1's FORBIDDEN + // pre-check (accountFallback.ts ~1729-1751) surfaces `ruleScope: + // "connection"` for a RAW 403 carrying that body too — so this branch can + // also fire on a 403, not just the restated 429. That is safe: for a 403 + // this branch and markAuthLevelExhaustion below write the SAME set with + // the SAME `${provider}:${connId}` key and both return `true` — they are + // set-equivalent for agentrouter on that status. The Cloudflare-1010 and + // Alibaba free-tier EXEMPTIONS further down in the 401/403 branch cannot + // apply here regardless of ordering: 1010 is a CDN fingerprint rejection + // agentrouter's own text never carries, and the Alibaba exemption is + // gated on isAlibabaModelStudioProvider(provider), which agentrouter is + // not. + // + // Unlike the connection-level/auth-level branches, this path deliberately + // does NOT fall through to markTransientOrConnectionLevel, so + // sets.transientRateLimitedProviders is NEVER populated for this failure. + // That is required, not just incidental: combo.ts (both dispatchers, see + // the `allowRateLimitedConnection` reads keyed off + // transientRateLimitedProviders) uses that set to force-allow reusing a + // rate-limited CONNECTION for the provider's remaining legs — i.e. it + // bypasses the very `rateLimitedUntil` filter this branch (and Task 2's + // markAccountUnavailable) just set. Marking it here would silently + // re-open the account this branch just cooled down. One secondary + // consequence: a SIBLING agentrouter connection that is merely + // rate-limited (not the one this branch exhausted) will also no longer be + // force-allowed for a later leg on the same provider — a remaining leg + // can now resolve to "no credentials available" instead of retrying a + // rate-limited sibling account, which is the intended, safer outcome. if (isAgentrouterConnectionQuotaScope(provider, opts.fallbackResult)) { markAgentrouterConnectionQuotaExhaustion(target, { sets, log, tag }); return true; diff --git a/tests/unit/agentrouter-lock-scope-10334.test.ts b/tests/unit/agentrouter-lock-scope-10334.test.ts index 8a86e04bf1..794f9eea40 100644 --- a/tests/unit/agentrouter-lock-scope-10334.test.ts +++ b/tests/unit/agentrouter-lock-scope-10334.test.ts @@ -434,6 +434,19 @@ test("combo in-request skip: agentrouter connection-scope quota marks exhaustedC 0, "must NOT exhaust the whole provider — sibling agentrouter connections keep their own quota" ); + // Important finding (review round 1): unlike markConnectionLevelExhaustion's + // path, this branch must NEVER populate transientRateLimitedProviders. That + // set drives combo.ts's `allowRateLimitedConnection` force-allow + // (open-sse/services/combo.ts:1005-1013 and :2734-2738), which bypasses the + // `rateLimitedUntil` filter in credential selection (src/sse/services/auth.ts:1238) + // for the provider's remaining legs this request. Marking it here would + // silently re-open the very connection Task 2's markAccountUnavailable (and + // this branch) just cooled down. + assert.equal( + sets.transientRateLimitedProviders.size, + 0, + "must NOT mark transientRateLimitedProviders — that would force-allow reusing the connection this branch just exhausted" + ); }); test("combo in-request skip: no connectionId falls back to whole-provider exhaustion", () => { @@ -524,6 +537,38 @@ test("guard: a credits-exhausted agentrouter fallbackResult with scope connectio assert.equal(sets.exhaustedProviders.size, 0); }); +// Minor finding (review round 1): the connection-scope branch is NOT +// 429-only. The "额度不足" rule (buildAgentrouterRules, providerErrorRules.ts) +// matches statuses {400, 403, 429}, and Task 1's FORBIDDEN pre-check +// (accountFallback.ts ~1729-1751, gated on honorsRuleLockScope) surfaces +// `ruleScope: "connection"` for a RAW 403 carrying that body too — before the +// generic apikey FORBIDDEN early-return, and before markAuthLevelExhaustion +// below ever sees it. Pin that a raw 403 with this shape takes the SAME +// connection-scope branch (not markAuthLevelExhaustion) and lands in the SAME +// set with the SAME key — the two paths are set-equivalent for agentrouter on +// this status, so this is not a behavior change, just documenting which +// branch actually runs. +test("combo in-request skip: a RAW 403 with connection-scope quota also takes this branch (not markAuthLevelExhaustion)", () => { + const sets = comboSets(); + const exhausted = applyComboTargetExhaustion(comboTarget(), { + ...comboBaseOpts, + result: { status: 403 }, + fallbackResult: CONNECTION_SCOPE_FALLBACK_RESULT, + sets, + }); + assert.equal(exhausted, true); + assert.ok( + sets.exhaustedConnections.has("agentrouter:conn-agentrouter-1"), + "a raw 403 carrying ruleScope=connection must exhaust the connection just like the restated-429 case" + ); + assert.equal(sets.exhaustedProviders.size, 0); + assert.equal( + sets.transientRateLimitedProviders.size, + 0, + "same suppression as the 429 case — must not force-allow reusing this connection" + ); +}); + // ─── Invariant sentinel ───────────────────────────────────────────────── // classifyProviderError (open-sse/services/errorClassifier.ts) must NEVER // classify agentrouter's restated 429 body ("用户额度不足") as quota_exhausted.