Hacktoberfest 2026: the issues maintainers tagged for October, open and beginner-friendly. Browse Hacktoberfest issues

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

Open
  • Is something wrong?
#769 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

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

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.

Description

completeCommitWork and Transaction::CommitSync both do:

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

Dominant language
C++
Stars
22
Forks
3
Avg merge
1d 7h
Merged PRs (30d)
49

Getting set up

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from HarperFast/rocksdb-js

All issues in HarperFast/rocksdb-js

Similar issues

More C++ issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.