fix: atomic lock create so a live holder is never stolen (lost update)#261
Merged
Conversation
Contributor
|
🎉 PR Validation ✅ PASSED Commit: Checks:
Ready to merge! ✨ 🔗 View workflow run |
vreshch
marked this pull request as ready for review
July 8, 2026 23:30
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Verdict: A - REAL data-integrity lost-update bug
The e2e "30 concurrent vault-add" test (
e2e/integrity.test.ts:279) failing once and passing on rerun is NOT a flake. Under CPU contention the advisory file lock silently lets two writers into the critical section and drops committed writes, while everyvault addstill exits 0. This is the same class of bug as cli#252/#231.Root cause
src/lib/fs/file-lock.tsacquired the lock withwriteFileSync(lockPath, "<pid> <at>", { flag: 'wx' }). Thewxcreate is atomic for the inode but not for the content: the file exists (O_CREAT|O_EXCL) before its<pid> <at>bytes are written.A concurrent
holderOf()that reads the lock in that window sees empty content,Number.parseIntyields NaN, andholderOfreturnsnull. The acquire/takeover logic then treated anullholder as "not a live holder" and proceeded totakeOverStale, whichunlinkQuiet(path)'d a live, fresh holder's lock and let the second writer in. The pid-liveness gate from cli#252 was bypassed because it never ran on a null holder.Cross-process read-race proof (one writer create/unlink loop + one reader loop on the same path, 8s):
~32% of concurrent reads observed the lock file mid-create with empty content.
Instrumented subprocess repro (30 concurrent
vault add, 40 runs, 2 cores under load) - every lost-write run is preceded by astole holder=nulltakeover, all withaddFail=0:Fix
createLockExclusivewrites the full<pid> <at>to a unique temp thenlinkSyncs it into place.linkis atomic and fails EEXIST if held (still the mutex), so the content is always complete the instant the lock inode appears. No reader can ever see an empty lock.nullholder is now treated as "unknown, refuse", never "absent/crashed". A lock is taken over only when a holder is positively proven crashed (dead pid / our own pid / past the 60s hard-stale bound).Tests (deterministic, before-fail / after-pass)
never steals a lock whose content reads empty (mid-create window)- reproduces the exact null-holder state and asserts a fresh acquirer refuses. Fails on old code (true), passes after (false).lock content is always complete the instant the file appears- 5000 acquire cycles, the lock never reads empty/half-written.Verification
npm run verifygreen (type-check + e2e types + eslint + prettier + 444 unit tests + build).git stashof the fix -> test returnstrue).