Skip to content

fix: stop the suite destroying real credentials, and stop short waits aborting a request hours early #258

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Nowaker wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix: stop the suite destroying real credentials, and stop short waits aborting a request hours early #258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
854a25d
test(isolation): stop the suite writing to the real account store
Nowaker Sep 17, 2026
d9f5f81
test(vitest): stop index.ts suites timing out on their own import
Nowaker Sep 17, 2026
e131d69
fix(retry): charge the rate-limit budget by wait length, not by attempt
Nowaker Sep 17, 2026
db13030
fix(rate-limit): re-probe upstream during a long all-accounts wait
Nowaker Sep 17, 2026
6c4c355
fix(accounts): never let a failed load empty a working account pool
Nowaker Sep 17, 2026
2825200
test(vitest): remove the temp home each run mints
Nowaker Sep 17, 2026
fc8b371
test(isolation): keep the sandbox guard working where tmp sits under …
Nowaker Sep 17, 2026
159ea11
fix(quota): make the on-demand probe ask upstream unconditionally
Nowaker Sep 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
200 changes: 177 additions & 23 deletions index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -1675,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<typeof setTimeout> | undefined;
const cancelEmptyReloadRetry = (): void => {
clearTimeout(emptyReloadRetryTimer);
emptyReloadRetryTimer = undefined;
emptyReloadRetries = 0;
};
const scheduleEmptyReloadRetry = (retry: () => Promise<void>): 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<void> => {
if (!cachedAccountManager) return;
const previous = cachedAccountManager;
Expand All @@ -1693,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
Expand Down Expand Up @@ -1725,21 +1787,41 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => {
accountsWatcherDisposed = true;
unsubscribeAccountsPath?.();
stopAccountsWatcher();
cancelEmptyReloadRetry();
unregisterCleanup(disposeAccountsWatcher);
};
const readAccountsDigest = async (path: string): Promise<string | undefined> => {
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<void> => {
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
Expand All @@ -1760,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
Expand All @@ -1775,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");
Expand All @@ -1795,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;
Expand All @@ -1821,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);
Expand Down Expand Up @@ -2396,9 +2488,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;
Expand Down Expand Up @@ -2450,25 +2553,44 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => {
totalMs: number,
message: string,
intervalMs: number = 5000,
probeUpstream?: () => Promise<boolean>,
): Promise<void> => {
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(
`${message} (${waitLabel} remaining)`,
"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 {
Expand All @@ -2477,6 +2599,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<boolean> => {
try {
await quotaMonitor.runNow();
} catch (error) {
Comment on lines +2608 to +2614

@greptile-apps greptile-apps Bot Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 quota probe can noop

when autoProtectCredits is false and notifications are unavailable or disabled, runNow() follows the monitor's normal enablement policy and returns without checking /wham/usage. the countdown therefore keeps sleeping through a server-side reset even though this path expects an upstream probe. the added vitest replaces runNow with an unconditional callback, so it misses this configuration. use a forced on-demand probe that bypasses notification scheduling policy, or make runNow always perform one check.

Knowledge Base Used:

Prompt To Fix With AI 
This is a comment left during a code review.
Path: index.ts
Line: 2608-2614

Comment:
**quota probe can noop**

when `autoProtectCredits` is false and notifications are unavailable or disabled, `runNow()` follows the monitor's normal enablement policy and returns without checking `/wham/usage`. the countdown therefore keeps sleeping through a server-side reset even though this path expects an upstream probe. the added vitest replaces `runNow` with an unconditional callback, so it misses this configuration. use a forced on-demand probe that bypasses notification scheduling policy, or make `runNow` always perform one check.

**Knowledge Base Used:**
- [Quota monitoring and notifications](https://app.greptile.com/zeian/-/custom-context/knowledge-base/ndycode/oc-codex-multi-auth/-/docs/quota-monitoring-and-notifications.md)
- [Upstream reliability controls](https://app.greptile.com/zeian/-/custom-context/knowledge-base/ndycode/oc-codex-multi-auth/-/docs/upstream-reliability-controls.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

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<string>();
Expand Down Expand Up @@ -3795,10 +3943,16 @@ 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`;
await sleepWithCountdown(addJitter(waitMs, 0.2), countdownMessage);
await sleepWithCountdown(
addJitter(waitMs, 0.2),
countdownMessage,
undefined,
probeUpstreamBlockLifted,
);
allRateLimitedRetries++;
continue;
}
Expand Down
24 changes: 20 additions & 4 deletions lib/quota-notifications.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,11 @@ export function createQuotaMonitor(overrides: Partial<MonitorDependencies> = {})
schedule(intervalMs, expectedGeneration);
};

const tick = async (expectedGeneration: number, reschedule: boolean): Promise<void> => {
const tick = async (
expectedGeneration: number,
reschedule: boolean,
force = false,
): Promise<void> => {
if (disposed || expectedGeneration !== generation) return;
if (running) {
if (reschedule) scheduleNext(expectedGeneration);
Expand All @@ -475,7 +479,11 @@ export function createQuotaMonitor(overrides: Partial<MonitorDependencies> = {})
// 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 {
Expand Down Expand Up @@ -510,7 +518,7 @@ export function createQuotaMonitor(overrides: Partial<MonitorDependencies> = {})
},
dispose: disposeMonitor,
async runNow() {
await tick(generation, false);
await tick(generation, false, true);
},
};
}
Expand Down Expand Up @@ -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 {
Expand Down
Loading