Temporarily take the derived-index runner lock without an unlock callback (rocksdb-js#848 workaround)

[kriszyp]

Temporary workaround — revert tracked in #2576 once a rocksdb-js release with #849 is pinned. The lock's own unlock callback is the right primitive; this PR sidesteps it only because the pinned rocksdb-js aborts Node 22 on it.

Fixes the Node 22 Aborted (core dumped) in Unit Test on main since #2430 merged (runs 34637871368, 34637971903): the process dies after vectorIndexPlane.test.js terminates a worker thread and the next test calls resetDatabases(). Node 24 and 26 pass.

Root cause — rocksdb-js, not Harper

Reproducible with a 30-line script and no Harper code: a worker calls db.tryLock(key, callback) while the main thread holds the lock, the worker is terminate()d, the main thread calls db.unlock(key) → SIGABRT on Node 22.23.1; Node 24.18.0 survives. DBDescriptor::lockReleaseByKey (db_descriptor.cpp:1185) calls the queued callback's thread-safe function for an env that no longer exists; Node 24 returns napi_closing (which the loop skips), Node 22 aborts. Lock callbacks have no per-env cleanup — the park-timeout registry does (releaseByEnv). The shared-buffer notify() listener path survives the same scenario on both versions.

The shared runtime made this deterministic: every non-owner worker keeps a standing tryLock waiter for each index (the old #2430 runtime only waited during a drain, which is why its Node 22 run passed). manageThreads.js uses worker.terminate() on restart, so this is a production path on Node 22 — and recordLock.ts (harper#483) uses the same tryLock(key, onUnlocked) primitive, so it is exposed independently of derived indexes.

The root-cause fix is HarperFast/rocksdb-js#849 (issue rocksdb-js#848): a dying env's queued lock callbacks are released from its cleanup hook, and the release paths hold locksMutex across the tsfn calls. Against that binary, main's runtime unchanged passes vectorIndexPlane.test.js 22/22 on Node 22 — so this PR is not required for correctness once rocksdb-js ships. It is still worth landing: a standing native waiter per worker per index is a resource the runtime does not need, and it unblocks main now without waiting on a rocksdb-js release and pin bump.

Fix

The runner takes the lock with no callback. A successor is woken by:

• the releasing owner's notify() on the readiness buffer — listeners survive a dead env, proven by the repro's buffer variant;
• lockRetryMilliseconds (5 s), for an owner that died without releasing — its lock is released natively and nothing notifies.

A readiness notification is acted on by an owner or a runner parked on the lock, and the releasing runner consumes the one it caused so an idle release does not re-acquire itself. That skip rests on a binding property, measured here against the pinned rocksdb-js 2.9.0: a notify() reaches the caller's own registration exactly once, and another thread's registration once. derivedIndexRuntimeNativeBackend.test.js now asserts that count against the real binding rather than leaving it to the unit-test fake.

A parked runner stays parked rather than re-probing the native lock on each commit wake, and a tryLock that throws replaces an armed contention retry instead of inheriting its deadline, so rebuildBackoffMilliseconds is the delay that actually applies. DESIGN.md § Derived-index runtime › Ownership and wake-up records the change and why.

Compatibility repair from the merged base

After this branch was opened, main merged #2561 (c90c61c3), whose eager allocation of an empty update payload caused the broad unit and integration failures now shown on the old PR head. This branch merges that exact tested base without rewriting history, then repairs the regression:

• Table.update() preserves the distinction between an absent update and an explicit empty patch. A lazy write binds its changes exactly once when save() selects its generation, so LMDB cannot rebind an earlier empty save to a later CRDT update at commit.
• Computed attributes reject assignment before saved-generation bookkeeping, restoring the established client error instead of exposing an internal receiver failure.
• Regression coverage proves an empty saved generation does not double-apply its successor and an untouched update of a missing row remains a no-op on both storage engines.

That regression is still live on main: its Unit Test workflow fails on Node 22, 24 and 26 at the tip (bba808e7, run 34657558850) with six test:unit:apitests failures — the four cache/query REST cases, assigning a computed attribute on a resource instance throws a client error, and can publish non-JSON. This PR's merge ref turns all six green, so it also unblocks main's CI.

For the human reviewer

Please merge rather than squash. The planned #2576 rollback must revert only the derived-index workaround and preserve compatibility commits 7eec8f7 and 68f7358; squashing would erase the boundary that makes that safe.

The wake path still has three explicit judgement calls:

• The skip flag rests on a measured binding property, not a contract. #skipNextNotify is correct only if a notify() self-delivers exactly once. That is now measured (probe against 2.9.0) and asserted in CI against the real binding, but it is not a documented rocksdb-js guarantee. The alternative — stamping the releasing owner's identity in the shared buffer so no runner has to guess whether an echo is its own — is correct under 0, 1 or N deliveries. It was not taken because Revert #2574 (notify-based runner-lock wake) once rocksdb-js with #849 is pinned #2576 deletes this path entirely; if that revert slips, the stamp is the durable form.

• A dead owner now costs up to 5 s of indexing lag, where the unlock callback picked it up immediately. lockRetryMilliseconds is an option, so the number is retunable, but the default is a behaviour change on worker restarts.

• Two review findings were declined. A floor on lockRetryMilliseconds / rebuildBackoffMilliseconds (a 0 would spin): neither option is reachable from user config — hnswDerivedIndex.ts passes only maxLagMilliseconds, which is the one already clamped. And wrapping wake(true) in the retry timer and in the #releasing continuation: both call sites are unchanged from origin/main, and the try/catch that was restored is on #notified() specifically because that one runs as the binding's thread-safe-function callback.

• The compatibility repair turns one silent record wipe into a ClientError. Dropping the #changes ??= Object.create(null) synthesis in Table.update() means a full update with an absent payload no longer invents an empty record. Measured on this head against a dist/ revert of those two lines: update(id, undefined, ctx) followed by a bare put() (and equally update(null, true) + save()) replaced a stored { id, status: 'kept' } with { id } before, and now aborts the transaction with A record must be an object, but received undefined. It is not reachable over REST — server/REST.ts:308 always passes an object target, so an empty-bodied PUT takes the legacy branch with a truthy #changes and is unchanged — so this is a programmatic-only path, and refusing the write is the safer outcome than erasing the record. The validate fallback narrowed with it (if (!recordUpdate) → a latch on recordUpdate === undefined); every other falsy payload — null from a replication patch apply, 0, '' — ends in the same no-op or the same ClientError on both sides of the diff.

The final review's two behavioral questions were also resolved without code changes, and both refutations were re-checked against source in a later adjudication pass:

• stop() cancelling the readiness registration before the release notifies. In the pinned rocksdb-js 2.9.0, getUserSharedBuffer builds the wrapper's two methods independently (dist/index.mjs:2011-2017): notify() is db.notify(encodeKey(key), args) — a database-level dispatch that consults the key's listener list at call time — while cancel() is only db.removeListener(encodeKey(key), options.callback). Cancelling this wrapper therefore removes this runner's own callback and nothing else, so #release()'s notify() still reaches every peer. Harper reaches that exact wrapper: readinessBuffer() calls RocksTransactionLogStore.getUserSharedBuffer, which delegates straight to the rocksdb-js store (resources/RocksTransactionLogStore.ts:614-615).
• The LMDB lock() → empty save() → set() drop. Unreachable: both Table.lock() entry points throw ClientError('Record locks are not supported on LMDB', 501) when the primary store is not a RocksDatabase (resources/Table.ts:2602 and resources/Table.ts:2628), and every #reloadLocked call site — the only thing that sets #lockWritable — sits behind one of them. On RocksDB the same sequence is unchanged by this PR: DatabaseTransaction.save() already validates inline (resources/DatabaseTransaction.ts:1199-1206), so the write was resolved against an undefined #changes before and after.

The remaining comment-only suggestions are left where they record cross-file invariants of this temporary workaround.

Verification

• Minimal repro (worker holds a derived-index registration → terminate() → resetDatabases()), Node 22: aborted before, survives after.
• CI has now confirmed the Node 22 fix on the exact head. Run 34666045755 checked out Merge 68f73588065b into bba808e749c4 (this head against main's current tip) and passed Unit Test on Node 22, 24 and 26. On Node 22 the chain reached test:unit:resources — the leg carrying vectorIndexPlane.test.js and the derived suites, and the one that used to abort — with 2491 passing, 33 pending, then test:unit:lmdb (1942 passing) and test:unit:bin (277 passing). Integration, Windows, Bun and uWS legs are green too.
• vectorIndexPlane.test.js + all derived suites on Node 22.23.1 locally: 102 passing, 1 pending (was: abort after "rebuilds after a whole-table snapshot reload marker"). Same on Node 24.18.0.
• Derived suites + vectorIndexPlane.test.js, Node 26.2.0: 105 passing, 1 pending. Full RocksDB resources: 2491 passing, 33 pending. Full LMDB resources: 1942 passing, 377 pending. API tests: 201 passing, 1 pending. Windows gate: 3736 passing across all nine groups.
• Real-server integration route: ACL + MQTT, 29 passing; invalid binary-root scan coverage, 6 passing. The full local integration run reached 2097 passing, 20 skipped, with only the optional Ollama suite cancelled by Node 26's JSON import-attribute behavior after the locally running service made that normally skipped path active; CI uses Node 24.
• npm run build, full Prettier, and lint on every changed file pass. Repository-wide lint still reports 15 pre-existing warnings outside the compatibility repair; the full aggregate unit command also exposes two environment-only baselines (dispatch-injected GIT_* variables and this worktree's 151-byte physical path). The isolated git-credentials test passes with those injected variables removed.
• Final exact-head review at 68f735880: Claude + Gemini + Harper domain, two full-review passes, independent coverage, Human-Review-Need 3.
• Each behaviour in the wake path was mutation-checked — the guard was removed in dist/ and the suite re-run — so none of these tests pass vacuously: the self-notify skip, the peer handoff, the idle-rotation guard, the dead-owner retry timer and the throwing-attempt backoff.
• The test dropped with the callback it tested (a synchronous unlock callback) is replaced by the five above; the two-runner test previously passed even when the releasing runner re-acquired the lock itself, and now requires the peer to mint an owner epoch.

Diff guide

• Design rationale
• Lazy generation capture
• Runner lock acquisition
• Computed setter guard
• Runner state-machine regressions
• Native notification contract
• Save-generation regressions

Refs #2430, #2567.

🤖 Generated with Claude Code

_{Review-Coverage: authored=codex; ran=gemini,claude; adjudicated=domain; declined=cursor-grok,cursor-composer; rounds=5; full=2 @ 68f7358}

_{Human-Review-Need: 3 (decisions: lock-handoff-workaround, lazy-generation-binding, temporary-retry-option, handoff-proof-boundary, computed-error-precedence) @ 68f7358}

[gemini-code-assist]

Code Review

This pull request refactors the locking mechanism in DerivedIndexRunner to avoid using an unlock callback, which can cause process aborts on Node 22 when workers are terminated. Instead, peers are woken up via notifications on the readiness buffer, commit wakes, and a fallback 5-second retry timer. The review feedback points out a potential issue where #skipNextNotify could remain true if the readiness buffer's notify method is undefined or throws an error, which would cause the runner to incorrectly skip the next legitimate notification. A code suggestion is provided to safely guard the notification call and reset the flag on failure.

[claude]

Reviewed; no blockers found.

[cb1kenobi]

This workaround stops using the native unlock callback that aborts Node 22 after a worker is terminated. Waiters now wake from the readiness notify, and a 5 second timer covers an owner that dies without notifying. The follow-up commits fix skip-arming, stop commit-wake polling, and ignore notifies on dormant ex-owners so idle systems do not rotate ownership. Nothing left on the changed lines is a confirmed blocker; earlier skip-flag and fake-coverage threads are already fixed.

—
Reviewed 37b46a2

[kriszyp]

CI note: the red Unit Test is main's, not this branch's — and it is hiding the leg this PR exists for.

The three failing Unit Test legs fail in test:unit:apitests with six failures that are identical on origin/main at c90c61c — same test names, same assertions (expected 27 to equal 22, expected undefined to equal 3, TypeError: Cannot read private member #writeGeneration…), same counts (5475 passing in test:unit:main, then 128 passing / 6 failing). main's Unit Test has been red since 927c474. #writeGeneration does not exist on this branch; it arrives through the merge ref from #2561.

The part that matters for this PR: test:unit:all is test:unit:main && test:unit:apitests && test:unit:resources && test:unit:lmdb. The apitests failure stops the chain, so test:unit:resources and test:unit:lmdb never run — on main or here. That is the leg carrying vectorIndexPlane.test.js and the derived-index suites, i.e. both the Node 22 abort this PR fixes and the code it changes. Neither is currently exercised by CI, and this PR cannot go green until the apitests breakage on main is fixed.

Locally at 37b46a2, under Node 26.2.0: test:unit:resources 2472 passing / 33 pending / 0 failing; derived suites + vectorIndexPlane.test.js 105 passing / 1 pending.

— Claude Opus 5

[cb1kenobi]

The runner now takes the lock without an unlock callback and wakes waiters from the readiness notify plus a retry timer. Earlier skip-flag, commit-wake polling, and idle-ownership-rotation issues are already fixed on this commit. The remaining judgement calls are documented rather than confirmed defects, and nothing on the changed lines is a blocker.

—
Reviewed 37b46a2

[cb1kenobi]

This PR stops passing an unlock callback to the derived-index runner lock, because a callback left behind by a terminated worker crashes Node 22 when another thread unlocks. Waiters now wake from the readiness-buffer notification plus a 5 second retry timer for an owner that died without releasing. The earlier skip-flag and fake-coverage findings were fixed in later commits, and nothing else on the changed lines is a confirmed blocker. The red CI legs are pre-existing failures on main, not this branch.

—
Reviewed 37b46a2

[cb1kenobi]

This PR stops passing an unlock callback to the derived-index runner lock, because a callback left behind by a terminated worker aborts Node 22 when another thread unlocks. Waiters now wake from the readiness-buffer notification plus a 5 second retry timer, and the merged-in compatibility repair keeps lazy updates bound to the generation that saved them. I traced the skip-flag interleavings, the acquire/backoff state machine, and the shared-memory test fake and found no blocking defect on the changed lines. Earlier skip-arming and fake-coverage threads were already fixed in later commits.

—
Reviewed 68f7358

[kriszyp]

Independent adjudication pass over all open review feedback at 68f73588. No code changes were warranted; every finding is ruled below with the evidence I checked, not inherited from the earlier replies.

Status of the review surface

All three inline threads are resolved and answered. The four most recent reviews (37b46a24 ×3, 68f73588 ×1) report no blocking defect on the changed lines, and the claude review bot reports no blockers at this head. So the only live material was the pre-push artifact's surviving findings, re-checked here against source.

Rulings

1. stop() cancels the readiness registration before #release() notifies peers — declined, refutation confirmed.

The earlier ruling rested on a claim about rocksdb-js internals, so I read the pinned binary rather than trusting it. In @harperfast/rocksdb-js@2.9.0, getUserSharedBuffer builds the two wrapper methods independently (dist/index.mjs:2011-2017):

buffer.notify = (...args) => this.db.notify(this.encodeKey(key), args);
buffer.cancel  = () => { if (options?.callback) this.db.removeListener(this.encodeKey(key), options.callback); };

notify() re-encodes the key and dispatches at the database level, consulting the key's listener list at call time; cancel() only removes this wrapper's own callback. So #readinessBuffer.cancel?.() at resources/derivedIndexRuntime.ts:632 cannot disable the peer notification at resources/derivedIndexRuntime.ts:1915 — it removes exactly the registration whose self-delivery unlock() deliberately does not arm a skip for (#skipNextNotify = !this.#stopped). Harper reaches that exact wrapper: readinessBuffer() → RocksTransactionLogStore.getUserSharedBuffer → rootStore.getUserSharedBuffer (resources/RocksTransactionLogStore.ts:614-615). The proposed "move cancel() into the #stopResult continuation" would be a no-op change. Ruling stands.

2. LMDB lock() → empty save() → set() silently drops the write — declined, unreachable.

Both Table.lock() entry points reject non-RocksDB stores before anything is staged: resources/Table.ts:2602 (static) and resources/Table.ts:2628 (instance) throw ClientError('Record locks are not supported on LMDB', 501) when !isRocksDB, and isRocksDB is primaryStore instanceof RocksDatabase (resources/Table.ts:616). #lockWritable is set only by #reloadLocked, whose five call sites (:2653, :2664, :2667, :2706, :2761) all sit inside those two guarded methods. There is no path to a lock-writable instance on LMDB, so the scenario cannot occur on the engine that defers validation to commit.

I also checked the RocksDB half, since "already dropped there" was the load-bearing part of the no-behaviour-change claim. It holds: DatabaseTransaction.save() validates inline (resources/DatabaseTransaction.ts:1199-1206), so before this PR validate ran during save() and bound recordUpdate = this.#changes while #changes was still undefined — exactly what captureChanges() now does a few microseconds earlier at resources/Table.ts:2256. Same outcome on both sides of the diff.

3. Added comments narrate rather than inform — declined, house-style judgement, unchanged.

The four the finding itself marked "worth keeping" are kept. Of the rest, the #notified() and #acquire() docblocks carry the two facts a reader cannot recover from the code — the measured self-delivery property that #skipNextNotify depends on, and why the lock is deliberately taken without its natural callback. Deleting either is how the guard gets "simplified" away before #2576 lands. Not worth a round of churn on a change with a tracked revert.

4. #skipNextNotify left standing by an absent or throwing notify (gemini + claude) — fixed, verified at head. resources/derivedIndexRuntime.ts:1911-1920: the flag is armed only inside the if (this.#readinessBuffer.notify) branch and cleared in the catch.

5. Handoff verified only by the fake (cb1kenobi) — half upheld, both halves landed. The delivery-contract half is refuted by finding 1's source reading; the coverage half was real and is now covered by behaviour, including the native-binding assertion at unitTests/resources/derivedIndexRuntimeNativeBackend.test.js:1941-1955, which ran and passed on Node 22 in CI.

One thing I checked that nobody raised

#notified() is a strict superset of main's readiness callback, which was () => { if (this.#owned) this.wake(true); } — owner-only. The !this.#owned && !this.#waitingForLock guard added in 37b46a2 restores that owner-only behaviour for a dormant runner and adds the parked case on top, so the idle-rotation fix cannot have narrowed any wake that main delivers. I also traced the #waitingForLock ⇒ armed-timer invariant and the paths that clear #lockRetryTimer: no state strands a runner that has work, and the worst observed cost of a mis-attributed notification is one extra tryLock probe or one lockRetryMilliseconds cadence, never a permanent stall.

CI note — supersedes my earlier comment on this thread

My 23:17 comment said the red Unit Test was main's and was hiding the leg this PR exists for. That is now resolved, and in the useful direction:

• This head is green. Run 34666045755 checked out Merge 68f73588065b into bba808e749c4 — this head against main's current tip — and passed Unit Test on Node 22, 24 and 26. On Node 22 the chain got past test:unit:apitests and reached test:unit:resources (2491 passing, 33 pending), the leg carrying vectorIndexPlane.test.js and the derived suites, and the one that used to abort. It did not abort. Every integration, Windows, Bun and uWS leg is green.
• main is still red. At bba808e7, run 34657558850 fails Unit Test on all three Node versions with the same six test:unit:apitests failures — including assigning a computed attribute on a resource instance throws a client error, which the resources/tracked.ts reorder in this branch fixes directly.

So this PR is both the Node 22 abort fix and the fix for main's currently-broken Unit Test workflow. Both facts are now in the PR description.

— Claude Opus 5

[cb1kenobi]

This PR stops passing an unlock callback to the derived-index runner lock, because a callback left behind by a terminated worker aborts Node 22 when another thread unlocks. Waiters now wake from the readiness-buffer notification plus a 5 second retry timer, and the merged compatibility repair keeps a lazy update bound to the generation that saved it. I traced the acquire/park/backoff state machine, the skip-flag interleavings, the lazy-capture latch, and the test fake, and found no blocking defect on the changed lines. The earlier skip-arming and fake-coverage threads are fixed at this head, and the three remaining concerns were already raised and adjudicated in the PR.

—
Reviewed 68f7358

[kriszyp]

Independent second adjudication at 68f73588 — no code changes warranted. I re-derived each of the three surviving findings from source rather than inheriting the earlier ruling; two of them I could close harder than the previous pass did.

1. stop() cancels the readiness registration before #release() notifies — declined, refutation confirmed

Read directly in the pinned @harperfast/rocksdb-js@2.9.0 (dist/index.mjs:2007-2018): the wrapper's two methods are built independently —

buffer.notify = (...args) => this.db.notify(this.encodeKey(key), args);
buffer.cancel = () => { if (options?.callback) this.db.removeListener(this.encodeKey(key), options.callback); };

notify() is a database-level dispatch that re-encodes the key and consults its listener list at call time; cancel() removes only this wrapper's callback. So a notify() after cancel() is neither a no-op nor "something worse" for peers — it still reaches every surviving registration. The finding's second sub-question (5 s of added lag per rolling worker restart) therefore does not arise. Moving cancel() into the #stopResult continuation would be a behaviour-neutral change.

2. LMDB lock() → empty save() → set() drop — declined, unreachable on both engines

The earlier reply closed the LMDB half. I also closed the half it left implicit — that a lock-writable write cannot reach a deferred-validate transaction:

• LMDB: both lock() entry points reject before anything is staged. resources/Table.ts:2601 (static) and resources/Table.ts:2627 (instance) are the first statement of each method: if (!isRocksDB) throw new ClientError('Record locks are not supported on LMDB', 501). Every #reloadLocked call site — the only thing that sets #lockWritable — is at :2653, :2664, :2667, :2706, :2761, all inside those two guarded methods.
• RocksDB: the transaction class is picked from the same flag (resources/Table.ts:6712-6716, :6753), and ImmediateTransaction there is the DatabaseTransaction one (resources/Table.ts:38), not LMDBTransaction's (imported as ImmediateLMDBTransaction, resources/Table.ts:123). Only LMDBTransaction.save() is the no-op (resources/LMDBTransaction.ts:368). So a lock-writable instance is always in a transaction that validates inline at resources/DatabaseTransaction.ts:1201, before and after this diff.

The engine flag itself is one global (resources/databases.ts:1901), so there is no mixed configuration that pairs a RocksDB-backed table with an LMDB transaction. Nothing to pin as an expectation, because the sequence has no reachable form.

3. Comment nit — declined, unchanged

House-style judgement on a change with a tracked revert in #2576. Agreed with leaving it.

Also re-checked, since these carry the fix

• The declined floor on lockRetryMilliseconds / rebuildBackoffMilliseconds. Confirmed unreachable from user config rather than taken on assertion: runtime.register(...) has exactly one production call site (resources/indexes/hnswDerivedIndex.ts:304), and its options is a literal carrying only maxLagMilliseconds (:317) — no spread of attribute.indexed. maxLagMilliseconds is the one clamped by Math.max(0, …). A 0 spin is not reachable.
• #waitingForLock ⇒ armed timer. The flag is set at exactly one place (resources/derivedIndexRuntime.ts:841) immediately followed by #armLockRetry, which either reuses an armed timer or arms one. #notified() clears the flag without clearing the timer, so every early return inside wake() still recovers within one cadence. No state parks a runner with no wake left.
• The test fake's fidelity. structuredClone(SharedArrayBuffer) returns another handle over the same memory (verified on this Node), so unitTests/resources/derivedIndexRuntime.test.js:57 really does model one allocation wrapped per lookup, and the exactly-once self-delivery premise is gated against the real binding at unitTests/resources/derivedIndexRuntimeNativeBackend.test.js:1941-1955 — which passed on Node 22 in run 34666045755.

No open thread, no new finding, and every check on this head is green. BLOCKED is REVIEW_REQUIRED — it is waiting on a human approval, not on a failing gate.

— Claude Opus 5

[cb1kenobi]

The derived-index runner now takes the lock without an unlock callback and wakes waiters from the readiness notify plus a retry timer. The skip-flag, park-on-timer, and idle-rotation paths match the tests and earlier adjudication, and the lazy save-generation latch keeps an empty write from picking up a later update. Nothing on the changed lines is a confirmed blocker.

—
Reviewed 68f7358

[kriszyp]

Third adjudication at 68f73588 — no open feedback, no code change warranted. All three inline threads are resolved, the claude review bot and the six most recent reviews report no blocker on the changed lines, and every check on this head is green. I re-derived the three surviving rulings from source rather than inheriting them; they hold:

• rocksdb-js cancel() vs notify() — read in the pinned @harperfast/rocksdb-js@2.9.0 in this worktree's node_modules (dist/index.mjs:2011-2017): notify is this.db.notify(this.encodeKey(key), args), a database-level dispatch that consults the key's listener list at call time; cancel is only this.db.removeListener(this.encodeKey(key), options.callback). stop()'s cancel cannot disable the peer notification.
• LMDB lock() guard — resources/Table.ts:2602 and :2628 are the first statement of each entry point, and every #reloadLocked call site sits behind one of them.
• No user-reachable 0 for the retry options — resources/indexes/hnswDerivedIndex.ts:317 passes only maxLagMilliseconds, which is the clamped one.

I also traced two things nobody had: #skipNextNotify can never be armed while the runner owns the lock (it is set only inside #release(), after #owned = false at resources/derivedIndexRuntime.ts:1892), and every path back to ownership runs #takeSharedRebuildRequest() in #acquired() — so a stale skip cannot swallow a rebuild request for an owner.

One new finding, now recorded in the PR description

The compatibility repair's Table.update() change has a behavioral consequence neither earlier pass covered. Dropping the #changes ??= Object.create(null) synthesis means a full update with an absent payload no longer invents an empty record. Measured here by reverting those lines in dist/ and re-running the same probe:

update(id, undefined, ctx) then a bare put(), over a stored { id, status: 'kept' }	result
pre-PR lines	record silently replaced with { id } — status erased, no error
this head	transaction aborts with A record must be an object, but received undefined

update(null, true) + save() behaves identically. It is not reachable over REST — server/REST.ts:308 always passes an object target, so an empty-bodied PUT takes the legacy branch with a truthy #changes and is unchanged — so this is a programmatic-only path, and refusing the write is the safer outcome than erasing the record.

The validate fallback narrowed with it (if (!recordUpdate) → a latch on recordUpdate === undefined). I traced every other falsy payload that reaches it — null from a replication patch apply (resources/Table.ts:960, resources/replayLogs.ts:287), 0, '' — and each ends in the same no-op or the same ClientError on both sides of the diff.

No code change made: the new behaviour is the safer one and the path is not reachable from a request. It is written up under For the human reviewer so the decision is visible at promotion time.

The description footers were left untouched — the head sha is unchanged, so the existing review receipt and Human-Review-Need: 3 still describe it.

— Claude Opus 5

[cb1kenobi]

The derived-index runner now takes the lock without an unlock callback and wakes successors from the readiness notify plus a retry timer, working around a Node 22 abort in the pinned rocksdb-js. I walked the acquire/park/backoff state machine, the skip-flag interleavings, and the lazy save-generation latch in Table.update, and found no state that strands a runner with work or drops a saved change. The earlier skip-arming and fake-coverage threads are fixed at this head, and the remaining concerns are already raised and adjudicated in the PR. Nothing on the changed lines is a confirmed blocker.

—
Reviewed 68f7358