fix: close storage and config review gaps

Author: ndycode

lib/config.ts

@@ -133,10 +133,11 @@ function withConfigMutationLock<T>(fn: () => Promise<T>): Promise<T> {
 
 async function withConfigProcessLock<T>(fn: () => Promise<T>): Promise<T> {
 	let lastError: NodeJS.ErrnoException | null = null;
+	let attempt = 0;
 
 	await fs.mkdir(dirname(CONFIG_PATH), { recursive: true });
 
-	for (let attempt = 0; attempt < CONFIG_LOCK_RETRY_ATTEMPTS; attempt += 1) {
+	while (attempt < CONFIG_LOCK_RETRY_ATTEMPTS) {
 		try {
 			const handle = await fs.open(CONFIG_LOCK_PATH, "wx", 0o600);
 			try {
@@ -167,6 +168,7 @@ async function withConfigProcessLock<T>(fn: () => Promise<T>): Promise<T> {
 						Math.min(CONFIG_LOCK_RETRY_BASE_DELAY_MS * 2 ** attempt, CONFIG_LOCK_RETRY_MAX_DELAY_MS),
 					),
 				);
+				attempt += 1;
 				continue;
 			}
 			throw error;
@@ -178,15 +180,20 @@ async function withConfigProcessLock<T>(fn: () => Promise<T>): Promise<T> {
 
 async function renameConfigWithWindowsRetry(sourcePath: string, destinationPath: string): Promise<void> {
 	let lastError: NodeJS.ErrnoException | null = null;
-	for (let attempt = 0; attempt < 5; attempt += 1) {
+	for (let attempt = 0; attempt < CONFIG_LOCK_RETRY_ATTEMPTS; attempt += 1) {
 		try {
 			await fs.rename(sourcePath, destinationPath);
 			return;
 		} catch (error) {
 			const code = (error as NodeJS.ErrnoException).code;
 			if (code === "EPERM" || code === "EBUSY") {
 				lastError = error as NodeJS.ErrnoException;
-				await new Promise((resolve) => setTimeout(resolve, 10 * 2 ** attempt));
+				await new Promise((resolve) =>
+					setTimeout(
+						resolve,
+						Math.min(CONFIG_LOCK_RETRY_BASE_DELAY_MS * 2 ** attempt, CONFIG_LOCK_RETRY_MAX_DELAY_MS),
+					),
+				);
 				continue;
 			}
 			throw error;

lib/storage.ts

@@ -1,4 +1,4 @@
-import { promises as fs, existsSync } from "node:fs";
+import { constants as fsConstants, promises as fs, existsSync } from "node:fs";
 import { randomBytes } from "node:crypto";
 import { dirname, join } from "node:path";
 import { ACCOUNT_LIMITS } from "./constants.js";
@@ -159,12 +159,16 @@ async function renameWithWindowsRetry(sourcePath: string, destinationPath: strin
   }
 }
 
-async function copyFileWithWindowsRetry(sourcePath: string, destinationPath: string): Promise<void> {
+async function copyFileWithWindowsRetry(
+  sourcePath: string,
+  destinationPath: string,
+  mode = 0,
+): Promise<void> {
   let lastError: NodeJS.ErrnoException | null = null;
 
   for (let attempt = 0; attempt < WINDOWS_RENAME_RETRY_ATTEMPTS; attempt += 1) {
     try {
-      await fs.copyFile(sourcePath, destinationPath);
+      await fs.copyFile(sourcePath, destinationPath, mode);
       return;
     } catch (error) {
       if (isWindowsLockError(error)) {
@@ -1136,7 +1140,10 @@ async function loadFlaggedAccountsUnlocked(
 }
 
 export async function loadFlaggedAccounts(): Promise<FlaggedAccountStorageV1> {
-	return withStorageLock(async () => loadFlaggedAccountsUnlocked());
+	return withStorageLock(async () => {
+		const accountsSnapshot = await loadAccountsInternal(saveAccountsUnlocked);
+		return loadFlaggedAccountsUnlocked(accountsSnapshot);
+	});
 }
 
 /**
@@ -1291,10 +1298,7 @@ function previewImportAccountsAgainstExistingNormalized(
 export async function backupRawAccountsFile(filePath: string, force = true): Promise<void> {
 	await withStorageLock(async () => {
 		const resolvedPath = resolvePath(filePath);
-
-		if (!force && existsSync(resolvedPath)) {
-			throw new Error(`File already exists: ${resolvedPath}`);
-		}
+		const copyMode = force ? 0 : fsConstants.COPYFILE_EXCL;
 
 		await migrateLegacyProjectStorageIfNeeded(saveAccountsUnlocked);
 		const storagePath = getStoragePath();
@@ -1303,7 +1307,14 @@ export async function backupRawAccountsFile(filePath: string, force = true): Pro
 		}
 
 		await fs.mkdir(dirname(resolvedPath), { recursive: true });
-		await copyFileWithWindowsRetry(storagePath, resolvedPath);
+		try {
+			await copyFileWithWindowsRetry(storagePath, resolvedPath, copyMode);
+		} catch (error) {
+			if ((error as NodeJS.ErrnoException).code === "EEXIST") {
+				throw new Error(`File already exists: ${resolvedPath}`);
+			}
+			throw error;
+		}
 		await fs.chmod(resolvedPath, 0o600).catch((chmodErr) => {
 			log.warn("Failed to restrict backup file permissions", {
 				path: resolvedPath,

test/plugin-config.test.ts

@@ -861,6 +861,46 @@ describe('Plugin Configuration', () => {
 			}
 		});
 
+		it('does not consume retry budget when stale lock cleanup succeeds repeatedly', async () => {
+			mockExistsSync.mockReturnValue(true);
+			const readSpy = vi.spyOn(fs.promises, 'readFile').mockResolvedValue(
+				JSON.stringify({ experimental: { syncFromCodexMultiAuth: { enabled: false } } }) as never
+			);
+			const mkdirSpy = vi.spyOn(fs.promises, 'mkdir').mockResolvedValue(undefined as never);
+			const writeSpy = vi.spyOn(fs.promises, 'writeFile').mockResolvedValue(undefined);
+			const renameSpy = vi.spyOn(fs.promises, 'rename').mockResolvedValue(undefined);
+			const unlinkSpy = vi.spyOn(fs.promises, 'unlink').mockResolvedValue(undefined);
+			const closeSpy = vi.fn(async () => undefined);
+			const staleError = Object.assign(new Error('lock exists'), { code: 'EEXIST' });
+			const openSpy = vi.spyOn(fs.promises, 'open');
+			for (let attempt = 0; attempt < 5; attempt += 1) {
+				openSpy.mockRejectedValueOnce(staleError as never);
+			}
+			openSpy.mockResolvedValue({
+				close: closeSpy,
+			} as never);
+			const statSpy = vi.spyOn(fs.promises, 'stat').mockResolvedValue({
+				mtimeMs: Date.now() - 31_000,
+			} as never);
+
+			try {
+				await setSyncFromCodexMultiAuthEnabled(true);
+				expect(statSpy).toHaveBeenCalledTimes(5);
+				expect(openSpy).toHaveBeenCalledTimes(6);
+				expect(unlinkSpy).toHaveBeenCalledTimes(6);
+				expect(writeSpy).toHaveBeenCalled();
+				expect(renameSpy).toHaveBeenCalled();
+			} finally {
+				readSpy.mockRestore();
+				mkdirSpy.mockRestore();
+				writeSpy.mockRestore();
+				renameSpy.mockRestore();
+				unlinkSpy.mockRestore();
+				openSpy.mockRestore();
+				statSpy.mockRestore();
+			}
+		});
+
 		it('reads the persisted sync toggle flag', () => {
 			expect(getSyncFromCodexMultiAuthEnabled({ experimental: { syncFromCodexMultiAuth: { enabled: true } } })).toBe(true);
 			expect(getSyncFromCodexMultiAuthEnabled({})).toBe(false);

test/storage.test.ts

@@ -1784,6 +1784,57 @@ describe("storage", () => {
       }
     });
 
+    it("reuses the loaded accounts snapshot inside loadFlaggedAccounts", async () => {
+      await saveAccounts({
+        version: 3,
+        activeIndex: 0,
+        activeIndexByFamily: {},
+        accounts: [
+          {
+            refreshToken: "account-refresh",
+            accountId: "account-id",
+            addedAt: 1,
+            lastUsed: 1,
+          },
+        ],
+      });
+      await saveFlaggedAccounts({
+        version: 1,
+        accounts: [
+          {
+            refreshToken: "account-refresh",
+            flaggedAt: 1,
+            addedAt: 1,
+            lastUsed: 1,
+          },
+          {
+            refreshToken: "orphan-refresh",
+            flaggedAt: 2,
+            addedAt: 2,
+            lastUsed: 2,
+          },
+        ],
+      });
+
+      const originalReadFile = fs.readFile.bind(fs);
+      const readSpy = vi.spyOn(fs, "readFile");
+      let accountReadCount = 0;
+      readSpy.mockImplementation(async (path, options) => {
+        if (String(path) === testStoragePath) {
+          accountReadCount++;
+        }
+        return originalReadFile(path as Parameters<typeof fs.readFile>[0], options as never);
+      });
+
+      try {
+        const flagged = await loadFlaggedAccounts();
+        expect(flagged.accounts.map((account) => account.refreshToken)).toEqual(["account-refresh"]);
+        expect(accountReadCount).toBe(1);
+      } finally {
+        readSpy.mockRestore();
+      }
+    });
+
     it("reuses the loaded accounts snapshot inside withFlaggedAccountsTransaction", async () => {
       await saveAccounts({
         version: 3,
@@ -1856,6 +1907,26 @@ describe("storage", () => {
       const raw = JSON.parse(await fs.readFile(backupPath, "utf-8")) as { accounts: Array<{ refreshToken: string }> };
       expect(raw.accounts[0]?.refreshToken).toBe("backup-refresh");
     });
+
+    it("surfaces a copy-time EEXIST when backing up raw accounts without force", async () => {
+      await saveAccounts({
+        version: 3,
+        activeIndex: 0,
+        activeIndexByFamily: {},
+        accounts: [{ accountId: "existing", refreshToken: "ref-existing", addedAt: 1, lastUsed: 1 }],
+      });
+
+      const backupPath = join(testWorkDir, "raw-backup-race.json");
+      const copySpy = vi.spyOn(fs, "copyFile").mockRejectedValue(
+        Object.assign(new Error("exists"), { code: "EEXIST" }) as never,
+      );
+
+      try {
+        await expect(backupRawAccountsFile(backupPath, false)).rejects.toThrow(/File already exists/);
+      } finally {
+        copySpy.mockRestore();
+      }
+    });
   });
 
   describe("setStoragePath", () => {