diff --git a/src/commands/compute.ts b/src/commands/compute.ts index 2fc911b..e2b69e0 100644 --- a/src/commands/compute.ts +++ b/src/commands/compute.ts @@ -1792,9 +1792,21 @@ 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) { + // 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) - } 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 +1937,22 @@ 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. + * 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 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,14 @@ 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)}` +} + /** 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..c33f639 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,69 @@ 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) + }) + + 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. @@ -1142,22 +1206,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 +1228,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 +1271,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) })