Summary
The cross-process driver install lock added in #1201 uses a lock directory plus rename as its primitive, with an inode comparison covering one window the ownership token cannot. Three independent reviewers, across three review rounds, converged on the same structural conclusion: that primitive cannot make "confirm this is still the lock I judged stale" and "claim it" a single atomic step, and the inode fallback rests on a guarantee that is not portable.
This issue tracks replacing the primitive. It is deliberately not a bug report against #1201 — that PR is a large net improvement over main, where there is no cross-process lock at all — but it should not be mistaken for a proof of mutual exclusion, and the remaining gaps should not be patched further in place.
Why replace rather than repair
Three rounds of fixes on this protocol each closed real races and then exposed new ones in the compensating branches themselves:
- Round 1 added the lock. Review found stale cleanup could delete a peer's fresh lock.
- Round 2 made the claim a
rename (atomic, one winner) and added a pre-check plus a post-rename verify-and-restore. Review found the restore branch leaves the lock pathname free, so a third contender can take it, the restore fails, and the moved live lock is deleted — admitting two owners.
- Round 2 also added an inode check for the
mkdir → owner.json window the token cannot cover. Review found st_ino is not a portable identity.
That pattern — each compensating branch generating the next finding — is the signature of a protocol being asked for a guarantee it cannot give, rather than of a fixable bug.
The specific gaps
1. Claim is not atomic with the staleness verdict. Nothing makes "read the owner record" and "rename the directory" one operation. The current code narrows the window from both sides (re-read before, verify what was actually moved after, restore on mismatch) but cannot close it.
2. The restore branch opens its own window. Between the mismatched rename and the restore, the lock pathname is unoccupied and a third process can create it. The restore then fails and the moved live lock is deleted.
3. Inode identity is not portable. fs.statSync(lockDir).ino is a per-filesystem identity. On Windows (untested on that PR), on overlay and network filesystems, and where a deleted directory's inode is promptly reused, the comparison can always-match (deleting a successor's live lock) or never-match (leaking our own). The failure is silent either way, and it degrades to exactly the pathname deletion it was added to prevent.
4. The lock wait outlasts only one peer. Each process counts its deadline from its own start, so with three or more simultaneous contenders the third's deadline can expire mid-install and it falls through to an unlocked performInstall over the same tree.
Proposed direction
An O_EXCL sentinel file whose content identifies the acquisition, replacing both the directory rename and the inode check:
open(path, "wx") is atomic and fails EEXIST when held — the same portable exclusion the lock directory gives — but the file has content, so the acquisition identity travels with the lock itself.- Claiming a stale lock becomes read-identity-then-conditionally-replace against a single object, rather than a verdict about one path followed by a rename of another.
- Release compares the identity in the file, so no inode comparison is needed and the
mkdir → owner.json window disappears: the identity is written by the same atomic operation that takes the lock.
Deriving the lock wait from the number of observed contenders, or re-checking readiness rather than falling through on timeout, should be considered alongside it (gap 4).
This wants its own PR, its own tests — including a multi-process control that demonstrates the assertion bites, as #1201's does — and its own review. It should not be appended to #1201.
Evidence
Six review threads on #1201, from three independent reviewers, all reaching this conclusion:
| Reviewer |
Finding |
Thread |
| codex |
Block new acquisitions while restoring a stale-lock claim |
#1201 (comment) |
| kilo-code |
releaseInstallLock's inode check relies on an unverified cross-platform guarantee |
#1201 (comment) |
| cubic |
Two acquisitions look identical when a stale lock has no readable owner.json |
#1201 (comment) |
| cubic |
Restoration rename can fail; the catch then deletes the moved live lock |
#1201 (comment) |
| cubic |
Release safety depends on a platform-dependent, unverified inode premise |
#1201 (comment) |
| codex |
Do not use reusable inodes as lock ownership |
#1201 (comment) |
Related but separate, also open on #1201 and not covered by this redesign:
Scope
withInstallLock, claimStaleLock, isStaleLock, releaseInstallLock, installLockPath / heldLockPath in packages/drivers/src/resolve.tspackages/drivers/test/install-lock.test.ts- The
external_directory permission patterns in packages/opencode/src/altimate/tools/warehouse-install-driver.ts, if the sentinel changes what paths are written
Relates to #1202.
Summary
The cross-process driver install lock added in #1201 uses a lock directory plus
renameas its primitive, with an inode comparison covering one window the ownership token cannot. Three independent reviewers, across three review rounds, converged on the same structural conclusion: that primitive cannot make "confirm this is still the lock I judged stale" and "claim it" a single atomic step, and the inode fallback rests on a guarantee that is not portable.This issue tracks replacing the primitive. It is deliberately not a bug report against #1201 — that PR is a large net improvement over
main, where there is no cross-process lock at all — but it should not be mistaken for a proof of mutual exclusion, and the remaining gaps should not be patched further in place.Why replace rather than repair
Three rounds of fixes on this protocol each closed real races and then exposed new ones in the compensating branches themselves:
rename(atomic, one winner) and added a pre-check plus a post-rename verify-and-restore. Review found the restore branch leaves the lock pathname free, so a third contender can take it, the restore fails, and the moved live lock is deleted — admitting two owners.mkdir→owner.jsonwindow the token cannot cover. Review foundst_inois not a portable identity.That pattern — each compensating branch generating the next finding — is the signature of a protocol being asked for a guarantee it cannot give, rather than of a fixable bug.
The specific gaps
1. Claim is not atomic with the staleness verdict. Nothing makes "read the owner record" and "rename the directory" one operation. The current code narrows the window from both sides (re-read before, verify what was actually moved after, restore on mismatch) but cannot close it.
2. The restore branch opens its own window. Between the mismatched rename and the restore, the lock pathname is unoccupied and a third process can create it. The restore then fails and the moved live lock is deleted.
3. Inode identity is not portable.
fs.statSync(lockDir).inois a per-filesystem identity. On Windows (untested on that PR), on overlay and network filesystems, and where a deleted directory's inode is promptly reused, the comparison can always-match (deleting a successor's live lock) or never-match (leaking our own). The failure is silent either way, and it degrades to exactly the pathname deletion it was added to prevent.4. The lock wait outlasts only one peer. Each process counts its deadline from its own start, so with three or more simultaneous contenders the third's deadline can expire mid-install and it falls through to an unlocked
performInstallover the same tree.Proposed direction
An
O_EXCLsentinel file whose content identifies the acquisition, replacing both the directory rename and the inode check:open(path, "wx")is atomic and failsEEXISTwhen held — the same portable exclusion the lock directory gives — but the file has content, so the acquisition identity travels with the lock itself.mkdir→owner.jsonwindow disappears: the identity is written by the same atomic operation that takes the lock.Deriving the lock wait from the number of observed contenders, or re-checking readiness rather than falling through on timeout, should be considered alongside it (gap 4).
This wants its own PR, its own tests — including a multi-process control that demonstrates the assertion bites, as #1201's does — and its own review. It should not be appended to #1201.
Evidence
Six review threads on #1201, from three independent reviewers, all reaching this conclusion:
releaseInstallLock's inode check relies on an unverified cross-platform guaranteeowner.jsonRelated but separate, also open on #1201 and not covered by this redesign:
Scope
withInstallLock,claimStaleLock,isStaleLock,releaseInstallLock,installLockPath/heldLockPathinpackages/drivers/src/resolve.tspackages/drivers/test/install-lock.test.tsexternal_directorypermission patterns inpackages/opencode/src/altimate/tools/warehouse-install-driver.ts, if the sentinel changes what paths are writtenRelates to #1202.