oficina / home Public

Explicit claude-fable-5 code APPROVE, no blockers, frozen 74dda570e0cf7c68278e40b48de1007dc76c82cc. Full local gate, focused race and actual exact-SHA CI green. Three compiled mutations failed on intended blocking/contention behavior. Review/CI /tmp/oficina-roster-lease-review; mutations /tmp/oficina-roster-lease-mutations/results.json. Canonical publication follows these passed gates. Full review:

APPROVE

I reviewed 74dda57 against 667dc82, the approved design review, and the inline sources. The delta is exactly the approved scope — TryLock in both lease accessors, the explicit unknown-kid guard, and a test-only SQLite dependency — and nothing else. No blockers. Findings by required lens:

Fail-closed semantics — verified. Valid returns false and Remaining returns 0 on TryLock failure; contention can never answer true or a positive duration. Both refusal paths skip observe() entirely, so a contended call mutates no cache state (asserted by TestLeaseContentionRefusesWithoutChangingAgeOrStanding: snapshot, deadline, and age all unchanged after contention). The Remaining()==0 degradation routes to the existing expiry path, as the design review anticipated.

Unchanged checks on success — verified. On a successful acquisition both accessors run the identical body: observe() (so the backward-wall-clock fence executes on every path that can answer true), then the unchanged version/usable/deadline comparison. TestLeaseContentionDoesNotSkipLaterClockFence closes the one subtle gap: a contended call skips observe(), but because it also skips updating wall/mono, the rollback evidence survives to the next successful acquisition and fences there. This is equivalent to the old blocking code, which likewise only observed after acquiring the lock — no fence regression.

SQLite writer inversion proof — real, not simulated. The regression test builds the actual cycle the amendment exists to break: a genuine modernc.org/sqlite pool with SetMaxOpenConns(1), a transaction holding the single writer connection (the "caller in-transaction" leg), a refresh whose publish holds Cache.mu across Durable.Save, and — critically — the db.Stats().WaitCount poll proving Save is genuinely parked on the connection pool rather than merely scheduled. The entered handshake alone would not prove blocking; the WaitCount assertion does, and the comment says so. The lease accessors are then called by the goroutine that owns the writer, which is precisely the deadlock shape (permanent, or FetchTimeout-repeated, per design advisory 5). Post-conditions are right: contended lease refused, subsequent check obtains a fresh valid lease (retry path intact), old lease dead on the new version.

Cleanup and cancellation — sound. The writer transaction is rolled back on every exit path before any t.Fatal (WaitCount stall, mutation timeout, and the deferred rollback; double-rollback is ErrTxDone, ignored). refreshed is drained so the flight goroutine cannot leak; all accessor channels are buffered size 1, so goroutines blocked in a mutated (blocking) build complete once publish releases Cache.mu and cannot wedge the suite. Save's context is the flight's 5-second real-time FetchTimeout, and the test's worst-case critical section (~1.5s of polls and selects) sits well inside it.

Mutations — meaningful, compiled, all caught. blocking-valid and blocking-remaining restore the blocking Lock (the exact mutation the approved design review demanded must fail) and both exit 1 via the 250ms accessor timeouts while the writer is genuinely held. contention-grants kills the fail-closed direction and exits 1 against the contention tests. All three compiled; none is an assertion-free pass.

Unknown-kid guard — correct and in scope (design review advisory 6, requested in this same revision). The explicit key, known := keys[e.KeyID] lookup before canonical/Verify removes the dependence on family.Verify rejecting a nil key. All refusals stay ErrRefused with no error-shape distinction, so no oracle is introduced; the reordering only moves between refused paths on attacker-visible public inputs. TestUnknownSigningKeyRefused covers both callers (provisioning and page contexts) including the sharp case — a valid signature under a real key with an unprovisioned identifier.

Dependency — test-only as claimed. modernc.org/sqlite is imported solely from lease_test.go, so no consumer compiles it; it is cgo-free, consistent with the estate's CGO_ENABLED=0 posture, and the indirect block and go.sum entries are the expected modernc/x-tools closure. No production code path touches it.

Advisories (non-blocking):

1. The kid guard has no mutation proving it load-bearing — deleting it would still pass TestUnknownSigningKeyRefused if family.Verify rejects nil keys, which is exactly why the guard exists. Acceptable as tested-behavior-plus-documented-intent; if you want the guard itself pinned, a future mutation run can substitute a verifier stub that panics on a nil key.
2. Spurious refusals under benign micro-contention are now possible: two concurrent Valid calls, or Valid racing a fast Check, can yield a one-shot refusal for a member in good standing, where blocking Lock would have waited microseconds. This is the approved trade (retryable, never widening), but when the contention-rate canary from design advisory 7 lands, count lease TryLock refusals separately from writer-saturation 503s so the two signals aren't conflated.
3. Minor flake window in the writer test: on a pathologically stalled CI host, the ~1.5s critical section could brush the 5s real-time FetchTimeout, failing as refreshed returning ErrUnavailable rather than a lease assertion. Distinguishable from a real regression by the failure message; note it if it ever fires.
4. MVS visibility: modernc.org/sqlite appears as a direct requirement to every module that pins roster, even though no non-test package imports it. Expected Go behavior, worth one line wherever Docs' dependency audit lives.

The two absolutes hold: contention resolves to refuse-and-retry on every path, and no path that answers true skips the clock fence or the version/deadline checks. Per the stated sequencing, this stays unpublished until the exact-SHA CI run is green.