Data loss: IsBusy commit retry orphans an uncommitted log position, pinning the committed watermark → committed reads truncate

[kriszyp]

Data loss: an IsBusy commit retry orphans an uncommitted log position, permanently pinning the committed watermark → committed reads truncate

Summary

When an optimistic transaction's Commit() returns IsBusy and is retried, the log position recorded by the first writeBatch is orphaned in uncommittedTransactionPositions and never erased. Because the committed-read watermark (lastCommittedPosition) is computed as uncommittedTransactionPositions.front() (the earliest in-flight position), that orphaned position becomes a permanent floor: every subsequent committed read on that log is capped at the orphaned offset, even though the log file physically contains all the later entries. The result is silent, permanent truncation of committed reads with no error.

Impact

• Silent permanent data loss for any consumer of committed log reads after an IsBusy retry. We hit it as a replication backlog never reconciling (Harper harper-pro#426): a follower resumes from a valid cursor T, the leader's resume iterator drains a short prefix and then returns done hundreds of records short of head; the follower sits permanently behind, connected: true, no error.
• Not limited to replication — any committed read bounded by lastCommittedPosition (audit-log reads, time-travel, etc.) truncates at the pinned offset.
• No self-heal: the orphaned position is the front() minimum forever, and no flush path advances past it.

Root cause (verified on main)

src/binding/transaction/transaction.cpp (async path; sync path is the same shape at ~490/510):

1. writeBatch(..., txnHandle->committedPosition) (~:268) inserts position A into uncommittedTransactionPositions and durably writes A's bytes to the log file; committedPosition now references A.
2. Commit() returns IsBusy.
3. commitFinished(...) is gated on !IsBusy() (~:300) → skipped. A is not erased.
4. resetTransaction() (~:319) recreates the txn but does not reset/erase committedPosition (src/binding/transaction/transaction_handle.cpp — clears txn/logEntryBatch/snapshotSet only).
5. The retry's writeBatch overwrites committedPosition with a new position B. A is now orphaned — no handle references it, so neither commitFinished nor commitAborted can ever target it.
6. The retry's commitFinished(B) erases B, but uncommittedTransactionPositions.front() is still A, so *lastCommittedPosition stays pinned at A (src/binding/transaction_log/transaction_log_store.cpp ~:527-531, and commitFinished ~:740).

Reader side — src/transaction-log-reader.ts: committed reads iterate while (position < size) where size is the low-32 of lastCommittedPosition (re-read live each next()). Once lastCommittedPosition is pinned at A, next() returns done at A. No throw, no warn.

Note: this also affects the coordinatedRetry path. On IsBusy it still takes the same resetTransaction() route (~:289 saves VT slots, then the shared IsBusy handling at ~:300/:316), so the orphan occurs there too. VT coordination may reduce how often IsBusy fires, but residual conflicts still pin the watermark.

Versions affected

Confirmed in 2.1.0 (the old quadratic-backoff retry path) and main / 2.2.0 (the coordinatedRetry / VerificationTable line). harper #410 (record-caching) routes IsBusy through coordinatedRetry and bumps to ^2.2.0, so it carries this bug forward rather than fixing it.

Reproduction

Harper-pro stress test backlog-recovery (4-node mesh, one node offline 30 min under churn, then catches up). Reproduces intermittently (~50% at OFFLINE=30 / KEYS=800). Captured signature: the offline node resumes from real cursors, receives a burst of ~247 records covering only the first ~10s of a 30-min backlog, then stops — identically on two independent leaders (deterministic boundary, not a race) — and never converges. The backlog is physically present in each leader's log file past the watermark.

A direct rocksdb-js repro: run a write under contention so an optimistic Commit() returns IsBusy and is retried, then read the committed log range — it truncates at the orphaned position. (A DEBUG_LOG build shows the Commit failed with IsBusy, resetting transaction line at the orphaning point.)

Suggested fix (design — has a subtlety)

commitAborted(pos) already does the right teardown (positionErase(pos) + recompute lastCommittedPosition from front()). The minimal fix is to call it for the orphaned position on the IsBusy branch (when committedPosition.logSequenceNumber > 0) before resetTransaction() overwrites the reference.

Subtlety: writeBatch durably writes A's bytes to the log file before the commit outcome is known. Simply erasing A's tracking and advancing the watermark past it would expose A's aborted bytes to committed readers (the retry re-writes the same records at B → duplicates in the log). So the fix must also account for the orphaned bytes. Options:

• (a) commitAborted(committedPosition) on the IsBusy branch + ensure A's aborted log bytes are skipped/truncated by the reader (e.g. an abort marker, or truncate back to A on abort).
• (b) Make the IsBusy retry reuse position A instead of allocating B (no orphan, no duplicate bytes).

This intersects the log-bytes/committed-watermark invariant that the VerificationTable / coordinatedRetry work owns, so it's best designed alongside that.

Confidence

High on the orphan→pinned-watermark→truncation chain (code-verified on main and 2.1.0, reader + native store). The one open item is a direct trace tying a specific captured stall to an IsBusy retry vs. another never-commitFinished path — but all such paths orphan the position identically, so the fix is the same.

🤖 Filed by Claude on behalf of Kris.

[kriszyp]

Draft candidate up: #669. Lands the watermark un-pin (commitAborted() on the IsBusy branch, async + sync) plus native GoogleTest coverage reproducing the orphan→pin (control) and the un-pin (fix). The aborted-byte fencing — option (a): on-disk abort marker + reader/recovery skip + crash-consistency, intersecting the VT/coordinatedRetry watermark invariant — is left as the open design decision in the PR body rather than guessed at. Not for merge until that's settled.