site-memory file lock breaks mutual exclusion when a critical section runs longer than staleMs

[arzaanxeng]

Context

src/site-memory/file-lock.ts implements a cross-process advisory lock so two webcmd processes never do a concurrent read-modify-write on the same site-memory file (notes.md, endpoints.json, field-map.json). Per its own header comment, that's exactly the failure it exists to prevent:

Two webcmd processes running that sequence against one file both read the same body and the second rename discards the first process's work.

The bug

breakIfAbandoned() decides a lock is abandoned using only two signals:

1. the lock file's mtime is older than staleMs (10s by default), OR
2. the owning pid is confirmed dead

It does not check whether the owning process is actually still alive and working. If a critical section legitimately runs longer than staleMs — a big file, a slow disk, a loaded machine, a network-mounted home directory — a second process will conclude the first one crashed, delete its lock file, and start its own critical section while the first process is still running its own. Both writers now race the same file, and whichever rename() lands last silently discards the other's work — no error, no warning.

The code comment justifies the 10s threshold by asserting "the critical section itself is a small read plus a rename, which takes milliseconds." That's true for today's callers in local-store.ts, but nothing enforces it — there's no heartbeat or liveness signal from the lock holder, just a timestamp check. Any slow I/O, contention, or a future caller doing more work inside withFileLock silently reintroduces the exact race this file is supposed to prevent.

Reproduction

Attached script (repro-lock-race.ts):

npx tsx repro-lock-race.ts

It runs two workers against withFileLock with a compressed staleMs (50ms) to make the race deterministic and fast — same mechanism as the real 10s default, just sped up. Worker A holds the lock for 150ms (3x staleMs); Worker B starts 80ms in.

Actual output, every run (confirmed 4/4):

OVERLAP: A + B are BOTH inside the critical section at once

RESULT: BUG CONFIRMED — two workers held the "lock" simultaneously.

Exit code 1.

A correct lock would make Worker B wait for Worker A regardless of how long A's section takes; this script would print no overlap and exit 0.

Why this matters

• It's silent — no crash, no log line, no exit-code signal. It would only surface as site memory occasionally losing a note/endpoint/field-mapping under real-world timing, which is hard to catch by hand-testing and easy to misattribute to something else.
• Current in-repo callers (updateText/updateJson in local-store.ts) are fast enough in practice that this likely hasn't fired in the wild — but that's incidental to caller behavior today, not something the lock itself guarantees.

Suggested direction (not attempting a fix here)

The timestamp-only staleness check needs to become an actual liveness check — e.g. the lock holder periodically touches/refreshes the lock file (or its pid) while inside the critical section, and breakIfAbandoned() only breaks a lock that's both stale and whose owning pid is confirmed gone (dropping the expired-only branch, or requiring a heartbeat extension instead of a flat timeout). Wanted to flag this and get a maintainer's take on the right approach before sending a PR, since it's a change to the actual exclusion guarantee rather than a self-contained fix.

repro-lock-race.ts