EventListener reaches the DBDescriptor via a late-bound, racy weak_ptr — data race, lost-during-open background errors, and a purge-skip leak

[cb1kenobi]

Summary

The shared RocksDB EventListener (TransactionLogEventListener in
src/binding/database/db_descriptor.cpp) reaches the DBDescriptor through a
std::shared_ptr<std::weak_ptr<DBDescriptor>> 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),
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<weak_ptr<DBDescriptor>> with a single
listener-owned state object created before DB::Open:

struct ListenerState {
  std::mutex mutex;
  std::weak_ptr<DBDescriptor> 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).
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.

[cb1kenobi]

Branch fix/730-listener-lifecycle (stacked on #730) addresses two of the three hazards:

• (1) weak_ptr data race and (2) background error lost during DB::Open — fixed by replacing the late-bound shared_ptr<weak_ptr<DBDescriptor>> with a DBEventListenerState created before DB::Open and shared with the descriptor. The descriptor pointer is published/read under a mutex (no data race), and the background-error mirror lives on the shared state so OnBackgroundError can record an error while open is still running.

Verified: full Vitest suite (726 passed / 1 skipped), 107 native GTests, and a Guard Malloc teardown stress (workers racing open/flush/close + terminate on the shared descriptor) — all clean.

(3) purge-skip leak remains open. A flush/background callback still promotes the descriptor for its duration, so a racing final close can observe use_count > 1 and skip the purge, and nothing re-runs it after the callback releases. The fix needs a deferred purge retry that must not synchronously close RocksDB from a background-callback thread (that would deadlock on WaitForCompact) — unlike the backup/checkpoint precedent, which retries from an async state destructor on a normal thread. Leaving this for a focused follow-up with Guard Malloc validation.

Generated by Barber AI 🤖