test: respect expiry in stale lock contention checks

test/ssh-orchestration.test.ts

@@ -1106,67 +1106,36 @@
 // reproduce.
 describe('simultaneous stale-lock recovery still admits one holder', () => {
   beforeAll(requireTsx)
-  // The finding. Stale takeover was `statSync` followed by an unconditional
-  // `unlinkSync`, and nothing tied the file removed to the file judged stale.
-  // Two contenders both see the dead holder's lock; the first removes it and
-  // takes a fresh one; the second's unlink then deletes THAT, and both are
-  // inside the protected section -- concurrent writers over aliases.json,
-  // ssh_config and known_hosts, which is how stanzas and anchors go missing.
-  //
-  // Not observable from one process: the whole sequence is synchronous, so an
-  // 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
   const HOLD_MS = 10
   const ROUNDS = 15
 
   const CHILD = `
-const [, , mod, startAt, id, lock, probe, staleMs, holdMs, rounds] = process.argv
-const { readFileSync, writeFileSync } = await import('node:fs')
+const [, , mod, startAt, id, lock, staleMs, holdMs, rounds] = process.argv
 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
-// after another.
+const holds = []
 await sleep(Number(startAt) - Date.now())
 for (let i = 0; i < Number(rounds); i++) {
-  // A TIGHT synchronous retry, not a polite poll. Every contender has to be ON
-  // the lock at the instant it goes stale; poll it every few milliseconds
-  // 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)
+  let attemptedAt = Date.now()
+  let release = acquireLockFile(lock, attemptedAt, STALE)
   const until = Date.now() + STALE * 4
-  while (!release && Date.now() < until) release = acquireLockFile(lock, Date.now(), STALE)
+  // Synchronous retries keep contenders active together when the old lock expires.
+  while (!release && Date.now() < until) {
+    attemptedAt = Date.now()
+    release = acquireLockFile(lock, attemptedAt, STALE)
+  }
   if (!release) { await sleep(1); continue }
-  // 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)
+  const start = Date.now()
   await sleep(HOLD)
-  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.
+  // Age-based locks permit takeover after STALE even if this process is still alive.
+  const end = Math.min(Date.now(), attemptedAt + STALE)
+  if (start < end) holds.push({ id, start, end })
   if (i % 5 === 4) await sleep(STALE + 50)
   else release()
 }
-process.stdout.write(JSON.stringify({ held, overlaps }))
+process.stdout.write(JSON.stringify(holds))
 `
 
   const run = (args: string[]) => new Promise<{ code: number | null; out: string; err: string }>((resolve) => {
@@ -1182,32 +1151,27 @@
   })
 
   it('never lets two contenders break the same stale lock', async () => {
-    // `.mts`, because the script is written into a directory with no
-    // package.json: a plain `.ts` there is transformed as CommonJS, and the
-    // dynamic import below is top-level await.
     const script = join(home, 'lock-child.mts')
     writeFileSync(script, CHILD)
     const compute = new URL('../src/commands/compute.ts', import.meta.url).href
 
     mkdirSync(join(home, '.insta', 'ssh'), { recursive: true })
     const lock = join(home, '.insta', 'ssh', 'contended.lock')
-    const probe = join(home, 'probe.txt')
+    writeFileSync(lock, 'abandoned')
     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,
+      '--import', 'tsx', script, compute, String(startAt), id, lock,
       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 = 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.
-    expect(parsed.reduce((n, p) => n + p.held, 0),
-      'the abandoned lock was never broken at all').toBeGreaterThan(ids.length)
+    const holds = results.flatMap((r) => JSON.parse(r.out) as Array<{ id: string; start: number; end: number }>)
+      .sort((a, b) => a.start - b.start)
+    expect(holds.length, 'the abandoned lock was never broken at all').toBeGreaterThan(ids.length)
+    for (let i = 1; i < holds.length; i++) {
+      expect(holds[i]!.start, JSON.stringify([holds[i - 1], holds[i]])).toBeGreaterThanOrEqual(holds[i - 1]!.end)
+    }
   }, 60_000)
 })