From 06f2390687a5cf3ff85ba4bfe4705a43549d2487 Mon Sep 17 00:00:00 2001 From: Neil Daquioag Date: Sat, 18 Apr 2026 15:20:23 +0800 Subject: [PATCH 1/4] fix(settings): retry section writes against latest disk state (R-LOGIC-01) Addresses R-LOGIC-01 / R-TEST-01 from the deep runtime audit. The async unified-settings save paths read the current settings record, mutate one section, and write it back without checking whether another process wrote a newer settings.json in the meantime. That creates a stale-overwrite race. Fix: capture the observed settings mtime, check it immediately before rename inside writeSettingsRecordAsync, and on ESTALE re-read the latest settings record and re-apply the section-scoped mutation up to 3 times. Added a regression test that injects a concurrent external write between the read and write and verifies the unrelated sections from the external write are preserved while the intended pluginConfig mutation still lands. --- lib/unified-settings.ts | 82 +++++++++++++++++++++++++++------ test/unified-settings.test.ts | 87 +++++++++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 13 deletions(-) diff --git a/lib/unified-settings.ts b/lib/unified-settings.ts index d0303b22..61b02063 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 { record, usedBackup } = await readSettingsRecordAsyncInternal(); + let expectedMtimeMs = await getUnifiedSettingsMtimeMs(); + 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; + } + ({ record, usedBackup } = await readSettingsRecordAsyncInternal()); + expectedMtimeMs = await getUnifiedSettingsMtimeMs(); + } + } }); } @@ -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 { record, usedBackup } = await readSettingsRecordAsyncInternal(); + let expectedMtimeMs = await getUnifiedSettingsMtimeMs(); + 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; + } + ({ record, usedBackup } = await readSettingsRecordAsyncInternal()); + expectedMtimeMs = await getUnifiedSettingsMtimeMs(); + } + } }); } 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, From 3279218e0b872141f701254896be43906136f436 Mon Sep 17 00:00:00 2001 From: Neil Daquioag Date: Sat, 18 Apr 2026 17:03:36 +0800 Subject: [PATCH 2/4] fix(settings): capture optimistic mtime from the same read handle (PR #429) Responds to the PR #429 review comment about the optimistic mtime check. The previous patch captured the expected mtime in a separate fs.stat() after the read, leaving a TOCTOU window between reading the file contents and sampling its metadata. readSettingsRecordAsyncInternal now uses the mtime from the same open file handle used for the async read and threads that value through the ESTALE retry loop. Regression test remains green. --- lib/unified-settings.ts | 47 ++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/lib/unified-settings.ts b/lib/unified-settings.ts index 61b02063..4fab1c3d 100644 --- a/lib/unified-settings.ts +++ b/lib/unified-settings.ts @@ -25,6 +25,7 @@ let settingsWriteQueue: Promise = Promise.resolve(); type SettingsReadResult = { record: JsonRecord | null; usedBackup: boolean; + mtimeMs?: number | null; }; function isRetryableFsError(error: unknown): boolean { @@ -90,13 +91,33 @@ function readSettingsRecordSyncFromPath(filePath: string): JsonRecord | null { async function readSettingsRecordAsyncFromPath( filePath: string, ): Promise { + const result = await readSettingsRecordAsyncFromPathWithMetadata(filePath); + return result?.record ?? null; +} + +async function readSettingsRecordAsyncFromPathWithMetadata( + filePath: string, +): Promise<{ record: JsonRecord; mtimeMs: number | null } | null> { + let handle: Awaited> | null = null; try { - return parseSettingsRecord(await fs.readFile(filePath, "utf8")); + handle = await fs.open(filePath, "r"); + const [content, stats] = await Promise.all([ + handle.readFile({ encoding: "utf8" }), + handle.stat(), + ]); + return { + record: parseSettingsRecord(content), + mtimeMs: stats.mtimeMs, + }; } catch (error) { if ((error as NodeJS.ErrnoException | undefined)?.code === "ENOENT") { return null; } throw error; + } finally { + await handle?.close().catch(() => { + // Best-effort cleanup for read handles. + }); } } @@ -267,11 +288,11 @@ function readSettingsRecordSync(): JsonRecord | null { async function readSettingsRecordAsyncInternal(): Promise { const primaryExists = existsSync(UNIFIED_SETTINGS_PATH); try { - const primaryRecord = await readSettingsRecordAsyncFromPath( + const primaryRead = await readSettingsRecordAsyncFromPathWithMetadata( UNIFIED_SETTINGS_PATH, ); - if (primaryRecord) { - return { record: primaryRecord, usedBackup: false }; + if (primaryRead) { + return { record: primaryRead.record, usedBackup: false, mtimeMs: primaryRead.mtimeMs }; } } catch (error) { if (!shouldFallbackToSettingsBackup(primaryExists, error)) { @@ -279,15 +300,15 @@ async function readSettingsRecordAsyncInternal(): Promise { } const backupRecord = await readSettingsBackupAsync(); if (backupRecord) { - return { record: backupRecord, usedBackup: true }; + return { record: backupRecord, usedBackup: true, mtimeMs: null }; } if (isInvalidSettingsRecordError(error)) { - return { record: null, usedBackup: false }; + return { record: null, usedBackup: false, mtimeMs: null }; } throw error; } - return { record: null, usedBackup: false }; + return { record: null, usedBackup: false, mtimeMs: null }; } async function readSettingsRecordAsync(): Promise { @@ -522,8 +543,7 @@ export function saveUnifiedPluginConfigSync(pluginConfig: JsonRecord): void { */ export async function saveUnifiedPluginConfig(pluginConfig: JsonRecord): Promise { await enqueueSettingsWrite(async () => { - let { record, usedBackup } = await readSettingsRecordAsyncInternal(); - let expectedMtimeMs = await getUnifiedSettingsMtimeMs(); + let { record, usedBackup, mtimeMs: expectedMtimeMs } = await readSettingsRecordAsyncInternal(); for (let attempt = 0; attempt < 3; attempt += 1) { const nextRecord = record ?? {}; nextRecord.pluginConfig = { ...pluginConfig }; @@ -537,8 +557,7 @@ export async function saveUnifiedPluginConfig(pluginConfig: JsonRecord): Promise if ((error as NodeJS.ErrnoException).code !== "ESTALE" || attempt >= 2) { throw error; } - ({ record, usedBackup } = await readSettingsRecordAsyncInternal()); - expectedMtimeMs = await getUnifiedSettingsMtimeMs(); + ({ record, usedBackup, mtimeMs: expectedMtimeMs } = await readSettingsRecordAsyncInternal()); } } }); @@ -581,8 +600,7 @@ export async function saveUnifiedDashboardSettings( dashboardDisplaySettings: JsonRecord, ): Promise { await enqueueSettingsWrite(async () => { - let { record, usedBackup } = await readSettingsRecordAsyncInternal(); - let expectedMtimeMs = await getUnifiedSettingsMtimeMs(); + let { record, usedBackup, mtimeMs: expectedMtimeMs } = await readSettingsRecordAsyncInternal(); for (let attempt = 0; attempt < 3; attempt += 1) { const nextRecord = record ?? {}; nextRecord.dashboardDisplaySettings = { ...dashboardDisplaySettings }; @@ -596,8 +614,7 @@ export async function saveUnifiedDashboardSettings( if ((error as NodeJS.ErrnoException).code !== "ESTALE" || attempt >= 2) { throw error; } - ({ record, usedBackup } = await readSettingsRecordAsyncInternal()); - expectedMtimeMs = await getUnifiedSettingsMtimeMs(); + ({ record, usedBackup, mtimeMs: expectedMtimeMs } = await readSettingsRecordAsyncInternal()); } } }); From d582d0ddfc5dddd41991f73e5b8813fad598824e Mon Sep 17 00:00:00 2001 From: Neil Daquioag Date: Sat, 18 Apr 2026 17:03:56 +0800 Subject: [PATCH 3/4] Revert "fix(settings): capture optimistic mtime from the same read handle (PR #429)" This reverts commit 3279218e0b872141f701254896be43906136f436. --- lib/unified-settings.ts | 47 +++++++++++++---------------------------- 1 file changed, 15 insertions(+), 32 deletions(-) diff --git a/lib/unified-settings.ts b/lib/unified-settings.ts index 4fab1c3d..61b02063 100644 --- a/lib/unified-settings.ts +++ b/lib/unified-settings.ts @@ -25,7 +25,6 @@ let settingsWriteQueue: Promise = Promise.resolve(); type SettingsReadResult = { record: JsonRecord | null; usedBackup: boolean; - mtimeMs?: number | null; }; function isRetryableFsError(error: unknown): boolean { @@ -91,33 +90,13 @@ function readSettingsRecordSyncFromPath(filePath: string): JsonRecord | null { async function readSettingsRecordAsyncFromPath( filePath: string, ): Promise { - const result = await readSettingsRecordAsyncFromPathWithMetadata(filePath); - return result?.record ?? null; -} - -async function readSettingsRecordAsyncFromPathWithMetadata( - filePath: string, -): Promise<{ record: JsonRecord; mtimeMs: number | null } | null> { - let handle: Awaited> | null = null; try { - handle = await fs.open(filePath, "r"); - const [content, stats] = await Promise.all([ - handle.readFile({ encoding: "utf8" }), - handle.stat(), - ]); - return { - record: parseSettingsRecord(content), - mtimeMs: stats.mtimeMs, - }; + return parseSettingsRecord(await fs.readFile(filePath, "utf8")); } catch (error) { if ((error as NodeJS.ErrnoException | undefined)?.code === "ENOENT") { return null; } throw error; - } finally { - await handle?.close().catch(() => { - // Best-effort cleanup for read handles. - }); } } @@ -288,11 +267,11 @@ function readSettingsRecordSync(): JsonRecord | null { async function readSettingsRecordAsyncInternal(): Promise { const primaryExists = existsSync(UNIFIED_SETTINGS_PATH); try { - const primaryRead = await readSettingsRecordAsyncFromPathWithMetadata( + const primaryRecord = await readSettingsRecordAsyncFromPath( UNIFIED_SETTINGS_PATH, ); - if (primaryRead) { - return { record: primaryRead.record, usedBackup: false, mtimeMs: primaryRead.mtimeMs }; + if (primaryRecord) { + return { record: primaryRecord, usedBackup: false }; } } catch (error) { if (!shouldFallbackToSettingsBackup(primaryExists, error)) { @@ -300,15 +279,15 @@ async function readSettingsRecordAsyncInternal(): Promise { } const backupRecord = await readSettingsBackupAsync(); if (backupRecord) { - return { record: backupRecord, usedBackup: true, mtimeMs: null }; + return { record: backupRecord, usedBackup: true }; } if (isInvalidSettingsRecordError(error)) { - return { record: null, usedBackup: false, mtimeMs: null }; + return { record: null, usedBackup: false }; } throw error; } - return { record: null, usedBackup: false, mtimeMs: null }; + return { record: null, usedBackup: false }; } async function readSettingsRecordAsync(): Promise { @@ -543,7 +522,8 @@ export function saveUnifiedPluginConfigSync(pluginConfig: JsonRecord): void { */ export async function saveUnifiedPluginConfig(pluginConfig: JsonRecord): Promise { await enqueueSettingsWrite(async () => { - let { record, usedBackup, mtimeMs: expectedMtimeMs } = await readSettingsRecordAsyncInternal(); + let { record, usedBackup } = await readSettingsRecordAsyncInternal(); + let expectedMtimeMs = await getUnifiedSettingsMtimeMs(); for (let attempt = 0; attempt < 3; attempt += 1) { const nextRecord = record ?? {}; nextRecord.pluginConfig = { ...pluginConfig }; @@ -557,7 +537,8 @@ export async function saveUnifiedPluginConfig(pluginConfig: JsonRecord): Promise if ((error as NodeJS.ErrnoException).code !== "ESTALE" || attempt >= 2) { throw error; } - ({ record, usedBackup, mtimeMs: expectedMtimeMs } = await readSettingsRecordAsyncInternal()); + ({ record, usedBackup } = await readSettingsRecordAsyncInternal()); + expectedMtimeMs = await getUnifiedSettingsMtimeMs(); } } }); @@ -600,7 +581,8 @@ export async function saveUnifiedDashboardSettings( dashboardDisplaySettings: JsonRecord, ): Promise { await enqueueSettingsWrite(async () => { - let { record, usedBackup, mtimeMs: expectedMtimeMs } = await readSettingsRecordAsyncInternal(); + let { record, usedBackup } = await readSettingsRecordAsyncInternal(); + let expectedMtimeMs = await getUnifiedSettingsMtimeMs(); for (let attempt = 0; attempt < 3; attempt += 1) { const nextRecord = record ?? {}; nextRecord.dashboardDisplaySettings = { ...dashboardDisplaySettings }; @@ -614,7 +596,8 @@ export async function saveUnifiedDashboardSettings( if ((error as NodeJS.ErrnoException).code !== "ESTALE" || attempt >= 2) { throw error; } - ({ record, usedBackup, mtimeMs: expectedMtimeMs } = await readSettingsRecordAsyncInternal()); + ({ record, usedBackup } = await readSettingsRecordAsyncInternal()); + expectedMtimeMs = await getUnifiedSettingsMtimeMs(); } } }); From e6de13d3b58642df37e68c0b727373d55c046d0e Mon Sep 17 00:00:00 2001 From: Neil Daquioag Date: Sat, 18 Apr 2026 17:15:45 +0800 Subject: [PATCH 4/4] fix(settings): snapshot mtime before async unified-settings read (PR #429) Responds to the PR #429 review comment. The optimistic mtime check was taking expectedMtimeMs after the async read, leaving a window where an external write could update the file between the read and the mtime snapshot. Swap the order so the expected mtime is sampled first, then the record is read, and mirror that order on ESTALE retry. This preserves the existing settings contract and test behavior while closing the specific stale-overwrite hole called out in review. --- lib/unified-settings.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/unified-settings.ts b/lib/unified-settings.ts index 61b02063..a35fb261 100644 --- a/lib/unified-settings.ts +++ b/lib/unified-settings.ts @@ -522,8 +522,8 @@ export function saveUnifiedPluginConfigSync(pluginConfig: JsonRecord): void { */ export async function saveUnifiedPluginConfig(pluginConfig: JsonRecord): Promise { await enqueueSettingsWrite(async () => { - let { record, usedBackup } = await readSettingsRecordAsyncInternal(); let expectedMtimeMs = await getUnifiedSettingsMtimeMs(); + let { record, usedBackup } = await readSettingsRecordAsyncInternal(); for (let attempt = 0; attempt < 3; attempt += 1) { const nextRecord = record ?? {}; nextRecord.pluginConfig = { ...pluginConfig }; @@ -537,8 +537,8 @@ export async function saveUnifiedPluginConfig(pluginConfig: JsonRecord): Promise if ((error as NodeJS.ErrnoException).code !== "ESTALE" || attempt >= 2) { throw error; } - ({ record, usedBackup } = await readSettingsRecordAsyncInternal()); expectedMtimeMs = await getUnifiedSettingsMtimeMs(); + ({ record, usedBackup } = await readSettingsRecordAsyncInternal()); } } }); @@ -581,8 +581,8 @@ export async function saveUnifiedDashboardSettings( dashboardDisplaySettings: JsonRecord, ): Promise { await enqueueSettingsWrite(async () => { - let { record, usedBackup } = await readSettingsRecordAsyncInternal(); let expectedMtimeMs = await getUnifiedSettingsMtimeMs(); + let { record, usedBackup } = await readSettingsRecordAsyncInternal(); for (let attempt = 0; attempt < 3; attempt += 1) { const nextRecord = record ?? {}; nextRecord.dashboardDisplaySettings = { ...dashboardDisplaySettings }; @@ -596,8 +596,8 @@ export async function saveUnifiedDashboardSettings( if ((error as NodeJS.ErrnoException).code !== "ESTALE" || attempt >= 2) { throw error; } - ({ record, usedBackup } = await readSettingsRecordAsyncInternal()); expectedMtimeMs = await getUnifiedSettingsMtimeMs(); + ({ record, usedBackup } = await readSettingsRecordAsyncInternal()); } } });