diff --git a/lib/storage.ts b/lib/storage.ts index 2b92654..30fafba 100644 --- a/lib/storage.ts +++ b/lib/storage.ts @@ -109,6 +109,7 @@ export function formatStorageErrorHint(error: unknown, path: string): string { } let storageMutex: Promise = Promise.resolve(); +let accountFileMutex: Promise = Promise.resolve(); function withStorageLock(fn: () => Promise): Promise { const previousMutex = storageMutex; @@ -119,6 +120,23 @@ function withStorageLock(fn: () => Promise): Promise { return previousMutex.then(fn).finally(() => releaseLock()); } +function withAccountFileMutex(fn: () => Promise): Promise { + const previousMutex = accountFileMutex; + let releaseLock: () => void; + accountFileMutex = new Promise((resolve) => { + releaseLock = resolve; + }); + return previousMutex.then(fn).finally(() => releaseLock()); +} + +async function withStorageSerializedFileLock(path: string, fn: () => Promise): Promise { + // Serialize file-lock acquisition to keep save ordering deterministic. + // Acquisition order: file-queue mutex -> file lock -> storage mutex. + return withAccountFileMutex(() => + withAccountFileLock(path, () => withStorageLock(fn)), + ); +} + type AnyAccountStorage = AccountStorageV1 | AccountStorageV3; type AccountLike = { @@ -318,16 +336,38 @@ function getAccountsLockPath(path: string): string { return `${path}.lock`; } +async function releaseStorageLockFallback(lockPath: string): Promise { + try { + await fs.rm(lockPath, { force: true }); + } catch (error) { + log.debug("Best-effort lock cleanup fallback failed", { + path: lockPath, + error: String(error), + }); + } +} + async function withAccountFileLock(path: string, fn: () => Promise): Promise { + const lockPath = getAccountsLockPath(path); await fs.mkdir(dirname(path), { recursive: true }); - const lock = await acquireFileLock(getAccountsLockPath(path), ACCOUNT_STORAGE_LOCK_OPTIONS); + const lock = await acquireFileLock(lockPath, ACCOUNT_STORAGE_LOCK_OPTIONS); try { return await fn(); } finally { - await lock.release(); + try { + await lock.release(); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code !== "ENOENT") { + log.warn("Failed to release account storage lock", { + path: lockPath, + error: String(error), + }); + await releaseStorageLockFallback(lockPath); + } + } } } - async function copyFileWithRetry( sourcePath: string, destinationPath: string, @@ -1242,14 +1282,13 @@ export async function withAccountStorageTransaction( ) => Promise, ): Promise { const path = getStoragePath(); - return withAccountFileLock(path, () => - withStorageLock(async () => { - const current = await loadAccountsInternal(saveAccountsUnlocked); - return handler(current, saveAccountsUnlocked); - }), - ); + return withStorageSerializedFileLock(path, async () => { + const current = await loadAccountsInternal(saveAccountsUnlocked); + return handler(current, saveAccountsUnlocked); + }); } + /** * Persists account storage to disk using atomic write (temp file + rename). * Creates the Codex multi-auth storage directory if it doesn't exist. @@ -1259,23 +1298,21 @@ export async function withAccountStorageTransaction( */ export async function saveAccounts(storage: AccountStorageV3): Promise { const path = getStoragePath(); - return withAccountFileLock(path, () => - withStorageLock(async () => { - await saveAccountsUnlocked(storage); - }), - ); + return withStorageSerializedFileLock(path, async () => { + await saveAccountsUnlocked(storage); + }); } + /** * Deletes the account storage file from disk. * Silently ignores if file doesn't exist. */ export async function clearAccounts(): Promise { const path = getStoragePath(); - return withAccountFileLock(path, () => - withStorageLock(async () => { - const walPath = getAccountsWalPath(path); - const backupPaths = getAccountsBackupRecoveryCandidates(path); + return withStorageSerializedFileLock(path, async () => { + const walPath = getAccountsWalPath(path); + const backupPaths = getAccountsBackupRecoveryCandidates(path); const clearPath = async (targetPath: string): Promise => { try { await fs.unlink(targetPath); @@ -1295,10 +1332,10 @@ export async function clearAccounts(): Promise { } catch { // Individual path cleanup is already best-effort with per-artifact logging. } - }), - ); + }); } + function normalizeFlaggedStorage(data: unknown): FlaggedAccountStorageV1 { if (!isRecord(data) || data.version !== 1 || !Array.isArray(data.accounts)) { return { version: 1, accounts: [] }; @@ -1597,3 +1634,5 @@ export async function rotateStoredSecretEncryption(): Promise<{ flaggedAccounts: flaggedCount, }; } + + diff --git a/test/storage.test.ts b/test/storage.test.ts index 3c3157e..82e356b 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { promises as fs, existsSync } from "node:fs"; import { dirname, join } from "node:path"; import { tmpdir } from "node:os"; +import * as fileLock from "../lib/file-lock.js"; import { getConfigDir, getProjectStorageKey } from "../lib/storage/paths.js"; import { deduplicateAccounts, @@ -1539,6 +1540,105 @@ describe("storage", () => { statSpy.mockRestore(); }); + it("falls back to best-effort cleanup when lock.release throws EBUSY", async () => { + const now = Date.now(); + const storage = { + version: 3 as const, + activeIndex: 0, + accounts: [{ refreshToken: "token", addedAt: now, lastUsed: now }], + }; + const lockPath = `${testStoragePath}.lock`; + + const acquireSpy = vi.spyOn(fileLock, "acquireFileLock").mockResolvedValue({ + path: lockPath, + release: async () => { + const err = new Error("EBUSY release") as NodeJS.ErrnoException; + err.code = "EBUSY"; + throw err; + }, + }); + const rmSpy = vi.spyOn(fs, "rm"); + try { + await saveAccounts(storage); + + expect(existsSync(testStoragePath)).toBe(true); + expect(rmSpy).toHaveBeenCalledWith(lockPath, { force: true }); + } finally { + rmSpy.mockRestore(); + acquireSpy.mockRestore(); + } + }); + + it("falls back to best-effort cleanup when lock.release throws EPERM", async () => { + const now = Date.now(); + const storage = { + version: 3 as const, + activeIndex: 0, + accounts: [{ refreshToken: "token", addedAt: now, lastUsed: now }], + }; + const lockPath = `${testStoragePath}.lock`; + + const acquireSpy = vi.spyOn(fileLock, "acquireFileLock").mockResolvedValue({ + path: lockPath, + release: async () => { + const err = new Error("EPERM release") as NodeJS.ErrnoException; + err.code = "EPERM"; + throw err; + }, + }); + const rmSpy = vi.spyOn(fs, "rm"); + try { + await saveAccounts(storage); + + expect(existsSync(testStoragePath)).toBe(true); + expect(rmSpy).toHaveBeenCalledWith(lockPath, { force: true }); + } finally { + rmSpy.mockRestore(); + acquireSpy.mockRestore(); + } + }); + + it("serializes lock acquisition across concurrent saves", async () => { + const now = Date.now(); + let activeLocks = 0; + let maxActiveLocks = 0; + + const acquireSpy = vi.spyOn(fileLock, "acquireFileLock").mockImplementation(async (path) => { + activeLocks += 1; + maxActiveLocks = Math.max(maxActiveLocks, activeLocks); + return { + path, + release: async () => { + await new Promise((resolve) => setTimeout(resolve, 20)); + activeLocks -= 1; + }, + }; + }); + try { + await Promise.all([ + saveAccounts({ + version: 3 as const, + activeIndex: 0, + accounts: [{ refreshToken: "token-1", addedAt: now + 1, lastUsed: now + 1 }], + }), + saveAccounts({ + version: 3 as const, + activeIndex: 0, + accounts: [{ refreshToken: "token-2", addedAt: now + 2, lastUsed: now + 2 }], + }), + saveAccounts({ + version: 3 as const, + activeIndex: 0, + accounts: [{ refreshToken: "token-3", addedAt: now + 3, lastUsed: now + 3 }], + }), + ]); + + expect(maxActiveLocks).toBe(1); + } finally { + acquireSpy.mockRestore(); + } + }); + it("retries backup copyFile on transient EBUSY and succeeds", async () => { const now = Date.now(); const storage = {