From 02d34d103cb974f0e3ee313266438e72368703fd Mon Sep 17 00:00:00 2001 From: ndycode Date: Wed, 4 Mar 2026 06:38:57 +0800 Subject: [PATCH 1/4] fix(storage): harden lock cleanup for stale/failed storage locks - cleanup stale/dead process lock artifacts before acquiring account lock - ensure lock release always attempts fallback cleanup - keep clearAccounts/saveTransactions serialized across file and memory locks Co-authored-by: Codex --- lib/storage.ts | 97 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 77 insertions(+), 20 deletions(-) diff --git a/lib/storage.ts b/lib/storage.ts index 2b92654..bfcd924 100644 --- a/lib/storage.ts +++ b/lib/storage.ts @@ -119,6 +119,10 @@ function withStorageLock(fn: () => Promise): Promise { return previousMutex.then(fn).finally(() => releaseLock()); } +async function withStorageSerializedFileLock(path: string, fn: () => Promise): Promise { + return withStorageLock(() => withAccountFileLock(path, fn)); +} + type AnyAccountStorage = AccountStorageV1 | AccountStorageV3; type AccountLike = { @@ -318,16 +322,70 @@ function getAccountsLockPath(path: string): string { return `${path}.lock`; } +async function releaseStorageLockFallback(lockPath: string): Promise { + try { + await fs.rm(lockPath, { force: true }); + } catch { + // Best-effort lock cleanup fallback. + } +} + +async function cleanupDeadProcessStorageLock(lockPath: string): Promise { + try { + const raw = await fs.readFile(lockPath, "utf-8"); + const parsed = JSON.parse(raw) as { pid?: number; acquiredAt?: number }; + const lockPid = Number(parsed?.pid); + const lockAcquiredAt = Number(parsed?.acquiredAt); + + if (Number.isFinite(lockPid) && lockPid > 0) { + let isDeadProcess = false; + try { + process.kill(lockPid, 0); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + isDeadProcess = code === "ESRCH"; + } + + if (isDeadProcess) { + await releaseStorageLockFallback(lockPath); + return; + } + } + + if (Number.isFinite(lockAcquiredAt) && Date.now() - lockAcquiredAt > ACCOUNT_STORAGE_LOCK_OPTIONS.staleAfterMs) { + await releaseStorageLockFallback(lockPath); + } + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === "ENOENT") { + return; + } + await releaseStorageLockFallback(lockPath); + } +} + async function withAccountFileLock(path: string, fn: () => Promise): Promise { + const lockPath = getAccountsLockPath(path); + await cleanupDeadProcessStorageLock(lockPath); 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 +1300,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 +1316,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 +1350,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 +1652,5 @@ export async function rotateStoredSecretEncryption(): Promise<{ flaggedAccounts: flaggedCount, }; } + + From d267377c3d7ac9b576fccc3fd850af3ea0e10afa Mon Sep 17 00:00:00 2001 From: ndycode Date: Wed, 4 Mar 2026 12:45:10 +0800 Subject: [PATCH 2/4] fix(storage): serialize account file lock queue Ensure account-storage mutations keep deterministic ordering while preserving the historical file-lock before in-process mutex acquisition sequence. Co-authored-by: Codex --- lib/storage.ts | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/lib/storage.ts b/lib/storage.ts index bfcd924..557036f 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,8 +120,22 @@ 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 { - return withStorageLock(() => withAccountFileLock(path, fn)); + // Serialize file-lock acquisition to keep save ordering deterministic, then + // preserve the historical lock order (file lock -> in-process mutex) so all + // account-storage mutation paths share the same acquisition sequence. + return withAccountFileMutex(() => + withAccountFileLock(path, () => withStorageLock(fn)), + ); } type AnyAccountStorage = AccountStorageV1 | AccountStorageV3; From 1f516d1320a8d6a2f360d5e033c6a496ac6c428e Mon Sep 17 00:00:00 2001 From: ndycode Date: Wed, 4 Mar 2026 12:59:48 +0800 Subject: [PATCH 3/4] fix(storage): remove racy pre-lock cleanup Drop pre-acquire dead-process lock cleanup and only run fallback lock-file deletion when lock.release() fails. Also align lock-order comment and add debug observability for fallback cleanup failures. Co-authored-by: Codex --- lib/storage.ts | 47 +++++++---------------------------------------- 1 file changed, 7 insertions(+), 40 deletions(-) diff --git a/lib/storage.ts b/lib/storage.ts index 557036f..30fafba 100644 --- a/lib/storage.ts +++ b/lib/storage.ts @@ -130,9 +130,8 @@ function withAccountFileMutex(fn: () => Promise): Promise { } async function withStorageSerializedFileLock(path: string, fn: () => Promise): Promise { - // Serialize file-lock acquisition to keep save ordering deterministic, then - // preserve the historical lock order (file lock -> in-process mutex) so all - // account-storage mutation paths share the same acquisition sequence. + // Serialize file-lock acquisition to keep save ordering deterministic. + // Acquisition order: file-queue mutex -> file lock -> storage mutex. return withAccountFileMutex(() => withAccountFileLock(path, () => withStorageLock(fn)), ); @@ -340,48 +339,16 @@ function getAccountsLockPath(path: string): string { async function releaseStorageLockFallback(lockPath: string): Promise { try { await fs.rm(lockPath, { force: true }); - } catch { - // Best-effort lock cleanup fallback. - } -} - -async function cleanupDeadProcessStorageLock(lockPath: string): Promise { - try { - const raw = await fs.readFile(lockPath, "utf-8"); - const parsed = JSON.parse(raw) as { pid?: number; acquiredAt?: number }; - const lockPid = Number(parsed?.pid); - const lockAcquiredAt = Number(parsed?.acquiredAt); - - if (Number.isFinite(lockPid) && lockPid > 0) { - let isDeadProcess = false; - try { - process.kill(lockPid, 0); - } catch (error) { - const code = (error as NodeJS.ErrnoException).code; - isDeadProcess = code === "ESRCH"; - } - - if (isDeadProcess) { - await releaseStorageLockFallback(lockPath); - return; - } - } - - if (Number.isFinite(lockAcquiredAt) && Date.now() - lockAcquiredAt > ACCOUNT_STORAGE_LOCK_OPTIONS.staleAfterMs) { - await releaseStorageLockFallback(lockPath); - } } catch (error) { - const code = (error as NodeJS.ErrnoException).code; - if (code === "ENOENT") { - return; - } - await releaseStorageLockFallback(lockPath); + 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 cleanupDeadProcessStorageLock(lockPath); await fs.mkdir(dirname(path), { recursive: true }); const lock = await acquireFileLock(lockPath, ACCOUNT_STORAGE_LOCK_OPTIONS); try { @@ -396,9 +363,9 @@ async function withAccountFileLock(path: string, fn: () => Promise): Promi path: lockPath, error: String(error), }); + await releaseStorageLockFallback(lockPath); } } - await releaseStorageLockFallback(lockPath); } } async function copyFileWithRetry( From 6c33f9ce2dad9f473f3d2c70eca030896b1a9ddc Mon Sep 17 00:00:00 2001 From: ndycode Date: Wed, 4 Mar 2026 13:07:07 +0800 Subject: [PATCH 4/4] test(storage): cover lock release fallback behavior Add regression tests for lock.release() EBUSY/EPERM fallback cleanup and concurrent save lock serialization to close outstanding PR feedback. Co-authored-by: Codex --- test/storage.test.ts | 100 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) 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 = {