mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-27 22:15:33 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
0612b0daa8
commit
ee14640ae9
@@ -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/<sha256-of-slug>.lock` with PID-liveness +
|
||||
* 5-minute TTL. Multi-process safe — two `gbrain` invocations writing
|
||||
* `~/.gbrain/page-locks/<sha256-of-slug>.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.
|
||||
*
|
||||
|
||||
+31
-31
@@ -9,8 +9,13 @@
|
||||
* Lock file path: `~/.gbrain/page-locks/<sha256-of-slug>.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 */
|
||||
}
|
||||
|
||||
+17
-7
@@ -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);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user