fix(machines): survive a machines.json left unreadable by a crash

changelog.d/machines-state-crash-recovery.md

@@ -0,0 +1,6 @@
+### Fixed
+- A crash or power loss no longer leaves the app unable to start because its saved remote PCs
+  cannot be read. The damaged file is kept beside it under a new name, the app starts without
+  those PCs, and Settings > Remote PCs names the file so you can add them again. The saved PCs
+  and herdr's machine approvals are now written so that a crash cannot leave them unreadable,
+  and only when they changed instead of on every refresh.

server/api.contract.test.ts

@@ -31,7 +31,7 @@ import { testShellEnv } from "../scripts/test-shell.ts";
 import { PendingInputError, PendingInputs } from "./pending-input.ts";
 import { loginPathProbe, ShellAgentLocator, type LoginPathProbe } from "./shell-agent.ts";
 import { DeviceStore } from "./devices.ts";
-import type { MachineEvent } from "../shared/machines.ts";
+import type { MachineEvent, MachineSettings } from "../shared/machines.ts";
 import { MAX_MUTED_PANES, type SharedPreferences } from "../shared/preferences.ts";
 
 /**
@@ -1308,6 +1308,27 @@ describe("agent rename", () => {
     }
   });
 
+  // #2044: a crash left machines.json its old length in NULs, and the server would not start
+  it("starts on saved PCs that are not JSON and names the file it set aside in the PC settings, read only", async () => {
+    const state = mkdtempSync(join(tmpdir(), "ovis-machines-set-aside-"));
+    writeFileSync(join(state, "machines.json"), Buffer.alloc(353));
+    const error = spyOn(console, "error").mockImplementation(() => {});
+    let app: ReturnType<typeof createServer> | undefined;
+    try {
+      app = createServer({ port: 0, hostname: "127.0.0.1", token: "", stateDir: state, tailscaleOwner: null });
+      const url = `http://127.0.0.1:${app.port}/api/machines/settings`;
+      const settings = await (await fetch(url)).json() as MachineSettings;
+      expect(settings).toEqual({ auto_update_bridges: true, set_aside: [expect.stringMatching(/^machines\.json\.corrupt-\S+$/)] });
+      expect(readdirSync(state)).toContain(settings.set_aside![0]!);
+      const patched = await fetch(url, { method: "PATCH", headers: { "content-type": "application/json", "x-herdr-machine": "1" }, body: JSON.stringify({ auto_update_bridges: false, set_aside: [] }) });
+      expect(patched.status).toBe(200);
+      expect(await patched.json()).toEqual({ auto_update_bridges: false, set_aside: settings.set_aside });
+    } finally {
+      app?.stop(); error.mockRestore();
+      rmSync(state, { recursive: true, force: true });
+    }
+  });
+
   it("validates the body before asking herdr", async () => {
     for (const [body, code] of [
       [{}, "missing_pane_id"],

server/herdr-profile-state.test.ts

@@ -1,8 +1,9 @@
-import { afterEach, expect, it } from "bun:test";
-import { mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs";
+import { afterEach, expect, it, spyOn } from "bun:test";
+import { existsSync, mkdtempSync, openSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { HerdrProfileState } from "./herdr-profile-state.ts";
+import { privateStateFs } from "./private-state.ts";
 import type { SessionSnapshot } from "../shared/protocol.ts";
 
 const dirs: string[] = [];
@@ -80,3 +81,62 @@ it("retries a failed key removal when the old target returns, after a restart an
   readded.flush();
   expect(readFileSync(join(dir, "herdr-profile-state.json"), "utf8")).not.toContain("retiring");
 });
+it("sets aside approvals that are not JSON, bytes untouched, and starts without them", () => {
+  const dir = directory(), path = join(dir, "herdr-profile-state.json");
+  const zeros = Buffer.alloc(120);
+  writeFileSync(path, zeros);
+  const error = spyOn(console, "error").mockImplementation(() => {});
+  try {
+    const store = new HerdrProfileState(dir);
+    expect(store.setAside).toMatch(/herdr-profile-state\.json\.corrupt-[^:]+$/);
+    expect(readFileSync(store.setAside!)).toEqual(zeros);
+    expect(existsSync(path)).toBe(false);
+    expect(store.approved("id", { destination: "BuildBox" })).toBe(false);
+    store.approve("id", { destination: "BuildBox" });
+    store.flush();
+    expect(new HerdrProfileState(dir).approved("id", { destination: "BuildBox" })).toBe(true);
+  } finally { error.mockRestore(); }
+});
+it("writes unchanged approvals again when their file was deleted or replaced on disk", () => {
+  const dir = directory(), path = join(dir, "herdr-profile-state.json");
+  const store = new HerdrProfileState(dir);
+  store.approve("id", { destination: "BuildBox" });
+  store.flush();
+  const saved = readFileSync(path, "utf8");
+  rmSync(path);
+  store.approve("id", { destination: "BuildBox" });
+  store.flush();
+  expect(readFileSync(path, "utf8")).toBe(saved);
+  writeFileSync(path, "[]");
+  store.approve("id", { destination: "BuildBox" });
+  store.flush();
+  expect(readFileSync(path, "utf8")).toBe(saved);
+  // the same length, other bytes
+  writeFileSync(path, saved.replace('"id"', '"xx"'));
+  store.approve("id", { destination: "BuildBox" });
+  store.flush();
+  expect(readFileSync(path, "utf8")).toBe(saved);
+});
+it("writes durably, and only when the document changed", () => {
+  const dir = directory();
+  const temporaries: string[] = [];
+  const open = spyOn(privateStateFs, "openSync").mockImplementation(((path: string, flags: string, mode?: number) => {
+    if (flags === "wx") temporaries.push(path);
+    return openSync(path, flags, mode);
+  }) as typeof openSync);
+  try {
+    const store = new HerdrProfileState(dir);
+    store.approve("id", { destination: "BuildBox" });
+    store.saveSnapshot("id", { panes: [{ pane_id: "test-pane" }] } as SessionSnapshot);
+    store.flush();
+    expect(temporaries).toHaveLength(1);
+    // every refresh brings a new snapshot object: the same panes write nothing
+    store.saveSnapshot("id", { panes: [{ pane_id: "test-pane" }] } as SessionSnapshot);
+    store.flush();
+    expect(temporaries).toHaveLength(1);
+    store.saveSnapshot("id", { panes: [{ pane_id: "other-pane" }] } as SessionSnapshot);
+    store.flush();
+    expect(temporaries).toHaveLength(2);
+    expect(new HerdrProfileState(dir).snapshot("id")?.panes[0]?.pane_id).toBe("other-pane");
+  } finally { open.mockRestore(); }
+});

server/herdr-profile-state.ts

@@ -1,8 +1,9 @@
 import { createHash } from "node:crypto";
-import { chmodSync, existsSync, mkdirSync, readFileSync, renameSync, writeFileSync } from "node:fs";
 import { join } from "node:path";
 import type { SshTarget } from "../shared/machines.ts";
 import type { SessionSnapshot } from "../shared/protocol.ts";
+import { writePrivateState } from "./private-state.ts";
+import { holdsDocument, readStateDocument, setAsideDamaged } from "./state-document.ts";
 
 /** `retiring`: this binding's key removal was started and has not succeeded yet. */
 interface ProfileState { binding: string; approved: boolean; connectionApproved: boolean; snapshot: SessionSnapshot | null; retiring?: true }
