fix(config): atomic saveConfig + never overwrite a real config on transient/corrupt read [corner-case R2-1,R2-2]

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

src/data/config.test.ts

@@ -1,9 +1,32 @@
-import { describe, it, expect, afterEach } from "vitest";
+import { describe, it, expect, afterEach, beforeEach, vi } from "vitest";
 import { join } from "node:path";
-import { mkdtempSync, rmSync, writeFileSync, existsSync, readFileSync } from "node:fs";
+import {
+  mkdtempSync,
+  rmSync,
+  writeFileSync,
+  existsSync,
+  readFileSync,
+  readdirSync,
+  renameSync,
+} from "node:fs";
 import { tmpdir } from "node:os";
 import { getDefaultConfig, loadConfig, saveConfig, migrateLegacyConfig } from "./config.js";
 
+// Wrap the fs functions config.ts uses in call-through spies so the atomic-write
+// and transient-read-error paths can be observed/forced. Everything else (mkdtemp,
+// rmSync, existsSync, …) is the real implementation via `...actual`, so all other
+// tests keep their real filesystem behavior. `vi.spyOn` can't be used here because
+// the node:fs ESM namespace is non-configurable in this setup.
+vi.mock("node:fs", async (importOriginal) => {
+  const actual = await importOriginal<typeof import("node:fs")>();
+  return {
+    ...actual,
+    readFileSync: vi.fn(actual.readFileSync),
+    writeFileSync: vi.fn(actual.writeFileSync),
+    renameSync: vi.fn(actual.renameSync),
+  };
+});
+
 describe("config", () => {
   const dirs: string[] = [];
 
@@ -263,3 +286,129 @@ describe("spotify config", () => {
     });
   });
 });
+
+// --- R2-1: saveConfig must write atomically (temp file + rename), never truncate ---
+
+describe("saveConfig atomic write", () => {
+  const dirs: string[] = [];
+  function makeTmpDir(): string {
+    const dir = mkdtempSync(join(tmpdir(), "tsmb-atomic-"));
+    dirs.push(dir);
+    return dir;
+  }
+  beforeEach(() => {
+    vi.clearAllMocks(); // reset call history, keep the call-through implementations
+  });
+  afterEach(() => {
+    for (const d of dirs) rmSync(d, { recursive: true, force: true });
+    dirs.length = 0;
+  });
+
+  it("round-trips (save then load equals) and leaves NO .tmp file behind", () => {
+    const dir = makeTmpDir();
+    const path = join(dir, "config.json");
+    const config = { ...getDefaultConfig(), webPort: 4567, adminPassword: "pw" };
+
+    saveConfig(path, config);
+
+    expect(loadConfig(path)).toEqual(config);
+    // No temp remnants in the target directory.
+    expect(readdirSync(dir).filter((f) => f.includes(".tmp"))).toEqual([]);
+  });
+
+  it("writes via a same-dir temp file then renameSync onto the final path", () => {
+    const dir = makeTmpDir();
+    const path = join(dir, "config.json");
+
+    saveConfig(path, getDefaultConfig());
+
+    expect(vi.mocked(renameSync)).toHaveBeenCalled();
+    const [from, to] = vi.mocked(renameSync).mock.calls[0] as [string, string];
+    expect(to).toBe(path); // renamed ONTO the real path
+    expect(String(from)).not.toBe(path); // ...from a distinct temp file
+    expect(join(String(from), "..")).toBe(join(path, "..")); // ...in the SAME directory
+  });
+
+  it("does not corrupt a pre-existing valid config when saving over it", () => {
+    const dir = makeTmpDir();
+    const path = join(dir, "config.json");
+    saveConfig(path, { ...getDefaultConfig(), adminPassword: "first", webPort: 1234 });
+
+    // Overwrite with a different, fully-formed config.
+    saveConfig(path, { ...getDefaultConfig(), adminPassword: "second", webPort: 9999 });
+
+    const loaded = loadConfig(path);
+    expect(loaded.adminPassword).toBe("second");
+    expect(loaded.webPort).toBe(9999);
+    // The on-disk file is a single complete JSON document (no partial/truncated write).
+    expect(() => JSON.parse(readFileSync(path, "utf-8"))).not.toThrow();
+    expect(readdirSync(dir).filter((f) => f.includes(".tmp"))).toEqual([]);
+  });
+
+  it("cleans up the temp file (no .tmp remnant) when the rename fails", () => {
+    const dir = makeTmpDir();
+    const path = join(dir, "config.json");
+    vi.mocked(renameSync).mockImplementationOnce(() => {
+      throw new Error("rename boom");
+    });
+
+    expect(() => saveConfig(path, getDefaultConfig())).toThrow(/rename boom/);
+    // The failed write left no temp file lying around.
+    expect(readdirSync(dir).filter((f) => f.includes(".tmp"))).toEqual([]);
+  });
+});
+
+// --- R2-2: loadConfig must not treat a transient/corrupt read as "missing" ---
+
+describe("loadConfig error handling", () => {
+  const dirs: string[] = [];
+  function makeTmpDir(): string {
+    const dir = mkdtempSync(join(tmpdir(), "tsmb-load-"));
+    dirs.push(dir);
+    return dir;
+  }
+  beforeEach(() => {
+    vi.clearAllMocks();
+  });
+  afterEach(() => {
+    for (const d of dirs) rmSync(d, { recursive: true, force: true });
+    dirs.length = 0;
+  });
+
+  it("(a) ENOENT (missing file) returns defaults — unchanged first-run behavior", () => {
+    const dir = makeTmpDir();
+    expect(loadConfig(join(dir, "config.json"))).toEqual(getDefaultConfig());
+  });
+
+  it("(b) a non-ENOENT read error (EBUSY) rethrows instead of returning defaults", () => {
+    const dir = makeTmpDir();
+    const path = join(dir, "config.json");
+    // A REAL config exists on disk; a transient lock must NOT collapse to defaults
+    // (the caller would otherwise overwrite this real config with defaults).
+    saveConfig(path, { ...getDefaultConfig(), adminPassword: "keep-me" });
+    vi.mocked(readFileSync).mockImplementationOnce(() => {
+      const err = new Error("EBUSY: resource busy or locked") as NodeJS.ErrnoException;
+      err.code = "EBUSY";
+      throw err;
+    });
+
+    expect(() => loadConfig(path)).toThrow(/EBUSY/);
+    // The on-disk config is untouched and still readable once the lock clears.
+    expect(loadConfig(path).adminPassword).toBe("keep-me");
+  });
+
+  it("(c) corrupt JSON returns defaults AND backs up the original to *.corrupt-*", () => {
+    const dir = makeTmpDir();
+    const path = join(dir, "config.json");
+    const garbage = "{ not: valid json, ";
+    writeFileSync(path, garbage, "utf-8");
+
+    const loaded = loadConfig(path);
+
+    expect(loaded).toEqual(getDefaultConfig());
+    const backups = readdirSync(dir).filter((f) => f.includes(".corrupt-"));
+    expect(backups.length).toBeGreaterThan(0);
+    // The corrupt original is preserved verbatim (recoverable, never deleted).
+    expect(readFileSync(join(dir, backups[0]), "utf-8")).toBe(garbage);
+  });
+});

