From 952a77c420b1b20ebd8aaa7617483ab79e7bd1ff Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Fri, 25 Sep 2026 13:50:20 +0200 Subject: [PATCH] fix(config): keep config.yaml readable while it is being saved (#823) config.yaml was rewritten in place, so a command that read it mid-save saw an empty file ("Invalid project config"), and a failed write left it truncated. The three local config writers (saveLocalConfig, saveLocalConfigForScope, the legacy role migration) now go through writeFileAtomic: a sibling temp file renamed over the target, removed on failure. The partition config.yaml already used it. An existing config.yaml keeps its mode; a newly created one is 0600 (was the umask default, usually 0644), as the partition config already is. writeFileAtomic now writes a symlinked target at the end of its link chain (temp file next to that file), so a symlinked config.yaml keeps its link instead of becoming a regular file. A dangling link gets its missing target (and directory) created, as the in-place write did; a link loop is refused with an error and nothing is written. --- src/__tests__/config-atomic-save.test.ts | 176 +++++++++++++++++++++++ src/config.ts | 8 +- src/utils/fs.ts | 38 ++++- 3 files changed, 211 insertions(+), 11 deletions(-) create mode 100644 src/__tests__/config-atomic-save.test.ts diff --git a/src/__tests__/config-atomic-save.test.ts b/src/__tests__/config-atomic-save.test.ts new file mode 100644 index 000000000..fd332e660 --- /dev/null +++ b/src/__tests__/config-atomic-save.test.ts @@ -0,0 +1,176 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import fse from 'fs-extra'; +import { chmodSync, lstatSync, mkdtempSync, mkdirSync, readFileSync, readdirSync, readlinkSync, statSync, writeFileSync, rmSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { loadLocalConfig, saveLocalConfig, saveLocalConfigForScope } from '../config.js'; +import type { LocalConfig } from '../types.js'; +import { log } from '../utils/logger.js'; +import { trySymlink } from './helpers/symlink.js'; + +// Issue #823 item 14: config.yaml was written in place. Opening it for writing +// truncates it, so a command reading it mid-save saw an empty file, and a +// failed write (ENOSPC, crash) left it empty or half-written. +describe('config.yaml saves are atomic', () => { + const originalHome = process.env.HOME; + let home: string; + let configDir: string; + let configPath: string; + let original: string; + + beforeEach(() => { + home = mkdtempSync(path.join(os.tmpdir(), 'teamai-atomic-config-')); + process.env.HOME = home; + configDir = path.join(home, '.teamai'); + configPath = path.join(configDir, 'config.yaml'); + mkdirSync(configDir, { recursive: true }); + original = 'repo:\n localPath: /nonexistent/team-repo\n remote: https://github.com/acme/team.git\nusername: dev\n'; + writeFileSync(configPath, original, 'utf-8'); + }); + + afterEach(() => { + vi.restoreAllMocks(); + if (originalHome === undefined) delete process.env.HOME; + else process.env.HOME = originalHome; + rmSync(home, { recursive: true, force: true }); + }); + + /** + * Every write into the config dir first truncates its target, the state an + * in-place write exposes between open(O_TRUNC) and the data landing. Then + * `during` runs (a concurrent reader), and the write either completes or + * fails with ENOSPC. + */ + function interruptConfigWrites(during: () => Promise, outcome: 'complete' | 'fail'): void { + const realWriteFile = fse.writeFile; + vi.spyOn(fse, 'writeFile').mockImplementation(async (file: unknown, data: unknown) => { + if (typeof file !== 'string' || typeof data !== 'string') throw new Error('unexpected writeFile call in test'); + if (path.dirname(file) !== configDir) return realWriteFile(file, data, 'utf-8'); + await realWriteFile(file, '', 'utf-8'); + await during(); + if (outcome === 'fail') throw Object.assign(new Error('ENOSPC: no space left on device, write'), { code: 'ENOSPC' }); + return realWriteFile(file, data, 'utf-8'); + }); + } + + async function currentConfig(): Promise { + const config = await loadLocalConfig(); + if (!config) throw new Error('expected the fixture config to load'); + return config; + } + + const savers = [ + ['saveLocalConfig', (config: LocalConfig) => saveLocalConfig(config)], + ['saveLocalConfigForScope', (config: LocalConfig) => saveLocalConfigForScope(config)], + ] as const; + + describe.each(savers)('%s', (_name, save) => { + it('never lets a concurrent reader see an empty or partial config', async () => { + const config = await currentConfig(); + let seenMidSave: string | undefined; + interruptConfigWrites(async () => { + seenMidSave = (await loadLocalConfig())?.username; + }, 'complete'); + + await save({ ...config, username: 'renamed' }); + + expect(seenMidSave).toBe('dev'); + expect((await loadLocalConfig())?.username).toBe('renamed'); + }); + + it('leaves the previous config intact and no temp file when the write fails', async () => { + const config = await currentConfig(); + interruptConfigWrites(async () => {}, 'fail'); + + await expect(save({ ...config, username: 'renamed' })).rejects.toThrow(/ENOSPC/); + + expect(readFileSync(configPath, 'utf-8')).toBe(original); + expect(readdirSync(configDir)).toEqual(['config.yaml']); + }); + + it('keeps the mode of an existing config.yaml', async () => { + chmodSync(configPath, 0o644); + + await save({ ...(await currentConfig()), username: 'renamed' }); + + expect(statSync(configPath).mode & 0o777).toBe(0o644); + }); + + it('writes through a symlinked config.yaml and keeps the link', async () => { + const realPath = path.join(home, 'dotfiles', 'config.yaml'); + mkdirSync(path.dirname(realPath)); + writeFileSync(realPath, original, 'utf-8'); + rmSync(configPath); + if (!trySymlink(realPath, configPath)) return; + + await save({ ...(await currentConfig()), username: 'renamed' }); + + expect(lstatSync(configPath).isSymbolicLink()).toBe(true); + expect(readlinkSync(configPath)).toBe(realPath); + expect(readFileSync(realPath, 'utf-8')).toContain('username: renamed'); + expect(readdirSync(path.dirname(realPath))).toEqual(['config.yaml']); + }); + + it('creates the missing target of a dangling config.yaml link and keeps the link', async () => { + const config = await currentConfig(); + const linkTarget = path.join('..', 'dotfiles', 'config.yaml'); + const realPath = path.join(home, 'dotfiles', 'config.yaml'); + rmSync(configPath); + if (!trySymlink(linkTarget, configPath)) return; + + await save({ ...config, username: 'renamed' }); + + expect(lstatSync(configPath).isSymbolicLink()).toBe(true); + expect(readlinkSync(configPath)).toBe(linkTarget); + expect(readFileSync(realPath, 'utf-8')).toContain('username: renamed'); + expect(readdirSync(path.dirname(realPath))).toEqual(['config.yaml']); + expect(readdirSync(configDir)).toEqual(['config.yaml']); + }); + + it('refuses a config.yaml symlink loop and leaves the links alone', async () => { + const config = await currentConfig(); + const otherLink = path.join(configDir, 'other.yaml'); + rmSync(configPath); + if (!trySymlink(otherLink, configPath) || !trySymlink(configPath, otherLink)) return; + + await expect(save({ ...config, username: 'renamed' })).rejects.toThrow(/symbolic link loop/); + + expect(readlinkSync(configPath)).toBe(otherLink); + expect(readlinkSync(otherLink)).toBe(configPath); + expect(readdirSync(configDir).sort()).toEqual(['config.yaml', 'other.yaml']); + }); + }); + + it('resolves a dangling link against the real directory of a symlinked config dir', async () => { + const config = await currentConfig(); + const realConfigDir = path.join(home, 'dotfiles', 'teamai'); + mkdirSync(realConfigDir, { recursive: true }); + rmSync(configDir, { recursive: true }); + if (!trySymlink(realConfigDir, configDir)) return; + if (!trySymlink(path.join('..', 'shared', 'config.yaml'), configPath)) return; + + await saveLocalConfig({ ...config, username: 'renamed' }); + + expect(readFileSync(path.join(home, 'dotfiles', 'shared', 'config.yaml'), 'utf-8')).toContain('username: renamed'); + expect(lstatSync(configPath).isSymbolicLink()).toBe(true); + }); + + it('leaves the config intact and no temp file when the legacy role migration write fails', async () => { + const repoDir = path.join(home, 'team-repo'); + mkdirSync(path.join(repoDir, 'manifest'), { recursive: true }); + writeFileSync( + path.join(repoDir, 'manifest', 'roles.yaml'), + 'version: 1\nroles:\n - id: hai\n resources: { knowledge: [], skills: [hai] }\n', + 'utf-8', + ); + original = original.replace('/nonexistent/team-repo', repoDir); + writeFileSync(configPath, original, 'utf-8'); + vi.spyOn(log, 'error').mockImplementation(() => {}); + interruptConfigWrites(async () => {}, 'fail'); + + await loadLocalConfig(); + + expect(readFileSync(configPath, 'utf-8')).toBe(original); + expect(readdirSync(configDir)).toEqual(['config.yaml']); + }); +}); diff --git a/src/config.ts b/src/config.ts index 1470f7ed2..421f5d313 100644 --- a/src/config.ts +++ b/src/config.ts @@ -17,7 +17,7 @@ import { getStatePath, getDataHome, } from './types.js'; -import { readFileSafe, readJson, writeFile, writeJson, expandHome, pathExists } from './utils/fs.js'; +import { readFileSafe, readJson, writeFileAtomic, writeJson, expandHome, pathExists } from './utils/fs.js'; import { resolveAnchors } from './utils/git.js'; import { getUserHome } from './utils/home.js'; import { resolvePartitionDir, writeAnchorFile } from './utils/partition.js'; @@ -56,7 +56,7 @@ async function migrateLegacyRoleConfig(config: LocalConfig, configPath: string): resourceProfileVersion: manifest.version, }; - await writeFile(expandHome(configPath), YAML.stringify(migrated)); + await writeFileAtomic(expandHome(configPath), YAML.stringify(migrated)); log.info('Migrated legacy teamai config to default role profile: hai'); return migrated; } @@ -112,7 +112,7 @@ function serializeLocalConfig(config: LocalConfig): string { * Save the local config */ export async function saveLocalConfig(config: LocalConfig): Promise { - await writeFile(expandHome(getUserConfigPath()), serializeLocalConfig(config)); + await writeFileAtomic(expandHome(getUserConfigPath()), serializeLocalConfig(config)); } /** @@ -250,7 +250,7 @@ export async function saveLocalConfigForScope( ): Promise { const dataHome = getDataHome(config); const configPath = path.join(dataHome, 'config.yaml'); - await writeFile(expandHome(configPath), serializeLocalConfig(config)); + await writeFileAtomic(expandHome(configPath), serializeLocalConfig(config)); // If the config lives in a project partition, drop the `anchor` reverse-lookup // file next to it (issue #374 P3). Previously only migration wrote it, so // freshly-init'd partitions had no anchor and `status --all` could not resolve diff --git a/src/utils/fs.ts b/src/utils/fs.ts index 20580522a..ae094a7ca 100644 --- a/src/utils/fs.ts +++ b/src/utils/fs.ts @@ -60,6 +60,29 @@ export async function writeFile(filePath: string, content: string): Promise { + let target = filePath; + for (let hops = 0; hops < MAX_SYMLINK_HOPS; hops++) { + try { + if (!(await fse.lstat(target)).isSymbolicLink()) return target; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return target; + throw error; + } + target = path.resolve(await fse.realpath(path.dirname(target)), await fse.readlink(target)); + } + throw new Error( + `Cannot write ${filePath}: it is part of a symbolic link loop. Point the link at a regular file, then retry.`, + ); +} + /** * Write a text file atomically (same-dir temp file + rename), preserving the * target's existing permission bits (or defaulting to 0o600 for a new file). @@ -72,21 +95,22 @@ export async function writeFile(filePath: string, content: string): Promise { - const expanded = expandHome(filePath); + const expanded = await symlinkTarget(expandHome(filePath)); await fse.ensureDir(path.dirname(expanded)); let mode = options?.mode ?? 0o600; - if (options?.mode === undefined) { - try { - mode = (await fse.stat(expanded)).mode & 0o777; - } catch (error) { - if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; - } + try { + if (options?.mode === undefined) mode = (await fse.stat(expanded)).mode & 0o777; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; } const tmp = `${expanded}.${process.pid}.${crypto.randomBytes(6).toString('hex')}.tmp`; try {