fix(browse): lock acquisition reports real errors instead of phantom contention (#1084)

acquireServerLock's bare catch treated EVERY failure as 'another process
holds the lock' — a missing state dir, EACCES, or ENOSPC read as permanent
phantom contention with nothing to debug. Now only EEXIST is contention:
ENOENT self-heals with one mkdirSecure retry, everything else throws
ServerLockError carrying the real errno, and the stale-lock unlink/retry
loop is depth-capped so it can't livelock. Fork's five-case test ported.

Ported from time-attack/gstack (GStack 2).

Co-authored-by: Sina Matian <sina@time-attack.dev>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

browse/src/cli.ts

@@ -372,12 +372,29 @@ async function startServer(extraEnv?: Record<string, string>): Promise<ServerSta
   throw new Error(`Server failed to start within ${MAX_START_WAIT / 1000}s`);
 }
 
+export class ServerLockError extends Error {
+  code: string;
+  constructor(code: string, lockPath: string, cause: string) {
+    super(`E_SERVER_LOCK (${code}): cannot acquire ${lockPath} — ${cause}`);
+    this.name = 'ServerLockError';
+    this.code = code;
+  }
+}
+
 /**
  * Acquire an exclusive lockfile to prevent concurrent ensureServer() races (TOCTOU).
- * Returns a cleanup function that releases the lock.
+ * Returns a cleanup function that releases the lock, or null when another
+ * LIVE process genuinely holds the lock (real contention).
+ *
+ * Error honesty (#1084): only EEXIST is contention. ENOENT (state dir
+ * missing) self-heals with one mkdir retry; every other errno (EACCES,
+ * ENOSPC, ...) throws ServerLockError with the real errno instead of
+ * reporting phantom "another process holds the lock" contention forever.
  */
