Remove a coordinated-retry park's lock wake callback when the park ends

[kriszyp]

⊙ Problem

A coordinatedRetry commit that loses a conflict parks on the holder's verification-table (VT) lock by registering a wake callback on that lock's LockTracker. LockTracker only had addWakeCallback() and wake(), so when a park ended any other way than the holder releasing (the bounded park timeout from fix(transaction): bound the coordinated-retry park with a descriptor-owned timeout, its worker exiting, or its database closing), its callback stayed registered until the holder eventually released. Behind a holder that never releases (Unsettled commit promise wedges all writes on a thread indefinitely, the case the timeout exists for), every re-park, about one per waiting transaction every 5 s, added another inert closure for as long as the lock was held. AGENTS.md invariant 12 recorded this as a known, deferred gap.

❓ Your call: Is this worth fixing now? As specified, yes: the growth is unbounded in holder lifetime in exactly the stuck-holder case the timeout was built for, and the fix is confined to the lock tracker and the park registry. The alternative is to keep it documented as a leak bounded by how long a holder stays stuck.

💡 Solution

LockTracker::addWakeCallback() now returns a move-only WakeRegistration; cancelling or destroying it removes the callback in O(1). Each park's ParkTimeoutRegistry entry owns its registration and cancels it as the park ends, so a lock's registered callbacks are exactly the parks still waiting on it. wake() behaves as before for live waiters: registration order, each once, invoked with no lock of its own held, and a registration after wake() returns empty so the caller resolves inline.

• The registration references a separately allocated WakeList (a shared_ptr plus a std::list iterator), never the tracker. The park drops its tracker reference right after registering, and dropping one takes the global writerMutex_ that wake() already runs under, so a tracker reference cannot be held across the park.
• The list is created by the first registration, not with the tracker: every transactional write installs a tracker under writerMutex_, and only contended locks get a list.
• wake() marks the list drained and runs the callbacks in place. It does not allocate, because a throw there (under writerMutex_, after a commit has landed) has no recovery. A cancel that loses the race to wake() leaves its node alone and the callback still runs, which is why the park's closure keeps its weak references and exactly-once gate.
• ParkTimeoutRegistry::resolve() cancels before calling the TSFN, so JavaScript that observes a park's result never sees it still registered. Env teardown and shutdown cancel by destroying the entry.
• A process-wide lockWakeCallbackCount() diagnostic (internal, not exported from index.ts) follows the transactionLogMapCount precedent.

⚖️ Alternatives

The planning review returned Framing-Verdict: better-alternative-exists; I adopted its alternative: a std::list with iterator tokens instead of the planned std::map keyed by registration id. It has O(1) removal under the registry mutex, the same one allocation per registration, and no id counter.

• Park keeps a counted tracker reference so removal by id is always safe: rejected. The park ends inside fire() on the real-wake path, which already holds writerMutex_, and unrefTracker() takes it again (self-deadlock); avoiding that needs a lock-free "never the last reference" decrement that breaks the VT rule that every tracker free is serialized by writerMutex_.
• Registry-side index keyed by tracker identity: rejected. It adds a process-global structure consulted under writerMutex_, and tracker addresses are reused, so identity needs the 14-bit generation, which wraps every 16K installs.
• Do less: prune dead callbacks in addWakeCallback(): rejected. It leaves the last batch registered on a lock nobody re-parks on (it fails "zero after N timed-out parks"), and scans every live waiter on each add.
• Do less: accept and document: rejected; the growth is unbounded in holder lifetime.

❓ Your call: A registration that fails to allocate resolves the park immediately with RETRY_NOW (the same path as a lock that was already released), spending one maxRetries attempt, instead of rejecting the commit. Either is a one-line change.

❓ Your call: The diagnostic is a binding-root lockWakeCallbackCount() like transactionLogMapCount, rather than a field in getRegistryStatus(). It is not part of the public index, so it can be moved later.

