HarperFast / HarperFast/rocksdb-js
Transaction-log tryClose can miss a bind-to-write transition and close with an uncommitted transaction
- 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
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