diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ff71be..f89d951 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,15 @@ 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 + 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 ### Changed diff --git a/src/vault.ts b/src/vault.ts index c4caa08..c02e673 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) { @@ -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; @@ -1218,18 +1219,33 @@ async function createLockCandidate(lockPath, token) { } } -async function acquireLock(paths, timeoutMs, staleMs, afterPublish = null) { +async function relocateWindowsLockCandidate(candidatePath, renameFn = rename) { + const relocated = `${candidatePath}.relocated`; + await renameFn(candidatePath, relocated); + return relocated; +} + +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(); const candidate = await createLockCandidate(paths.lock, token); - let observedWindowsDestination = false; let published = false; + let windowsPublishContention = false; try { let retriedWindowsReleaseRace = false; + 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 @@ -1245,20 +1261,36 @@ 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) { - observedWindowsDestination = true; + windowsPublishContention = true; throw error; } - } - 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; + 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. + 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. 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; + continue; + } catch { + throw error; + } + } + windowsPublishContention = true; } throw error; } @@ -1302,11 +1334,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) && - (observedWindowsDestination || - (await readLockDirectory(paths.lock)) !== null)); + const contention = isRenameCollision(error) || windowsPublishContention; if (!contention) throw error; } @@ -1431,6 +1459,8 @@ export class LocalCredentialVault { #beforeGeneratePersistForTest = null; #afterGeneratePersistForTest = null; #afterLockPublishForTest = null; + #renameForTest = null; + #lockPlatformForTest = null; declare dir: string; declare paths: { key: string; @@ -1498,6 +1528,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) { @@ -2099,12 +2148,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 { @@ -2174,7 +2218,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); @@ -2286,7 +2330,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))) { @@ -2425,7 +2469,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 0fce708..a8acb6c 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. A sibling rename of the intact + // candidate distinguishes that vanished dest from a lasting ACL denial. const context = await fixture(); try { await saveLogin( @@ -1373,6 +1377,89 @@ 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("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;