From c1655a25b352922a181271e32d865bd9a14be0ae Mon Sep 17 00:00:00 2001 From: CarmenDou <15951653662@163.com> Date: Wed, 7 Oct 2026 21:25:21 -0700 Subject: [PATCH 1/2] fix(lock): release cannot delete a lock taken over during it, and the contention test models abandonment as exit release() was check-then-act. It read the lock, saw its own token and unlinked the path. A takeover landing between that read and the unlink wrote the new holder's token into the same inode, and the old holder then deleted the new holder's lock, so a third process got in with wx while the new holder was still inside. release() now takes the same hardlink claim that breakStaleLock takes for that token before it checks and unlinks, so a release and a takeover of one token exclude each other. When the claim already exists a takeover is under way and the lock is left alone. Any other link error fails every takeover too, so the plain check still runs there. The claim name is derived in one helper used by both sides. The new unit test replays a takeover just before each fs call the release makes on the lock and asserts exactly one holder remains. It fails on the old release at the unlink step. The stale-lock contention test modelled abandonment as a live process sleeping with the lock held under a 300 ms age bound. On a loaded runner a live holder was starved past that bound and legitimately broken by age, which the test reported as two holders. It now passes staleMs Infinity as the file locks do, and a holder abandons by exiting with the lock held, in waves of 8 processes so each wave also opens on a dead holder. A starved live holder is never broken, and a dead holder is broken by every spinning contender at once. --- src/commands/compute.ts | 34 +++++++++--- test/ssh-orchestration.test.ts | 98 +++++++++++++++++++++------------- 2 files changed, 88 insertions(+), 44 deletions(-) diff --git a/src/commands/compute.ts b/src/commands/compute.ts index 2fc911b..353968f 100644 --- a/src/commands/compute.ts +++ b/src/commands/compute.ts @@ -1792,9 +1792,22 @@ export function acquireLockFile(path: string, now: number, staleMs: number): (() // ask whether the holder still exists. const token = `${process.pid}:${randomUUID()}` const release = () => { + // The claim a takeover of this token takes, so a release and a takeover never interleave. + const claim = takeoverClaim(path, token) + let claimed = false + try { + linkSync(path, claim) + claimed = true + } catch (e) { + // EEXIST is a takeover in progress, so the lock is no longer ours to remove. + if ((e as NodeJS.ErrnoException)?.code === 'EEXIST') return + // Any other error (gone, no hardlinks) fails every takeover too, so the check below is safe. + } try { if (readFileSync(path, 'utf8') === token) unlinkSync(path) - } catch { /* already gone, or taken over by someone else */ } + } catch { /* already gone, or taken over by someone else */ } finally { + if (claimed) try { unlinkSync(claim) } catch { /* the inode keeps its other name */ } + } } const take = (): (() => void) | undefined => { try { @@ -1925,21 +1938,21 @@ function processAlive(pid: number): boolean { * `path` keeps its inode throughout, so there is no moment at which the lock * at `path` can have become somebody else's between the check and the act: * the holder judged dead cannot release, every other breaker is behind the - * claim, and a release by anyone else checks for its own token first. Should - * the old holder's release ever run, it reads a foreign token and leaves the - * file alone. + * claim, and a release by anyone else checks for its own token first. + * The old holder's release takes this same claim, so it never lands inside a takeover. * * Residual, and deliberately not "fixed": a breaker killed between the link - * and its write leaves the claim behind, and that one token can then no - * longer be broken (removing the claim by hand is the way out, and the busy - * message names the lock). Every scheme for reaping an abandoned claim needs + * and its write, or a release killed before its unlink, leaves the claim + * behind, and that one token can then no longer be broken or released + * (removing the claim by hand is the way out, and the busy message names + * the lock). Every scheme for reaping an abandoned claim needs * to decide the claim is dead and then remove it -- the same check-then-act * this function exists to eliminate, one level up. A claim left behind AFTER * the write is inert: the lock now carries the breaker's token, and the next * breaker keys its claim on that. A wedged lock is recoverable and says so; * two writers in the same section silently lose the user's config. */ function breakStaleLock(path: string, stale: string, mine: string): boolean { - const claim = `${path}.stale.${createHash('sha256').update(stale).digest('hex').slice(0, 32)}` + const claim = takeoverClaim(path, stale) try { linkSync(path, claim) } catch { @@ -1966,6 +1979,11 @@ function breakStaleLock(path: string, stale: string, mine: string): boolean { } } +/** The hardlink that arbitrates breaking or releasing the lock `token` names. */ +function takeoverClaim(path: string, token: string): string { + return `${path}.stale.${createHash('sha256').update(token).digest('hex').slice(0, 32)}` +} + /** Side-effect seams, following the `TrackDeps` convention used by telemetry. * Present so the ORDER of operations is testable: minting before the collision * check, or advertising a command that cannot use the credential, is invisible diff --git a/test/ssh-orchestration.test.ts b/test/ssh-orchestration.test.ts index e0e5b9d..07aa000 100644 --- a/test/ssh-orchestration.test.ts +++ b/test/ssh-orchestration.test.ts @@ -7,7 +7,8 @@ // single piece -- only from running the steps together and watching what // happens, and in what order. import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll, vi } from 'vitest' -import { chmodSync, copyFileSync, existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, symlinkSync, unlinkSync, writeFileSync } from 'node:fs' +import fs, { chmodSync, copyFileSync, existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, symlinkSync, unlinkSync, writeFileSync } from 'node:fs' +import { syncBuiltinESMExports } from 'node:module' import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' import { execFileSync, spawn } from 'node:child_process' @@ -601,6 +602,40 @@ describe('the renewal lock survives a holder that outlives the staleness window' expect(c, 'the lock was never released').toBeTruthy() c!() }) + + it('a takeover landing inside the superseded holder release still leaves one holder', () => { + // B breaks A's aged lock just before the Nth fs call of A's release on the lock, one N per pass. + const lock = join(home, '.insta', 'ssh', 'api.insta.renew.lock') + const patched = fs as unknown as Record unknown> + const steps = ['linkSync', 'readFileSync', 'unlinkSync'] + let interrupted = 0 + for (let at = 1; ; at++) { + const a = acquireRenewalLock('api.insta')! + expect(a).toBeTruthy() + const real = Object.fromEntries(steps.map((s) => [s, patched[s]!])) + let calls = 0 + let inB = false + let b: (() => void) | undefined + for (const s of steps) { + patched[s] = (p: unknown, ...rest: unknown[]) => { + if (!inB && String(p) === lock && ++calls === at) { + inB = true + try { b = acquireRenewalLock('api.insta', Date.now() + 61_000) } finally { inB = false } + } + return real[s]!(p, ...rest) + } + } + syncBuiltinESMExports() + try { a() } finally { Object.assign(patched, real); syncBuiltinESMExports() } + const c = acquireRenewalLock('api.insta') + const holders = [b && 'B', c && 'C'].filter(Boolean) + b?.(); c?.() + if (calls < at) break + interrupted++ + expect(holders, `a takeover before release step ${at} left the wrong number of holders`).toHaveLength(1) + } + expect(interrupted, 'the takeover never landed inside the release').toBeGreaterThanOrEqual(2) + }) }) // Everything above either stubs the two install steps or asserts a REFUSAL. @@ -1142,22 +1177,17 @@ describe('simultaneous stale-lock recovery still admits one holder', () => { // in-process "concurrent" caller serialises itself. Only real processes // interleave. // - // Driven through acquireLockFile rather than acquireRenewalLock so the - // staleness window is milliseconds. With the real one-minute window the race - // is a few microseconds wide and happens once per run, which a spawned - // process lands in only by luck -- the first draft of this test passed - // against the defect. Here every contender ABANDONS the lock periodically, so - // the pack goes through simultaneous stale recovery dozens of times per run. - const STALE_MS = 300 + // staleMs is Infinity as for the file locks, so an exited holder is broken, a starved one never. const HOLD_MS = 10 - const ROUNDS = 15 + const ROUNDS = 5 + const WAVES = 3 + const SPIN_MS = 5_000 const CHILD = ` -const [, , mod, startAt, id, lock, probe, staleMs, holdMs, rounds] = process.argv +const [, , mod, startAt, id, lock, probe, holdMs, rounds, spinMs] = process.argv const { readFileSync, writeFileSync } = await import('node:fs') const { acquireLockFile } = await import(mod) const sleep = (ms) => new Promise((r) => setTimeout(r, ms)) -const STALE = Number(staleMs), HOLD = Number(holdMs) let held = 0 const overlaps = [] // A common start, so the contenders are on the lock together rather than one @@ -1169,27 +1199,21 @@ for (let i = 0; i < Number(rounds); i++) { // instead and they arrive one at a time, the first takes it cleanly and the // defect never gets its interleaving -- which is exactly how an earlier draft // of this test passed against the broken code. - let release = acquireLockFile(lock, Date.now(), STALE) - const until = Date.now() + STALE * 4 - while (!release && Date.now() < until) release = acquireLockFile(lock, Date.now(), STALE) - if (!release) { await sleep(1); continue } + let release = acquireLockFile(lock, Date.now(), Infinity) + const until = Date.now() + Number(spinMs) + while (!release && Date.now() < until) release = acquireLockFile(lock, Date.now(), Infinity) + // Gave up, so a wedged lock costs one wait and the parent's positive control reports it. + if (!release) break // A mutual-exclusion probe rather than a count: stamp a shared file, hold, // and read it back. A second holder admitted at any point during the section // overwrites the stamp, and it does not matter which of the two notices. - // HOLD is far shorter than STALE, so a holder is never itself stale and a - // takeover during the section is always a defect rather than the contract. writeFileSync(probe, id) - await sleep(HOLD) + await sleep(Number(holdMs)) const seen = readFileSync(probe, 'utf8') if (seen === id) held++ else overlaps.push(id + ' saw ' + seen) - // Every fifth acquisition is ABANDONED rather than released: the process that - // died holding the lock. This is what puts every other contender into stale - // recovery on the same file at the same moment, which is the interleaving - // under test. Waited out, so this process is well clear of the section before - // anyone is entitled to break in. - if (i % 5 === 4) await sleep(STALE + 50) - else release() + // The last acquisition is ABANDONED: this process exits holding it, and every spinner breaks in. + if (i < Number(rounds) - 1) release() } process.stdout.write(JSON.stringify({ held, overlaps })) ` @@ -1218,21 +1242,23 @@ process.stdout.write(JSON.stringify({ held, overlaps })) const lock = join(home, '.insta', 'ssh', 'contended.lock') const probe = join(home, 'probe.txt') const ids = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h'] - const startAt = Date.now() + 1_000 - const results = await Promise.all(ids.map((id) => run([ - '--import', 'tsx', script, compute, String(startAt), id, lock, probe, - String(STALE_MS), String(HOLD_MS), String(ROUNDS), - ]))) - for (const r of results) expect(r.code, `a contender failed: ${r.err}`).toBe(0) + const parsed: Array<{ held: number; overlaps: string[] }> = [] + // In waves, as a process abandons once. Each wave also opens on the last wave's dead holder. + for (let wave = 0; wave < WAVES; wave++) { + const startAt = Date.now() + 1_000 + const results = await Promise.all(ids.map((id) => run([ + '--import', 'tsx', script, compute, String(startAt), `${id}${wave}`, lock, probe, + String(HOLD_MS), String(ROUNDS), String(SPIN_MS), + ]))) + for (const r of results) expect(r.code, `a contender failed: ${r.err}`).toBe(0) + parsed.push(...results.map((r) => JSON.parse(r.out) as { held: number; overlaps: string[] })) + } - const parsed = results.map((r) => JSON.parse(r.out) as { held: number; overlaps: string[] }) expect(parsed.flatMap((p) => p.overlaps), 'two contenders were inside the lock at once').toEqual([]) - // The positive control. A takeover that never happens satisfies the line - // above trivially, and would wedge the lock for good after one crash: every - // round following the first abandonment would just return undefined. + // The positive control. With no takeover, nothing gets in after wave one's first abandonment. expect(parsed.reduce((n, p) => n + p.held, 0), - 'the abandoned lock was never broken at all').toBeGreaterThan(ids.length) + 'the abandoned lock was never broken at all').toBeGreaterThan(ids.length * (ROUNDS - 1) + 1) }, 60_000) }) From 00140fea86ed23ed9dfc1fd5387db563f5c76792 Mon Sep 17 00:00:00 2001 From: CarmenDou <15951653662@163.com> Date: Wed, 7 Oct 2026 21:39:31 -0700 Subject: [PATCH 2/2] fix(lock): release fails closed when it cannot take its claim From review. A transient link error such as ENOSPC fell back to the old read-then-unlink, which a takeover could still land inside. Release now leaves the lock unless the error says the disk has no hardlinks, where no takeover can claim either. --- src/commands/compute.ts | 9 ++++++--- test/ssh-orchestration.test.ts | 29 +++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/commands/compute.ts b/src/commands/compute.ts index 353968f..e2b69e0 100644 --- a/src/commands/compute.ts +++ b/src/commands/compute.ts @@ -1799,9 +1799,8 @@ export function acquireLockFile(path: string, now: number, staleMs: number): (() linkSync(path, claim) claimed = true } catch (e) { - // EEXIST is a takeover in progress, so the lock is no longer ours to remove. - if ((e as NodeJS.ErrnoException)?.code === 'EEXIST') return - // Any other error (gone, no hardlinks) fails every takeover too, so the check below is safe. + // Leave the lock unless the disk has no hardlinks, where no takeover can claim it either. + if (!LINK_UNSUPPORTED.has(String((e as NodeJS.ErrnoException)?.code))) return } try { if (readFileSync(path, 'utf8') === token) unlinkSync(path) @@ -1940,6 +1939,7 @@ function processAlive(pid: number): boolean { * the holder judged dead cannot release, every other breaker is behind the * claim, and a release by anyone else checks for its own token first. * The old holder's release takes this same claim, so it never lands inside a takeover. + * A release that cannot take it leaves the lock, to be broken like an abandoned one. * * Residual, and deliberately not "fixed": a breaker killed between the link * and its write, or a release killed before its unlink, leaves the claim @@ -1979,6 +1979,9 @@ function breakStaleLock(path: string, stale: string, mine: string): boolean { } } +/** link() errors from a disk without hardlinks, where a release may skip its claim. */ +const LINK_UNSUPPORTED = new Set(['EPERM', 'ENOTSUP', 'EOPNOTSUPP', 'ENOSYS']) + /** The hardlink that arbitrates breaking or releasing the lock `token` names. */ function takeoverClaim(path: string, token: string): string { return `${path}.stale.${createHash('sha256').update(token).digest('hex').slice(0, 32)}` diff --git a/test/ssh-orchestration.test.ts b/test/ssh-orchestration.test.ts index 07aa000..c33f639 100644 --- a/test/ssh-orchestration.test.ts +++ b/test/ssh-orchestration.test.ts @@ -636,6 +636,35 @@ describe('the renewal lock survives a holder that outlives the staleness window' } expect(interrupted, 'the takeover never landed inside the release').toBeGreaterThanOrEqual(2) }) + + it('a release that cannot take its claim leaves the lock, unless the disk has no hardlinks', () => { + // A transient link failure must not reopen the race, and a disk without hardlinks still releases. + const lock = join(home, '.insta', 'ssh', 'api.insta.renew.lock') + const patched = fs as unknown as Record unknown> + const realLink = patched.linkSync! + const releaseWithLinkError = (code: string) => { + const a = acquireRenewalLock('api.insta')! + expect(a).toBeTruthy() + patched.linkSync = (p: unknown, ...rest: unknown[]) => { + if (String(p) === lock) throw Object.assign(new Error(code), { code }) + return realLink(p, ...rest) + } + syncBuiltinESMExports() + try { a() } finally { patched.linkSync = realLink; syncBuiltinESMExports() } + } + + releaseWithLinkError('ENOSPC') + expect(existsSync(lock), 'a release without its claim removed the lock').toBe(true) + expect(acquireRenewalLock('api.insta'), 'the lock left behind was not held').toBeUndefined() + const b = acquireRenewalLock('api.insta', Date.now() + 61_000) + expect(b, 'the lock left behind could not be broken once stale').toBeTruthy() + b!() + + releaseWithLinkError('EPERM') + const c = acquireRenewalLock('api.insta') + expect(c, 'a disk without hardlinks never released').toBeTruthy() + c!() + }) }) // Everything above either stubs the two install steps or asserts a REFUSAL.