-function acquireServerLock(): (() => void) | null {
-  const lockPath = `${config.stateFile}.lock`;
+export function acquireServerLock(
+  lockPath: string = `${config.stateFile}.lock`,
+  depth = 0,
+): (() => void) | null {
   try {
     // 'wx' — create exclusively, fails if file already exists (atomic check-and-create)
     // Using string flag instead of numeric constants for Bun Windows compatibility
@@ -385,8 +402,18 @@ function acquireServerLock(): (() => void) | null {
     fs.writeSync(fd, `${process.pid}\n`);
     fs.closeSync(fd);
     return () => { safeUnlink(lockPath); };
-  } catch {
-    // Lock already held — check if the holder is still alive
+  } catch (err: any) {
+    if (err?.code === 'ENOENT') {
+      // Lock dir missing — create it and retry once.
+      if (depth >= 1) throw new ServerLockError('ENOENT', lockPath, 'lock directory could not be created');
+      mkdirSecure(path.dirname(lockPath));
+      return acquireServerLock(lockPath, depth + 1);
+    }
+    if (err?.code !== 'EEXIST') {
+      throw new ServerLockError(err?.code || 'UNKNOWN', lockPath, err?.message || String(err));
+    }
+    // EEXIST — real contention. Check if the holder is still alive.
+    // Depth cap 5 bounds the stale-lock unlink/retry livelock.
     try {
       const holderPid = parseInt(fs.readFileSync(lockPath, 'utf8').trim(), 10);
       if (holderPid && isProcessAlive(holderPid)) {
@@ -394,9 +421,15 @@ function acquireServerLock(): (() => void) | null {
       }
       // Stale lock — remove and retry
       fs.unlinkSync(lockPath);
-      return acquireServerLock();
-    } catch {
-      return null;
+      if (depth >= 5) return null;
+      return acquireServerLock(lockPath, depth + 1);
+    } catch (readErr: any) {
+      if (readErr?.code === 'ENOENT') {
+        // Lock vanished between open and read (holder released) — retry.
+        if (depth >= 5) return null;
+        return acquireServerLock(lockPath, depth + 1);
+      }
+      throw new ServerLockError(readErr?.code || 'UNKNOWN', lockPath, readErr?.message || String(readErr));
     }
   }
 }

browse/test/server-lock-errors.test.ts

@@ -0,0 +1,80 @@
+/**
+ * acquireServerLock error honesty (#1084 regression).
+ *
+ * The old code wrapped fs.openSync(lockPath, 'wx') in a bare `catch {}` —
+ * EVERY errno (EACCES, EIO, ENOSPC, ENOENT) fell into the "lock already
+ * held" path and surfaced as "another instance is starting the server",
+ * a phantom 15s contention timeout that masked the real filesystem error.
+ *
+ * New contract:
+ *   - EEXIST + live holder  → null (real contention)
+ *   - EEXIST + dead holder  → stale lock removed, acquired
+ *   - ENOENT (dir missing)  → create dir, retry once, acquired
+ *   - anything else         → ServerLockError with the real errno
+ */
+
+import { describe, test, expect, afterAll } from 'bun:test';
+import * as fs from 'fs';
+import * as os from 'os';
+import * as path from 'path';
+import { acquireServerLock, ServerLockError } from '../src/cli';
+
+const tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-lock-'));
+afterAll(() => {
+  // Restore write perm so cleanup can delete the read-only dir.
+  try { fs.chmodSync(path.join(tmpRoot, 'rodir'), 0o700); } catch {}
+  fs.rmSync(tmpRoot, { recursive: true, force: true });
+});
+
+describe('acquireServerLock (#1084 error honesty)', () => {
+  test('happy path: acquires and releases', () => {
+    const lockPath = path.join(tmpRoot, 'happy.lock');
+    const release = acquireServerLock(lockPath);
+    expect(release).not.toBeNull();
+    expect(fs.readFileSync(lockPath, 'utf8').trim()).toBe(String(process.pid));
+    release!();
+    expect(fs.existsSync(lockPath)).toBe(false);
+  });
+
+  test('EACCES throws ServerLockError with the real errno — NOT phantom contention', () => {
+    if (process.platform === 'win32' || process.getuid?.() === 0) return; // chmod semantics differ
+    const rodir = path.join(tmpRoot, 'rodir');
+    fs.mkdirSync(rodir, { recursive: true });
+    fs.chmodSync(rodir, 0o500); // r-x: open('wx') inside fails EACCES
+    const lockPath = path.join(rodir, 'browse.json.lock');
+    let thrown: any = null;
+    try {
+      acquireServerLock(lockPath); // old code: returned null (phantom contention)
+    } catch (err) {
+      thrown = err;
+    }
+    expect(thrown).toBeInstanceOf(ServerLockError);
+    expect(thrown.code).toBe('EACCES');
+    expect(thrown.message).toContain('E_SERVER_LOCK (EACCES)');
+    expect(thrown.message).toContain(lockPath);
+  });
+
+  test('ENOENT (missing lock dir) creates the dir and acquires', () => {
+    const lockPath = path.join(tmpRoot, 'newdir', 'browse.json.lock');
+    // old code: openSync ENOENT → bare catch → readFileSync ENOENT → null
+    const release = acquireServerLock(lockPath);
+    expect(release).not.toBeNull();
+    expect(fs.existsSync(lockPath)).toBe(true);
+    release!();
+  });
+
+  test('EEXIST + dead holder: removes stale lock and acquires', () => {
+    const lockPath = path.join(tmpRoot, 'stale.lock');
+    fs.writeFileSync(lockPath, '999999999\n'); // PID that cannot be alive
+    const release = acquireServerLock(lockPath);
+    expect(release).not.toBeNull();
+    release!();
+  });
+
+  test('EEXIST + live holder: returns null (real contention, no throw)', () => {
+    const lockPath = path.join(tmpRoot, 'live.lock');
+    fs.writeFileSync(lockPath, `${process.pid}\n`); // this test process is alive
+    expect(acquireServerLock(lockPath)).toBeNull();
+    fs.unlinkSync(lockPath);
+  });
+});