Merge PR #1310: per-process state-file tempfile path to fix concurrent-write ENOENT

browse/src/server.ts

@@ -317,6 +317,27 @@ const CONSOLE_LOG_PATH = config.consoleLog;
 const NETWORK_LOG_PATH = config.networkLog;
 const DIALOG_LOG_PATH = config.dialogLog;
 
+/**
+ * Per-process state-file temp path. The state-file write pattern is
+ * `writeFileSync(tmp, ...) → renameSync(tmp, stateFile)` for atomicity,
+ * but a shared `${stateFile}.tmp` filename means two concurrent writers
+ * (cold-start race when N CLIs hit a fresh repo simultaneously, parallel
+ * /tunnel/start handlers, or a combination) collide on the rename: the
+ * first writer's renameSync moves the shared temp file out of the way,
+ * the second writer's writeFileSync re-creates it, the second rename
+ * then races with the first writer's already-renamed state. Worst case
+ * the second renameSync throws ENOENT mid-air, killing one of the
+ * spawning daemons during startup.
+ *
+ * Per-process suffix (pid + 4 random bytes) makes each writer's temp
+ * path unique. The atomic rename still gives last-writer-wins semantics
+ * for the final state.json content; the only behavior change is that
+ * concurrent writers no longer kill each other on the rename.
+ */
+function tmpStatePath(): string {
+  return `${config.stateFile}.tmp.${process.pid}.${crypto.randomBytes(4).toString('hex')}`;
+}
+
 
 // ─── Sidebar agent / chat state ripped ──────────────────────────────
 // ChatEntry, SidebarSession, TabAgentState interfaces; chatBuffer,
