HarperFast / HarperFast/rocksdb-js

Commit-completion's compare-then-store on TransactionState can overwrite an Aborted set concurrently by close()

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

`completeCommitWork` and `Transaction::CommitSync` both do:

```cpp
if (handle->state == TransactionState::Committing) {
handle->state = TransactionState::Pending;
}
```

The comment on the async one already names the hazard it is trying to avoid — *"Guard: keep Aborted if close() already set it (DB closing during commit) — don't let Transaction::Abort call Rollback() on a null txn"* — but a compare-then-store is not atomic. `close()` runs on a different thread (`DBDescriptor::close()` from another env's JS thread, and `close()` sets `Aborted` at `transaction_handle.cpp`), so it can land between this load and this store. The guard then reads `Committing`, `close()` sets `Aborted` and destroys `txn`, and the store puts the state back to `Pending`.

A handle left `Pending` with `txn == nullptr` is exactly the state the guard exists to prevent: a subsequent `Transaction::Abort` passes its state checks and calls `Rollback()` on a null transaction.

The window is small and requires a DB close concurrent with a failing commit, which is why it has not been seen in the wild. It is pre-existing — surfaced by review while working on #2107 / #768, not introduced by it.

**Suggested fix:** make the transition itself atomic rather than guarding it with a read. That means `state` becomes `std::atomic` and these sites become `compare_exchange_strong(expected = Committing, Pending)`. Note two things that came out of trying the field conversion in #768 and are worth knowing before starting:

- `DEBUG_LOG` is variadic and `std::atomic` has no copy constructor, so every `DEBUG_LOG(..., this->state)` site (`transaction_handle.cpp` `get`/`getSync`/`putSync`/`removeSync`) has to cast, or the debug build stops compiling.
- The implicit `std::atomic` operators are `seq_cst`. `state` is checked on the read and write paths, so the conversion needs explicit relaxed loads/stores at those sites or it adds a full barrier to every operation.

Both are why #768 reverted its own conversion rather than carrying it — the field change is worth doing on its own terms, with those two costs handled deliberately.

---

## Scope now also covers the field conversion (from #768 review)

Gemini flagged the same `state` field on #768, arguing a data race between the commit thread's write in `executeCommitWork` and `onWrapperCollected()`'s read on the JS thread. **That specific mechanism is not reachable** and the finding was closed on that PR: `Transaction.commit()` is an `async` method whose suspended frame holds `this` across the await (its `finally` touches `this.#txn`), and the native side holds strong napi references to `resolve`/`reject`, which root that frame — so the JS wrapper cannot be collected while a commit is in flight, and the finalizer cannot run concurrently with `executeCommitWork`.

What *is* real, besides the compare-then-store above, is the same shape against `close()`: every plain read of `state` (`Transaction::Abort`, `Transaction::Commit`, `getSync`, `putSync`, `onWrapperCollected`) can race `close()` setting `Aborted` from another environment's JS thread on the `DBDescriptor::close()` PATH A. Formal UB, benign in practice today — a stale `Committing` returns without closing while the other thread's `close()` performs the release, and a stale non-`Committing` calls `close()`, which hits the `closed.exchange(true)` gate.

So the conversion and the `compare_exchange_strong` fix want to land together here rather than being split: the transition fix needs the atomic, and the atomic on its own buys only the formal-UB cleanup.

A tested conversion already exists and can be lifted rather than redone — relaxed loads/stores at all ~15 sites plus the `DEBUG_LOG` casts, release and debug both building, full suite green. It was written on the #768 branch, backed out to keep that PR on the leak, and is kept at `~/dev/tmp/2107-snapshot-leak/768-state-race-and-gc-pressure.patch` on Kris's machine (only the `state` half is still relevant; the GC-pressure half was superseded by the `DENO_V8_FLAGS` fix). Ask before redoing it from scratch.

Contributor guide

Open the contributing guide

Research direction

Start with completeCommitWork and Transaction::CommitSync, then trace state reads and writes in transaction_handle.cpp, including DBDescriptor::close() and onWrapperCollected(). Review the tested state conversion from #768 if available, and verify release and debug builds plus the full test suite; done means the concurrent close/commit transition is safe without regressions.

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
Quiet
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.