HarperFast / HarperFast/rocksdb-js

EventListener callbacks pin DBDescriptor without purge-retry (pre-existing #672-class leak)

Open
#807 0 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 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

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.