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
- 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
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