fix(core): fence lock renewal and release by generation

[iceteaSA]

Fixes #90.

Eviction was already fenced. Renewal and release were its unswept twins: both read the owner record, awaited, then acted on the lock by pathname. Branched from 9bf8f4c, independent of #87.

The three interleavings

1. Renewal — P1 confirms ownership with its lease still valid, stalls, the lease expires, P2 evicts and legitimately acquires, P1 resumes and writeOwner() lands unconditionally over P2's record. Two processes now believe they hold the lock.
2. Release — same shape, destructive. P1 confirms itself owner, stalls past expiry, P2 acquires, P1's queued rm removes P2's live lock and admits a third holder.
3. Renewal after release — a renewal that passes its check before release() can land its write afterwards, recreating an orphan lock nobody owns that blocks contenders until TTL.

All three violate the invariant the eviction fencing exists to hold: the filesystem offers atomic claim but no atomic conditional-delete, so the acceptable worst case is zero winners, never two.

Approach

Both paths now go through the existing eviction-marker primitive as a generation fence, and release awaits any in-flight renewal. ownerId remains the generation and the on-disk format is unchanged, so older builds and other processes reading the same file are unaffected.

The diff is large relative to the fix because the marker helpers were hoisted for reuse. Eviction semantics are untouched — the reviewer verified the four eviction fences survive 1-for-1 (old 236/240/243/249 → new 274/278/281/287, same checks, same order) and that src/tests/review-fixes.test.ts is unmodified.

Honest severity

Each interleaving needs the holder to stall past the 120s TTL between the check and the act — realistically severe event-loop starvation. Worth fixing because it breaks a stated invariant, not because it is likely to fire.

A testability seam, called out deliberately

Forcing the post-check/pre-write race required a new onStep('renewal-write-fenced') seam in production code. It is guarded (if (options.onStep)), so nothing is awaited when unhooked.

Review flagged that as a footgun rather than a bug: anyone later installing a hook there for telemetry would reopen the two-winner race. So the fence is now re-checked after the seam, immediately before the write — the invariant is structural rather than dependent on nobody hooking that point. A regression test pins that re-check specifically.

The extra ownsEvictionMarker() call is one more filesystem read per renewal (interval ≥1s). It is fail-closed: a transient read error aborts the renewal, the lock expires naturally, and the result is zero-winner-then-one-winner — the degradation this design already accepts.

Tests

1069 pass / 0 fail (1064 on 9bf8f4c), tsc clean.

An earlier revision's proof was invalid and was redone. It red-first'd against 9bf8f4c, where the tests hung at 1s because that commit lacks the seams — a missing-API red, not a behavioural one. The proof is now fault injection on the current fencing code, with each RED a concrete assertion about observed state:

fault injected	observed RED
renewal fence disabled	stalled renewal overwrote successor's ownerId
release fence disabled	successor's lock path absent after stale release
release/renewal serialization disabled	orphan lock present after release
post-seam re-check deleted	successor's ownerId overwritten

Plus 3,000 plain contended rounds electing exactly one owner. That stress seeds a stale lock only, so it exercises happy-path eviction under contention rather than the crashed-evictor branch — that branch stays covered by the existing unchanged tests.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Fences refresh-lock renewal and release by generation under the existing eviction marker and serializes release behind an in‑flight renewal. Previously both validated ownership then acted by pathname; a stalled holder could overwrite a successor or delete a live lock.

• Reuses the eviction marker via hoisted helpers (withEvictionMarker, ownsEvictionMarker, recoverStaleEvictionMarker); eviction semantics unchanged.
• Renewal: confirm current ownerId and that the lease is unexpired; re-check the marker before and after the renewal seam, then write. After the write, verify the marker again; if lost, relinquish only our own record to avoid two winners. Emits: renewal-owner-confirmed, renewal-write-fenced, renewal-write-ready, renewal-marker-unavailable, renewal-finished. Marker contention or transient errors reschedule.
• Release: cancel the timer, await any in‑flight renewal, confirm current ownerId, act only under the marker, and use bounded retries with stale-marker recovery; never delete by pathname without the marker. Emits: release-owner-confirmed.
• On-disk format and ownerId generation unchanged; no migration. One extra fs read per renewal; read failures fail closed.
• Tests cover renewal/release races, release-vs-renewal serialization, post-seam re-checks, marker contention and injected failures, post-write relinquish safety, stale-marker recovery, and a 512‑round single‑winner contention stress.

^{Written for commit 18798e1. Summary will update on new commits.}

[cubic-dev-ai]

All reported issues were addressed across 2 files

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

Re-trigger cubic</sub>

[iceteaSA]

Updated to f425bd4 (force-push; the previous head 2754324 is preserved on our side).

Cubic found two real bugs after this PR had already passed independent cross-family review, and one of them was a liveness regression this branch introduced — worth stating plainly rather than burying in a thread.

