fix(settings): clean up orphaned temp files and harden atomic writes
This commit is contained in:
@@ -560,10 +560,15 @@ const writeJsonFile = async (filePath, data) => {
|
|||||||
// Atomic: write to a temp file then rename. Readers never see a partial
|
// Atomic: write to a temp file then rename. Readers never see a partial
|
||||||
// JSON file that could parse-error and get coerced to {}.
|
// 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)}`;
|
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 });
|
try {
|
||||||
if (process.platform !== 'win32') await fsp.chmod(tmp, 0o600);
|
await fsp.writeFile(tmp, JSON.stringify(data, null, 2), { encoding: 'utf8', mode: 0o600 });
|
||||||
await fsp.rename(tmp, filePath);
|
if (process.platform !== 'win32') await fsp.chmod(tmp, 0o600);
|
||||||
if (process.platform !== 'win32') await fsp.chmod(filePath, 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 = () => {
|
const readSettingsRoot = () => {
|
||||||
|
|||||||
@@ -77,10 +77,15 @@ const writeJsonRoot = async (settingsFilePath, root) => {
|
|||||||
await fsp.mkdir(path.dirname(settingsFilePath), { recursive: true });
|
await fsp.mkdir(path.dirname(settingsFilePath), { recursive: true });
|
||||||
// Atomic write: concurrent readers (main.mjs, web server) would otherwise
|
// Atomic write: concurrent readers (main.mjs, web server) would otherwise
|
||||||
// see partial JSON and readJsonRoot()'s catch would silently coerce to {},
|
// 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)}`;
|
const tmp = `${settingsFilePath}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`;
|
||||||
await fsp.writeFile(tmp, JSON.stringify(root, null, 2));
|
try {
|
||||||
await fsp.rename(tmp, settingsFilePath);
|
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;
|
const defaultTrue = () => true;
|
||||||
|
|||||||
@@ -173,17 +173,20 @@ const readSharedSettingsFromDisk = (): Record<string, unknown> => {
|
|||||||
};
|
};
|
||||||
|
|
||||||
const writeSharedSettingsToDisk = async (changes: Record<string, unknown>): Promise<void> => {
|
const writeSharedSettingsToDisk = async (changes: Record<string, unknown>): Promise<void> => {
|
||||||
|
let tmp: string | null = null;
|
||||||
try {
|
try {
|
||||||
await fs.promises.mkdir(path.dirname(OPENCHAMBER_SHARED_SETTINGS_PATH), { recursive: true });
|
await fs.promises.mkdir(path.dirname(OPENCHAMBER_SHARED_SETTINGS_PATH), { recursive: true });
|
||||||
const current = readSharedSettingsFromDisk();
|
const current = readSharedSettingsFromDisk();
|
||||||
const next: Record<string, unknown> = { ...current, ...changes };
|
const next: Record<string, unknown> = { ...current, ...changes };
|
||||||
// Atomic write: tmp file + rename. Readers never see a partial/truncated
|
// Atomic write: tmp file + rename. Readers never see a partial/truncated
|
||||||
// JSON that would fail to parse and silently get coerced to {}.
|
// 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.writeFile(tmp, JSON.stringify(next, null, 2), 'utf8');
|
||||||
await fs.promises.rename(tmp, OPENCHAMBER_SHARED_SETTINGS_PATH);
|
await fs.promises.rename(tmp, OPENCHAMBER_SHARED_SETTINGS_PATH);
|
||||||
} catch {
|
} catch {
|
||||||
// ignore
|
if (tmp) {
|
||||||
|
await fs.promises.rm(tmp, { force: true }).catch(() => {});
|
||||||
|
}
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|||||||
@@ -236,8 +236,13 @@ export const createAgentMemoryRuntime = (deps) => {
|
|||||||
const writeJsonAtomic = async (filePath, value) => {
|
const writeJsonAtomic = async (filePath, value) => {
|
||||||
const temporaryPath = `${filePath}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}`;
|
const temporaryPath = `${filePath}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}`;
|
||||||
await fsPromises.mkdir(path.dirname(filePath), { recursive: true });
|
await fsPromises.mkdir(path.dirname(filePath), { recursive: true });
|
||||||
await fsPromises.writeFile(temporaryPath, JSON.stringify(value, null, 2), 'utf8');
|
try {
|
||||||
await fsPromises.rename(temporaryPath, filePath);
|
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) => {
|
const withWriteLock = async (key, mutate) => {
|
||||||
|
|||||||
@@ -547,25 +547,41 @@ export const createSettingsRuntime = (deps) => {
|
|||||||
// briefly opens the target file. Preserve atomic rename everywhere it works,
|
// briefly opens the target file. Preserve atomic rename everywhere it works,
|
||||||
// but fall back to a direct replacement so settings persistence does not
|
// but fall back to a direct replacement so settings persistence does not
|
||||||
// get permanently wedged on Windows desktop installs.
|
// get permanently wedged on Windows desktop installs.
|
||||||
await fsPromises.copyFile(tmp, target);
|
try {
|
||||||
await fsPromises.rm(tmp, { force: true });
|
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 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 {
|
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 });
|
await fsPromises.writeFile(tmp, JSON.stringify(settings, null, 2), { encoding: 'utf8', mode: 0o600 });
|
||||||
if (process.platform !== 'win32') await fsPromises.chmod(tmp, 0o600);
|
if (process.platform !== 'win32') await fsPromises.chmod(tmp, 0o600);
|
||||||
await replaceFile(tmp, SETTINGS_FILE_PATH);
|
await replaceFile(tmp, SETTINGS_FILE_PATH);
|
||||||
if (process.platform !== 'win32') await fsPromises.chmod(SETTINGS_FILE_PATH, 0o600);
|
if (process.platform !== 'win32') await fsPromises.chmod(SETTINGS_FILE_PATH, 0o600);
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
|
await fsPromises.rm(tmp, { force: true }).catch(() => {});
|
||||||
console.warn('Failed to write settings file:', error);
|
console.warn('Failed to write settings file:', error);
|
||||||
throw error;
|
throw error;
|
||||||
}
|
}
|
||||||
@@ -854,7 +870,13 @@ export const createSettingsRuntime = (deps) => {
|
|||||||
return { settings: next, changed: true };
|
return { settings: next, changed: true };
|
||||||
};
|
};
|
||||||
|
|
||||||
|
let hasCleanedOrphanedTempFiles = false;
|
||||||
|
|
||||||
const readSettingsFromDiskMigrated = async () => {
|
const readSettingsFromDiskMigrated = async () => {
|
||||||
|
if (!hasCleanedOrphanedTempFiles) {
|
||||||
|
hasCleanedOrphanedTempFiles = true;
|
||||||
|
await cleanupOrphanedSettingsTempFiles(path.dirname(SETTINGS_FILE_PATH));
|
||||||
|
}
|
||||||
const current = await readSettingsFromDisk();
|
const current = await readSettingsFromDisk();
|
||||||
const migration1 = await migrateSettingsFromLegacyLastDirectory(current);
|
const migration1 = await migrateSettingsFromLegacyLastDirectory(current);
|
||||||
const migration2 = await migrateSettingsFromLegacyThemePreferences(migration1.settings);
|
const migration2 = await migrateSettingsFromLegacyThemePreferences(migration1.settings);
|
||||||
|
|||||||
@@ -151,4 +151,71 @@ describe('settings runtime', () => {
|
|||||||
await fsPromises.rm(tempRoot, { recursive: true, force: true });
|
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 });
|
||||||
|
}
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -231,8 +231,13 @@ export const createProjectContextRuntime = (deps) => {
|
|||||||
const writeJsonAtomic = async (filePath, value) => {
|
const writeJsonAtomic = async (filePath, value) => {
|
||||||
const temporaryPath = `${filePath}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}`;
|
const temporaryPath = `${filePath}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}`;
|
||||||
await fsPromises.mkdir(path.dirname(filePath), { recursive: true });
|
await fsPromises.mkdir(path.dirname(filePath), { recursive: true });
|
||||||
await fsPromises.writeFile(temporaryPath, JSON.stringify(value, null, 2), 'utf8');
|
try {
|
||||||
await fsPromises.rename(temporaryPath, filePath);
|
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) => {
|
const withWriteLock = async (projectId, mutate) => {
|
||||||
|
|||||||
@@ -536,8 +536,13 @@ export const createProjectConfigRuntime = (deps) => {
|
|||||||
};
|
};
|
||||||
|
|
||||||
await fsPromises.mkdir(parentDirectory, { recursive: true });
|
await fsPromises.mkdir(parentDirectory, { recursive: true });
|
||||||
await fsPromises.writeFile(temporaryPath, JSON.stringify(merged, null, 2), 'utf8');
|
try {
|
||||||
await fsPromises.rename(temporaryPath, filePath);
|
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) => {
|
const withProjectWriteLock = async (projectID, mutate) => {
|
||||||
|
|||||||
Reference in New Issue
Block a user