From fb9ea4202c42a6997d95ce4eb606eebc9842b568 Mon Sep 17 00:00:00 2001 From: ColinM-sys Date: Wed, 8 Apr 2026 23:40:11 -0400 Subject: [PATCH 1/3] fix(cli): close stale-lock cleanup race in acquireOnboardLock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The acquireOnboardLock stale-cleanup path read a stale lock, decided the holder was dead, and unconditionally unlinked LOCK_FILE before retrying writeFileSync(wx). Two concurrent processes that both observe the same stale lock will both try to clean it up — and the slower of the two can unlink the *fresh* lock the faster process just claimed, breaking mutual exclusion: both processes end up holding 'their' lock and onboard runs in parallel against the same shared session state. Reported as #1281, originally surfaced by CodeRabbit on #1272. Fix: capture the stale file's inode via fs.statSync({ bigint: true }) at the same time we read its contents, then in a new unlinkIfInodeMatches() helper, re-stat right before fs.unlinkSync and bail if the inode has changed. The dual stat-then-unlink is the only portable POSIX primitive Node exposes for this — there is no atomic "unlink-if-inode" syscall — so a sufficiently unlucky race can still slip through. The window is orders of magnitude smaller than the unconditional unlink it replaces, and the outer retry loop will detect a wrong unlink on its next writeFileSync(wx) attempt because either we re-create the file or we observe a new lock with a different inode. Also bumps MAX_ATTEMPTS from 2 to 5 because the inode-verified cleanup can take a few more spins under contention before one cleaner wins. Adds a regression test that simulates the race deterministically by wrapping fs.statSync so the first stat succeeds against the original stale inode, then atomically swaps the lock file (unlink + recreate) to give it a new inode before unlinkIfInodeMatches re-stats it. The test asserts the fresh claim survives the race and is the file on disk after acquireOnboardLock returns. Verified by stashing the source fix and re-running: the new test fails on the unguarded code as expected, and passes with the inode guard in place. Closes #1281 --- src/lib/onboard-session.test.ts | 78 +++++++++++++++++++++++++++++++++ src/lib/onboard-session.ts | 73 ++++++++++++++++++++++++++---- 2 files changed, 142 insertions(+), 9 deletions(-) diff --git a/src/lib/onboard-session.test.ts b/src/lib/onboard-session.test.ts index d72ad02367..ed5a968780 100644 --- a/src/lib/onboard-session.test.ts +++ b/src/lib/onboard-session.test.ts @@ -173,6 +173,84 @@ describe("onboard session", () => { expect(written.pid).toBe(process.pid); }); + it("regression #1281: stale-cleanup race does not unlink a fresh lock claimed by another process", () => { + // Reproduces the race: the lock file we read as 'stale' gets replaced + // with a fresh claim from a faster concurrent process between our + // read and our unlink. The slower process must NOT unlink the fresh + // lock, otherwise both processes end up thinking they hold the lock. + fs.mkdirSync(path.dirname(session.LOCK_FILE), { recursive: true }); + + // 1. Lay down a stale lock from a dead PID (PID 999999 on the test box). + const staleLock = JSON.stringify({ + pid: 999999, + startedAt: "2026-03-25T00:00:00.000Z", + command: "nemoclaw onboard", + }); + fs.writeFileSync(session.LOCK_FILE, staleLock, { mode: 0o600 }); + + // 2. Wrap fs.statSync so that the FIRST stat (which the cleanup + // helper uses for inode comparison) sees the original stale + // inode, but right after that read we swap the file out for a + // "fresh claim from a faster concurrent process". The second + // stat (inside unlinkIfInodeMatches) will then see a different + // inode and the cleanup must skip the unlink. + let statCallCount = 0; + const originalStatSync = fs.statSync; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (fs as any).statSync = function (...args: unknown[]) { + statCallCount += 1; + // After the first stat (inside acquireOnboardLock), simulate the + // race: a concurrent fast process unlinks the stale lock and + // writes a fresh one with a different inode. + if (statCallCount === 1) { + try { + const result = (originalStatSync as unknown as (...a: unknown[]) => unknown).apply( + fs, + args, + ); + // Now swap the file: unlink + recreate produces a new inode. + fs.unlinkSync(session.LOCK_FILE); + fs.writeFileSync( + session.LOCK_FILE, + JSON.stringify({ + pid: process.pid, + startedAt: new Date().toISOString(), + command: "nemoclaw onboard (fresh claim from concurrent process)", + }), + { mode: 0o600 }, + ); + return result; + } catch (err) { + throw err; + } + } + return (originalStatSync as unknown as (...a: unknown[]) => unknown).apply(fs, args); + }; + + try { + // The acquire call will see EEXIST (stale lock present), stat it, + // then the swap happens, then the second stat (inside the cleanup + // helper) sees a different inode → must NOT unlink. + const result = session.acquireOnboardLock("nemoclaw onboard --resume"); + // The fresh lock that the simulated concurrent process wrote + // should still be on disk after acquireOnboardLock returns. + expect(fs.existsSync(session.LOCK_FILE)).toBe(true); + const onDisk = JSON.parse(fs.readFileSync(session.LOCK_FILE, "utf8")); + // The lock content should be the fresh claim, NOT the stale one + // and NOT a new one written by acquireOnboardLock after a wrong + // unlink. + expect(onDisk.command).toContain("fresh claim from concurrent process"); + // acquireOnboardLock may report success (it found its own pid in + // the fresh claim, which happens to be process.pid in this test + // simulation) or report a contender — what matters is that the + // fresh claim survived. We assert the survival above. + void result; + } finally { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (fs as any).statSync = originalStatSync; + } + }); + it("treats unreadable or transient lock contents as a retry, not a stale lock", () => { fs.mkdirSync(path.dirname(session.LOCK_FILE), { recursive: true }); fs.writeFileSync(session.LOCK_FILE, "{not-json", { mode: 0o600 }); diff --git a/src/lib/onboard-session.ts b/src/lib/onboard-session.ts index 1f470bb6a3..b95cbe968e 100644 --- a/src/lib/onboard-session.ts +++ b/src/lib/onboard-session.ts @@ -346,7 +346,15 @@ export function acquireOnboardLock(command: string | null = null): LockResult { 2, ); - for (let attempt = 0; attempt < 2; attempt++) { + // The retry budget here used to be 2, which is the bare minimum needed + // for "see-stale → cleanup → reclaim". With the inode-verified cleanup + // below it can take a few additional spins under contention because + // multiple concurrent stale-cleaners can race and lose to each other + // before one reclaims, so give the loop a little more room. + // See issue #1281. + const MAX_ATTEMPTS = 5; + + for (let attempt = 0; attempt < MAX_ATTEMPTS; attempt++) { try { fs.writeFileSync(LOCK_FILE, payload, { flag: "wx", mode: 0o600 }); return { acquired: true, lockFile: LOCK_FILE, stale: false }; @@ -355,8 +363,17 @@ export function acquireOnboardLock(command: string | null = null): LockResult { throw error; } + // Capture both the parsed lock and the inode so we can verify the + // file we're about to unlink is STILL the same stale file we read. + // Without the inode check, two concurrent processes can both read + // the same stale lock, and the slower one will unlink the fresh + // lock the faster one just claimed, breaking mutual exclusion. + // See issue #1281. let existing: LockInfo | null; + let staleInode: bigint | null; try { + const stat = fs.statSync(LOCK_FILE, { bigint: true }); + staleInode = stat.ino; existing = parseLockFile(fs.readFileSync(LOCK_FILE, "utf8")); } catch (readError: unknown) { if ((readError as NodeJS.ErrnoException)?.code === "ENOENT") { @@ -365,9 +382,12 @@ export function acquireOnboardLock(command: string | null = null): LockResult { throw readError; } if (!existing) { + // Malformed lock file — leave it on disk (a human or another + // process may be mid-write) and retry. Pre-#1281 behavior + // preserved: never unlink a malformed lock automatically. continue; } - if (existing && isProcessAlive(existing.pid)) { + if (isProcessAlive(existing.pid)) { return { acquired: false, lockFile: LOCK_FILE, @@ -378,19 +398,54 @@ export function acquireOnboardLock(command: string | null = null): LockResult { }; } - try { - fs.unlinkSync(LOCK_FILE); - } catch (unlinkError: unknown) { - if ((unlinkError as NodeJS.ErrnoException)?.code !== "ENOENT") { - throw unlinkError; - } - } + // Stale: unlink ONLY if the file on disk is still the same inode + // we just read. If a concurrent process already cleaned up and + // claimed the lock, the inode will have changed and we'll fall + // through to the next iteration where writeFileSync(wx) will + // either succeed (we win) or fail EEXIST against the new holder + // (and we re-read it). + unlinkIfInodeMatches(LOCK_FILE, staleInode); } } return { acquired: false, lockFile: LOCK_FILE, stale: true }; } +/** + * Unlink LOCK_FILE only if its current inode equals `expectedInode`. + * The dual stat-then-unlink is the only portable POSIX primitive Node + * exposes for this — there's no atomic "unlink-if-inode" syscall — so + * a sufficiently unlucky race can still slip through. The window is + * orders of magnitude smaller than the unconditional unlink it + * replaces, and the outer loop will detect a wrong unlink on its next + * `writeFileSync(wx)` attempt because either we re-create the file + * or we observe the new lock with a different inode. + */ +function unlinkIfInodeMatches(filePath: string, expectedInode: bigint | null): void { + if (expectedInode === null) { + return; + } + try { + const stat = fs.statSync(filePath, { bigint: true }); + if (stat.ino !== expectedInode) { + // Someone else replaced the file. Leave it alone. + return; + } + } catch (statError: unknown) { + if ((statError as NodeJS.ErrnoException)?.code === "ENOENT") { + return; + } + throw statError; + } + try { + fs.unlinkSync(filePath); + } catch (unlinkError: unknown) { + if ((unlinkError as NodeJS.ErrnoException)?.code !== "ENOENT") { + throw unlinkError; + } + } +} + export function releaseOnboardLock(): void { try { if (!fs.existsSync(LOCK_FILE)) return; From 19219a3d98d312cb9cab69206f21b336454b2f57 Mon Sep 17 00:00:00 2001 From: ColinM-sys Date: Thu, 9 Apr 2026 00:02:40 -0400 Subject: [PATCH 2/3] fix(cli): hold lock fd for ownership verification on release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit flagged a residual TOCTOU window in unlinkIfInodeMatches: between statSync and unlinkSync, another process could replace the file. Switch the acquisition primitive to fs.openSync(LOCK_FILE, "wx", 0o600) and keep the resulting file descriptor at module scope for the lifetime of the lock. On release, compare fstatSync(fd).ino against statSync(LOCK_FILE).ino — if the two diverge, another process owns the path now and we leave it alone. The legacy pid-based release fallback is preserved so tests that write the lock file directly (without going through acquireOnboardLock) keep their existing semantics for malformed and foreign-pid locks. Also tighten the #1281 regression test per CodeRabbit nit: the simulated concurrent writer now uses process.ppid (a distinct live PID, not process.pid) so the test asserts the mutual-exclusion loser path — result.acquired === false and result.holderPid === ppid — rather than ambiguously accepting either outcome. Refs: #1281 Signed-off-by: ColinM-sys --- src/lib/onboard-session.test.ts | 21 ++++++--- src/lib/onboard-session.ts | 75 ++++++++++++++++++++++++++++++--- 2 files changed, 85 insertions(+), 11 deletions(-) diff --git a/src/lib/onboard-session.test.ts b/src/lib/onboard-session.test.ts index ed5a968780..1e33b70c95 100644 --- a/src/lib/onboard-session.test.ts +++ b/src/lib/onboard-session.test.ts @@ -209,11 +209,17 @@ describe("onboard session", () => { args, ); // Now swap the file: unlink + recreate produces a new inode. + // Use a DISTINCT live PID (the parent process — typically the + // shell that launched the test runner) so the assertions + // below can verify the mutual-exclusion loser path: if + // acquireOnboardLock confused the fresh claim with its own + // pid, this test would silently pass even though the + // contender should lose. See CodeRabbit feedback on PR #1656. fs.unlinkSync(session.LOCK_FILE); fs.writeFileSync( session.LOCK_FILE, JSON.stringify({ - pid: process.pid, + pid: process.ppid, startedAt: new Date().toISOString(), command: "nemoclaw onboard (fresh claim from concurrent process)", }), @@ -240,11 +246,14 @@ describe("onboard session", () => { // and NOT a new one written by acquireOnboardLock after a wrong // unlink. expect(onDisk.command).toContain("fresh claim from concurrent process"); - // acquireOnboardLock may report success (it found its own pid in - // the fresh claim, which happens to be process.pid in this test - // simulation) or report a contender — what matters is that the - // fresh claim survived. We assert the survival above. - void result; + // The fresh claim is held by a different live PID (process.ppid), + // so acquireOnboardLock MUST report acquisition failure and + // surface that pid as the holder. This is the mutual-exclusion + // loser path — without it, the regression would only verify the + // fresh file survived, not that the contender correctly stood + // down. + expect(result.acquired).toBe(false); + expect(result.holderPid).toBe(process.ppid); } finally { // eslint-disable-next-line @typescript-eslint/no-explicit-any (fs as any).statSync = originalStatSync; diff --git a/src/lib/onboard-session.ts b/src/lib/onboard-session.ts index b95cbe968e..ef2cfbb28d 100644 --- a/src/lib/onboard-session.ts +++ b/src/lib/onboard-session.ts @@ -334,6 +334,13 @@ function isProcessAlive(pid: number): boolean { } } +// File descriptor we hold across the lifetime of an acquired lock. On +// release, fstat(fd).ino vs stat(path).ino confirms the on-disk path +// still resolves to the file we created — closing the residual TOCTOU +// window in the inode-only check by tying ownership to a live +// descriptor rather than a value re-read from disk. See #1281. +let heldLockFd: number | null = null; + export function acquireOnboardLock(command: string | null = null): LockResult { ensureSessionDir(); const payload = JSON.stringify( @@ -355,9 +362,13 @@ export function acquireOnboardLock(command: string | null = null): LockResult { const MAX_ATTEMPTS = 5; for (let attempt = 0; attempt < MAX_ATTEMPTS; attempt++) { + let fd: number; try { - fs.writeFileSync(LOCK_FILE, payload, { flag: "wx", mode: 0o600 }); - return { acquired: true, lockFile: LOCK_FILE, stale: false }; + // openSync(..., "wx", mode) is the atomic create-or-fail + // primitive. We hold the resulting fd at module scope so + // releaseOnboardLock() can later confirm the on-disk path still + // resolves to the same file we created (fstat ino vs stat ino). + fd = fs.openSync(LOCK_FILE, "wx", 0o600); } catch (error: unknown) { if ((error as NodeJS.ErrnoException)?.code !== "EEXIST") { throw error; @@ -401,11 +412,25 @@ export function acquireOnboardLock(command: string | null = null): LockResult { // Stale: unlink ONLY if the file on disk is still the same inode // we just read. If a concurrent process already cleaned up and // claimed the lock, the inode will have changed and we'll fall - // through to the next iteration where writeFileSync(wx) will - // either succeed (we win) or fail EEXIST against the new holder - // (and we re-read it). + // through to the next iteration where openSync(wx) will either + // succeed (we win) or fail EEXIST against the new holder (and we + // re-read it). unlinkIfInodeMatches(LOCK_FILE, staleInode); + continue; + } + + // Atomic create succeeded — write the payload and keep the fd open + // for the lifetime of the lock so releaseOnboardLock() can verify + // ownership via the live descriptor. + try { + fs.writeSync(fd, payload); + } catch (writeError) { + try { fs.closeSync(fd); } catch { /* ignore */ } + try { fs.unlinkSync(LOCK_FILE); } catch { /* ignore */ } + throw writeError; } + heldLockFd = fd; + return { acquired: true, lockFile: LOCK_FILE, stale: false }; } return { acquired: false, lockFile: LOCK_FILE, stale: true }; @@ -447,6 +472,46 @@ function unlinkIfInodeMatches(filePath: string, expectedInode: bigint | null): v } export function releaseOnboardLock(): void { + // Preferred path: we hold the fd from a successful acquireOnboardLock. + // Verify the on-disk path still resolves to the same file (fstat ino + // == stat ino) before unlinking. If they disagree, another process + // has already replaced the lock and we must NOT touch their file. + if (heldLockFd !== null) { + const fd = heldLockFd; + heldLockFd = null; + try { + const fdStat = fs.fstatSync(fd, { bigint: true }); + let pathInode: bigint | null = null; + try { + const pathStat = fs.statSync(LOCK_FILE, { bigint: true }); + pathInode = pathStat.ino; + } catch (error: unknown) { + if ((error as NodeJS.ErrnoException)?.code !== "ENOENT") { + // Unexpected — fall through to closing the fd. + } + } + if (pathInode !== null && pathInode === fdStat.ino) { + try { + fs.unlinkSync(LOCK_FILE); + } catch (unlinkError: unknown) { + if ((unlinkError as NodeJS.ErrnoException)?.code !== "ENOENT") { + // Best effort — surfacing this would mask the real error. + } + } + } + } catch { + // fstat can fail if the fd was already closed somehow; nothing + // safe to do beyond closing it below. + } finally { + try { fs.closeSync(fd); } catch { /* ignore */ } + } + return; + } + + // Fallback (no fd held — e.g., a test wrote the lock file directly, + // or a previous release already ran): preserve the legacy pid-based + // behavior so we never unlink a malformed lock and never unlink a + // lock owned by another pid. try { if (!fs.existsSync(LOCK_FILE)) return; let existing: LockInfo | null = null; From 6629df6fdff31f9e673a032e1551ac65824b2de2 Mon Sep 17 00:00:00 2001 From: Prekshi Vyas Date: Thu, 16 Apr 2026 14:23:41 -0700 Subject: [PATCH 3/3] fix(test): swap lock before stat #2 so unlinkIfInodeMatches is exercised MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit correctly flagged that swapping on stat #1 caused readFileSync to see the live PID and exit via isProcessAlive — unlinkIfInodeMatches was never called. Move the swap to just before stat #2 (inside unlinkIfInodeMatches): stat #1 reads the original stale inode, readFileSync sees the dead PID, isProcessAlive returns false, stale-cleanup runs, and stat #2 sees the new inode and skips the unlink. Use write-to-temp + rename instead of unlink + recreate to guarantee a different inode even on tmpfs/overlayfs which can reuse inodes. Signed-off-by: Prekshi Vyas --- src/lib/onboard-session.test.ts | 67 +++++++++++++++------------------ 1 file changed, 31 insertions(+), 36 deletions(-) diff --git a/src/lib/onboard-session.test.ts b/src/lib/onboard-session.test.ts index 1e33b70c95..75e75f7a3b 100644 --- a/src/lib/onboard-session.test.ts +++ b/src/lib/onboard-session.test.ts @@ -188,47 +188,42 @@ describe("onboard session", () => { }); fs.writeFileSync(session.LOCK_FILE, staleLock, { mode: 0o600 }); - // 2. Wrap fs.statSync so that the FIRST stat (which the cleanup - // helper uses for inode comparison) sees the original stale - // inode, but right after that read we swap the file out for a - // "fresh claim from a faster concurrent process". The second - // stat (inside unlinkIfInodeMatches) will then see a different - // inode and the cleanup must skip the unlink. + // 2. Wrap fs.statSync so the swap happens just before stat #2: + // - stat #1 (inside acquireOnboardLock): reads the stale inode + // and returns it unmodified. readFileSync then reads the + // ORIGINAL stale lock (dead PID 999999), isProcessAlive + // returns false, and acquireOnboardLock enters the stale- + // cleanup path calling unlinkIfInodeMatches. + // - stat #2 (inside unlinkIfInodeMatches): BEFORE the actual + // stat, swap the file for a fresh claim. stat #2 then sees + // a different inode → must skip the unlink. + // + // CodeRabbit correctly flagged the original test: swapping on + // stat #1 caused readFileSync to see the live PID and exit + // via isProcessAlive, never reaching unlinkIfInodeMatches. let statCallCount = 0; const originalStatSync = fs.statSync; // eslint-disable-next-line @typescript-eslint/no-explicit-any (fs as any).statSync = function (...args: unknown[]) { statCallCount += 1; - // After the first stat (inside acquireOnboardLock), simulate the - // race: a concurrent fast process unlinks the stale lock and - // writes a fresh one with a different inode. - if (statCallCount === 1) { - try { - const result = (originalStatSync as unknown as (...a: unknown[]) => unknown).apply( - fs, - args, - ); - // Now swap the file: unlink + recreate produces a new inode. - // Use a DISTINCT live PID (the parent process — typically the - // shell that launched the test runner) so the assertions - // below can verify the mutual-exclusion loser path: if - // acquireOnboardLock confused the fresh claim with its own - // pid, this test would silently pass even though the - // contender should lose. See CodeRabbit feedback on PR #1656. - fs.unlinkSync(session.LOCK_FILE); - fs.writeFileSync( - session.LOCK_FILE, - JSON.stringify({ - pid: process.ppid, - startedAt: new Date().toISOString(), - command: "nemoclaw onboard (fresh claim from concurrent process)", - }), - { mode: 0o600 }, - ); - return result; - } catch (err) { - throw err; - } + // Just before stat #2 (inside unlinkIfInodeMatches), simulate + // the race: a concurrent fast process unlinks the stale lock + // and writes a fresh claim. stat #2 then sees a new inode. + if (statCallCount === 2) { + // Write the fresh claim to a temp file first, then rename over + // the stale lock. This guarantees a different inode even on + // tmpfs/overlayfs which can reuse inodes after unlink+recreate. + const tmpClaim = session.LOCK_FILE + ".race-tmp"; + fs.writeFileSync( + tmpClaim, + JSON.stringify({ + pid: process.ppid, + startedAt: new Date().toISOString(), + command: "nemoclaw onboard (fresh claim from concurrent process)", + }), + { mode: 0o600 }, + ); + fs.renameSync(tmpClaim, session.LOCK_FILE); } return (originalStatSync as unknown as (...a: unknown[]) => unknown).apply(fs, args); };