diff --git a/lib/unified-settings.ts b/lib/unified-settings.ts index d0303b22..a35fb261 100644 --- a/lib/unified-settings.ts +++ b/lib/unified-settings.ts @@ -385,7 +385,7 @@ function writeSettingsRecordSync( */ async function writeSettingsRecordAsync( record: JsonRecord, - options?: { skipBackupSnapshot?: boolean }, + options?: { skipBackupSnapshot?: boolean; expectedMtimeMs?: number | null }, ): Promise { await fs.mkdir(getCodexMultiAuthDir(), { recursive: true }); const payload = normalizeForWrite(record); @@ -397,6 +397,24 @@ async function writeSettingsRecordAsync( await fs.writeFile(tempPath, data, "utf8"); let moved = false; try { + if (options && "expectedMtimeMs" in options) { + let currentMtimeMs: number | null = null; + try { + currentMtimeMs = (await fs.stat(UNIFIED_SETTINGS_PATH)).mtimeMs; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code !== "ENOENT") { + throw error; + } + } + if (currentMtimeMs !== options.expectedMtimeMs) { + const staleError = new Error( + "Unified settings changed on disk during save; retrying with latest state.", + ) as NodeJS.ErrnoException; + staleError.code = "ESTALE"; + throw staleError; + } + } for (let attempt = 0; attempt < 5; attempt += 1) { try { await fs.rename(tempPath, UNIFIED_SETTINGS_PATH); @@ -420,6 +438,18 @@ async function writeSettingsRecordAsync( } } +async function getUnifiedSettingsMtimeMs(): Promise { + try { + return (await fs.stat(UNIFIED_SETTINGS_PATH)).mtimeMs; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === "ENOENT") { + return null; + } + throw error; + } +} + async function enqueueSettingsWrite(task: () => Promise): Promise { const run = settingsWriteQueue.catch(() => {}).then(task); settingsWriteQueue = run.then( @@ -492,12 +522,25 @@ export function saveUnifiedPluginConfigSync(pluginConfig: JsonRecord): void { */ export async function saveUnifiedPluginConfig(pluginConfig: JsonRecord): Promise { await enqueueSettingsWrite(async () => { - const { record, usedBackup } = await readSettingsRecordAsyncInternal(); - const nextRecord = record ?? {}; - nextRecord.pluginConfig = { ...pluginConfig }; - await writeSettingsRecordAsync(nextRecord, { - skipBackupSnapshot: usedBackup, - }); + let expectedMtimeMs = await getUnifiedSettingsMtimeMs(); + let { record, usedBackup } = await readSettingsRecordAsyncInternal(); + for (let attempt = 0; attempt < 3; attempt += 1) { + const nextRecord = record ?? {}; + nextRecord.pluginConfig = { ...pluginConfig }; + try { + await writeSettingsRecordAsync(nextRecord, { + skipBackupSnapshot: usedBackup, + expectedMtimeMs, + }); + return; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ESTALE" || attempt >= 2) { + throw error; + } + expectedMtimeMs = await getUnifiedSettingsMtimeMs(); + ({ record, usedBackup } = await readSettingsRecordAsyncInternal()); + } + } }); } @@ -538,11 +581,24 @@ export async function saveUnifiedDashboardSettings( dashboardDisplaySettings: JsonRecord, ): Promise { await enqueueSettingsWrite(async () => { - const { record, usedBackup } = await readSettingsRecordAsyncInternal(); - const nextRecord = record ?? {}; - nextRecord.dashboardDisplaySettings = { ...dashboardDisplaySettings }; - await writeSettingsRecordAsync(nextRecord, { - skipBackupSnapshot: usedBackup, - }); + let expectedMtimeMs = await getUnifiedSettingsMtimeMs(); + let { record, usedBackup } = await readSettingsRecordAsyncInternal(); + for (let attempt = 0; attempt < 3; attempt += 1) { + const nextRecord = record ?? {}; + nextRecord.dashboardDisplaySettings = { ...dashboardDisplaySettings }; + try { + await writeSettingsRecordAsync(nextRecord, { + skipBackupSnapshot: usedBackup, + expectedMtimeMs, + }); + return; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ESTALE" || attempt >= 2) { + throw error; + } + expectedMtimeMs = await getUnifiedSettingsMtimeMs(); + ({ record, usedBackup } = await readSettingsRecordAsyncInternal()); + } + } }); } diff --git a/test/unified-settings.test.ts b/test/unified-settings.test.ts index 687f09b6..bad0b1db 100644 --- a/test/unified-settings.test.ts +++ b/test/unified-settings.test.ts @@ -736,6 +736,93 @@ describe("unified settings", () => { }); }); + it("retries plugin section write against newer on-disk record to avoid stale overwrite", async () => { + const { + getUnifiedSettingsPath, + saveUnifiedPluginConfig, + } = await import("../lib/unified-settings.js"); + + await fs.writeFile( + getUnifiedSettingsPath(), + JSON.stringify( + { + version: 1, + pluginConfig: { codexMode: false, retries: 9 }, + dashboardDisplaySettings: { + menuShowLastUsed: true, + uiThemePreset: "green", + }, + experimentalDraft: { + enabled: false, + panels: ["future"], + }, + }, + null, + 2, + ), + "utf8", + ); + + const originalWriteFile = fs.writeFile; + const writeSpy = vi.spyOn(fs, "writeFile"); + let injectedConcurrentWrite = false; + writeSpy.mockImplementation(async (...args: Parameters) => { + const [filePath, data] = args; + const result = await originalWriteFile(...args); + if (!injectedConcurrentWrite && String(filePath).includes(".tmp")) { + injectedConcurrentWrite = true; + await originalWriteFile( + getUnifiedSettingsPath(), + `${JSON.stringify( + { + version: 1, + pluginConfig: { codexMode: false, retries: 9 }, + dashboardDisplaySettings: { + menuShowLastUsed: false, + uiThemePreset: "purple", + }, + experimentalDraft: { + enabled: true, + panels: ["fresh"], + }, + }, + null, + 2, + )}\n`, + "utf8", + ); + } + return result; + }); + + try { + await saveUnifiedPluginConfig({ codexMode: true, fetchTimeoutMs: 45_000 }); + } finally { + writeSpy.mockRestore(); + } + + expect(injectedConcurrentWrite).toBe(true); + const parsed = JSON.parse( + await fs.readFile(getUnifiedSettingsPath(), "utf8"), + ) as { + pluginConfig?: Record; + dashboardDisplaySettings?: Record; + experimentalDraft?: Record; + }; + expect(parsed.pluginConfig).toEqual({ + codexMode: true, + fetchTimeoutMs: 45_000, + }); + expect(parsed.dashboardDisplaySettings).toEqual({ + menuShowLastUsed: false, + uiThemePreset: "purple", + }); + expect(parsed.experimentalDraft).toEqual({ + enabled: true, + panels: ["fresh"], + }); + }); + it("falls back to backup when the primary settings file is unreadable", async () => { const { saveUnifiedPluginConfig,