fix(browse): surface non-EEXIST errors in acquireServerLock instead of masking them

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 <noreply@anthropic.com>
This commit is contained in:
Garry Tan
2026-08-14 20:21:00 -07:00
co-authored by Claude Fable 5
parent 8e46c6e521
commit 4122721eb0
2 changed files with 128 additions and 13 deletions
+49 -13
View File
@@ -408,12 +408,31 @@ async function startServer(extraEnv?: Record<string, string>): Promise<ServerSta
throw new Error(`Server failed to start within ${MAX_START_WAIT / 1000}s`);
}
function errorCode(err: unknown): string {
if (err && typeof err === 'object' && 'code' in err) {
const code = (err as { code?: unknown }).code;
if (typeof code === 'string' && code.length > 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);
}
}