fix(lock): release cannot delete a lock taken over during it

[CarmenDou]

What

The Linux test job has failed on main since 5cdb4bc in "simultaneous stale-lock recovery still admits one holder" ("two contenders were inside the lock at once"), 24 of 104 Linux attempts since 10-01. Two causes, both fixed here.

• A real race in the lock. release() read the lock, then unlinked it. A takeover landing between the two writes the new holder's token into the same inode, so the old holder unlinked the NEW holder's lock and a third process got in with wx. In production only the renewal lock can hit it (a live holder broken by its 60 s age rule), so the cost was a possible duplicate renewal.
• The test's timing assumption. It modelled abandonment as a live process sleeping while holding, with a 300 ms stale bound, and assumed a 10 ms hold never reaches 300 ms. On a loaded runner a starved live holder passes 300 ms and is legitimately broken by age, which the test then reports as two holders.

How

• src/commands/compute.ts release (:1794): takes the same hardlink claim breakStaleLock uses as its arbiter before it checks and unlinks. When the claim cannot be taken (EEXIST is a takeover in progress, ENOENT means the lock is gone, ENOSPC or EIO says nothing about a concurrent takeover), release fails closed and leaves the file, to be broken like an abandoned lock. The one exception is LINK_UNSUPPORTED (EPERM, ENOTSUP, EOPNOTSUPP, ENOSYS), a disk without hardlinks, where no takeover can claim either, so release keeps the old check rather than wedging every lock there. The claim is removed in finally.
• takeoverClaim (:1986) is now the one place the claim name is built, used by both.
• The breakStaleLock docblock: the old line that said a late release "reads a foreign token" is replaced, and the residual paragraph now also covers a release killed between its link and its unlink (that token's lock wedges until removed by hand, as with a breaker killed mid-takeover).
• test/ssh-orchestration.test.ts:
  • New deterministic test (:606): B breaks A's aged renewal lock just before each fs call of A's release in turn, then C tries to acquire. Exactly one of B and C may hold.
  • New test (:640): a release whose claim link fails with ENOSPC leaves the lock, which stays held and is broken once stale. With EPERM (no hardlinks) the release still frees it.
  • The contention test (:1262) uses staleMs = Infinity as the file locks do, and abandons by the holder process exiting while it holds. 3 waves of 8 processes, each child abandons its last acquisition. A live holder is never broken however long it is starved, and every exit puts all spinners into breaking the same dead lock at once.

Verify

• RED: on the old release, the takeover-inside-release test fails with "a takeover before release step 2 left the wrong number of holders: expected [ 'B', 'C' ] to have a length of 1".
• Under load (Node 22.22.1, 72 busy-loop processes on a 12-core Mac): the rewritten test passed 30 of 30. The old test against the same load failed 1 of 10, so the load reproduces the original failure.
• RED for the claim failure: before the fail-closed change the ENOSPC test failed with "a release without its claim removed the lock". With LINK_UNSUPPORTED emptied it fails with "a disk without hardlinks never released".
• Mutations, each alone: the old check-then-act release fails only the takeover-inside-release test. The claim removed from breakStaleLock (takeover still writes) fails the takeover-inside-release test every time and the contention test in 4 of 10 idle runs, so it still exercises simultaneous breaking.
• npm run typecheck and npm test pass on Node 22.22.1 (102 files, 2107 tests).

Not changed: the contention test no longer covers breaking a live holder by age under contention. The deterministic tests at :586 and :606 and the slow renewal holder test cover that. The test now spawns 24 processes instead of 8 (about 5 s idle, 10 s under heavy load, 60 s timeout).

#345 rewrites the same test without touching the lock, and its own test job is red on the release race. This change unblocks #358.

🤖 Generated with Claude Code

[cubic-dev-ai]

All reported issues were addressed across 2 files

<sub>Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>

[jwfing]

Summary

The normal release/takeover race is addressed, but the fallback after a hardlink failure can reintroduce the same race.

Requirements context

I assessed the change against the PR description, the existing lock invariants and tests, and the context in linked PRs #345 and #358. No separate lock design document was found; the intended contract is that release must never unlink a lock that a concurrent stale takeover has acquired, while the contention test must not treat a scheduler-starved live holder as abandoned.

Findings

Critical

• src/commands/compute.ts:1798-1807 — Only EEXIST fails closed. Every other linkSync error falls through to the original read-then-unlink sequence, but an error such as ENOSPC, EDQUOT, or a transient I/O failure does not prove a concurrent takeover will also fail later. An interleaving can therefore be: A's claim creation fails; A reads its token; filesystem conditions recover; B creates the claim and writes B's token into the inode; A unlinks B's lock. This is the exact race the PR intends to eliminate. Release should retry or fail closed whenever it cannot establish the claim, and the deterministic test should cover this error path.

Suggestion

(none)

Information

• test/ssh-orchestration.test.ts:606-637 — The deterministic test otherwise exercises takeover at each normal filesystem step and is a strong regression test for the successful-claim path.
• test/ssh-orchestration.test.ts:1167-1262 — Using process exit with staleMs = Infinity removes the prior scheduler-dependent age assumption while retaining a positive stale-takeover control.
• src/commands/compute.ts:1794-1985 — No new untrusted-input, authentication, secret-logging, dependency, or other security concern was found. Hashing the token also keeps it out of the claim filename.
• src/commands/compute.ts:1794-1810 — The additional synchronous hash, hardlink, and unlink occur only during local lock release; no material production performance concern was found. The contention test remains bounded by its timeout.
• Repository dependencies were unavailable locally (tsc was not installed), so I could not rerun the suite in this checkout. The head commit's Linux and Windows test checks are both green.

Verdict

Request changes: fail closed when the release cannot acquire its arbitration claim before merging.

[jwfing]

Summary

The change correctly serializes release and stale takeover using the same token-derived hardlink claim, preventing an old holder from deleting a replacement lock.

Requirements context

I assessed the change against the PR description and the lock protocol documented in src/commands/compute.ts:1775-1785 and src/commands/compute.ts:1916-1953. No separate lock specification was found; the linked PRs provide integration context, but the required behavior is defined primarily by the PR description and these in-code invariants.

Findings

Critical

(none)

Suggestion

• test/ssh-orchestration.test.ts:609-629, test/ssh-orchestration.test.ts:643-653 — The deterministic tests mutate the process-global node:fs exports and synchronize them into named ESM imports. This is restored safely with finally, but it conflicts with the repository convention to inject side-effect dependencies rather than mock globals (.claude/skills/developing-insta-cli/SKILL.md:17-19) and could become fragile if these tests are later made concurrent. Consider exposing a narrow filesystem dependency seam for the lock implementation.

Information

• src/commands/compute.ts:1794-1809, src/commands/compute.ts:1954-1987 — Functionally, release and takeover now contend on the same claim. A failed claim ordinarily leaves the lock intact, while the explicitly unsupported-hardlink fallback preserves release behavior on filesystems where takeover cannot use the claim either.
• test/ssh-orchestration.test.ts:606-667, test/ssh-orchestration.test.ts:1196-1291 — Coverage exercises takeover at each release step, fail-closed and unsupported-link errors, and simultaneous recovery from genuinely exited holders. The positive control also prevents a permanently wedged implementation from passing trivially.
• src/commands/compute.ts:1793-1809, src/commands/compute.ts:1982-1987 — No security-relevant regression found: token material is hashed in the claim filename, no secrets are logged, and no authentication, network, dependency, or command-execution surface changes.
• src/commands/compute.ts:1794-1809 — No material production performance concern found. Release adds one hash calculation and a bounded pair of hardlink operations on an already synchronous, local lock path; there are no new unbounded production loops or allocations.
• Validation was static because the review was explicitly read-only; I did not run the test suite, which writes temporary files. git diff --check origin/main...HEAD was clean.

Verdict

Approved under the supplied rubric: zero Critical findings. The dependency-injection point is non-blocking.

[jwfing]

LGTM - approved.