From 854a25d6303c1b549a2d8bb2832a354d2d67986d Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 04:00:49 -0500 Subject: [PATCH 1/8] test(isolation): stop the suite writing to the real account store The suite drives real AccountManager instances and calls loadAccounts / saveAccounts without overriding storage, so `npm test` resolved ~/.opencode/oc-codex-multi-auth-accounts.json against the developer's own home and wrote fixtures over it. On 2026-09-17 that replaced a live five-account ChatGPT pool with two test records (accountId "test-account" and "new-import", addedAt 1ms and 2ms past the epoch) and took the running opencode fleet down for roughly 40 minutes: live processes reported "No Codex accounts configured. Run `opencode auth login`." and "All 2 account(s) are rate-limited". Recovery needed a 15-day-old backup, and five of the seven accounts it restored came back with dead refresh tokens. The redirect has to be `test.env` rather than a setupFiles entry. vitest applies test.env in the worker before it imports any test module, while a setup file runs once the module graph is already loading, which is too late for lib/config.ts, lib/accounts/recovery.ts, lib/logger.ts, lib/prompts/codex.ts, lib/prompts/opencode-codex.ts and lib/auto-update-checker.ts: each captures homedir() at module scope. Verified empirically rather than assumed. With the real HOME inherited on the command line, LOG_DIR still resolves inside the sandbox. A redirect alone is one refactor away from lapsing silently, so the storage layer also fails closed. Under VITEST, any account-storage write, unlink, or lock-sidecar probe that resolves inside the real home throws TEST_HOME_ESCAPE instead of proceeding. The check compares against os.userInfo().homedir, which reads the passwd entry rather than $HOME and so still names the real home after the redirect; the sandbox cannot spoof it. It is inert outside vitest. The guard runs before `acquireOrDetectLock`, not inside the try that wraps it, because that probe writes a lock sidecar next to the accounts file and would therefore touch the real store even on a pure read, and because the surrounding catch would swallow the refusal. test/paths.test.ts needed fixing as a consequence rather than by coincidence. Its two lookalike-prefix cases build a sibling of an allowed root and require it to be outside all three roots; with HOME under tmpdir(), every sibling of home is a child of tmpdir(), which resolvePath legitimately allows, so the assertions stopped throwing. Mocking homedir() and tmpdir() to fixed unrelated roots makes them independent of where the real HOME points. They pass with HOME both inside and outside tmpdir(), which also means the isolated home can keep living under tmpdir(). AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- lib/storage/load-save.ts | 38 +++++++++++++++++++- lib/storage/paths.ts | 2 +- test/paths.test.ts | 19 ++++++++++ test/test-home-isolation.test.ts | 62 ++++++++++++++++++++++++++++++++ vitest.config.ts | 24 +++++++++++++ 5 files changed, 143 insertions(+), 2 deletions(-) create mode 100644 test/test-home-isolation.test.ts diff --git a/lib/storage/load-save.ts b/lib/storage/load-save.ts index fddcbe71..ef20c727 100644 --- a/lib/storage/load-save.ts +++ b/lib/storage/load-save.ts @@ -25,7 +25,7 @@ import { AnyAccountStorageSchema, getValidationErrors } from "../schemas.js"; import { renameWithWindowsRetry } from "./atomic-write.js"; import { formatStorageErrorHint, StorageError } from "./errors.js"; import { normalizeAccountStorage } from "./normalize.js"; -import { getConfigDir } from "./paths.js"; +import { getConfigDir, isWithinDirectory } from "./paths.js"; import { getCurrentLegacyProjectStoragePath, getCurrentProjectRoot, @@ -142,6 +142,10 @@ async function checkWorktreeLockForCurrentStorage( }); return; } + // Before the probe, not inside the try: `acquireOrDetectLock` writes a lock + // sidecar next to the accounts file, so a leaked HOME would touch the real + // store here even on a pure read, and this function's catch would hide it. + assertTestRunNeverTouchesRealHome(path); try { const result = await acquireOrDetectLock(path); if (!result.acquired && result.foreign) { @@ -171,6 +175,36 @@ async function checkWorktreeLockForCurrentStorage( } } +/** + * Refuse to mutate account storage inside the developer's real home while the + * test suite is running. + * + * `vitest.config.ts` redirects HOME to a sandbox before any module loads, but a + * test that restores the captured real HOME, or a future regression in that + * config, would otherwise write fixtures straight over live ChatGPT + * credentials. `os.userInfo()` reads the passwd entry instead of `$HOME`, so it + * still names the real home after the redirect and gives the check something + * the sandbox cannot spoof. Inert outside vitest. + */ +function assertTestRunNeverTouchesRealHome(path: string): void { + if (!process.env.VITEST) return; + + let realHome: string; + try { + realHome = os.userInfo().homedir; + } catch { + return; + } + if (!realHome || !isWithinDirectory(realHome, path)) return; + + throw new StorageError( + `Refusing to write account storage inside the real home directory during a test run: ${path}`, + "TEST_HOME_ESCAPE", + path, + "A test resolved account storage against the developer's real home. Point HOME at a temp directory for the whole vitest process (see vitest.config.ts) instead of overriding it per test.", + ); +} + async function ensureGitignore(storagePath: string): Promise { if (!getCurrentStoragePath()) return; @@ -537,6 +571,7 @@ async function loadAccountsInternal( * Callers must already be inside withStorageLock when using this helper directly. */ async function writeAccountsToPathUnlocked(path: string, storage: AccountStorageV3): Promise { + assertTestRunNeverTouchesRealHome(path); const uniqueSuffix = `${Date.now()}.${Math.random().toString(36).slice(2, 8)}`; const tempPath = `${path}.${uniqueSuffix}.tmp`; @@ -759,6 +794,7 @@ export async function clearAccounts(): Promise { let jsonCleared = true; try { const path = getStoragePath(); + assertTestRunNeverTouchesRealHome(path); await fs.unlink(path); } catch (error) { const code = (error as NodeJS.ErrnoException).code; diff --git a/lib/storage/paths.ts b/lib/storage/paths.ts index 1dc45bc6..ad2e11ce 100644 --- a/lib/storage/paths.ts +++ b/lib/storage/paths.ts @@ -81,7 +81,7 @@ function normalizePathForComparison(filePath: string): string { return process.platform === "win32" ? resolvedPath.toLowerCase() : resolvedPath; } -function isWithinDirectory(baseDir: string, targetPath: string): boolean { +export function isWithinDirectory(baseDir: string, targetPath: string): boolean { const normalizedBase = normalizePathForComparison(baseDir); const normalizedTarget = normalizePathForComparison(targetPath); const rel = relative(normalizedBase, normalizedTarget); diff --git a/test/paths.test.ts b/test/paths.test.ts index ca215ec8..c3a88ceb 100644 --- a/test/paths.test.ts +++ b/test/paths.test.ts @@ -2,10 +2,29 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { homedir, tmpdir } from "node:os"; import path from "node:path"; +// `resolvePath` guards against a path that merely shares a string prefix with +// an allowed root, so the lookalike cases below build a sibling of one root and +// require it to be outside all three. Against the real layout that only holds +// by accident: the suite runs with HOME redirected under the temp dir, which +// makes every sibling of home a child of tmpdir() and turns the assertion into +// a no-op. Fixed roots keep these cases meaningful wherever HOME points. +// +// This mock and the isolated HOME in `vitest.config.ts` are load-bearing +// together. Removing the mock as redundant silently turns both lookalike +// assertions into no-ops rather than failing them. +const FAKE_HOME = path.resolve("/fake-storage-paths-home"); +const FAKE_TMPDIR = path.resolve("/fake-storage-paths-tmp"); + vi.mock("node:fs", () => ({ existsSync: vi.fn(), })); +vi.mock("node:os", async (importOriginal) => ({ + ...(await importOriginal()), + homedir: () => FAKE_HOME, + tmpdir: () => FAKE_TMPDIR, +})); + import { existsSync } from "node:fs"; import { getConfigDir, diff --git a/test/test-home-isolation.test.ts b/test/test-home-isolation.test.ts new file mode 100644 index 00000000..ec58263b --- /dev/null +++ b/test/test-home-isolation.test.ts @@ -0,0 +1,62 @@ +import { describe, it, expect } from "vitest"; +import { homedir, userInfo } from "node:os"; +import { isAbsolute, relative, resolve } from "node:path"; +import { LOG_DIR } from "../lib/logger.js"; +import { ACCOUNTS_FILE_NAME } from "../lib/constants.js"; +import { saveAccounts } from "../lib/storage/load-save.js"; +import { getConfigDir } from "../lib/storage/paths.js"; +import { + getStoragePath, + setStoragePath, + setStoragePathDirect, +} from "../lib/storage/state.js"; + +function realUserHome(): string { + return userInfo().homedir; +} + +function isUnder(baseDir: string, targetPath: string): boolean { + const rel = relative(resolve(baseDir), resolve(targetPath)); + return rel === "" || (!rel.startsWith("..") && !isAbsolute(rel)); +} + +describe("test home isolation", () => { + it("redirects homedir() away from the real user home", () => { + expect(process.env.OC_CODEX_TEST_HOME).toBeTruthy(); + expect(homedir()).toBe(process.env.OC_CODEX_TEST_HOME); + expect(isUnder(realUserHome(), homedir())).toBe(false); + }); + + it("keeps every resolved account storage path out of the real user home", () => { + const real = realUserHome(); + + setStoragePath(null); + expect(isUnder(real, getStoragePath())).toBe(false); + + setStoragePath(process.cwd()); + expect(isUnder(real, getStoragePath())).toBe(false); + + setStoragePath(null); + expect(isUnder(real, getConfigDir())).toBe(false); + }); + + // LOG_DIR is captured at module scope from homedir(), so it only lands in + // the sandbox if the override beat the import. That is the property a + // setupFiles entry cannot provide and this whole mechanism exists for. + it("beats module-scope homedir() capture", () => { + expect(isUnder(realUserHome(), LOG_DIR)).toBe(false); + expect(isUnder(process.env.OC_CODEX_TEST_HOME as string, LOG_DIR)).toBe(true); + }); + + it("refuses to write account storage that escapes into the real home", async () => { + const escaped = resolve(realUserHome(), ".opencode", ACCOUNTS_FILE_NAME); + setStoragePathDirect(escaped); + try { + await expect( + saveAccounts({ version: 3, accounts: [], activeIndex: 0 }), + ).rejects.toMatchObject({ code: "TEST_HOME_ESCAPE" }); + } finally { + setStoragePathDirect(null); + } + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index 5499527f..4240487d 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,9 +1,33 @@ import { defineConfig } from 'vitest/config'; +import { mkdtempSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +/** + * Per-run throwaway home. The suite writes real account storage, so without + * this a `npm test` resolves `~/.opencode/oc-codex-multi-auth-accounts.json` + * against the developer's actual home and overwrites live ChatGPT credentials + * with fixtures. + * + * This must be `test.env`, not a `setupFiles` entry: vitest applies `test.env` + * before the worker imports any test module, and `lib/config.ts`, + * `lib/accounts/recovery.ts` and `lib/logger.ts` capture `homedir()` at module + * scope, so anything later than import time is too late for them. + */ +const isolatedHome = + process.env.OC_CODEX_TEST_HOME ?? + mkdtempSync(join(tmpdir(), 'oc-codex-multi-auth-test-home-')); +process.env.OC_CODEX_TEST_HOME = isolatedHome; export default defineConfig({ test: { globals: true, environment: 'node', + env: { + HOME: isolatedHome, + USERPROFILE: isolatedHome, + OC_CODEX_TEST_HOME: isolatedHome, + }, include: ['test/**/*.test.ts'], exclude: [ 'node_modules/**', From d9f5f81db8fff99c99104897d83d71a6a42fac2d Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 04:06:32 -0500 Subject: [PATCH 2/8] test(vitest): stop index.ts suites timing out on their own import test/index-retry.test.ts fails two of its six cases on a 5s timeout under full-suite load, and has done so on a clean checkout of upstream main. Nothing in those cases is slow: they already run on fake timers, and the whole file finishes its assertions in well under a second once loaded. What exceeds the timeout is the import. Four suites import the real `index.ts`, and the first one scheduled pays the transform of a 4900-line entry plus its dependency graph. Measured on an idle machine: 3.2s-6.7s for the cold import, ~400ms for a warm re-import after `vi.resetModules()`. With vitest's 5s default that is a coin flip before any test body runs, and CPU contention from the rest of the suite decides it. The floor is raised for the whole run rather than for one file, because a per-file timeout only moves the hazard to whichever of the four suites is scheduled first next time. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- vitest.config.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/vitest.config.ts b/vitest.config.ts index 4240487d..41c0fe84 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -28,6 +28,12 @@ export default defineConfig({ USERPROFILE: isolatedHome, OC_CODEX_TEST_HOME: isolatedHome, }, + // Four suites import the real `index.ts`, and the first one scheduled pays + // the transform of a 4900-line entry plus its dependency graph: measured at + // 3.2s-6.7s on an idle machine, against a 5s default. Whichever suite loses + // that race times out under full-suite CPU contention, which is flakiness in + // the harness rather than in any assertion (a warm re-import costs ~400ms). + testTimeout: 15_000, include: ['test/**/*.test.ts'], exclude: [ 'node_modules/**', From e131d69fd80e60743efb33655fa0e43f2d306e30 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 04:08:03 -0500 Subject: [PATCH 3/8] fix(retry): charge the rate-limit budget by wait length, not by attempt When every account is rate-limited the request waits and retries, and that loop is gated on `consumeRetryBudget("rateLimitGlobal", ...)`. The budget is tiny - 1 conservative, 3 balanced, 10 aggressive - and a wait cost one unit however briefly it blocked. Three consecutive 400ms waits therefore exhausted the default and the request hard-failed with "All N account(s) are rate-limited", when one more second of waiting would have served it. Nothing else in that gate can end the loop. `retryAllAccountsMaxRetries` defaults to Infinity, and `retryAllAccountsMaxWaitMs` defaults to 0, which the gate reads as uncapped - so the budget is the only term that can go false. The tracker is constructed per request, so this is not budget carried over from an earlier one either. Observed in production. Three independent sessions on a healthy 7-account pool died after roughly nine minutes each, reporting a true reset four hours out: `All 7 account(s) are rate-limited. Try again in 4h 0m 0s`. Three units against a real 4h wait should have been about twelve hours of sleeping. The accounts had just had their quota stamps cleared, so each attempt looked viable, went out, took a real 429, slept briefly, and repeated until the budget was gone. A unit now measures blocking time rather than attempts. `consumeWait` charges a full unit for a wait at or above RETRY_WAIT_BUDGET_UNIT_MS (5s), so a multi-hour block stays governed exactly as it was, and accumulates shorter waits on a per-bucket carry so a burst of sub-second probes is effectively free. An exhausted bucket still refuses a free wait: the carry is what bounds how long short waits can loop, and without that check the loop would never terminate. `consume` is unchanged and remains the default, so every other retry class keeps counting attempts. Only a caller that passes a wait is charged by duration, and the metrics counter follows the tracker's own usage rather than assuming one unit per call, so a free wait no longer reports budget it never spent. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- index.ts | 16 +++++++- lib/request/retry-budget.ts | 42 ++++++++++++++++++++ test/retry-budget.test.ts | 79 +++++++++++++++++++++++++++++++++++++ 3 files changed, 135 insertions(+), 2 deletions(-) diff --git a/index.ts b/index.ts index 1cb71259..99b35279 100644 --- a/index.ts +++ b/index.ts @@ -2396,9 +2396,20 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { const consumeRetryBudget = ( bucket: RetryBudgetClass, reason: string, + waitMs?: number, ): boolean => { - if (retryBudget.consume(bucket)) { - runtimeMetrics.retryBudgetUsage[bucket] += 1; + // Pass the wait so the charge scales with how long the retry + // blocks. Metrics follow the tracker's own counter rather than + // assuming one unit, or a free sub-second wait would report + // budget it never spent. + const usedBefore = retryBudget.getUsage()[bucket]; + const granted = + waitMs === undefined + ? retryBudget.consume(bucket) + : retryBudget.consumeWait(bucket, waitMs); + if (granted) { + runtimeMetrics.retryBudgetUsage[bucket] += + retryBudget.getUsage()[bucket] - usedBefore; return true; } runtimeMetrics.retryBudgetExhaustions += 1; @@ -3795,6 +3806,7 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { consumeRetryBudget( "rateLimitGlobal", `All accounts rate-limited wait ${waitMs}ms`, + waitMs, ) ) { const countdownMessage = `All ${count} account(s) rate-limited. Waiting`; diff --git a/lib/request/retry-budget.ts b/lib/request/retry-budget.ts index d1ea2beb..a86d01bf 100644 --- a/lib/request/retry-budget.ts +++ b/lib/request/retry-budget.ts @@ -43,6 +43,18 @@ const PROFILE_LIMITS: Record = { }, }; +/** + * How much blocking one budget unit buys when a retry is charged through + * {@link RetryBudgetTracker.consumeWait}. + * + * The budgets are small (1/3/10) and were being charged one unit per wait + * regardless of length, so three consecutive sub-second waits exhausted the + * default and hard-failed a request that one more second would have served. + * Waiting is only expensive in proportion to the time it costs the caller, so + * that is what a unit now measures. + */ +export const RETRY_WAIT_BUDGET_UNIT_MS = 5_000; + const RETRY_BUDGET_CLASSES: RetryBudgetClass[] = [ "authRefresh", "network", @@ -87,12 +99,42 @@ function createUsedCounters(): RetryBudgetLimits { export class RetryBudgetTracker { private readonly used: RetryBudgetLimits = createUsedCounters(); + private readonly waitCarryMs: RetryBudgetLimits = createUsedCounters(); private readonly limits: RetryBudgetLimits; constructor(limits: RetryBudgetLimits) { this.limits = { ...limits }; } + /** + * Charge a retry that blocks for `waitMs` against a bucket, in proportion to + * how long it blocks. + * + * A wait of {@link RETRY_WAIT_BUDGET_UNIT_MS} or longer costs a full unit, + * so a multi-hour block stays governed exactly as before. Shorter waits + * accumulate on a per-bucket carry and only cost a unit once they have added + * up to one, so a burst of sub-second waits is effectively free. + * + * An exhausted bucket refuses even a free wait: the carry bounds how long + * short waits can loop, and without that check they would loop forever once + * the budget ran out. + */ + consumeWait(bucket: RetryBudgetClass, waitMs: number): boolean { + if (this.getRemaining(bucket) <= 0) return false; + + const wait = Number.isFinite(waitMs) && waitMs > 0 ? waitMs : 0; + if (wait >= RETRY_WAIT_BUDGET_UNIT_MS) return this.consume(bucket); + + const carried = this.waitCarryMs[bucket] + wait; + if (carried < RETRY_WAIT_BUDGET_UNIT_MS) { + this.waitCarryMs[bucket] = carried; + return true; + } + + this.waitCarryMs[bucket] = carried - RETRY_WAIT_BUDGET_UNIT_MS; + return this.consume(bucket); + } + consume(bucket: RetryBudgetClass): boolean { const limit = this.limits[bucket]; if (!Number.isFinite(limit)) { diff --git a/test/retry-budget.test.ts b/test/retry-budget.test.ts index 8f3e62fe..8d67dba7 100644 --- a/test/retry-budget.test.ts +++ b/test/retry-budget.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest"; import { RetryBudgetTracker, resolveRetryBudgetLimits, + RETRY_WAIT_BUDGET_UNIT_MS, type RetryBudgetLimits, } from "../lib/request/retry-budget.js"; @@ -72,6 +73,84 @@ describe("retry-budget", () => { expect(tracker.getUsage().authRefresh).toBe(1); }); + describe("consumeWait", () => { + const balanced = () => new RetryBudgetTracker(resolveRetryBudgetLimits("balanced")); + + it("does not charge the budget for a burst of sub-second waits", () => { + const tracker = balanced(); + + // The production regression: three consecutive 400ms waits spent the + // whole default budget and hard-failed the request. + for (let i = 0; i < 3; i++) { + expect(tracker.consumeWait("rateLimitGlobal", 400)).toBe(true); + } + expect(tracker.getUsage().rateLimitGlobal).toBe(0); + expect(tracker.getRemaining("rateLimitGlobal")).toBe(3); + + for (let i = 0; i < 27; i++) { + expect(tracker.consumeWait("rateLimitGlobal", 400)).toBe(true); + } + }); + + it("charges a full unit for a wait at or above the unit length", () => { + const tracker = balanced(); + + expect(tracker.consumeWait("rateLimitGlobal", RETRY_WAIT_BUDGET_UNIT_MS)).toBe(true); + expect(tracker.getUsage().rateLimitGlobal).toBe(1); + + expect(tracker.consumeWait("rateLimitGlobal", 6 * 60 * 60 * 1000)).toBe(true); + expect(tracker.consumeWait("rateLimitGlobal", 6 * 60 * 60 * 1000)).toBe(true); + expect(tracker.getUsage().rateLimitGlobal).toBe(3); + + // A long wait stays governed: the fourth exceeds the balanced budget. + expect(tracker.consumeWait("rateLimitGlobal", 6 * 60 * 60 * 1000)).toBe(false); + }); + + it("accumulates short waits into whole units", () => { + const tracker = balanced(); + const waitMs = RETRY_WAIT_BUDGET_UNIT_MS / 10; + + for (let i = 0; i < 10; i++) { + expect(tracker.consumeWait("rateLimitGlobal", waitMs)).toBe(true); + } + expect(tracker.getUsage().rateLimitGlobal).toBe(1); + }); + + it("refuses free short waits once the bucket is exhausted", () => { + const tracker = balanced(); + + for (let i = 0; i < 3; i++) { + expect(tracker.consumeWait("rateLimitGlobal", RETRY_WAIT_BUDGET_UNIT_MS)).toBe(true); + } + + // Without this the carry would grant sub-unit waits forever and the + // retry loop could never terminate. + expect(tracker.consumeWait("rateLimitGlobal", 1)).toBe(false); + expect(tracker.consumeWait("rateLimitGlobal", 0)).toBe(false); + }); + + it("keeps per-bucket carries independent", () => { + const tracker = balanced(); + const waitMs = RETRY_WAIT_BUDGET_UNIT_MS / 2; + + expect(tracker.consumeWait("rateLimitGlobal", waitMs)).toBe(true); + expect(tracker.consumeWait("rateLimitShort", waitMs)).toBe(true); + expect(tracker.getUsage().rateLimitGlobal).toBe(0); + expect(tracker.getUsage().rateLimitShort).toBe(0); + + expect(tracker.consumeWait("rateLimitGlobal", waitMs)).toBe(true); + expect(tracker.getUsage().rateLimitGlobal).toBe(1); + expect(tracker.getUsage().rateLimitShort).toBe(0); + }); + + it("treats a zero-limit bucket as immediately exhausted", () => { + const tracker = new RetryBudgetTracker( + resolveRetryBudgetLimits("balanced", { rateLimitGlobal: 0 }), + ); + expect(tracker.consumeWait("rateLimitGlobal", 1)).toBe(false); + }); + }); + it("clones constructor limits to avoid external mutation", () => { const limits: RetryBudgetLimits = { authRefresh: 1, From db13030a8fc0a366edb00101d7653ec41da50be4 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 04:09:51 -0500 Subject: [PATCH 4/8] fix(rate-limit): re-probe upstream during a long all-accounts wait With the retry budget no longer spent on short waits, a request that finds every account blocked sleeps out the real reset, which can be hours or - `retryAllAccountsMaxRetries` defaults to Infinity - days. Its only wake-up was the accounts file changing on disk and the watcher swapping the cached manager. That covers a peer process clearing a block, and it covers `opencode auth login` adding an account mid-sleep, since both write that file. It does not cover a reset granted server-side: the backend restoring quota changes nothing locally, so the sleeper keeps sleeping against capacity that has already come back. A wait of a minute or more now re-probes upstream as well. The probe is the quota monitor's own `runNow`, which refreshes `/wham/usage` for every account and persists what it finds, a recovery included - and persisting drops the cached manager, which is what makes the enclosing retry loop re-resolve one that no longer reports a block. It starts a minute in and doubles to a quarter-hour ceiling, so a multi-day sleep costs a handful of usage requests rather than one per five-second countdown tick. A probe that throws is logged at debug and the wait continues. The next probe is scheduled from the moment a probe returns rather than from when it was due, so a slow usage request cannot leave `nextProbeAt` in the past and collapse the countdown sleep to zero. Both wake paths are covered end to end against a real request: one where usage reports the quota back with no file write at all, and one where a login adds a second account while the first stays blocked on disk. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- index.ts | 69 +++++++++++++++++++++++++-- test/accounts-live-reload.test.ts | 79 ++++++++++++++++++++++++++++++- 2 files changed, 142 insertions(+), 6 deletions(-) diff --git a/index.ts b/index.ts index 99b35279..c005291d 100644 --- a/index.ts +++ b/index.ts @@ -394,6 +394,15 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { let startupPreflightShown = false; let beginnerSafeModeEnabled = false; const MIN_BACKOFF_MS = 100; + // An all-accounts rate-limit wait can run for days, and the local accounts + // file is its only wake-up. A quota reset granted server-side leaves that file + // untouched, so such a wait would be slept straight through. Long waits + // therefore re-probe upstream: first after a minute, doubling to a quarter + // hour, so a multi-day sleep costs a handful of usage requests rather than one + // per countdown tick. + const UPSTREAM_REPROBE_MIN_WAIT_MS = 60_000; + const UPSTREAM_REPROBE_FIRST_DELAY_MS = 60_000; + const UPSTREAM_REPROBE_MAX_DELAY_MS = 15 * 60_000; const runtimeMetrics: RuntimeMetrics = { startedAt: Date.now(), @@ -2461,16 +2470,35 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { totalMs: number, message: string, intervalMs: number = 5000, + probeUpstream?: () => Promise, ): Promise => { const startTime = Date.now(); const endTime = startTime + totalMs; - + let probeDelayMs = UPSTREAM_REPROBE_FIRST_DELAY_MS; + let nextProbeAt = + probeUpstream && totalMs >= UPSTREAM_REPROBE_MIN_WAIT_MS + ? startTime + probeDelayMs + : Number.POSITIVE_INFINITY; + while (Date.now() < endTime) { if (cachedAccountManager !== accountManager) return; if (abortSignal?.aborted) { throw abortError(); } - + + if (probeUpstream && Date.now() >= nextProbeAt) { + if (await probeUpstream()) return; + if (cachedAccountManager !== accountManager) return; + if (abortSignal?.aborted) { + throw abortError(); + } + probeDelayMs = Math.min(probeDelayMs * 2, UPSTREAM_REPROBE_MAX_DELAY_MS); + // Measured from the end of the probe, so a slow usage + // request cannot schedule the next one in the past and + // collapse the countdown sleep below to zero. + nextProbeAt = Date.now() + probeDelayMs; + } + const remaining = Math.max(0, endTime - Date.now()); const waitLabel = formatWaitTime(remaining); await showToast( @@ -2478,8 +2506,8 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { "warning", { duration: Math.min(intervalMs + 1000, toastDurationMs) }, ); - - const sleepTime = Math.min(intervalMs, remaining); + + const sleepTime = Math.min(intervalMs, remaining, nextProbeAt - Date.now()); if (sleepTime > 0) { await sleep(sleepTime); } else { @@ -2488,6 +2516,32 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { } }; + /** + * True when an all-accounts wait can stop early. + * + * `runNow` refreshes `/wham/usage` for every account and persists + * whatever it finds, so a reset that never touched local disk + * becomes visible here. Persisting a recovery also drops the cached + * manager, which is what makes the enclosing retry loop re-resolve + * one that no longer reports a block. + */ + const probeUpstreamBlockLifted = async (): Promise => { + try { + await quotaMonitor.runNow(); + } catch (error) { + logDebug( + `[${PLUGIN_NAME}] Upstream quota re-probe failed: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + return false; + } + if (cachedAccountManager !== accountManager) return true; + const manager = accountManager; + if (!manager) return false; + return manager.getMinWaitTimeForFamily(modelFamily, model) === 0; + }; + let allRateLimitedRetries = 0; let emptyResponseRetries = 0; const attemptedUnsupportedFallbackModels = new Set(); @@ -3810,7 +3864,12 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { ) ) { const countdownMessage = `All ${count} account(s) rate-limited. Waiting`; - await sleepWithCountdown(addJitter(waitMs, 0.2), countdownMessage); + await sleepWithCountdown( + addJitter(waitMs, 0.2), + countdownMessage, + undefined, + probeUpstreamBlockLifted, + ); allRateLimitedRetries++; continue; } diff --git a/test/accounts-live-reload.test.ts b/test/accounts-live-reload.test.ts index 1998ded9..d903336a 100644 --- a/test/accounts-live-reload.test.ts +++ b/test/accounts-live-reload.test.ts @@ -9,6 +9,7 @@ const captured = vi.hoisted((): { context?: ToolContext; listener?: () => void; onWatch?: () => void; + onQuotaProbe?: () => Promise; reads: Promise[]; maxRetries?: number; } => ({ reads: [] })); @@ -32,7 +33,11 @@ vi.mock("../lib/tools/index.js", () => ({ createToolRegistry: (context: ToolContext) => { captured.context = context; return {}; }, })); vi.mock("../lib/quota-notifications.js", () => ({ - createQuotaMonitor: () => ({ start() {}, dispose() {} }), + createQuotaMonitor: () => ({ + start() {}, + dispose() {}, + runNow: async () => { await captured.onQuotaProbe?.(); }, + }), })); vi.mock("../lib/auto-update-checker.js", () => ({ checkAndNotify: vi.fn(async () => {}) })); vi.mock("../lib/config.js", async (original) => ({ @@ -91,6 +96,7 @@ describe("accounts live reload", () => { await fs.writeFile(path, JSON.stringify(storage(true))); captured.listener = undefined; captured.onWatch = undefined; + captured.onQuotaProbe = undefined; captured.reads.length = 0; captured.maxRetries = undefined; plugin = await Reflect.apply(OpenAIOAuthPlugin, undefined, [{ @@ -285,6 +291,77 @@ describe("accounts live reload", () => { await vi.advanceTimersByTimeAsync(5000); expect((await response).status).toBe(200); }); + it("wakes a long wait when an upstream re-probe finds the block lifted", async () => { + const manager = captured.context?.cachedAccountManagerRef.current; + if (!manager) throw new Error("Missing manager"); + const account = manager.getCurrentAccount(); + if (!account) throw new Error("Missing account"); + manager.markQuotaExhausted(account, Date.now() + 86_400_000, "gpt-5.1"); + vi.spyOn(globalThis, "fetch").mockImplementation(async () => new Response("data: [DONE]\n\n", { + status: 200, headers: { "content-type": "text/event-stream" }, + })); + let enteredWait: () => void = () => {}; + const waiting = new Promise((resolve) => { enteredWait = resolve; }); + const minWait = manager.getMinWaitTimeForFamily.bind(manager); + vi.spyOn(manager, "getMinWaitTimeForFamily").mockImplementation((...args) => { + enteredWait(); + return minWait(...args); + }); + let probes = 0; + captured.onQuotaProbe = async () => { + probes += 1; + // What a server-side grant looks like: usage reports the quota back, + // the recovery is persisted, and the cached manager is dropped. The + // accounts file is never written by another process, so the watcher + // has nothing to fire on - this is the wake-up it cannot provide. + captured.context?.invalidateAccountManagerCache(); + }; + const response = request("https://api.openai.com/v1/responses", { + method: "POST", body: JSON.stringify({ model: "gpt-5.1", stream: true, input: [] }), + }); + await waiting; + for (let elapsed = 0; elapsed < 120_000 && probes === 0; elapsed += 5000) { + await vi.advanceTimersByTimeAsync(5000); + } + expect(probes).toBe(1); + await vi.advanceTimersByTimeAsync(5000); + expect((await response).status).toBe(200); + }); + it("wakes a waiting request when a login adds an account mid-sleep", async () => { + const manager = captured.context?.cachedAccountManagerRef.current; + if (!manager) throw new Error("Missing manager"); + const account = manager.getCurrentAccount(); + if (!account) throw new Error("Missing account"); + const blockedUntil = Date.now() + 86_400_000; + manager.markQuotaExhausted(account, blockedUntil, "gpt-5.1"); + vi.spyOn(globalThis, "fetch").mockImplementation(async () => new Response("data: [DONE]\n\n", { + status: 200, headers: { "content-type": "text/event-stream" }, + })); + let enteredWait: () => void = () => {}; + const waiting = new Promise((resolve) => { enteredWait = resolve; }); + const minWait = manager.getMinWaitTimeForFamily.bind(manager); + vi.spyOn(manager, "getMinWaitTimeForFamily").mockImplementation((...args) => { + enteredWait(); + return minWait(...args); + }); + const response = request("https://api.openai.com/v1/responses", { + method: "POST", body: JSON.stringify({ model: "gpt-5.1", stream: true, input: [] }), + }); + await waiting; + const reloaded = nextReload(); + // The incumbent account stays blocked on disk, so the only thing that can + // end this wait is the account the login added. + await fs.writeFile(path, JSON.stringify({ ...storage(true), accounts: [ + { ...storage(true).accounts[0], quotaExhaustedUntil: blockedUntil }, + { accountId: "fresh-login", refreshToken: "fresh-refresh", accessToken: "fresh-access", + expiresAt: Date.now() + 86_400_000, enabled: true, addedAt: 2, lastUsed: 2 }, + ] })); + await tick(); + await settle(); + await reloaded; + await vi.advanceTimersByTimeAsync(5000); + expect((await response).status).toBe(200); + }); it("keeps externally cleared blocks cleared despite queued and late saves from the old manager", async () => { const previous = captured.context?.cachedAccountManagerRef.current; if (!previous) throw new Error("Missing manager"); From 6c4c355508b68d88c72d63d8ba1a9d5e8f860c58 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 04:12:13 -0500 Subject: [PATCH 5/8] fix(accounts): never let a failed load empty a working account pool `loadAccounts()` reports a read or parse failure exactly as it reports an absent file - by returning null. `AccountState.initializeFromStorage()` turns that null into an AccountManager holding zero accounts, and both reload paths installed it unconditionally. The process then answered No Codex accounts configured. Run `opencode auth login`. while the accounts file on disk was intact and every other process on the machine was serving requests from it. Cross-process lock contention makes the failing read reachable: several opencode instances share one accounts file, and a read that loses a race against another process's atomic temp-file rename surfaces as exactly this empty result. Two guards, one per install site: - `reloadCachedAccountManager` compares the fresh manager against the incumbent it is replacing. A fresh manager with no accounts replacing an incumbent that has some is refused, the incumbent keeps serving, and a bounded retry (3 attempts, 2s apart) runs in case the next read succeeds. - `reloadForExternalAccountsChange` cannot compare against the incumbent, because an invalidation may legitimately have retired it and left the cache null. It compares against the file instead: the watcher already reads and hashes the changed file, so counting its `accounts` array costs nothing and says directly whether the accounts went away or the read failed. A file that carries accounts but loads as empty is refused and retried through the existing bounded retry path. The file-based comparison is what makes a genuine deletion still work. An external writer that really does remove the last account leaves an empty array on disk, the observed count is 0, the guard does not fire, and the empty pool is adopted as it should be. Both directions are covered by tests. Emptying the pool through the plugin's own surfaces (`codex-remove`, logout, a storage-mode switch) installs a manager directly rather than arriving on either of these paths, so neither guard can block a user-initiated removal. `readAccountsDigest` becomes `readAccountsFileState` and returns the count alongside the digest. The count is taken off the raw parsed document rather than the schema-validated union, so it reads the same for a V1, V2, or V3 file. The retry timers are unref'd and cancelled on watcher disposal, so a process shutting down mid-retry is not held open. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- index.ts | 115 +++++++++++++++++++++++++----- test/accounts-live-reload.test.ts | 29 ++++++++ 2 files changed, 128 insertions(+), 16 deletions(-) diff --git a/index.ts b/index.ts index c005291d..aa6ea961 100644 --- a/index.ts +++ b/index.ts @@ -1684,6 +1684,50 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { } }; + /** + * `loadAccounts()` reports a read or parse failure the same way it reports + * an absent file - by returning null - and a null load builds a manager + * holding zero accounts. Installing that over a working pool makes this + * process answer "No Codex accounts configured" while the accounts file on + * disk is intact, which cross-process lock contention makes reachable. + * + * Emptying the pool for real always goes through an explicit action + * (`codex-remove`, logout, a storage-mode switch); each installs its own + * manager rather than arriving here, so refusing the shrink costs a genuine + * deletion nothing. + */ + const isUntrustworthyEmptyReload = ( + incumbent: AccountManager | null, + reloaded: AccountManager, + ): boolean => + incumbent !== null && + incumbent !== reloaded && + reloaded.getAccountCount() === 0 && + incumbent.getAccountCount() > 0; + + const EMPTY_RELOAD_RETRY_DELAY_MS = 2000; + const EMPTY_RELOAD_MAX_RETRIES = 3; + let emptyReloadRetries = 0; + let emptyReloadRetryTimer: ReturnType | undefined; + const cancelEmptyReloadRetry = (): void => { + clearTimeout(emptyReloadRetryTimer); + emptyReloadRetryTimer = undefined; + emptyReloadRetries = 0; + }; + const scheduleEmptyReloadRetry = (retry: () => Promise): void => { + if (emptyReloadRetries >= EMPTY_RELOAD_MAX_RETRIES) { + emptyReloadRetries = 0; + return; + } + emptyReloadRetries += 1; + clearTimeout(emptyReloadRetryTimer); + emptyReloadRetryTimer = setTimeout(() => { + emptyReloadRetryTimer = undefined; + void retry(); + }, EMPTY_RELOAD_RETRY_DELAY_MS); + emptyReloadRetryTimer.unref(); + }; + const reloadCachedAccountManager = async (): Promise => { if (!cachedAccountManager) return; const previous = cachedAccountManager; @@ -1702,6 +1746,15 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { } try { const reloadedManager = await AccountManager.loadFromDisk(); + if (isUntrustworthyEmptyReload(previous, reloadedManager)) { + reloadedManager.disposeShutdownHandler(); + logWarn( + `[${PLUGIN_NAME}] Account reload returned no accounts while ${previous.getAccountCount()} are held; keeping the loaded pool and retrying`, + ); + scheduleEmptyReloadRetry(reloadCachedAccountManager); + return; + } + cancelEmptyReloadRetry(); cachedAccountManager = reloadedManager; accountManagerPromise = Promise.resolve(reloadedManager); // Dispose only after the replacement is installed so we never leak @@ -1734,21 +1787,41 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { accountsWatcherDisposed = true; unsubscribeAccountsPath?.(); stopAccountsWatcher(); + cancelEmptyReloadRetry(); unregisterCleanup(disposeAccountsWatcher); }; - const readAccountsDigest = async (path: string): Promise => { + const readAccountsFileState = async ( + path: string, + ): Promise<{ digest: string; accountCount: number } | undefined> => { try { const content = await readFile(path, "utf8"); - if (!AnyAccountStorageSchema.safeParse(JSON.parse(content)).success) return; - return createHash("sha256").update(content).digest("hex"); + const data = JSON.parse(content) as unknown; + if (!AnyAccountStorageSchema.safeParse(data).success) return; + // Counted off the raw document rather than the parsed union so the + // count is the same for every storage version. + const accounts = (data as { accounts?: unknown }).accounts; + return { + digest: createHash("sha256").update(content).digest("hex"), + accountCount: Array.isArray(accounts) ? accounts.length : 0, + }; } catch { return; } }; const reloadForExternalAccountsChange = async (path: string, generation: number, attempt = 0, retired?: AccountManager): Promise => { - const digest = await readAccountsDigest(path); - if (generation !== accountsWatchGeneration || !digest || path !== getStoragePath()) return; + const observed = await readAccountsFileState(path); + if (generation !== accountsWatchGeneration || !observed || path !== getStoragePath()) return; + const digest = observed.digest; if (digest === consumeLastWrittenAccountsDigest(path)) return; + const retryLater = (): void => { + if (attempt < 2 && generation === accountsWatchGeneration && !accountsWatcherDisposed) { + accountsReloadTimer = setTimeout(() => { + accountsReloadTimer = undefined; + void reloadForExternalAccountsChange(path, generation, attempt + 1, retired); + }, 1500); + accountsReloadTimer.unref(); + } + }; const previous = cachedAccountManager; try { // A null cache means an invalidation retired the incumbent; the @@ -1769,6 +1842,21 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { reloaded.disposeShutdownHandler(); return; } + // The file this reload observed carried accounts but the load + // produced none, so `loadAccounts()` failed to read it rather than + // the accounts having gone away - a failure it reports as an empty + // result, never as a throw, so the catch below cannot see it. + // Adopting it would answer "No Codex accounts configured" against an + // intact file; the retired incumbent still serves its accounts until + // a retry lands a real one. + if (observed.accountCount > 0 && reloaded.getAccountCount() === 0) { + reloaded.disposeShutdownHandler(); + logWarn( + `[${PLUGIN_NAME}] Externally changed accounts file holds ${observed.accountCount} account(s) but loaded as empty; keeping the current pool and retrying`, + ); + retryLater(); + return; + } const outgoing = cachedAccountManager; if (outgoing && outgoing !== retired) { // Another actor replaced the cached manager while this reload @@ -1784,13 +1872,7 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { observedAccountsDigest = digest; } catch { logWarn("Could not reload externally updated account storage"); - if (attempt < 2 && generation === accountsWatchGeneration && !accountsWatcherDisposed) { - accountsReloadTimer = setTimeout(() => { - accountsReloadTimer = undefined; - void reloadForExternalAccountsChange(path, generation, attempt + 1, retired); - }, 1500); - accountsReloadTimer.unref(); - } + retryLater(); return; } logDebug("Reloaded cached account manager after external accounts file change"); @@ -1804,8 +1886,9 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { const path = watchedAccountsPath; if (!path) return; const generation = accountsWatchGeneration; - const digest = await readAccountsDigest(path); - if (generation !== accountsWatchGeneration || !digest || digest === observedAccountsDigest) return; + const observed = await readAccountsFileState(path); + if (generation !== accountsWatchGeneration || !observed || observed.digest === observedAccountsDigest) return; + const digest = observed.digest; observedAccountsDigest = digest; clearTimeout(accountsReloadTimer); accountsReloadTimer = undefined; @@ -1830,9 +1913,9 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { }); watchedAccountsPath = path; const generation = accountsWatchGeneration; - const initialDigest = await readAccountsDigest(path); + const initial = await readAccountsFileState(path); if (generation !== accountsWatchGeneration) return; - observedAccountsDigest = initialDigest; + observedAccountsDigest = initial?.digest; // Stat polling follows the path across the storage writer's temp-file rename. watchFile(path, { interval: 1500, persistent: false }, onAccountsStatChanged); unregisterCleanup(disposeAccountsWatcher); diff --git a/test/accounts-live-reload.test.ts b/test/accounts-live-reload.test.ts index d903336a..3612f87a 100644 --- a/test/accounts-live-reload.test.ts +++ b/test/accounts-live-reload.test.ts @@ -362,6 +362,35 @@ describe("accounts live reload", () => { await vi.advanceTimersByTimeAsync(5000); expect((await response).status).toBe(200); }); + it("keeps the loaded pool when an external change loads as empty", async () => { + const previous = captured.context?.cachedAccountManagerRef.current; + if (!previous) throw new Error("Missing manager"); + expect(previous.getAccountCount()).toBe(1); + const empty = new AccountManager(undefined, { ...storage(true), accounts: [] }); + const load = vi.spyOn(AccountManager, "loadFromDisk").mockResolvedValueOnce(empty); + await fs.writeFile(path, JSON.stringify(storage(false))); + await tick(); + await settle(); + expect(load).toHaveBeenCalledTimes(1); + expect(captured.context?.cachedAccountManagerRef.current).toBe(previous); + expect(captured.context?.cachedAccountManagerRef.current?.getAccountCount()).toBe(1); + const reloaded = nextReload(); + await vi.advanceTimersByTimeAsync(1500); + await drainReads(); + await reloaded; + expect(load).toHaveBeenCalledTimes(2); + expect(captured.context?.cachedAccountManagerRef.current?.getAccountsSnapshot()[0]?.enabled).toBe(false); + }); + it("adopts an external change that genuinely removes the last account", async () => { + const previous = captured.context?.cachedAccountManagerRef.current; + const reloaded = nextReload(); + await fs.writeFile(path, JSON.stringify({ ...storage(true), accounts: [] })); + await tick(); + await settle(); + await reloaded; + expect(captured.context?.cachedAccountManagerRef.current).not.toBe(previous); + expect(captured.context?.cachedAccountManagerRef.current?.getAccountCount()).toBe(0); + }); it("keeps externally cleared blocks cleared despite queued and late saves from the old manager", async () => { const previous = captured.context?.cachedAccountManagerRef.current; if (!previous) throw new Error("Missing manager"); From 2825200c1e619c16f8635e74b78de43401c02696 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 05:15:04 -0500 Subject: [PATCH 6/8] test(vitest): remove the temp home each run mints The per-run home added alongside the HOME redirect was never removed, so every `npm test` left one behind. On a tmpfs `/tmp` that accumulates: 24 of them had collected on this machine, `/tmp` reached 98%, and the resulting ENOSPC killed a `vitest run` outright with `ENOSPC: no space left on device` on a pure unit-test file. A test harness that degrades the machine it runs on is the harness's own bug, not the operator's. A `globalSetup` teardown is the right hook: it runs once, in the main process, after every worker is finished, so it cannot race a suite that is still writing. The HOME redirect stays in `test.env` exactly where it was - that placement is load-bearing, because `lib/config.ts`, `lib/accounts/recovery.ts` and `lib/logger.ts` capture `homedir()` at module scope and `test.env` is the only hook that lands before the worker imports them. Deleting a directory unattended deserves more care than deleting one by hand, so three conditions gate it and a path failing any of them is left alone rather than guessed at: - the config must have minted the directory itself. A home handed in through `OC_CODEX_TEST_HOME` belongs to whoever set it, and a CI harness that points the suite at a directory it manages must get that directory back. The config records ownership when it mints, so an inherited path and a minted one are distinguishable even when they look identical. - the resolved path must still sit under `tmpdir()`. - it must carry the prefix `mkdtempSync` was given. `force: true` keeps an already-removed directory from throwing, so an interrupted run cannot leave a failure that outlives it. The tests drive `teardown` against directories they create themselves, never against the live run's own home, so a future regression in the gate can only destroy scratch. Two of them are deliberately near identical - same path shape, opposite ownership - because that pins the ownership flag as the only thing deciding the delete. One more asserts that the prefix this module exports still matches the one the config minted with: the two are spelled in separate files, and were they to drift apart teardown would quietly stop matching and the leak would return with nothing failing. Every guard was control-run: each was broken in turn and the matching test confirmed failing before being restored. Verified end to end by counting `/tmp` before and after a full run - 20 before, 20 after, so the run minted a home and took it away again. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- test/global-setup.ts | 30 ++++++++++ test/test-home-isolation.test.ts | 95 +++++++++++++++++++++++++++++++- vitest.config.ts | 8 ++- 3 files changed, 129 insertions(+), 4 deletions(-) create mode 100644 test/global-setup.ts diff --git a/test/global-setup.ts b/test/global-setup.ts new file mode 100644 index 00000000..2ad8ab64 --- /dev/null +++ b/test/global-setup.ts @@ -0,0 +1,30 @@ +import { rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { resolve } from "node:path"; + +export const MINTED_HOME_PREFIX = "oc-codex-multi-auth-test-home-"; + +/** + * Remove the throwaway home `vitest.config.ts` minted for this run. + * + * Every run otherwise leaves one behind, and on a tmpfs `/tmp` they accumulate + * until a run dies of ENOSPC. + * + * Three conditions gate the delete, because this is an unattended `rm -rf`: + * the config must have minted the directory itself rather than been handed one + * through `OC_CODEX_TEST_HOME`, the resolved path must still sit directly under + * `tmpdir()`, and it must carry the prefix `mkdtempSync` was given. A path that + * fails any of them is left alone rather than guessed at. + */ +export async function teardown(): Promise { + if (process.env.OC_CODEX_TEST_HOME_OWNED !== "1") return; + + const home = process.env.OC_CODEX_TEST_HOME; + if (!home) return; + + const resolved = resolve(home); + const expectedPrefix = resolve(tmpdir(), MINTED_HOME_PREFIX); + if (resolved === expectedPrefix || !resolved.startsWith(expectedPrefix)) return; + + await rm(resolved, { recursive: true, force: true }); +} diff --git a/test/test-home-isolation.test.ts b/test/test-home-isolation.test.ts index ec58263b..3f2618d8 100644 --- a/test/test-home-isolation.test.ts +++ b/test/test-home-isolation.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect } from "vitest"; -import { homedir, userInfo } from "node:os"; -import { isAbsolute, relative, resolve } from "node:path"; +import { existsSync, mkdtempSync, mkdirSync, rmSync } from "node:fs"; +import { homedir, tmpdir, userInfo } from "node:os"; +import { isAbsolute, join, relative, resolve } from "node:path"; import { LOG_DIR } from "../lib/logger.js"; import { ACCOUNTS_FILE_NAME } from "../lib/constants.js"; import { saveAccounts } from "../lib/storage/load-save.js"; @@ -10,6 +11,7 @@ import { setStoragePath, setStoragePathDirect, } from "../lib/storage/state.js"; +import { MINTED_HOME_PREFIX, teardown } from "./global-setup.js"; function realUserHome(): string { return userInfo().homedir; @@ -60,3 +62,92 @@ describe("test home isolation", () => { } }); }); + +describe("test home teardown", () => { + // Every case drives `teardown` against a directory this test made, never + // against the live run's own home, so a broken gate can only destroy scratch. + const runTeardown = async ( + home: string | undefined, + owned: boolean, + ): Promise => { + const previousHome = process.env.OC_CODEX_TEST_HOME; + const previousOwned = process.env.OC_CODEX_TEST_HOME_OWNED; + if (home === undefined) delete process.env.OC_CODEX_TEST_HOME; + else process.env.OC_CODEX_TEST_HOME = home; + if (owned) process.env.OC_CODEX_TEST_HOME_OWNED = "1"; + else delete process.env.OC_CODEX_TEST_HOME_OWNED; + try { + await teardown(); + } finally { + if (previousHome === undefined) delete process.env.OC_CODEX_TEST_HOME; + else process.env.OC_CODEX_TEST_HOME = previousHome; + if (previousOwned === undefined) delete process.env.OC_CODEX_TEST_HOME_OWNED; + else process.env.OC_CODEX_TEST_HOME_OWNED = previousOwned; + } + }; + + const mintedLookalike = (): string => + mkdtempSync(join(tmpdir(), MINTED_HOME_PREFIX)); + + // `vitest.config.ts` spells the prefix as a literal and this module exports + // it as a constant. Were the two to drift apart, teardown would simply stop + // matching the home the config minted and the leak would return silently. + it("agrees with the prefix the config actually minted", () => { + if (process.env.OC_CODEX_TEST_HOME_OWNED !== "1") return; + const home = process.env.OC_CODEX_TEST_HOME as string; + expect(resolve(home).startsWith(resolve(tmpdir(), MINTED_HOME_PREFIX))).toBe(true); + }); + + it("removes a home it minted itself", async () => { + const home = mintedLookalike(); + expect(existsSync(home)).toBe(true); + await runTeardown(home, true); + expect(existsSync(home)).toBe(false); + }); + + // Identical path shape to the case above, so the ownership flag is the only + // thing left deciding it. A home handed in through the environment belongs + // to whoever set it. + it("keeps an inherited home that looks exactly like a minted one", async () => { + const home = mintedLookalike(); + try { + await runTeardown(home, false); + expect(existsSync(home)).toBe(true); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); + + it("keeps a path outside the temp directory", async () => { + const scratchRoot = join(process.cwd(), "tmp"); + mkdirSync(scratchRoot, { recursive: true }); + const outside = mkdtempSync(join(scratchRoot, MINTED_HOME_PREFIX)); + try { + await runTeardown(outside, true); + expect(existsSync(outside)).toBe(true); + } finally { + rmSync(outside, { recursive: true, force: true }); + } + }); + + it("keeps a temp path that does not carry the minted prefix", async () => { + const foreign = mkdtempSync(join(tmpdir(), "oc-codex-unrelated-")); + try { + await runTeardown(foreign, true); + expect(existsSync(foreign)).toBe(true); + } finally { + rmSync(foreign, { recursive: true, force: true }); + } + }); + + // An interrupted run can leave the env half-set or the directory already + // gone. Neither may throw, or the failure outlives the run it came from. + it("tolerates a missing home and an already-removed one", async () => { + await expect(runTeardown(undefined, true)).resolves.toBeUndefined(); + + const home = mintedLookalike(); + await runTeardown(home, true); + expect(existsSync(home)).toBe(false); + await expect(runTeardown(home, true)).resolves.toBeUndefined(); + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index 41c0fe84..d28c7049 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -14,10 +14,13 @@ import { join } from 'node:path'; * `lib/accounts/recovery.ts` and `lib/logger.ts` capture `homedir()` at module * scope, so anything later than import time is too late for them. */ +const inheritedHome = process.env.OC_CODEX_TEST_HOME; const isolatedHome = - process.env.OC_CODEX_TEST_HOME ?? - mkdtempSync(join(tmpdir(), 'oc-codex-multi-auth-test-home-')); + inheritedHome ?? mkdtempSync(join(tmpdir(), 'oc-codex-multi-auth-test-home-')); process.env.OC_CODEX_TEST_HOME = isolatedHome; +// Only a home this config minted may be removed once the run ends. One handed +// in through the environment belongs to whoever set it. +if (!inheritedHome) process.env.OC_CODEX_TEST_HOME_OWNED = '1'; export default defineConfig({ test: { @@ -34,6 +37,7 @@ export default defineConfig({ // that race times out under full-suite CPU contention, which is flakiness in // the harness rather than in any assertion (a warm re-import costs ~400ms). testTimeout: 15_000, + globalSetup: ['./test/global-setup.ts'], include: ['test/**/*.test.ts'], exclude: [ 'node_modules/**', From fc8b3714c741ed5c6a9f5f3fc2221b9c5fc0b124 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 13:37:11 -0500 Subject: [PATCH 7/8] test(isolation): keep the sandbox guard working where tmp sits under home The guard added earlier in this branch refused any account-storage path under `os.userInfo().homedir`. On Windows `os.tmpdir()` normally resolves to `C:\Users\\AppData\Local\Temp`, so the sandbox itself is a descendant of the real home and every legitimate write inside it was refused. CI runs a windows-latest job, so that is the whole suite red on one of four matrix legs rather than a local-only annoyance. The exemption is the sandbox and the temp directory, not the home tree: a path under the real home but outside both is still the production store. Each exemption is ignored when the root would swallow the real home, so `OC_CODEX_TEST_HOME=/` or `TMPDIR=$HOME` cannot disarm the check by widening it. Guarding writes alone was also not enough. A missing project store sends the loader to the global one, whose path is re-resolved from `homedir()` at that moment, and the legacy-storage migration reads its source file before any write happens. With HOME restored, both reach the live pool through paths the write-time check never sees. Both are guarded at the point of resolution, ahead of the `catch` blocks that would otherwise swallow the escape as an ordinary read failure. The isolation assertions no longer say "outside the real home", which is false on Windows while isolation is working perfectly. They say the path is inside the sandbox and outside `~/.opencode`, which holds everywhere. That also settles the objection that the assertion depends on the checkout living outside the home directory: per-project storage is namespaced under `getConfigDir()`, so it follows the redirected home even when the checkout is inside the real one, as it is on CI. Two further holes, both reachable rather than theoretical: - A caller exporting OC_CODEX_TEST_HOME_OWNED alongside its own OC_CODEX_TEST_HOME had that directory recursively deleted by teardown, because the flag was believed rather than derived. The config now clears what it inherits, so ownership is only ever what it set itself. - The OS credential store is the one place a HOME redirect cannot reach. An inherited CODEX_KEYCHAIN=1 routed fixture writes into the developer's real keychain; the suite now pins it off, and the tests that need the backend opt in per test. Verified by control run for each guard: the exemptions removed under a sandbox-under-home layout (TMPDIR pointed inside the real home, which reproduces the Windows shape on Linux) refuse a legitimate sandbox write; the read-path guards removed let the global fallback resolve into the real home; an inherited ownership flag left set deletes the caller's directory. Windows itself was not executed here - the platform claim is reasoned from `os.tmpdir()`'s documented layout, not observed. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 4e488d6 --- lib/storage/load-save.ts | 38 ++++++++-- test/test-home-isolation.test.ts | 121 +++++++++++++++++++++++++++---- vitest.config.ts | 5 ++ 3 files changed, 145 insertions(+), 19 deletions(-) diff --git a/lib/storage/load-save.ts b/lib/storage/load-save.ts index ef20c727..0066c209 100644 --- a/lib/storage/load-save.ts +++ b/lib/storage/load-save.ts @@ -176,15 +176,22 @@ async function checkWorktreeLockForCurrentStorage( } /** - * Refuse to mutate account storage inside the developer's real home while the + * Refuse to touch account storage inside the developer's real home while the * test suite is running. * * `vitest.config.ts` redirects HOME to a sandbox before any module loads, but a * test that restores the captured real HOME, or a future regression in that - * config, would otherwise write fixtures straight over live ChatGPT - * credentials. `os.userInfo()` reads the passwd entry instead of `$HOME`, so it - * still names the real home after the redirect and gives the check something - * the sandbox cannot spoof. Inert outside vitest. + * config, would otherwise read or overwrite live ChatGPT credentials. + * `os.userInfo()` reads the passwd entry instead of `$HOME`, so it still names + * the real home after the redirect and gives the check something the sandbox + * cannot spoof. Inert outside vitest. + * + * The sandbox and the temp directory are exempt, and both exemptions are + * load-bearing rather than convenience: on Windows `os.tmpdir()` normally sits + * inside the user profile, so there every legitimate sandbox path is also a + * real-home path and a bare home-prefix test would reject the entire suite. + * Neither exemption can reach the production store, which lives under the real + * home's `.opencode`. */ function assertTestRunNeverTouchesRealHome(path: string): void { if (!process.env.VITEST) return; @@ -197,8 +204,17 @@ function assertTestRunNeverTouchesRealHome(path: string): void { } if (!realHome || !isWithinDirectory(realHome, path)) return; + // A root exempts what lies beneath it only while it does not itself swallow + // the real home. Without that clause `OC_CODEX_TEST_HOME=/` or `TMPDIR=$HOME` + // would disarm the guard completely. + const exempts = (root: string | undefined): boolean => + !!root && !isWithinDirectory(root, realHome) && isWithinDirectory(root, path); + + if (exempts(process.env.OC_CODEX_TEST_HOME)) return; + if (exempts(os.tmpdir())) return; + throw new StorageError( - `Refusing to write account storage inside the real home directory during a test run: ${path}`, + `Refusing to touch account storage inside the real home directory during a test run: ${path}`, "TEST_HOME_ESCAPE", path, "A test resolved account storage against the developer's real home. Point HOME at a temp directory for the whole vitest process (see vitest.config.ts) instead of overriding it per test.", @@ -241,6 +257,10 @@ async function migrateStorageFileIfNeeded( persist: (storage: AccountStorageV3) => Promise, label: string, ): Promise { + // Before the existsSync, and outside the try: this reads the legacy file and + // the catch below swallows everything except a forward-compat reject, so a + // guard placed any later would be silently discarded. + if (legacyPath) assertTestRunNeverTouchesRealHome(legacyPath); if (!legacyPath || legacyPath === nextPath || !existsSync(legacyPath)) { return null; } @@ -337,6 +357,12 @@ async function loadGlobalAccountsFallback(): Promise { return null; } + // The project store is missing, so this reaches for the GLOBAL one, which + // resolves against `homedir()` and is the real pool whenever HOME has been + // restored. Guarded here rather than at the read below, because the catch + // there returns null for everything and would hide the escape. + assertTestRunNeverTouchesRealHome(getGlobalAccountsStoragePath()); + const migrated = await migrateLegacyGlobalStorageIfNeeded(); if (migrated) { return migrated; diff --git a/test/test-home-isolation.test.ts b/test/test-home-isolation.test.ts index 3f2618d8..4e0c4160 100644 --- a/test/test-home-isolation.test.ts +++ b/test/test-home-isolation.test.ts @@ -4,7 +4,7 @@ import { homedir, tmpdir, userInfo } from "node:os"; import { isAbsolute, join, relative, resolve } from "node:path"; import { LOG_DIR } from "../lib/logger.js"; import { ACCOUNTS_FILE_NAME } from "../lib/constants.js"; -import { saveAccounts } from "../lib/storage/load-save.js"; +import { loadAccounts, saveAccounts } from "../lib/storage/load-save.js"; import { getConfigDir } from "../lib/storage/paths.js"; import { getStoragePath, @@ -17,37 +17,83 @@ function realUserHome(): string { return userInfo().homedir; } +/** + * The real account pool lives in `~/.opencode`, not across the whole home tree. + * + * Isolation cannot be expressed as "outside the real home": on Windows + * `tmpdir()` sits under the user profile, so the sandbox is legitimately a + * descendant of it and that assertion would fail while isolation was working + * perfectly. Containment in the sandbox, and separation from the real store, + * hold on every platform. + */ +function realStoreDir(): string { + return resolve(realUserHome(), ".opencode"); +} + +function sandboxHome(): string { + const home = process.env.OC_CODEX_TEST_HOME; + if (!home) { + throw new Error("OC_CODEX_TEST_HOME is unset; vitest.config.ts must mint a sandbox home"); + } + return home; +} + function isUnder(baseDir: string, targetPath: string): boolean { const rel = relative(resolve(baseDir), resolve(targetPath)); return rel === "" || (!rel.startsWith("..") && !isAbsolute(rel)); } +function expectSandboxed(path: string): void { + expect(isUnder(sandboxHome(), path)).toBe(true); + expect(isUnder(realStoreDir(), path)).toBe(false); +} + describe("test home isolation", () => { - it("redirects homedir() away from the real user home", () => { - expect(process.env.OC_CODEX_TEST_HOME).toBeTruthy(); - expect(homedir()).toBe(process.env.OC_CODEX_TEST_HOME); - expect(isUnder(realUserHome(), homedir())).toBe(false); + it("redirects homedir() into the sandbox", () => { + expect(homedir()).toBe(sandboxHome()); + expect(resolve(homedir())).not.toBe(resolve(realUserHome())); }); - it("keeps every resolved account storage path out of the real user home", () => { - const real = realUserHome(); - + it("keeps every resolved account storage path inside the sandbox", () => { setStoragePath(null); - expect(isUnder(real, getStoragePath())).toBe(false); + expectSandboxed(getStoragePath()); + // Per-project storage is namespaced under `getConfigDir()` rather than + // under the project itself, so this follows the redirected home even + // when the checkout sits inside the real one, as it does on CI. setStoragePath(process.cwd()); - expect(isUnder(real, getStoragePath())).toBe(false); + expectSandboxed(getStoragePath()); setStoragePath(null); - expect(isUnder(real, getConfigDir())).toBe(false); + expectSandboxed(getConfigDir()); }); // LOG_DIR is captured at module scope from homedir(), so it only lands in // the sandbox if the override beat the import. That is the property a // setupFiles entry cannot provide and this whole mechanism exists for. it("beats module-scope homedir() capture", () => { - expect(isUnder(realUserHome(), LOG_DIR)).toBe(false); - expect(isUnder(process.env.OC_CODEX_TEST_HOME as string, LOG_DIR)).toBe(true); + expectSandboxed(LOG_DIR); + }); + + // Where the sandbox is a descendant of the real home, a guard keyed on the + // home tree alone would refuse this and redden the whole Windows job. + it("still allows account writes inside the sandbox", async () => { + // A private path, not the sandbox's own global store: every test file + // shares this HOME, so writing the real one would hand another file an + // empty pool mid-run. + const probeDir = join(sandboxHome(), "sandbox-write-probe"); + const probe = join(probeDir, ACCOUNTS_FILE_NAME); + setStoragePathDirect(probe); + try { + expectSandboxed(probe); + await expect( + saveAccounts({ version: 3, accounts: [], activeIndex: 0 }), + ).resolves.toBeUndefined(); + expect(existsSync(probe)).toBe(true); + } finally { + setStoragePathDirect(null); + rmSync(probeDir, { recursive: true, force: true }); + } }); it("refuses to write account storage that escapes into the real home", async () => { @@ -61,6 +107,38 @@ describe("test home isolation", () => { setStoragePathDirect(null); } }); + + // Guarding writes is not sufficient. A missing project store sends the + // loader to the GLOBAL one, whose path is re-resolved from `homedir()` at + // that moment, so a test that restores the real HOME reads the live pool + // through a path the current-storage check already waved through. + it("refuses to read the global fallback out of the real home", async () => { + const projectRoot = join(sandboxHome(), "fallback-probe-project"); + mkdirSync(join(projectRoot, ".opencode"), { recursive: true }); + setStoragePath(projectRoot); + expectSandboxed(getStoragePath()); + + // A directory that does not exist, never the real store: should this + // guard ever regress, the test has to fail rather than read credentials. + const restoredHome = join(realUserHome(), `.oc-codex-guard-probe-${process.pid}`); + const previousHome = process.env.HOME; + const previousProfile = process.env.USERPROFILE; + process.env.HOME = restoredHome; + process.env.USERPROFILE = restoredHome; + try { + expect(isUnder(realUserHome(), getConfigDir())).toBe(true); + await expect(loadAccounts()).rejects.toMatchObject({ + code: "TEST_HOME_ESCAPE", + }); + } finally { + if (previousHome === undefined) delete process.env.HOME; + else process.env.HOME = previousHome; + if (previousProfile === undefined) delete process.env.USERPROFILE; + else process.env.USERPROFILE = previousProfile; + setStoragePath(null); + rmSync(projectRoot, { recursive: true, force: true }); + } + }); }); describe("test home teardown", () => { @@ -118,6 +196,23 @@ describe("test home teardown", () => { } }); + // The case above only holds while nothing sets the flag for an inherited + // home. A caller exporting both variables would otherwise have its own + // directory recursively deleted, so the config must clear what it inherits. + it("clears an inherited ownership flag rather than trusting it", async () => { + const previousOwned = process.env.OC_CODEX_TEST_HOME_OWNED; + process.env.OC_CODEX_TEST_HOME_OWNED = "1"; + try { + // Re-runs the config's env setup. OC_CODEX_TEST_HOME is already set, + // so it takes the inherited branch and mints no directory. + await import("../vitest.config.js"); + expect(process.env.OC_CODEX_TEST_HOME_OWNED).toBeUndefined(); + } finally { + if (previousOwned === undefined) delete process.env.OC_CODEX_TEST_HOME_OWNED; + else process.env.OC_CODEX_TEST_HOME_OWNED = previousOwned; + } + }); + it("keeps a path outside the temp directory", async () => { const scratchRoot = join(process.cwd(), "tmp"); mkdirSync(scratchRoot, { recursive: true }); diff --git a/vitest.config.ts b/vitest.config.ts index d28c7049..dff3e8e3 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -21,6 +21,7 @@ process.env.OC_CODEX_TEST_HOME = isolatedHome; // Only a home this config minted may be removed once the run ends. One handed // in through the environment belongs to whoever set it. if (!inheritedHome) process.env.OC_CODEX_TEST_HOME_OWNED = '1'; +else delete process.env.OC_CODEX_TEST_HOME_OWNED; export default defineConfig({ test: { @@ -30,6 +31,10 @@ export default defineConfig({ HOME: isolatedHome, USERPROFILE: isolatedHome, OC_CODEX_TEST_HOME: isolatedHome, + // The OS credential store is the one place the HOME redirect cannot + // reach. An inherited opt-in would route fixture writes into the + // developer's real keychain; tests that need the backend opt in per test. + CODEX_KEYCHAIN: '0', }, // Four suites import the real `index.ts`, and the first one scheduled pays // the transform of a 4900-line entry plus its dependency graph: measured at From 159ea11c91fbb39ae73b87b0144168a27f36a5cc Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 13:37:51 -0500 Subject: [PATCH 8/8] fix(quota): make the on-demand probe ask upstream unconditionally The long-wait wake-up added earlier in this branch calls `quotaMonitor.runNow()` to find out whether a server-side reset has landed. `runNow` shared one code path with the unattended interval poll, including its enablement gate, so with `autoProtectCredits: false` and notifications off or unavailable it returned without contacting `/wham/usage` at all. The request then slept out the full block against capacity that may already have returned, which is exactly the failure the wake-up exists to prevent. Both switches govern the UNATTENDED poll: one opts out of spending a background request budget, the other out of desktop alerts. Neither says anything about a caller that is blocked and asking directly. `runNow` now forces the check past that gate while leaving rescheduling, and the notification opt-out itself, untouched - a forced probe still delivers no alert the user switched off, and still leaves no standing timer behind. Clearing a recovered quota was gated on `autoProtectCredits` too, and that gating was wrong in the same direction. The flag opts out of BLOCKING rotation, but the request path stamps `quotaExhaustedUntil` from 429 response headers regardless of it. An account blocked that way, in a configuration with the flag off, had nothing able to release it: the one routine that clears the stamp declined to run. The clear is now keyed on the evidence - no exhausted window observed, and usage reporting recovery - rather than on a flag about whether to impose blocks. Two tests drove `runNow()` while asserting the gate suppressed the poll. That assertion described the defect, so one moves to the scheduled path (`start()` plus timers), where the unattended invariant genuinely lives and where two sibling tests already hold it, and the other inverts to pin the new contract. Coverage of the unattended gate is unchanged. Verified by control run: `runNow` reverted to the unforced tick, and the force term removed from the check gate, each fail the forced-probe test; re-gating the recovery clear on `autoProtectCredits` fails the new credit-protection-off test. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 4e488d6 --- lib/quota-notifications.ts | 24 ++++++++++++++--- test/quota-notifications-fetch.test.ts | 24 +++++++++++++++++ test/quota-notifications.test.ts | 36 ++++++++++++++++++++++---- 3 files changed, 75 insertions(+), 9 deletions(-) diff --git a/lib/quota-notifications.ts b/lib/quota-notifications.ts index dc28f5ef..a97810b4 100644 --- a/lib/quota-notifications.ts +++ b/lib/quota-notifications.ts @@ -455,7 +455,11 @@ export function createQuotaMonitor(overrides: Partial = {}) schedule(intervalMs, expectedGeneration); }; - const tick = async (expectedGeneration: number, reschedule: boolean): Promise => { + const tick = async ( + expectedGeneration: number, + reschedule: boolean, + force = false, + ): Promise => { if (disposed || expectedGeneration !== generation) return; if (running) { if (reschedule) scheduleNext(expectedGeneration); @@ -475,7 +479,11 @@ export function createQuotaMonitor(overrides: Partial = {}) // request simply retries on the next interval, so it cannot turn usage // endpoint throttling into a routing block. keepPolling = config.autoProtectCredits !== false || notificationsEnabled; - if (keepPolling) await check(config, expectedGeneration); + // Both switches govern the UNATTENDED poll. A forced check is an + // on-demand request from a caller that is blocked on the answer, so + // honouring them here would let `runNow()` return without asking + // upstream anything at all. + if (force || keepPolling) await check(config, expectedGeneration); } catch (error) { logDebug(`Quota monitor tick failed: ${(error as Error).message}`); } finally { @@ -510,7 +518,7 @@ export function createQuotaMonitor(overrides: Partial = {}) }, dispose: disposeMonitor, async runNow() { - await tick(generation, false); + await tick(generation, false, true); }, }; } @@ -551,7 +559,15 @@ async function fetchUsageForAccount( // only the proactive routing guard is unavailable until the next poll. logWarn(`Failed to persist exhausted usage quota: ${(error as Error).message}`); } - } else if (autoProtectCredits && isUsageQuotaRecovered([usage.primary, usage.secondary])) { + // Clearing a stale block is not part of the credit guard. + // `autoProtectCredits` opts out of BLOCKING rotation, while the request + // path stamps a block from 429 headers regardless of it. Gating the + // clear on it too left those accounts blocked with nothing able to + // clear them, so the long-wait probe could never wake. + } else if ( + quotaExhaustedResetAtMs === undefined && + isUsageQuotaRecovered([usage.primary, usage.secondary]) + ) { try { if (await persistUsageQuotaRecovery(account)) onCredentialsPersisted(); } catch { diff --git a/test/quota-notifications-fetch.test.ts b/test/quota-notifications-fetch.test.ts index befad211..47b3e23c 100644 --- a/test/quota-notifications-fetch.test.ts +++ b/test/quota-notifications-fetch.test.ts @@ -243,6 +243,30 @@ describe("default quota fetch path", () => { expect(onCredentialsPersisted).toHaveBeenCalledOnce(); }); + // Setting and clearing a block are not symmetric. `autoProtectCredits` + // opts out of BLOCKING rotation, but the request path still stamps a block + // from 429 headers regardless of it, so gating the clear on it too left + // those accounts blocked with nothing able to release them. + it("clears recovered quota even with credit protection switched off", async () => { + ensureCodexUsageAccessToken.mockResolvedValue({ accessToken: "access-1", persisted: false }); + fetchCodexUsage.mockResolvedValue({ rate_limit: { + primary_window: { used_percent: 10, limit_window_seconds: 18_000 }, + secondary_window: { used_percent: 20, limit_window_seconds: 604_800 }, + } }); + persistUsageQuotaRecovery.mockResolvedValue(true); + await monitorWith({ + loadConfig: () => ({ + enabled: true, + autoProtectCredits: false, + intervalMs: 1_000, + notifyEveryCheck: true, + thresholds: [25, 10, 0], + }), + notify: vi.fn().mockResolvedValue(true), + }).runNow(); + expect(persistUsageQuotaRecovery).toHaveBeenCalledWith(storage.accounts[0]); + }); + it.each([ {}, { rate_limit: { primary_window: { used_percent: 0, limit_window_seconds: 0 } } }, diff --git a/test/quota-notifications.test.ts b/test/quota-notifications.test.ts index a6f1b6be..74e799b5 100644 --- a/test/quota-notifications.test.ts +++ b/test/quota-notifications.test.ts @@ -399,6 +399,7 @@ describe("quota monitor lifecycle", () => { }); it("does not poll a configuration that can never deliver", async () => { + vi.useFakeTimers(); const loadStorage = vi.fn().mockResolvedValue(null); const monitor = createQuotaMonitor({ // Enabled, but with no thresholds and no every-check alert there is @@ -406,11 +407,16 @@ describe("quota monitor lifecycle", () => { loadConfig: () => ({ enabled: true, autoProtectCredits: false, intervalMs: 1_000, notifyEveryCheck: false, thresholds: [] }), loadStorage, notificationsSupported: () => true, + initialDelayMs: 10, }); - await monitor.runNow(); + // Driven through the scheduled poll rather than `runNow()`, which is + // forced by design for the on-demand caller. + monitor.start(); + await vi.advanceTimersByTimeAsync(10); expect(loadStorage).not.toHaveBeenCalled(); + monitor.dispose(); }); it("keeps polling on the configured interval while enabled", async () => { @@ -446,16 +452,36 @@ describe("quota monitor lifecycle", () => { expect(loadStorage).not.toHaveBeenCalled(); }); - it("does not poll accounts when notifications and credit protection are disabled", async () => { - const loadStorage = vi.fn().mockResolvedValue(null); + // Both switches govern the UNATTENDED poll, an invariant the two scheduling + // tests above already hold. `runNow()` is the on-demand path, and its one + // production caller is the all-accounts rate-limit wait, which is blocked + // until it learns whether upstream capacity has returned. Honouring the + // switches here left that wait asleep through a server-side reset, since + // nothing local changes when a reset is granted. + it("polls on demand even when notifications and credit protection are disabled", async () => { + const loadStorage = vi.fn().mockResolvedValue({ + version: 3 as const, + accounts: [{ refreshToken: "token", addedAt: 0, lastUsed: 0 }], + activeIndex: 0, + }); + const fetchSummary = vi.fn().mockResolvedValue(accountUsage({ + fiveHourUsed: 10, + weeklyUsed: 10, + })); + const notify = vi.fn(); const monitor = createQuotaMonitor({ - loadConfig: () => ({ enabled: true, autoProtectCredits: false, intervalMs: 1_000, notifyEveryCheck: false, thresholds: [25, 10, 0] }), + loadConfig: () => ({ enabled: false, autoProtectCredits: false, intervalMs: 1_000, notifyEveryCheck: false, thresholds: [25, 10, 0] }), loadStorage, + fetchSummary, + notify, notificationsSupported: () => false, }); await monitor.runNow(); - expect(loadStorage).not.toHaveBeenCalled(); + + expect(fetchSummary).toHaveBeenCalledOnce(); + // Forcing the probe must not deliver an alert the user switched off. + expect(notify).not.toHaveBeenCalled(); }); it("polls to protect Credits even when desktop notifications are unavailable", async () => {