src/data/config.ts

@@ -1,4 +1,12 @@
-import { readFileSync, writeFileSync, mkdirSync, existsSync, copyFileSync, rmSync } from "node:fs";
+import {
+  readFileSync,
+  writeFileSync,
+  mkdirSync,
+  existsSync,
+  copyFileSync,
+  rmSync,
+  renameSync,
+} from "node:fs";
 import { dirname } from "node:path";
 import type { BotAccess, GuestPermissions } from "./permissions.js";
 import { GUEST_PERMISSION_FLAGS } from "./permissions.js";
@@ -92,10 +100,48 @@ export function getDefaultConfig(): BotConfig {
 
 export function loadConfig(path: string): BotConfig {
   const defaults = getDefaultConfig();
-  try {
-    const raw = readFileSync(path, "utf-8");
-    const partial = JSON.parse(raw) as Partial<BotConfig>;
 
+  // Distinguish the three failure modes so a *real* on-disk config is NEVER
+  // silently replaced with defaults (the caller saveConfig()s right after load,
+  // which would otherwise erase spotify creds / adminPassword / adminGroups /
+  // guestMode permanently):
+  //   (a) file ABSENT (ENOENT) — normal first run → defaults.
+  //   (b) any OTHER read error (EBUSY/EACCES/EPERM/EISDIR/…) on an existing file —
+  //       rethrow (fail-fast at boot). A loud crash beats silent credential loss.
+  //   (c) file readable but JSON.parse fails (corrupt) — back the file up first
+  //       (never delete it), THEN return defaults so boot can proceed.
+  let raw: string;
+  try {
+    raw = readFileSync(path, "utf-8");
+  } catch (err) {
+    if ((err as NodeJS.ErrnoException).code === "ENOENT") {
+      return defaults; // (a) missing file — first run
+    }
+    throw err; // (b) transient/permission error on an existing file — do not clobber it
+  }
+
+  let partial: Partial<BotConfig>;
+  try {
+    partial = JSON.parse(raw) as Partial<BotConfig>;
+  } catch {
+    // (c) Corrupt content: move the unreadable file aside to a timestamped backup
+    // so the data stays recoverable, then fall back to defaults. Prefer an atomic
+    // same-dir rename; if that fails, copy instead. If it can't be preserved at
+    // all, rethrow rather than let the caller overwrite unrecoverable data.
+    const backup = `${path}.corrupt-${Date.now()}`;
+    try {
+      renameSync(path, backup);
+    } catch {
+      try {
+        copyFileSync(path, backup);
+      } catch (backupErr) {
+        throw backupErr;
+      }
+    }
+    return defaults;
+  }
+
+  {
     // Normalize/sanitize guestMode on load. The WRITE path (POST /api/bot/settings)
     // sanitizes too, but a hand-edited/legacy/corrupt config.json reaches the gate
     // directly — so coerce it here as well, mirroring that write-path logic.
@@ -163,14 +209,33 @@ export function loadConfig(path: string): BotConfig {
       guestMode: gm,
       spotify,
     };
-  } catch {
-    return defaults;
   }
 }
 
 export function saveConfig(path: string, config: BotConfig): void {
   mkdirSync(dirname(path), { recursive: true });
-  writeFileSync(path, JSON.stringify(config, null, 2), "utf-8");
+  const json = JSON.stringify(config, null, 2);
+
+  // Atomic write: serialize to a sibling temp file in the SAME directory, then
+  // rename it onto the final path. rename is an atomic replace on POSIX and modern
+  // Windows, so a crash / power loss / ENOSPC mid-write can never leave config.json
+  // truncated — a reader always sees either the previous file or the fully-written
+  // new one, never a partial. The temp lives in the same dir so the rename stays on
+  // one filesystem (a cross-device rename would fail); pid + timestamp keep
+  // concurrent writers from colliding on the temp name.
+  const tmp = `${path}.${process.pid}.${Date.now()}.tmp`;
+  try {
+    writeFileSync(tmp, json, "utf-8");
+    renameSync(tmp, path);
+  } catch (err) {
+    // Never leave a partial temp file behind on failure.
+    try {
+      rmSync(tmp, { force: true });
+    } catch {
+      /* best-effort cleanup */
+    }
+    throw err;
+  }
 }
 
 /**