From 4122721eb00dff5004ca74ee48d27e977c7a0502 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 14 Aug 2026 19:38:59 -0700 Subject: [PATCH] fix(browse): surface non-EEXIST errors in acquireServerLock instead of masking them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit acquireServerLock caught every open failure as if the lock were held: EACCES/EROFS/ENOENT surfaced as phantom "another process holds the lock" (null return, no diagnostics), and a failed stale-lock read or unlink was swallowed the same way. Each failure class now logs a coded, pathed diagnostic: non-EEXIST open errors, holder-PID read errors (ENOENT retries the acquire — the holder released between open and read), and stale-lock unlink errors. Four-case unit test included. Closes #1084. Contributed by @jbetala7 (PR #1725); same fix independently by @JiayuuWang (PR #1097). Co-Authored-By: Claude Fable 5 --- browse/src/cli.ts | 62 ++++++++++++++++++++++------ browse/test/cli-lock.test.ts | 79 ++++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 13 deletions(-) create mode 100644 browse/test/cli-lock.test.ts diff --git a/browse/src/cli.ts b/browse/src/cli.ts index 3c00f0fc2..ced2385fb 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -408,12 +408,31 @@ async function startServer(extraEnv?: Record): Promise 0) return code; + } + return 'UNKNOWN'; +} + +function errorMessage(err: unknown): string { + if (err && typeof err === 'object' && 'message' in err) { + const message = (err as { message?: unknown }).message; + if (typeof message === 'string' && message.length > 0) return message; + } + return String(err); +} + +function logServerLockError(action: string, lockPath: string, err: unknown): void { + console.error(`[browse] acquireServerLock: unexpected ${errorCode(err)} while ${action} ${lockPath}: ${errorMessage(err)}`); +} + /** * Acquire an exclusive lockfile to prevent concurrent ensureServer() races (TOCTOU). * Returns a cleanup function that releases the lock. */ -function acquireServerLock(): (() => void) | null { - const lockPath = `${config.stateFile}.lock`; +export function acquireServerLock(lockPath: string = `${config.stateFile}.lock`): (() => 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 @@ -421,19 +440,36 @@ 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 - try { - const holderPid = parseInt(fs.readFileSync(lockPath, 'utf8').trim(), 10); - if (holderPid && isProcessAlive(holderPid)) { - return null; // Another live process holds the lock - } - // Stale lock — remove and retry - fs.unlinkSync(lockPath); - return acquireServerLock(); - } catch { + } catch (err) { + if (errorCode(err) !== 'EEXIST') { + logServerLockError('opening', lockPath, err); return null; } + + // Lock already held — check if the holder is still alive + let holderPid: number; + try { + holderPid = parseInt(fs.readFileSync(lockPath, 'utf8').trim(), 10); + } catch (readErr) { + if (errorCode(readErr) === 'ENOENT') { + return acquireServerLock(lockPath); + } + logServerLockError('reading holder PID from', lockPath, readErr); + return null; + } + + if (holderPid && isProcessAlive(holderPid)) { + return null; // Another live process holds the lock + } + + // Stale lock — remove and retry + try { + fs.unlinkSync(lockPath); + } catch (unlinkErr) { + logServerLockError('removing stale', lockPath, unlinkErr); + return null; + } + return acquireServerLock(lockPath); } } diff --git a/browse/test/cli-lock.test.ts b/browse/test/cli-lock.test.ts new file mode 100644 index 000000000..79f4df8d3 --- /dev/null +++ b/browse/test/cli-lock.test.ts @@ -0,0 +1,79 @@ +import { describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { acquireServerLock } from '../src/cli'; + +function withTempDir(fn: (dir: string) => T): T { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-lock-')); + try { + return fn(dir); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +function captureErrors(fn: () => T): { result: T; messages: string[] } { + const original = console.error; + const messages: string[] = []; + console.error = (...args: unknown[]) => { + messages.push(args.map(String).join(' ')); + }; + try { + return { result: fn(), messages }; + } finally { + console.error = original; + } +} + +describe('browse CLI server lock diagnostics (#1084)', () => { + test('logs non-EEXIST open failures instead of reporting phantom lock contention', () => { + withTempDir((dir) => { + const lockPath = path.join(dir, 'missing-parent', 'browse.json.lock'); + const { result, messages } = captureErrors(() => acquireServerLock(lockPath)); + + expect(result).toBeNull(); + expect(messages.join('\n')).toContain('unexpected ENOENT while opening'); + expect(messages.join('\n')).toContain(lockPath); + }); + }); + + test('returns null silently when a live process holds the lock', () => { + withTempDir((dir) => { + const lockPath = path.join(dir, 'browse.json.lock'); + fs.writeFileSync(lockPath, `${process.pid}\n`); + + const { result, messages } = captureErrors(() => acquireServerLock(lockPath)); + + expect(result).toBeNull(); + expect(messages).toEqual([]); + }); + }); + + test('logs holder PID read failures with code and lock path', () => { + withTempDir((dir) => { + const lockPath = path.join(dir, 'browse.json.lock'); + fs.mkdirSync(lockPath); + + const { result, messages } = captureErrors(() => acquireServerLock(lockPath)); + + expect(result).toBeNull(); + expect(messages.join('\n')).toContain('unexpected EISDIR while reading holder PID from'); + expect(messages.join('\n')).toContain(lockPath); + }); + }); + + test('removes stale lock and reacquires it', () => { + withTempDir((dir) => { + const lockPath = path.join(dir, 'browse.json.lock'); + fs.writeFileSync(lockPath, 'not-a-pid\n'); + + const release = acquireServerLock(lockPath); + + expect(release).toBeFunction(); + expect(fs.readFileSync(lockPath, 'utf-8').trim()).toBe(String(process.pid)); + release?.(); + expect(fs.existsSync(lockPath)).toBe(false); + }); + }); +});