diff --git a/docs/architecture/RESILIENCE_GUIDE.md b/docs/architecture/RESILIENCE_GUIDE.md index 5aaba73652..98629da7db 100644 --- a/docs/architecture/RESILIENCE_GUIDE.md +++ b/docs/architecture/RESILIENCE_GUIDE.md @@ -318,14 +318,44 @@ excludeMarkers, defaultRetryAfterMs}`), matched via `applyStatusRestatement()`. - Retry eligibility: `429` is in `RETRY_AFTER_ELIGIBLE_STATUSES` (`open-sse/services/combo/unavailableRetryGate.ts`), so a restated error carries a real retry window instead of surfacing as a dead `403`. +- The synthetic `60s` `defaultRetryAfterMs` (`upstreamStatusRestatement.ts`) + is only what the restated response tells the **client**; it is not itself + the connection's internal cooldown/lockout duration — that is governed + separately by whichever mechanism actually handles the restated error + (Connection Cooldown's escalating backoff, §2, base `3s` for API-key + providers; or Model Lockout, §3, for per-model-quota providers like + agentrouter). The router can become eligible to retry internally sooner + than the 60s window it advertises to the client — intentional headroom, + not a bug. Permanent errors (agentrouter's `无权访问模型` — no access to this model) are NEVER restated: `excludeMarkers` vetoes the rule even when `textMarkers` hit, so the error keeps its original status and nothing retries it forever. A separate provider classification rule (`agentrouter-model-access-denied` in `open-sse/config/providerErrorRules.ts`) -locks only the affected model instead (Model Lockout, §3), so the connection -keeps serving other models. +declares an `auth_error`/scope-`model` match for this text, but it does not +fire on the live production path today: the rule only matches `status === +403`, and `checkFallbackError`'s apikey-category `FORBIDDEN` branch +(`open-sse/services/accountFallback.ts`) returns early for a plain 403 +*before* the provider-rule lookup ever runs. In practice a `无权访问模型` 403 +is handled the same way as the base apikey-provider 403 path (see Connection +Cooldown, §2), not as a 6h model lockout. The rule still exists as a +declarative classification consumable by future callers of `classifyError` +with context — wiring it into the production `checkFallbackError` path is +tracked as a follow-up, not yet done. + +Restated quota errors (`额度不足`) do reach a provider rule in production +(`agentrouter-user-quota-exhausted`, scope `"connection"`), but `scope` on +`ProviderErrorRuleMatch` is currently informational — the persistence path +(`checkFallbackError` → `combo.ts`) only consumes `reason` and `cooldownMs`, +never `scope`. What actually happens for agentrouter (`passthroughModels: +true` → `hasPerModelQuota()` returns `true`) is a **per-model** lockout via +`recordModelLockoutFailure()`: the connection itself is never cooled down for +this error (`combo.ts` skips `recordProviderCooldown` for 429 when +`hasPerModelQuota` is true), so other models on the same account keep being +tried — each one burns one call and its own lockout before combo routing +moves on. Honoring `scope` end-to-end (so a `"connection"` match actually +locks the connection) is tracked as a follow-up. ### Two-stage design: status restatement, then classification diff --git a/open-sse/config/providerErrorRules.ts b/open-sse/config/providerErrorRules.ts index ae2f495573..dd59e991bc 100644 --- a/open-sse/config/providerErrorRules.ts +++ b/open-sse/config/providerErrorRules.ts @@ -29,7 +29,15 @@ export type ProviderErrorRule = { export type ProviderErrorRuleMatch = { reason: ConfiguredErrorReason; - /** Default "provider" — lock the whole connection so other providers take over. */ + /** + * Intended lock scope. NOTE: this field is currently INFORMATIONAL — no + * consumer of `getProviderErrorRuleMatch` (checkFallbackError, combo.ts) + * reads `scope` today; only `reason` and `cooldownMs` are consulted. The + * actual lock scope applied at runtime is decided independently by each + * call site (e.g. `hasPerModelQuota()` deciding model- vs connection-level + * lockout). Honoring this field end-to-end is tracked as a follow-up — + * see `docs/architecture/RESILIENCE_GUIDE.md` §7. + */ scope: "model" | "provider" | "connection"; /** Optional explicit cooldown; falls back to the existing per-reason defaults. */ cooldownMs?: number; @@ -189,8 +197,22 @@ function buildOpenrouterRules(): ProviderErrorRule[] { // hands these rules the full error text instead of just {code, type}. // - "额度不足": account-wide temporary quota → quota_exhausted, scope // "connection" (mirror of the Opencode account-wide rationale above). -// - "无权访问模型": this key permanently lacks access to ONE model → lock only -// the model so the connection keeps serving the rest (Model Lockout tier). +// NOTE: `scope` on ProviderErrorRuleMatch is currently informational — +// checkFallbackError/combo.ts only consume `reason` and `cooldownMs`, not +// `scope`. For agentrouter specifically (passthroughModels: true → +// hasPerModelQuota() is true), this quota_exhausted match actually +// resolves to a PER-MODEL lockout (recordModelLockoutFailure), not a +// connection-wide lock — other models on the same account keep being +// tried by combo routing (each burning one call) until they lock out +// individually. Honoring `scope` end-to-end is tracked as a follow-up. +// - "无权访问模型": declares auth_error/scope "model" (intent: lock only the +// model so the connection keeps serving the rest — Model Lockout tier). +// This rule does NOT fire on the production path today: it only matches +// `status === 403`, but checkFallbackError's apikey FORBIDDEN branch +// returns early for a plain 403 before this rule is ever consulted (see +// the note above). A live `无权访问模型` 403 is handled like the base +// apikey-provider 403 today. Wiring this rule into that path is tracked +// as a follow-up. function buildAgentrouterRules(): ProviderErrorRule[] { const AGENTROUTER_ERROR_STATUSES = new Set([400, 403, 429]); return [ diff --git a/open-sse/config/upstreamStatusRestatement.ts b/open-sse/config/upstreamStatusRestatement.ts index 75f84ecf72..ccadf2d704 100644 --- a/open-sse/config/upstreamStatusRestatement.ts +++ b/open-sse/config/upstreamStatusRestatement.ts @@ -29,6 +29,14 @@ * "insufficient_quota" are in CREDITS_EXHAUSTED_SIGNALS * (accountFallback.ts) and would flip the connection into a terminal * credits_exhausted state — never use them as markers here. + * + * Accepted trade-off: matching only on response body text means a + * legitimate 400 whose body ECHOES user-supplied content containing a + * marker (e.g. a prompt that itself contains "额度不足") would be restated to + * 429 and lose the combo's 400 stop-guard. This is treated as an acceptable + * risk because these markers are rare outside a genuine upstream error; + * keeping markers short, provider-specific, and non-generic (as above) + * minimizes false-positive restatement. */ export type UpstreamStatusRestatementRule = { diff --git a/tests/unit/agentrouter-error-rules.test.ts b/tests/unit/agentrouter-error-rules.test.ts index d9cecd78fd..2315a3076c 100644 --- a/tests/unit/agentrouter-error-rules.test.ts +++ b/tests/unit/agentrouter-error-rules.test.ts @@ -2,14 +2,27 @@ import test from "node:test"; import assert from "node:assert/strict"; /** - * agentrouter.org quota model: - * - "额度不足" (quota insufficient) is ACCOUNT-wide and temporary → lock the - * connection (scope "connection") so combo routing moves to another - * account/provider instead of hammering the same key. - * - "无权访问模型" (no access to this model) is permanent PER MODEL → lock only - * the model (scope "model"); the connection keeps serving other models. + * agentrouter.org quota model, as declared by the `providerErrorRules.ts` + * classification layer: + * - "额度不足" (quota insufficient) is ACCOUNT-wide and temporary → the rule + * declares scope "connection". + * - "无权访问模型" (no access to this model) is permanent PER MODEL → the rule + * declares scope "model". * Status matching accepts both the raw upstream 403 AND the restated 429 * (upstreamStatusRestatement.ts rewrites 403→429 before classification). + * + * IMPORTANT — `scope` above is what the rule DECLARES, not what production + * enforces: `ProviderErrorRuleMatch.scope` is not consumed by + * checkFallbackError/combo.ts today (only `reason`/`cooldownMs` are). For + * agentrouter (passthroughModels: true → hasPerModelQuota() true), the + * quota_exhausted match actually resolves to a PER-MODEL lockout in + * production, not a connection-wide lock — other models on the same account + * keep being tried by combo routing until they lock out individually. And + * the "无权访问模型" rule never reaches production traffic at all today: it + * only matches raw `status === 403`, but checkFallbackError's apikey + * FORBIDDEN branch returns early for a plain 403 before any provider rule is + * consulted (see A7). See `docs/architecture/RESILIENCE_GUIDE.md` §7 for the + * full writeup and the tracked follow-up to honor `scope`. */ const { providerRuleRegistry, getProviderErrorRuleMatch } = await import( @@ -40,7 +53,7 @@ test("A3: quota body also matches the raw (pre-restatement) 403", () => { assert.equal(match.reason, "quota_exhausted"); }); -test("A4: 无权访问模型 → auth_error scope model (model lockout, not connection)", () => { +test("A4: 无权访问模型 → auth_error scope model, at the RULE layer only (getProviderErrorRuleMatch directly) — this rule never receives production traffic (see A7): checkFallbackError's apikey FORBIDDEN branch returns early for a plain 403 before reaching this rule", () => { const match = getProviderErrorRuleMatch("agentrouter", 403, {}, { error: { message: "无权访问模型 claude-sonnet-4" }, }); @@ -49,7 +62,7 @@ test("A4: 无权访问模型 → auth_error scope model (model lockout, not conn assert.equal(match.scope, "model"); }); -test("A5: classifyError integration — quota text wins over the 403→AUTH_ERROR status fallback", () => { +test("A5: classifyError layer guard — quota text wins over the 403→AUTH_ERROR status fallback (classifyError itself has no production caller today; the production guard is A6/checkFallbackError)", () => { const reason = classifyError(403, "用户额度不足", { provider: "agentrouter", headers: {},