From a6eafcbf6bc92d4f0c8e7f4d271d7f6b5be96b87 Mon Sep 17 00:00:00 2001 From: can1357 Date: Wed, 5 Aug 2026 02:30:15 +0200 Subject: [PATCH] fix(ai): keep concurrency caps out of auth rotation (cherry picked from commit bd6285ad9b16ae6f0a75e336a1c4bf961d3e43c7) --- packages/ai/src/error/auth-classify.ts | 9 +-- packages/ai/src/error/flags.ts | 5 +- packages/ai/src/error/rate-limit.ts | 11 ++- packages/ai/test/auth-retry.test.ts | 10 +-- packages/ai/test/rate-limit-utils.test.ts | 5 ++ packages/coding-agent/CHANGELOG.md | 1 + .../coding-agent/src/session/agent-session.ts | 15 ++-- ...session-copilot-credential-removal.test.ts | 72 ------------------- packages/utils/CHANGELOG.md | 1 + 9 files changed, 35 insertions(+), 94 deletions(-) delete mode 100644 packages/coding-agent/test/agent-session-copilot-credential-removal.test.ts diff --git a/packages/ai/src/error/auth-classify.ts b/packages/ai/src/error/auth-classify.ts index 575bcc1a1..a253b0ad8 100644 --- a/packages/ai/src/error/auth-classify.ts +++ b/packages/ai/src/error/auth-classify.ts @@ -1,6 +1,6 @@ import { extractHttpStatusFromError } from "@oh-my-pi/pi-utils"; import { isOAuthExpiry, isUsageLimit } from "./flags"; -import { isUsageLimitOutcome } from "./rate-limit"; +import { isConcurrencyCapExclusion, isUsageLimitOutcome } from "./rate-limit"; /** * Whether an OAuth refresh failure is definitive (the credential must be @@ -38,9 +38,10 @@ export function isAuthRetryableError(error: unknown): boolean { if (isUsageLimit(error)) return true; if (isInvalidatedOAuthTokenError(error)) return true; const httpStatus = extractHttpStatusFromError(error); - if (httpStatus === 401 || httpStatus === 403) return true; const message = error instanceof Error ? error.message : typeof error === "string" ? error : undefined; const embeddedStatus = message ? extractHttpStatusFromError({ message }) : undefined; - if (embeddedStatus === 401 || embeddedStatus === 403) return true; - return isUsageLimitOutcome(httpStatus ?? embeddedStatus, message); + const status = httpStatus ?? embeddedStatus; + if (isConcurrencyCapExclusion(status, message)) return false; + if (status === 401 || status === 403) return true; + return isUsageLimitOutcome(status, message); } diff --git a/packages/ai/src/error/flags.ts b/packages/ai/src/error/flags.ts index 9426e44e7..c72ba9074 100644 --- a/packages/ai/src/error/flags.ts +++ b/packages/ai/src/error/flags.ts @@ -430,7 +430,10 @@ export function classify(error: unknown, api?: Api): number { if (code === "overloaded_error" || code === "rate_limit_error") { linkKinds |= Flag.Transient; } - if (codeStatus === 401 || codeStatus === 403) { + if ( + (codeStatus === 401 || codeStatus === 403) && + !(codeStatus === 403 && parseRateLimitReason(link.message) === "CONCURRENT_LIMIT") + ) { linkKinds |= Flag.AuthFailed; } else if (codeStatus === 429) { if ((linkKinds & Flag.UsageLimit) === 0) { diff --git a/packages/ai/src/error/rate-limit.ts b/packages/ai/src/error/rate-limit.ts index 7be4e71d9..972a533ec 100644 --- a/packages/ai/src/error/rate-limit.ts +++ b/packages/ai/src/error/rate-limit.ts @@ -29,13 +29,10 @@ const OPENROUTER_DAILY_FREE_LIMIT_PATTERN = /\bfree[-_ ]models[-_ ]per[-_ ]day\b // model capacity, while quota/rate-limit/server wording remains authoritative. const RESOURCE_EXHAUSTED_PATTERN = /resource.?exhausted/gi; const CONCURRENT_LIMIT_PATTERN = - // Require an actual cap signal (limit/quota/exceeded/reached) near "concurrent". - // The first two alternatives rely on `\b`, which treats `_` as a word char, so - // structured snake_case codes ("concurrent_limit_exceeded", - // "concurrent_requests_limit_reached", "concurrency_quota_exceeded") need the - // third alternative. Bare space-separated concurrency feature rejections stay - // excluded because they neither use `[-_]` nor carry a cap keyword. - /\bconcurren\w*\b[^\n]{0,60}\b(?:limit|quota|exceed\w*|reach\w*)\b|\b(?:limit|quota|exceed\w*|reach\w*)\b[^\n]{0,60}\bconcurren\w*\b|\bconcurren[a-z]*[-_](?:[a-z]+[_-])*(?:limit|quota|exceed\w*|reach\w*)/i; + // Require an actual cap signal near "concurrent". "Too many concurrent + // requests" is itself a cap signal; bare feature rejections such as + // "concurrent invocation is not supported" remain excluded. + /\btoo many\s+concurren\w*\s+(?:requests?|invocations?)\b|\bconcurren\w*\b[^\n]{0,60}\b(?:limit|quota|exceed\w*|reach\w*)\b|\b(?:limit|quota|exceed\w*|reach\w*)\b[^\n]{0,60}\bconcurren\w*\b|\bconcurren[a-z]*[-_](?:[a-z]+[_-])*(?:limit|quota|exceed\w*|reach\w*)/i; const ACCOUNT_SCOPED_403_PATTERN = // The bare "limit will reset" / "will reset in" phrasing also appears on // statusless per-minute transients ("Rate limit will reset in 30 seconds"), diff --git a/packages/ai/test/auth-retry.test.ts b/packages/ai/test/auth-retry.test.ts index ad8e9bcb7..e438dcf04 100644 --- a/packages/ai/test/auth-retry.test.ts +++ b/packages/ai/test/auth-retry.test.ts @@ -275,7 +275,7 @@ describe("withAuth", () => { expect(contexts.map(ctx => ctx.lastChance)).toEqual([false, true, true, true]); }); - it("does not directly rotate through every sibling on a 403 concurrency cap", async () => { + it("leaves a 403 concurrency cap to the transient retry layer", async () => { const keys: string[] = []; const contexts: ApiKeyResolveContext[] = []; const pool = ["k0", "k1", "k2", "k3"]; @@ -295,10 +295,10 @@ describe("withAuth", () => { ), ).rejects.toBe(concurrencyCap); - // The concurrency classification takes precedence over plain-403 direct - // rotation: refresh once, then take only the legacy sibling switch. - expect(keys).toEqual(["k0", "k1", "k2"]); - expect(contexts.map(ctx => ctx.lastChance)).toEqual([false, false, true]); + // The outer transient retry/backoff layer owns concurrency caps. The auth + // retry layer must not refresh or select a sibling credential. + expect(keys).toEqual(["k0"]); + expect(contexts.map(ctx => ctx.lastChance)).toEqual([false]); }); it("surfaces the last 403 when every sibling is denied", async () => { diff --git a/packages/ai/test/rate-limit-utils.test.ts b/packages/ai/test/rate-limit-utils.test.ts index 8d2feb84c..15e5b9bab 100644 --- a/packages/ai/test/rate-limit-utils.test.ts +++ b/packages/ai/test/rate-limit-utils.test.ts @@ -65,6 +65,8 @@ describe("parseRateLimitReason", () => { expect(parseRateLimitReason("concurrent_limit_exceeded")).toBe("CONCURRENT_LIMIT"); expect(parseRateLimitReason("concurrent_requests_limit_reached")).toBe("CONCURRENT_LIMIT"); expect(parseRateLimitReason("concurrency_quota_exceeded")).toBe("CONCURRENT_LIMIT"); + expect(parseRateLimitReason("Too many concurrent requests")).toBe("CONCURRENT_LIMIT"); + expect(parseRateLimitReason("Too many concurrent invocations")).toBe("CONCURRENT_LIMIT"); expect(parseRateLimitReason("Rate limit reached for gpt-4o")).toBe("RATE_LIMIT_EXCEEDED"); expect(parseRateLimitReason("Your quota will reset at 07-28")).toBe("QUOTA_EXHAUSTED"); }); @@ -351,6 +353,9 @@ describe("isUsageLimitOutcome", () => { expect(isConcurrencyCapExclusion(undefined, message)).toBe(true); expect(isConcurrencyCapExclusion(402, message)).toBe(false); expect(isConcurrencyCapExclusion(403, "Forbidden")).toBe(false); + const classified = classify(new ProviderHttpError(message, 403)); + expect(is(classified, Flag.AuthFailed)).toBe(false); + expect(is(classified, Flag.Transient)).toBe(true); }); // The same bare concurrency wording can reach turn recovery without a diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index e018a8f43..45ea58480 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -13,6 +13,7 @@ ### Fixed +- Retried concurrent-request caps with a short backoff without deleting valid Copilot credentials or rotating through sibling accounts. - Reduced streaming CPU usage by coalescing the cumulative `message_update` deltas of a turn at the event-controller dispatch boundary: at most one streaming-state rebuild runs per ~33ms window instead of one per token, cutting the per-token handler work that dominated the CPU profile of streaming sessions (especially at high token rates) while preserving per-delta speech output. Subscriber dispatch is serialized so a rapid stream tail (`message_update` → `message_end` → `agent_end`) cannot overtake the coalesced flush. ([#7443](https://github.com/can1357/oh-my-pi/issues/7443)) - Fixed translated MCP importers (Claude Code, Cursor, Gemini CLI, Windsurf, VS Code) silently dropping a server's `enabled: false` flag, so a server disabled at the source config stayed mounted; the flag is now propagated and honored like Codex, OpenCode, and native `mcp.json`. These importers now also load project entries before same-named user entries (matching native/Codex) so a project `enabled: false` suppresses a same-named user server ([#7652](https://github.com/can1357/oh-my-pi/issues/7652)). - Removed the per-call `model` override from the eval `agent()` helper (all runtimes), completing the earlier task-tool removal (`9f8aa87dbf`). Subagents always use their selected agent's frontmatter model and settings; a legacy `model` argument is silently ignored, so an explicit `model: "default"` can no longer route children onto the parent session model ([#6438](https://github.com/can1357/oh-my-pi/issues/6438)). diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index d77535165..5cfd1169f 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -2693,13 +2693,18 @@ export class AgentSession { // Invalidate GitHub Copilot credentials on a hard auth failure (401, or an // expired/revoked token) so stale tokens aren't reused on the next request. - // A 403 whose body is a recognized account usage cap is NOT an auth failure — - // the credential is valid, only temporarily blocked until its reset window — - // so gate the removal on the absence of Flag.UsageLimit and keep the - // still-valid credential around until the cap resets. + // Account usage caps and concurrency caps leave the credential valid: the + // former rotates until its reset window, while the latter is retried after + // a short backoff without touching the credential pool. if (msg.stopReason === "error" && msg.provider === "github-copilot") { const errorId = AIError.classifyMessage(msg); - if (AIError.is(errorId, AIError.Flag.AuthFailed) && !AIError.is(errorId, AIError.Flag.UsageLimit)) { + const isConcurrencyCap = + AIError.parseRateLimitReason(msg.errorMessage ?? "") === "CONCURRENT_LIMIT"; + if ( + AIError.is(errorId, AIError.Flag.AuthFailed) && + !AIError.is(errorId, AIError.Flag.UsageLimit) && + !isConcurrencyCap + ) { await this.#modelRegistry.authStorage.remove("github-copilot"); } } diff --git a/packages/coding-agent/test/agent-session-copilot-credential-removal.test.ts b/packages/coding-agent/test/agent-session-copilot-credential-removal.test.ts deleted file mode 100644 index 24547b579..000000000 --- a/packages/coding-agent/test/agent-session-copilot-credential-removal.test.ts +++ /dev/null @@ -1,72 +0,0 @@ -import { expect, it, spyOn } from "bun:test"; -import { Agent } from "@oh-my-pi/pi-agent-core"; -import type { AssistantMessage } from "@oh-my-pi/pi-ai"; -import { getBundledModel } from "@oh-my-pi/pi-catalog/models"; -import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; -import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; -import { AgentSession } from "@oh-my-pi/pi-coding-agent/session/agent-session"; -import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage"; -import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; -import { TempDir } from "@oh-my-pi/pi-utils"; - -it("removes a Copilot credential for 401 but retains it for a 403 account cap", async () => { - const tempDir = TempDir.createSync("@pi-copilot-credential-removal-"); - const authStorage = await AuthStorage.create(tempDir.join("testauth.db")); - const modelRegistry = new ModelRegistry(authStorage, tempDir.join("models.yml")); - const model = getBundledModel("anthropic", "claude-sonnet-4-5"); - if (!model) throw new Error("Expected bundled Anthropic test model to exist"); - - const agent = new Agent({ - initialState: { model, systemPrompt: ["Test"], tools: [], messages: [] }, - }); - const session = new AgentSession({ - agent, - sessionManager: SessionManager.inMemory(), - settings: Settings.isolated({ "compaction.enabled": false }), - modelRegistry, - }); - const removeSpy = spyOn(authStorage, "remove").mockResolvedValue(undefined); - - try { - const unauthorized: AssistantMessage = { - role: "assistant", - content: [], - api: "openai-responses", - provider: "github-copilot", - model: "gpt-5-mini", - stopReason: "error", - errorMessage: "GitHub Copilot authentication failed (HTTP 401).", - errorStatus: 401, - usage: { - input: 0, - output: 0, - cacheRead: 0, - cacheWrite: 0, - totalTokens: 0, - cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, - }, - timestamp: Date.now(), - }; - agent.emitExternalEvent({ type: "message_end", message: unauthorized }); - agent.emitExternalEvent({ type: "agent_end", messages: [unauthorized] }); - await session.waitForIdle(); - expect(removeSpy).toHaveBeenCalledWith("github-copilot"); - - removeSpy.mockClear(); - const accountCap: AssistantMessage = { - ...unauthorized, - errorMessage: "Reached overall message rate limit. Your limit will reset in 13 minutes.", - errorStatus: 403, - timestamp: Date.now(), - }; - agent.emitExternalEvent({ type: "message_end", message: accountCap }); - agent.emitExternalEvent({ type: "agent_end", messages: [accountCap] }); - await session.waitForIdle(); - expect(removeSpy).not.toHaveBeenCalled(); - } finally { - await session.dispose(); - removeSpy.mockRestore(); - authStorage.close(); - tempDir.removeSync(); - } -}); diff --git a/packages/utils/CHANGELOG.md b/packages/utils/CHANGELOG.md index 14f518d3c..f970f939f 100644 --- a/packages/utils/CHANGELOG.md +++ b/packages/utils/CHANGELOG.md @@ -9,6 +9,7 @@ ### Fixed - Honor the current process `PATH` when caching executable lookups, preventing stale tool paths after environment reloads. +- Parsed account-cap reset windows such as “Your limit will reset in 13 minutes” so credential backoff honors the provider's full reset duration. ## [17.2.6] - 2026-08-03