Serialize database destruction with concurrent opens

[kriszyp]

Summary

Rebases this PR onto latest main. While this was in flight, main independently landed an equivalent-but-different fix for the same destroy-vs-open race (2c2066aa, 84157bf9): a resolved-identity, per-entry gate (DBKey{path, readOnly, secondaryPath}) instead of this PR's original path-only destroyingPaths set. Per Kris's direction, this rebase adopts main's gate rather than porting the PR's original design: the first commit skips the PR's now-duplicate commit and adapts every dependent commit onto main's per-entry/cross-key conditions, and the second carries forward the PR's quarantine/retry/compaction-cancellation work and its test fixtures on top of that. destroyingPaths itself is re-derived (not ported) as a separate, lock-free-compatible gate for the window between "registry entries erased" and "physical files deleted" — physical deletion runs without databasesMutex held (it's I/O-bound), so main's design alone left that window ungated; this was found by test failures against the carried-forward fixtures, not assumed from the PR.

This is the rocksdb-js root-cause fix for Harper PR #2169: it moves a lifecycle invariant Harper worked around in JavaScript (locking root opens) into the native registry, makes teardown failures observable/recoverable via quarantine + database:closeFailed, and adds cancellation tokens so a manual compaction cannot block teardown indefinitely.

Second rebase: onto main's WriteBufferManager stall watchdog (#824)

main moved again (7213b98a, "Make a WriteBufferManager write stall observable") and this branch was CONFLICTING. Four conflicts, all resolved keeping both sides:

• binding.cpp Shutdown — main's watchdog begin…Shutdown()/join…Watchdog() bracket now wraps this PR's try/catch
  • throw. Two ordering constraints had to be reconciled: this PR moved GlobalEvents::Shutdown() after DBRegistry::Shutdown() (a quarantining close emits database:closeFailed, which needs its listeners to still exist — test/fixtures/fork-shutdown-failure.mts asserts the event), and the watchdog join must run before the throw, or a failed shutdown() leaves the 1 Hz thread alive. Same combination in the last-env cleanup hook.
• backup.cpp — both hunks were pure PR-side additions (BackupInFlightClaim, the napi_cancelled in-flight decrement) that git could not place.
• db_registry.h — this PR's CloseResult CloseDB(...) return type alongside main's CollectWriteBufferManagerInventory.
• AGENTS.md — invariant renumbering. Main's WBM-stall invariant becomes 21; this PR's three become 22/23/24, with the two internal cross-references (see invariant 21 → 22, see invariant 22 → 23) updated. Main's droppedColumns.clear() in finishClose() and its two-argument ColumnFamilyDescriptor constructor were carried into this PR's rewritten finishClose(bool destroying) / OpenDB().

Fixed by the rebase's own independent pre-push review

Two full rounds against the rebased head (codex + gemini + harper-domain; Cursor legs are disabled on any diff that edits AGENTS.md). Round 1 found five real items, all fixed; round 2 verified each against HEAD and produced no new actionable findings.

