HarperFast / HarperFast/rocksdb-js

DBRegistry::Shutdown() abandons other open databases to an unsafe process-exit teardown when one database's close-time flush fails (SIGABRT)

Open
#798 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
C++
Stars
21
Forks
2
Avg merge
2d 9h
Merged PRs (30d)
36

Description

## Summary

Found while rebasing PR #787 onto `main` and running the full `pnpm test` suite (60 files,
827 tests) to verify the rebase. Every in-suite test passes, but the *process itself* then
crashes at exit with:

```
rocksdb-js database registry cleanup failed: Failed to flush database during close: IO error: While open a file for appending: /testdb-/000010.log: Permission denied
pthread lock: Invalid argument
```

i.e. a genuine flush IO error for one database, immediately followed by a native SIGABRT.

## Confirmed native stack

gdb, `ROCKSDB_ASAN=1` Release build with `LD_PRELOAD`d ASan — ASan itself reports no heap
corruption, so this is a logic-level double-teardown rather than a buffer bug:

```
__GI_abort
rocksdb::port::Mutex::Lock() [clone .cold] <- aborts, pthread_mutex_lock returns EINVAL
rocksdb::PeriodicTaskScheduler::Unregister(...)
rocksdb::DBImpl::CancelPeriodicTaskScheduler()
rocksdb::DBImpl::CancelAllBackgroundWork(bool)
rocksdb::DBImpl::CloseHelper()
rocksdb::DBImpl::Close()
rocksdb::DBImpl::WaitForCompact(WaitForCompactOptions const&)
rocksdb::StackableDB::WaitForCompact(...)
rocksdb_js::DBDeleter::operator() src/binding/database/db_descriptor.h:54 (options.close_db = true)
std::shared_ptr::~shared_ptr
rocksdb_js::DBDescriptor::~DBDescriptor src/binding/database/db_descriptor.cpp:368
...
rocksdb_js::DBRegistry::~DBRegistry src/binding/database/db_registry.h:72 <- process-exit static destructor
__run_exit_handlers -> exit()
```

## Root cause, as far as I traced it

`DBRegistry::Shutdown()` (db_registry.cpp:935) iterates every open descriptor and closes it
via `closeClaimedDescriptors`. Per AGENTS.md invariant 6, a close-time flush failure is
*deliberately* fatal for `Shutdown()`/`PurgeAll()` (`failOnCompletedWithError`) rather than
silently swallowed, because dropping it would hide possible data loss:

```cpp
if (closeError) std::rethrow_exception(closeError);
```

That's by design for the *one* database that failed. But rethrowing exits `Shutdown()`
entirely — any *other* descriptors it had not yet reached (or was waiting on) are left in
the registry map, never gracefully closed. `Binding::Init`'s module cleanup hook
(binding.cpp:222-233) only logs the exception (`cleanup("database registry", ...)`), so from
Node's perspective the env teardown "succeeds." The leftover descriptors then survive to the
process's actual C++ static destructor sweep (`DBRegistry::~DBRegistry`, invoked from
`exit()`), which force-destroys each one through `DBDeleter`
(`db->WaitForCompact({.close_db=true})`) — a path that isn't designed to safely coordinate
teardown across *multiple* databases the way the graceful `Shutdown()`/`finishClose()` path
is. That's where the abort happens.

## What I haven't root-caused

The *triggering* flush error itself (`Permission denied` opening a WAL append file) — I
could not explain why that specific open failed. It's possible this is a genuine product bug
(some earlier operation left a descriptor/permission in a bad state) or an artifact of my dev
machine's very large, long-accumulated shared test tmp directory (8000+ leftover `testdb-*`
directories from unrelated sessions/days). I was not able to isolate this further within the
scope of a rebase task.

## Reproduction

- Bisected with two throwaway worktrees: clean on PR #787 alone (pre-rebase tip, 55 test
files), clean on plain `origin/main` alone (60 test files). Only the combination (both
histories' full test batteries, 60 files) reproduces — 3/3 on a plain build, 1/1 under
`ROCKSDB_ASAN=1` Release with `LD_PRELOAD`d ASan.
- Always the same shape: WAL file `000010.log` (db path varies, file number is consistently
#10), always inside the final `DBRegistry::Shutdown()` sweep at full-suite process exit.
- Have not reproduced it from a single test file or from either half of the suite alone — it
appears to need the larger combined battery's DB volume/concurrency.

## Related

- #741 (closed) — the umbrella issue for "shared `DBDescriptor` survives one env's teardown
while another env is still using it," same general hazard class, different manifestation
(worker-env teardown corrupting concurrent commits vs. this: process-exit static teardown
after `Shutdown()` aborts mid-sweep). Its open children #783, #784, #785, #786 don't cover
this specific path.
- #746 — same `mutex lock ... Invalid argument` / EINVAL symptom class, different subsystem
(`CommitWorker` teardown on Deno/macOS CI vs. `DBDeleter`/`PeriodicTaskScheduler` here on
Node/Linux at full-suite process exit).

## Suggested next step

Reproduce in a clean tmp directory to rule out environmental noise on the triggering flush
error. If it reproduces cleanly, the actionable fix is likely in `DBRegistry::Shutdown()`:
collect close errors from all descriptors and rethrow once at the end, rather than
abandoning the remaining descriptors to the unsafe static-destructor path on the first
failure.

— KrAIs, on behalf of @kriszyp

Contributor guide

Open the contributing guide

Research direction

Start by reproducing with a clean temporary directory using the full pnpm test suite, then read DBRegistry::Shutdown() and closeClaimedDescriptors in src/binding/database/db_registry.cpp. Trace the cleanup path through binding.cpp:222-233, db_descriptor.h:54, and db_descriptor.cpp:368. Done means a close-time flush failure does not leave other descriptors for unsafe static-destructor teardown, while the failure remains reported.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, node.js
Domain
backend, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.