HarperFast / HarperFast/rocksdb-js
EventListener callbacks pin DBDescriptor without purge-retry (pre-existing #672-class leak)
- Dominant language
- C++
- Stars
- 21
- Forks
- 2
- Avg merge
- 2d 9h
- Merged PRs (30d)
- 36
Description
### Summary
The RocksDB `EventListener` callbacks on `TransactionLogEventListener` take a transient `shared_ptr` pin of the `DBDescriptor` via `DBEventListenerState::lockDescriptor()` and release it without the purge-retry the registry's ownership contract requires (see invariant #6 in `AGENTS.md`, HarperFast/rocksdb-js#672).
If the final `DBHandle` closes while such a callback is running on a RocksDB background thread, `DBRegistry::PurgeIfUnreferenced()` sees `use_count > 1` (registry + the callback's transient pin) and skips the purge; when the callback then releases its pin, nothing re-runs the purge. The descriptor and its open RocksDB can remain registered until the path is reopened.
### Affected call sites (all three share the pattern)
- `OnFlushBegin` — `src/binding/database/db_descriptor.cpp:230`
- `OnFlushCompleted` — `src/binding/database/db_descriptor.cpp:266`
- `OnStallConditionsChanged` — `src/binding/database/db_descriptor.cpp:338`
### Why a separate issue
Surfaced by cross-model review of #801 (the `writeStall` event), but it is **pre-existing** — the flush callbacks have carried the same transient pin since before that PR. A correct fix is architectural (a background-thread-safe purge-retry after releasing the pin, or a listener-drain barrier in the close path, applied to all three callbacks), so it shouldn't be bolted narrowly onto the new callback. The async ops that already handle this correctly (backup / backup-stream / checkpoint, per invariant #6) re-run `PurgeIfUnreferenced` from their state destructors — the listener callbacks need an equivalent.
### Notes
- Narrow window (callback in flight exactly as the last handle closes); consequence is a leaked descriptor + open RocksDB until the path is reopened (a later open of the same path reuses the still-registered descriptor rather than leaking twice).
- Any fix must not re-enter the registry unsafely from the background thread (see invariant #12's re-entrancy warnings).
Contributor guide
Research direction
Start with AGENTS.md invariants #6 and #12, then inspect the three callbacks in src/binding/database/db_descriptor.cpp at lines 230, 266, and 338. Compare their transient descriptor handling with the backup, backup-stream, and checkpoint state destructors; done means all three callbacks satisfy the purge-retry contract without unsafe background-thread re-entry.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 47/100