mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-15 03:32:21 +03:00
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.
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user