diff --git a/src/site-memory/file-lock.test.ts b/src/site-memory/file-lock.test.ts index bfedb2e9..657db491 100644 --- a/src/site-memory/file-lock.test.ts +++ b/src/site-memory/file-lock.test.ts @@ -34,6 +34,33 @@ describe('site memory file lock', () => { await expect(exists(lockPathFor(target))).resolves.toBe(false); }); + it('does not break a live holder\'s lock even when its critical section outlives staleMs', async () => { + const target = await tempTarget(); + const staleMs = 40; + let inside = 0; + let overlapped = false; + + async function worker(holdMs: number) { + await withFileLock(target, async () => { + inside += 1; + if (inside > 1) overlapped = true; + await new Promise((resolve) => { setTimeout(resolve, holdMs); }); + inside -= 1; + }, { staleMs, timeoutMs: 5_000 }); + } + + // Worker A's critical section (150ms) runs well past staleMs (40ms). + // Without a heartbeat refreshing the lock's mtime, worker B would see + // the lock as abandoned and break it out from under A. + const a = worker(150); + await new Promise((resolve) => { setTimeout(resolve, 80); }); + const b = worker(20); + await Promise.all([a, b]); + + expect(overlapped).toBe(false); + await expect(exists(lockPathFor(target))).resolves.toBe(false); + }); + it('retries a transient Windows EPERM when creating the lock', async () => { const target = await tempTarget(); vi.mocked(open).mockRejectedValueOnce( diff --git a/src/site-memory/file-lock.ts b/src/site-memory/file-lock.ts index eae2265f..338dadcf 100644 --- a/src/site-memory/file-lock.ts +++ b/src/site-memory/file-lock.ts @@ -10,22 +10,28 @@ * when it does not already exist, atomically, on every platform we support. * * Abandoned locks never wedge site memory. A lock whose owner process is gone - * is broken on the next attempt, and any lock older than `staleMs` is broken - * regardless — the critical section itself is a small read plus a rename, which - * takes milliseconds. `timeoutMs` is deliberately longer than `staleMs` so an - * abandoned lock is always broken rather than surfaced to the user as an error. + * is broken on the next attempt. A lock is otherwise only broken once it has + * gone `staleMs` without a heartbeat: the holder in `withFileLock` refreshes + * the lock file's mtime on an interval well inside `staleMs` for as long as + * `fn()` runs, so a live holder's lock never looks abandoned regardless of how + * long its critical section actually takes — only a holder that has stopped + * heartbeating (crashed, or the process died) goes stale. `timeoutMs` is + * deliberately longer than `staleMs` so an abandoned lock is always broken + * rather than surfaced to the user as an error. */ import { randomUUID } from 'node:crypto'; -import { open, readFile, stat, unlink } from 'node:fs/promises'; +import { open, readFile, stat, unlink, utimes } from 'node:fs/promises'; import { hostname } from 'node:os'; import { basename } from 'node:path'; import { CliError, EXIT_CODES } from '../errors.js'; import { isActionablePid, isPidAlive } from '../session-lease.js'; -/** A lock held longer than this is treated as abandoned by a crashed process. */ +/** A lock held longer than this without a heartbeat is treated as abandoned. */ export const LOCK_STALE_MS = 10_000; /** Total acquire budget. Longer than LOCK_STALE_MS so stale locks are broken, not reported. */ export const LOCK_TIMEOUT_MS = 15_000; +/** Heartbeats land at a fraction of staleMs, so one missed tick can't cause a false break. */ +const HEARTBEAT_DIVISOR = 3; const RETRY_MIN_MS = 5; const RETRY_MAX_MS = 50; @@ -50,14 +56,42 @@ export function lockPathFor(target: string): string { /** Run `fn` while holding the cross-process lock for `target`. */ export async function withFileLock(target: string, fn: () => Promise, options: FileLockOptions = {}): Promise { const lockPath = lockPathFor(target); + const staleMs = options.staleMs ?? LOCK_STALE_MS; const token = await acquire(lockPath, options); + const heartbeat = startHeartbeat(lockPath, staleMs); try { return await fn(); } finally { + heartbeat.stop(); await release(lockPath, token); } } +/** + * Keeps `lockPath`'s mtime fresh for as long as the caller holds the lock, so + * `breakIfAbandoned` never mistakes a live, still-working holder for a crashed + * one just because its critical section is slow. Best-effort: a missed touch + * (e.g. the file briefly unreadable under load) is not fatal — it only risks + * the lock being broken early, the same failure mode this replaces, not a new + * one — and a touch is never attempted once `stop()` has been called. + */ +function startHeartbeat(lockPath: string, staleMs: number): { stop: () => void } { + const intervalMs = Math.max(1, Math.floor(staleMs / HEARTBEAT_DIVISOR)); + let stopped = false; + const timer = setInterval(() => { + if (stopped) return; + const now = new Date(); + utimes(lockPath, now, now).catch(() => undefined); + }, intervalMs); + timer.unref?.(); + return { + stop() { + stopped = true; + clearInterval(timer); + }, + }; +} + async function acquire(lockPath: string, options: FileLockOptions): Promise { const staleMs = options.staleMs ?? LOCK_STALE_MS; const timeoutMs = options.timeoutMs ?? LOCK_TIMEOUT_MS;