🔧 Changes

• src/binding/core/verification_table.h / .cpp: LockTracker gains WakeList (mutex, drained flag, std::list of callbacks) and the move-only WakeRegistration (list shared_ptr + iterator; move-assignment cancels the registration it replaces). addWakeCallback() creates the list on first use and returns a registration with the strong exception guarantee. cancel() erases its node unless the list is drained and reports whether it did. wake() moves the list out of the tracker, marks it drained, and runs the callbacks in place. A process-wide atomic counts registered callbacks.

❓ Your call: Registrations hold the list strongly, so callbacks wake() already ran stay allocated until every park on that lock lets go of its registration (immediately on the wake path; at most one park timeout otherwise). The alternative, weak ownership plus a liveness check before every iterator copy, frees them sooner but needs that check on each move. Easy to reverse.

• src/binding/database/db_descriptor.h / .cpp: each ParkTimeout entry owns its WakeRegistration, and the class comment now gives the remaining reason parks are keyed by id. resolve() cancels before calling the TSFN; releaseByEnv() and shutdown() cancel by destroying the entry. New attachWakeRegistration() (declaration) hands a registration to a park that is still pending, or cancels it if the park already ended.

⚠️ Look hardest: the window between schedule() and attachWakeRegistration(). The park is published first (so a wake in that window still finds it), and a park the timeout, a wake or shutdown ended in that window gets its late registration cancelled on return.