• Stale transaction-log cache survives a foreign close into the reopen — a regression from this PR's own invariant-18 fix. The ownerThreadId guard that makes a foreign close napi-safe also means a cross-env destroy()/shutdown() can no longer clear logRefs, so a reopened handle handed useLog() back a TransactionLog whose TransactionLogHandle::store weak_ptr pointed at the unregistered store of the closed lifecycle. Only addEntry re-resolves; every read accessor reported an empty log. DBHandle::open() now releases the cache on the owning thread before DBRegistry::OpenDB. test/fixtures/fork-foreign-close-log-cache.mts holds the stale TransactionLog alive across the foreign shutdown() (the cache entry is a weak napi_ref, so letting it be collected would mask the bug) and asserts log size, queried entry count, and object identity — it reports 0 bytes, expected 31 without the fix, 3/3 clean with it.
• A destroy-cleanup tombstone omitted transactionDetails — RegistryStatusDB declares it non-optional, so a monitor reading entry.transactionDetails.length threw on exactly the entry shape that only appears when a physical destroy failed, i.e. when the diagnostic is needed.
• Three test-delay seams still called ::getenv off the JS thread — ROCKSDB_JS_BACKUP_DELAY_MS (libuv worker), ROCKSDB_JS_DESTROY_DELAY_MS and ROCKSDB_JS_CLOSE_RETRY_DELAY_MS (any teardown thread), all added by this PR, now snapshotted in initializeTestSeams() like the fault flags beside them. AGENTS.md's entries for the three say so.
• fork-shutdown-retry.mts raced the retry claim — the worker posts before calling shutdown(), so the parent's open could reach the still quarantined entry and fail with "previous close failed" instead of measuring the wait; and a handle opened while the process-wide shutdown() loop is still scanning can be force-closed before the data assertion reads it. Both barriers the two-descriptor fixture already had.
• fork-compact-cancel-async.mts could drop the destroy result — await outcome yields to the event loop with no 'message' listener attached, so a { destroyed: true } posted in that window was delivered to a zero-listener emitter and dropped, hanging the fixture to its runner timeout. The promise is now claimed before the await. The sync sibling is not affected (compactSync() blocks the loop, so the queued message is only delivered after the next listener attaches).

Red CI caught a real path-identity defect

The first rebase push turned macOS CI red (Bun + Deno) with four quarantine tests failing on /private/var/... vs /var/.... Root cause, not a test problem: a destroy-cleanup tombstone in registryStatus() and every database:closeFailed event reported the registry key's resolved identity, so a caller matching either against the path it opened did not recognize it. That is exactly what AGENTS.md invariant 19 forbids ("returning only the resolved identity breaks callers that match paths against the spelling they supplied") and what registryStatus() already did correctly for a live descriptor.

• emitCloseFailures() reports descriptor->path.
• DBRegistryEntry::reportedPath remembers the opening caller's spelling, because a tombstone has no descriptor left to ask. DestroyDB captures it from the first claimed descriptor under databasesMutex, and a retry of a failed destroy — which finds only the tombstone — falls back to the spelling that entry already remembered (the round-3 review finding).

Not caused by the rebase: the pre-rebase head carried identical code and was green on the older macos-26-arm64 runner image (20260728 → 20260831), whose TMPDIR made the latent mismatch reachable. test/destroy.test.ts now covers it on every platform with an explicit symlink rather than depending on macOS's /var link, and asserts both the initial failure and the retry. Each report site was reverted individually to confirm the test fails — with expected undefined to be true, the same assertion macOS CI produced, and expected [ …(2) ] to match object [ …(2) ] for the retry.

A CI "flake" that was a real use-after-free

The first two rebase pushes each had exactly one job die with SIGSEGV in fork-destroy-open.mts — a different runtime each time (Bun/ubuntu, then Deno/ubuntu), which reads like flake. It is not. Running that fixture six-way concurrently reproduced it locally at 2/60, and gdb named the frame:

__strlen_avx2
v8::String::NewFromUtf8
napi_set_named_property
rocksdb_js::DBRegistry::RegistryStatus

databasesMutex covers the registry map, not a descriptor's own maps. registryStatus() walked descriptor->columns — guarded by columnsMutex — while a cross-env destroy()'s finishClose() cleared that map from the worker thread, so name.c_str() pointed into a freed map node and napi_set_named_property() strlen()ed it. locks.size() was read the same way (a count, so a torn read rather than a fault). The column summary is now snapshotted under columnsMutex (plus the per-CF userSharedBuffersMutex for its buffer count) with the N-API values built after releasing it — holding it across those calls would risk a finalizer re-entering the same non-recursive mutex on this thread, which is why transactions already had this shape under txnsMutex. locks.size() is read under locksMutex.

No lock-order inversion: OpenDB and CollectWriteBufferManagerInventory already establish databasesMutex → columnsMutex, getUserSharedBuffer is a leaf, and no locksMutex region reaches the registry. Pre-existing on main (its registryStatus() walks columns unguarded too); this PR is what makes it reachable, because destroy() now force-closes every descriptor and the PR's own fixtures poll registryStatus() across that window.