@@ -21,10 +22,21 @@ export class HerdrProfileState {
   private entries = new Map<string, ProfileState>();
   private dirty = false;
   private path: string;
-  constructor(private directory: string) {
+  /** the document this store last wrote: a flush that would write the same bytes writes nothing
+   * while the file still holds them */
+  private written: string | undefined;
+  /** where a file that was not JSON was moved at startup (setAsideDamaged), its approvals with it */
+  readonly setAside: string | null = null;
+  constructor(directory: string) {
     this.path = join(directory, "herdr-profile-state.json");
-    if (!existsSync(this.path)) return;
-    const saved: unknown = JSON.parse(readFileSync(this.path, "utf8"));
+    const read = readStateDocument(this.path);
+    if (read.kind === "missing") return;
+    if (read.kind === "malformed") {
+      this.setAside = setAsideDamaged(this.path);
+      console.error(`${this.path} is not JSON; it was set aside as ${this.setAside}, and herdr's saved machines need approving again`);
+      return;
+    }
+    const saved = read.value;
     if (!Array.isArray(saved)) throw new Error("Invalid herdr-profile-state.json; approvals were preserved");
     for (const item of saved) {
       if (!item || typeof item.profile_id !== "string" || !item.profile_id || this.entries.has(item.profile_id)
@@ -82,11 +94,17 @@ export class HerdrProfileState {
     if (errors.length) throw new AggregateError(errors, "Some saved machine keys could not be removed; cleanup will retry");
     return removed;
   }
+  /**
+   * Writes the approvals durably (writePrivateState): a crash must not leave the file unreadable. A
+   * refresh hands over a new snapshot object each time, so an unchanged one still marks the state
+   * dirty; only a document that differs from the one on disk pays for the write and its fsync.
+   */
   flush(): void {
     if (!this.dirty) return;
-    mkdirSync(this.directory, { recursive: true, mode: 0o700 });
-    const tmp = this.path + ".tmp";
-    writeFileSync(tmp, JSON.stringify([...this.entries].map(([profile_id, state]) => ({ profile_id, ...state }))), { mode: 0o600 });
-    chmodSync(tmp, 0o600); renameSync(tmp, this.path); this.dirty = false;
+    const document = [...this.entries].map(([profile_id, state]) => ({ profile_id, ...state }));
+    const text = JSON.stringify(document);
+    // the file on disk may have been deleted or replaced since this store wrote it
+    if (text !== this.written || !holdsDocument(this.path, text)) { writePrivateState(this.path, document); this.written = text; }
+    this.dirty = false;
   }
 }

server/machines-refresh.test.ts

@@ -1,5 +1,5 @@
 import { afterEach, describe, expect, it, spyOn } from "bun:test";
-import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
+import { mkdirSync, mkdtempSync, openSync, readFileSync, rmSync, writeFileSync } from "node:fs";
 import { createServer, type Socket } from "node:net";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
@@ -8,6 +8,7 @@ import type { ServerMessage, SessionSnapshot } from "../shared/protocol.ts";
 import { CompletionTracker } from "./completion.ts";
 import { handleMachineRequest } from "./machine-api.ts";
 import { bridgeAgentNews, MachineManager, SOURCE_CHECK_INTERVAL_MS } from "./machines.ts";
+import { privateStateFs } from "./private-state.ts";
 import type { PushService } from "./push.ts";
 import { REMOTE_LIMITS, RemoteResponseTooLarge } from "./remote-json.ts";
 
@@ -1021,3 +1022,41 @@ describe("workspace order evidence through the manager", () => {
     expect(JSON.stringify(await (await ask("")).json())).not.toContain("fixture-token");
   });
 });
+
+describe("saving a remote PC's snapshot", () => {
+  it("rewrites machines.json durably, and only when a refresh changed what it holds", async () => {
+    const { manager, drive, remote, runtime } = await remoteFixture();
+    const internals = manager as unknown as { stateDir: string; saveTimer?: unknown };
+    // each durable write creates its temporary file exclusively ("wx") and fsyncs it
+    const temporaries: string[] = [];
+    const open = spyOn(privateStateFs, "openSync");
+    const fsync = spyOn(privateStateFs, "fsyncSync");
+    cleanups.push(() => { open.mockRestore(); fsync.mockRestore(); });
+    open.mockImplementation(((path: string, flags: string, mode?: number) => {
+      if (flags === "wx" && path.includes("machines.json")) temporaries.push(path);
+      return openSync(path, flags, mode);
+    }) as typeof openSync);
+    // a refresh saves after a short wait (saveSoon): a save is done once its timer is gone
+    const refreshed = async () => {
+      await drive.refresh(runtime);
+      for (let attempt = 0; attempt < 200 && internals.saveTimer !== undefined; attempt++) await Bun.sleep(25);
+      expect(internals.saveTimer).toBeUndefined();
+    };
+    const saved = () => JSON.parse(readFileSync(join(internals.stateDir, "machines.json"), "utf8")) as { snapshot: SessionSnapshot | null }[];
+
+    await refreshed();
+    expect(temporaries).toHaveLength(1);
+    expect(fsync).toHaveBeenCalled();
+    expect(saved()[0]!.snapshot?.panes[0]?.agent_status).toBe("working");
+
+    // the same panes again, in a new object as every read brings: nothing to write
+    await refreshed();
+    await refreshed();
+    expect(temporaries).toHaveLength(1);
+
+    remote.set(snapshot("done"));
+    await refreshed();
+    expect(temporaries).toHaveLength(2);
+    expect(saved()[0]!.snapshot?.panes[0]?.agent_status).toBe("done");
+  });
+});

server/machines.test.ts

@@ -1,5 +1,5 @@
 import { afterEach, describe, expect, it, spyOn } from "bun:test";
-import { chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
+import { chmodSync, existsSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join, win32 } from "node:path";
 import { describeProgress } from "../src/lib/bridgeProgress.ts";
@@ -1049,3 +1049,73 @@ describe("bridge verification failures", () => {
     expect((await unanswered({ host: "203.0.113.99" })).forwards).toEqual([[18472]]);
   });
 });
+
+// A crash can leave machines.json its old length in NULs (#2044): the app must still start.
+describe("saved PCs that cannot be read", () => {
+  const offline = async () => { throw new Error("local offline"); };
+  const dirs: string[] = [];
+  afterEach(() => { for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); });
+  const directory = () => { const dir = mkdtempSync(join(tmpdir(), "herdr-machines-state-")); dirs.push(dir); return dir; };
+
+  it("sets a file of NULs aside and starts without the PCs it held", () => {
+    const dir = directory();
+    const zeros = Buffer.alloc(353);
+    writeFileSync(join(dir, "machines.json"), zeros);
+    const error = spyOn(console, "error").mockImplementation(() => {});
+    let manager: MachineManager | undefined;
+    try {
+      manager = new MachineManager(dir, {} as PushService, new CompletionTracker(null), offline);
+      const aside = readdirSync(dir).filter((name) => name.startsWith("machines.json.corrupt-"));
+      expect(aside).toHaveLength(1);
+      expect(aside[0]).not.toContain(":");
+      expect(readFileSync(join(dir, aside[0]!))).toEqual(zeros);
+      expect(existsSync(join(dir, "machines.json"))).toBe(false);
+      expect(manager.list().map((machine) => machine.kind)).toEqual(["local"]);
+      expect(manager.settings()).toEqual({ auto_update_bridges: true, set_aside: aside });
+      expect(String(error.mock.calls[0]?.[0])).toContain(aside[0]!);
+      // the next save makes a readable file again
+      (manager as unknown as { persist(): void }).persist();
+      expect(JSON.parse(readFileSync(join(dir, "machines.json"), "utf8"))).toEqual([]);
+    } finally { manager?.stop(); error.mockRestore(); }
+  });
+
+  it("sets aside herdr's approvals that are not JSON, and says so with the PCs' settings", () => {
+    const dir = directory();
+    writeFileSync(join(dir, "herdr-profile-state.json"), Buffer.alloc(120));
+    const error = spyOn(console, "error").mockImplementation(() => {});
+    let manager: MachineManager | undefined;
+    try {
+      manager = new MachineManager(dir, {} as PushService, new CompletionTracker(null), offline);
+      expect(manager.settings().set_aside).toEqual([expect.stringMatching(/^herdr-profile-state\.json\.corrupt-/)]);
+    } finally { manager?.stop(); error.mockRestore(); }
+  });
+
+  it("writes an unchanged list again when its file was deleted or replaced on disk", () => {
+    const dir = directory(), path = join(dir, "machines.json");
+    writeFileSync(path, JSON.stringify([{ id: "11111111-1111-1111-1111-111111111111", name: "Saved PC", target: { destination: "BuildBox" }, enabled: false, snapshot: null }]));
+    const manager = new MachineManager(dir, {} as PushService, new CompletionTracker(null), offline);
+    try {
+      const persist = () => (manager as unknown as { persist(): void }).persist();
+      persist();
+      const saved = readFileSync(path, "utf8");
+      rmSync(path);
+      persist();
+      expect(readFileSync(path, "utf8")).toBe(saved);
+      writeFileSync(path, "[]");
+      persist();
+      expect(readFileSync(path, "utf8")).toBe(saved);
+      // the same length, other bytes
+      writeFileSync(path, saved.replace("Saved PC", "Other PC"));
+      persist();
+      expect(readFileSync(path, "utf8")).toBe(saved);
+    } finally { manager.stop(); }
+  });
+
+  it("still refuses to start on JSON that is not a list of PCs, leaving it in place", () => {
+    const dir = directory();
+    writeFileSync(join(dir, "machines.json"), "{}");
+    expect(() => new MachineManager(dir, {} as PushService, new CompletionTracker(null), offline)).toThrow("Invalid machines.json");
+    expect(readFileSync(join(dir, "machines.json"), "utf8")).toBe("{}");
+    expect(readdirSync(dir).filter((name) => name.includes("corrupt"))).toEqual([]);
+  });
+});