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

Open
#754 3 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.