test/fixtures/fork-registry-status-column-race.mts makes it deterministic instead of leaving it to CI luck: a worker churns dropSync() against the shared descriptor while the main thread polls registryStatus(), with the new ROCKSDB_JS_REGISTRY_STATUS_COLUMNS_DELAY_MS seam parking the walk per column family so an erase lands inside it. The column names run past libstdc++'s 15-char small-string buffer so the erase frees a separate heap allocation — an SSO name usually survives the free intact and hides the bug, which is why the first attempt at this fixture (driving the close-time columns.clear() instead) reproduced 0/7 and was deleted rather than shipped unreferenced. 5/5 abort without the fix, 3/3 clean with it, and the concurrent fork-destroy-open.mts stress went 2/60 → 0/132. Reverting the snapshot fails the churn fixture, so the close-time path is covered by the same net.

Review-thread adjudication

All three previously-unresolved threads were re-checked against the current code; none is left unanswered, and the one new thread is ruled on below.

• shutdown() can abort process exit and skip later cleanup (new, binding.cpp) — claim verified, prescribed fix overruled. Details under ## For the human reviewer.
• Raw path aliases bypass destruction ownership (db_registry.cpp) — unchanged ruling: real, pre-existing (registry identity has always been the raw path string), and canonicalizing changes a user-visible identity contract. Wants its own PR.
• Quarantined descriptors still admit transaction commits (transaction.cpp) — unchanged
  ruling: the window is real but is not specific to quarantine, and the suggested fix (reject admission when descriptor->isClosing()) is what AGENTS.md invariant 18 exists to prevent. The fix that closes it — running the closables sweep before the flush in finishClose() — reorders the most delicate path here and wants its own change.

Third rebase: onto main's tsdown bump

