From 9979bf289f0903dc6573b117149ad4bd103b222b Mon Sep 17 00:00:00 2001 From: Arzaan <25bee110@nith.ac.in> Date: Wed, 26 Aug 2026 13:48:15 +0530 Subject: [PATCH 1/2] Improve file lock handling with heartbeat mechanism Enhance file lock mechanism with heartbeat and mtime refresh to prevent premature lock expiration. --- src/site-memory/file-lock.ts | 60 +++++++++++++++++++++++++++++++----- 1 file changed, 53 insertions(+), 7 deletions(-) diff --git a/src/site-memory/file-lock.ts b/src/site-memory/file-lock.ts index 5e1761f4..b085ac11 100644 --- a/src/site-memory/file-lock.ts +++ b/src/site-memory/file-lock.ts @@ -9,14 +9,19 @@ * marker the filesystem can see: `open(..., 'wx')` creates the lock file only * 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. + * Abandoned locks never wedge site memory. A lock whose owner process is + * confirmed gone is broken on the next attempt. `staleMs` only breaks a lock + * when we cannot confirm the owner is still running (different host, or an + * owner field we can't check) — a same-host owner we can see is alive is never + * broken on staleness alone, so a critical section that runs long (a big file, + * a slow disk, a loaded machine) is never mistaken for a crash. The holder also + * refreshes the lock's mtime with a heartbeat while it works, as a second line + * of defense for the cross-host/unconfirmed-owner case. `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'; @@ -50,14 +55,46 @@ 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, token, staleMs); try { return await fn(); } finally { + clearInterval(heartbeat); await release(lockPath, token); } } +/** + * Refresh the lock file's mtime while the critical section is still running, so a + * critical section that legitimately runs longer than `staleMs` (a big file, a slow + * disk, a loaded machine) does not look abandoned to another process. Ticks at a + * fraction of `staleMs` so at least one refresh lands before the file would otherwise + * go stale. `.unref()`d so a stuck interval never keeps the process alive. + */ +function startHeartbeat(lockPath: string, token: string, staleMs: number): NodeJS.Timeout { + const intervalMs = Math.max(1_000, Math.floor(staleMs / 3)); + const timer = setInterval(() => { + void touchIfOwned(lockPath, token); + }, intervalMs); + timer.unref?.(); + return timer; +} + +/** Only refresh the mtime if we still hold this lock — never extend a lock reassigned to someone else. */ +async function touchIfOwned(lockPath: string, token: string): Promise { + try { + const owner = await readOwner(lockPath); + if (owner.token !== token) return; + const now = new Date(); + await utimes(lockPath, now, now); + } catch { + // Best-effort: if the touch fails, breakIfAbandoned's owner-liveness check is + // still there to stop a live holder's lock from being broken out from under it. + } +} + async function acquire(lockPath: string, options: FileLockOptions): Promise { const staleMs = options.staleMs ?? LOCK_STALE_MS; const timeoutMs = options.timeoutMs ?? LOCK_TIMEOUT_MS; @@ -101,8 +138,17 @@ async function breakIfAbandoned(lockPath: string, staleMs: number): Promise staleMs; - const ownerGone = owner.host === hostname() && isActionablePid(owner.pid) && !isPidAlive(owner.pid); + const ownerGone = checkable && !isPidAlive(owner.pid); if (!expired && !ownerGone) return false; const after = await statOrUndefined(lockPath); From 38a0b94b9c974166776783a25a1808008a5a03b7 Mon Sep 17 00:00:00 2001 From: Arzaan <25bee110@nith.ac.in> Date: Wed, 26 Aug 2026 13:49:01 +0530 Subject: [PATCH 2/2] Add tests for file lock behavior with stale locks --- src/site-memory/file-lock.test.ts | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/site-memory/file-lock.test.ts b/src/site-memory/file-lock.test.ts index d323eab2..424c0e99 100644 --- a/src/site-memory/file-lock.test.ts +++ b/src/site-memory/file-lock.test.ts @@ -67,6 +67,34 @@ describe('site memory file lock', () => { await expect(readFile(lockPathFor(target), 'utf8')).resolves.toContain('held'); }); + it('does not break a live same-host lock just because it is older than staleMs', async () => { + const target = await tempTarget(); + const lockPath = lockPathFor(target); + await writeFile(lockPath, `${JSON.stringify({ pid: process.pid, host: hostname(), token: 'still-working' })}\n`); + const past = new Date(Date.now() - 60_000); + await utimes(lockPath, past, past); + + // Same bug as the header comment describes: a legitimate critical section that + // outlives staleMs must never be conceded to a second writer while it's still running. + await expect(withFileLock(target, async () => 'written', { staleMs: 1_000, timeoutMs: 50 })) + .rejects.toMatchObject({ code: 'SITE_MEMORY_BUSY' }); + await expect(readFile(lockPath, 'utf8')).resolves.toContain('still-working'); + }); + + it('heartbeats the mtime of a lock held across a critical section longer than staleMs', async () => { + const target = await tempTarget(); + const lockPath = lockPathFor(target); + + await withFileLock(target, async () => { + await new Promise((resolve) => { setTimeout(resolve, 120); }); + // A concurrent second acquirer must not be able to break this lock mid-flight. + await expect(withFileLock(target, async () => 'written', { staleMs: 30, timeoutMs: 20 })) + .rejects.toMatchObject({ code: 'SITE_MEMORY_BUSY' }); + }, { staleMs: 30, timeoutMs: 1_000 }); + + await expect(exists(lockPath)).resolves.toBe(false); + }); + it('does not delete a lock that was broken and taken over by someone else', async () => { const target = await tempTarget(); const lockPath = lockPathFor(target);