HarperFast / HarperFast/rocksdb-js

Transaction-log tryClose can miss a bind-to-write transition and close with an uncommitted transaction

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

`TransactionLogStore::tryClose()` can return true and close a store while it has a written, uncommitted transaction. Its phase-2 collection check and phase-3 admission latch are separated by an unlocked interval. A transaction can move entirely through `pendingTransactionCount` in that interval, so phase 3 observes zero and closes anyway.

Found during #462's synchronization audit on main `b4d104562e7d5353b6d7a4412e0a8b48ee805062`; the performance branch does not modify this code. This is distinct from #808's orphaned `txn.state` file and from #784's transaction-handle drain contract.

## Concrete interleaving

1. Closer passes phase 1 (`pendingTransactionCount == 0`).
2. Closer checks `uncommittedTransactionPositions` under `writeMutex` + `dataSetsMutex`, sees no real transaction, and releases both mutexes.
3. Another thread binds a transaction under `transactionBindMutex`, sees `isClosing == false`, and increments `pendingTransactionCount`.
4. That transaction's `writeBatch()` finishes: it inserts its log position and the new sentinel under `dataSetsMutex`, then decrements `pendingTransactionCount` to zero. Its RocksDB commit / `commitFinished()` has not run yet.
5. Closer enters phase 3 under `transactionBindMutex`, sees zero pending, and sets `isClosing`. Phase 4 closes the files without rechecking the real uncommitted position.

The count-to-position handoff is individually protected, but the close decision does not atomically observe both representations. Replacing either mutex with an atomic counter would not resolve this protocol gap.

Relevant code: [phase 2/3](https://github.com/HarperFast/rocksdb-js/blob/b4d104562e7d5353b6d7a4412e0a8b48ee805062/src/binding/transaction_log/transaction_log_store.cpp#L124), [write completion](https://github.com/HarperFast/rocksdb-js/blob/b4d104562e7d5353b6d7a4412e0a8b48ee805062/src/binding/transaction_log/transaction_log_store.cpp#L1010), and [PurgeStores](https://github.com/HarperFast/rocksdb-js/blob/b4d104562e7d5353b6d7a4412e0a8b48ee805062/src/binding/transaction_log/transaction_log_store_registry.cpp#L350).

## Observed result and scope

A standalone native reproducer pauses a temporary copy of the production function immediately before phase 3. It performs the normal bind protocol and a real `writeBatch()`, withholding `commitFinished()`, then resumes the closer:

```text
before phase 3: pending=0 real uncommitted=1
tryClose=1 isClosing=1 real uncommitted=1
```

Expected: `tryClose()` returns false while that real position is uncommitted. The scheduling hook only widens a permitted interleaving; it does not change the count, position checks, locks, or closing logic. This demonstrates the violated native close precondition. It is **not** a report of a customer incident or a reproduced JavaScript-level data-loss event.

`PurgeStores(..., destroy=true)` treats a successful `tryClose()` as permission to erase the store from the registry and subsequently remove its directory. Thus the race can discard a transaction-log prefix still awaiting commit publication. Fixing it needs a single admission/drain protocol that excludes new binds while checking pending and written transactions; preserve the existing aim of avoiding filesystem I/O under `transactionBindMutex`.

## Reproducer (macOS, run from built repository root)

Add this one scheduling hook to a **temporary copy** of `transaction_log_store.cpp`, immediately before the `// Phase 3` comment, leaving the repository source unchanged:

```cpp
extern void mutex462BeforePhase3();
mutex462BeforePhase3();
```

Compile that copy with the following driver and the same Node-free store dependencies used by the native test target. The constructor and `writeBatch()` are the real implementation:

```cpp
#include
#include
#include
#include
#include "transaction_log/transaction_log_store.h"
#include "transaction_log/transaction_log_entry.h"
std::atomic phase3{false}, resumeClose{false};
namespace rocksdb_js {
void mutex462BeforePhase3() {
phase3.store(true); phase3.notify_one();
while (!resumeClose.load()) resumeClose.wait(false);
}
}
int main() {
auto path = std::filesystem::path("/tmp/rocksdb-tryclose-repro/store");
std::filesystem::remove_all(path.parent_path());
rocksdb_js::TransactionLogStore store("store",path,0,std::chrono::milliseconds(0),0);
bool closed = false;
std::thread closer([&] { closed = store.tryClose(); });
while (!phase3.load()) phase3.wait(false);
// Exactly the admission protected by transactionBindMutex in UseLog/addLogEntry.
{ std::lock_guard lock(store.transactionBindMutex);
if (store.isClosing.load()) std::abort();
++store.pendingTransactionCount;
}
std::string payload="not yet committed";
rocksdb_js::TransactionLogEntryBatch batch(1770000000001.0);
batch.addEntry(std::make_unique(nullptr,payload.data(),payload.size()));
rocksdb_js::LogPosition position;
store.writeBatch(batch,position);
size_t outstanding=0;
for (auto pos:store.uncommittedTransactionPositions)
if (pos.logSequenceNumber != store.nextLogPosition.logSequenceNumber || pos.positionInLogFile != store.nextLogPosition.positionInLogFile) ++outstanding;
std::cout << "before phase 3: pending=" << store.pendingTransactionCount.load() << " real uncommitted=" << outstanding << std::endl;
resumeClose.store(true); resumeClose.notify_one(); closer.join();
std::cout << "tryClose=" << closed << " isClosing=" << store.isClosing.load() << " real uncommitted=" << outstanding << std::endl;
std::filesystem::remove_all(path.parent_path());
return closed && outstanding ? 0 : 1;
}

```

On this machine the compile command was `c++ -std=c++20 -O2 -mmacosx-version-min=26.0 -DROCKSDB_JS_NATIVE_TESTS -I/include/node -Isrc/binding -Isrc/binding/transaction_log -Ideps/rocksdb/include src/binding/transaction_log/transaction_log_file.cpp src/binding/transaction_log/transaction_log_recovery.cpp src/binding/core/platform.cpp src/binding/core/debug.cpp test/native/event_emitter_stub.cc -o /tmp/tryclose-repro`.

Priority: **P1** — a confirmed integrity precondition violation with a narrow concurrent destructive-purge trigger; no demonstrated production loss or verified released-version range, so not P0. No matching repository epic was found. No customer or security context accompanies this local finding.

Contributor guide

Open the contributing guide

Research direction

Start with phase 2/3 and write completion in src/binding/transaction_log/transaction_log_store.cpp, then inspect PurgeStores in src/binding/transaction_log/transaction_log_store_registry.cpp. Run the supplied native reproducer with the temporary scheduling hook and verify that tryClose() refuses to close while a real transaction position remains uncommitted, without adding filesystem I/O under transactionBindMutex.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.