HarperFast / HarperFast/rocksdb-js
Pessimistic transactions still poison the environment on a drop-race commit
- Dominant language
- C++
- Stars
- 21
- Forks
- 2
- Avg merge
- 2d 9h
- Merged PRs (30d)
- 36
Description
## What
`DBHandle::transactionWriteOptions()` (`src/binding/database/db_handle.h`) deliberately
excludes `ignore_missing_column_families` — setting it would turn a transaction commit that
spans a live column family and a dropped one into a silent partial commit (live half applied,
dropped half discarded, reported as success). That would break the atomicity
`Transaction::Commit` promises, so PR #725 kept this off the transactional path.
The tradeoff: in **pessimistic** mode, a commit that races a column-family drop reaches
RocksDB's fatal path and latches a background error on the whole environment — the same
poisoning behaviour PR #725 fixes for non-transactional `putSync`/`removeSync`, still reachable
here. In **optimistic** mode (Harper's default, and the only mode Harper uses) this is already
safe: conflict validation rejects the commit early with an attributable
`Could not access column family ` error and nothing is applied.
This is a **pre-existing bug, unchanged by PR #725** — not a regression it introduces. It's
already documented at the call site (`db_handle.h`, see `transactionWriteOptions()`'s doc
comment) and pinned by a test (`test/drop.test.ts`, "should not partially apply a pessimistic
transaction spanning a dropped column family") that asserts only atomicity and explicitly notes
the poisoning is a separate issue.
## Why it needs real work, not a small patch
A transaction's touched column-family set isn't known at `BeginTransaction` — it's only
finalized as writes are staged. A correct fix needs:
- Per-column-family "in-flight touch" tracking, incremented on each transactional
`Put`/`Delete` and decremented on commit/rollback.
- A "dropping" flag on `ColumnFamilyDescriptor` so a write racing an in-progress drop fails
fast instead of proceeding.
- `Database::Drop`/`DropSync` (`src/binding/database/database.cpp`) checking/waiting on that
count before calling `DropColumnFamily`, coordinated with the existing `columnsMutex` lock
ordering and the async commit pipeline (`CommitWorker`) without introducing deadlocks against
RocksDB's own lock timeouts.
This spans `db_descriptor.h/.cpp`, `db_handle.h/.cpp`, `database.cpp`'s transactional write
paths, and `transaction.cpp`/`transaction_handle.cpp`'s commit/rollback paths, plus new
concurrency tests — a cross-cutting hot-path change with real deadlock/perf risk, not
appropriate to rush into a review-response patch.
## Refs
Flagged in review of #725. See `db_handle.h`'s `transactionWriteOptions()` doc comment and
`test/drop.test.ts`'s pessimistic-transaction drop test for the current state.
Contributor guide
Research direction
Start with transactionWriteOptions() in src/binding/database/db_handle.h and the existing pessimistic drop-race test in test/drop.test.ts. Trace transactional write and commit/rollback paths through db_descriptor.h/.cpp, db_handle.h/.cpp, database.cpp, transaction.cpp, and transaction_handle.cpp, including CommitWorker and columnsMutex ordering. Done means a racing pessimistic drop cannot poison the environment, commits remain atomic, and concurrency tests cover the behavior without deadlocks.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, node.js
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 35/100