From ee14640ae9f0c1dc8c8bec072bf6d7bc947c2da2 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 21 Jul 2026 14:57:44 -0700 Subject: [PATCH] fix(page-lock): never fast-steal a fresh lock on a dead-looking PID (#2840) process.kill(pid, 0) returns ESRCH for LIVE holders in another PID namespace (containerized serve + jobs-work sharing GBRAIN_HOME), so the PID-liveness fast-steal silently stole live locks and re-opened the concurrent read-modify-write race the lock exists to prevent. Staleness is now mtime-TTL only (holders already refresh(); crashed holders age out in 5 min), matching the #2348 pglite-lock philosophy: never steal a possibly-live holder. Ownership for refresh()/release() moves from PID comparison (same cross-namespace false-self problem) to a per-acquire random token, so a stale handle can never clobber or unlink a new owner's lock. Fixes #2840 Co-Authored-By: Claude Fable 5 --- src/core/facts/fence-write.ts | 4 +-- src/core/page-lock.ts | 62 +++++++++++++++++------------------ test/page-lock.test.ts | 24 ++++++++++---- 3 files changed, 50 insertions(+), 40 deletions(-) diff --git a/src/core/facts/fence-write.ts b/src/core/facts/fence-write.ts index ae35769fb..404a32a95 100644 --- a/src/core/facts/fence-write.ts +++ b/src/core/facts/fence-write.ts @@ -11,8 +11,8 @@ * * Concurrency: reuses the v0.28 page-lock primitive * (`src/core/page-lock.ts`), an FS-level lockfile under - * `~/.gbrain/page-locks/.lock` with PID-liveness + - * 5-minute TTL. Multi-process safe — two `gbrain` invocations writing + * `~/.gbrain/page-locks/.lock` with a 5-minute + * mtime TTL. Multi-process safe — two `gbrain` invocations writing * to the same entity page serialize through the same kernel-visible * lockfile. 5-second timeout per the plan's "5s retry" failure mode. * diff --git a/src/core/page-lock.ts b/src/core/page-lock.ts index a8346bbc9..315e8d625 100644 --- a/src/core/page-lock.ts +++ b/src/core/page-lock.ts @@ -9,8 +9,13 @@ * Lock file path: `~/.gbrain/page-locks/.lock`. SHA-256 * keeps filenames safe regardless of slug content (slashes, unicode, etc.). * - * File contents: `{pid}\n{iso-timestamp}`. Staleness = mtime older than - * `LOCK_TTL_MS` (5 min) OR the PID is no longer alive on this host. + * File contents: `{pid}\n{iso-timestamp}\n{owner-token}`. Staleness = mtime + * older than `LOCK_TTL_MS` (5 min) — nothing else. PID-liveness + * (`process.kill(pid, 0)`) is deliberately NOT consulted (#2840): across PID + * namespaces (two containers sharing GBRAIN_HOME) a live holder's PID reads + * as ESRCH, so a liveness fast-steal silently steals a live lock. Same + * philosophy as the #2348 pglite-lock: never steal a possibly-live holder. + * Holders `refresh()` to stay fresh; a crashed holder ages out via the TTL. * * Usage: * @@ -24,7 +29,7 @@ import { existsSync, mkdirSync, readFileSync, statSync, unlinkSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; -import { createHash } from 'node:crypto'; +import { createHash, randomUUID } from 'node:crypto'; import { gbrainPath } from './config.ts'; const LOCK_TTL_MS = 5 * 60 * 1000; // 5 minutes — matches eng-review fold spec @@ -53,62 +58,57 @@ function lockPathFor(slug: string, lockRoot?: string): string { return join(dir, `${sha}.lock`); } -function isPidAlive(pid: number): boolean { - if (pid <= 0) return false; - // Note: unlike cycle.ts (single lock per process), page-lock allows - // multiple concurrent locks per process for DIFFERENT slugs. A same-pid - // collision on the SAME slug means another concurrent caller in this - // process holds it — treat as live and let mtime expiry handle stale - // post-crash cases. - if (pid === process.pid) return true; +/** Third line of the lock file. Ownership checks use this token, not the PID: + * PID numbers are meaningless identity across PID namespaces (container A's + * pid 7 and container B's pid 7 are different processes on a shared volume). */ +function tokenOf(lockPath: string): string | null { try { - process.kill(pid, 0); - return true; - } catch (e) { - const code = (e as NodeJS.ErrnoException).code; - // ESRCH = no such process; anything else (e.g. EPERM) = still alive. - return code !== 'ESRCH'; + return readFileSync(lockPath, 'utf-8').trim().split('\n')[2] ?? null; + } catch { + return null; } } function tryAcquireOnce(slug: string, lockPath: string): PageLockHandle | null { const dir = join(lockPath, '..'); mkdirSync(dir, { recursive: true }); - const pid = process.pid; if (existsSync(lockPath)) { try { const st = statSync(lockPath); const ageMs = Date.now() - st.mtimeMs; - const content = readFileSync(lockPath, 'utf-8').trim(); - const existingPid = parseInt(content.split('\n')[0] || '0', 10); - const pidAlive = isPidAlive(existingPid); - - if (pidAlive && ageMs < LOCK_TTL_MS) { - return null; // live holder + // #2840: staleness is mtime-TTL ONLY. Do NOT fast-steal on a + // dead-looking PID — process.kill(pid, 0) returns ESRCH for LIVE + // holders in another PID namespace (containerized serve + jobs-work + // sharing GBRAIN_HOME), so "PID dead" is not evidence the lock is free. + if (ageMs < LOCK_TTL_MS) { + return null; // fresh lock — assume live holder } - // Stale — fall through to overwrite. + // TTL expired — stale, fall through to overwrite. } catch { - // Any read/stat error → treat as stale. + // Stat error (file vanished mid-check) → treat as stale. } } - writeFileSync(lockPath, `${pid}\n${new Date().toISOString()}\n`); + const ownerToken = randomUUID(); + const write = () => + writeFileSync(lockPath, `${process.pid}\n${new Date().toISOString()}\n${ownerToken}\n`); + write(); return { slug, refresh: async () => { try { - writeFileSync(lockPath, `${pid}\n${new Date().toISOString()}\n`); + // Only refresh if the on-disk lock is still ours — a stale handle + // must never clobber a new owner's lock (same rule as pglite-lock). + if (tokenOf(lockPath) === ownerToken) write(); } catch { /* non-fatal — next acquirer will see it as stale */ } }, release: async () => { try { - const content = readFileSync(lockPath, 'utf-8').trim(); - const heldPid = parseInt(content.split('\n')[0] || '0', 10); - if (heldPid === pid) unlinkSync(lockPath); + if (tokenOf(lockPath) === ownerToken) unlinkSync(lockPath); } catch { /* already gone */ } diff --git a/test/page-lock.test.ts b/test/page-lock.test.ts index e6443f7ce..6f7bade71 100644 --- a/test/page-lock.test.ts +++ b/test/page-lock.test.ts @@ -56,15 +56,22 @@ describe('acquirePageLock', () => { await lock!.release(); }); - test('reclaims lock when holder PID is no longer alive', async () => { + test('#2840: does NOT steal a fresh lock whose PID looks dead (cross-PID-namespace holder)', async () => { const slug = 'people/charlie'; const path = lockFile(slug); require('node:fs').mkdirSync(tmp, { recursive: true }); - // PID 999999999 is virtually guaranteed to not exist. + // PID 999999999 reads as ESRCH here — exactly what a LIVE holder in + // another PID namespace (container sharing GBRAIN_HOME) looks like. + // Fresh mtime → must NOT be stolen; staleness is mtime-TTL only. writeFileSync(path, `999999999\n${new Date().toISOString()}\n`); const lock = await acquirePageLock(slug, { lockRoot: tmp }); - expect(lock).not.toBeNull(); - await lock!.release(); + expect(lock).toBeNull(); + // Same lock past the TTL IS reclaimable (dead holders age out). + const tenMinAgo = new Date(Date.now() - 10 * 60 * 1000); + utimesSync(path, tenMinAgo, tenMinAgo); + const after = await acquirePageLock(slug, { lockRoot: tmp }); + expect(after).not.toBeNull(); + await after!.release(); }); test('refresh() updates timestamp', async () => { @@ -86,12 +93,15 @@ describe('acquirePageLock', () => { const path = lockFile(slug); require('node:fs').mkdirSync(tmp, { recursive: true }); writeFileSync(path, `999999999\n${new Date().toISOString()}\n`); - // Acquire — this rewrites the lock with our pid. + // Backdate past the TTL so the lock is reclaimable (staleness is mtime-only). + const tenMinAgo = new Date(Date.now() - 10 * 60 * 1000); + utimesSync(path, tenMinAgo, tenMinAgo); + // Acquire — this rewrites the lock with our pid + owner token. const lock = await acquirePageLock(slug, { lockRoot: tmp }); expect(lock).not.toBeNull(); - // Manually rewrite with a foreign pid. + // Manually rewrite with a foreign owner (no matching token). writeFileSync(path, `888888888\n${new Date().toISOString()}\n`); - // Release should be a no-op (different pid). + // Release should be a no-op (different owner). await lock!.release(); expect(existsSync(path)).toBe(true); });