• src/binding/transaction/transaction.cpp: completeCommitWork() attaches the returned registration to its park, and falls back to the existing inline resolve when the lock was already released or registration throws. The comment on the closure's weak_ptr captures now gives the remaining reason they are needed (a callback wake() already claimed can outlive its park).
• src/binding/binding.cpp, src/load-binding.ts: the lockWakeCallbackCount() diagnostic (native function, export, TypeScript binding).
• AGENTS.md invariant 12 (known-gap paragraph replaced, fire(id)'s locks and the weak-capture reason updated), src/binding/core/DESIGN.md (new "Wake registrations" section, including the no-allocation rule for wake()) and the root DESIGN.md index line.

✅ Verification

Route: new child-process integration tests, plus native unit tests for the list itself.

• pnpm test:native: 319/319 before the merge. The LockTrackerWake suite cover cancel-then-wake, order and exactly-once delivery, wake with no waiters and repeated wake, cancel racing wake(), cancel after the tracker is freed, release of captured state on destruction, and move-assignment. The concurrent test repeated 30× clean; an instrumented run showed every round reaching all three outcomes (≈2000 cancelled before the wake, 15–46 claimed by it, ≈2000 added after it).
• test/lock-tracker.test.ts: 16/16 under ROCKSDB_JS_COMMIT_THREAD unset, 0, and 2. The four new scenarios each first observe a registration, then require zero while the holder still holds: timeout (3 parks against one held lock), wake (release well under the 5 s default timeout), worker-exit (Node only), foreign-close (a one-slot VT puts the park on another database's tracker; closing the waiting database).
• Fails without the fix: leaking the registration (the base behavior) makes timeout, worker-exit and foreign-close fail with 1 callback left registered; wake still passes, as it should.
• pnpm test (full, Node): 1139 passed, 10 skipped, 0 failed at 47781d1; after merging main (which brought in Make optimistic commit lock buckets and validation policy configurable and its concurrent commit threads), 1155 passed, 10 skipped, 0 failed, and pnpm test:native 323/323. pnpm check: clean.
• Not run here: Bun and Deno are not installed on this worker; CI covers them.

Test files: test/native/verification_table_test.cc (the LockTrackerWake suite, concurrent test), test/fixtures/fork-park-wake-registration.mts (timeout scenario, foreign-close scenario), test/workers/park-wake-registration-worker.mts (the parked worker), test/lib/park.ts (shared holder/park setup; it calls populateVersion first because the process-wide VT is created on the first version lookup and writes before that lock nothing), and test/lock-tracker.test.ts (runs the scenarios; the existing park-timeout test now shares its child-process helper).

🤖 Generated with Claude Code

https://claude.ai/code/session_01TCdH23J2tPQgQq4PrWGjEh

— Claude Opus 5.5

Related PRs: #897 independent (now in main and merged into this branch; it shares transaction.cpp and db_descriptor.*, not the park code), #842 independent, #767 independent, #900 independent, #902 independent (reached main through #897), #742 independent (already in base), #901 independent (already in base), #890 independent (already in base)
Complexity: complicated

_{Dispatch: task rocksdb-js-wake-callback-deregistration · queued by unknown · ran by claude/opus/xhigh · worker kzyp-xps-1}

_{Review-Coverage: authored=claude; ran=cursor-composer,gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-kimi,cursor-muse; rounds=3; full=1 @ 2b9befd}

_{Review-Attention: study ~15m (critical: transaction.cpp; decisions: do-less-alternative, strong-list-ownership, cancel-inside-resolve, diagnostic-surface) @ 2b9befd}

[gemini-code-assist]

Code Review

This pull request introduces a mechanism to cancel lock wake registrations when a coordinated-retry park ends due to timeout, environment exiting, or database closing. It adds a LockTracker::WakeRegistration class to manage the lifetime of wake callbacks, ensuring they are properly removed from the LockTracker's wake list when cancelled, preventing memory growth issues. Additionally, it introduces diagnostic functions and comprehensive tests to verify that registrations are cleaned up correctly under various scenarios. There are no review comments, so I have no feedback to provide.

[github-actions]

📊 Benchmark Results

get-sync.bench.ts

getSync() > random keys - small key size (100 records)

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	23.50K ops/sec	42.56	40.95	611.926	0.115	117,480
🥈 rocksdb	2	10.86K ops/sec	92.05	88.87	24,663.409	1.00	54,319

getSync() > sequential keys - small key size (100 records)

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	27.41K ops/sec	36.48	35.40	462.507	0.100	137,049
🥈 rocksdb	2	10.96K ops/sec	91.24	88.80	619.318	0.054	54,802

ranges.bench.ts

getRange() > small range (100 records, 50 range)

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	24.71K ops/sec	40.46	36.68	1,962.43	0.300	123,569
🥈 rocksdb	2	15.40K ops/sec	64.94	56.70	1,035.726	0.124	76,997

realistic-load.bench.ts

Realistic write load with workers > write variable records with transaction log

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	452.01 ops/sec	2,212.344	44.40	50,085.503	13.66	914
🥈 lmdb	2	27.84 ops/sec	35,920.538	460.736	1,120,401.397	136.338	64.00

transaction-log.bench.ts

Transaction log > read 100 iterators while write log with 100 byte records

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	35.25K ops/sec	28.36	12.96	20,754.689	0.844	176,275
🥈 lmdb	2	440.21 ops/sec	2,271.627	139.626	13,308.273	1.28	2,202

Transaction log > read one entry from random position from log with 1000 100 byte records

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	631.73K ops/sec	1.58	1.37	3,899.669	0.174	3,158,662
🥈 lmdb	2	470.88K ops/sec	2.12	1.16	791.963	0.281	2,354,417

worker-put-sync.bench.ts

putSync() > random keys - small key size (100 records, 10 workers)

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	832.80 ops/sec	1,200.771	1,025.097	1,939.552	0.366	1,666
🥈 lmdb	2	1.17 ops/sec	854,947.661	811,286.719	883,171.67	1.92	10.00

worker-transaction-log.bench.ts

Transaction log with workers > write log with 100 byte records

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	18.20K ops/sec	54.95	29.69	21,327.066	2.35	36,397
🥈 lmdb	2	838.81 ops/sec	1,192.16	186.745	12,639.523	5.18	1,680

---

Results from commit be1ab81

[cb1kenobi]

LGTM!