From d70e6586f3ff2d71bd1c644e8a3931c752642c5d Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 14 Aug 2026 12:49:29 -0700 Subject: [PATCH] fix(browse): lock acquisition reports real errors instead of phantom contention (#1084) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Co-Authored-By: Claude Fable 5 --- browse/src/cli.ts | 49 +++++++++++++--- browse/test/server-lock-errors.test.ts | 80 ++++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 8 deletions(-) create mode 100644 browse/test/server-lock-errors.test.ts diff --git a/browse/src/cli.ts b/browse/src/cli.ts index bd231743f..213471f54 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -372,12 +372,29 @@ async function startServer(extraEnv?: Record): Promise 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)); } } } diff --git a/browse/test/server-lock-errors.test.ts b/browse/test/server-lock-errors.test.ts new file mode 100644 index 000000000..a60afebb3 --- /dev/null +++ b/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); + }); +});