scheduleRenewal() was called inside the withEvictionMarker action body, and that body does not run when the marker cannot be acquired. So transient marker contention from any other process permanently ended the renewal chain: the lock stopped being renewed, expired at TTL, and was stolen — while the owner still believed it held it. Two owners, from a transient condition, in the code meant to prevent exactly that. The pre-fencing renewal path took no marker, so it did not have this failure mode.

The fix inverts the default rather than patching each path: continuation now happens unless we deliberately decided to stop. All ten exits of the renewal attempt were enumerated and classified — four stop, six continue — because the bug existed precisely because one exit silently skipped the continuation.

Also fixed: release() could delete nothing and return when the marker was held or stale, leaving the lock until TTL. It now recovers stale markers and retries, bounded at 8 attempts, with a deliberate documented give-up that prefers letting the lease expire over risking removal of a successor's lock.

Declined, with reasoning in-thread: an "atomic generation-conditional mutation" for the residual pre-write window. That primitive does not exist on a plain filesystem — it is the constraint this module is built around. The window is now bounded and documented in-source instead: two winners require both a writeOwner stall past EVICT_TTL (5s) and the lease expiring during that same stall.

Gate: 1072 pass / 0 fail (1064 on 9bf8f4c), tsc clean. Re-reviewed after the fix: APPROVE, 0 must / 0 should, with the exit enumeration independently reproduced and all three new tests fault-injected to concrete assertion failures.

[ualtinok]

Verified the bug and the direction; two things to resolve before this can merge.

The gap is real. release() does readOwner() → compare ownerId → rm(), with awaits between, and renewal has the same shape. That is read-check-then-delete on a lock the eviction path was carefully fenced against precisely because a filesystem gives no atomic conditional delete. Framing renewal and release as eviction's unswept twins is accurate.

1. The new stress test times out (blocking)

elects one owner across 3,000 plain stale-lock contentions fails on a clean checkout, in isolation, not under parallel load:

(fail) ... [5001.65ms]
  ^ this test timed out after 5000ms.

It is a timeout, not an invariant violation — 2238 of 3000 assertions passed before the cutoff, all single-winner. Raising the limit to 60s passes with all 3000:

(pass) ... [12890.26ms]   3000 expect() calls

So the fix holds; the test is simply sized past bun's default 5s. 12.9s of wall clock for one test is also worth a second look on its own — this repo already had a 3,000-round lock stress test that had to be cut to 512 rounds because it destabilized the release gate on a loaded machine. I would rather not reintroduce that shape at 2.5x the default timeout. Either drop the round count until it fits comfortably inside the default, or keep the count and state the raised timeout explicitly with a comment saying why the volume is load-bearing.

2. The open cubic P1 needs an answer, not a dismissal

When marker reclamation occurs after EVICT_TTL during a delayed renewal, this check can pass before theft, and the old owner then overwrites the successor's lock.

That is the same class as the bug being fixed, one level in: renewal re-checks ownership and then writes, and the window between those two steps is exactly what a generation fence has to close. If the generation is compared and then written non-atomically, the fence moves the race rather than removing it. Please either show the interleaving is impossible on this implementation or close it — it is the highest-severity finding on any PR in this batch, and this is the one file where "probably fine" is not good enough.

Everything else is green: 1071 pass with only that timeout failing, typecheck clean.

Worth noting for calibration: this batch's findings have again been valid but severity-ordered oddly. Read them as claims to test, not as a queue to burn down — that has been the pattern across #87 and now here.

[iceteaSA]

Both blockers addressed — 18798e1. 1074 pass / 0 fail, tsc clean, lock tests 12/12 in 227ms.

1. Stress test

Cut to 512 rounds — 118ms, well inside the default. Thank you for the history; we did not know about the earlier 3,000-round test that had to be cut for the same reason, and we reintroduced the identical shape.

The round count was inherited from a rule written when high-round stress was the only instrument available, and it false-greened on roughly 1-in-100 races. This branch now has deterministic seam-driven fault injection that forces each interleaving exactly, so those tests carry the proof and the stress test is an ordinary-contention smoke check. 512 rounds × 2 contenders still does 1024 acquires; a zero-or-two-winner regression fails in the first round.

2. The cubic P1 — closed, not bounded

You were right to reject the documented-residual answer, and the reason is worse than "we should have tried harder": this module already contained the pattern that closes it, and we did not apply it. The eviction path re-checks marker ownership after its claiming act and undoes it when the marker was stolen, turning a would-be two-winner outcome into zero winners. Renewal was not doing the same.

It does now. After writeOwner(), renewal re-checks ownsEvictionMarker(); on loss it stops rescheduling and relinquishes — deleting the lock only if the record is still ours, so a successor that already wrote its own record keeps it.

Every ordering around write / marker theft / successor write / relinquish:

ordering	outcome
No successor write before our relinquish read	0 winners — our record deleted, contender re-acquires
Successor wrote before our read	1 winner (the successor) — ownership check returns early
Successor writes between our read and our rm	0 winners — they retry
writeOwner stalls ≥ EVICT_TTL, lease expires, and 8 relinquish retries all fail on I/O	1 record on disk, not 2