main moved again (dependabot's tsdown bump, merged as 531af655). No conflicts — the only delta between the previous head's merge-base and origin/main was package.json/pnpm-lock.yaml, and git rebase origin/main replayed all 11 commits clean. Build, pnpm test:native (194/194), and the full Vitest suite (935 passed / 9 skipped) all pass post-rebase; pnpm fmt:check/lint/ type-check are clean.

Independent pre-push review (round 25, full — a force-push is never an ancestor of the prior review): codex (graded) and Gemini both ran; the Harper-domain adjudicator hit its time budget and was killed (SIGKILL, timeout) before it could rank/filter, so I adjudicated the raw output myself against the code:

• Codex's three "surviving findings" (iterator mutex cost, an atomic load's default memory order, a comment-style nit) are unchanged carries-forward from the ~24 prior rounds already reflected in this description — not new, not acted on.
• Gemini reprised the shared-benchmark-db-path claim already dismissed above, plus a new minor claim that benchmark/setup.ts's teardown resolve()-then-throw silently reports success: it conflates the serialization gate (activeBenchmark/promise, used only to sequence between benchmarks) with the current benchmark's own result (the async setup() call's returned promise, which the trailing throw genuinely rejects — throws: true is set exactly so vitest surfaces it). Not applied.
• Gemini's one new claim outside prior rounds — db_registry.cpp:1029 holds databasesMutex across napi_create_string_utf8 calls in RegistryStatus(), and if V8 GC runs a DBHandle finalizer synchronously mid-allocation, that finalizer's DBRegistry::CloseDB would re-lock the same non-recursive mutex on the same thread — checks out as a real hazard class (it's exactly what invariant 6's columnsMutex/txnsMutex/locksMutex narrow-scoping exists to avoid for the other locks in this same function), but the outer databasesMutex hold is byte-for-byte unchanged from origin/main (confirmed via git show origin/main:src/binding/database/db_registry.cpp) — it predates this PR and every prior round. Left alone as out-of-scope for a rebase; flagged separately for its own issue.

For the human reviewer

shutdown() now throws — the review claim is correct, and I kept the behavior. Your call to overrule me. The bot's facts check out, and I verified the Node semantics directly: a throw from a process.on('exit') listener skips every exit listener registered after it (unconditionally), and flips the exit code to 1 unless an uncaughtException handler is installed. Harper core calls it exactly that way — resources/RocksTransactionLogStore.ts:19, process.on('exit', () => shutdown()), bare, registered at module load — so on a close-time flush failure at exit it would skip later exit listeners, including ones registered by application components. On main, shutdown() genuinely never throws (descriptor->close() is void and the close-time flush status is discarded), so this is a behavior change, not a clarification.

Three reasons I did not adopt "keep shutdown() non-throwing":

1. db.close() already throws the identical error (database.cpp:211, with the "Call shutdown() to retry close, or destroy() to delete the database" suffix). Making shutdown() silent would have the two close entry points disagree about whether a failed close is an error.
2. Three of DBRegistry::Shutdown()'s four throw sites are lifecycle timeouts, not close
   failures: shutdownMutex contention, the per-descriptor drain, and the destroy-in-flight wait. database:closeFailed cannot carry those — no descriptor failed. A blanket non-throwing shutdown() would return normally while databases are still open or files still being deleted, which is a worse silent failure than the one being avoided.
3. Harper does not listen for database:closeFailed (no listener anywhere in the repo), so today the throw is the only channel that reaches it. Event-only reporting would make a close-time flush failure fully invisible there.

What I did instead is make the contract explicit where a caller will see it: the README already documented the throw and the try/catch exit-listener pattern, and the exported shutdown now carries the same JSDoc. The harper-side guard is still missing and is a one-line change at resources/RocksTransactionLogStore.ts:19; it needs to land with or before this, and it is outside this repo. If you would rather not ask that of consumers, the alternative is a return value plus a separate throwing API, and I'd want your direction before building it.

Carried from the original PR body, still open (unchanged by either rebase):

• Registry identity compares raw path strings, so a ../symlink alias can still bypass the destroy/open gate (narrower than it reads — RocksDB's own LOCK file blocks a second read-write open through an alias; a read-only alias during destroy is the live hazard). Wants its own PR — canonicalizing changes a user-visible identity contract (registryStatus().path, lock/backup file paths, TransactionLogStoreRegistry keys).
• A transaction can still be admitted against a quarantined descriptor via the legacy libuv commit path, since the closables sweep never ran for that descriptor. The fix (running the closables sweep before the flush in finishClose()) reorders the most delicate path here and wants its own change.
• Last-env module cleanup still keys off an unsynchronized --moduleRefCount == 0; a fresh env loading the module concurrently isn't coordinated against a concurrent Shutdown()/Teardown(). Pre-existing.
• DBIteratorHandle::Next()'s unconditional per-row iteratorMutex lock/unlock on the hottest read path. Estimated low single digits percent overhead against a several-hundred-ns per-row N-API cost — real, but the obvious fix (a relaxed-load gate) is insufficient (a closer could still free the iterator between the load and the mutex), and a correct reader/closer handshake plus a pnpm bench range-scan comparison is more than a rebase should take on.

Declined this round, with reasons:

• isClosing() as a memory_order_relaxed load (raised as a major by Gemini, downgraded to a nit by the adjudicator in both rounds, and independently scored a nit by Codex). Its own estimate is that the LDAR is dwarfed by iterator->Next(), and getKeysCount() is not a request hot path. The flag is also read under mutexes in the registry paths, so changing the default ordering of a widely-used accessor is not a local change.
• The aggregate comment-narration nit (a handful of blocks in db_registry.cpp, db_descriptor.{h,cpp}, async.h, db_handle.cpp, closable.h, binding.cpp that narrate implementation history or address the reviewer). The technical content is accurate; churning ten comment blocks across a 69-file lifecycle diff adds review surface for no behavior change. Better as a follow-up sweep over the whole file set at once.
• RegistryStatusDB.userSharedBuffers is declared non-optional but no branch of registryStatus() sets a top-level property of that name (it is per-column-family, which is what the README documents) — so entry.userSharedBuffers > 0 silently reads undefined > 0. Verified pre-existing on main; same for columnFamilies being typed string[] against an object. Wants its own change.
• A cached TransactionLog retained across a foreign close and never reopened still reports an empty log to its holder. Unchanged from main (which deleted the weak ref on close, leaving any retained object equally stale), and the open-time invalidation above fixes the reopen path, which is the reachable one.

Remaining CI failure, not from this PR

test/txn-close-commit-uaf.test.ts aborts on Deno/macOS only. That is the pre-existing worker-env teardown abort the test itself names and already mitigates — const retry = process.versions.deno && process.platform === 'darwin' ? 1 : 0 with a comment pointing at #746 — and it exhausted that single retry. It appeared on the same job before the path-spelling and use-after-free commits, and nothing in this PR touches that repro's path.

Verification

• pnpm check (type-check + lint + fmt) — clean.
• pnpm test:native — 194/194 passed.
• pnpm test (Vitest) — 933 passed, 9 skipped (expected Deno/GC-related skips), 0 failed.
• test/destroy.test.ts alone — 31/31, including the new foreign-close log-cache fixture.
• test/destroy.test.ts alone — 32/32, run 5× to check the new symlink test for flakiness.
• Independent pre-push review, six rounds on the rebased head. Round 1 (--full, forced by the rebase — the prior review is no longer an ancestor) found the five items above. Round 2 (delta) confirmed all five fixed against HEAD and surfaced no new actionable finding; its three surviving items are the two declined nits plus the pre-existing userSharedBuffers type mismatch, and it recorded a correction to round 1 (which had claimed the tombstone branch omitted userSharedBuffers too — never a top-level field). Round 3, on the path-spelling fix, found the tombstone-retry gap, fixed above. Round 4 converged: the graded leg reported zero findings.
• Rounds 5 and 6 covered the use-after-free fix above: round 5 found the unreferenced fixture (removed) and a comment of mine that overclaimed which RegistryStatusDB fields the tombstone branch fills (reworded); round 6 converged, with every item previously adjudicated.
• Gemini claims checked and rejected rather than applied, across the rounds: checkpoint.cpp leaking operationsInFlight when admission/queueing fails (CheckpointInFlightClaim is the RAII equivalent of BackupInFlightClaim, and handedOff is set only after both succeed), and benchmark/setup.ts making concurrent workers share one database path (they already did before this PR — the declaration moved within the same scope chain, and sharing one database across workers is the point of those benchmarks); checkpoint.cpp's CheckpointInFlightClaim again; ReleaseLogRefsByEnv "permanently leaking" napi_refs for an erased descriptor (they are env-owned and reclaimed at env teardown, and once the entry is erased no foreign close() can reach the handle through closables, which is the hazard invariant 18 exists for); and, for the third time, a per-call __cxa_guard_acquire on the seam flags (a std::atomic<int> with a constexpr constructor is constant-initialized, so no guard is emitted — confirmed by inspecting the generated assembly in an earlier round).
• Earlier rounds on the pre-rebase head (four, converged) are unchanged and described in the commit history.

Refs #787

🤖 Generated with Claude Code

Final maintenance pass — 2026-09-09

This note supersedes earlier rebase, review-coverage, and verification statements above. The branch is rebased cleanly onto 17fceeef; PR head ac31bdea contains the final review-feedback fix.

• The new registryStatus() thread was valid: carrying a shared_ptr<DBDescriptor> beyond databasesMutex could make a racing last-handle close see an extra owner and skip its purge with no release-side retry. RegistryStatusEntry now contains only copied values, captured under the registry and owning child locks, and all N-API construction happens after unlocking. The audited registry-to-child ordering is documented beside each mutex, and invariant 6 records the ownership-pin failure mode.
• The new worker fixture uses a dedicated status worker, asserts that the close actually overlaps the delayed registry walk, and then verifies the entry was purged. It is wired into the destroy lifecycle suite. The earlier column-map race fixture now reports its own timeout before Vitest's outer deadline.
• The three older open threads remain human decisions: canonical path identity changes user-visible behavior; rejecting transaction admission on isClosing() violates invariant 18 and the safer closables-before-flush reorder needs a separate change; and shutdown() error reporting is a consumer-compatibility choice rather than a mechanical review fix.

Changes

• Public lifecycle behavior and configuration are documented in README.md and streaming-copy semantics in docs/backups.md. DBSettings::Config validates the new lifecycle wait bound stored in db_settings.h; database.ts documents close-failure events and carries read-only intent into destroy, while store.ts distinguishes closing from closed sync operations. db_options.h only updates the shifted invariant reference.
• The native close boundary is coordinated across every resource type: closable.h adds early cancellation and env-ref release hooks; database.cpp guards synchronous native access, propagates quarantine errors, passes the correct compaction token, and validates destroy authority; database.h carries the cancellable compaction signatures; db_descriptor.cpp implements the close claim, ordered drains, failure quarantine, retry, and cancellation; and db_handle.h adds per-handle serialization and cancellation state.
• Async admission and teardown are made race-safe in napi/async.h: cancellation and registration share a mutex, drains are unbounded before native release, and queue failures unwind their claims. backup.cpp, backup_stream.cpp, and checkpoint.cpp pin the descriptor only for the copy lifetime, balance cancelled/failed admissions, release before promise settlement, and retry a purge that the temporary pin deferred.
• Transaction and iterator lifetimes now obey the same admission/drain contract. transaction.cpp rejects commits racing transaction close and protects count scans; transaction_handle.cpp makes registration conditional, replaces the timeout-and-leak stopgap with a complete drain, and delegates cancellable counts; transaction_handle.h exposes that aborted-scan result. db_iterator.cpp, db_iterator_handle.cpp, and db_iterator_handle.h serialize iterator access against foreign close, make repeated iterator close idempotent, and cancel unbounded count scans rather than returning partial results.
• Transaction-log changes are mechanical invariant renumbering in transaction_log_file.cpp, transaction_log_file_windows.cpp, and transaction_log_store.cpp; they preserve the rule that read-only followers never mutate the primary's log tree.
• Benchmarks now retain one shared database until every worker closes it and surface teardown failures in benchmark/setup.ts. The instance stress coordinator waits on distinct close acknowledgements in db-instances.stress.test.ts, its database worker closes its message port, and the transaction worker closes both database and port so process teardown exercises no live handles.
• Integration coverage makes the new error semantics explicit: background-error.test.ts force-destroys a sticky background-error descriptor; backup-stream.test.ts, backup.test.ts, and checkpoint.test.ts prove copy races settle without leaks or premature native teardown; drop.test.ts checks flush-failure quarantine and explicit recovery; lifecycle.test.ts proves async admission resets on reopen; ranges.test.ts proves iterator return/throw idempotence; and secondary.test.ts verifies destroy waits for follower catch-up and later purges the descriptor pin.
• The forked fault-injection fixtures exercise close quarantine and recovery at process boundaries: fork-close-failure.mts, fork-destroy-failure.mts, fork-flush-failure.mts, fork-gc-close-failure.mts, fork-quarantined-exit.mts, and fork-shutdown-failure.mts cover explicit close errors, failed deletion tombstones, flush retries, finalizer failures, quarantined exit, and global shutdown reporting respectively.
• Cross-env lifecycle fixtures make destroy ownership and retry timing deterministic: fork-backup-destroy.mts holds a backup during destroy; fork-destroy-open.mts and its worker race deletion against reopening; fork-open-attach-destroy.mts and its worker cover the adopted-descriptor attach window; fork-lifecycle-timeout.mts and fork-lifecycle-timeout-two-descriptors.mts prove bounded conflicting-operation waits; and fork-shutdown-retry.mts plus its worker verify reopen waits for a quarantined close retry to complete.
• Unbounded native work gets a dedicated cancellation regression: fork-compact-cancel-async.mts, fork-compact-cancel-close.mts, fork-compact-cancel-destroy.mts, and fork-compact-cancel-sync.mts cover both tokens and both close paths; fork-count-destroy-race.mts and fork-iterator-next-race.mts cover count cancellation and iterator serialization; and shutdown-compact-cancel-worker.mts drives foreign shutdown across async compaction.
• The status-snapshot tests use column-drop-churn-worker.mts to erase column metadata during inspection. Finally, test/lib/util.ts preserves the original assertion error while still attempting every close and reports the first cleanup failure only when the test itself passed.

Verification

• pnpm check — clean (type-check, lint, formatting); git diff --check — clean.
• pnpm test:native — 194/194 passed.
• pnpm test — 956 passed, 9 skipped, 0 failed.
• The regression failed on 87842947 with a retained registry entry (followed by a teardown SIGSEGV) and passes on this head; the strengthened overlap assertion also passes.
• Planning fallback after the default Claude leg hit quota: Framing-Verdict: chosen-approach-sound (Gemini). Final full review produced no new actionable
  finding: Gemini repeated an adjudicated benchmark false positive and the existing comment nit; Claude failed at startup, Cursor was policy-pruned for the AGENTS.md edit, and the domain leg timed out. These are review-infrastructure coverage gaps, not test failures.
• Current-head CI has reported: validation and security checks pass; platform, native, benchmark, and stress jobs are running with no reported failure.

Complexity: complicated

_{Review-Coverage: authored=codex; ran=gemini; blocked=claude(exit-1),domain(timeout); declined=cursor-grok,cursor-composer; rounds=8 @ ac31bde}

_{Human-Review-Need: 4 @ ac31bde}

[gemini-code-assist]

Code Review

This pull request introduces robust database lifecycle management for RocksDB JS bindings. It implements a timed-wait mechanism (lifecycleWaitSeconds) for open, destroy, and shutdown operations to prevent concurrent lifecycle conflicts. It also introduces a "quarantine" state for database paths when a native close, flush, compaction, or physical directory cleanup fails, preventing subsequent opens until the cleanup is retried via destroy() or shutdown(). Additionally, it ensures that in-flight operations (like backups and checkpoints) are safely awaited before destruction, and that thread-affine N-API references are cleaned up safely. 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.94K ops/sec	41.77	39.80	1,886.019	0.138	119,706
🥈 rocksdb	2	10.30K ops/sec	97.11	93.34	24,057.195	0.972	51,486

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

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	27.67K ops/sec	36.15	34.82	538.799	0.108	138,329
🥈 rocksdb	2	10.49K ops/sec	95.37	91.88	610.244	0.050	52,426

ranges.bench.ts

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

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	25.53K ops/sec	39.17	35.81	1,993.039	0.294	127,659
🥈 rocksdb	2	15.67K ops/sec	63.80	56.34	1,121.35	0.119	78,373

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	338.72 ops/sec	2,952.271	110.053	70,710.142	22.82	678
🥈 lmdb	2	26.35 ops/sec	37,946.363	429.727	1,215,067.958	136.3	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	38.65K ops/sec	25.87	11.99	20,829.827	0.842	193,273
🥈 lmdb	2	440.34 ops/sec	2,270.948	239.641	27,258.768	1.55	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	737.12K ops/sec	1.36	1.16	5,042.571	0.209	3,685,590
🥈 lmdb	2	430.95K ops/sec	2.32	1.33	7,042.059	0.549	2,154,750

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	855.36 ops/sec	1,169.104	1,010.818	1,910.376	0.283	1,711
🥈 lmdb	2	1.14 ops/sec	874,242.212	825,381.715	951,517.939	3.64	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	23.34K ops/sec	42.85	29.66	20,336.246	2.07	46,672
🥈 lmdb	2	808.49 ops/sec	1,236.868	55.24	14,550.659	5.40	1,617

---

Results from commit 347546f

[cb1kenobi]

Reviewed f24ef7a5 — no issues found. This PR looks good, nice job!

Re-review of the one new commit since b35ad3f7 ("Address remaining lifecycle review threads"). Both previously-open findings are confirmed fixed in the code, not just marked resolved:

• db_iterator.cpp:289 (Medium, per-row getenv) — fixed. The lookup is hoisted into initializeTestSeams(), which runs as the first statement of NAPI_MODULE_INIT, and Next() now does a relaxed atomic load. Verified at the object-code level: ROCKSDB_JS_ITERATOR_NEXT_DELAY_MS no longer appears in db_iterator.o (only in binding.o), and the compiled DBIterator::Next contains zero getenv calls. The new ROCKSDB_JS_COUNT_DELAY_MS seam got the same treatment up front.
• db_registry.cpp:960/961 (Medium, closeError asymmetry) — fixed. The four copies of the finishClose() → erase-or-quarantine → notify → emit tail are collapsed into closeClaimedDescriptors(), and the policy that had drifted is now one named, documented option (failOnCompletedWithError). The gate reduces exactly to the old DestroyDB behavior when false and the old unconditional behavior when true, so the refactor is behavior-preserving while making the remaining asymmetry deliberate rather than accidental.
• The earlier getCount Low is also addressed: countRemaining() polls isClosing() per row and reports the abort instead of a partial count, on both the database and transaction paths, and the inaccurate "compaction is the only unbounded in-flight op" comment is corrected.

Also checked and cleared: the dropped if (condition) null guard in PurgeIfUnreferenced is safe (both DBRegistryEntry constructors make_shared the condition; the old guard only mattered because the notify used to sit outside the if (descriptor) block); the newly-added closeRetrying = false on the PurgeAll/PurgeIfUnreferenced quarantine paths is a no-op, since only DestroyDB/Shutdown ever latch it and beginClose() is single-shot.

Verification: full suite 55 files, 773 passed / 1 skipped / 0 failed; targeted destroy + ranges 64/64 including the new aborts an in-flight getCount() when a foreign destroy begins fixture. CI green on the head (Windows jobs still pending at review time).

One merge-ordering note, not a defect in this PR: finishClose() still holds txnsMutex across cancelForDB(), which takes writerMutex_, and this PR makes that a routine path because destroy() now force-closes every descriptor. If #744 lands with PurgeIfUnreferenced still on the wake callback path, its lock-order inversion becomes materially more likely — worth sequencing #744's fix before or with this.

—
Generated by Barber AI

[cb1kenobi]

Reviewed 542b058a — no issues found. This PR looks good, nice job!

Re-review of the one new commit since f24ef7a5 (the branch was rebased onto main after rocksdb-js#744 merged; verified via git range-diff that all 35 prior commits carried forward unchanged modulo rebase context, with commit 542b058a new at the tip).

542b058a fixes a real race: compactCancelRequested now stays armed for finishClose()'s whole duration (an async compact-on-close pass opts out via a new cancellable param instead), and Transaction::GetCount now takes an OperationGuard + isClosing() check so finishClose()'s drain can't return early and let the closables sweep roll back the transaction mid-scan. Both changes are consistent with the existing OperationGuard/ACQUIRE_OPERATIONS_LOCK pattern elsewhere in the codebase.

Also re-verified at object-code level:

• DBIterator::Next() has zero getenv calls in its compiled disassembly (ROCKSDB_JS_ITERATOR_NEXT_DELAY_MS is absent from db_iterator.o's string table); the seam is now a relaxed atomic load, set once in initializeTestSeams().
• closeClaimedDescriptors() remains the single teardown tail for all four callers (CloseDB, DestroyDB, PurgeAll, Shutdown), with the completed-but-errored policy as the named ClaimedCloseOptions.failOnCompletedWithError option — false only for destroy(), true (fatal) everywhere else.
• finishClose() still takes txnsMutex and holds it across cancelForDB(), which itself takes VT's writerMutex_ — the txnsMutex → writerMutex_ ordering is unchanged by rocksdb-js#744 merging.

pnpm test (destroy.test.ts lifecycle suite: 19/19), pnpm test:native (148/148, 3 expected macOS skips), and pnpm check all pass clean at this head.

—
Generated by Barber AI

[cb1kenobi]

Please rebase with main and resolve the merge conflicts.

[cb1kenobi]

Reviewed ba0e1c0 and found no blocking issues. No new blocking defects were confirmed on changed lines. Existing findings were not repeated.

—
Generated by Barber AI

[cb1kenobi]

This was a big one, but it looks great! I love the OperationGuard!