diff --git a/browse/src/cli.ts b/browse/src/cli.ts index 59327b792..8cf971c24 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -372,12 +372,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 @@ -385,19 +404,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); + }); + }); +});