fix(ai): keep concurrency caps out of auth rotation
(cherry picked from commit bd6285ad9b16ae6f0a75e336a1c4bf961d3e43c7)
This commit is contained in:
can1357
@@ -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);
|
||||
}
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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"),
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)).
|
||||
|
||||
@@ -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");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
});
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user