@@ -1597,7 +1618,7 @@ async function start() {
           // Update state file
           const stateContent = JSON.parse(fs.readFileSync(config.stateFile, 'utf-8'));
           stateContent.tunnel = { url: tunnelUrl, domain: domain || null, startedAt: new Date().toISOString() };
-          const tmpState = config.stateFile + '.tmp';
+          const tmpState = tmpStatePath();
           fs.writeFileSync(tmpState, JSON.stringify(stateContent, null, 2), { mode: 0o600 });
           fs.renameSync(tmpState, config.stateFile);
 
@@ -2127,7 +2148,7 @@ async function start() {
     // without clobbering a recycled PID.
     ...(xvfb ? { xvfbPid: xvfb.pid, xvfbStartTime: xvfb.startTime, xvfbDisplay: xvfb.display } : {}),
   };
-  const tmpFile = config.stateFile + '.tmp';
+  const tmpFile = tmpStatePath();
   fs.writeFileSync(tmpFile, JSON.stringify(state, null, 2), { mode: 0o600 });
   fs.renameSync(tmpFile, config.stateFile);
 
@@ -2208,7 +2229,7 @@ async function start() {
         // Update state file with tunnel URL
         const stateContent = JSON.parse(fs.readFileSync(config.stateFile, 'utf-8'));
         stateContent.tunnel = { url: tunnelUrl, domain: domain || null, startedAt: new Date().toISOString() };
-        const tmpState = config.stateFile + '.tmp';
+        const tmpState = tmpStatePath();
         fs.writeFileSync(tmpState, JSON.stringify(stateContent, null, 2), { mode: 0o600 });
         fs.renameSync(tmpState, config.stateFile);
       } catch (err: any) {
@@ -2238,7 +2259,7 @@ async function start() {
       console.log(`[browse] Tunnel listener bound (local-only test mode) on 127.0.0.1:${tunnelPort}`);
       const stateContent = JSON.parse(fs.readFileSync(config.stateFile, 'utf-8'));
       stateContent.tunnelLocalPort = tunnelPort;
-      const tmpState = config.stateFile + '.tmp';
+      const tmpState = tmpStatePath();
       fs.writeFileSync(tmpState, JSON.stringify(stateContent, null, 2), { mode: 0o600 });
       fs.renameSync(tmpState, config.stateFile);
     } catch (err: any) {

browse/test/server-tmp-state-path.test.ts

@@ -0,0 +1,110 @@
+/**
+ * Regression: state-file temp path uniqueness.
+ *
+ * The daemon writes `.gstack/browse.json` via the standard atomic-rename
+ * pattern: `writeFileSync(tmp, …) → renameSync(tmp, stateFile)`. The
+ * pattern is correct for a single writer. It breaks for *concurrent*
+ * writers when they share a single temp filename:
+ *
+ *   t0  Writer A: writeFileSync(stateFile + '.tmp', payloadA)
+ *   t1  Writer B: writeFileSync(stateFile + '.tmp', payloadB)   // overwrites A
+ *   t2  Writer A: renameSync(stateFile + '.tmp', stateFile)    // moves B's payload
+ *   t3  Writer B: renameSync(stateFile + '.tmp', stateFile)    // ENOENT — file gone
+ *
+ * A 15-CLI cold-start race against a fresh repo reproduces this in the
+ * wild — one of the spawned daemons dies with:
+ *
+ *   [browse] Failed to start: ENOENT: no such file or directory,
+ *   rename '…/.gstack/browse.json.tmp' -> '…/.gstack/browse.json'
+ *
+ * Fix: per-process temp path via `tmpStatePath()` (pid + 4 random bytes
+ * of suffix). Each concurrent writer gets a unique path; the atomic
+ * rename still gives last-writer-wins semantics on the final state file
+ * content, but writers no longer kill each other on the rename step.
+ *
+ * This source-level guard locks two invariants:
+ *   1. No remaining `stateFile + '.tmp'` literals in server.ts (regression
+ *      catch — a future copy-paste or revert would re-introduce the bug)
+ *   2. The 4 known state-write call sites all use `tmpStatePath()`
+ *      (positive coverage)
+ *
+ * Same pattern as terminal-agent.test.ts and dual-listener.test.ts:
+ * read source as text, assert invariant, no daemon required.
+ */
+
+import { describe, test, expect } from 'bun:test';
+import { readFileSync } from 'fs';
+import * as path from 'path';
+
+const SERVER_TS = readFileSync(
+  path.resolve(import.meta.dir, '../src/server.ts'),
+  'utf-8',
+);
+
+describe('server.ts — state-file temp-path uniqueness', () => {
+  test('no remaining `stateFile + \'.tmp\'` literals (regression catch)', () => {
+    // The shared-temp-filename pattern that caused the cold-start ENOENT
+    // race. A future contributor that copy-pastes the old pattern (or a
+    // revert) will fail this test.
+    const sharedTempLiterals = [
+      ...SERVER_TS.matchAll(/stateFile\s*\+\s*['"`]\.tmp['"`]/g),
+    ];
+    expect(
+      sharedTempLiterals.length,
+      `Found ${sharedTempLiterals.length} reference(s) to the shared ` +
+        `\`stateFile + '.tmp'\` pattern. Use \`tmpStatePath()\` instead — ` +
+        `the shared pattern races on rename when two daemons spawn ` +
+        `concurrently (cold-start race + parallel /tunnel/start).`,
+    ).toBe(0);
+  });
+
+  test('every state-file writeFileSync call uses tmpStatePath()', () => {
+    // Find every `writeFileSync(X, JSON.stringify(stateContent...` or
+    // `…(state, …)` call and verify X is `tmpStatePath()` or a variable
+    // assigned from `tmpStatePath()`.
+    const writeCalls = [
+      ...SERVER_TS.matchAll(
+        /fs\.writeFileSync\s*\(\s*(\w+)\s*,\s*JSON\.stringify\(\s*(state|stateContent)/g,
+      ),
+    ];
+    expect(
+      writeCalls.length,
+      'expected at least one state-file write site',
+    ).toBeGreaterThan(0);
+
+    for (const m of writeCalls) {
+      const varName = m[1]!;
+      // Walk back to the assignment of varName — must come from tmpStatePath()
+      const assignRe = new RegExp(
+        `(?:const|let)\\s+${varName}\\s*=\\s*tmpStatePath\\(\\)`,
+      );
+      expect(
+        assignRe.test(SERVER_TS),
+        `state-file writeFileSync uses \`${varName}\` but no \`const ${varName} = tmpStatePath()\` ` +
+          `assignment was found upstream. Either assign from tmpStatePath() ` +
+          `or pass tmpStatePath() inline — the shared \`stateFile + '.tmp'\` ` +
+          `pattern races under concurrent daemon startup`,
+      ).toBe(true);
+    }
+  });
+
+  test('tmpStatePath() declaration includes a per-process unique suffix', () => {
+    // Lock the suffix shape so a future contributor doesn't accidentally
+    // strip the uniqueness back out by simplifying the helper.
+    const declMatch = SERVER_TS.match(
+      /function tmpStatePath\(\)[^{]*\{([\s\S]*?)\n\}/,
+    );
+    expect(declMatch, 'tmpStatePath() declaration not found').not.toBeNull();
+    const body = declMatch![1]!;
+
+    // Must reference both process.pid and crypto.randomBytes — two
+    // independent sources of uniqueness.
+    expect(body, 'tmpStatePath() must include process.pid in the suffix').toContain(
+      'process.pid',
+    );
+    expect(
+      body,
+      'tmpStatePath() must include a random suffix via crypto.randomBytes',
+    ).toContain('crypto.randomBytes');
+  });
+});