From bf0cbf66b3c902abbfdf58636950f00c054cb456 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 18 Sep 2026 02:14:08 +0000 Subject: [PATCH 1/4] fix: treat Windows vault lock EPERM as contention 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 --- CHANGELOG.md | 7 +++++++ src/vault.ts | 30 +++++++++++++----------------- tests/node/vault.test.ts | 4 ++++ 3 files changed, 24 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ff71be..f84c348 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,13 @@ Releases before 1.1.3 predate this file; their notes live on the proxy sits inside the trust boundary, so a loopback or LAN proxy host sees the traffic the policy already allowed. +### Fixed + +- On Windows, a vault lock publish rename that returns EPERM or EACCES is + always treated as lock contention, even when the destination is already + gone. Simultaneous stale-lock recovery could otherwise fail a writer + with a raw filesystem error after one retry. + ## [2.8.7] - 2026-09-18 ### Changed diff --git a/src/vault.ts b/src/vault.ts index c4caa08..3b8a9bb 100644 --- a/src/vault.ts +++ b/src/vault.ts @@ -1126,7 +1126,8 @@ function startLockHeartbeat(lockPath, token, handle, staleMs) { // rename can lose that race repeatedly but never for long. Retry briefly; // the pin vanishes as soon as the reader's descriptor closes. The lock's // publish rename deliberately does not come through here: EPERM there is -// the collision signal that tells a contender the lock is already taken. +// the collision signal that tells a contender the lock is already taken, +// including when the winner has already released and the dest is gone. async function renameOutlastingReaders(from, to) { if (process.platform !== "win32") return rename(from, to); let delay = 5; @@ -1223,7 +1224,6 @@ async function acquireLock(paths, timeoutMs, staleMs, afterPublish = null) { while (true) { const token = randomUUID(); const candidate = await createLockCandidate(paths.lock, token); - let observedWindowsDestination = false; let published = false; try { let retriedWindowsReleaseRace = false; @@ -1247,19 +1247,18 @@ async function acquireLock(paths, timeoutMs, staleMs, afterPublish = null) { } catch (error) { if (isWindowsRenameDestinationError(error)) { const current = await readLockDirectory(paths.lock); - if (current !== null) { - observedWindowsDestination = true; - throw error; + if (current !== null) throw error; + if (!retriedWindowsReleaseRace) { + // Windows reports an existing destination as EACCES/EPERM. The + // owner may release between that error and our observation, so + // retry this intact candidate once. If the dest is still gone, + // the outer loop treats the same error as contention instead + // of a hard access failure — 24-way stale-lock recovery can + // miss a dest that existed only long enough to reject rename. + retriedWindowsReleaseRace = true; + continue; } } - if (isWindowsRenameDestinationError(error) && !retriedWindowsReleaseRace) { - // Windows reports an existing destination as EACCES/EPERM. The - // owner may release between that error and our observation, so - // retry this intact candidate once before treating it as a real - // access failure. - retriedWindowsReleaseRace = true; - continue; - } throw error; } } @@ -1303,10 +1302,7 @@ async function acquireLock(paths, timeoutMs, staleMs, afterPublish = null) { await rm(candidate.path, { recursive: true, force: true }).catch(() => {}); if (published) throw error; const contention = - isRenameCollision(error) || - (isWindowsRenameDestinationError(error) && - (observedWindowsDestination || - (await readLockDirectory(paths.lock)) !== null)); + isRenameCollision(error) || isWindowsRenameDestinationError(error); if (!contention) throw error; } diff --git a/tests/node/vault.test.ts b/tests/node/vault.test.ts index 0fce708..5c64540 100644 --- a/tests/node/vault.test.ts +++ b/tests/node/vault.test.ts @@ -1338,6 +1338,10 @@ test("a reader holding the data file open does not fail a concurrent save", asyn }); 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. const context = await fixture(); try { await saveLogin( From 8e9b107947f340b4e078fff5a814eee71bdef5dd Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 18 Sep 2026 02:25:47 +0000 Subject: [PATCH 2/4] fix: probe Windows lock candidates before treating EPERM as contention 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 --- CHANGELOG.md | 10 ++++++---- src/vault.ts | 37 ++++++++++++++++++++++++++++++------- tests/node/vault.test.ts | 4 ++-- 3 files changed, 38 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f84c348..f89d951 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,10 +38,12 @@ Releases before 1.1.3 predate this file; their notes live on the ### Fixed -- On Windows, a vault lock publish rename that returns EPERM or EACCES is - always treated as lock contention, even when the destination is already - gone. Simultaneous stale-lock recovery could otherwise fail a writer - with a raw filesystem error after one retry. +- On Windows, a vault lock publish rename that returns EPERM or EACCES + while the destination is already gone is treated as lock contention + only after the candidate can still rename to a unique sibling. + Simultaneous stale-lock recovery no longer fails a writer with a raw + filesystem error after one retry, and a lasting ACL denial still + surfaces as that access error instead of `VAULT_LOCK_TIMEOUT`. ## [2.8.7] - 2026-09-18 diff --git a/src/vault.ts b/src/vault.ts index 3b8a9bb..bfaca8a 100644 --- a/src/vault.ts +++ b/src/vault.ts @@ -1219,14 +1219,22 @@ async function createLockCandidate(lockPath, token) { } } +async function relocateWindowsLockCandidate(candidatePath) { + const relocated = `${candidatePath}.relocated`; + await rename(candidatePath, relocated); + return relocated; +} + async function acquireLock(paths, timeoutMs, staleMs, afterPublish = null) { const started = Date.now(); while (true) { const token = randomUUID(); const candidate = await createLockCandidate(paths.lock, token); let published = false; + let windowsPublishContention = false; try { let retriedWindowsReleaseRace = false; + let relocatedWindowsCandidate = false; while (true) { try { await rename(candidate.path, paths.lock); @@ -1247,17 +1255,33 @@ async function acquireLock(paths, timeoutMs, staleMs, afterPublish = null) { } catch (error) { if (isWindowsRenameDestinationError(error)) { const current = await readLockDirectory(paths.lock); - if (current !== null) throw error; + if (current !== null) { + windowsPublishContention = true; + throw error; + } if (!retriedWindowsReleaseRace) { // Windows reports an existing destination as EACCES/EPERM. The // owner may release between that error and our observation, so - // retry this intact candidate once. If the dest is still gone, - // the outer loop treats the same error as contention instead - // of a hard access failure — 24-way stale-lock recovery can - // miss a dest that existed only long enough to reject rename. + // retry this intact candidate once. retriedWindowsReleaseRace = true; continue; } + if (!relocatedWindowsCandidate) { + // Dest is gone. A colliding dest that vanished, and a real + // ACL that forbids the publish rename, look the same. A + // unique sibling rename tells them apart: if this candidate + // can move, the source and parent are writable, so the + // original EPERM was dest contention. + try { + candidate.path = await relocateWindowsLockCandidate(candidate.path); + relocatedWindowsCandidate = true; + windowsPublishContention = true; + continue; + } catch { + throw error; + } + } + windowsPublishContention = true; } throw error; } @@ -1301,8 +1325,7 @@ async function acquireLock(paths, timeoutMs, staleMs, afterPublish = null) { await candidate.leaseHandle?.close().catch(() => {}); await rm(candidate.path, { recursive: true, force: true }).catch(() => {}); if (published) throw error; - const contention = - isRenameCollision(error) || isWindowsRenameDestinationError(error); + const contention = isRenameCollision(error) || windowsPublishContention; if (!contention) throw error; } diff --git a/tests/node/vault.test.ts b/tests/node/vault.test.ts index 5c64540..afdc08f 100644 --- a/tests/node/vault.test.ts +++ b/tests/node/vault.test.ts @@ -1340,8 +1340,8 @@ test("a reader holding the data file open does not fail a concurrent save", asyn 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. + // can release before a loser observes it. A sibling rename of the intact + // candidate distinguishes that vanished dest from a lasting ACL denial. const context = await fixture(); try { await saveLogin( From 9884b05264c724c8b843498075bac4a71b8fec72 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 18 Sep 2026 02:34:11 +0000 Subject: [PATCH 3/4] test: inject dest-less Windows vault lock EPERM on every platform 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 --- src/vault.ts | 58 ++++++++++++++++++++++++++++------------ tests/node/vault.test.ts | 54 +++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 17 deletions(-) diff --git a/src/vault.ts b/src/vault.ts index bfaca8a..a6780b5 100644 --- a/src/vault.ts +++ b/src/vault.ts @@ -120,8 +120,8 @@ function isRenameCollision(error) { return ["EEXIST", "ENOTEMPTY"].includes(error?.code); } -function isWindowsRenameDestinationError(error) { - return process.platform === "win32" && ["EACCES", "EPERM"].includes(error?.code); +function isWindowsRenameDestinationError(error, platform: string = process.platform) { + return platform === "win32" && ["EACCES", "EPERM"].includes(error?.code); } async function pathExists(candidate) { @@ -1219,13 +1219,21 @@ async function createLockCandidate(lockPath, token) { } } -async function relocateWindowsLockCandidate(candidatePath) { +async function relocateWindowsLockCandidate(candidatePath, renameFn = rename) { const relocated = `${candidatePath}.relocated`; - await rename(candidatePath, relocated); + await renameFn(candidatePath, relocated); return relocated; } -async function acquireLock(paths, timeoutMs, staleMs, afterPublish = null) { +async function acquireLock( + paths, + timeoutMs, + staleMs, + afterPublish = null, + io: { rename?: typeof rename; platform?: string } = {}, +) { + const renameFn = isCallable(io.rename) ? io.rename : rename; + const platform = isString(io.platform) ? io.platform : process.platform; const started = Date.now(); while (true) { const token = randomUUID(); @@ -1237,7 +1245,7 @@ async function acquireLock(paths, timeoutMs, staleMs, afterPublish = null) { let relocatedWindowsCandidate = false; while (true) { try { - await rename(candidate.path, paths.lock); + await renameFn(candidate.path, paths.lock); published = true; if (candidate.leaseHandle === null) { // Windows: the lease handle could not be held across the publish @@ -1253,7 +1261,7 @@ async function acquireLock(paths, timeoutMs, staleMs, afterPublish = null) { await afterPublish?.(paths.lock); break; } catch (error) { - if (isWindowsRenameDestinationError(error)) { + if (isWindowsRenameDestinationError(error, platform)) { const current = await readLockDirectory(paths.lock); if (current !== null) { windowsPublishContention = true; @@ -1273,7 +1281,7 @@ async function acquireLock(paths, timeoutMs, staleMs, afterPublish = null) { // can move, the source and parent are writable, so the // original EPERM was dest contention. try { - candidate.path = await relocateWindowsLockCandidate(candidate.path); + candidate.path = await relocateWindowsLockCandidate(candidate.path, renameFn); relocatedWindowsCandidate = true; windowsPublishContention = true; continue; @@ -1450,6 +1458,8 @@ export class LocalCredentialVault { #beforeGeneratePersistForTest = null; #afterGeneratePersistForTest = null; #afterLockPublishForTest = null; + #renameForTest = null; + #lockPlatformForTest = null; declare dir: string; declare paths: { key: string; @@ -1517,6 +1527,25 @@ export class LocalCredentialVault { if (isCallable(resolved._afterLockPublishForTest)) { this.#afterLockPublishForTest = resolved._afterLockPublishForTest; } + if (isCallable(resolved._renameForTest)) { + this.#renameForTest = resolved._renameForTest; + } + if (isString(resolved._lockPlatformForTest)) { + this.#lockPlatformForTest = resolved._lockPlatformForTest; + } + } + + #acquireLock() { + return acquireLock( + this.paths, + this.lockTimeoutMs, + this.staleLockMs, + this.#afterLockPublishForTest, + { + rename: this.#renameForTest || rename, + platform: this.#lockPlatformForTest || process.platform, + }, + ); } #trackSecret(value) { @@ -2118,12 +2147,7 @@ export class LocalCredentialVault { const target = normalizeHttpOrigin(origin); return serialize(this.dir, async () => { await ensurePrivateDirectory(this.dir); - const release = await acquireLock( - this.paths, - this.lockTimeoutMs, - this.staleLockMs, - this.#afterLockPublishForTest, - ); + const release = await this.#acquireLock(); let result; let failure = null; try { @@ -2193,7 +2217,7 @@ export class LocalCredentialVault { */ async #readSnapshot() { if (!(await pathExists(this.paths.data))) return null; - const release = await acquireLock(this.paths, this.lockTimeoutMs, this.staleLockMs); + const release = await this.#acquireLock(); try { const { key, snapshot } = await this.#load(); key.fill(0); @@ -2305,7 +2329,7 @@ export class LocalCredentialVault { const wanted = requiredString(String(id ?? ""), "Credential id", 256); return serialize(this.dir, async () => { await ensurePrivateDirectory(this.dir); - const release = await acquireLock(this.paths, this.lockTimeoutMs, this.staleLockMs); + const release = await this.#acquireLock(); try { await release.assertOwned(); if (!(await pathExists(this.paths.data))) { @@ -2444,7 +2468,7 @@ export class LocalCredentialVault { async #ownerTransaction(operation: () => Promise): Promise { return serialize(this.dir, async () => { await ensurePrivateDirectory(this.dir); - const release = await acquireLock(this.paths, this.lockTimeoutMs, this.staleLockMs); + const release = await this.#acquireLock(); try { await release.assertOwned(); return await operation(); diff --git a/tests/node/vault.test.ts b/tests/node/vault.test.ts index afdc08f..ead0f65 100644 --- a/tests/node/vault.test.ts +++ b/tests/node/vault.test.ts @@ -1377,6 +1377,60 @@ test("simultaneous stale-lock recovery cannot unlink a fresh writer lock", async } }); +test("a dest-less Windows publish EPERM succeeds after the candidate relocates", async () => { + const events = []; + let publishAttempts = 0; + const context = await fixture({ + _lockPlatformForTest: "win32", + _renameForTest: async (from, to) => { + if (path.basename(to) === "vault.lock") { + publishAttempts += 1; + events.push(`publish:${publishAttempts}:${path.basename(from)}`); + if (publishAttempts <= 2) { + throw Object.assign(new Error("operation not permitted"), { code: "EPERM" }); + } + } else { + events.push(`move:${path.basename(from)}->${path.basename(to)}`); + } + return rename(from, to); + }, + }); + try { + const listed = await context.vault.handleRequest("list", {}, EXAMPLE); + assert.deepEqual(listed.credentials, []); + assert.equal(publishAttempts, 3); + assert.match(events[0], /^publish:1:vault\.lock\.candidate\./); + assert.match(events[1], /^publish:2:vault\.lock\.candidate\./); + assert.match(events[2], /^move:vault\.lock\.candidate\..+\.relocated$/); + assert.match(events[3], /^publish:3:vault\.lock\.candidate\..+\.relocated$/); + } finally { + await context.cleanup(); + } +}); + +test("a dest-less Windows publish EPERM that cannot relocate stays an access error", async () => { + const context = await fixture({ + lockTimeoutMs: 200, + _lockPlatformForTest: "win32", + _renameForTest: async (from, to) => { + if (path.basename(to) === "vault.lock" || to.endsWith(".relocated")) { + throw Object.assign(new Error("operation not permitted"), { code: "EACCES" }); + } + return rename(from, to); + }, + }); + try { + const started = Date.now(); + await assert.rejects( + context.vault.handleRequest("list", {}, EXAMPLE), + (error: any) => error?.code === "EACCES", + ); + assert.ok(Date.now() - started < 1_000, "must not wait for the lock timeout"); + } finally { + await context.cleanup(); + } +}); + test("a replaced live lock is detected without deleting its replacement", async () => { let entered; let resume; From 10ce39d80a28f8988494be996bf9e3e0dd45a887 Mon Sep 17 00:00:00 2001 From: CuriosityOS <184718100+CuriosityOS@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:19:09 +0800 Subject: [PATCH 4/4] vault: only record Windows publish contention when the error is classified 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. --- src/vault.ts | 5 +++-- tests/node/vault.test.ts | 29 +++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/vault.ts b/src/vault.ts index a6780b5..c02e673 100644 --- a/src/vault.ts +++ b/src/vault.ts @@ -1279,11 +1279,12 @@ async function acquireLock( // ACL that forbids the publish rename, look the same. A // unique sibling rename tells them apart: if this candidate // can move, the source and parent are writable, so the - // original EPERM was dest contention. + // original EPERM was dest contention. Contention is only + // recorded once a later publish error is classified as such, + // so an unrelated failure on the retry still surfaces as-is. try { candidate.path = await relocateWindowsLockCandidate(candidate.path, renameFn); relocatedWindowsCandidate = true; - windowsPublishContention = true; continue; } catch { throw error; diff --git a/tests/node/vault.test.ts b/tests/node/vault.test.ts index ead0f65..a8acb6c 100644 --- a/tests/node/vault.test.ts +++ b/tests/node/vault.test.ts @@ -1431,6 +1431,35 @@ test("a dest-less Windows publish EPERM that cannot relocate stays an access err } }); +test("an unrelated publish failure after the candidate relocates is not retried as contention", async () => { + let publishAttempts = 0; + const context = await fixture({ + lockTimeoutMs: 200, + _lockPlatformForTest: "win32", + _renameForTest: async (from, to) => { + if (path.basename(to) === "vault.lock") { + publishAttempts += 1; + if (publishAttempts <= 2) { + throw Object.assign(new Error("operation not permitted"), { code: "EPERM" }); + } + throw Object.assign(new Error("no space left on device"), { code: "ENOSPC" }); + } + return rename(from, to); + }, + }); + try { + const started = Date.now(); + await assert.rejects( + context.vault.handleRequest("list", {}, EXAMPLE), + (error: any) => error?.code === "ENOSPC", + ); + assert.equal(publishAttempts, 3, "the relocated candidate is published once, then the error surfaces"); + assert.ok(Date.now() - started < 1_000, "must not wait for the lock timeout"); + } finally { + await context.cleanup(); + } +}); + test("a replaced live lock is detected without deleting its replacement", async () => { let entered; let resume;