HarperFast / HarperFast/rocksdb-js
EventListener reaches the DBDescriptor via a late-bound, racy weak_ptr — data race, lost-during-open background errors, and a purge-skip leak
- Dominant language
- C++
- Stars
- 21
- Forks
- 2
- Avg merge
- 2d 9h
- Merged PRs (30d)
- 36
Description
## Summary
The shared RocksDB `EventListener` (`TransactionLogEventListener` in
`src/binding/database/db_descriptor.cpp`) reaches the `DBDescriptor` through a
`std::shared_ptr>` that is **bound after `DB::Open`
returns** (`*descriptorWeakPtr = descriptor`), while RocksDB background threads
can invoke the listener's callbacks *during and after* open. This is a
pre-existing pattern (it predates the background-error work); a Codex
cross-model review of the `#730` branch surfaced three concrete hazards in it.
Filing as a scoped follow-up so `#730` can land without expanding into a
teardown-lifecycle refactor.
The listener currently overrides `OnFlushBegin` / `OnFlushCompleted` and (on the
`#730` branch) `OnBackgroundError` / `OnErrorRecoveryEnd`.
## Hazards
### 1. Data race on the shared `weak_ptr` (UB) — blocker
The opening thread executes `*descriptorWeakPtr = descriptor` while a background
flush/compaction thread may be calling `descriptorPtr->lock()` on the *same*
`weak_ptr` object. `std::weak_ptr` thread-safety only covers *distinct*
smart-pointer objects sharing a control block — concurrent read + write of one
`weak_ptr` object is a data race and undefined behavior (potential corruption /
crash). Affects the flush callbacks today; the `#730` background-error callbacks
inherit it.
### 2. A background error latched during `DB::Open` is lost (`#730`-specific) — significant
If RocksDB schedules recovery flush/compaction during `DB::Open` and it fails
*before* the weak pointer is bound, `descriptorPtr->lock()` returns null and the
callback returns without recording anything. Open can then succeed with RocksDB
internally latched read-only while `db.backgroundError` reports `null`.
### 3. A callback pin can permanently skip the descriptor purge (leak) — significant
Both callback bodies promote the descriptor to a `shared_ptr` for their
duration. If the final JS handle closes while a callback holds that reference,
`DBRegistry::PurgeIfUnreferenced()` observes `use_count > 1` and skips teardown;
when the callback returns and drops its ref, nothing re-runs the purge. The
registry entry and the open RocksDB instance leak. This is exactly the
skipped-purge / retry-after-release hazard documented in `AGENTS.md` invariant
#6 (the same shape fixed for backup/checkpoint in
[HarperFast/rocksdb-js#672](https://github.com/HarperFast/rocksdb-js/issues/672)),
but the retry must **not** synchronously close RocksDB from a background callback
thread — it has to be deferred.
## Proposed direction
Replace the late-bound `shared_ptr>` with a single
listener-owned state object created **before** `DB::Open`:
```cpp
struct ListenerState {
std::mutex mutex;
std::weak_ptr descriptor; // published under `mutex` after open
BackgroundErrorState bgError; // self-contained; usable before the descriptor exists
};
```
- Publication (`state->descriptor = descriptor`) and every `descriptor.lock()`
read go through `mutex` → fixes (1).
- `OnBackgroundError` writes to `bgError` directly (no descriptor needed), so an
error latched during open is captured and transferred to the descriptor once
it is constructed → fixes (2). The descriptor shares the same `bgError`.
- After a callback releases its promoted `shared_ptr`, schedule a **deferred**
`PurgeIfUnreferenced` retry off the RocksDB background thread (never close the
DB synchronously from the callback) → fixes (3).
## Verification
This is teardown-lifecycle code — the exact class that surfaces as native heap
corruption. Verify with the Guard Malloc / worker-loop procedure in `AGENTS.md`
("Debugging native heap corruption"), plus `test/commit-teardown.test.ts` /
`test/concurrent-teardown.test.ts` style coverage for a close racing a
background callback.
## Context
Found by a Codex cross-model review of the `#730` branch
([#730 — No in-process recovery from a latched background error; only a restart clears it](https://github.com/HarperFast/rocksdb-js/issues/730)).
Hazards (1) and (3) exist on `main` independently (flush listener); (2) is
`#730`-specific. Best landed stacked on `#730` since it also fixes that branch's
new callbacks.
🤖 Filed by Claude on behalf of Chris.
Contributor guide
Research direction
Start in src/binding/database/db_descriptor.cpp and read AGENTS.md invariant #6 plus the native heap corruption debugging procedure. Trace TransactionLogEventListener through DB::Open, its flush and background-error callbacks, and DBRegistry::PurgeIfUnreferenced; compare the teardown patterns in test/commit-teardown.test.ts and test/concurrent-teardown.test.ts. Done means callbacks are synchronized, errors during open are retained, and purge retries are deferred off RocksDB callback threads.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, node.js
- Domain
- databases
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 38/100