From dfd49b4bdf0472e9feaa3ad49635fe722b3f017a Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sat, 1 Aug 2026 13:07:10 -0600 Subject: [PATCH 01/19] fix(transaction): bound the coordinated-retry park with a timeout A commit that loses a conflict under coordinatedRetry parks on the conflicting holder's VT LockTracker and only resolved RETRY_NOW when that lock's last holder released. If the holder never releases (a leaked/ abandoned transaction, or a wake lost to the #741 double-release corruption), the commit promise never settled -- in production (harper#2001) a worker's write path was disabled for 5+ hours. Add a bounded wait (ROCKSDB_JS_PARK_TIMEOUT_MS, default 3000ms) that resolves RETRY_NOW even if the holder never releases. The timeout and the wake callback race through a shared atomic flag (independent of the per-park heap state so a late loser never touches memory the winner may have already freed), guaranteeing exactly-once resolve regardless of which fires first. Refs #741 Co-Authored-By: Claude Opus --- src/binding/transaction/transaction.cpp | 56 ++++++++++++++++++++++--- test/lock-tracker.test.ts | 54 ++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 6 deletions(-) diff --git a/src/binding/transaction/transaction.cpp b/src/binding/transaction/transaction.cpp index 655d97a9f..52e9ef095 100644 --- a/src/binding/transaction/transaction.cpp +++ b/src/binding/transaction/transaction.cpp @@ -1,6 +1,8 @@ +#include #include #include #include +#include #include #include "database/database.h" #include "database/db_descriptor.h" @@ -214,6 +216,22 @@ static void retryNowFinalize(napi_env env, void* finalizeData, void* /*hint*/) { delete ctx; } +/** + * Bounded wait (ms) for a coordinated-retry commit parked on a conflicting + * holder's VT lock, selected by ROCKSDB_JS_PARK_TIMEOUT_MS. If the holder + * never releases (leaked/abandoned transaction, or a wake lost to #741's + * double-release corruption), the park resolves RETRY_NOW anyway once this + * elapses instead of hanging forever (harper#2001). A spurious early + * RETRY_NOW is harmless -- the JS layer just retries and may conflict again. + */ +static unsigned parkTimeoutMs() { + static const unsigned ms = []() -> unsigned { + const char* v = ::getenv("ROCKSDB_JS_PARK_TIMEOUT_MS"); + return v != nullptr ? static_cast(::atoi(v)) : 3000; + }(); + return ms; +} + /** * State for the `Commit` async work. */ @@ -439,16 +457,42 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { ); ::napi_unref_threadsafe_function(env, tsfn); + // Exactly-once gate shared between the wake path and the timeout path + // below. Lives in its own heap allocation independent of `ctx` (which + // the winning side's release eventually frees via retryNowFinalize) so + // the losing side -- which may run minutes later, from any thread, long + // after the winner's ctx/tsfn are gone -- never dereferences memory that + // may already be freed; it just loses the CAS and touches nothing. + auto fired = std::make_shared>(false); + auto fireOnce = [tsfn, fired]() { + bool expected = false; + if (fired->compare_exchange_strong(expected, true)) { + ::napi_call_threadsafe_function(tsfn, nullptr, napi_tsfn_nonblocking); + ::napi_release_threadsafe_function(tsfn, napi_tsfn_release); + } + }; + // Register wake callback; if the tracker already fired wake() // before we got here, addWakeCallback returns false and we // call+release the TSFN immediately (async on the JS thread). - bool registered = t->addWakeCallback([tsfn]() { - ::napi_call_threadsafe_function(tsfn, nullptr, napi_tsfn_nonblocking); - ::napi_release_threadsafe_function(tsfn, napi_tsfn_release); - }); + bool registered = t->addWakeCallback(fireOnce); if (!registered) { - ::napi_call_threadsafe_function(tsfn, nullptr, napi_tsfn_nonblocking); - ::napi_release_threadsafe_function(tsfn, napi_tsfn_release); + fireOnce(); + } else { + // #741 bound: park behind a holder that never releases (leaked/ + // abandoned transaction, or a wake lost to the double-release + // corruption tracked in #741) would otherwise never settle + // (harper#2001). This detached wait races fireOnce with the wake + // callback above; whichever runs first wins the CAS and the other + // is a safe no-op. Not tied to `env`'s loop: a plain sleep thread + // (not a uv_timer) keeps this addon's ABI stable across Node + // versions -- libuv's internal struct layout is not part of the + // N-API surface this addon otherwise sticks to. + unsigned timeoutMs = parkTimeoutMs(); + std::thread([fireOnce, timeoutMs]() { + std::this_thread::sleep_for(std::chrono::milliseconds(timeoutMs)); + fireOnce(); + }).detach(); } vt->unrefTracker(t); diff --git a/test/lock-tracker.test.ts b/test/lock-tracker.test.ts index f616ab7a0..032dbe409 100644 --- a/test/lock-tracker.test.ts +++ b/test/lock-tracker.test.ts @@ -168,6 +168,60 @@ describe('Coordinated retry (Phase 3)', () => { })); }); +// Regression coverage for #741: a coordinated-retry commit that loses a +// conflict parks on the conflicting holder's VT lock and, before this fix, +// only resolved when that lock's LAST holder released -- if the holder is a +// leaked/abandoned transaction (or the wake is lost), the commit's promise +// never settled (harper#2001: a worker's write path disabled for 5+ hours). +// A parked commit must now also resolve RETRY_NOW after a bounded wait. +describe('Coordinated retry — bounded park timeout (#741)', () => { + it('a commit parked behind a never-releasing holder settles with RETRY_NOW within the deadline', () => + dbRunner({ dbOptions: [{ encoding: false, verificationTable: true }] }, async ({ db }) => { + const key = Buffer.from('park-timeout-abandoned-holder'); + const v0 = 1.6e12; + await db.put(key, valueWithVersion(v0)); + // Materialize the VT slot -- without an initial populateVersion the + // slot starts at 0 and lockSlotForWrite still installs a lock either + // way, but this matches the production shape (a live, cached key) + // and is what the local repro found necessary to actually exercise + // the park path instead of silently no-op'ing. + db.populateVersion(key, v0); + expect(db.verifyVersion(key, v0)).toBe(true); + + // Abandoned holder: stages a write (installing the VT lock on the + // key's slot) and is deliberately never committed or aborted, with + // its reference retained for the lifetime of the test -- this is the + // "leaked/abandoned transaction" scenario from harper#2001. Its + // LockTracker holder count therefore never reaches zero and wake() + // is never called for anything parked on it. + const holder = new Transaction(db.store, { coordinatedRetry: true }); + holder.putSync(key, valueWithVersion(2.1e12)); + + // Establish txn's snapshot with a plain (non-VT) read before the + // conflicting external commit below, so RocksDB's optimistic conflict + // check has something to validate against. + const txn = new Transaction(db.store, { coordinatedRetry: true }); + await txn.get(key); + + // A write committed from outside txn bumps the key's sequence past + // txn's snapshot, so txn's own write below will conflict at commit. + await db.put(key, valueWithVersion(2.2e12)); + + // txn's write joins the VT lock the abandoned holder already + // installed on the same slot (same key -> same slot). + txn.putSync(key, valueWithVersion(2.3e12)); + + const start = Date.now(); + const result = await txn.commit(); + const elapsed = Date.now() - start; + + expect(result).toBe(RETRY_NOW); + // Prior to the bounded park this hangs until env teardown (45s+ in + // the local repro); it must now settle well inside that. + expect(elapsed).toBeLessThan(8000); + })); +}); + // Regression coverage for the VT-fast-path / optimistic-snapshot interaction. // A read satisfied entirely from the Verification Table (returning // FRESH_VERSION_FLAG, skipping the RocksDB read) must STILL establish the From 15bbcb772755ec80f8423dda2978b5aabef8d431 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sat, 1 Aug 2026 13:55:45 -0600 Subject: [PATCH 02/19] fix(transaction): move park timeout to a descriptor-owned thread Addresses BLOCK findings from cross-model pre-push review of cc3c2052 (codex+gemini+grok+harper-domain, independent=true): - blocker: a detached std::thread per park was an unbounded resource cliff on exactly the contention/abandoned-holder path parks are dense on. Replaced with one park-timeout thread per DBDescriptor (lazily started, joined at finishClose() like commitWorker) tracking every outstanding deadline -- DBDescriptor::scheduleParkTimeout / runParkTimeoutLoop / fireParkTimeout in db_descriptor.{h,cpp}. - blocker: the detached thread's tsfn call could race Node freeing a torn-down worker env's threadsafe functions (AGENTS.md: "a per-commit tsfn acquire is NOT sufficient -- env teardown does not honor tsfn acquire counts"). Each park is now tracked by env in the descriptor and releaseParkTimeoutsByEnv cancels a dying env's pending parks from the same module env-cleanup hook that already scrubs commit completions (DBRegistry::ReleaseParkTimeoutsByEnv, wired in binding.cpp), releasing the tsfn without calling it. - major (unchecked std::thread ctor throw -> process abort): moot with a single lazily-started thread per descriptor instead of one per park. - minor: ROCKSDB_JS_PARK_TIMEOUT_MS now parsed with strtoul + explicit negative/overflow rejection instead of atoi, so a malformed value falls back to the safe default instead of silently reintroducing an unbounded hang (negative wrapping through unsigned) or an immediate- fire spin (non-numeric -> 0). - major (test couldn't distinguish the timeout branch from the already-existing !parked fast path): added a lower bound (elapsed >= 2500ms) alongside the upper bound. - nit: trimmed comments that narrated mechanics/addressed the reviewer. Not fixed in this pass (documented in AGENTS.md as a known gap): LockTracker::wakeCallbacks has no removal API, so a permanently- abandoned holder's wake registrations still accumulate across retries for the life of the incident -- deferred rather than risk an unreviewed change to verification_table.cpp's concurrency invariants. Also not fixed: a timeout-triggered RETRY_NOW consumes a maxRetries attempt like any other, so a legitimately slow (not abandoned) holder can exhaust the retry budget before it would have woken naturally -- an accepted tradeoff per the original task ("a spurious early RETRY_NOW is harmless"), flagged for follow-up in the PR description. Co-Authored-By: Claude Opus --- AGENTS.md | 48 +++++++++ src/binding/binding.cpp | 5 + src/binding/database/db_descriptor.cpp | 138 ++++++++++++++++++++++++ src/binding/database/db_descriptor.h | 76 +++++++++++++ src/binding/database/db_registry.cpp | 27 +++++ src/binding/database/db_registry.h | 1 + src/binding/transaction/transaction.cpp | 58 ++++++---- test/lock-tracker.test.ts | 38 ++----- 8 files changed, 340 insertions(+), 51 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7daf60f7f..2444bdced 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -193,6 +193,10 @@ sufficient (env teardown does not honor tsfn acquire counts); see each completion callback (widens teardown race windows) - `ROCKSDB_JS_TXN_GET_DELAY_MS` - Test-only: delay a transaction's cold-cache async get before it reads (exercises orphan cleanup past the async-work wait timeout) +- `ROCKSDB_JS_PARK_TIMEOUT_MS` - Bounded wait (default `3000`) before a + coordinated-retry commit parked on a conflicting holder's VT lock resolves + RETRY_NOW unconditionally, in case the holder never releases (see + "Coordinated retry" note below) ## Test Structure @@ -411,6 +415,50 @@ sufficient (env teardown does not honor tsfn acquire counts); see if a zero were taken as a terminator, would let a chain "end" anywhere in megabytes of padding. Resolve it only on a break — `getLogFileSize` crosses into native and takes the store mutex, so a per-frame call would tax every healthy read. +12. **Coordinated retry parks on a lock, bounded by a descriptor-owned timeout**: a `coordinatedRetry` + commit that loses a conflict (`IsBusy`) parks instead of rejecting immediately — + `completeCommitWork` (`src/binding/transaction/transaction.cpp`) registers a wake callback on the + conflicting VT slot's `LockTracker` (via `addWakeCallback`) and resolves `RETRY_NOW` only when + that lock's last holder releases (`VerificationTable::releaseWriteIntent` → `LockTracker::wake`). + A holder that never releases — a leaked/abandoned transaction, or a wake lost to a bug elsewhere — + would otherwise park forever (harper#2001: a worker's write path disabled for 5+ hours until + restart). `DBDescriptor::scheduleParkTimeout` (`db_descriptor.{h,cpp}`) bounds this with + `ROCKSDB_JS_PARK_TIMEOUT_MS` (default `5000` — the top of this fix's requested 2-5s range, to + leave maximum headroom for a holder that is merely slow rather than abandoned, since a timeout + consumes a `coordinatedRetry` attempt exactly like a real wake does and `maxRetries` is finite): + one park-timeout thread **per descriptor** — lazily started, joined at `finishClose()` (and again, + idempotently, from the destructor as a safety net, matching `commitWorker`) — tracks every + outstanding deadline instead of spawning a thread per park (the contention path is exactly where + an abandoned holder makes parks dense, so per-park threads would be a resource cliff, not a fix). + Each park is identified by a monotonic `uint64_t id`, not its entry's address: `LockTracker::wakeCallbacks` + has no removal API (see the gap noted below), so a stale closure can outlive its entry, and an + address-keyed lookup risks resolving a _different_, later park that reused the same freed heap + address. The timeout thread and the LockTracker wake callback race through one heap-allocated + `std::atomic` per park (independent of the per-park `RetryNowContext`, whose refs/TSFN the + winning side's release eventually frees) — whichever fires first calls+releases the TSFN under + `parkTimeoutMutex` and erases the entry; the loser finds it already gone and touches nothing. That + same mutex is what a dying env's `releaseParkTimeoutsByEnv` (wired into the module env-cleanup + hook next to `ReleaseCommitCompletionsByEnv`) takes to cancel — release without calling — that + env's pending parks before Node frees their tsfns; `retryNowCallJs` also guards `env == nullptr` + like `commitCompletionCallJs` does, for the same tsfn-queue-drained-during-teardown reason. The + LockTracker wake closure captures a **`std::weak_ptr`**, not a raw pointer: a park + can end up registered on a tracker installed by a _different_ database on a colliding VT slot + (`VerificationTable::lockSlotForWrite` joins an existing tracker without retagging its `dbId`), so + that lock's eventual release wakes a park whose own descriptor may have already closed and been + destroyed — `cancelForDB()` only wakes trackers tagged with _its own_ `vtEpoch`, so it cannot be + relied on to have resolved a foreign-`dbId` park before the descriptor goes away. `.lock()` failing + is the expected outcome once that happens: `shutdownParkTimeouts()` (called from `finishClose()` + right after `cancelForDB`, before the descriptor can be destroyed) unconditionally resolves every + park still in its own `parkTimeouts` regardless of whether the real holder ever wakes it, so by the + time the weak reference can fail to lock, the park has already settled. Deliberately a plain + `std::thread`, not a `uv_timer_t`: this addon ships one prebuilt binary across Node ABI versions via + N-API, and libuv's struct layout is not part of that stable surface. Known gap: `LockTracker::wakeCallbacks` + itself has no removal API. Before this change an abandoned holder accrued one inert callback per + waiter and then everything hung; now each waiter re-parks (and re-registers) every + `ROCKSDB_JS_PARK_TIMEOUT_MS` up to `maxRetries`, so registrations accumulate per _retry_ rather than + per incident for as long as it lasts (each is inert once its own park resolves, so this is a + memory-growth concern, not a correctness one) — deferred rather than risking an unreviewed change to + `verification_table.cpp`'s concurrency invariants under this fix's scope. 12. **A dropped transaction must release itself**: `DBDescriptor::transactionAdd` holds a **strong** `shared_ptr` (the parallel `closables` entry is weak), so the registry alone keeps a diff --git a/src/binding/binding.cpp b/src/binding/binding.cpp index b799a05f3..0b88ba46d 100644 --- a/src/binding/binding.cpp +++ b/src/binding/binding.cpp @@ -201,6 +201,11 @@ NAPI_MODULE_INIT() { // tsfns, so the shared commit thread stops marshalling into a torn-down // env (mirrors the listener cleanup above). rocksdb_js::DBRegistry::ReleaseCommitCompletionsByEnv(dyingEnv); + // Same reasoning for a coordinated-retry commit parked on a VT lock: + // cancel this env's pending park timeouts before Node frees their + // tsfns, so the descriptor's park-timeout thread never fires into a + // torn-down env. + rocksdb_js::DBRegistry::ReleaseParkTimeoutsByEnv(dyingEnv); int32_t newRefCount = --moduleRefCount; if (newRefCount == 0) { diff --git a/src/binding/database/db_descriptor.cpp b/src/binding/database/db_descriptor.cpp index 4fbcffa35..4102c8cb1 100644 --- a/src/binding/database/db_descriptor.cpp +++ b/src/binding/database/db_descriptor.cpp @@ -475,6 +475,15 @@ void DBDescriptor::finishClose() { } } + // Stop and join the park-timeout thread now that cancelForDB() has woken + // every real holder above -- any entries still pending at this point + // belong to envs that never got a chance to release their own park + // (already reachable via releaseParkTimeoutsByEnv, not this shutdown + // path), so nothing here needs to fire them, only stop the thread and + // let the destructor's guarantee (park entries erased == no live handle + // left) hold. + this->shutdownParkTimeouts(); + // Unregister from transaction log store registry - this will clean up stores // when the last descriptor for this path is closed TransactionLogStoreRegistry::Unregister(this->path); @@ -565,6 +574,135 @@ void DBDescriptor::releaseCommitCompletionsByEnv(napi_env env) { } } +DBDescriptor::ParkTimeout* DBDescriptor::scheduleParkTimeout( + napi_env env, + unsigned timeoutMs, + napi_threadsafe_function tsfn, + std::shared_ptr> fired +) { + std::lock_guard lock(this->parkTimeoutMutex); + if (this->parkTimeoutStopped) { + // Descriptor already closing: cancelForDB() (finishClose(), before + // this point) already woke every real holder this descriptor owns, + // so a park reaching here has nothing left to wait for -- the caller + // resolves inline instead of registering with a thread we're not + // going to start again. + return nullptr; + } + auto entry = std::make_unique(); + entry->deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(timeoutMs); + entry->env = env; + entry->tsfn = tsfn; + entry->fired = std::move(fired); + ParkTimeout* raw = entry.get(); + this->parkTimeouts.push_back(std::move(entry)); + if (!this->parkTimeoutThreadStarted) { + this->parkTimeoutThreadStarted = true; + this->parkTimeoutThread = std::thread([this]() { this->runParkTimeoutLoop(); }); + } + this->parkTimeoutCv.notify_all(); + return raw; +} + +void DBDescriptor::runParkTimeoutLoop() { + setThreadName("rocksdb-park-timeout"); + std::unique_lock lock(this->parkTimeoutMutex); + for (;;) { + if (this->parkTimeoutStopped) { + return; + } + if (this->parkTimeouts.empty()) { + this->parkTimeoutCv.wait(lock); + continue; + } + auto earliest = std::min_element( + this->parkTimeouts.begin(), + this->parkTimeouts.end(), + [](const std::unique_ptr& a, const std::unique_ptr& b) { + return a->deadline < b->deadline; + } + ); + auto now = std::chrono::steady_clock::now(); + if ((*earliest)->deadline > now) { + this->parkTimeoutCv.wait_until(lock, (*earliest)->deadline); + continue; + } + // Pop and fire every entry due at this wakeup (a single wait can cover + // several parks with close deadlines) while still holding the mutex -- + // the same discipline dispatchCommitCompletion uses: a concurrent + // releaseParkTimeoutsByEnv cannot free the tsfn mid-call. + for (auto it = this->parkTimeouts.begin(); it != this->parkTimeouts.end();) { + if ((*it)->deadline > now) { + ++it; + continue; + } + std::unique_ptr due = std::move(*it); + it = this->parkTimeouts.erase(it); + bool expected = false; + if (due->fired->compare_exchange_strong(expected, true)) { + ::napi_call_threadsafe_function(due->tsfn, nullptr, napi_tsfn_nonblocking); + ::napi_release_threadsafe_function(due->tsfn, napi_tsfn_release); + } + // else: the wake callback already won the CAS and will + // call+release tsfn itself; nothing left for us to do. + } + } +} + +void DBDescriptor::fireParkTimeout(ParkTimeout* entry) { + std::lock_guard lock(this->parkTimeoutMutex); + auto it = std::find_if( + this->parkTimeouts.begin(), + this->parkTimeouts.end(), + [entry](const std::unique_ptr& p) { return p.get() == entry; } + ); + if (it == this->parkTimeouts.end()) { + // Already claimed by the timeout thread or by releaseParkTimeoutsByEnv. + return; + } + std::unique_ptr owned = std::move(*it); + this->parkTimeouts.erase(it); + bool expected = false; + if (owned->fired->compare_exchange_strong(expected, true)) { + ::napi_call_threadsafe_function(owned->tsfn, nullptr, napi_tsfn_nonblocking); + ::napi_release_threadsafe_function(owned->tsfn, napi_tsfn_release); + } +} + +void DBDescriptor::releaseParkTimeoutsByEnv(napi_env env) { + std::lock_guard lock(this->parkTimeoutMutex); + for (auto it = this->parkTimeouts.begin(); it != this->parkTimeouts.end();) { + if ((*it)->env != env) { + ++it; + continue; + } + // Mark fired first so neither the timeout thread nor a later real + // wake ever calls into the tsfn we're about to release -- the + // promise's env is gone, nothing is listening for the resolve. + bool expected = false; + (*it)->fired->compare_exchange_strong(expected, true); + if (!expected) { + ::napi_release_threadsafe_function((*it)->tsfn, napi_tsfn_release); + } + it = this->parkTimeouts.erase(it); + } +} + +void DBDescriptor::shutdownParkTimeouts() { + std::thread toJoin; + { + std::lock_guard lock(this->parkTimeoutMutex); + this->parkTimeoutStopped = true; + if (this->parkTimeoutThreadStarted) { + toJoin = std::move(this->parkTimeoutThread); + } + } + this->parkTimeoutCv.notify_all(); + if (toJoin.joinable()) { + toJoin.join(); + } +} + /** * Registers a database resource to be closed when the descriptor is closed. * diff --git a/src/binding/database/db_descriptor.h b/src/binding/database/db_descriptor.h index ff0b27247..f8b2a399a 100644 --- a/src/binding/database/db_descriptor.h +++ b/src/binding/database/db_descriptor.h @@ -4,11 +4,15 @@ #include #include #include +#include +#include #include #include #include +#include #include #include +#include #include "rocksdb/db.h" #include "rocksdb/statistics.h" #include "rocksdb/utilities/transaction_db.h" @@ -293,7 +297,79 @@ struct DBDescriptor final : public std::enable_shared_from_this { */ void releaseCommitCompletionsByEnv(napi_env env); + /** + * Bounded wait for a coordinated-retry commit parked on a conflicting + * holder's VT lock (`completeCommitWork` in transaction.cpp), so a holder + * that never releases (leaked/abandoned transaction) resolves RETRY_NOW + * instead of parking forever (harper#2001). One entry per outstanding + * park; `fired` is the exactly-once gate shared with the LockTracker wake + * callback registered on the same park -- whichever side wins the CAS + * calls+releases `tsfn`, the loser touches nothing. Owned by the + * descriptor (not one-thread-per-park and not the per-park heap state) + * so `parkTimeoutThread` -- lazily started, joined at close like + * `commitWorker` -- is the only thread ever created for this, and so a + * dying env's entries can be scrubbed by the same env-cleanup hook that + * releases commit completions instead of firing into a tsfn Node is + * about to free. + */ + struct ParkTimeout { + std::chrono::steady_clock::time_point deadline; + napi_env env; + napi_threadsafe_function tsfn; + std::shared_ptr> fired; + }; + std::mutex parkTimeoutMutex; + std::condition_variable parkTimeoutCv; + // Small and bounded by outstanding parks (each entry is removed by + // whichever of fireParkTimeout / releaseParkTimeoutsByEnv claims it + // first) -- not by total parks ever registered. + std::vector> parkTimeouts; + std::thread parkTimeoutThread; + bool parkTimeoutThreadStarted = false; + bool parkTimeoutStopped = false; + + /** + * JS thread (`completeCommitWork`). Registers a bounded wait, lazily + * starting the descriptor's single park-timeout thread. Returns the + * entry (owned by the descriptor) to capture in the LockTracker wake + * callback so a genuine wake can also resolve through `fireParkTimeout` + * -- or nullptr if the descriptor is already closing, in which case the + * caller must resolve inline (`cancelForDB` already woke every real + * holder by the time parking stops being accepted). + */ + ParkTimeout* scheduleParkTimeout( + napi_env env, + unsigned timeoutMs, + napi_threadsafe_function tsfn, + std::shared_ptr> fired + ); + + /** + * Fires a specific park's timeout early because its VT lock's holder + * released (LockTracker wake callback, any thread). A no-op if the entry + * is already gone -- claimed by the timeout thread or by + * releaseParkTimeoutsByEnv -- so a raw, non-owning `ParkTimeout*` is safe + * to capture: cancelForDB() (DBDescriptor::close(), before the + * descriptor can be destroyed) synchronously wakes every lock this + * descriptor owns, so a wake callback can only ever fire while the + * descriptor it points at is still alive. + */ + void fireParkTimeout(ParkTimeout* entry); + + /** + * Module env-cleanup hook. Cancels every pending park timeout registered + * for a dying env -- released, never called, so the background thread + * (or a later real wake) can never fire into a tsfn Node is about to + * free. + */ + void releaseParkTimeoutsByEnv(napi_env env); + + /** Descriptor close: stop and join the park-timeout thread. */ + void shutdownParkTimeouts(); + private: + /** Runs on parkTimeoutThread until shutdownParkTimeouts() stops it. */ + void runParkTimeoutLoop(); DBDescriptor( const std::string& path, const DBOptions& options, diff --git a/src/binding/database/db_registry.cpp b/src/binding/database/db_registry.cpp index e79a591b6..316aeee5c 100644 --- a/src/binding/database/db_registry.cpp +++ b/src/binding/database/db_registry.cpp @@ -657,6 +657,33 @@ void DBRegistry::ReleaseCommitCompletionsByEnv(napi_env env) { } } +/** + * Cancels each descriptor's pending park timeouts owned by the given env. + * Called from the module env-cleanup hook so a worker thread exiting does not + * leave a coordinated-retry park's timeout thread calling into a torn-down + * env ~ROCKSDB_JS_PARK_TIMEOUT_MS later. Mirrors ReleaseCommitCompletionsByEnv. + */ +void DBRegistry::ReleaseParkTimeoutsByEnv(napi_env env) { + if (!instance) { + return; + } + + std::vector> descriptors; + { + std::lock_guard lock(instance->databasesMutex); + descriptors.reserve(instance->databases.size()); + for (auto& [_key, entry] : instance->databases) { + if (entry.descriptor) { + descriptors.push_back(entry.descriptor); + } + } + } + + for (auto& descriptor : descriptors) { + descriptor->releaseParkTimeoutsByEnv(env); + } +} + /** * Shutdown will force all databases to flush in-memory data to disk and purge the registry. */ diff --git a/src/binding/database/db_registry.h b/src/binding/database/db_registry.h index afec14325..f99729d61 100644 --- a/src/binding/database/db_registry.h +++ b/src/binding/database/db_registry.h @@ -97,6 +97,7 @@ class DBRegistry final { static napi_value RegistryStatus(napi_env env, napi_callback_info info); static void RemoveListenersByEnv(napi_env env); static void ReleaseCommitCompletionsByEnv(napi_env env); + static void ReleaseParkTimeoutsByEnv(napi_env env); static void Shutdown(); static size_t Size(); }; diff --git a/src/binding/transaction/transaction.cpp b/src/binding/transaction/transaction.cpp index 52e9ef095..6c7d92dca 100644 --- a/src/binding/transaction/transaction.cpp +++ b/src/binding/transaction/transaction.cpp @@ -1,5 +1,7 @@ #include +#include #include +#include #include #include #include @@ -223,11 +225,25 @@ static void retryNowFinalize(napi_env env, void* finalizeData, void* /*hint*/) { * double-release corruption), the park resolves RETRY_NOW anyway once this * elapses instead of hanging forever (harper#2001). A spurious early * RETRY_NOW is harmless -- the JS layer just retries and may conflict again. + * Malformed input (non-numeric, negative, out of range) falls back to the + * default rather than `atoi`'s silent 0 (immediate-fire spin) or a negative + * value wrapping through `unsigned` into a multi-day effective hang -- this + * is an operational knob someone may reach for mid-incident, not a test seam. */ static unsigned parkTimeoutMs() { static const unsigned ms = []() -> unsigned { + constexpr unsigned kDefault = 3000; const char* v = ::getenv("ROCKSDB_JS_PARK_TIMEOUT_MS"); - return v != nullptr ? static_cast(::atoi(v)) : 3000; + if (v == nullptr || *v == '\0' || v[0] == '-') { + return kDefault; + } + char* end = nullptr; + errno = 0; + unsigned long parsed = ::strtoul(v, &end, 10); + if (end == v || *end != '\0' || errno == ERANGE || parsed > UINT32_MAX) { + return kDefault; + } + return static_cast(parsed); }(); return ms; } @@ -436,6 +452,7 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { bool parked = false; VerificationTable* vt = DBSettings::getInstance().getVerificationTableRaw(); + DBDescriptor* descriptor = (state->handle->dbHandle) ? state->handle->dbHandle->descriptor.get() : nullptr; for (auto* slot : state->savedSlots) { // refTrackerIfLocked takes a temporary reference under the VT // writer mutex, so the tracker cannot be freed by a concurrent @@ -457,14 +474,24 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { ); ::napi_unref_threadsafe_function(env, tsfn); - // Exactly-once gate shared between the wake path and the timeout path - // below. Lives in its own heap allocation independent of `ctx` (which - // the winning side's release eventually frees via retryNowFinalize) so - // the losing side -- which may run minutes later, from any thread, long - // after the winner's ctx/tsfn are gone -- never dereferences memory that - // may already be freed; it just loses the CAS and touches nothing. + // Exactly-once gate: independent of `ctx` so a loser arriving after + // the winner's ctx/tsfn are already gone just loses the CAS. auto fired = std::make_shared>(false); - auto fireOnce = [tsfn, fired]() { + + // #741 bound: descriptor-owned park timeout so a holder that never + // releases resolves RETRY_NOW after ROCKSDB_JS_PARK_TIMEOUT_MS + // instead of hanging forever (harper#2001). Null only while the + // descriptor is closing, in which case DBDescriptor::close() is + // already waking every real holder and the !registered branch + // below resolves this the same way. + DBDescriptor::ParkTimeout* parkEntry = + descriptor ? descriptor->scheduleParkTimeout(env, parkTimeoutMs(), tsfn, fired) : nullptr; + + auto fireOnce = [tsfn, fired, descriptor, parkEntry]() { + if (descriptor && parkEntry) { + descriptor->fireParkTimeout(parkEntry); + return; + } bool expected = false; if (fired->compare_exchange_strong(expected, true)) { ::napi_call_threadsafe_function(tsfn, nullptr, napi_tsfn_nonblocking); @@ -478,21 +505,6 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { bool registered = t->addWakeCallback(fireOnce); if (!registered) { fireOnce(); - } else { - // #741 bound: park behind a holder that never releases (leaked/ - // abandoned transaction, or a wake lost to the double-release - // corruption tracked in #741) would otherwise never settle - // (harper#2001). This detached wait races fireOnce with the wake - // callback above; whichever runs first wins the CAS and the other - // is a safe no-op. Not tied to `env`'s loop: a plain sleep thread - // (not a uv_timer) keeps this addon's ABI stable across Node - // versions -- libuv's internal struct layout is not part of the - // N-API surface this addon otherwise sticks to. - unsigned timeoutMs = parkTimeoutMs(); - std::thread([fireOnce, timeoutMs]() { - std::this_thread::sleep_for(std::chrono::milliseconds(timeoutMs)); - fireOnce(); - }).detach(); } vt->unrefTracker(t); diff --git a/test/lock-tracker.test.ts b/test/lock-tracker.test.ts index 032dbe409..55fcc388c 100644 --- a/test/lock-tracker.test.ts +++ b/test/lock-tracker.test.ts @@ -168,47 +168,30 @@ describe('Coordinated retry (Phase 3)', () => { })); }); -// Regression coverage for #741: a coordinated-retry commit that loses a -// conflict parks on the conflicting holder's VT lock and, before this fix, -// only resolved when that lock's LAST holder released -- if the holder is a -// leaked/abandoned transaction (or the wake is lost), the commit's promise -// never settled (harper#2001: a worker's write path disabled for 5+ hours). -// A parked commit must now also resolve RETRY_NOW after a bounded wait. +// Regression coverage for #741: a park behind a leaked/abandoned holder must +// resolve RETRY_NOW after a bounded wait instead of hanging forever +// (harper#2001). Default ROCKSDB_JS_PARK_TIMEOUT_MS is 3000ms; the lower +// bound below is what tells this apart from the `!parked` fast path (which +// also resolves RETRY_NOW, just near-instantly). describe('Coordinated retry — bounded park timeout (#741)', () => { it('a commit parked behind a never-releasing holder settles with RETRY_NOW within the deadline', () => dbRunner({ dbOptions: [{ encoding: false, verificationTable: true }] }, async ({ db }) => { const key = Buffer.from('park-timeout-abandoned-holder'); const v0 = 1.6e12; await db.put(key, valueWithVersion(v0)); - // Materialize the VT slot -- without an initial populateVersion the - // slot starts at 0 and lockSlotForWrite still installs a lock either - // way, but this matches the production shape (a live, cached key) - // and is what the local repro found necessary to actually exercise - // the park path instead of silently no-op'ing. db.populateVersion(key, v0); expect(db.verifyVersion(key, v0)).toBe(true); - // Abandoned holder: stages a write (installing the VT lock on the - // key's slot) and is deliberately never committed or aborted, with - // its reference retained for the lifetime of the test -- this is the - // "leaked/abandoned transaction" scenario from harper#2001. Its - // LockTracker holder count therefore never reaches zero and wake() - // is never called for anything parked on it. + // Abandoned holder: staged write, never committed or aborted. const holder = new Transaction(db.store, { coordinatedRetry: true }); holder.putSync(key, valueWithVersion(2.1e12)); - // Establish txn's snapshot with a plain (non-VT) read before the - // conflicting external commit below, so RocksDB's optimistic conflict - // check has something to validate against. + // Establish txn's snapshot before the conflicting external commit + // below, so RocksDB's optimistic conflict check has something to + // validate against. const txn = new Transaction(db.store, { coordinatedRetry: true }); await txn.get(key); - - // A write committed from outside txn bumps the key's sequence past - // txn's snapshot, so txn's own write below will conflict at commit. await db.put(key, valueWithVersion(2.2e12)); - - // txn's write joins the VT lock the abandoned holder already - // installed on the same slot (same key -> same slot). txn.putSync(key, valueWithVersion(2.3e12)); const start = Date.now(); @@ -216,8 +199,7 @@ describe('Coordinated retry — bounded park timeout (#741)', () => { const elapsed = Date.now() - start; expect(result).toBe(RETRY_NOW); - // Prior to the bounded park this hangs until env teardown (45s+ in - // the local repro); it must now settle well inside that. + expect(elapsed).toBeGreaterThanOrEqual(2500); expect(elapsed).toBeLessThan(8000); })); }); From 15088793242cc6b601f4642f27ecccc69240cf27 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sat, 1 Aug 2026 14:32:42 -0600 Subject: [PATCH 03/19] fix(transaction): close park-timeout lifetime and correctness gaps Round-3 response to the second BLOCK verdict (codex+gemini+grok+ harper-domain, independent=true) on the descriptor-owned redesign: - blocker: the LockTracker wake closure captured a raw DBDescriptor*. A park can end up registered on a tracker installed by a *different* database on a colliding VT slot (lockSlotForWrite joins an existing tracker without retagging its dbId), so that lock's eventual release wakes a park whose own descriptor may have already closed and been destroyed -- cancelForDB only wakes trackers tagged with its own vtEpoch, so it can't be relied on to have resolved a foreign-dbId park first. Now captures a std::weak_ptr and .lock()s it; a failed lock means shutdownParkTimeouts (below) already resolved the park. - major: retryNowCallJs had no `env == nullptr` guard, unlike commitCompletionCallJs -- Node drains a tearing-down env's tsfn queue by invoking call_js_cb with a null env, and this change adds a second producer onto that tsfn's queue. Added the same guard. - major: a park's identity was its ParkTimeout*. Since LockTracker::wakeCallbacks has no removal API, a stale closure can outlive its entry, and the freed heap address could be reused by an unrelated later park -- an ABA hazard that could resolve the wrong commit's promise. Identity is now a monotonic uint64_t id. - major: shutdownParkTimeouts only stopped and joined the thread; any park still pending (notably one on a foreign-dbId tracker per the blocker above) was neither fired nor released -- an unresolved promise, or a leaked tsfn, exactly the class of bug this PR exists to fix, now happening at DB-close time instead. It now drains (call+release) everything remaining after the join, and both finishClose() and the destructor call it (idempotently), matching commitWorker's own belt-and-suspenders shutdown discipline. - major: the default (3000ms) plus maxRetries (default 3) capped a *coordinated* wait at ~9s, and every timeout consumed a retry attempt indistinguishably from a real wake -- a holder that legitimately holds a VT write intent for a few seconds (large batch commit, slow fsync under compaction backpressure) could exhaust the retry budget and abandon a write that would previously have parked and succeeded. Raised the default to 5000ms, the top of this task's requested 2-5s range, for maximum headroom within scope; not fully solved (would need the native/JS layers to distinguish a timeout- triggered RETRY_NOW from a genuine wake so it doesn't consume maxRetries budget -- flagged as a follow-up in the PR). - minor: `ROCKSDB_JS_PARK_TIMEOUT_MS=" -1"` (leading whitespace) bypassed the negative check and could wrap to ~49 days on a 32-bit build. Skips leading whitespace before checking for '-'. Also folded in the "explicit 0 spins" minor from round 1/2 (parsed == 0 now falls back to the default too, alongside non-numeric/negative/overflow). Also updated the AGENTS.md note and the test's timing bounds (>=4500ms, <10000ms) for the 5000ms default. Co-Authored-By: Claude Opus --- AGENTS.md | 2 +- src/binding/database/db_descriptor.cpp | 52 +++++++++++++---- src/binding/database/db_descriptor.h | 68 +++++++++++++-------- src/binding/transaction/transaction.cpp | 78 ++++++++++++++++++++----- test/lock-tracker.test.ts | 6 +- 5 files changed, 150 insertions(+), 56 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 2444bdced..98799c25e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -193,7 +193,7 @@ sufficient (env teardown does not honor tsfn acquire counts); see each completion callback (widens teardown race windows) - `ROCKSDB_JS_TXN_GET_DELAY_MS` - Test-only: delay a transaction's cold-cache async get before it reads (exercises orphan cleanup past the async-work wait timeout) -- `ROCKSDB_JS_PARK_TIMEOUT_MS` - Bounded wait (default `3000`) before a +- `ROCKSDB_JS_PARK_TIMEOUT_MS` - Bounded wait (default `5000`) before a coordinated-retry commit parked on a conflicting holder's VT lock resolves RETRY_NOW unconditionally, in case the holder never releases (see "Coordinated retry" note below) diff --git a/src/binding/database/db_descriptor.cpp b/src/binding/database/db_descriptor.cpp index 4102c8cb1..c77da31fe 100644 --- a/src/binding/database/db_descriptor.cpp +++ b/src/binding/database/db_descriptor.cpp @@ -358,6 +358,12 @@ DBDescriptor::DBDescriptor( DBDescriptor::~DBDescriptor() { DEBUG_LOG("%p DBDescriptor::~DBDescriptor Closing \"%s\"\n", this, this->path.c_str()); this->close(); + // Belt-and-suspenders, matching commitWorker/logWorker's own destructor + // shutdown: close() no-ops if finishClose() already ran (beginClose() + // returned false because some other caller is/was closing), and + // shutdownParkTimeouts() is idempotent, so this is a no-op in the + // common case and a safety net otherwise. + this->shutdownParkTimeouts(); } /** @@ -574,7 +580,7 @@ void DBDescriptor::releaseCommitCompletionsByEnv(napi_env env) { } } -DBDescriptor::ParkTimeout* DBDescriptor::scheduleParkTimeout( +uint64_t DBDescriptor::scheduleParkTimeout( napi_env env, unsigned timeoutMs, napi_threadsafe_function tsfn, @@ -582,26 +588,24 @@ DBDescriptor::ParkTimeout* DBDescriptor::scheduleParkTimeout( ) { std::lock_guard lock(this->parkTimeoutMutex); if (this->parkTimeoutStopped) { - // Descriptor already closing: cancelForDB() (finishClose(), before - // this point) already woke every real holder this descriptor owns, - // so a park reaching here has nothing left to wait for -- the caller - // resolves inline instead of registering with a thread we're not - // going to start again. - return nullptr; + // Descriptor already closing: the caller must resolve inline without + // registering with the LockTracker at all (see the header comment). + return 0; } auto entry = std::make_unique(); + entry->id = this->nextParkTimeoutId++; entry->deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(timeoutMs); entry->env = env; entry->tsfn = tsfn; entry->fired = std::move(fired); - ParkTimeout* raw = entry.get(); + uint64_t id = entry->id; this->parkTimeouts.push_back(std::move(entry)); if (!this->parkTimeoutThreadStarted) { this->parkTimeoutThreadStarted = true; this->parkTimeoutThread = std::thread([this]() { this->runParkTimeoutLoop(); }); } this->parkTimeoutCv.notify_all(); - return raw; + return id; } void DBDescriptor::runParkTimeoutLoop() { @@ -649,15 +653,16 @@ void DBDescriptor::runParkTimeoutLoop() { } } -void DBDescriptor::fireParkTimeout(ParkTimeout* entry) { +void DBDescriptor::fireParkTimeout(uint64_t id) { std::lock_guard lock(this->parkTimeoutMutex); auto it = std::find_if( this->parkTimeouts.begin(), this->parkTimeouts.end(), - [entry](const std::unique_ptr& p) { return p.get() == entry; } + [id](const std::unique_ptr& p) { return p->id == id; } ); if (it == this->parkTimeouts.end()) { - // Already claimed by the timeout thread or by releaseParkTimeoutsByEnv. + // Already claimed by the timeout thread, releaseParkTimeoutsByEnv, or + // shutdownParkTimeouts. return; } std::unique_ptr owned = std::move(*it); @@ -689,18 +694,41 @@ void DBDescriptor::releaseParkTimeoutsByEnv(napi_env env) { } void DBDescriptor::shutdownParkTimeouts() { + std::vector> remaining; std::thread toJoin; { std::lock_guard lock(this->parkTimeoutMutex); + if (this->parkTimeoutStopped && !this->parkTimeoutThreadStarted) { + // Already fully shut down (e.g. finishClose() already ran; this is + // the destructor's belt-and-suspenders call) -- nothing left to do. + return; + } this->parkTimeoutStopped = true; if (this->parkTimeoutThreadStarted) { toJoin = std::move(this->parkTimeoutThread); + this->parkTimeoutThreadStarted = false; } + remaining = std::move(this->parkTimeouts); + this->parkTimeouts.clear(); } this->parkTimeoutCv.notify_all(); if (toJoin.joinable()) { toJoin.join(); } + // Resolve every park still pending now instead of leaving its promise + // unresolved forever. Anything still here belongs to an env that hasn't + // torn down (a dying env's entries were already released by + // releaseParkTimeoutsByEnv), so its tsfn is safe to call; RETRY_NOW is + // the correct outcome for a park whose descriptor is closing regardless + // of whether its real holder's wake ever reaches it (see the header + // comment on the cross-database VT-slot-collision case this closes). + for (auto& entry : remaining) { + bool expected = false; + if (entry->fired->compare_exchange_strong(expected, true)) { + ::napi_call_threadsafe_function(entry->tsfn, nullptr, napi_tsfn_nonblocking); + ::napi_release_threadsafe_function(entry->tsfn, napi_tsfn_release); + } + } } /** diff --git a/src/binding/database/db_descriptor.h b/src/binding/database/db_descriptor.h index f8b2a399a..903493c77 100644 --- a/src/binding/database/db_descriptor.h +++ b/src/binding/database/db_descriptor.h @@ -302,17 +302,22 @@ struct DBDescriptor final : public std::enable_shared_from_this { * holder's VT lock (`completeCommitWork` in transaction.cpp), so a holder * that never releases (leaked/abandoned transaction) resolves RETRY_NOW * instead of parking forever (harper#2001). One entry per outstanding - * park; `fired` is the exactly-once gate shared with the LockTracker wake - * callback registered on the same park -- whichever side wins the CAS - * calls+releases `tsfn`, the loser touches nothing. Owned by the - * descriptor (not one-thread-per-park and not the per-park heap state) - * so `parkTimeoutThread` -- lazily started, joined at close like - * `commitWorker` -- is the only thread ever created for this, and so a - * dying env's entries can be scrubbed by the same env-cleanup hook that - * releases commit completions instead of firing into a tsfn Node is - * about to free. + * park, identified by a monotonic `id` rather than the entry's address -- + * `LockTracker::wakeCallbacks` has no removal API (a known, documented + * gap; see the AGENTS.md note), so a stale closure can outlive its entry + * and its `unique_ptr`'s heap address can be reused by a later park; an + * address-keyed lookup would then resolve the wrong park. `fired` is the + * exactly-once gate shared with the LockTracker wake callback registered + * on the same park -- whichever side wins the CAS calls+releases `tsfn`, + * the loser touches nothing. Owned by the descriptor (not one-thread- + * per-park and not the per-park heap state) so `parkTimeoutThread` -- + * lazily started, joined at close like `commitWorker` -- is the only + * thread ever created for this, and so a dying env's entries can be + * scrubbed by the same env-cleanup hook that releases commit + * completions instead of firing into a tsfn Node is about to free. */ struct ParkTimeout { + uint64_t id; std::chrono::steady_clock::time_point deadline; napi_env env; napi_threadsafe_function tsfn; @@ -324,6 +329,7 @@ struct DBDescriptor final : public std::enable_shared_from_this { // whichever of fireParkTimeout / releaseParkTimeoutsByEnv claims it // first) -- not by total parks ever registered. std::vector> parkTimeouts; + uint64_t nextParkTimeoutId = 1; std::thread parkTimeoutThread; bool parkTimeoutThreadStarted = false; bool parkTimeoutStopped = false; @@ -331,13 +337,15 @@ struct DBDescriptor final : public std::enable_shared_from_this { /** * JS thread (`completeCommitWork`). Registers a bounded wait, lazily * starting the descriptor's single park-timeout thread. Returns the - * entry (owned by the descriptor) to capture in the LockTracker wake - * callback so a genuine wake can also resolve through `fireParkTimeout` - * -- or nullptr if the descriptor is already closing, in which case the - * caller must resolve inline (`cancelForDB` already woke every real - * holder by the time parking stops being accepted). - */ - ParkTimeout* scheduleParkTimeout( + * new entry's id to capture in the LockTracker wake callback so a + * genuine wake can also resolve through `fireParkTimeout` -- or 0 if the + * descriptor is already closing. The caller must then resolve inline + * without registering with the LockTracker at all: by this point + * `cancelForDB` has already run, so a lock that becomes parkable only + * after that point (the `cancelForDB` / `shutdownParkTimeouts` window) + * would otherwise park with no timeout backing it. + */ + uint64_t scheduleParkTimeout( napi_env env, unsigned timeoutMs, napi_threadsafe_function tsfn, @@ -346,15 +354,11 @@ struct DBDescriptor final : public std::enable_shared_from_this { /** * Fires a specific park's timeout early because its VT lock's holder - * released (LockTracker wake callback, any thread). A no-op if the entry - * is already gone -- claimed by the timeout thread or by - * releaseParkTimeoutsByEnv -- so a raw, non-owning `ParkTimeout*` is safe - * to capture: cancelForDB() (DBDescriptor::close(), before the - * descriptor can be destroyed) synchronously wakes every lock this - * descriptor owns, so a wake callback can only ever fire while the - * descriptor it points at is still alive. + * released (LockTracker wake callback, any thread). A no-op if the id is + * already gone -- claimed by the timeout thread, by + * releaseParkTimeoutsByEnv, or drained at shutdown. */ - void fireParkTimeout(ParkTimeout* entry); + void fireParkTimeout(uint64_t id); /** * Module env-cleanup hook. Cancels every pending park timeout registered @@ -364,7 +368,21 @@ struct DBDescriptor final : public std::enable_shared_from_this { */ void releaseParkTimeoutsByEnv(napi_env env); - /** Descriptor close: stop and join the park-timeout thread. */ + /** + * Descriptor close: stop and join the park-timeout thread, then resolve + * (call+release) every park still pending. `cancelForDB` (called just + * before this, in `finishClose()`) wakes every lock tagged with this + * descriptor's own `vtEpoch`, but a park can be registered on a tracker + * installed by a *different* database on a colliding VT slot + * (`VerificationTable::lockSlotForWrite` joins an existing tracker + * without retagging its `dbId`) -- that lock's eventual release wakes + * the other database, not this one, so this descriptor's own park would + * otherwise wait on a wake that may never come from its perspective. + * Draining here guarantees every park this descriptor scheduled settles + * by the time it closes, independent of which VT tracker it ended up on. + * Idempotent (safe to call from both `finishClose()` and the destructor, + * matching `commitWorker`'s own belt-and-suspenders shutdown call). + */ void shutdownParkTimeouts(); private: diff --git a/src/binding/transaction/transaction.cpp b/src/binding/transaction/transaction.cpp index 6c7d92dca..6f3760c6b 100644 --- a/src/binding/transaction/transaction.cpp +++ b/src/binding/transaction/transaction.cpp @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -203,6 +204,13 @@ struct RetryNowContext { }; static void retryNowCallJs(napi_env env, napi_value /*func*/, void* context, void* /*data*/) { + // env is nullptr when the env is tearing down and this tsfn's queue is + // being drained as part of that (same as commitCompletionCallJs above) — + // nothing left to resolve into; retryNowFinalize still runs afterward to + // free ctx. + if (env == nullptr) { + return; + } auto* ctx = reinterpret_cast(context); napi_value global, resolveFn, retryVal; ::napi_get_global(env, &global); @@ -224,7 +232,12 @@ static void retryNowFinalize(napi_env env, void* finalizeData, void* /*hint*/) { * never releases (leaked/abandoned transaction, or a wake lost to #741's * double-release corruption), the park resolves RETRY_NOW anyway once this * elapses instead of hanging forever (harper#2001). A spurious early - * RETRY_NOW is harmless -- the JS layer just retries and may conflict again. + * RETRY_NOW is harmless -- the JS layer just retries and may conflict again, + * consuming a coordinatedRetry attempt as it would for a genuine wake; the + * default is deliberately the top of the 2-5s range this was scoped to, to + * leave the most headroom for a holder that is merely slow (a large batch + * commit, backpressure under compaction) rather than abandoned, since a + * commit's transaction.commit() attempts are finite (maxRetries, default 3). * Malformed input (non-numeric, negative, out of range) falls back to the * default rather than `atoi`'s silent 0 (immediate-fire spin) or a negative * value wrapping through `unsigned` into a multi-day effective hang -- this @@ -232,15 +245,26 @@ static void retryNowFinalize(napi_env env, void* finalizeData, void* /*hint*/) { */ static unsigned parkTimeoutMs() { static const unsigned ms = []() -> unsigned { - constexpr unsigned kDefault = 3000; + constexpr unsigned kDefault = 5000; const char* v = ::getenv("ROCKSDB_JS_PARK_TIMEOUT_MS"); - if (v == nullptr || *v == '\0' || v[0] == '-') { + if (v == nullptr) { + return kDefault; + } + const char* firstNonSpace = v; + while (*firstNonSpace != '\0' && ::isspace(static_cast(*firstNonSpace))) { + ++firstNonSpace; + } + if (*firstNonSpace == '\0' || *firstNonSpace == '-') { return kDefault; } char* end = nullptr; errno = 0; unsigned long parsed = ::strtoul(v, &end, 10); - if (end == v || *end != '\0' || errno == ERANGE || parsed > UINT32_MAX) { + // parsed == 0 covers both a literal "0" and `atoi`'s old silent + // non-numeric fallback; either way, firing every park immediately is + // exactly the unparked spin this bound exists to prevent, so treat it + // the same as malformed input rather than as a deliberate opt-out. + if (end == v || *end != '\0' || errno == ERANGE || parsed > UINT32_MAX || parsed == 0) { return kDefault; } return static_cast(parsed); @@ -452,7 +476,8 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { bool parked = false; VerificationTable* vt = DBSettings::getInstance().getVerificationTableRaw(); - DBDescriptor* descriptor = (state->handle->dbHandle) ? state->handle->dbHandle->descriptor.get() : nullptr; + std::shared_ptr descriptor = + state->handle->dbHandle ? state->handle->dbHandle->descriptor : nullptr; for (auto* slot : state->savedSlots) { // refTrackerIfLocked takes a temporary reference under the VT // writer mutex, so the tracker cannot be freed by a concurrent @@ -480,16 +505,39 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { // #741 bound: descriptor-owned park timeout so a holder that never // releases resolves RETRY_NOW after ROCKSDB_JS_PARK_TIMEOUT_MS - // instead of hanging forever (harper#2001). Null only while the - // descriptor is closing, in which case DBDescriptor::close() is - // already waking every real holder and the !registered branch - // below resolves this the same way. - DBDescriptor::ParkTimeout* parkEntry = - descriptor ? descriptor->scheduleParkTimeout(env, parkTimeoutMs(), tsfn, fired) : nullptr; - - auto fireOnce = [tsfn, fired, descriptor, parkEntry]() { - if (descriptor && parkEntry) { - descriptor->fireParkTimeout(parkEntry); + // instead of hanging forever (harper#2001). 0 only while the + // descriptor is closing. + uint64_t parkId = descriptor ? descriptor->scheduleParkTimeout(env, parkTimeoutMs(), tsfn, fired) : 0; + + if (descriptor && parkId == 0) { + // Descriptor closing: resolve now without registering with the + // LockTracker at all (see scheduleParkTimeout's header comment) + // -- a lock that only becomes parkable in the narrow + // cancelForDB/shutdownParkTimeouts window would otherwise have + // no timeout backing its park. + vt->unrefTracker(t); + bool expected = false; + if (fired->compare_exchange_strong(expected, true)) { + ::napi_call_threadsafe_function(tsfn, nullptr, napi_tsfn_nonblocking); + ::napi_release_threadsafe_function(tsfn, napi_tsfn_release); + } + parked = true; + break; + } + + // A weak reference: LockTracker::wakeCallbacks has no removal API + // (a known, documented gap), so this closure can outlive the park + // it was registered for -- including outliving `descriptor` itself + // (e.g. a foreign-dbId tracker from a colliding VT slot, woken by + // a DB this park was never registered against). `.lock()` failing + // means shutdownParkTimeouts already resolved this park as part of + // that descriptor's own close, so there is nothing left to do. + std::weak_ptr weakDescriptor = descriptor; + auto fireOnce = [tsfn, fired, weakDescriptor, parkId]() { + if (parkId != 0) { + if (auto d = weakDescriptor.lock()) { + d->fireParkTimeout(parkId); + } return; } bool expected = false; diff --git a/test/lock-tracker.test.ts b/test/lock-tracker.test.ts index 55fcc388c..2ebe0cd20 100644 --- a/test/lock-tracker.test.ts +++ b/test/lock-tracker.test.ts @@ -170,7 +170,7 @@ describe('Coordinated retry (Phase 3)', () => { // Regression coverage for #741: a park behind a leaked/abandoned holder must // resolve RETRY_NOW after a bounded wait instead of hanging forever -// (harper#2001). Default ROCKSDB_JS_PARK_TIMEOUT_MS is 3000ms; the lower +// (harper#2001). Default ROCKSDB_JS_PARK_TIMEOUT_MS is 5000ms; the lower // bound below is what tells this apart from the `!parked` fast path (which // also resolves RETRY_NOW, just near-instantly). describe('Coordinated retry — bounded park timeout (#741)', () => { @@ -199,8 +199,8 @@ describe('Coordinated retry — bounded park timeout (#741)', () => { const elapsed = Date.now() - start; expect(result).toBe(RETRY_NOW); - expect(elapsed).toBeGreaterThanOrEqual(2500); - expect(elapsed).toBeLessThan(8000); + expect(elapsed).toBeGreaterThanOrEqual(4500); + expect(elapsed).toBeLessThan(10000); })); }); From b9b3048d59fbad34406a99aa534ada752a9f00a2 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sat, 1 Aug 2026 15:08:31 -0600 Subject: [PATCH 04/19] fix(transaction): fix null-descriptor bypass and shutdown drain race Round-4 response to the third review (codex+gemini+grok+harper-domain, independent=true) -- no confirmed blocker this round; two majors: - The inline-resolve guard was `if (descriptor && parkId == 0)`. DBHandle::close() can reset `descriptor` to null concurrently while another handle keeps the DB open; in that case parkId is 0 but the guard was false, so the code fell through to registering a wake callback with no timeout thread behind it -- an unbounded park, exactly the harper#2001 class this PR exists to close. Changed to `if (parkId == 0)`: the inline-resolve path only needs tsfn/fired, not descriptor. - shutdownParkTimeouts() moved pending entries out of parkTimeouts and called their tsfns *after* releasing parkTimeoutMutex, defeating the barrier releaseParkTimeoutsByEnv depends on for env-teardown safety (runParkTimeoutLoop fires under the lock; the drain didn't). A concurrent releaseParkTimeoutsByEnv for a dying env could find nothing to cancel (already pulled out) while shutdown is mid-call on that exact env's tsfn, racing Node freeing it. Now drains under the same lock it takes to stop the thread, then joins outside it (joining while holding the lock would deadlock the loop's cv.wait_until). Also: check napi_create_threadsafe_function's status instead of using an uninitialized handle on failure (pre-existing gap, widened by this change storing the handle for a background thread to call later rather than using it immediately); corrected a stale comment claiming pending parks at descriptor-close "need not fire" (they do, for the foreign-dbId case the weak_ptr fix addresses); trimmed several narrating comments flagged across all three review rounds. Co-Authored-By: Claude Opus --- src/binding/database/db_descriptor.cpp | 61 +++++++++++-------------- src/binding/transaction/transaction.cpp | 39 ++++++++-------- 2 files changed, 48 insertions(+), 52 deletions(-) diff --git a/src/binding/database/db_descriptor.cpp b/src/binding/database/db_descriptor.cpp index c77da31fe..01021b0bc 100644 --- a/src/binding/database/db_descriptor.cpp +++ b/src/binding/database/db_descriptor.cpp @@ -358,11 +358,8 @@ DBDescriptor::DBDescriptor( DBDescriptor::~DBDescriptor() { DEBUG_LOG("%p DBDescriptor::~DBDescriptor Closing \"%s\"\n", this, this->path.c_str()); this->close(); - // Belt-and-suspenders, matching commitWorker/logWorker's own destructor - // shutdown: close() no-ops if finishClose() already ran (beginClose() - // returned false because some other caller is/was closing), and - // shutdownParkTimeouts() is idempotent, so this is a no-op in the - // common case and a safety net otherwise. + // Idempotent safety net, matching commitWorker/logWorker's own + // destructor shutdown. this->shutdownParkTimeouts(); } @@ -481,13 +478,10 @@ void DBDescriptor::finishClose() { } } - // Stop and join the park-timeout thread now that cancelForDB() has woken - // every real holder above -- any entries still pending at this point - // belong to envs that never got a chance to release their own park - // (already reachable via releaseParkTimeoutsByEnv, not this shutdown - // path), so nothing here needs to fire them, only stop the thread and - // let the destructor's guarantee (park entries erased == no live handle - // left) hold. + // A park can be registered on a foreign-dbId tracker (colliding VT slot; + // see the ParkTimeout header comment), so cancelForDB() above cannot be + // relied on to have woken everything this descriptor is waiting on. + // shutdownParkTimeouts() resolves whatever is left regardless. this->shutdownParkTimeouts(); // Unregister from transaction log store registry - this will clean up stores @@ -631,10 +625,7 @@ void DBDescriptor::runParkTimeoutLoop() { this->parkTimeoutCv.wait_until(lock, (*earliest)->deadline); continue; } - // Pop and fire every entry due at this wakeup (a single wait can cover - // several parks with close deadlines) while still holding the mutex -- - // the same discipline dispatchCommitCompletion uses: a concurrent - // releaseParkTimeoutsByEnv cannot free the tsfn mid-call. + // Fire while still holding the mutex, like dispatchCommitCompletion. for (auto it = this->parkTimeouts.begin(); it != this->parkTimeouts.end();) { if ((*it)->deadline > now) { ++it; @@ -647,8 +638,6 @@ void DBDescriptor::runParkTimeoutLoop() { ::napi_call_threadsafe_function(due->tsfn, nullptr, napi_tsfn_nonblocking); ::napi_release_threadsafe_function(due->tsfn, napi_tsfn_release); } - // else: the wake callback already won the CAS and will - // call+release tsfn itself; nothing left for us to do. } } } @@ -694,7 +683,6 @@ void DBDescriptor::releaseParkTimeoutsByEnv(napi_env env) { } void DBDescriptor::shutdownParkTimeouts() { - std::vector> remaining; std::thread toJoin; { std::lock_guard lock(this->parkTimeoutMutex); @@ -708,27 +696,32 @@ void DBDescriptor::shutdownParkTimeouts() { toJoin = std::move(this->parkTimeoutThread); this->parkTimeoutThreadStarted = false; } - remaining = std::move(this->parkTimeouts); + // Resolve every park still pending, under the same mutex + // releaseParkTimeoutsByEnv/fireParkTimeout/runParkTimeoutLoop all + // serialize their tsfn calls on -- draining outside the lock would + // let a concurrent releaseParkTimeoutsByEnv for a dying env observe + // "nothing to cancel" (we already pulled its entry out) while we are + // mid-call on that exact env's tsfn, racing Node freeing it. RETRY_NOW + // is the correct outcome for a park whose descriptor is closing + // regardless of whether its real holder's wake ever reaches it (see + // the header comment on the cross-database VT-slot-collision case + // this closes). + for (auto& entry : this->parkTimeouts) { + bool expected = false; + if (entry->fired->compare_exchange_strong(expected, true)) { + ::napi_call_threadsafe_function(entry->tsfn, nullptr, napi_tsfn_nonblocking); + ::napi_release_threadsafe_function(entry->tsfn, napi_tsfn_release); + } + } this->parkTimeouts.clear(); } + // Notify + join outside the lock: the loop's cv.wait_until needs to + // re-acquire parkTimeoutMutex to observe parkTimeoutStopped and return, + // so joining while still holding it would deadlock. this->parkTimeoutCv.notify_all(); if (toJoin.joinable()) { toJoin.join(); } - // Resolve every park still pending now instead of leaving its promise - // unresolved forever. Anything still here belongs to an env that hasn't - // torn down (a dying env's entries were already released by - // releaseParkTimeoutsByEnv), so its tsfn is safe to call; RETRY_NOW is - // the correct outcome for a park whose descriptor is closing regardless - // of whether its real holder's wake ever reaches it (see the header - // comment on the cross-database VT-slot-collision case this closes). - for (auto& entry : remaining) { - bool expected = false; - if (entry->fired->compare_exchange_strong(expected, true)) { - ::napi_call_threadsafe_function(entry->tsfn, nullptr, napi_tsfn_nonblocking); - ::napi_release_threadsafe_function(entry->tsfn, napi_tsfn_release); - } - } } /** diff --git a/src/binding/transaction/transaction.cpp b/src/binding/transaction/transaction.cpp index 6f3760c6b..c18601f7a 100644 --- a/src/binding/transaction/transaction.cpp +++ b/src/binding/transaction/transaction.cpp @@ -490,31 +490,37 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { napi_value resource_name; ::napi_create_string_latin1(env, "transaction.retry", NAPI_AUTO_LENGTH, &resource_name); napi_threadsafe_function tsfn; - ::napi_create_threadsafe_function( + napi_status tsfnStatus = ::napi_create_threadsafe_function( env, nullptr, nullptr, resource_name, 0, 1, ctx, retryNowFinalize, ctx, retryNowCallJs, &tsfn ); + if (tsfnStatus != napi_ok) { + // Creation failed (e.g. an already-pending exception): nothing to + // call+release, and ctx is not yet owned by any finalize -- fall + // through to the plain !parked resolve below instead of leaving + // a garbage tsfn handle in a park entry. + vt->unrefTracker(t); + break; + } ::napi_unref_threadsafe_function(env, tsfn); // Exactly-once gate: independent of `ctx` so a loser arriving after // the winner's ctx/tsfn are already gone just loses the CAS. auto fired = std::make_shared>(false); - // #741 bound: descriptor-owned park timeout so a holder that never - // releases resolves RETRY_NOW after ROCKSDB_JS_PARK_TIMEOUT_MS - // instead of hanging forever (harper#2001). 0 only while the - // descriptor is closing. + // #741 bound (harper#2001): resolves RETRY_NOW after + // ROCKSDB_JS_PARK_TIMEOUT_MS if the holder never releases. 0 both + // when the descriptor is closing and when there is no descriptor + // at all (DBHandle::close() can reset it concurrently) -- either + // way there is no timeout thread behind this park. uint64_t parkId = descriptor ? descriptor->scheduleParkTimeout(env, parkTimeoutMs(), tsfn, fired) : 0; - if (descriptor && parkId == 0) { - // Descriptor closing: resolve now without registering with the - // LockTracker at all (see scheduleParkTimeout's header comment) - // -- a lock that only becomes parkable in the narrow - // cancelForDB/shutdownParkTimeouts window would otherwise have - // no timeout backing its park. + if (parkId == 0) { + // No timeout thread behind this park -- resolve now rather + // than register with the LockTracker unbounded. vt->unrefTracker(t); bool expected = false; if (fired->compare_exchange_strong(expected, true)) { @@ -525,13 +531,10 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { break; } - // A weak reference: LockTracker::wakeCallbacks has no removal API - // (a known, documented gap), so this closure can outlive the park - // it was registered for -- including outliving `descriptor` itself - // (e.g. a foreign-dbId tracker from a colliding VT slot, woken by - // a DB this park was never registered against). `.lock()` failing - // means shutdownParkTimeouts already resolved this park as part of - // that descriptor's own close, so there is nothing left to do. + // weak_ptr, not raw: LockTracker::wakeCallbacks has no removal API, + // so this closure can outlive `descriptor` (e.g. a foreign-dbId + // tracker from a colliding VT slot). `.lock()` failing means + // shutdownParkTimeouts already resolved this park at close. std::weak_ptr weakDescriptor = descriptor; auto fireOnce = [tsfn, fired, weakDescriptor, parkId]() { if (parkId != 0) { From 91f41e44b1912246f0041ede697e122d0d3196b4 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sat, 1 Aug 2026 15:45:43 -0600 Subject: [PATCH 05/19] fix(transaction): close remaining park-timeout races, O(1) fire path Round-5 response to the fourth review (codex+gemini+grok+harper-domain, independent=true). No blocker survived that round's domain adjudication either -- three raw "blocker" findings were downgraded to major/minor after tracing the actual mechanism -- but it found real new issues: - The LockTracker wake closure's weakDescriptor.lock() is a transient extra ref on the descriptor, exactly the shape that can make a racing close()'s PurgeIfUnreferenced observe use_count() > 1 and skip the purge (HarperFast/rocksdb-js#672's exact class of bug -- already fixed for backup/checkpoint state, missed here). Now retries PurgeIfUnreferenced after releasing the ref, matching AsyncBackupState. - runParkTimeoutLoop's wait_until bound a const reference into a ParkTimeout the mutex-releasing wait could let another thread erase (a real wake racing the timeout) -- a freed-memory read once the wait re-checked the deadline. Copies the deadline into a local first. - fireParkTimeout's O(N) vector scan+erase runs inside LockTracker:: wake(), which holds the *process-global* VT writerMutex_ -- under a contention burst (exactly what abandoned holders cause), every other database's write-intent registration serializes behind this one's O(N) work. parkTimeouts is now an unordered_map, making fireParkTimeout's lookup O(1); the deadline scan (unaffected by the global lock) stays a linear pass, which is fine at realistic N. - scheduleParkTimeout set parkTimeoutThreadStarted before constructing the thread; a throwing ctor (thread/resource exhaustion) both unwinds through an N-API callback with no catch (process abort) and, if caught, permanently disables ever starting a timeout thread again (silent regression to an unbounded park). Now wraps construction in try/catch, sets the flag only on success, and returns 0 (caller resolves inline) on failure so the next park retries. Also: added a wall-clock upper bound to an existing wake-path test (Coordinated retry Phase 3) so a broken LockTracker::wake() can't hide behind the #741 timeout and still pass; updated the AGENTS.md note on wakeCallbacks growth (now per-retry, not per-incident, since each timeout causes a re-park); further comment trims flagged across all four review rounds, including one describing removed (atoi) code. Co-Authored-By: Claude Opus --- src/binding/database/db_descriptor.cpp | 71 +++++++++++++------------ src/binding/database/db_descriptor.h | 60 +++++++-------------- src/binding/transaction/transaction.cpp | 31 ++++++----- test/lock-tracker.test.ts | 8 +++ 4 files changed, 82 insertions(+), 88 deletions(-) diff --git a/src/binding/database/db_descriptor.cpp b/src/binding/database/db_descriptor.cpp index 01021b0bc..4febe93bd 100644 --- a/src/binding/database/db_descriptor.cpp +++ b/src/binding/database/db_descriptor.cpp @@ -10,6 +10,7 @@ #include "rocksdb/utilities/options_util.h" #include #include +#include #include namespace rocksdb_js { @@ -586,6 +587,18 @@ uint64_t DBDescriptor::scheduleParkTimeout( // registering with the LockTracker at all (see the header comment). return 0; } + if (!this->parkTimeoutThreadStarted) { + try { + this->parkTimeoutThread = std::thread([this]() { this->runParkTimeoutLoop(); }); + } catch (const std::system_error&) { + // Thread creation failed (e.g. thread/resource exhaustion): leave + // the flag false so the next park retries, and tell the caller to + // resolve inline now rather than register a park nothing will + // ever time out. + return 0; + } + this->parkTimeoutThreadStarted = true; + } auto entry = std::make_unique(); entry->id = this->nextParkTimeoutId++; entry->deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(timeoutMs); @@ -593,11 +606,7 @@ uint64_t DBDescriptor::scheduleParkTimeout( entry->tsfn = tsfn; entry->fired = std::move(fired); uint64_t id = entry->id; - this->parkTimeouts.push_back(std::move(entry)); - if (!this->parkTimeoutThreadStarted) { - this->parkTimeoutThreadStarted = true; - this->parkTimeoutThread = std::thread([this]() { this->runParkTimeoutLoop(); }); - } + this->parkTimeouts.emplace(id, std::move(entry)); this->parkTimeoutCv.notify_all(); return id; } @@ -616,22 +625,25 @@ void DBDescriptor::runParkTimeoutLoop() { auto earliest = std::min_element( this->parkTimeouts.begin(), this->parkTimeouts.end(), - [](const std::unique_ptr& a, const std::unique_ptr& b) { - return a->deadline < b->deadline; - } + [](const auto& a, const auto& b) { return a.second->deadline < b.second->deadline; } ); auto now = std::chrono::steady_clock::now(); - if ((*earliest)->deadline > now) { - this->parkTimeoutCv.wait_until(lock, (*earliest)->deadline); + if (earliest->second->deadline > now) { + // Copy the deadline: wait_until releases the lock while parked, + // during which this entry can be erased (a real wake racing the + // timeout) and the map node freed -- a bound reference into it + // would be a read of freed memory once the wait re-checks time. + std::chrono::steady_clock::time_point deadline = earliest->second->deadline; + this->parkTimeoutCv.wait_until(lock, deadline); continue; } // Fire while still holding the mutex, like dispatchCommitCompletion. for (auto it = this->parkTimeouts.begin(); it != this->parkTimeouts.end();) { - if ((*it)->deadline > now) { + if (it->second->deadline > now) { ++it; continue; } - std::unique_ptr due = std::move(*it); + std::unique_ptr due = std::move(it->second); it = this->parkTimeouts.erase(it); bool expected = false; if (due->fired->compare_exchange_strong(expected, true)) { @@ -644,17 +656,13 @@ void DBDescriptor::runParkTimeoutLoop() { void DBDescriptor::fireParkTimeout(uint64_t id) { std::lock_guard lock(this->parkTimeoutMutex); - auto it = std::find_if( - this->parkTimeouts.begin(), - this->parkTimeouts.end(), - [id](const std::unique_ptr& p) { return p->id == id; } - ); + auto it = this->parkTimeouts.find(id); if (it == this->parkTimeouts.end()) { // Already claimed by the timeout thread, releaseParkTimeoutsByEnv, or // shutdownParkTimeouts. return; } - std::unique_ptr owned = std::move(*it); + std::unique_ptr owned = std::move(it->second); this->parkTimeouts.erase(it); bool expected = false; if (owned->fired->compare_exchange_strong(expected, true)) { @@ -666,7 +674,7 @@ void DBDescriptor::fireParkTimeout(uint64_t id) { void DBDescriptor::releaseParkTimeoutsByEnv(napi_env env) { std::lock_guard lock(this->parkTimeoutMutex); for (auto it = this->parkTimeouts.begin(); it != this->parkTimeouts.end();) { - if ((*it)->env != env) { + if (it->second->env != env) { ++it; continue; } @@ -674,9 +682,9 @@ void DBDescriptor::releaseParkTimeoutsByEnv(napi_env env) { // wake ever calls into the tsfn we're about to release -- the // promise's env is gone, nothing is listening for the resolve. bool expected = false; - (*it)->fired->compare_exchange_strong(expected, true); + it->second->fired->compare_exchange_strong(expected, true); if (!expected) { - ::napi_release_threadsafe_function((*it)->tsfn, napi_tsfn_release); + ::napi_release_threadsafe_function(it->second->tsfn, napi_tsfn_release); } it = this->parkTimeouts.erase(it); } @@ -696,21 +704,16 @@ void DBDescriptor::shutdownParkTimeouts() { toJoin = std::move(this->parkTimeoutThread); this->parkTimeoutThreadStarted = false; } - // Resolve every park still pending, under the same mutex - // releaseParkTimeoutsByEnv/fireParkTimeout/runParkTimeoutLoop all - // serialize their tsfn calls on -- draining outside the lock would - // let a concurrent releaseParkTimeoutsByEnv for a dying env observe - // "nothing to cancel" (we already pulled its entry out) while we are - // mid-call on that exact env's tsfn, racing Node freeing it. RETRY_NOW - // is the correct outcome for a park whose descriptor is closing - // regardless of whether its real holder's wake ever reaches it (see - // the header comment on the cross-database VT-slot-collision case - // this closes). + // Resolve every park still pending, under the same mutex the other + // three methods serialize their tsfn calls on -- draining outside the + // lock would let a concurrent releaseParkTimeoutsByEnv for a dying + // env observe "nothing to cancel" while this is mid-call on that + // same env's tsfn, racing Node freeing it. for (auto& entry : this->parkTimeouts) { bool expected = false; - if (entry->fired->compare_exchange_strong(expected, true)) { - ::napi_call_threadsafe_function(entry->tsfn, nullptr, napi_tsfn_nonblocking); - ::napi_release_threadsafe_function(entry->tsfn, napi_tsfn_release); + if (entry.second->fired->compare_exchange_strong(expected, true)) { + ::napi_call_threadsafe_function(entry.second->tsfn, nullptr, napi_tsfn_nonblocking); + ::napi_release_threadsafe_function(entry.second->tsfn, napi_tsfn_release); } } this->parkTimeouts.clear(); diff --git a/src/binding/database/db_descriptor.h b/src/binding/database/db_descriptor.h index 903493c77..0b9baa8ea 100644 --- a/src/binding/database/db_descriptor.h +++ b/src/binding/database/db_descriptor.h @@ -299,22 +299,16 @@ struct DBDescriptor final : public std::enable_shared_from_this { /** * Bounded wait for a coordinated-retry commit parked on a conflicting - * holder's VT lock (`completeCommitWork` in transaction.cpp), so a holder - * that never releases (leaked/abandoned transaction) resolves RETRY_NOW - * instead of parking forever (harper#2001). One entry per outstanding - * park, identified by a monotonic `id` rather than the entry's address -- - * `LockTracker::wakeCallbacks` has no removal API (a known, documented - * gap; see the AGENTS.md note), so a stale closure can outlive its entry - * and its `unique_ptr`'s heap address can be reused by a later park; an - * address-keyed lookup would then resolve the wrong park. `fired` is the - * exactly-once gate shared with the LockTracker wake callback registered - * on the same park -- whichever side wins the CAS calls+releases `tsfn`, - * the loser touches nothing. Owned by the descriptor (not one-thread- - * per-park and not the per-park heap state) so `parkTimeoutThread` -- - * lazily started, joined at close like `commitWorker` -- is the only - * thread ever created for this, and so a dying env's entries can be - * scrubbed by the same env-cleanup hook that releases commit - * completions instead of firing into a tsfn Node is about to free. + * holder's VT lock, so a holder that never releases resolves RETRY_NOW + * instead of parking forever (harper#2001, see AGENTS.md). Keyed by a + * monotonic `id`, not the entry's address: `LockTracker::wakeCallbacks` + * has no removal API, so a stale closure can outlive its entry and an + * address-keyed lookup could resolve a later, unrelated park reusing the + * freed address. `fired` is the exactly-once gate shared with the + * LockTracker wake callback for the same park -- whichever side wins the + * CAS calls+releases `tsfn`. `LockTracker::wake()` runs `fireParkTimeout` + * under the process-global VT `writerMutex_`, so the map (not a vector) + * is what keeps that lookup O(1). */ struct ParkTimeout { uint64_t id; @@ -325,10 +319,7 @@ struct DBDescriptor final : public std::enable_shared_from_this { }; std::mutex parkTimeoutMutex; std::condition_variable parkTimeoutCv; - // Small and bounded by outstanding parks (each entry is removed by - // whichever of fireParkTimeout / releaseParkTimeoutsByEnv claims it - // first) -- not by total parks ever registered. - std::vector> parkTimeouts; + std::unordered_map> parkTimeouts; uint64_t nextParkTimeoutId = 1; std::thread parkTimeoutThread; bool parkTimeoutThreadStarted = false; @@ -336,14 +327,10 @@ struct DBDescriptor final : public std::enable_shared_from_this { /** * JS thread (`completeCommitWork`). Registers a bounded wait, lazily - * starting the descriptor's single park-timeout thread. Returns the - * new entry's id to capture in the LockTracker wake callback so a - * genuine wake can also resolve through `fireParkTimeout` -- or 0 if the - * descriptor is already closing. The caller must then resolve inline - * without registering with the LockTracker at all: by this point - * `cancelForDB` has already run, so a lock that becomes parkable only - * after that point (the `cancelForDB` / `shutdownParkTimeouts` window) - * would otherwise park with no timeout backing it. + * starting the descriptor's single park-timeout thread. Returns the new + * entry's id, or 0 if the descriptor is closing or thread creation + * failed -- either way the caller resolves inline instead of parking + * with no timeout thread behind it. */ uint64_t scheduleParkTimeout( napi_env env, @@ -370,18 +357,11 @@ struct DBDescriptor final : public std::enable_shared_from_this { /** * Descriptor close: stop and join the park-timeout thread, then resolve - * (call+release) every park still pending. `cancelForDB` (called just - * before this, in `finishClose()`) wakes every lock tagged with this - * descriptor's own `vtEpoch`, but a park can be registered on a tracker - * installed by a *different* database on a colliding VT slot - * (`VerificationTable::lockSlotForWrite` joins an existing tracker - * without retagging its `dbId`) -- that lock's eventual release wakes - * the other database, not this one, so this descriptor's own park would - * otherwise wait on a wake that may never come from its perspective. - * Draining here guarantees every park this descriptor scheduled settles - * by the time it closes, independent of which VT tracker it ended up on. - * Idempotent (safe to call from both `finishClose()` and the destructor, - * matching `commitWorker`'s own belt-and-suspenders shutdown call). + * (call+release) every park still pending. `cancelForDB`, called just + * before this, cannot be relied on to have woken everything -- a park + * can be registered on a foreign-`dbId` tracker (colliding VT slot) that + * only wakes a different database. Idempotent (called from both + * `finishClose()` and the destructor, matching `commitWorker`). */ void shutdownParkTimeouts(); diff --git a/src/binding/transaction/transaction.cpp b/src/binding/transaction/transaction.cpp index c18601f7a..d9372bf0c 100644 --- a/src/binding/transaction/transaction.cpp +++ b/src/binding/transaction/transaction.cpp @@ -10,6 +10,7 @@ #include "database/database.h" #include "database/db_descriptor.h" #include "database/db_handle.h" +#include "database/db_registry.h" #include "iterator/db_iterator.h" #include "database/db_settings.h" #include "napi/macros.h" @@ -228,20 +229,12 @@ static void retryNowFinalize(napi_env env, void* finalizeData, void* /*hint*/) { /** * Bounded wait (ms) for a coordinated-retry commit parked on a conflicting - * holder's VT lock, selected by ROCKSDB_JS_PARK_TIMEOUT_MS. If the holder - * never releases (leaked/abandoned transaction, or a wake lost to #741's - * double-release corruption), the park resolves RETRY_NOW anyway once this - * elapses instead of hanging forever (harper#2001). A spurious early - * RETRY_NOW is harmless -- the JS layer just retries and may conflict again, - * consuming a coordinatedRetry attempt as it would for a genuine wake; the - * default is deliberately the top of the 2-5s range this was scoped to, to - * leave the most headroom for a holder that is merely slow (a large batch - * commit, backpressure under compaction) rather than abandoned, since a - * commit's transaction.commit() attempts are finite (maxRetries, default 3). - * Malformed input (non-numeric, negative, out of range) falls back to the - * default rather than `atoi`'s silent 0 (immediate-fire spin) or a negative - * value wrapping through `unsigned` into a multi-day effective hang -- this - * is an operational knob someone may reach for mid-incident, not a test seam. + * holder's VT lock, selected by ROCKSDB_JS_PARK_TIMEOUT_MS (harper#2001). + * Default is the top of the 2-5s range this was scoped to: a timeout + * consumes a coordinatedRetry attempt like a genuine wake would, so a + * higher default leaves more headroom for a merely-slow (not abandoned) + * holder before maxRetries exhausts. Malformed input falls back to the + * default rather than producing a degenerate effective timeout. */ static unsigned parkTimeoutMs() { static const unsigned ms = []() -> unsigned { @@ -539,7 +532,17 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { auto fireOnce = [tsfn, fired, weakDescriptor, parkId]() { if (parkId != 0) { if (auto d = weakDescriptor.lock()) { + // This lock() is itself a transient extra ref, exactly + // the shape that can make a racing close()'s + // PurgeIfUnreferenced observe use_count() > 1 and skip + // (HarperFast/rocksdb-js#672) -- retry it after + // dropping our ref, like BackupState/checkpoint state + // do for the same reason. d->fireParkTimeout(parkId); + std::string path = d->path; + bool readOnly = d->readOnly; + d.reset(); + DBRegistry::PurgeIfUnreferenced(path, readOnly); } return; } diff --git a/test/lock-tracker.test.ts b/test/lock-tracker.test.ts index 2ebe0cd20..a1aff06f3 100644 --- a/test/lock-tracker.test.ts +++ b/test/lock-tracker.test.ts @@ -119,6 +119,7 @@ describe('Coordinated retry (Phase 3)', () => { // Force IsBusy by running concurrent transactions writing the same key // under coordinatedRetry: true. database.ts handles RETRY_NOW internally // via immediate retry; callers never see it as a return value. + const start = Date.now(); const results = await Promise.allSettled( Array.from({ length: 4 }, async (_, i) => { const v = Buffer.alloc(16); @@ -131,6 +132,7 @@ describe('Coordinated retry (Phase 3)', () => { ); }) ); + const elapsed = Date.now() - start; // All transactions should eventually succeed (coordinatedRetry retries // without error) or fail gracefully; none should throw unexpectedly. @@ -141,6 +143,12 @@ describe('Coordinated retry (Phase 3)', () => { } } + // A conflict here resolves via LockTracker's real wake (the other + // commit finishing), not the #741 park timeout (default 5000ms) -- + // bound the wall clock well under that so a broken wake path can't + // hide behind the timeout and still pass. + expect(elapsed).toBeLessThan(3000); + // Slot should be 0 (released) after all transactions settle. const newV = 2.5e12; db.populateVersion(key, newV); From 203c12c6bb44612fe477d048931f953631ddb86a Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sat, 1 Aug 2026 17:00:36 -0600 Subject: [PATCH 06/19] ci: retry Deno tests on macos-latest for a known pre-existing teardown race commit-teardown.test.ts crashes with SIGABRT (mutex lock failed: Invalid argument) inside the shared commit-thread teardown fixture on Deno macOS CI. Confirmed pre-existing and unrelated to this PR's diff by finding the identical crash on two other unrelated PRs in the last 24h (a dependabot bump and fix/dropped-cf-write-poisons-env), hitting both commit-thread modes. Filed HarperFast/rocksdb-js#746 to track the root cause and added a retry for macos-latest, mirroring the existing windows-latest retry (HarperFast/rocksdb-js#695) so this pre-existing race doesn't block PR #744. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012F3qheoCwRvpjgjgu5Vc1K --- .github/workflows/pr.yml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index ea40df0e1..e973c2035 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -214,5 +214,12 @@ jobs: run: pnpm build - name: Run tests on Deno + # macos-latest gets an extra retry for a known pre-existing native crash in + # commit-teardown.test.ts (HarperFast/rocksdb-js#746, mutex lock failed: + # Invalid argument during worker-env teardown vs. the shared commit thread) + # that has hit multiple unrelated PRs on this OS/runtime combo. Not a fix + # for #746 itself, just enough retry budget that a rare pre-existing race + # doesn't block unrelated PRs -- same pattern as the windows-latest retry + # below (HarperFast/rocksdb-js#695). shell: bash - run: pnpm test:deno || ([ "${{ matrix.os }}" = "windows-latest" ] && pnpm test:deno) + run: pnpm test:deno || ([ "${{ matrix.os }}" = "windows-latest" -o "${{ matrix.os }}" = "macos-latest" ] && pnpm test:deno) From d09d9ff72a4a47e25e003e170a966b3e8ac043e8 Mon Sep 17 00:00:00 2001 From: Chris Barber Date: Mon, 24 Aug 2026 11:19:34 -0500 Subject: [PATCH 07/19] Fix formatting --- AGENTS.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 98799c25e..cec863747 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -415,6 +415,7 @@ sufficient (env teardown does not honor tsfn acquire counts); see if a zero were taken as a terminator, would let a chain "end" anywhere in megabytes of padding. Resolve it only on a break — `getLogFileSize` crosses into native and takes the store mutex, so a per-frame call would tax every healthy read. + 12. **Coordinated retry parks on a lock, bounded by a descriptor-owned timeout**: a `coordinatedRetry` commit that loses a conflict (`IsBusy`) parks instead of rejecting immediately — `completeCommitWork` (`src/binding/transaction/transaction.cpp`) registers a wake callback on the @@ -460,7 +461,7 @@ sufficient (env teardown does not honor tsfn acquire counts); see memory-growth concern, not a correctness one) — deferred rather than risking an unreviewed change to `verification_table.cpp`'s concurrency invariants under this fix's scope. -12. **A dropped transaction must release itself**: `DBDescriptor::transactionAdd` holds a **strong** +13. **A dropped transaction must release itself**: `DBDescriptor::transactionAdd` holds a **strong** `shared_ptr` (the parallel `closables` entry is weak), so the registry alone keeps a `TransactionHandle` alive and `~TransactionHandle` — hence `close()`, the only `ClearSnapshot()` path — is unreachable while it is registered. The `NativeTransaction` finalizer therefore calls @@ -482,7 +483,7 @@ sufficient (env teardown does not honor tsfn acquire counts); see handle fields that are fixed before publication (`id`, `createdAt`), because `txnsMutex` covers map membership while mutable-field writers hold no lock. -13. **A recovered active transaction-log file ends on a transaction boundary when recovery can +14. **A recovered active transaction-log file ends on a transaction boundary when recovery can prove one**: only a batch's final entry carries `TRANSACTION_LOG_ENTRY_LAST_FLAG`, so a crash mid-batch leaves whole, well-framed entries that are a _prefix_ of a transaction. `recoverTail()` discards them From 68dea99c285381e9359a7fe2cd28456ad363b571 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 24 Aug 2026 16:03:43 -0600 Subject: [PATCH 08/19] fix(transaction): keep the park wake path off the descriptor and registry A LockTracker wake callback is invoked inline by LockTracker::wake() with the process-global VT writerMutex_ held, so DBRegistry::PurgeIfUnreferenced from that callback could claim the purge and run finishClose() -> cancelForDB() -> a second lock of the same non-recursive mutex, wedging every database's write-intent path -- the harper#2001 symptom this bound exists to fix. Move the park bookkeeping into a standalone ParkTimeoutRegistry owned by the descriptor through a shared_ptr, and have the wake closure hold a weak_ptr to that instead of to the descriptor: fire() touches one mutex and one map, the transient .lock() no longer perturbs the use_count PurgeIfUnreferenced decides on, and the purge retry (and its databasesMutex acquisition under the VT lock) is gone. Also from review: - index parks by deadline as well as id, so the timeout thread stops re-scanning the whole map on every wakeup while fire() waits on that mutex under the global VT lock, and only notify when the new park is the earliest - read ROCKSDB_JS_PARK_TIMEOUT_MS per park instead of once per process, and clamp positive values up to a 50ms floor (0 still falls back to the default) - cover db.close() with a commit still parked - use performance.now() for the elapsed-time assertions - gate the macOS Deno retry on rocksdb-js#746's crash signature so an unrelated macOS regression still fails on the first run Refs #741 Co-Authored-By: Claude Opus --- .github/workflows/pr.yml | 28 +++- AGENTS.md | 68 ++++++--- src/binding/database/db_descriptor.cpp | 170 ++++++++++++---------- src/binding/database/db_descriptor.h | 185 +++++++++++++++--------- src/binding/database/db_registry.cpp | 2 +- src/binding/transaction/transaction.cpp | 99 ++++++------- test/lock-tracker.test.ts | 44 +++++- 7 files changed, 366 insertions(+), 230 deletions(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index e973c2035..9ec9da051 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -214,12 +214,24 @@ jobs: run: pnpm build - name: Run tests on Deno - # macos-latest gets an extra retry for a known pre-existing native crash in - # commit-teardown.test.ts (HarperFast/rocksdb-js#746, mutex lock failed: - # Invalid argument during worker-env teardown vs. the shared commit thread) - # that has hit multiple unrelated PRs on this OS/runtime combo. Not a fix - # for #746 itself, just enough retry budget that a rare pre-existing race - # doesn't block unrelated PRs -- same pattern as the windows-latest retry - # below (HarperFast/rocksdb-js#695). + # macos-latest gets an extra retry ONLY for the known pre-existing native + # crash in commit-teardown.test.ts (HarperFast/rocksdb-js#746, "mutex lock + # failed: Invalid argument" during worker-env teardown vs. the shared commit + # thread), which has hit multiple unrelated PRs on this OS/runtime combo. + # Gating on that crash string, rather than re-rolling any failure like the + # windows-latest retry below (HarperFast/rocksdb-js#695), keeps this OS's + # signal for every other regression. Not a fix for #746 itself. shell: bash - run: pnpm test:deno || ([ "${{ matrix.os }}" = "windows-latest" -o "${{ matrix.os }}" = "macos-latest" ] && pnpm test:deno) + run: | + # pipefail is what makes the `| tee` below report the test exit code + # instead of tee's. + set -o pipefail + if pnpm test:deno 2>&1 | tee "$RUNNER_TEMP/deno-test.log"; then + exit 0 + fi + case "${{ matrix.os }}" in + windows-latest) ;; + macos-latest) grep -q 'mutex lock failed' "$RUNNER_TEMP/deno-test.log" || exit 1 ;; + *) exit 1 ;; + esac + pnpm test:deno diff --git a/AGENTS.md b/AGENTS.md index cec863747..f2cb96a62 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -196,7 +196,10 @@ sufficient (env teardown does not honor tsfn acquire counts); see - `ROCKSDB_JS_PARK_TIMEOUT_MS` - Bounded wait (default `5000`) before a coordinated-retry commit parked on a conflicting holder's VT lock resolves RETRY_NOW unconditionally, in case the holder never releases (see - "Coordinated retry" note below) + "Coordinated retry" note below). Read per park; values below `50` are clamped + up to it, and `0` (an ambiguous "disable the bound") falls back to the default + like any malformed value. There is no opt-out: a deployment that would rather + wait than fail a legitimately slow holder raises the value instead ## Test Structure @@ -423,37 +426,58 @@ sufficient (env teardown does not honor tsfn acquire counts); see that lock's last holder releases (`VerificationTable::releaseWriteIntent` → `LockTracker::wake`). A holder that never releases — a leaked/abandoned transaction, or a wake lost to a bug elsewhere — would otherwise park forever (harper#2001: a worker's write path disabled for 5+ hours until - restart). `DBDescriptor::scheduleParkTimeout` (`db_descriptor.{h,cpp}`) bounds this with + restart). `ParkTimeoutRegistry` (`db_descriptor.{h,cpp}`) bounds this with `ROCKSDB_JS_PARK_TIMEOUT_MS` (default `5000` — the top of this fix's requested 2-5s range, to leave maximum headroom for a holder that is merely slow rather than abandoned, since a timeout consumes a `coordinatedRetry` attempt exactly like a real wake does and `maxRetries` is finite): - one park-timeout thread **per descriptor** — lazily started, joined at `finishClose()` (and again, - idempotently, from the destructor as a safety net, matching `commitWorker`) — tracks every - outstanding deadline instead of spawning a thread per park (the contention path is exactly where - an abandoned holder makes parks dense, so per-park threads would be a resource cliff, not a fix). + one registry, and one lazily-started timeout thread, **per descriptor** — joined at + `finishClose()` (and again, idempotently, from the destructor as a safety net, matching + `commitWorker`) — tracks every outstanding deadline instead of spawning a thread per park (the + contention path is exactly where an abandoned holder makes parks dense, so per-park threads would + be a resource cliff, not a fix). Deliberately a plain `std::thread`, not a `uv_timer_t`: this addon + ships one prebuilt binary across Node ABI versions via N-API, and libuv's struct layout is not part + of that stable surface. + + **A `LockTracker` wake callback runs under the process-global VT `writerMutex_`, so it must not + block and must not re-enter the VT or `DBRegistry`.** `LockTracker::wake()` invokes its callbacks + inline and both callers (`releaseWriteIntent`, `cancelForDB`) hold that mutex across the whole + function. Re-entering the registry from there self-deadlocks: `DBRegistry::PurgeIfUnreferenced` + can claim the purge and call `finishClose()` → `cancelForDB()` → a second lock of the same + non-recursive `writerMutex_`, wedging every database's write-intent path process-wide — the exact + symptom this note exists to fix. It is also an AB-BA against `finishClose`'s `txnsMutex` → + `writerMutex_` order, and it would run a flush, a manual compaction, `WaitForCompact` and thread + joins under the global VT lock. That is why `ParkTimeoutRegistry` is a standalone object owned by + the descriptor through a `shared_ptr` rather than state on the descriptor itself: the wake closure + captures a **`std::weak_ptr`** and calls only `fire(id)`, which touches one + mutex and one map. Weak, not raw, because a park can end up registered on a tracker installed by a + _different_ database on a colliding VT slot (`VerificationTable::lockSlotForWrite` joins an existing + tracker without retagging its `dbId`), so that lock's eventual release wakes a park whose own + database may already have closed — `cancelForDB()` only wakes trackers tagged with _its own_ + `vtEpoch`, so it cannot be relied on to have resolved a foreign-`dbId` park first. Weak **to the + registry and not to the descriptor** because a `weak_ptr::lock()` is a transient extra + reference, and `PurgeIfUnreferenced` decides on `use_count() <= 1`: a racing close would see the + inflated count, skip the purge, and leak the registry entry plus the open RocksDB — the + HarperFast/rocksdb-js#672 hazard, which the wake path cannot repair by retrying the purge (that is + the re-entrancy above). `.lock()` failing is the expected outcome once the owning database closes: + `ParkTimeoutRegistry::shutdown()` (called from `finishClose()` right after `cancelForDB`, before + the descriptor can be destroyed) unconditionally resolves every park it still holds regardless of + whether the real holder ever wakes it, so by the time the weak reference can fail, the park has + already settled. + Each park is identified by a monotonic `uint64_t id`, not its entry's address: `LockTracker::wakeCallbacks` has no removal API (see the gap noted below), so a stale closure can outlive its entry, and an address-keyed lookup risks resolving a _different_, later park that reused the same freed heap address. The timeout thread and the LockTracker wake callback race through one heap-allocated `std::atomic` per park (independent of the per-park `RetryNowContext`, whose refs/TSFN the - winning side's release eventually frees) — whichever fires first calls+releases the TSFN under - `parkTimeoutMutex` and erases the entry; the loser finds it already gone and touches nothing. That - same mutex is what a dying env's `releaseParkTimeoutsByEnv` (wired into the module env-cleanup + winning side's release eventually frees) — whichever fires first calls+releases the TSFN under the + registry's `mutex` and erases the entry; the loser finds it already gone and touches nothing. That + same mutex is what a dying env's `releaseByEnv` (wired into the module env-cleanup hook next to `ReleaseCommitCompletionsByEnv`) takes to cancel — release without calling — that env's pending parks before Node frees their tsfns; `retryNowCallJs` also guards `env == nullptr` - like `commitCompletionCallJs` does, for the same tsfn-queue-drained-during-teardown reason. The - LockTracker wake closure captures a **`std::weak_ptr`**, not a raw pointer: a park - can end up registered on a tracker installed by a _different_ database on a colliding VT slot - (`VerificationTable::lockSlotForWrite` joins an existing tracker without retagging its `dbId`), so - that lock's eventual release wakes a park whose own descriptor may have already closed and been - destroyed — `cancelForDB()` only wakes trackers tagged with _its own_ `vtEpoch`, so it cannot be - relied on to have resolved a foreign-`dbId` park before the descriptor goes away. `.lock()` failing - is the expected outcome once that happens: `shutdownParkTimeouts()` (called from `finishClose()` - right after `cancelForDB`, before the descriptor can be destroyed) unconditionally resolves every - park still in its own `parkTimeouts` regardless of whether the real holder ever wakes it, so by the - time the weak reference can fail to lock, the park has already settled. Deliberately a plain - `std::thread`, not a `uv_timer_t`: this addon ships one prebuilt binary across Node ABI versions via - N-API, and libuv's struct layout is not part of that stable surface. Known gap: `LockTracker::wakeCallbacks` + like `commitCompletionCallJs` does, for the same tsfn-queue-drained-during-teardown reason. Parks + are indexed twice, by id and by deadline (`std::multimap`): `fire()` needs an O(1) lookup because it + runs under the global VT mutex, and the timeout thread needs the earliest deadline on every wakeup + without an O(N) scan on that same lock. Known gap: `LockTracker::wakeCallbacks` itself has no removal API. Before this change an abandoned holder accrued one inert callback per waiter and then everything hung; now each waiter re-parks (and re-registers) every `ROCKSDB_JS_PARK_TIMEOUT_MS` up to `maxRetries`, so registrations accumulate per _retry_ rather than diff --git a/src/binding/database/db_descriptor.cpp b/src/binding/database/db_descriptor.cpp index 4febe93bd..8cd211545 100644 --- a/src/binding/database/db_descriptor.cpp +++ b/src/binding/database/db_descriptor.cpp @@ -361,7 +361,7 @@ DBDescriptor::~DBDescriptor() { this->close(); // Idempotent safety net, matching commitWorker/logWorker's own // destructor shutdown. - this->shutdownParkTimeouts(); + this->parkTimeouts->shutdown(); } /** @@ -482,8 +482,8 @@ void DBDescriptor::finishClose() { // A park can be registered on a foreign-dbId tracker (colliding VT slot; // see the ParkTimeout header comment), so cancelForDB() above cannot be // relied on to have woken everything this descriptor is waiting on. - // shutdownParkTimeouts() resolves whatever is left regardless. - this->shutdownParkTimeouts(); + // ParkTimeoutRegistry::shutdown() resolves whatever is left regardless. + this->parkTimeouts->shutdown(); // Unregister from transaction log store registry - this will clean up stores // when the last descriptor for this path is closed @@ -575,21 +575,21 @@ void DBDescriptor::releaseCommitCompletionsByEnv(napi_env env) { } } -uint64_t DBDescriptor::scheduleParkTimeout( +uint64_t ParkTimeoutRegistry::schedule( napi_env env, unsigned timeoutMs, napi_threadsafe_function tsfn, std::shared_ptr> fired ) { - std::lock_guard lock(this->parkTimeoutMutex); - if (this->parkTimeoutStopped) { + std::lock_guard lock(this->mutex); + if (this->stopped) { // Descriptor already closing: the caller must resolve inline without // registering with the LockTracker at all (see the header comment). return 0; } - if (!this->parkTimeoutThreadStarted) { + if (!this->threadStarted) { try { - this->parkTimeoutThread = std::thread([this]() { this->runParkTimeoutLoop(); }); + this->thread = std::thread([this]() { this->runLoop(); }); } catch (const std::system_error&) { // Thread creation failed (e.g. thread/resource exhaustion): leave // the flag false so the next park retries, and tell the caller to @@ -597,83 +597,97 @@ uint64_t DBDescriptor::scheduleParkTimeout( // ever time out. return 0; } - this->parkTimeoutThreadStarted = true; + this->threadStarted = true; } auto entry = std::make_unique(); - entry->id = this->nextParkTimeoutId++; - entry->deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(timeoutMs); + entry->id = this->nextId++; entry->env = env; entry->tsfn = tsfn; entry->fired = std::move(fired); uint64_t id = entry->id; - this->parkTimeouts.emplace(id, std::move(entry)); - this->parkTimeoutCv.notify_all(); + auto deadlineIt = this->deadlines.emplace( + std::chrono::steady_clock::now() + std::chrono::milliseconds(timeoutMs), + id + ); + entry->deadlineIt = deadlineIt; + this->parks.emplace(id, std::move(entry)); + if (deadlineIt == this->deadlines.begin()) { + // Only the new earliest deadline needs the loop re-armed (this also + // covers waking it out of the indefinite wait when `deadlines` was + // empty); any later one already fires within a wait it will take. + this->cv.notify_all(); + } return id; } -void DBDescriptor::runParkTimeoutLoop() { +std::unique_ptr ParkTimeoutRegistry::take(uint64_t id) { + auto it = this->parks.find(id); + if (it == this->parks.end()) { + return nullptr; + } + std::unique_ptr owned = std::move(it->second); + this->deadlines.erase(owned->deadlineIt); + this->parks.erase(it); + return owned; +} + +void ParkTimeoutRegistry::resolve(ParkTimeout& park) { + bool expected = false; + if (park.fired->compare_exchange_strong(expected, true)) { + ::napi_call_threadsafe_function(park.tsfn, nullptr, napi_tsfn_nonblocking); + ::napi_release_threadsafe_function(park.tsfn, napi_tsfn_release); + } +} + +void ParkTimeoutRegistry::runLoop() { setThreadName("rocksdb-park-timeout"); - std::unique_lock lock(this->parkTimeoutMutex); + std::unique_lock lock(this->mutex); for (;;) { - if (this->parkTimeoutStopped) { + if (this->stopped) { return; } - if (this->parkTimeouts.empty()) { - this->parkTimeoutCv.wait(lock); + if (this->deadlines.empty()) { + this->cv.wait(lock); continue; } - auto earliest = std::min_element( - this->parkTimeouts.begin(), - this->parkTimeouts.end(), - [](const auto& a, const auto& b) { return a.second->deadline < b.second->deadline; } - ); auto now = std::chrono::steady_clock::now(); - if (earliest->second->deadline > now) { - // Copy the deadline: wait_until releases the lock while parked, - // during which this entry can be erased (a real wake racing the - // timeout) and the map node freed -- a bound reference into it - // would be a read of freed memory once the wait re-checks time. - std::chrono::steady_clock::time_point deadline = earliest->second->deadline; - this->parkTimeoutCv.wait_until(lock, deadline); + // Copy the deadline: wait_until releases the lock while parked, during + // which this entry can be erased (a real wake racing the timeout) and + // the map node freed -- a bound reference into it would be a read of + // freed memory once the wait re-checks time. + std::chrono::steady_clock::time_point earliest = this->deadlines.begin()->first; + if (earliest > now) { + this->cv.wait_until(lock, earliest); continue; } // Fire while still holding the mutex, like dispatchCommitCompletion. - for (auto it = this->parkTimeouts.begin(); it != this->parkTimeouts.end();) { - if (it->second->deadline > now) { - ++it; + while (!this->deadlines.empty() && this->deadlines.begin()->first <= now) { + auto deadlineIt = this->deadlines.begin(); + auto parkIt = this->parks.find(deadlineIt->second); + this->deadlines.erase(deadlineIt); + if (parkIt == this->parks.end()) { continue; } - std::unique_ptr due = std::move(it->second); - it = this->parkTimeouts.erase(it); - bool expected = false; - if (due->fired->compare_exchange_strong(expected, true)) { - ::napi_call_threadsafe_function(due->tsfn, nullptr, napi_tsfn_nonblocking); - ::napi_release_threadsafe_function(due->tsfn, napi_tsfn_release); - } + std::unique_ptr due = std::move(parkIt->second); + this->parks.erase(parkIt); + ParkTimeoutRegistry::resolve(*due); } } } -void DBDescriptor::fireParkTimeout(uint64_t id) { - std::lock_guard lock(this->parkTimeoutMutex); - auto it = this->parkTimeouts.find(id); - if (it == this->parkTimeouts.end()) { - // Already claimed by the timeout thread, releaseParkTimeoutsByEnv, or - // shutdownParkTimeouts. +void ParkTimeoutRegistry::fire(uint64_t id) { + std::lock_guard lock(this->mutex); + std::unique_ptr owned = this->take(id); + if (!owned) { + // Already claimed by the timeout thread, releaseByEnv, or shutdown. return; } - std::unique_ptr owned = std::move(it->second); - this->parkTimeouts.erase(it); - bool expected = false; - if (owned->fired->compare_exchange_strong(expected, true)) { - ::napi_call_threadsafe_function(owned->tsfn, nullptr, napi_tsfn_nonblocking); - ::napi_release_threadsafe_function(owned->tsfn, napi_tsfn_release); - } + ParkTimeoutRegistry::resolve(*owned); } -void DBDescriptor::releaseParkTimeoutsByEnv(napi_env env) { - std::lock_guard lock(this->parkTimeoutMutex); - for (auto it = this->parkTimeouts.begin(); it != this->parkTimeouts.end();) { +void ParkTimeoutRegistry::releaseByEnv(napi_env env) { + std::lock_guard lock(this->mutex); + for (auto it = this->parks.begin(); it != this->parks.end();) { if (it->second->env != env) { ++it; continue; @@ -686,47 +700,49 @@ void DBDescriptor::releaseParkTimeoutsByEnv(napi_env env) { if (!expected) { ::napi_release_threadsafe_function(it->second->tsfn, napi_tsfn_release); } - it = this->parkTimeouts.erase(it); + this->deadlines.erase(it->second->deadlineIt); + it = this->parks.erase(it); } } -void DBDescriptor::shutdownParkTimeouts() { +void ParkTimeoutRegistry::shutdown() { std::thread toJoin; { - std::lock_guard lock(this->parkTimeoutMutex); - if (this->parkTimeoutStopped && !this->parkTimeoutThreadStarted) { + std::lock_guard lock(this->mutex); + if (this->stopped && !this->threadStarted) { // Already fully shut down (e.g. finishClose() already ran; this is // the destructor's belt-and-suspenders call) -- nothing left to do. return; } - this->parkTimeoutStopped = true; - if (this->parkTimeoutThreadStarted) { - toJoin = std::move(this->parkTimeoutThread); - this->parkTimeoutThreadStarted = false; + this->stopped = true; + if (this->threadStarted) { + toJoin = std::move(this->thread); + this->threadStarted = false; } // Resolve every park still pending, under the same mutex the other // three methods serialize their tsfn calls on -- draining outside the - // lock would let a concurrent releaseParkTimeoutsByEnv for a dying - // env observe "nothing to cancel" while this is mid-call on that - // same env's tsfn, racing Node freeing it. - for (auto& entry : this->parkTimeouts) { - bool expected = false; - if (entry.second->fired->compare_exchange_strong(expected, true)) { - ::napi_call_threadsafe_function(entry.second->tsfn, nullptr, napi_tsfn_nonblocking); - ::napi_release_threadsafe_function(entry.second->tsfn, napi_tsfn_release); - } + // lock would let a concurrent releaseByEnv for a dying env observe + // "nothing to cancel" while this is mid-call on that same env's tsfn, + // racing Node freeing it. + for (auto& entry : this->parks) { + ParkTimeoutRegistry::resolve(*entry.second); } - this->parkTimeouts.clear(); + this->parks.clear(); + this->deadlines.clear(); } // Notify + join outside the lock: the loop's cv.wait_until needs to - // re-acquire parkTimeoutMutex to observe parkTimeoutStopped and return, - // so joining while still holding it would deadlock. - this->parkTimeoutCv.notify_all(); + // re-acquire the mutex to observe `stopped` and return, so joining while + // still holding it would deadlock. + this->cv.notify_all(); if (toJoin.joinable()) { toJoin.join(); } } +ParkTimeoutRegistry::~ParkTimeoutRegistry() { + this->shutdown(); +} + /** * Registers a database resource to be closed when the descriptor is closed. * diff --git a/src/binding/database/db_descriptor.h b/src/binding/database/db_descriptor.h index 0b9baa8ea..5e8ad919e 100644 --- a/src/binding/database/db_descriptor.h +++ b/src/binding/database/db_descriptor.h @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -63,6 +64,117 @@ struct DBDeleter { } }; +/** + * Bounded waits for coordinated-retry commits parked on a conflicting holder's + * VT lock, so a holder that never releases resolves RETRY_NOW instead of + * parking forever (harper#2001, see AGENTS.md note 12). One instance per + * `DBDescriptor`, with one lazily-started thread tracking every outstanding + * deadline. + * + * Deliberately standalone — it holds no reference back to its descriptor, and + * nothing reachable from `fire()` touches the descriptor or `DBRegistry`. + * `fire()` is called from a `LockTracker` wake callback, which + * `LockTracker::wake()` invokes inline while the *process-global* VT + * `writerMutex_` is held (`VerificationTable::releaseWriteIntent` and + * `cancelForDB` both wake under it), so the standing invariant on that path is: + * do not block, and do not re-enter the VT or the registry. A + * `DBRegistry::PurgeIfUnreferenced` from there can claim the purge and run + * `finishClose()` -> `cancelForDB()` -> a second lock of that same + * non-recursive `writerMutex_`, wedging every database's write-intent path. + * Holding the wake closure's `weak_ptr` on this object rather than on the + * descriptor is also what keeps its transient `.lock()` out of the + * descriptor's `use_count`, which `PurgeIfUnreferenced` keys its "no handles + * left" decision on — so there is no purge-skip window to retry in the first + * place (the HarperFast/rocksdb-js#672 hazard). + * + * Parks are keyed by a monotonic `id`, not the entry's address: + * `LockTracker::wakeCallbacks` has no removal API, so a stale closure can + * outlive its entry and an address-keyed lookup could resolve a later, + * unrelated park that reused the freed address. `fired` is the exactly-once + * gate shared with that park's wake callback — whichever side wins the CAS + * calls+releases `tsfn`, always under `mutex` so a concurrent `releaseByEnv()` + * for a dying env cannot observe "nothing to cancel" while the other side is + * mid-call on that env's tsfn. + */ +class ParkTimeoutRegistry final { +public: + ~ParkTimeoutRegistry(); + + /** + * JS thread (`completeCommitWork`). Registers a bounded wait, lazily + * starting the single timeout thread. Returns the new park's id, or 0 if + * the registry is shut down or thread creation failed -- either way the + * caller resolves inline instead of parking with no timeout behind it. + */ + uint64_t schedule( + napi_env env, + unsigned timeoutMs, + napi_threadsafe_function tsfn, + std::shared_ptr> fired + ); + + /** + * Resolves a specific park early because its VT lock's holder released + * (LockTracker wake callback, any thread, VT `writerMutex_` held). A no-op + * if the id is already gone -- claimed by the timeout thread, by + * `releaseByEnv`, or drained by `shutdown`. + */ + void fire(uint64_t id); + + /** + * Module env-cleanup hook. Cancels every pending park registered for a + * dying env -- released, never called, so neither the timeout thread nor a + * later real wake can fire into a tsfn Node is about to free. + */ + void releaseByEnv(napi_env env); + + /** + * Descriptor close: stop and join the timeout thread, then resolve + * (call+release) every park still pending. `cancelForDB`, called just + * before this, cannot be relied on to have woken everything -- a park can + * be registered on a foreign-`dbId` tracker (colliding VT slot) that only + * wakes a different database. Idempotent (called from both `finishClose()` + * and the descriptor's destructor, matching `commitWorker`). + */ + void shutdown(); + +private: + using DeadlineIndex = std::multimap; + + struct ParkTimeout { + uint64_t id; + napi_env env; + napi_threadsafe_function tsfn; + std::shared_ptr> fired; + DeadlineIndex::iterator deadlineIt; + }; + + /** Detaches `id` from both indexes; null if already claimed. Holds `mutex`. */ + std::unique_ptr take(uint64_t id); + + /** + * Calls+releases a claimed park's tsfn, unless another side already won the + * exactly-once gate. Every caller holds `mutex`. + */ + static void resolve(ParkTimeout& park); + + /** Runs on `thread` until `shutdown()` stops it. */ + void runLoop(); + + std::mutex mutex; + std::condition_variable cv; + std::unordered_map> parks; + // Deadline-ordered view of `parks`: the timeout thread needs the earliest + // deadline on every wakeup, and scanning for it under `mutex` would put an + // O(N) loop on the same lock `fire()` must take while holding the global VT + // `writerMutex_`. + DeadlineIndex deadlines; + uint64_t nextId = 1; + std::thread thread; + bool threadStarted = false; + bool stopped = false; +}; + /** * Descriptor for a RocksDB database, its column families, and any in-flight * transactions. The DBRegistry uses this to track active databases and reuse @@ -298,76 +410,15 @@ struct DBDescriptor final : public std::enable_shared_from_this { void releaseCommitCompletionsByEnv(napi_env env); /** - * Bounded wait for a coordinated-retry commit parked on a conflicting - * holder's VT lock, so a holder that never releases resolves RETRY_NOW - * instead of parking forever (harper#2001, see AGENTS.md). Keyed by a - * monotonic `id`, not the entry's address: `LockTracker::wakeCallbacks` - * has no removal API, so a stale closure can outlive its entry and an - * address-keyed lookup could resolve a later, unrelated park reusing the - * freed address. `fired` is the exactly-once gate shared with the - * LockTracker wake callback for the same park -- whichever side wins the - * CAS calls+releases `tsfn`. `LockTracker::wake()` runs `fireParkTimeout` - * under the process-global VT `writerMutex_`, so the map (not a vector) - * is what keeps that lookup O(1). - */ - struct ParkTimeout { - uint64_t id; - std::chrono::steady_clock::time_point deadline; - napi_env env; - napi_threadsafe_function tsfn; - std::shared_ptr> fired; - }; - std::mutex parkTimeoutMutex; - std::condition_variable parkTimeoutCv; - std::unordered_map> parkTimeouts; - uint64_t nextParkTimeoutId = 1; - std::thread parkTimeoutThread; - bool parkTimeoutThreadStarted = false; - bool parkTimeoutStopped = false; - - /** - * JS thread (`completeCommitWork`). Registers a bounded wait, lazily - * starting the descriptor's single park-timeout thread. Returns the new - * entry's id, or 0 if the descriptor is closing or thread creation - * failed -- either way the caller resolves inline instead of parking - * with no timeout thread behind it. - */ - uint64_t scheduleParkTimeout( - napi_env env, - unsigned timeoutMs, - napi_threadsafe_function tsfn, - std::shared_ptr> fired - ); - - /** - * Fires a specific park's timeout early because its VT lock's holder - * released (LockTracker wake callback, any thread). A no-op if the id is - * already gone -- claimed by the timeout thread, by - * releaseParkTimeoutsByEnv, or drained at shutdown. - */ - void fireParkTimeout(uint64_t id); - - /** - * Module env-cleanup hook. Cancels every pending park timeout registered - * for a dying env -- released, never called, so the background thread - * (or a later real wake) can never fire into a tsfn Node is about to - * free. - */ - void releaseParkTimeoutsByEnv(napi_env env); - - /** - * Descriptor close: stop and join the park-timeout thread, then resolve - * (call+release) every park still pending. `cancelForDB`, called just - * before this, cannot be relied on to have woken everything -- a park - * can be registered on a foreign-`dbId` tracker (colliding VT slot) that - * only wakes a different database. Idempotent (called from both - * `finishClose()` and the destructor, matching `commitWorker`). + * Bounded waits for this database's parked coordinated-retry commits. + * Never null; owned by shared_ptr so a LockTracker wake callback can hold + * a weak reference to it without referencing the descriptor (see + * `ParkTimeoutRegistry`). Drained and joined by `finishClose()`. */ - void shutdownParkTimeouts(); + const std::shared_ptr parkTimeouts = + std::make_shared(); private: - /** Runs on parkTimeoutThread until shutdownParkTimeouts() stops it. */ - void runParkTimeoutLoop(); DBDescriptor( const std::string& path, const DBOptions& options, diff --git a/src/binding/database/db_registry.cpp b/src/binding/database/db_registry.cpp index 316aeee5c..d1c669815 100644 --- a/src/binding/database/db_registry.cpp +++ b/src/binding/database/db_registry.cpp @@ -680,7 +680,7 @@ void DBRegistry::ReleaseParkTimeoutsByEnv(napi_env env) { } for (auto& descriptor : descriptors) { - descriptor->releaseParkTimeoutsByEnv(env); + descriptor->parkTimeouts->releaseByEnv(env); } } diff --git a/src/binding/transaction/transaction.cpp b/src/binding/transaction/transaction.cpp index d9372bf0c..2fcb05cfd 100644 --- a/src/binding/transaction/transaction.cpp +++ b/src/binding/transaction/transaction.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -233,36 +234,39 @@ static void retryNowFinalize(napi_env env, void* finalizeData, void* /*hint*/) { * Default is the top of the 2-5s range this was scoped to: a timeout * consumes a coordinatedRetry attempt like a genuine wake would, so a * higher default leaves more headroom for a merely-slow (not abandoned) - * holder before maxRetries exhausts. Malformed input falls back to the - * default rather than producing a degenerate effective timeout. + * holder before maxRetries exhausts -- a deployment that would rather wait + * than fail raises this, it cannot disable the bound. Read per park (this is + * the conflict path, not a hot path) so a process can be started with a small + * value for tests without depending on which park happens to run first. + * Malformed input falls back to the default rather than producing a + * degenerate effective timeout; a positive value below kMinimum is clamped up + * to it instead, since the intent there is unambiguous. */ static unsigned parkTimeoutMs() { - static const unsigned ms = []() -> unsigned { - constexpr unsigned kDefault = 5000; - const char* v = ::getenv("ROCKSDB_JS_PARK_TIMEOUT_MS"); - if (v == nullptr) { - return kDefault; - } - const char* firstNonSpace = v; - while (*firstNonSpace != '\0' && ::isspace(static_cast(*firstNonSpace))) { - ++firstNonSpace; - } - if (*firstNonSpace == '\0' || *firstNonSpace == '-') { - return kDefault; - } - char* end = nullptr; - errno = 0; - unsigned long parsed = ::strtoul(v, &end, 10); - // parsed == 0 covers both a literal "0" and `atoi`'s old silent - // non-numeric fallback; either way, firing every park immediately is - // exactly the unparked spin this bound exists to prevent, so treat it - // the same as malformed input rather than as a deliberate opt-out. - if (end == v || *end != '\0' || errno == ERANGE || parsed > UINT32_MAX || parsed == 0) { - return kDefault; - } - return static_cast(parsed); - }(); - return ms; + constexpr unsigned kDefault = 5000; + constexpr unsigned kMinimum = 50; + const char* v = ::getenv("ROCKSDB_JS_PARK_TIMEOUT_MS"); + if (v == nullptr) { + return kDefault; + } + const char* firstNonSpace = v; + while (*firstNonSpace != '\0' && ::isspace(static_cast(*firstNonSpace))) { + ++firstNonSpace; + } + if (*firstNonSpace == '\0' || *firstNonSpace == '-') { + return kDefault; + } + char* end = nullptr; + errno = 0; + unsigned long parsed = ::strtoul(v, &end, 10); + // parsed == 0 is the one ambiguous input: a literal "0" reads as "disable + // the bound", which is exactly the unbounded park this exists to prevent, + // so it falls back to the default like malformed input rather than being + // honored as an opt-out or clamped up to kMinimum. + if (end == v || *end != '\0' || errno == ERANGE || parsed > UINT32_MAX || parsed == 0) { + return kDefault; + } + return std::max(kMinimum, static_cast(parsed)); } /** @@ -509,7 +513,9 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { // when the descriptor is closing and when there is no descriptor // at all (DBHandle::close() can reset it concurrently) -- either // way there is no timeout thread behind this park. - uint64_t parkId = descriptor ? descriptor->scheduleParkTimeout(env, parkTimeoutMs(), tsfn, fired) : 0; + uint64_t parkId = descriptor + ? descriptor->parkTimeouts->schedule(env, parkTimeoutMs(), tsfn, fired) + : 0; if (parkId == 0) { // No timeout thread behind this park -- resolve now rather @@ -525,31 +531,18 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { } // weak_ptr, not raw: LockTracker::wakeCallbacks has no removal API, - // so this closure can outlive `descriptor` (e.g. a foreign-dbId + // so this closure can outlive the park registry (e.g. a foreign-dbId // tracker from a colliding VT slot). `.lock()` failing means - // shutdownParkTimeouts already resolved this park at close. - std::weak_ptr weakDescriptor = descriptor; - auto fireOnce = [tsfn, fired, weakDescriptor, parkId]() { - if (parkId != 0) { - if (auto d = weakDescriptor.lock()) { - // This lock() is itself a transient extra ref, exactly - // the shape that can make a racing close()'s - // PurgeIfUnreferenced observe use_count() > 1 and skip - // (HarperFast/rocksdb-js#672) -- retry it after - // dropping our ref, like BackupState/checkpoint state - // do for the same reason. - d->fireParkTimeout(parkId); - std::string path = d->path; - bool readOnly = d->readOnly; - d.reset(); - DBRegistry::PurgeIfUnreferenced(path, readOnly); - } - return; - } - bool expected = false; - if (fired->compare_exchange_strong(expected, true)) { - ::napi_call_threadsafe_function(tsfn, nullptr, napi_tsfn_nonblocking); - ::napi_release_threadsafe_function(tsfn, napi_tsfn_release); + // ParkTimeoutRegistry::shutdown already resolved this park at close. + // The weak reference is deliberately to the registry and not to the + // descriptor: this runs inline under the process-global VT + // `writerMutex_`, where re-entering DBRegistry can self-deadlock and + // where a transient descriptor ref would perturb the use_count that + // PurgeIfUnreferenced decides on (see the ParkTimeoutRegistry docs). + std::weak_ptr weakParks = descriptor->parkTimeouts; + auto fireOnce = [weakParks, parkId]() { + if (auto parks = weakParks.lock()) { + parks->fire(parkId); } }; diff --git a/test/lock-tracker.test.ts b/test/lock-tracker.test.ts index a1aff06f3..d63176c27 100644 --- a/test/lock-tracker.test.ts +++ b/test/lock-tracker.test.ts @@ -1,6 +1,7 @@ import { constants } from '../src/load-binding.ts'; import { RETRY_NOW, Transaction } from '../src/transaction.ts'; import { dbRunner } from './lib/util.ts'; +import { setTimeout as delay } from 'node:timers/promises'; import { describe, expect, it } from 'vitest'; const FRESH_VERSION_FLAG = constants.FRESH_VERSION_FLAG; @@ -202,14 +203,53 @@ describe('Coordinated retry — bounded park timeout (#741)', () => { await db.put(key, valueWithVersion(2.2e12)); txn.putSync(key, valueWithVersion(2.3e12)); - const start = Date.now(); + const start = performance.now(); const result = await txn.commit(); - const elapsed = Date.now() - start; + const elapsed = performance.now() - start; expect(result).toBe(RETRY_NOW); expect(elapsed).toBeGreaterThanOrEqual(4500); expect(elapsed).toBeLessThan(10000); })); + + // Covers the wake-during-close path (the holder's intents are released from + // inside finishClose(), so the park's LockTracker callback runs while the + // close is in progress) and the timeout thread being joined with a park + // live. It does NOT reach ParkTimeoutRegistry::shutdown()'s drain of still + // -pending parks: that only happens when the holder belongs to a different + // database on a colliding VT slot, which is hash-dependent and not + // arrangeable from here (verified by disabling the drain — this stays green). + it('a commit parked at db.close() settles instead of hanging', () => + dbRunner({ dbOptions: [{ encoding: false, verificationTable: true }] }, async ({ db }) => { + const key = Buffer.from('park-timeout-close-drain'); + const v0 = 1.6e12; + await db.put(key, valueWithVersion(v0)); + db.populateVersion(key, v0); + + const holder = new Transaction(db.store, { coordinatedRetry: true }); + holder.putSync(key, valueWithVersion(2.1e12)); + + const txn = new Transaction(db.store, { coordinatedRetry: true }); + await txn.get(key); + await db.put(key, valueWithVersion(2.2e12)); + txn.putSync(key, valueWithVersion(2.3e12)); + + const commit = txn.commit(); + // Long enough for the park to be registered with the timeout thread, + // short enough to stay far from its 5000ms deadline. + await delay(250); + + const start = performance.now(); + db.close(); + // Settled, not resolved: `commit()`'s `aftercommit` notify rejects + // once the database is closed, whatever the native result was. + const [settled] = await Promise.allSettled([commit]); + const elapsed = performance.now() - start; + + expect(settled.status).toBe('rejected'); + // Under the deadline, so this can only have come from the close. + expect(elapsed).toBeLessThan(4000); + })); }); // Regression coverage for the VT-fast-path / optimistic-snapshot interaction. From f6e7e4cb80f28989617f40f06da4ca724c4dcd63 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 24 Aug 2026 16:47:46 -0600 Subject: [PATCH 09/19] test: scope coordinated retry lifecycle regressions Run the bounded-park regression in an isolated child with the timeout set before the addon loads, covering the 50ms floor without adding five seconds to every suite. Keep the close-with-live-park coverage and scope the pre-existing Deno/macOS retry to commit-teardown instead of re-running the full Deno suite.\n\nRefs #741\n\nCo-Authored-By: GPT-5 Codex --- .github/workflows/pr.yml | 21 +--------- test/commit-teardown.test.ts | 12 ++++-- test/fixtures/fork-park-timeout.mts | 48 +++++++++++++++++++++++ test/lock-tracker.test.ts | 60 +++++++++++++++-------------- 4 files changed, 90 insertions(+), 51 deletions(-) create mode 100644 test/fixtures/fork-park-timeout.mts diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 9ec9da051..ea40df0e1 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -214,24 +214,5 @@ jobs: run: pnpm build - name: Run tests on Deno - # macos-latest gets an extra retry ONLY for the known pre-existing native - # crash in commit-teardown.test.ts (HarperFast/rocksdb-js#746, "mutex lock - # failed: Invalid argument" during worker-env teardown vs. the shared commit - # thread), which has hit multiple unrelated PRs on this OS/runtime combo. - # Gating on that crash string, rather than re-rolling any failure like the - # windows-latest retry below (HarperFast/rocksdb-js#695), keeps this OS's - # signal for every other regression. Not a fix for #746 itself. shell: bash - run: | - # pipefail is what makes the `| tee` below report the test exit code - # instead of tee's. - set -o pipefail - if pnpm test:deno 2>&1 | tee "$RUNNER_TEMP/deno-test.log"; then - exit 0 - fi - case "${{ matrix.os }}" in - windows-latest) ;; - macos-latest) grep -q 'mutex lock failed' "$RUNNER_TEMP/deno-test.log" || exit 1 ;; - *) exit 1 ;; - esac - pnpm test:deno + run: pnpm test:deno || ([ "${{ matrix.os }}" = "windows-latest" ] && pnpm test:deno) diff --git a/test/commit-teardown.test.ts b/test/commit-teardown.test.ts index eda9c2200..a1903fbd0 100644 --- a/test/commit-teardown.test.ts +++ b/test/commit-teardown.test.ts @@ -13,6 +13,8 @@ const COMMIT_THREAD_MODES: Array<{ label: string; mode: string | undefined }> = { label: '2', mode: '2' }, ]; +const retry = process.versions.deno && process.platform === 'darwin' ? 1 : 0; + /** * Runs the repro fixture in a child process so a native abort (SIGABRT/SIGSEGV) * from an async-commit completion racing worker-env teardown surfaces as a @@ -70,8 +72,12 @@ function spawnRepro( describe('Async commit completion vs. worker env teardown', () => { it.each(COMMIT_THREAD_MODES)( 'should survive worker env teardown with commits in flight on the shared commit thread (ROCKSDB_JS_COMMIT_THREAD=$label)', - ({ mode }) => expectSurvives(mode), - // Worker spawn/teardown dominates wall time and is slow on macOS/Windows. - 120_000 + { + // The single retry is limited to the pre-existing Deno/macOS crash in #746. + retry, + // Worker spawn/teardown dominates wall time and is slow on macOS/Windows. + timeout: 120_000, + }, + ({ mode }) => expectSurvives(mode) ); }); diff --git a/test/fixtures/fork-park-timeout.mts b/test/fixtures/fork-park-timeout.mts new file mode 100644 index 000000000..8545f03f0 --- /dev/null +++ b/test/fixtures/fork-park-timeout.mts @@ -0,0 +1,48 @@ +import { RocksDatabase, Transaction } from '../../src/index.ts'; +import { RETRY_NOW } from '../../src/transaction.ts'; + +const dbPath = process.argv[2]; + +if (!dbPath) { + console.error('Usage: fork-park-timeout.mts '); + process.exit(1); +} + +function valueWithVersion(version: number): Buffer { + const value = Buffer.alloc(16); + value.writeDoubleBE(version, 0); + return value; +} + +const db = new RocksDatabase(dbPath, { encoding: false, verificationTable: true }); +const keepAlive = setInterval(() => {}, 1000); + +try { + db.open(); + const key = Buffer.from('park-timeout-abandoned-holder'); + const initialVersion = 1.6e12; + await db.put(key, valueWithVersion(initialVersion)); + db.populateVersion(key, initialVersion); + if (!db.verifyVersion(key, initialVersion)) { + throw new Error('failed to populate the verification-table version'); + } + + const holder = new Transaction(db.store, { coordinatedRetry: true }); + holder.putSync(key, valueWithVersion(2.1e12)); + + const transaction = new Transaction(db.store, { coordinatedRetry: true }); + await transaction.get(key); + await db.put(key, valueWithVersion(2.2e12)); + transaction.putSync(key, valueWithVersion(2.3e12)); + + const start = performance.now(); + const result = await transaction.commit(); + const elapsed = performance.now() - start; + + if (result !== RETRY_NOW || elapsed < 40 || elapsed >= 4000) { + throw new Error(`unexpected park result=${String(result)} elapsed=${elapsed}`); + } +} finally { + clearInterval(keepAlive); + db.close(); +} diff --git a/test/lock-tracker.test.ts b/test/lock-tracker.test.ts index d63176c27..aa5ebf6a2 100644 --- a/test/lock-tracker.test.ts +++ b/test/lock-tracker.test.ts @@ -1,10 +1,13 @@ import { constants } from '../src/load-binding.ts'; import { RETRY_NOW, Transaction } from '../src/transaction.ts'; import { dbRunner } from './lib/util.ts'; +import { spawn } from 'node:child_process'; +import { join } from 'node:path'; import { setTimeout as delay } from 'node:timers/promises'; import { describe, expect, it } from 'vitest'; const FRESH_VERSION_FLAG = constants.FRESH_VERSION_FLAG; +const parkTimeoutFixturePath = join(__dirname, 'fixtures', 'fork-park-timeout.mts'); // Builds a value buffer whose first 8 bytes are the big-endian float64 version, // matching VerificationTable::extractVersionFromValue (Harper's record format). @@ -179,37 +182,38 @@ describe('Coordinated retry (Phase 3)', () => { // Regression coverage for #741: a park behind a leaked/abandoned holder must // resolve RETRY_NOW after a bounded wait instead of hanging forever -// (harper#2001). Default ROCKSDB_JS_PARK_TIMEOUT_MS is 5000ms; the lower -// bound below is what tells this apart from the `!parked` fast path (which -// also resolves RETRY_NOW, just near-instantly). +// (harper#2001). The lower bound below is what tells this apart from the +// `!parked` fast path, which also resolves RETRY_NOW near-instantly. describe('Coordinated retry — bounded park timeout (#741)', () => { it('a commit parked behind a never-releasing holder settles with RETRY_NOW within the deadline', () => - dbRunner({ dbOptions: [{ encoding: false, verificationTable: true }] }, async ({ db }) => { - const key = Buffer.from('park-timeout-abandoned-holder'); - const v0 = 1.6e12; - await db.put(key, valueWithVersion(v0)); - db.populateVersion(key, v0); - expect(db.verifyVersion(key, v0)).toBe(true); - - // Abandoned holder: staged write, never committed or aborted. - const holder = new Transaction(db.store, { coordinatedRetry: true }); - holder.putSync(key, valueWithVersion(2.1e12)); - - // Establish txn's snapshot before the conflicting external commit - // below, so RocksDB's optimistic conflict check has something to - // validate against. - const txn = new Transaction(db.store, { coordinatedRetry: true }); - await txn.get(key); - await db.put(key, valueWithVersion(2.2e12)); - txn.putSync(key, valueWithVersion(2.3e12)); - - const start = performance.now(); - const result = await txn.commit(); - const elapsed = performance.now() - start; + dbRunner({ skipOpen: true }, async ({ dbPath }) => { + const { code, signal } = await new Promise<{ + code: number | null; + signal: NodeJS.Signals | null; + }>((resolve, reject) => { + const child = spawn(process.execPath, [parkTimeoutFixturePath, dbPath], { + env: { ...process.env, ROCKSDB_JS_PARK_TIMEOUT_MS: '1' }, + }); + const timer = setTimeout(() => child.kill(), 4000); + let stderr = ''; + child.stderr?.on('data', (chunk) => { + stderr += chunk.toString(); + }); + child.on('close', (childCode, childSignal) => { + clearTimeout(timer); + if (childCode !== 0 || childSignal) { + console.error(`Park timeout child stderr:\n${stderr}`); + } + resolve({ code: childCode, signal: childSignal }); + }); + child.on('error', (error) => { + clearTimeout(timer); + reject(error); + }); + }); - expect(result).toBe(RETRY_NOW); - expect(elapsed).toBeGreaterThanOrEqual(4500); - expect(elapsed).toBeLessThan(10000); + expect(signal).toBeNull(); + expect(code).toBe(0); })); // Covers the wake-during-close path (the holder's intents are released from From 94906e7228a32a92eb2fddd27b2ab46c6650cfaa Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 24 Aug 2026 17:13:08 -0600 Subject: [PATCH 10/19] fix(transaction): harden retry park setup Co-Authored-By: GPT-5 Codex --- src/binding/database/db_descriptor.cpp | 2 +- src/binding/transaction/transaction.cpp | 100 +++++++++++++++++------- 2 files changed, 71 insertions(+), 31 deletions(-) diff --git a/src/binding/database/db_descriptor.cpp b/src/binding/database/db_descriptor.cpp index 8cd211545..f06fa92ce 100644 --- a/src/binding/database/db_descriptor.cpp +++ b/src/binding/database/db_descriptor.cpp @@ -590,7 +590,7 @@ uint64_t ParkTimeoutRegistry::schedule( if (!this->threadStarted) { try { this->thread = std::thread([this]() { this->runLoop(); }); - } catch (const std::system_error&) { + } catch (...) { // Thread creation failed (e.g. thread/resource exhaustion): leave // the flag false so the next park retries, and tell the caller to // resolve inline now rather than register a park nothing will diff --git a/src/binding/transaction/transaction.cpp b/src/binding/transaction/transaction.cpp index 2fcb05cfd..aa2fb2636 100644 --- a/src/binding/transaction/transaction.cpp +++ b/src/binding/transaction/transaction.cpp @@ -276,15 +276,48 @@ struct TransactionCommitState final : BaseAsyncState*> savedSlots; + std::weak_ptr parkTimeouts; TransactionCommitState( napi_env env, std::shared_ptr handle ) : BaseAsyncState>(env, handle), - hasLog(false) {} + hasLog(false), + parkTimeouts( + handle && handle->coordinatedRetry && handle->dbHandle && handle->dbHandle->descriptor + ? handle->dbHandle->descriptor->parkTimeouts + : std::shared_ptr() + ) {} }; +static void rejectRetryNowSetupFailure( + napi_env env, + TransactionCommitState* state, + napi_status status +) { + napi_value error = nullptr; + bool exceptionPending = false; + if (::napi_is_exception_pending(env, &exceptionPending) == napi_ok && exceptionPending) { + ::napi_get_and_clear_last_exception(env, &error); + } + if (error == nullptr) { + std::string detail = "Failed to initialize coordinated retry: " + + getNapiExtendedError(env, status); + napi_value message; + if (::napi_create_string_utf8(env, detail.c_str(), detail.size(), &message) == napi_ok) { + ::napi_create_error(env, nullptr, message, &error); + } + } + if (error == nullptr) { + ::napi_get_and_clear_last_exception(env, &error); + } + if (error == nullptr) { + ::napi_get_undefined(env, &error); + } + state->callReject(error); +} + /** * Log-lane stage of the commit: validates the handle and writes the * transaction-log batch (recording the committed position). Runs off the JS @@ -465,16 +498,9 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { state->handle->close(); } - // Transfer resolve/reject refs from state to a RetryNowContext - // so the TSFN finalize can clean them up. - auto* ctx = new RetryNowContext{state->resolveRef, state->rejectRef}; - state->resolveRef = nullptr; - state->rejectRef = nullptr; - bool parked = false; VerificationTable* vt = DBSettings::getInstance().getVerificationTableRaw(); - std::shared_ptr descriptor = - state->handle->dbHandle ? state->handle->dbHandle->descriptor : nullptr; + std::shared_ptr parkTimeouts = state->parkTimeouts.lock(); for (auto* slot : state->savedSlots) { // refTrackerIfLocked takes a temporary reference under the VT // writer mutex, so the tracker cannot be freed by a concurrent @@ -485,7 +511,18 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { // Create a TSFN that calls resolve(RETRY_NOW) when fired. napi_value resource_name; - ::napi_create_string_latin1(env, "transaction.retry", NAPI_AUTO_LENGTH, &resource_name); + napi_status resourceStatus = ::napi_create_string_latin1( + env, "transaction.retry", NAPI_AUTO_LENGTH, &resource_name + ); + if (resourceStatus != napi_ok) { + vt->unrefTracker(t); + rejectRetryNowSetupFailure(env, state, resourceStatus); + return; + } + + auto* ctx = new RetryNowContext{state->resolveRef, state->rejectRef}; + state->resolveRef = nullptr; + state->rejectRef = nullptr; napi_threadsafe_function tsfn; napi_status tsfnStatus = ::napi_create_threadsafe_function( env, nullptr, nullptr, resource_name, @@ -495,12 +532,14 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { &tsfn ); if (tsfnStatus != napi_ok) { - // Creation failed (e.g. an already-pending exception): nothing to - // call+release, and ctx is not yet owned by any finalize -- fall - // through to the plain !parked resolve below instead of leaving - // a garbage tsfn handle in a park entry. + state->resolveRef = ctx->resolveRef; + state->rejectRef = ctx->rejectRef; + ctx->resolveRef = nullptr; + ctx->rejectRef = nullptr; + delete ctx; vt->unrefTracker(t); - break; + rejectRetryNowSetupFailure(env, state, tsfnStatus); + return; } ::napi_unref_threadsafe_function(env, tsfn); @@ -513,8 +552,8 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { // when the descriptor is closing and when there is no descriptor // at all (DBHandle::close() can reset it concurrently) -- either // way there is no timeout thread behind this park. - uint64_t parkId = descriptor - ? descriptor->parkTimeouts->schedule(env, parkTimeoutMs(), tsfn, fired) + uint64_t parkId = parkTimeouts + ? parkTimeouts->schedule(env, parkTimeoutMs(), tsfn, fired) : 0; if (parkId == 0) { @@ -539,8 +578,13 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { // `writerMutex_`, where re-entering DBRegistry can self-deadlock and // where a transient descriptor ref would perturb the use_count that // PurgeIfUnreferenced decides on (see the ParkTimeoutRegistry docs). - std::weak_ptr weakParks = descriptor->parkTimeouts; - auto fireOnce = [weakParks, parkId]() { + std::weak_ptr weakParks = parkTimeouts; + std::weak_ptr> weakFired = fired; + auto fireOnce = [weakParks, weakFired, parkId]() { + auto fired = weakFired.lock(); + if (!fired || fired->load(std::memory_order_acquire)) { + return; + } if (auto parks = weakParks.lock()) { parks->fire(parkId); } @@ -560,18 +604,14 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { } if (!parked) { - // No active lock found; resolve RETRY_NOW directly (we are on - // the JS thread in this complete callback). - napi_value global, resolveFn, retryVal; - ::napi_get_global(env, &global); - ::napi_get_reference_value(env, ctx->resolveRef, &resolveFn); - ::napi_create_int32(env, RETRY_NOW_VALUE, &retryVal); - ::napi_call_function(env, global, resolveFn, 1, &retryVal, nullptr); - ::napi_delete_reference(env, ctx->resolveRef); - ::napi_delete_reference(env, ctx->rejectRef); - delete ctx; + napi_value retryVal; + napi_status retryStatus = ::napi_create_int32(env, RETRY_NOW_VALUE, &retryVal); + if (retryStatus == napi_ok) { + state->callResolve(retryVal); + } else { + rejectRetryNowSetupFailure(env, state, retryStatus); + } } - // If parked, ctx is owned by the TSFN finalize; do not free here. } else { // Normal error path: reset to Pending so JS can retry. // Guard: keep Aborted if close() already set it (DB closing From 146574b54335aa97e925592679b027f8bc199996 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 24 Aug 2026 17:40:10 -0600 Subject: [PATCH 11/19] fix(database): avoid pinning descriptors in park cleanup Co-Authored-By: GPT-5 Codex --- src/binding/database/db_registry.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/binding/database/db_registry.cpp b/src/binding/database/db_registry.cpp index d1c669815..bafed0a8f 100644 --- a/src/binding/database/db_registry.cpp +++ b/src/binding/database/db_registry.cpp @@ -668,19 +668,19 @@ void DBRegistry::ReleaseParkTimeoutsByEnv(napi_env env) { return; } - std::vector> descriptors; + std::vector> registries; { std::lock_guard lock(instance->databasesMutex); - descriptors.reserve(instance->databases.size()); + registries.reserve(instance->databases.size()); for (auto& [_key, entry] : instance->databases) { if (entry.descriptor) { - descriptors.push_back(entry.descriptor); + registries.push_back(entry.descriptor->parkTimeouts); } } } - for (auto& descriptor : descriptors) { - descriptor->parkTimeouts->releaseByEnv(env); + for (auto& registry : registries) { + registry->releaseByEnv(env); } } From 5b2a06a53fe415e318d12d6387d13c2d3604d286 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 24 Aug 2026 18:51:57 -0600 Subject: [PATCH 12/19] fix(transaction): read the park timeout once per process ::getenv is not safe against a concurrent ::setenv, and a park runs on whichever env's JS thread owns the transaction while a `process.env` write on the main thread goes through uv_os_setenv -> setenv(3), which may reallocate `environ`. The per-park read introduced in 2a5ae5be put that race on every coordinated-retry commit. It bought nothing in exchange: worker-thread `process.env` writes never reach ::getenv (core/test_seam.h), so the only way to vary the value is a child process started with it already set -- which is what the #741 regression fixture does. Restores the function-local static used by commitThreadMode()/commitDelayMs() in the same file, keeping the 50 ms clamp and the zero-is-malformed rule inside the initializer. Co-Authored-By: Claude Opus --- AGENTS.md | 12 +++-- src/binding/transaction/transaction.cpp | 66 +++++++++++++++---------- 2 files changed, 47 insertions(+), 31 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f2cb96a62..9b7a1740d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -196,10 +196,14 @@ sufficient (env teardown does not honor tsfn acquire counts); see - `ROCKSDB_JS_PARK_TIMEOUT_MS` - Bounded wait (default `5000`) before a coordinated-retry commit parked on a conflicting holder's VT lock resolves RETRY_NOW unconditionally, in case the holder never releases (see - "Coordinated retry" note below). Read per park; values below `50` are clamped - up to it, and `0` (an ambiguous "disable the bound") falls back to the default - like any malformed value. There is no opt-out: a deployment that would rather - wait than fail a legitimately slow holder raises the value instead + "Coordinated retry" note below). Read once per process (a function-local + `static`, like the other two above — `::getenv` is not safe against a + concurrent `::setenv` from a `process.env` write, and a park runs on whichever + env's JS thread owns the transaction), so it must be set in the environment a + process is started with. Values below `50` are clamped up to it, and `0` (an + ambiguous "disable the bound") falls back to the default like any malformed + value. There is no opt-out: a deployment that would rather wait than fail a + legitimately slow holder raises the value instead ## Test Structure diff --git a/src/binding/transaction/transaction.cpp b/src/binding/transaction/transaction.cpp index aa2fb2636..fb5784379 100644 --- a/src/binding/transaction/transaction.cpp +++ b/src/binding/transaction/transaction.cpp @@ -235,38 +235,50 @@ static void retryNowFinalize(napi_env env, void* finalizeData, void* /*hint*/) { * consumes a coordinatedRetry attempt like a genuine wake would, so a * higher default leaves more headroom for a merely-slow (not abandoned) * holder before maxRetries exhausts -- a deployment that would rather wait - * than fail raises this, it cannot disable the bound. Read per park (this is - * the conflict path, not a hot path) so a process can be started with a small - * value for tests without depending on which park happens to run first. + * than fail raises this, it cannot disable the bound. + * + * Read once per process behind a function-local static, like commitThreadMode() + * and commitDelayMs() below: ::getenv is not safe against a concurrent + * ::setenv, and a park runs on whichever env's JS thread owns the transaction + * while `process.env` writes on the main thread go through uv_os_setenv -> + * setenv(3), which may reallocate `environ`. Per-park reads bought no test + * flexibility to pay for that -- worker-thread `process.env` writes never reach + * ::getenv at all (see core/test_seam.h), so the only way to vary this is a + * child process started with the value already set, which the #741 regression + * fixture does. + * * Malformed input falls back to the default rather than producing a * degenerate effective timeout; a positive value below kMinimum is clamped up * to it instead, since the intent there is unambiguous. */ static unsigned parkTimeoutMs() { - constexpr unsigned kDefault = 5000; - constexpr unsigned kMinimum = 50; - const char* v = ::getenv("ROCKSDB_JS_PARK_TIMEOUT_MS"); - if (v == nullptr) { - return kDefault; - } - const char* firstNonSpace = v; - while (*firstNonSpace != '\0' && ::isspace(static_cast(*firstNonSpace))) { - ++firstNonSpace; - } - if (*firstNonSpace == '\0' || *firstNonSpace == '-') { - return kDefault; - } - char* end = nullptr; - errno = 0; - unsigned long parsed = ::strtoul(v, &end, 10); - // parsed == 0 is the one ambiguous input: a literal "0" reads as "disable - // the bound", which is exactly the unbounded park this exists to prevent, - // so it falls back to the default like malformed input rather than being - // honored as an opt-out or clamped up to kMinimum. - if (end == v || *end != '\0' || errno == ERANGE || parsed > UINT32_MAX || parsed == 0) { - return kDefault; - } - return std::max(kMinimum, static_cast(parsed)); + static const unsigned ms = []() -> unsigned { + constexpr unsigned kDefault = 5000; + constexpr unsigned kMinimum = 50; + const char* v = ::getenv("ROCKSDB_JS_PARK_TIMEOUT_MS"); + if (v == nullptr) { + return kDefault; + } + const char* firstNonSpace = v; + while (*firstNonSpace != '\0' && ::isspace(static_cast(*firstNonSpace))) { + ++firstNonSpace; + } + if (*firstNonSpace == '\0' || *firstNonSpace == '-') { + return kDefault; + } + char* end = nullptr; + errno = 0; + unsigned long parsed = ::strtoul(v, &end, 10); + // parsed == 0 is the one ambiguous input: a literal "0" reads as "disable + // the bound", which is exactly the unbounded park this exists to prevent, + // so it falls back to the default like malformed input rather than being + // honored as an opt-out or clamped up to kMinimum. + if (end == v || *end != '\0' || errno == ERANGE || parsed > UINT32_MAX || parsed == 0) { + return kDefault; + } + return std::max(kMinimum, static_cast(parsed)); + }(); + return ms; } /** From 97df02031a616c4d840f30b180bf2f0022a4ee74 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 24 Aug 2026 18:51:57 -0600 Subject: [PATCH 13/19] test: correct the park-close test's coverage claim The comment claimed the close-with-a-live-park test covered the timeout thread being joined. It does not: ParkTimeoutRegistry::shutdown() resolves every pending park under its mutex before joining, so the promise settles on time whether the thread is joined or detached -- confirmed locally by mutating join() to detach(), rebuilding and re-running the file (12/12 green). A detached runLoop() costs a freed-registry touch, which is a teardown/ASan-shaped failure a deadline assertion cannot see. The test also passes at the merge base, so it is close-path smoke coverage rather than a regression test for this change. Co-Authored-By: Claude Opus --- test/lock-tracker.test.ts | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/test/lock-tracker.test.ts b/test/lock-tracker.test.ts index aa5ebf6a2..590eb0f2a 100644 --- a/test/lock-tracker.test.ts +++ b/test/lock-tracker.test.ts @@ -216,13 +216,23 @@ describe('Coordinated retry — bounded park timeout (#741)', () => { expect(code).toBe(0); })); - // Covers the wake-during-close path (the holder's intents are released from - // inside finishClose(), so the park's LockTracker callback runs while the - // close is in progress) and the timeout thread being joined with a park - // live. It does NOT reach ParkTimeoutRegistry::shutdown()'s drain of still - // -pending parks: that only happens when the holder belongs to a different - // database on a colliding VT slot, which is hash-dependent and not - // arrangeable from here (verified by disabling the drain — this stays green). + // Drives a close with a park live, so a hang anywhere on the wake-during- + // close path (the holder's intents are released from inside finishClose(), + // so the park's LockTracker callback runs while the close is in progress) + // shows up as a timing failure here. Be precise about what that does NOT + // cover, since the assertion is a deadline: + // - Not the shutdown() join. shutdown() resolves every pending park under + // its mutex BEFORE joining, so the promise settles on time whether the + // thread is joined or detached (mutation-tested: join -> detach stays + // green). What a detached runLoop() costs is touching a freed registry — + // a teardown/ASan-shaped failure, invisible to a stopwatch. + // - Not ParkTimeoutRegistry::shutdown()'s drain of still-pending parks: + // that needs the holder to belong to a different database on a colliding + // VT slot, which is hash-dependent and not arrangeable from here + // (verified by disabling the drain — this stays green). + // It also passes at the merge base, so it is close-path smoke coverage + // rather than a regression test for this change; the fork-based test above + // is the one that goes red without the park timeout. it('a commit parked at db.close() settles instead of hanging', () => dbRunner({ dbOptions: [{ encoding: false, verificationTable: true }] }, async ({ db }) => { const key = Buffer.from('park-timeout-close-drain'); From 2de60273e5e84dc329bd1435f522855e4225b2ba Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 24 Aug 2026 19:00:29 -0600 Subject: [PATCH 14/19] docs: trim the park-timeout comments to their invariants Independent review flagged both new blocks as narration: the parkTimeoutMs docblock restated rationale AGENTS.md already carries, and the close test's comment read as a mutation-testing changelog aimed at a reviewer rather than the next reader. Keeps what the code cannot say -- why the env read is once-per-process, and which two close-path behaviours a deadline assertion cannot discriminate. Co-Authored-By: Claude Opus --- src/binding/transaction/transaction.cpp | 15 ++++++-------- test/lock-tracker.test.ts | 26 +++++++++---------------- 2 files changed, 15 insertions(+), 26 deletions(-) diff --git a/src/binding/transaction/transaction.cpp b/src/binding/transaction/transaction.cpp index fb5784379..24c24d60f 100644 --- a/src/binding/transaction/transaction.cpp +++ b/src/binding/transaction/transaction.cpp @@ -237,15 +237,12 @@ static void retryNowFinalize(napi_env env, void* finalizeData, void* /*hint*/) { * holder before maxRetries exhausts -- a deployment that would rather wait * than fail raises this, it cannot disable the bound. * - * Read once per process behind a function-local static, like commitThreadMode() - * and commitDelayMs() below: ::getenv is not safe against a concurrent - * ::setenv, and a park runs on whichever env's JS thread owns the transaction - * while `process.env` writes on the main thread go through uv_os_setenv -> - * setenv(3), which may reallocate `environ`. Per-park reads bought no test - * flexibility to pay for that -- worker-thread `process.env` writes never reach - * ::getenv at all (see core/test_seam.h), so the only way to vary this is a - * child process started with the value already set, which the #741 regression - * fixture does. + * Read once per process, like commitThreadMode()/commitDelayMs() below: 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 (uv_os_setenv -> + * setenv(3) may reallocate `environ`). Varying it therefore means starting a + * process with the value already set -- worker-thread `process.env` writes + * never reach ::getenv at all (core/test_seam.h). * * Malformed input falls back to the default rather than producing a * degenerate effective timeout; a positive value below kMinimum is clamped up diff --git a/test/lock-tracker.test.ts b/test/lock-tracker.test.ts index 590eb0f2a..5de8949ae 100644 --- a/test/lock-tracker.test.ts +++ b/test/lock-tracker.test.ts @@ -216,23 +216,15 @@ describe('Coordinated retry — bounded park timeout (#741)', () => { expect(code).toBe(0); })); - // Drives a close with a park live, so a hang anywhere on the wake-during- - // close path (the holder's intents are released from inside finishClose(), - // so the park's LockTracker callback runs while the close is in progress) - // shows up as a timing failure here. Be precise about what that does NOT - // cover, since the assertion is a deadline: - // - Not the shutdown() join. shutdown() resolves every pending park under - // its mutex BEFORE joining, so the promise settles on time whether the - // thread is joined or detached (mutation-tested: join -> detach stays - // green). What a detached runLoop() costs is touching a freed registry — - // a teardown/ASan-shaped failure, invisible to a stopwatch. - // - Not ParkTimeoutRegistry::shutdown()'s drain of still-pending parks: - // that needs the holder to belong to a different database on a colliding - // VT slot, which is hash-dependent and not arrangeable from here - // (verified by disabling the drain — this stays green). - // It also passes at the merge base, so it is close-path smoke coverage - // rather than a regression test for this change; the fork-based test above - // is the one that goes red without the park timeout. + // Catches a hang on the wake-during-close path: the holder's intents are + // released from inside finishClose(), so the park's LockTracker callback + // runs while the close is in progress. The assertion is a deadline, so it + // cannot see either of the other two close-path behaviours — shutdown() + // resolves pending parks under its mutex before joining the timeout thread, + // so a detached thread settles this just as fast (its cost is a freed- + // registry touch, an ASan-shaped failure), and the drain of still-pending + // parks needs the holder on another database's colliding VT slot, which is + // hash-dependent and not arrangeable from here. it('a commit parked at db.close() settles instead of hanging', () => dbRunner({ dbOptions: [{ encoding: false, verificationTable: true }] }, async ({ db }) => { const key = Buffer.from('park-timeout-close-drain'); From 59f3d4b0f2ba2bd1bf5796ee011428921f4ba4a1 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 24 Aug 2026 20:24:54 -0600 Subject: [PATCH 15/19] test: use a monotonic clock for the conflict-wake elapsed bound The reply resolving the review thread on this file said the elapsed-time assertions had moved to performance.now(), but the wall-clock bound added to the concurrent-conflict test still read Date.now() on both ends. An NTP step between the two reads is enough to push `elapsed` past the 3000ms bound (or negative) with nothing wrong in the wake path. Co-Authored-By: Claude Opus 5 --- test/lock-tracker.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/lock-tracker.test.ts b/test/lock-tracker.test.ts index 5de8949ae..c2df55ffa 100644 --- a/test/lock-tracker.test.ts +++ b/test/lock-tracker.test.ts @@ -123,7 +123,7 @@ describe('Coordinated retry (Phase 3)', () => { // Force IsBusy by running concurrent transactions writing the same key // under coordinatedRetry: true. database.ts handles RETRY_NOW internally // via immediate retry; callers never see it as a return value. - const start = Date.now(); + const start = performance.now(); const results = await Promise.allSettled( Array.from({ length: 4 }, async (_, i) => { const v = Buffer.alloc(16); @@ -136,7 +136,7 @@ describe('Coordinated retry (Phase 3)', () => { ); }) ); - const elapsed = Date.now() - start; + const elapsed = performance.now() - start; // All transactions should eventually succeed (coordinatedRetry retries // without error) or fail gracefully; none should throw unexpectedly. From dc1bacfd42c7f2847cd81ac1a9bf77c1878a6216 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 00:03:00 -0600 Subject: [PATCH 16/19] fix(transaction): don't release a closing park timeout tsfn twice ParkTimeoutRegistry::resolve() and the no-timeout-thread degrade path in transaction.cpp called napi_release_threadsafe_function unconditionally after napi_call_threadsafe_function, discarding the status. If the call returns napi_closing (env teardown racing this resolve), the tsfn may already be mid-teardown on Node's side, and releasing it again is a use-after-close. Guard the release on status == napi_ok, matching the pattern already used elsewhere in this file (dispatchCommitCompletion/releaseCommitCompletionsByEnv). Found by the pre-push cross-model review during the #744 rebase. Co-Authored-By: Claude Sonnet --- src/binding/database/db_descriptor.cpp | 8 ++++++-- src/binding/transaction/transaction.cpp | 9 +++++++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/binding/database/db_descriptor.cpp b/src/binding/database/db_descriptor.cpp index f06fa92ce..b5ed6920e 100644 --- a/src/binding/database/db_descriptor.cpp +++ b/src/binding/database/db_descriptor.cpp @@ -634,8 +634,12 @@ std::unique_ptr ParkTimeoutRegistry::take(uint void ParkTimeoutRegistry::resolve(ParkTimeout& park) { bool expected = false; if (park.fired->compare_exchange_strong(expected, true)) { - ::napi_call_threadsafe_function(park.tsfn, nullptr, napi_tsfn_nonblocking); - ::napi_release_threadsafe_function(park.tsfn, napi_tsfn_release); + // A closing tsfn (env teardown racing this resolve) must not be + // touched again -- napi_closing means Node may already be freeing it. + napi_status status = ::napi_call_threadsafe_function(park.tsfn, nullptr, napi_tsfn_nonblocking); + if (status == napi_ok) { + ::napi_release_threadsafe_function(park.tsfn, napi_tsfn_release); + } } } diff --git a/src/binding/transaction/transaction.cpp b/src/binding/transaction/transaction.cpp index 24c24d60f..6dbcd78ec 100644 --- a/src/binding/transaction/transaction.cpp +++ b/src/binding/transaction/transaction.cpp @@ -571,8 +571,13 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { vt->unrefTracker(t); bool expected = false; if (fired->compare_exchange_strong(expected, true)) { - ::napi_call_threadsafe_function(tsfn, nullptr, napi_tsfn_nonblocking); - ::napi_release_threadsafe_function(tsfn, napi_tsfn_release); + // A closing tsfn (env teardown racing this resolve) must + // not be touched again -- see the matching guard in + // ParkTimeoutRegistry::resolve. + napi_status callStatus = ::napi_call_threadsafe_function(tsfn, nullptr, napi_tsfn_nonblocking); + if (callStatus == napi_ok) { + ::napi_release_threadsafe_function(tsfn, napi_tsfn_release); + } } parked = true; break; From dd8a4b7eb6e9e3c119ea59037054b666694b6ea2 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 00:46:12 -0600 Subject: [PATCH 17/19] fix(transaction): drop the unreachable park-resolve gate and bound the docs Addresses the three open review threads on #744. The `parkId == 0` branch owned `fired` outright: every path that returns 0 from `ParkTimeoutRegistry::schedule` does so before the entry is published, and the LockTracker wake callback is not built until after this branch breaks out, so the compare-exchange could never fail and its comment described a race that cannot occur. The fixture's own `elapsed` assertion is the deadline check; the parent's kill timer only needs to catch a true hang, and at 4000 ms it also had to cover node boot, type-stripping, addon load and the seed writes -- so a slow runner and a regression both surfaced as SIGTERM with the fixture's diagnostic never thrown. Raised to 20 s, under vitest's 30 s testTimeout so the kill path still dumps the child's stderr. README documented the coordinated-retry wait as unbounded, which the park timeout made wrong: the wait now ends after ROCKSDB_JS_PARK_TIMEOUT_MS and consumes a retry attempt, so a holder blocked longer than roughly maxRetries x that timeout abandons rather than waiting. Co-Authored-By: Claude Opus --- README.md | 7 +++++++ src/binding/transaction/transaction.cpp | 19 +++++++++---------- test/lock-tracker.test.ts | 9 ++++++++- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index c3a53ce6c..ec239aeb6 100644 --- a/README.md +++ b/README.md @@ -779,6 +779,13 @@ not committed after the configured number of coordinated retries, the transactio an `ERR_TRANSACTION_ABANDONED` error. Coordinated retry requires the column family to be opened with `verificationTable: true`. +That wait is bounded so a conflicting transaction that is never committed or aborted cannot block +the commit forever: if the write intent has not been released after `ROCKSDB_JS_PARK_TIMEOUT_MS` +(default `5000`), the commit resolves anyway and consumes a retry attempt exactly as a real release +would. A conflicting transaction held for longer than roughly `maxRetries` times that timeout +therefore ends in `ERR_TRANSACTION_ABANDONED` rather than waiting indefinitely. Deployments where +waiting is preferable to failing should raise the timeout; it has no opt-out. + ### Class: `Transaction` The transaction callback is passed in a `Transaction` instance which contains all of the same data diff --git a/src/binding/transaction/transaction.cpp b/src/binding/transaction/transaction.cpp index 6dbcd78ec..9e2d731cd 100644 --- a/src/binding/transaction/transaction.cpp +++ b/src/binding/transaction/transaction.cpp @@ -567,17 +567,16 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { if (parkId == 0) { // No timeout thread behind this park -- resolve now rather - // than register with the LockTracker unbounded. + // than register with the LockTracker unbounded. Every path that + // returns 0 does so before `schedule` publishes `fired`, and the + // wake callback below is not built yet, so this side owns the + // park outright and needs no exactly-once gate. vt->unrefTracker(t); - bool expected = false; - if (fired->compare_exchange_strong(expected, true)) { - // A closing tsfn (env teardown racing this resolve) must - // not be touched again -- see the matching guard in - // ParkTimeoutRegistry::resolve. - napi_status callStatus = ::napi_call_threadsafe_function(tsfn, nullptr, napi_tsfn_nonblocking); - if (callStatus == napi_ok) { - ::napi_release_threadsafe_function(tsfn, napi_tsfn_release); - } + // A closing tsfn (env teardown racing this resolve) must not be + // touched again -- see the matching guard in + // ParkTimeoutRegistry::resolve. + if (::napi_call_threadsafe_function(tsfn, nullptr, napi_tsfn_nonblocking) == napi_ok) { + ::napi_release_threadsafe_function(tsfn, napi_tsfn_release); } parked = true; break; diff --git a/test/lock-tracker.test.ts b/test/lock-tracker.test.ts index c2df55ffa..6b3689176 100644 --- a/test/lock-tracker.test.ts +++ b/test/lock-tracker.test.ts @@ -194,7 +194,14 @@ describe('Coordinated retry — bounded park timeout (#741)', () => { const child = spawn(process.execPath, [parkTimeoutFixturePath, dbPath], { env: { ...process.env, ROCKSDB_JS_PARK_TIMEOUT_MS: '1' }, }); - const timer = setTimeout(() => child.kill(), 4000); + // Backstop against a true hang only — the child's own `elapsed` + // assertion is the deadline check. This timer additionally covers + // node boot, type-stripping, addon load and the seed writes, so a + // budget near the child's would make a slow runner (SIGTERM, the + // fixture's diagnostic never thrown) indistinguishable from a + // regression. Kept under vitest's 30s testTimeout so the kill path + // still dumps the child's stderr. + const timer = setTimeout(() => child.kill(), 20_000); let stderr = ''; child.stderr?.on('data', (chunk) => { stderr += chunk.toString(); From 46b1cce8db47ec9b545cbcecf1e7232902e481e7 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 00:51:45 -0600 Subject: [PATCH 18/19] docs: state that ROCKSDB_JS_PARK_TIMEOUT_MS=0 does not disable the bound Documenting the knob as operator-facing without its parsing rules left `0` -- the value an operator reaches for first to restore the old blocking wait -- silently running at the 5000ms default. Co-Authored-By: Claude Opus --- README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index ec239aeb6..d82b5cd58 100644 --- a/README.md +++ b/README.md @@ -784,7 +784,9 @@ the commit forever: if the write intent has not been released after `ROCKSDB_JS_ (default `5000`), the commit resolves anyway and consumes a retry attempt exactly as a real release would. A conflicting transaction held for longer than roughly `maxRetries` times that timeout therefore ends in `ERR_TRANSACTION_ABANDONED` rather than waiting indefinitely. Deployments where -waiting is preferable to failing should raise the timeout; it has no opt-out. +waiting is preferable to failing should raise the timeout; there is no way to disable the bound. +In particular `0` does not disable it: `0`, negative, and unparseable values all fall back to the +`5000` default, and a value between `1` and `49` is clamped up to `50`. ### Class: `Transaction` From d2587a273272a49d4ff29870f7f2bc65c7ccd285 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 00:56:24 -0600 Subject: [PATCH 19/19] docs: trim the comments added for the park-resolve review fixes Co-Authored-By: Claude Opus --- src/binding/transaction/transaction.cpp | 8 ++++---- test/lock-tracker.test.ts | 12 +++++------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/binding/transaction/transaction.cpp b/src/binding/transaction/transaction.cpp index 9e2d731cd..31eba32df 100644 --- a/src/binding/transaction/transaction.cpp +++ b/src/binding/transaction/transaction.cpp @@ -567,10 +567,10 @@ static void completeCommitWork(napi_env env, TransactionCommitState* state) { if (parkId == 0) { // No timeout thread behind this park -- resolve now rather - // than register with the LockTracker unbounded. Every path that - // returns 0 does so before `schedule` publishes `fired`, and the - // wake callback below is not built yet, so this side owns the - // park outright and needs no exactly-once gate. + // than register with the LockTracker unbounded. No exactly-once + // gate: `schedule` returns 0 only before it publishes `fired`, + // and the wake callback below is not built yet, so this side + // owns the park outright. vt->unrefTracker(t); // A closing tsfn (env teardown racing this resolve) must not be // touched again -- see the matching guard in diff --git a/test/lock-tracker.test.ts b/test/lock-tracker.test.ts index 6b3689176..df3ee77fb 100644 --- a/test/lock-tracker.test.ts +++ b/test/lock-tracker.test.ts @@ -194,13 +194,11 @@ describe('Coordinated retry — bounded park timeout (#741)', () => { const child = spawn(process.execPath, [parkTimeoutFixturePath, dbPath], { env: { ...process.env, ROCKSDB_JS_PARK_TIMEOUT_MS: '1' }, }); - // Backstop against a true hang only — the child's own `elapsed` - // assertion is the deadline check. This timer additionally covers - // node boot, type-stripping, addon load and the seed writes, so a - // budget near the child's would make a slow runner (SIGTERM, the - // fixture's diagnostic never thrown) indistinguishable from a - // regression. Kept under vitest's 30s testTimeout so the kill path - // still dumps the child's stderr. + // Hang backstop only — the child's own `elapsed` assertion is the + // deadline check, and this also has to cover node boot and addon + // load, so a budget near the child's would report a slow runner as + // a regression. Under vitest's 30s testTimeout so the kill still + // dumps stderr. const timer = setTimeout(() => child.kill(), 20_000); let stderr = ''; child.stderr?.on('data', (chunk) => {