From fcbe2c0414d8a999544dfa60f798bb83e82ba53f Mon Sep 17 00:00:00 2001 From: Bohdan Triapitsyn Date: Sat, 22 Aug 2026 11:55:38 +0300 Subject: [PATCH] fix(settings): clean up orphaned temp files and harden atomic writes --- packages/electron/main.mjs | 13 ++-- packages/electron/ssh-manager.mjs | 11 ++- .../vscode/src/bridge-settings-runtime.ts | 7 +- .../web/server/lib/agent-memory/runtime.js | 9 ++- .../server/lib/opencode/settings-runtime.js | 42 +++++++++--- .../lib/opencode/settings-runtime.test.js | 67 +++++++++++++++++++ .../web/server/lib/project-context/runtime.js | 9 ++- .../web/server/lib/projects/project-config.js | 9 ++- 8 files changed, 142 insertions(+), 25 deletions(-) diff --git a/packages/electron/main.mjs b/packages/electron/main.mjs index 46805505..c18d9ffa 100644 --- a/packages/electron/main.mjs +++ b/packages/electron/main.mjs @@ -560,10 +560,15 @@ const writeJsonFile = async (filePath, data) => { // Atomic: write to a temp file then rename. Readers never see a partial // JSON file that could parse-error and get coerced to {}. const tmp = `${filePath}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; - await fsp.writeFile(tmp, JSON.stringify(data, null, 2), { encoding: 'utf8', mode: 0o600 }); - if (process.platform !== 'win32') await fsp.chmod(tmp, 0o600); - await fsp.rename(tmp, filePath); - if (process.platform !== 'win32') await fsp.chmod(filePath, 0o600); + try { + await fsp.writeFile(tmp, JSON.stringify(data, null, 2), { encoding: 'utf8', mode: 0o600 }); + if (process.platform !== 'win32') await fsp.chmod(tmp, 0o600); + await fsp.rename(tmp, filePath); + if (process.platform !== 'win32') await fsp.chmod(filePath, 0o600); + } catch (error) { + await fsp.rm(tmp, { force: true }).catch(() => {}); + throw error; + } }; const readSettingsRoot = () => { diff --git a/packages/electron/ssh-manager.mjs b/packages/electron/ssh-manager.mjs index 5c0b52ba..5f390a19 100644 --- a/packages/electron/ssh-manager.mjs +++ b/packages/electron/ssh-manager.mjs @@ -77,10 +77,15 @@ const writeJsonRoot = async (settingsFilePath, root) => { await fsp.mkdir(path.dirname(settingsFilePath), { recursive: true }); // Atomic write: concurrent readers (main.mjs, web server) would otherwise // see partial JSON and readJsonRoot()'s catch would silently coerce to {}, - // causing the next read-modify-write to wipe the entire settings file. + // causing the next read-modify-write wipe the entire settings file. const tmp = `${settingsFilePath}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; - await fsp.writeFile(tmp, JSON.stringify(root, null, 2)); - await fsp.rename(tmp, settingsFilePath); + try { + await fsp.writeFile(tmp, JSON.stringify(root, null, 2)); + await fsp.rename(tmp, settingsFilePath); + } catch (error) { + await fsp.rm(tmp, { force: true }).catch(() => {}); + throw error; + } }; const defaultTrue = () => true; diff --git a/packages/vscode/src/bridge-settings-runtime.ts b/packages/vscode/src/bridge-settings-runtime.ts index 78ba0751..ee48b7e8 100644 --- a/packages/vscode/src/bridge-settings-runtime.ts +++ b/packages/vscode/src/bridge-settings-runtime.ts @@ -173,17 +173,20 @@ const readSharedSettingsFromDisk = (): Record => { }; const writeSharedSettingsToDisk = async (changes: Record): Promise => { + let tmp: string | null = null; try { await fs.promises.mkdir(path.dirname(OPENCHAMBER_SHARED_SETTINGS_PATH), { recursive: true }); const current = readSharedSettingsFromDisk(); const next: Record = { ...current, ...changes }; // Atomic write: tmp file + rename. Readers never see a partial/truncated // JSON that would fail to parse and silently get coerced to {}. - const tmp = `${OPENCHAMBER_SHARED_SETTINGS_PATH}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; + tmp = `${OPENCHAMBER_SHARED_SETTINGS_PATH}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; await fs.promises.writeFile(tmp, JSON.stringify(next, null, 2), 'utf8'); await fs.promises.rename(tmp, OPENCHAMBER_SHARED_SETTINGS_PATH); } catch { - // ignore + if (tmp) { + await fs.promises.rm(tmp, { force: true }).catch(() => {}); + } } }; diff --git a/packages/web/server/lib/agent-memory/runtime.js b/packages/web/server/lib/agent-memory/runtime.js index a53edb63..0969b625 100644 --- a/packages/web/server/lib/agent-memory/runtime.js +++ b/packages/web/server/lib/agent-memory/runtime.js @@ -236,8 +236,13 @@ export const createAgentMemoryRuntime = (deps) => { const writeJsonAtomic = async (filePath, value) => { const temporaryPath = `${filePath}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}`; await fsPromises.mkdir(path.dirname(filePath), { recursive: true }); - await fsPromises.writeFile(temporaryPath, JSON.stringify(value, null, 2), 'utf8'); - await fsPromises.rename(temporaryPath, filePath); + try { + await fsPromises.writeFile(temporaryPath, JSON.stringify(value, null, 2), 'utf8'); + await fsPromises.rename(temporaryPath, filePath); + } catch (error) { + await fsPromises.rm(temporaryPath, { force: true }).catch(() => {}); + throw error; + } }; const withWriteLock = async (key, mutate) => { diff --git a/packages/web/server/lib/opencode/settings-runtime.js b/packages/web/server/lib/opencode/settings-runtime.js index 382d2d4b..bed2d240 100644 --- a/packages/web/server/lib/opencode/settings-runtime.js +++ b/packages/web/server/lib/opencode/settings-runtime.js @@ -547,25 +547,41 @@ export const createSettingsRuntime = (deps) => { // briefly opens the target file. Preserve atomic rename everywhere it works, // but fall back to a direct replacement so settings persistence does not // get permanently wedged on Windows desktop installs. - await fsPromises.copyFile(tmp, target); - await fsPromises.rm(tmp, { force: true }); + try { + await fsPromises.copyFile(tmp, target); + } finally { + await fsPromises.rm(tmp, { force: true }).catch(() => {}); + } + }; + + const cleanupOrphanedSettingsTempFiles = async (directory) => { + try { + const entries = await fsPromises.readdir(directory, { withFileTypes: true }); + const cleanupTasks = entries + .filter((entry) => entry.isFile() && entry.name.startsWith('settings.json.tmp-')) + .map((entry) => fsPromises.rm(path.join(directory, entry.name), { force: true }).catch(() => {})); + await Promise.all(cleanupTasks); + } catch { + // Best-effort cleanup: errors reading directory must not fail settings operations + } }; const writeSettingsToDisk = async (settings) => { + const settingsDirectory = path.dirname(SETTINGS_FILE_PATH); + await fsPromises.mkdir(settingsDirectory, { recursive: true, mode: 0o700 }); + if (process.platform !== 'win32') await fsPromises.chmod(settingsDirectory, 0o700); + // Atomic write: Electron main and ssh-manager read this file via plain + // readFile + JSON.parse and silently coerce parse errors to {}. A + // partial read during a non-atomic writeFile would make their next + // read-modify-write wipe the settings file. + const tmp = `${SETTINGS_FILE_PATH}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; try { - const settingsDirectory = path.dirname(SETTINGS_FILE_PATH); - await fsPromises.mkdir(settingsDirectory, { recursive: true, mode: 0o700 }); - if (process.platform !== 'win32') await fsPromises.chmod(settingsDirectory, 0o700); - // Atomic write: Electron main and ssh-manager read this file via plain - // readFile + JSON.parse and silently coerce parse errors to {}. A - // partial read during a non-atomic writeFile would make their next - // read-modify-write wipe the settings file. - const tmp = `${SETTINGS_FILE_PATH}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; await fsPromises.writeFile(tmp, JSON.stringify(settings, null, 2), { encoding: 'utf8', mode: 0o600 }); if (process.platform !== 'win32') await fsPromises.chmod(tmp, 0o600); await replaceFile(tmp, SETTINGS_FILE_PATH); if (process.platform !== 'win32') await fsPromises.chmod(SETTINGS_FILE_PATH, 0o600); } catch (error) { + await fsPromises.rm(tmp, { force: true }).catch(() => {}); console.warn('Failed to write settings file:', error); throw error; } @@ -854,7 +870,13 @@ export const createSettingsRuntime = (deps) => { return { settings: next, changed: true }; }; + let hasCleanedOrphanedTempFiles = false; + const readSettingsFromDiskMigrated = async () => { + if (!hasCleanedOrphanedTempFiles) { + hasCleanedOrphanedTempFiles = true; + await cleanupOrphanedSettingsTempFiles(path.dirname(SETTINGS_FILE_PATH)); + } const current = await readSettingsFromDisk(); const migration1 = await migrateSettingsFromLegacyLastDirectory(current); const migration2 = await migrateSettingsFromLegacyThemePreferences(migration1.settings); diff --git a/packages/web/server/lib/opencode/settings-runtime.test.js b/packages/web/server/lib/opencode/settings-runtime.test.js index 7c774d8e..46a84827 100644 --- a/packages/web/server/lib/opencode/settings-runtime.test.js +++ b/packages/web/server/lib/opencode/settings-runtime.test.js @@ -151,4 +151,71 @@ describe('settings runtime', () => { await fsPromises.rm(tempRoot, { recursive: true, force: true }); } }); + + it('cleans up orphaned settings.json.tmp files during startup migration', async () => { + const { runtime, settingsFilePath, tempRoot, cleanup } = await createRuntime(); + try { + const settingsDir = path.dirname(settingsFilePath); + const orphan1 = path.join(settingsDir, 'settings.json.tmp-1234-11111-abc'); + const orphan2 = path.join(settingsDir, 'settings.json.tmp-5678-22222-def'); + const unrelated = path.join(settingsDir, 'other-file.json'); + + await fsPromises.writeFile(orphan1, '{"broken": true}', 'utf8'); + await fsPromises.writeFile(orphan2, '{"broken": true}', 'utf8'); + await fsPromises.writeFile(unrelated, '{"keep": true}', 'utf8'); + await fsPromises.writeFile(settingsFilePath, '{"theme": "light"}', 'utf8'); + + await runtime.readSettingsFromDiskMigrated(); + + const files = await fsPromises.readdir(settingsDir); + expect(files).toContain('settings.json'); + expect(files).toContain('other-file.json'); + expect(files).not.toContain('settings.json.tmp-1234-11111-abc'); + expect(files).not.toContain('settings.json.tmp-5678-22222-def'); + } finally { + await cleanup(); + } + }); + + it('removes temp file when writeSettingsToDisk encounters a write error', async () => { + const tempRoot = await fsPromises.mkdtemp(path.join(os.tmpdir(), 'oc-settings-runtime-')); + const settingsFilePath = path.join(tempRoot, 'settings.json'); + let capturedTmp = null; + const wrappedFs = { + ...fsPromises, + rename: async (src, dst) => { + capturedTmp = src; + const error = new Error('unexpected disk failure'); + error.code = 'EIO'; + throw error; + }, + }; + const runtime = createSettingsRuntime({ + fsPromises: wrappedFs, + path, + crypto, + SETTINGS_FILE_PATH: settingsFilePath, + sanitizeProjects: (projects) => Array.isArray(projects) ? projects : [], + sanitizeSettingsUpdate: (settings) => settings, + mergePersistedSettings: (_current, changes) => changes, + normalizeSettingsPaths: (settings) => ({ settings, changed: false }), + normalizeStringArray: (values) => Array.isArray(values) ? values.filter((value) => typeof value === 'string') : [], + formatSettingsResponse: (settings) => settings, + resolveDirectoryCandidate: (value) => value, + normalizeManagedRemoteTunnelHostname: (value) => value, + normalizeManagedRemoteTunnelPresets: (value) => value, + normalizeManagedRemoteTunnelPresetTokens: (value) => value, + syncManagedRemoteTunnelConfigWithPresets: async () => {}, + upsertManagedRemoteTunnelToken: async () => {}, + }); + + try { + await expect(runtime.writeSettingsToDisk({ theme: 'dark' })).rejects.toThrow('unexpected disk failure'); + expect(capturedTmp).toBeTruthy(); + const files = await fsPromises.readdir(tempRoot); + expect(files.some((f) => f.startsWith('settings.json.tmp-'))).toBe(false); + } finally { + await fsPromises.rm(tempRoot, { recursive: true, force: true }); + } + }); }); diff --git a/packages/web/server/lib/project-context/runtime.js b/packages/web/server/lib/project-context/runtime.js index a7044791..7c22ca3c 100644 --- a/packages/web/server/lib/project-context/runtime.js +++ b/packages/web/server/lib/project-context/runtime.js @@ -231,8 +231,13 @@ export const createProjectContextRuntime = (deps) => { const writeJsonAtomic = async (filePath, value) => { const temporaryPath = `${filePath}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}`; await fsPromises.mkdir(path.dirname(filePath), { recursive: true }); - await fsPromises.writeFile(temporaryPath, JSON.stringify(value, null, 2), 'utf8'); - await fsPromises.rename(temporaryPath, filePath); + try { + await fsPromises.writeFile(temporaryPath, JSON.stringify(value, null, 2), 'utf8'); + await fsPromises.rename(temporaryPath, filePath); + } catch (error) { + await fsPromises.rm(temporaryPath, { force: true }).catch(() => {}); + throw error; + } }; const withWriteLock = async (projectId, mutate) => { diff --git a/packages/web/server/lib/projects/project-config.js b/packages/web/server/lib/projects/project-config.js index 03962aa4..1b946b34 100644 --- a/packages/web/server/lib/projects/project-config.js +++ b/packages/web/server/lib/projects/project-config.js @@ -536,8 +536,13 @@ export const createProjectConfigRuntime = (deps) => { }; await fsPromises.mkdir(parentDirectory, { recursive: true }); - await fsPromises.writeFile(temporaryPath, JSON.stringify(merged, null, 2), 'utf8'); - await fsPromises.rename(temporaryPath, filePath); + try { + await fsPromises.writeFile(temporaryPath, JSON.stringify(merged, null, 2), 'utf8'); + await fsPromises.rename(temporaryPath, filePath); + } catch (error) { + await fsPromises.rm(temporaryPath, { force: true }).catch(() => {}); + throw error; + } }; const withProjectWriteLock = async (projectID, mutate) => {