acquireLock (src/update.ts) is the lock behind pull/push (the sync lock that keeps another pull from racing the report), migrate, bootstrap, the update check and, since #758, the one-time usage discard. Under contention it hands the same lock to two live processes.
Evidence
16 processes, each calling acquireLock / releaseLock on one path in a loop. While a process holds the lock it creates a marker file with O_EXCL; EEXIST means a second holder at the same time.
| Run |
Attempts |
Acquired |
Two holders at once |
| 1 |
16 × 2000 |
791 |
9 |
| 2 |
16 × 2000 |
1048 |
6 |
| 3 |
16 × 2000 |
955 |
12 |
About 1% of acquisitions overlap another holder.
Cause
The reclaim branch treats a lock that has just vanished as stale, then renames over whatever is there by then:
A holds the lock, B contends
B exclusiveCreate(lock) EEXIST
B isLockStale(lock) …
A releaseLock → remove(lock)
B readFile fails → "vanished" → stale = true
B acquireReclaimSentinel ok
C exclusiveCreate(lock) ok: C holds the lock
B exclusiveCreate(lock) EEXIST (C's lock)
B isLockStale(lock) may read C's file mid-write (empty → "unparseable" → stale),
or a lock that vanished again
B rename(tmp, lock) overwrites C's lock: B and C both hold it
Confirmed by counting, at the rename in that branch, targets owned by a different live pid: 100 and 122 in two runs of 16 × 1000, against 22 and 24 overlaps inside the critical section.
A second, smaller hole, found in review of #758: exclusiveCreate uses writeFile(…, { flag: 'wx' }), so the lock file exists before its content, and a reader in between sees an empty file and calls it stale. Closing only this one (payload written to a temp file, then linked into place) does not remove the overlaps: 7 in 16 × 500 with it closed.
Proposed fix
// update.ts
-async function isLockStale(resolved): Promise<boolean>
+async function lockState(resolved): Promise<'live' | 'stale' | 'missing'>
// acquireLock, reclaim branch (sentinel held)
- if (!(await isLockStale(resolved))) return false;
- rename(tmp, resolved)
+ loop: missing → exclusiveCreate again · live → return false · stale → rename(tmp, resolved)
// exclusiveCreate
- writeFile(target, payload, { flag: 'wx' })
+ writeFile(tmp, payload); link(tmp, target) // the name never exists without its contentWith the sentinel held, a lock that is present and stale cannot be replaced by anyone else, so renaming over it stays safe; only the missing case needs the retry. A regression test can run the stress loop above with a few workers (child processes) and assert no overlap.
Impact
Several sessions starting at once each run a session-start pull, so two pulls can hold the sync lock together and report the same usage twice or truncate events the other just read. For the usage discard in #758, the worst case is losing one telemetry event.
acquireLock(src/update.ts) is the lock behindpull/push(the sync lock that keeps another pull from racing the report),migrate,bootstrap, the update check and, since #758, the one-time usage discard. Under contention it hands the same lock to two live processes.Evidence
16 processes, each calling
acquireLock/releaseLockon one path in a loop. While a process holds the lock it creates a marker file withO_EXCL;EEXISTmeans a second holder at the same time.About 1% of acquisitions overlap another holder.
Cause
The reclaim branch treats a lock that has just vanished as stale, then renames over whatever is there by then:
Confirmed by counting, at the
renamein that branch, targets owned by a different live pid: 100 and 122 in two runs of 16 × 1000, against 22 and 24 overlaps inside the critical section.A second, smaller hole, found in review of #758:
exclusiveCreateuseswriteFile(…, { flag: 'wx' }), so the lock file exists before its content, and a reader in between sees an empty file and calls it stale. Closing only this one (payload written to a temp file, thenlinked into place) does not remove the overlaps: 7 in 16 × 500 with it closed.Proposed fix
With the sentinel held, a lock that is present and stale cannot be replaced by anyone else, so renaming over it stays safe; only the missing case needs the retry. A regression test can run the stress loop above with a few workers (child processes) and assert no overlap.
Impact
Several sessions starting at once each run a session-start
pull, so two pulls can hold the sync lock together and report the same usage twice or truncate events the other just read. For the usage discard in #758, the worst case is losing one telemetry event.