fix(config): keep config.yaml readable while it is being saved (#823)

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<void>, 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<LocalConfig> {
+    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']);
+  });
+});

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<void> {
-  await writeFile(expandHome(getUserConfigPath()), serializeLocalConfig(config));
+  await writeFileAtomic(expandHome(getUserConfigPath()), serializeLocalConfig(config));
 }
 
 /**
@@ -250,7 +250,7 @@ export async function saveLocalConfigForScope(
 ): Promise<void> {
   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

src/utils/fs.ts

@@ -60,6 +60,29 @@ export async function writeFile(filePath: string, content: string): Promise<void
   await fse.writeFile(expanded, content, 'utf-8');
 }
 
+const MAX_SYMLINK_HOPS = 40;
+
+/**
+ * The path a link chain starting at `filePath` ends at, whether or not a file
+ * exists there yet; `filePath` itself when it is not a link. Each hop resolves
+ * against the real directory of the link, as the kernel does.
+ */
+async function symlinkTarget(filePath: string): Promise<string> {
+  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<void
  * such as a partition's config.yaml; `writeFile` (a plain overwrite) is fine
  * for regenerable files. Pass options.mode to force restrictive permissions
  * for a file that newly contains credentials.
+ *
+ * A symlinked target is written at the file its link chain ends at, so the
+ * links stay; for a dangling link that file (and its directory) is created.
  */
 export async function writeFileAtomic(
   filePath: string,
   content: string,
   options?: { mode?: number },
 ): Promise<void> {
-  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 {