fix(transaction): bound the coordinated-retry park with a descriptor-owned timeout

[kriszyp]

Summary

Fixes #741 by bounding every coordinated-retry park. A commit blocked behind a holder that never releases now resolves RETRY_NOW after ROCKSDB_JS_PARK_TIMEOUT_MS (default 5000 ms) instead of leaving the write path hung indefinitely.

• Adds a descriptor-owned timeout registry with monotonic deadline ordering and a lazily started thread, joined during descriptor close.
• Keeps the VT wake callback off DBDescriptor and DBRegistry: it captures only weak timeout-registry and exactly-once state, avoiding registry re-entry and descriptor reference-count perturbation while the process-global VT writer mutex is held (wake path).
• Cancels timeout TSFNs during worker-environment cleanup while snapshotting only independently owned registries, so cleanup cannot make a concurrent descriptor purge skip (env cleanup).
• Handles timeout-thread and N-API/TSFN setup failures with a bounded immediate retry or terminal promise rejection; no setup failure can silently leave the commit promise pending (failure path).
• Reads ROCKSDB_JS_PARK_TIMEOUT_MS once per process behind a function-local static, matching commitThreadMode()/commitDelayMs() in the same file: a park runs on whichever env's JS thread owns the transaction, and ::getenv is not safe against a process.env write on another thread (parkTimeoutMs()).
• Guards both TSFN resolution paths (ParkTimeoutRegistry::resolve and the no-timeout-thread degrade) on napi_call_threadsafe_function's status before releasing: a napi_closing result means the tsfn may already be mid-teardown on Node's side, so releasing it again is a use-after-close (ParkTimeoutRegistry::resolve) — found by the pre-push review during this rebase, not present in earlier rounds.
• Measures every elapsed-time assertion on the monotonic clock, including the conflict-wake bound in the concurrent-transactions test, which was still reading Date.now() on both ends (lock-tracker.test.ts) — an NTP step between the two reads was enough to push it past the 3000 ms bound with nothing wrong in the wake path.
• Documents the bound where the behavior is specified. The README described the coordinated-retry wait as lasting "until the conflicting transaction has committed and released its write intent", which this change makes untrue; it now states the timeout, that a timeout consumes a retry attempt, the resulting ERR_TRANSACTION_ABANDONED at roughly maxRetries x the timeout, and that 0/negative/malformed values select the 5000 ms default rather than disabling the bound (README retry section).
• Drops the exactly-once gate on the no-timeout-thread degrade. Every path that returns 0 from ParkTimeoutRegistry::schedule does so before the entry publishes fired, and the fireOnce wake closure is not built until after this branch breaks out, so that side owns the park outright and the compare-exchange could never fail (degrade path).
• Raises the fork fixture's parent-side kill timer from 4 s to 20 s. The child's own elapsed assertion is the deadline check; the parent's timer additionally covers node boot, type-stripping, addon load and the seed writes, so at the same 4 s budget a slow runner and a real regression both surfaced as SIGTERM with the fixture's diagnostic never thrown. 20 s stays under vitest's 30 s testTimeout so the kill path still dumps the child's stderr (backstop).
• Adds deterministic timeout and close-lifecycle regressions, including an isolated process that proves the test fails against origin/main (tests).

For the human reviewer

The implementation deliberately preserves the task's policy choices:

• A timeout consumes one of the finite coordinated-retry attempts. A legitimately slow holder can therefore fail after roughly maxRetries × timeout; operators can raise the timeout but cannot disable the bound with 0. This is now stated in the README rather than only in the code, so it is a documented contract from here on.
• The knob is process-wide (env var, read once at the first park) and the implementation uses one lazy timeout thread per descriptor. Moving this to a per-database open option or a process-global timer would be a separate API/architecture change.
• LockTracker has no callback-removal API. Timed-out callbacks become inert immediately, but registrations remain until the holder wakes; adding cancellation is deferred because it changes the VT concurrency surface. Note the shape of the residual: registrations now accumulate per retry rather than per incident, so a long-lived abandoned holder under heavy contention grows one tracker's callback vector for as long as the incident lasts.

Declined, with reasons — each is a real observation, none is required to make the fix correct:

