HarperFast / HarperFast/rocksdb-js

withLock(): a pending lock dispatch does not keep the event loop alive — promise never settles on an idle process, double free at teardown

Open
#771 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

A pending `db.withLock()` dispatch does not keep the Node event loop alive. If nothing else is
keeping the loop busy, the lock callback is never dispatched, the promise `withLock()` returned never
settles, and the queued threadsafe-function call is finally drained during environment teardown —
against callback data that has already been freed, aborting the process with
`free(): invalid pointer`.

Reproduces on `main` (`b255c5ce`) with stock `node`, no flags, no Vitest.

## Reproduction

```js
import { RocksDatabase } from './dist/index.mjs';
const db = new RocksDatabase('/tmp/withlock-repro-' + Date.now());
db.open();
const keepAlive = process.env.KEEPALIVE === '1' ? setInterval(() => {}, 10) : null;
for (let i = 0; i < 3; i++) {
await db.withLock('foo', () => {});
console.log('iteration', i, 'done');
}
console.log('all done');
if (keepAlive) clearInterval(keepAlive);
db.close();
```

```
$ node repro.mjs
iteration 0 done
Warning: Detected unsettled top-level await at repro.mjs:6
free(): invalid pointer
Aborted (core dumped)

$ KEEPALIVE=1 node repro.mjs
iteration 0 done
iteration 1 done
iteration 2 done
all done
```

The only difference is an unrelated `setInterval` holding the loop open. That is the whole bug: the
first `withLock` completes, and from then on a queued lock dispatch has nothing referencing the loop,
so Node concludes there is no work left and begins teardown with the call still queued.

The callback being sync or async makes no difference, nor does using the same key or distinct keys
per iteration, nor whether the database is closed or garbage collected in between.

## Backtrace

```
_Sp_counted_ptr::_M_dispose()
_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release()
rocksdb_js::callJsCallback(napi_env, napi_value, void*, void*)
v8impl::(anonymous namespace)::ThreadSafeFunction::AsyncCb(uv_async_s*)
uv__async_io → uv__io_poll → uv_run
node::Environment::CleanupHandles()
node::Environment::RunCleanup()
node::FreeEnvironment(node::Environment*)
```

`callJsCallback` adopts the raw `LockCallbackCompletionData*` into a `shared_ptr` for RAII
(`db_descriptor.cpp`), so the deferred delivery disposes a pointer whose storage is already gone.

## Where to look

`DBDescriptor::lockCall` (`src/binding/database/db_descriptor.cpp`):

- It calls `napi_release_threadsafe_function(threadsafeCallback, napi_tsfn_release)` after every
dispatch. If those releases are not balanced by an acquire per queued callback, the tsfn drops to
zero references and stops holding the loop open — which matches the observed behavior exactly.
- Its failure path deletes the callback data and calls `onCallbackComplete` only when the status is
neither `napi_ok` nor `napi_closing`. On `napi_closing` the data leaks *and* the deferred is never
rejected, so a caller in that case waits forever with no error.

I have not attempted a fix; both of the above are candidates rather than a diagnosis.

## Impact

Masked in any process whose event loop is busy for other reasons — a server under load, or Vitest —
where the call is still delivered, just late. That lateness is itself observable: with
`DENO_V8_FLAGS=--expose-gc` set so Deno's Vitest workers actually expose GC (see the companion
issue), `test/lock.test.ts > withLock() > should lock and unlock` takes **27.5s** instead of ~160ms on
Deno 2.8.3, and the two withLock tests after it hit the 30s timeout. The existing Deno job does not
catch this because it exposes no GC and the timing never shifts.

The failure modes for a consumer are a `withLock()` promise that never settles when the process goes
idle, and an abort during shutdown.

---
Filed by KrAIs (Claude Opus 5) while fixing CI on #768.

Contributor guide

Open the contributing guide

Research direction

Start with DBDescriptor::lockCall in src/binding/database/db_descriptor.cpp, then reproduce the idle-process behavior with the Node script in the issue. Compare the dispatch and failure paths, and use test/lock.test.ts as the existing behavioral check; done means withLock() settles without an unrelated event-loop keepalive and teardown no longer aborts.

Written by the indexing model from the issue text.

Assessment

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.