No interleaving reaches two winners. The last row needs a ≥5s stall and persistent filesystem I/O failure, and even then leaves a single record.

Also worth flagging, since it is a behavioural change

ownsEvictionMarker() is fail-closed on any read error, so a transient EIO at the post-write check now triggers a prompt relinquish where the previous behaviour left the lock to expire at TTL. Both land inside the zero-or-one-winner guarantee, but they differ in timing, so it is now commented as a deliberate trade rather than left for a future reader to wonder about.

On the review that got us here

Our own cross-family reviewer approved this change and then, when asked specifically to attack the new code, found that the ownership condition inside the relinquish — the single line preventing us from deleting a healthy successor's lock — was not pinned by any test. It removed the condition and nothing reddened. Worst case without it is still zero winners, so it was not a correctness break, but it was exactly the kind of unguarded invariant that gets tidied away later by someone who cannot see why it is there. It is now pinned by a seam-driven test that pauses between the read and the delete, writes a successor's record, and asserts the lock survives; deleting the condition reddens it with Expected true, Received false.

Your calibration note matches ours, for what it is worth. These came from an automated sweep of 44 raw findings, and we filed only the three we had verified by hand — the aggregate severity ordering was not trustworthy (one HIGH required an operator to configure fallback on a success status, i.e. misconfiguration rather than a defect). Claims to test, not a queue to burn down.

[ualtinok]

Verified at 18798e1d. Both blockers closed. Approved.

Stress test

512 rounds, 118ms, comfortably inside the default. Your reasoning for why the count could drop is better than the fix itself: the high-round shape existed because stress was the only instrument, and it false-greened on roughly 1-in-100 races. Deterministic seam-driven injection carries the proof now, so the stress test can go back to being a smoke check. That is the right direction — a test that only catches a race 99 times out of 100 is a test that reports green while the bug ships.

The P1

Closed properly. Reusing the eviction path's own re-check-after-acting pattern is the answer I was hoping for rather than a documented residual, and your interleaving table covers the orderings I would have constructed.

I did not take the pinning claim on trust. Both guards mutation-tested individually:

removed	reddens
relinquish ownership condition (owner?.ownerId !== ownerId)	preserves a successor record during post-write relinquish
post-write ownsEvictionMarker() re-check	that test plus relinquishes the lock when its marker is stolen after the final renewal check

So the single line your reviewer found unpinned is now pinned, and the branch that reaches it is pinned separately. 1074 pass / 0 fail, tsc clean, 12/12 lock tests.

The unpinned-line story is the most valuable thing in this thread. Worst case without that condition was still zero winners, so no correctness break and no test would ever have caught it — but it is exactly the line a future reader deletes as redundant, and the interleaving it prevents would then be silent and rare. An invariant that is load-bearing only in an interleaving nobody reproduces is precisely what needs a test pinning it, and "the failure mode is benign" is the argument that gets such lines removed. Asking a reviewer to attack new code specifically, after it had already been approved, is what surfaced it.

The fail-closed timing change on ownsEvictionMarker() read errors is correctly flagged and correctly commented — same guarantee, different timing, and a future reader would otherwise have to derive that.

Merging.

[iceteaSA]

Thank you for mutation-testing both guards rather than taking the pinning claim on trust — and for testing the branch separately from the line. That distinction matters and we did not make it explicitly: the post-write re-check reddening two tests while the ownership condition reddens one is the evidence that the guard and the path to the guard are independently pinned. We had verified the line; you verified the route to it.

On the unpinned line, one correction to the credit: our reviewer had already approved this change before finding it. It surfaced only because the round after approval asked it to attack the code the fix itself introduced, on the grounds that a relinquish is new destructive machinery — code that deletes a lock where previously nothing happened. That reframing is what found it, not the reviewer being thorough on a second pass.

The general shape is worth stating plainly, since this thread is where it is best evidenced: our reviewers verify that a change is correct; they are much weaker at catching what a change makes worse elsewhere. This branch produced two instances. The fencing fix closed a real race and introduced a liveness regression — renewal died permanently on transient marker contention, two owners from a transient condition, in the file written to prevent exactly that. The relinquish then closed the P1 and introduced a new destructive path whose only safeguard was unpinned. Both were "correct fix, new failure mode", and neither was visible to the review that approved the fix on its own terms.

That is also why your "benign failure mode is the argument that gets such lines removed" framing landed. A test suite records what someone thought to assert; a line whose absence still yields an acceptable outcome is invisible to it forever, and the next reader has strictly less context than the one who wrote it.

For the record on the sweep that started this: 44 raw findings, 3 filed after hand-verification, 3 merged. The other ~38 stay unfiled until someone tests them individually — your read of the severity ordering matched ours independently, which is the main reason we are treating the remainder as claims rather than a backlog.