• Park timeouts are operationally silent. A synthetic wake is indistinguishable from a real one, so an exhausted retry reports the same generic Transaction did not commit after N coordinated retries a hot key produces. A transaction:parkTimeout global event (the transactionLog:warning shape) would make the abandoned-holder incident diagnosable — new API surface, so it belongs in its own change. Partially mitigated this round on the configuration half: a misconfigured ROCKSDB_JS_PARK_TIMEOUT_MS is still silent at runtime, but the README now says which values are ignored, so an operator who sets 0 can find out why it had no effect.

• db.transaction() orchestration is not covered end-to-end. test/fixtures/fork-park-timeout.mts drives Transaction.commit() directly and asserts the native RETRY_NOW. I traced src/database.ts:1014-1027 and the loop does terminate correctly, but nothing pins it.

• A parked commit does not hold the event loop (napi_unref_threadsafe_function, pre-existing — the fixture's setInterval keep-alive exists for this). Ref-ing for the park's duration is only safe now that the wait is bounded, but it is a behavior change beyond this fix.

• Comment density in db_descriptor.cpp. All three lenses flag the inline narration around ParkTimeoutRegistry (// Fire while still holding the mutex, // Notify + join outside the lock) as saying what the next line says. I trimmed only the two blocks this round touched; rewriting comments from earlier rounds under a nit that has been adjudicated as a nit five times running seemed like the wrong kind of churn for a review-feedback pass. The two blocks that do carry invariants — AGENTS.md note 12 and the ParkTimeoutRegistry class header — are the reason the Async DB ops (backup, createCheckpoint) defer registry purge when closed before settling #672 purge-skip does not come back, and the domain lens explicitly asked that they stay.

• fire() takes the park registry's mutex while the global VT writerMutex_ is held, so a wake can wait behind runLoop()/shutdown() draining a batch of due parks. Everything done under that mutex is non-blocking — napi_call_threadsafe_function is napi_tsfn_nonblocking, and shutdown()'s join() is deliberately outside the lock — and N is bounded by one descriptor's concurrent coordinated-retry commits. The suggested try_lock would silently drop a real early wake and fall back to the full timeout, which is worse than the latency it avoids.

• The first ::getenv in parkTimeoutMs() can still race a process.env write. True, and exactly the exposure commitThreadMode()/commitDelayMs() already carry three lines below — the previous round asked for this shape specifically. Closing it properly means capturing all three at module init, which is a separate change to code this PR does not otherwise touch.

• std::bad_alloc between the ref transfer and the park registration would terminate the process. Pre-existing: before this change the same window ended in addWakeCallback() pushing onto a std::vector<std::function>, which can throw identically, and no N-API callback in this codebase is exception-guarded. Worth noting ParkTimeoutRegistry::schedule() does not leak on a partial insert — an orphaned deadline whose park is missing is erased by runLoop()'s existing parks.find() == end() branch.

• retry: 1 on commit-teardown.test.ts can let a probabilistic native abort pass. It is the narrowing the previous round asked for (Deno-on-macOS only, one test, rather than re-rolling the whole suite); a skipIf would remove the crash guard on that platform outright rather than weaken it.

• retryNowFinalize deletes its refs without an env == nullptr guard — checked and declined as a false positive. The NULL-env convention is documented for napi_threadsafe_function_call_js only; the finalize callback runs on the JS thread with a live env when the thread count hits zero, which is the contract event_emitter.cpp:216-219 already states and relies on. The function is also unchanged by this PR.

• Join-at-close is still not covered by a test. I re-ran the mutation myself: join() → detach() in ParkTimeoutRegistry::shutdown() leaves the file 12/12 green, because shutdown() resolves pending parks under its mutex before joining. The close test's comment no longer claims otherwise. Real coverage needs the ASan lane, not another deadline assertion.

Cross-model review used independent Codex and Gemini lenses plus Harper-domain adjudication across nine rounds. It found and drove fixes for the original wake-path deadlock, transient descriptor pins, deadline lookup, thread-creation failure, promise-settlement failures, getenv thread-safety, comment accuracy, the wall-clock assertion above, and — in round 8, rebasing onto latest main — the napi_closing use-after-close noted above. Round 9 (delta, after that fix) confirmed no new defect and that the other findings carry forward unchanged. Rounds 10-12 covered this review-feedback round: round 10 verified the CAS removal is sound (confirming schedule returns 0 only before it moves fired into an entry, and that fireOnce is constructed afterwards) and raised one new finding caused by the README addition itself - documenting the knob as operator-facing while every misconfiguration of it stays silent - which round 11 closed by stating the parsing rules; round 12 covered the comment trim. No round found a defect in the delta. The Cursor lens has never run on this branch: cursor-review refuses any diff that touches agent instructions, and this one edits AGENTS.md.

Rebase note (rounds 8–9): rebased onto latest origin/main (da0ef135) with no semantic conflicts — the only overlap was an independent upstream AGENTS.md invariant renumbering (main inserted its own invariant 13; this PR's invariant 13 became 14), resolved by keeping both. pnpm build, pnpm test:native (151 passed), and the park/lock-tracker/commit-teardown/orphan-gc Vitest suites (21 passed) were re-run clean against the rebased tip before and after the napi_closing fix above.

Verification

• pnpm build
• pnpm check — clean (type-check, oxlint, oxfmt)
• pnpm test:native — 151 passed
• pnpm test — 58 files passed, 805 tests passed, 3 skipped
• node --expose-gc ./node_modules/vitest/vitest.mjs test/lock-tracker.test.ts test/commit-teardown.test.ts — 14 passed after the final delta
• Re-verified from a clean worktree at 65153100 (Linux, Node 26.2.0, RocksDB 11.8.1): pnpm check clean, pnpm test:native 151 passed, pnpm test 58 files / 805 passed / 3 skipped, and the two park files 14 passed.
• Re-verified again after the rebase onto main and the napi_closing fix, at dc1bacfd (Linux, Node 26.2.0, RocksDB 11.8.1): full pnpm build, pnpm test:native 151 passed, and lock-tracker.test.ts + commit-teardown.test.ts + transaction-orphan-gc.test.ts 21 passed.
• Re-verified after this review-feedback round at d2587a27 (Linux, Node 26.2.0, RocksDB 11.8.1): pnpm build:binding + pnpm build:bundle, pnpm check clean, pnpm test 59 files / 819 passed / 3 skipped (the one initial failure was transaction-log-crash-recovery.test.ts needing a dist bundle that did not exist in the fresh worktree; it passes once built), and lock-tracker.test.ts 12/12 in 2.93 s with the raised backstop.
• Fails-on-base proof: the isolated timeout regression was applied to origin/main, where the child exceeded its 4-second guard and was terminated; the branch completes in under 500 ms.
• Env-read regression check: with the value read once per process, the fork fixture still completes in 369 ms and still asserts elapsed >= 40, so both halves of the parse rule (the child's ROCKSDB_JS_PARK_TIMEOUT_MS=1 taking effect, and the 50 ms clamp) stay covered.

Refs #741

_{Review-Coverage: authored=claude; ran=codex,gemini; adjudicated=domain; blocked=cursor-grok(no-receipt); declined=cursor-composer; rounds=12 @ d2587a2}

_{Human-Review-Need: 4 (decisions: timeout-consumes-a-retry-attempt, default-5000ms-and-no-opt-out, bound-at-the-park-vs-reap-the-holder, env-var-only-configuration, plain-std-thread-per-descriptor) @ d2587a2}

[gemini-code-assist]

Code Review

This pull request introduces a bounded park timeout (defaulting to 5000ms) for coordinated-retry commits waiting on conflicting locks, preventing indefinite hangs caused by leaked or abandoned transactions. It implements a descriptor-owned timeout thread, ensures safe cleanup of thread-safe functions during environment teardown, and adds regression tests. The feedback suggests using performance.now() instead of Date.now() in the tests to provide monotonic, high-resolution timing and avoid potential flakiness from system clock adjustments.

[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	24.69K ops/sec	40.49	39.13	518.705	0.107	123,473
🥈 rocksdb	2	10.93K ops/sec	91.46	87.79	31,065.105	1.22	54,669

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

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	28.74K ops/sec	34.79	33.59	521.914	0.103	143,711
🥈 rocksdb	2	10.49K ops/sec	95.34	92.69	2,879.657	0.122	52,444

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.09K ops/sec	41.50	36.45	1,775.753	0.301	120,475
🥈 rocksdb	2	16.16K ops/sec	61.87	53.28	1,079.226	0.121	80,822

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	377.99 ops/sec	2,645.579	74.58	75,424.304	14.41	756
🥈 lmdb	2	26.10 ops/sec	38,321.361	419.198	1,200,938.239	135.699	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	39.80K ops/sec	25.13	11.49	20,788.79	0.847	198,993
🥈 lmdb	2	440.77 ops/sec	2,268.76	77.76	16,531.036	1.24	2,204

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	710.65K ops/sec	1.41	1.21	446.263	0.062	3,553,254
🥈 lmdb	2	448.40K ops/sec	2.23	1.15	8,118.139	0.540	2,242,008

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	828.90 ops/sec	1,206.419	1,026.614	5,127.862	0.828	1,658
🥈 lmdb	2	1.13 ops/sec	886,201.565	830,478.393	986,428.821	4.60	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	21.89K ops/sec	45.68	29.97	20,524.644	2.09	43,786
🥈 lmdb	2	837.47 ops/sec	1,194.074	92.22	9,297.709	4.98	1,675

---

Results from commit e99b6f4

[kriszyp]

Addressed all seven unresolved review threads. The PR body now records the verification results, remaining coverage gap, and explicit timeout-policy decisions for human review.

[cb1kenobi]

Reviewed a45e8493 — no issues found in the incremental diff. This PR looks good, nice job!

Re-review scope: 88580c00..a45e8493 is a fast-forward (both heads share merge base 23f21726, verified with git range-diff — 11 commits identical, 3 new). Genuine new work: 3 commits / 3 files, +53/-38.

Both of my remaining Low findings are addressed:

• getenv thread-safety — 48f985cf restores the function-local static, matching commitThreadMode()/commitDelayMs(), with the 50 ms clamp and zero-is-malformed rule inside the initializer. Confirmed the only consumer of the env var is a spawned child process (test/lock-tracker.test.ts:195), so nothing depended on the per-park read; lock-tracker still runs 12/12 in 3.32s.
• Coverage claim — 87e7378c + a45e8493 correct it rather than overstating. I re-ran the mutation at this head to check: toJoin.join() → toJoin.detach() in ParkTimeoutRegistry::shutdown(), rebuilt, mutation confirmed in the artifact (nm -u db_descriptor.o shows std::__1::thread::detach(), absent from the clean build), still 12/12 green. The corrected comment matches that result.

Re-verified from the prior round, since a later commit could have reintroduced it:

• The PurgeIfUnreferenced-under-writerMutex_ self-deadlock is still structurally impossible here. nm -u transaction.o shows 42 undefined rocksdb_js:: symbols and zero DBRegistry::; PurgeIfUnreferenced is absent. Validated with a negative control — re-injecting the call into fireOnce makes the symbol appear (42 → 43) and a marker string land in both the .o and the .node; reverting removes both. Note this is symbol-level and structural evidence, not a dynamic repro.
• Lock ordering is unchanged: txnsMutex → writerMutex_ already exists at the merge base (cancelForDB under txnsLock), and ParkTimeoutRegistry's mutex remains a leaf, so there is no ordering constraint against Serialize database destruction with concurrent opens #787 in either direction.

Test results at this head: lock-tracker 12/12 (3.32s), full suite 807 passed / 1 skipped, CI 24 pass / 2 skipping. Against the merge base, the fork-based timeout regression fails (child SIGTERM'd at its 4s guard) while the close-path test passes — so the coverage split is exactly what the corrected comment now describes.

—
Generated by Barber AI

[cb1kenobi]

Reviewed dc1bacfd — no issues found in the incremental diff. This PR looks good, nice job!

Re-review scope — derived with git range-diff, not the compare API. Merge bases computed separately per head: merge-base 65153100 origin/main = 23f21726, merge-base dc1bacfd origin/main = da0ef135. They differ, so this was a rebase onto 14 new main commits, not a fast-forward — which means the delta had to be checked for content that came from neither side. Three independent methods agree it is clean:

1. git range-diff 23f21726..65153100 da0ef135..dc1bacfd — 15 commits map 1:1, 1 appended. The four marked ! (15bbcb77, 15088793, d09d9ff7, 94906e72) differ only in AGENTS.md/transaction.cpp context lines that main moved.
2. git merge-tree --write-tree 65153100 da0ef135 conflicts in AGENTS.md and transaction.cpp. Diffing that auto-merge tree against dc1bacfd^ shows the resolution keeps main's wrapperCollected → close() block and this PR's relocation of the RetryNowContext transfer — both derivable from the two sides, nothing foreign introduced.
3. Comparing the PR's own net patch (added/removed lines only) between the two ranges differs by exactly 4 lines, all of them the AGENTS.md invariant renumber (12 → 13, 13 → 14) forced by main inserting its own invariant 12.

Genuine new work: 1 commit, 2 files, +13/−4 — dc1bacfd "don't release a closing park timeout tsfn twice".

That commit is sound. Guarding napi_release_threadsafe_function on the preceding napi_call_threadsafe_function status is the pattern already used at db_descriptor.cpp:1132-1136 and :1172-1176; the == napi_ok form is behaviourally identical to != napi_closing here because the park tsfn is created with max_queue_size = 0 (unlimited), so napi_queue_full is unreachable, and the exactly-once fired CAS means the thread count cannot reach zero before this call — so napi_closing at this point really only means env teardown, where Node destroys the tsfn itself and the skipped release cannot leak. (Small nit on the commit message only: dispatchCommitCompletion/releaseCommitCompletionsByEnv don't actually implement this pattern — the first never releases, the second releases without calling. lockReleaseByKey is the real precedent.)

Prior findings re-checked against this head, in the artifact rather than from thread state:

• Critical (wake-path re-entry) — still fixed. fireOnce still captures only weak_ptr<ParkTimeoutRegistry> + weak_ptr<std::atomic<bool>> + parkId and calls fire(id). nm -u build/Release/obj.target/rocksdb-js/src/binding/transaction/transaction.o | c++filt: 175 undefined / 43 rocksdb_js:: / 0 DBRegistry::. The only registry symbols this TU references are ParkTimeoutRegistry::schedule and ::fire. The 174→175 / 42→43 drift from the prior head is exactly one symbol, TransactionHandle::close(), which is main's code arriving via the rebase. Negative control re-run through a real make BUILDTYPE=Release (not node-gyp build, which can silently no-op): re-injecting a DBRegistry::PurgeIfUnreferenced call with a unique marker takes the counts to 176/44/1 and puts the marker in both the .o and the .node; reverting restores a byte-identical symbol set and removes the marker from both. This is source + symbol-graph evidence with a validated detector — not a dynamic repro; forcing the interleaving would require mutating the code under review.
• Low (join-at-close uncovered) — still open, and now conclusively so. toJoin.join() → toJoin.detach() in ParkTimeoutRegistry::shutdown(), rebuilt and confirmed in the artifact (nm -u db_descriptor.o gains std::__1::thread::detach(), absent from the clean build): lock-tracker 12/12 green, and this time I also ran the whole suite against the mutated build — 60 files, 814 passed / 1 skipped, all green. So nothing anywhere in the suite kills it. The test comment at test/lock-tracker.test.ts:219-227 says exactly this, so the claim matches the measurement; the gap is documented, not misstated.
• Medium (a timeout-driven RETRY_NOW spends a coordinatedRetry attempt) — unchanged by this push. src/database.ts and src/transaction.ts remain untouched by the entire PR at this head, parkTimeoutMs() is still the function-local static with the 5000 ms default, and no opt-out, backoff, or change to the retry accounting was added. It stays where it was: a documented policy decision for the human reviewer, not something for you to action.

Testing. Local build at dc1bacfd: lock-tracker 12/12 in 3.11s, lock-tracker + commit-teardown 14/14, full suite 60 files / 814 passed / 1 skipped. Merge base da0ef135 built from scratch for comparison (nm confirms zero ParkTimeoutRegistry symbols in its .node): the same two files give 12/12. The PR's own test files are byte-identical between 65153100 and dc1bacfd — the test/ delta between those heads is all main's (transaction-orphan-gc.test.ts, stats.test.ts, a fixture), which is also what moves the suite total from 808 to 815 — so the earlier fails-on-base result for the fork-based timeout regression carries forward unchanged. CI at this head: 24 pass / 2 skipping — the full matrix is green.

One detection note for anyone repeating the join mutation: the linked .node carries an undefined std::__1::thread::detach() even on a clean build — it comes from deps/rocksdb/lib/librocksdb.a, not from this project (no project object references it, and there is no .detach() anywhere in src/). Only the nm -u on db_descriptor.o discriminates. That joins the two artifacts already known here: the residual join() traced to commit_worker.h:110, and the DEBUG_LOG marker being a release-build no-op (debug.h:39).

Generated by Barber AI

[cb1kenobi]

Tested. Looks good!