test(cli): deterministic torn-write regression coverage; document module

Address the openchamber-ai review's non-blocking notes:

- Concurrency evidence: the torn-write test now injects a slow, chunked
  writeFile (one open handle, file grows prefix->full) so a torn read is
  deterministically observable in the 30ms window. A companion test runs the
  naive direct writer under the same load and asserts torn reads ARE produced,
  proving the atomicity test can actually fail on the pre-fix writer.
- Windows fallback comment: no longer claims the copyFile fallback is atomic;
  it is called out as a last resort confined to Windows.
- Module map: document cli-settings-accessors.js in bin/lib/DOCUMENTATION.md.

packages/web/bin/lib/DOCUMENTATION.md

@@ -78,6 +78,18 @@ These modules hold reusable, non-presentational logic for commands.
 - `cli-paths.js`
   - Data, run, log, settings, tunnel profile, and managed-local config paths.
 
+- `cli-settings-accessors.js`
+  - Minimal settings.json read/write for CLI contexts that must not load the
+    full web settings runtime (`connect-url` relay identity resolution).
+  - Mirrors the settings runtime's guarantees so a CLI read-modify-write can
+    never corrupt shared state: atomic tmp+rename writes (no concurrent reader
+    in the running app can observe a torn file), a strict read that throws on
+    corrupt/unreadable payloads, and the same `0600` file mode.
+  - The strict read gates relay identity regeneration exactly like the server
+    runtime: a swallowed read failure can never mint a replacement signing or
+    encryption keypair, which would change `serverId` and orphan every paired
+    device and push binding.
+
 - `cli-process.js`
   - PID files, instance registry files, process identity checks, runtime metadata checks, and process termination helpers.
 

packages/web/bin/lib/cli-settings-accessors.js

@@ -78,8 +78,10 @@ export const createSettingsAccessors = ({ fsPromises, path, dataDir, settingsFil
     }
 
     // Windows can transiently reject the atomic replace while another process
-    // briefly holds the target open. Copy the COMPLETE tmp file into place so
+    // briefly holds the target open. Fall back to copying the COMPLETE tmp file
-    // persistence never wedges; a reader can still never see partial content.
+    // so persistence never wedges. Note: copyFile is NOT atomic — this is a
+    // last-resort path confined to Windows, matching the settings runtime's
+    // fallback, not a substitute for the atomic rename used everywhere else.
     await fsPromises.copyFile(tmp, target);
     await fsPromises.rm(tmp, { force: true });
   };

packages/web/bin/lib/cli-settings-accessors.test.js

@@ -16,8 +16,66 @@ const withTempDir = async (fn) => {
   }
 };
 
