HarperFast / HarperFast/rocksdb-js

Pessimistic transactions still poison the environment on a drop-race commit

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

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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.