HarperFast / HarperFast/rocksdb-js
Commit-completion's compare-then-store on TransactionState can overwrite an Aborted set concurrently by close()
- 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
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