-const makeAccessors = (dir) =>
+const makeAccessors = (dir, overrides = {}) =>
-  createSettingsAccessors({ fsPromises: fs.promises, path, dataDir: dir, settingsFileName: 'settings.json' });
+  createSettingsAccessors({
+    fsPromises: fs.promises,
+    path,
+    dataDir: dir,
+    settingsFileName: 'settings.json',
+    ...overrides,
+  });
+
+// Wraps writeFile so each write lands in two chunks with a pause in between —
+// a stand-in for a large, slow write on a real disk (one open handle, so the
+// file grows from the prefix to the full payload). With a non-atomic writer a
+// concurrent reader deterministically catches the half-written file in that
+// window; with the atomic tmp+rename writer the target only ever changes via a
+// complete rename, so the window is never observable.
+const makeSlowWriteFs = () => {
+  const realFs = fs.promises;
+  const slowWriteFile = async (filePath, data) => {
+    const handle = await realFs.open(filePath, 'w');
+    try {
+      const half = Math.floor(data.length / 2);
+      await handle.writeFile(data.slice(0, half), 'utf8');
+      await new Promise((resolve) => setTimeout(resolve, 30));
+      await handle.writeFile(data.slice(half), 'utf8');
+    } finally {
+      await handle.close();
+    }
+  };
+  return { slowWriteFile, fsPromises: { ...realFs, writeFile: slowWriteFile } };
+};
+
+// Runs `writer` against filePath while a concurrent reader hammers it; returns
+// how many times the reader observed an unparseable (torn) payload. ENOENT
+// during the very first write is not a tear and is excluded.
+const countTornReads = async (filePath, writer, iterations) => {
+  const big = { theme: 'dark', filler: 'x'.repeat(4096) };
+  let torn = 0;
+  let stop = false;
+  const reader = (async () => {
+    while (!stop) {
+      try {
+        const parsed = JSON.parse(await fs.promises.readFile(filePath, 'utf8'));
+        if (parsed && typeof parsed === 'object') {
+          expect(parsed.theme).toBe('dark');
+        }
+      } catch (error) {
+        if (error?.code !== 'ENOENT') {
+          torn += 1;
+        }
+      }
+      await new Promise((resolve) => setTimeout(resolve, 0));
+    }
+  })();
+  for (let i = 0; i < iterations; i += 1) {
+    await writer({ ...big, n: i });
+  }
+  stop = true;
+  await reader;
+  return torn;
+};
 
 describe('cli settings accessors', () => {
   it('persists the full object atomically and cleans up its tmp file', async () => {
@@ -33,41 +91,36 @@ describe('cli settings accessors', () => {
     });
   });
 
-  it('never leaves a partial file observable by a concurrent reader during writes', async () => {
+  it('atomic writes: concurrent readers never observe a torn file, even under slow writes', async () => {
     await withTempDir(async (dir) => {
-      const accessors = makeAccessors(dir);
+      const { fsPromises } = makeSlowWriteFs();
+      const accessors = makeAccessors(dir, { fsPromises });
       const filePath = path.join(dir, 'settings.json');
 
-      // Hammer reads concurrently with writes; every observed payload must be a
+      // Each write is chunked with a pause, yet the reader must never see a
-      // complete, parseable object (the old plain writeFile could surface a
+      // partial payload: the target only changes via a complete atomic rename.
-      // torn file mid-rename, which is what tripped the relay identity logic).
+      const torn = await countTornReads(filePath, (settings) => accessors.writeSettingsToDisk(settings), 20);
-      const stop = { value: false };
+      expect(torn).toBe(0);
-      const reader = (async () => {
-        while (!stop.value) {
-          try {
-            const parsed = JSON.parse(await fs.promises.readFile(filePath, 'utf8'));
-            if (parsed && typeof parsed === 'object') {
-              // A complete object is always fine; anything else would be a tear.
-              expect(parsed.theme).toBe('dark');
-            }
-          } catch {
-            // ENOENT during the very first write is acceptable.
-          }
-          await new Promise((resolve) => setTimeout(resolve, 0));
-        }
-      })();
 
-      const big = { theme: 'dark', filler: 'x'.repeat(4096) };
+      const leftovers = fs.readdirSync(dir).filter((name) => name.startsWith('settings.json.tmp-'));
-      await Promise.all(
+      expect(leftovers).toEqual([]);
-        Array.from({ length: 50 }, (_, i) =>
+    });
-          accessors.writeSettingsToDisk({ ...big, n: i }).catch(() => {}),
+  });
-        ),
+
+  it('demonstrates the protected failure mode: a naive direct writer tears under the same slow write', async () => {
+    await withTempDir(async (dir) => {
+      const { slowWriteFile } = makeSlowWriteFs();
+      const filePath = path.join(dir, 'settings.json');
+
+      // The old CLI accessor wrote straight to settings.json with writeFile.
+      // The same slow-write load therefore MUST produce torn reads — proving
+      // the concurrency test above can actually fail on the pre-fix writer.
+      const torn = await countTornReads(
+        filePath,
+        (settings) => slowWriteFile(filePath, JSON.stringify(settings)),
+        20,
       );
-      stop.value = true;
+      expect(torn).toBeGreaterThan(0);
-      await reader;
-
-      const final = JSON.parse(fs.readFileSync(filePath, 'utf8'));
-      expect(final.theme).toBe('dark');
     });
   });