Serialize database destruction with concurrent opens

[kriszyp]

Summary

Harper currently works around a rocksdb-js lifecycle race by locking root opens in JavaScript. This moves the invariant into the native registry: physical destruction now owns a database path across read-write/read-only descriptors, concurrent opens wait, and shutdown is serialized with both operations.

The change also makes teardown failures observable and recoverable, and prevents destroy/shutdown from releasing the native database beneath directory backups, streaming backups, or checkpoints.

For the human reviewer

• destroy() intentionally changes from refusing while peer handles exist to closing every in-process handle for the physical path before removing it. This is what allows Harper schema propagation to race safely with a database drop. Cross-process coordination remains RocksDB's lock responsibility.
• A failed native close quarantines the path and emits database:closeFailed. A failed post-destroy directory cleanup is visible in registryStatus() and can only be retried by the explicit destructive verbs, destroy() or shutdown(); open() remains non-destructive.
• Close-time flush/compaction errors are reported after native teardown completes. This can surface an error from close() in a finally, but silently ignoring a failed flush would hide possible data loss.
• Destroy waits for registered in-flight backups/checkpoints before teardown. Streaming backups poll the closing state so their JS backpressure handshake cannot deadlock the synchronous destroy.
• The registry and path gates are process-global. Raw path spellings are not canonicalized, matching the existing registry key behavior.
• Cross-thread handle close can still race an owner-thread close on the same DBHandle; shutdown already exercised that path before this change. N-API reference deletion is now owner-thread-only, and worker lifecycle fixtures pass, but the remaining shared-handle synchronization is a follow-up decision for the storage maintainer.
• New in the 2026-08-26 rebase (below): waitForAsyncWorkCompletion() is now unbounded (async.h#L189), replacing the previous 5s bound. That was a deliberate fix for a genuine UAF (a flush legitimately waiting out a write stall could still be executing when the old bound gave up and finishClose() proceeded to release the native DB) — but it means any future call site that admits work via registerAsyncWork()/admitAsyncWorkOrReject() and then fails to release that claim on every exit path (including rare N-API failures) now hangs the handle's close forever, not just for 5s. queueAsyncWorkOrReject() (async.h#L274) closes the one gap the pre-push review found in this repo's own call sites; the contract itself (every registration must be balanced on every exit path, forever) is a standing invariant future changes need to keep honoring, which is why AGENTS.md invariant 17 documents it explicitly.

This is the rocksdb-js root-cause fix for Harper PR #2169, "Prevent job wedges on runtime database opens". Harper's JavaScript .open lock should remain out of the released path once a package containing this change is available.

2026-08-26 rebase + async-work lifecycle repair

Rebased onto latest origin/main (no textual conflicts affecting this PR's own commits). Also implemented the async-work admission/cancellation repair called out as a known gap by the pre-push review of the prior head: AsyncWorkHandle::registerAsyncWork()/cancelAllAsyncWork() now share a mutex so a registration racing a close either fully lands before cancellation publishes or is refused (async.h#L222); every Flush/Compact/Clear/async Get/backup/checkpoint/commit call site rejects cleanly on refusal instead of proceeding into a closing handle (database.cpp). A follow-on pre-push review round (Gemini + Harper-domain) then found and this fixed a real leak the first pass introduced: every admitAsyncWorkOrReject() call site followed successful admission with a bare napi_queue_async_work() that, on the rare N-API queue failure, threw and returned without releasing the just-admitted claim — a stuck count that, combined with the now-unbounded wait above, would block that handle's close (and every later OpenDB() for its path) forever. backup_stream.cpp's state is refcounted and shared with an N-API tsfn, so it got a hand-written equivalent rather than the generic helper (which would double-free there) — see backup_stream.cpp#L721.

Findings surfaced by this round's review that are outside this PR's diff (pre-existing on the branch before today, from earlier commits) — not fixed here, flagged for separate triage:

• transaction.cpp:765 — an async commit()'s state = Committing write isn't restored on the ERR_TRANSACTION_CLOSING refusal path the way the coordinated-retry paths restore it, so a commit that raced a path destroy can leave the handle reporting Committing after it was actually rolled back; a caller retrying commit() per this repo's own documented pattern gets a resolved promise for a transaction that never landed.
• database.cpp (Destroy) — a failed physical delete leaves a tombstone that blocks every later OpenDB() for the path, and if the caller drops their RocksDatabase instance, there is no in-process way to retry destroy() to clear it (the native handle only populates path on a successful open()).
• db_registry.cpp Shutdown() — a closeError captured from one database can be silently discarded if a later wait for a different database times out, downgrading a possible-data-loss signal to a background event.
• db_registry.cpp — destroy-cleanup tombstones omit transactionDetails, which RegistryStatusDB declares required.
• db_iterator.cpp:284 — a per-row mutex added by an earlier commit on this branch (Serialize iterators with forced teardown) taxes the hot read path for a window that only matters during a foreign forced close.
• db_descriptor.cpp — compactCancelRequested (the manual-compaction cancel token that keeps finishClose()'s untimed drain bounded) has no direct test, unlike its count-scan sibling.

Verified: native pnpm test:native 151/151; pnpm check (type-check/lint/format) clean; targeted vitest runs of every path this repair touches (clear/compact/flush/get/backup/backup-stream/checkpoint/transactions/cross-column-family/orphan-gc) all passing. A full pnpm test run was attempted but this session hit sustained extreme contention on the shared build machine (uptime load averages up to 34 on 20 cores from concurrent, unrelated sessions) that produced non-reproducible timeouts unrelated to this diff — confirmed by an extended-timeout A/B (--testTimeout 300000) showing identical behavior with and without the changes under review, and by targeted runs completing cleanly once isolated. See the dispatch log for the full trace.

Verification

• node_modules/.bin/node-gyp build
• node_modules/.bin/tsc --noEmit
• node_modules/.bin/oxlint
• node_modules/.bin/oxfmt --check
• Full JavaScript suite: 768 passed, 1 skipped (prior head, before today's rebase)
• Native suite: 151 passed
• Targeted suite (today's diff's blast radius): 235 passed, 1 skipped

Review coverage

• Full and incremental reviews by Claude, Codex, Gemini, Cursor Grok, Cursor Composer, and Harper storage-domain adjudication.
• Review-found blockers fixed: shutdown no longer abandons healthy databases; destroy-cleanup tombstones are explicit and recoverable; directory backups participate in teardown accounting; cancelled queued operations release their in-flight claims; close-failure events cover every registry teardown path; async-work admission races a close cleanly; a queue-failure after successful admission no longer strands a claim.
• Human decisions remain around force-destroy semantics, synchronous wait behavior, the pre-existing concurrent close race described above, and the outside-this-diff findings listed under "2026-08-26 rebase" above.

— Claude Sonnet 5 (2026-08-26 rebase + async-work repair), GPT-5 Codex (original)

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

_{Human-Review-Need: 4 (decisions: shutdown-throws-vs-reports, quarantine-blocks-reopen, untimed-drain-semantics, destroy-waits-for-backup, opened-means-not-closing, lifecycle-wait-seconds-global, destroy-requires-opened-handle, iterator-mutex-per-next) @ 107b216}

[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	24.23K ops/sec	41.26	39.90	602.79	0.114	121,171
🥈 rocksdb	2	10.60K ops/sec	94.31	90.33	31,082.921	1.22	53,015

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

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	27.84K ops/sec	35.92	34.22	1,341.177	0.115	139,209
🥈 rocksdb	2	10.40K ops/sec	96.16	93.52	582.487	0.050	51,996

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.55K ops/sec	39.14	35.56	1,938.421	0.290	127,734
🥈 rocksdb	2	15.84K ops/sec	63.13	55.10	1,077.677	0.114	79,205

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	349.62 ops/sec	2,860.238	64.06	72,271.77	18.33	700
🥈 lmdb	2	26.62 ops/sec	37,563.361	43.27	1,184,568.338	136.987	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	37.28K ops/sec	26.83	11.84	21,066.264	0.856	186,387
🥈 lmdb	2	446.48 ops/sec	2,239.738	183.537	12,834.985	1.29	2,233

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	735.64K ops/sec	1.36	1.18	5,002.85	0.207	3,678,184
🥈 lmdb	2	454.95K ops/sec	2.20	1.25	7,191.564	0.557	2,274,731

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	838.94 ops/sec	1,191.976	1,034.303	2,365.927	0.322	1,678
🥈 lmdb	2	1.18 ops/sec	850,628.539	781,897.215	905,098.093	3.24	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	22.73K ops/sec	43.99	29.71	20,391.147	2.08	45,468
🥈 lmdb	2	834.32 ops/sec	1,198.578	263.568	20,046.395	5.26	1,669

---

Results from commit ae746b7

[cb1kenobi]

Low: getCount() is the remaining unbounded OperationGuard holder — the cancellation added for compactRange does not cover it

The new compactCancelRequested handshake (db_descriptor.cpp:308/:1822) fixes the compactSync() half of this: a manual compaction now aborts when finishClose() starts its untimed drain. getCount() is the other half and is untouched.

This guard is held across the full-range scan on line 949 (while (itHandle->iterator->Valid()) { ++count; itHandle->iterator->Next(); }), which has no isClosing() check and no cancellation. finishClose() still drains with a bare operationsInFlight.wait() (db_descriptor.cpp:307), so a getCount() over a large range blocks destroy()/shutdown() for its full duration — and the blocked thread holds the path gate, so concurrent OpenDB() calls for that path time out with Timed out opening database ... destruction is still in progress.

The comment added at db_descriptor.cpp:304 states compaction is "the one in-flight op that can run unboundedly", which is not quite right while this scan exists.

Suggested fix: break the loop when descriptor->isClosing() becomes true (returning an aborted status rather than a partial count), so the guard is released promptly. Then the comment at db_descriptor.cpp:304 holds.

—
Generated by Barber AI

[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]

High: full-suite process exit reproducibly fails in DBRegistry::Shutdown() -- native I/O error + a raw pthread lock: Invalid argument diagnostic

Ran pnpm test (full suite, CI=1, --expose-gc) twice against this head (3cdd9f94) in a clean worktree. Both times, after all 838/839 tests reported passing, the process printed at exit:

rocksdb-js database registry cleanup failed: Failed to flush database during close: IO error: While open a file for appending: /.../rocksdb-js-tests/testdb-<hash>/000010.log: Permission denied
pthread lock: Invalid argument
[ELIFECYCLE] Test failed. See above for more details.

000010.log and Permission denied were consistent across both runs (different temp-dir hashes, same log-file number and same error). This comes from the cleanup("database registry", ...) wrapper in binding.cpp around DBRegistry::Shutdown(). The second line, pthread lock: Invalid argument, is not a C++ exception -- it's the raw diagnostic macOS libpthread prints when a mutex/condvar operation is attempted on an invalid or already-destroyed primitive, printed after the caught-and-reported flush failure, i.e. during continued teardown of this function or a subsequent one in the same cleanup chain.

Ran the same full suite against the true merge base (06961b5f, main without this PR) in an identical clean worktree: 0/1 runs showed this -- clean exit, no stderr noise, exit 0. Shutdown() itself is substantially rewritten by this PR (this diff hunk), including the new shutdownMutex / lifecycleCondition / destroyingPaths machinery that didn't exist on main. Narrowing to just test/shutdown.test.ts + test/destroy.test.ts + test/concurrent-teardown.test.ts alone does NOT reproduce it (clean exit in 24s) -- it requires the full 61-file suite's cumulative state, i.e. some earlier, unrelated test leaves a database registered but unflushed, and only the final process-wide Shutdown() pass (this PR's new code) touches it and fails.

Confidence: high that this is a real, reproducible regression on this branch (2/2 dirty on head, 1/1 clean on base, isolated repro attempt inconclusive due to needing full-suite state). Not yet bisected to the exact introducing hunk or the specific earlier test that leaves the dangling descriptor -- that would need either a bisection across the 61 test files or added logging in Shutdown()/finishClose() to identify which path is still registered at final teardown.

Suggested next step: reproduce with ROCKSDB_JS_DEBUG/DEBUG_LOG enabled around Shutdown()'s descriptor enumeration to log which path is still in instance->databases at process exit, and check whether the OS temp-file cleaner or a concurrent test's destroy() on an overlapping path could be racing this database's directory at that moment (the new destroyingPaths set is meant to prevent exactly this kind of overlap -- worth checking whether Shutdown()'s own path is exempted from it).

—
Generated by Barber AI

[cb1kenobi]

Medium: duplicate invariant 17. — already failing CI's Check job

This branch's own new invariant (632: **Async-work admission and cancellation share one mutex...**) and the invariant that arrived via #780's merge into main (654: **An env's pending transactions are reaped by its cleanup hook...**) are both numbered 17.. Confirmed in both your branch tip and GitHub's own refs/pull/787/merge content — this isn't a stale-local-clone artifact.

This is not just a cosmetic nit: it's already red. The Check job (pnpm fmt:check → oxfmt --check) is failing right now on ba0e1c07 specifically because of this file (verified against the actual CI run and reproduced locally with oxfmt --check / oxfmt AGENTS.md). oxfmt does auto-renumber ordered-list items in Markdown — running it without --check rewrites the second 17. to 18. — which also means the doc note just added a few lines above this ("oxfmt formats TS/JS/JSON only. It does not touch C++ or Markdown") is itself incorrect; oxfmt clearly does reformat this file's Markdown lists. No source file references "invariant 17" by number today, so renumbering to 18. is a clean, safe fix with no cross-references to chase.

Suggested fix:

Suggested change

	17. **An env's pending transactions are reaped by its cleanup hook — never by `DBHandle::close()`**:
	18. **An env's pending transactions are reaped by its cleanup hook — never by `DBHandle::close()`**:

—
Generated by Barber AI

[cb1kenobi]

Low: the compaction-cancel-timing fix in this commit has no regression coverage

Moving the compactCancelRequested arm from finishClose() into beginClose() closes a real race window (a manual compactRange() starting between close-claim and the old, later arm point wouldn't be told to cancel promptly). But mutation-testing it shows the suite doesn't enforce the new timing: I reverted this exact change (armed the token back in finishClose() instead, matching the pre-this-commit behavior) in a clean rebuild and ran compaction.test.ts, destroy.test.ts, shutdown.test.ts, concurrent-teardown.test.ts, and db-options.test.ts — 66/66 still green. Nothing currently exercises a manual compactRange() in flight at the moment beginClose()/DestroyDB()/Shutdown() claims the descriptor, so this specific ordering guarantee could regress silently.

Suggested fix: a test that starts a manual (non-cancellable) compactRange() on one thread, then triggers destroy()/shutdown() from another the moment the compaction begins (mirroring the existing test_seam.h delay-based race harnesses already used elsewhere in this PR, e.g. countScanDelayMsFlag), asserting the close completes promptly instead of blocking for the compaction's full duration.

—
Generated by Barber AI