Repository navigation
fix: treat Windows vault lock EPERM as contention - #211
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| (isWindowsRenameDestinationError(error) && | ||
| (observedWindowsDestination || | ||
| (await readLockDirectory(paths.lock)) !== null)); | ||
| isRenameCollision(error) || isWindowsRenameDestinationError(error); |
There was a problem hiding this comment.
🟡 Later publish failures become contention
After relocation sets windowsPublishContention, any later publish error becomes contention. Disk failures retry until VAULT_LOCK_TIMEOUT instead of reaching the caller.
Learn more
The candidate relocation proves only that the preceding EPERM or EACCES represented contention. The code then retries the publish with the relocated candidate. If that new rename fails for another reason, the persistent boolean still makes the outer catch suppress that new error.
Example: The relocation succeeds after a vanished lock caused EPERM. The next publish returns ENOSPC. The operation discards that error, creates more candidates, and eventually reports VAULT_LOCK_TIMEOUT instead of the disk-space failure.
Recommended fix: Combine windowsPublishContention with isWindowsRenameDestinationError(error) when classifying the caught error. Continue recognizing EEXIST and ENOTEMPTY through isRenameCollision.
Was this helpful? React with 👍 or 👎 to provide feedback.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| test("simultaneous stale-lock recovery cannot unlink a fresh writer lock", async () => { | ||
| // 24 children race one stale lock. On Windows the publish rename reports | ||
| // EPERM for an existing dest (research/windows-fs-probe.mjs); the winner | ||
| // can release before a loser observes it. That must be contention, not a | ||
| // fatal EPERM — Cross-platform unit tests (windows-latest, 22) on 2.8.7. |
There was a problem hiding this comment.
This stress test starts 24 writers behind a shared barrier, but it does not force the publish rename to fail with EPERM or EACCES while the lock destination disappears before it is read. All writers can therefore succeed without executing the changed Windows contention path, so a later regression in that path would not be detected. Add a focused filesystem seam or unit test that injects this error-and-missing-destination sequence, verifies a retry succeeds, and records that the branch ran. This is non-blocking, but without it the Windows-specific fix has only probabilistic coverage.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Artifacts
- Authored and executed script that inspects the test wiring and runs the unchanged vault suite three times; it shows the test has no executable error injection and Linux cannot enter the Windows-only predicate.
- Executed parent-revision build and complete vault test suite, including the 24-child stale-lock test; all 32 runnable tests passed, showing the pre-change test also passes without deterministic error forcing.
- Executed current-revision validator and three full vault-suite runs; each passed on Linux while the validator recorded the absent injector and Windows-only branch gate, confirming the coverage gap.
Cross-platform unit tests (windows-latest, 22) failed on 2.8.7: simultaneous stale-lock recovery threw EPERM when renaming a lock candidate over vault.lock. NTFS reports an existing dest as EPERM; the winner can release before the loser observes it. After one retry that still saw no dest, acquireLock treated the error as fatal. Unpublished EPERM/EACCES on the publish rename is now always contention: back off and try again, same as a visible colliding lock. Co-authored-by: Nav <CuriosityOS@users.noreply.github.com>
A dest-less publish EPERM can be a vanished colliding lock or a lasting ACL denial. Rename the intact candidate to a unique sibling: if that works, the source and parent are writable and the original error is contention. If it fails, surface the access error instead of looping until VAULT_LOCK_TIMEOUT. Co-authored-by: Nav <CuriosityOS@users.noreply.github.com>
The 24-child stress test can pass without taking the dest-less publish path. Add a rename/platform seam and two focused cases: relocate after two dest-less EPERMs, and a sibling-rename failure that stays EACCES instead of waiting out the lock timeout. Co-authored-by: Nav <CuriosityOS@users.noreply.github.com>
…ified as such After the candidate relocated, contention was flagged before the retry, so an unrelated failure on that publish (ENOSPC, EIO) looped until VAULT_LOCK_TIMEOUT instead of surfacing. Flag it only on the branches that actually classify an EPERM/EACCES as contention, and cover the unrelated-error path with a seam test.
d200818 to
10ce39d
Compare
2 participants
What and why
Cross-platform unit tests (windows-latest, 22)failed onmainata12e116(2.8.7).simultaneous stale-lock recovery cannot unlink a fresh writer lockdied withEPERMrenaming a lock candidate overvault.lock.NTFS reports an existing directory dest as EPERM (
research/windows-fs-probe.mjs). The winner can release before the loser observes the dest, so after one retryacquireLocktreated a dest-less EPERM as a hard failure.A dest-less EPERM is now probed: if the intact candidate can rename to a unique sibling, the source and parent are writable and the original error is contention. If that sibling rename also fails, the original access error is returned instead of looping until
VAULT_LOCK_TIMEOUT.A rename/platform test seam covers that sequence on every OS: two dest-less EPERMs, a recorded sibling relocate, then a successful publish; and a sibling-rename failure that stays
EACCESinstead of waiting out the lock timeout.Checklist
bun test tests/node/vault.test.tsafterbun run buildis the verification for this lock-path change. Fullrelease:checkwill run on CI (Windows included).types/*.d.tsin this same commit. No public API change._renameForTestand_lockPlatformForTestare test-only constructor hooks, same as_lockAcquiredForTest.src/worker.tsordist/src/worker.jsdirectly.CHANGELOG.mdunder Unreleased.How it was verified
a12e116,Cross-platform unit tests (windows-latest, 22).EPERM: operation not permitted, rename '…\vault.lock.candidate.…' -> '…\vault.lock'fromacquireLock, thenvault child exited 1.bun run build && bun test tests/node/vault.test.ts— 34 pass, 1 skip.