Release the retained read handle's write intents when an outstanding-iterator commit replays

[kriszyp]

Why

8d69f1bd1 replaced the LINGERING deferral: a commit with outstanding read iterators now replays its staged writes onto a fresh transaction and commits them immediately, keeping the original native handle open solely so the iterators can finish. That closed a real class of acknowledged-write loss.

What it left behind is the retained handle's verification-table write intents. The replay owns those writes now, so no commit will ever run on the original handle — nothing releases its intents until the last iterator drains (or the long-transaction monitor releases the snapshot). Meanwhile every other writer's coordinatedRetry commit that conflicts on those slots parks on them. With today's unbounded park that is #2001's per-thread write wedge: a live capture showed one slot lock-tagged with holders=3 woken=0 more than five hours after onset, with the wedged thread 503-ing every write.

The retention window predates 8d69f1bd1 (LINGERING held the intents too), but there the pending commit still intended to write them. After the replay they are dead locks, which also makes them safe to release early.

What

Call abandonWrites() on the retained handle right after the replay commit is submitted. It drops the dead VT intents while leaving reads — including read-your-own-writes — working for the outstanding iterators.

Ordering is safe by construction: the replay's save loop has already staged its own intents on the same slots, so VT publish-blocking never gaps; a writer parked on the original's tracker wakes once both are released.

The call is fenced in try/catch, matching this function's other post-submit steps (onCommit, leaveWriteQueue): the replay commit is already in flight, so a throw here must not skip the aftercommit notify or the chain-store commit for an already-durable write.

Depends on HarperFast/rocksdb-js#747

abandonWrites() does not exist in the pinned @harperfast/rocksdb-js 2.6.1 — this change is inert until #747 lands, is released, and the dependency is bumped. The call is feature-detected (?.) so it is safe to merge in either order, and the test asserts the native method must exist once a version ≥ 2.7 is installed, so the fix cannot stay silently no-op after the bump.

Testing

unitTests/resources/transaction.test.js — new RocksDB-only case: an outstanding iterator plus a staged write drives the replay branch, and asserts the retained handle is abandoned exactly once, the replayed write is durable, and the iterator keeps producing entries afterward. Version-gated assertion that the native method is present once the dependency carries it.

End-to-end verification route: the release semantics are proven in rocksdb-js#747's park/wake test (a parked coordinated-retry commit resolves RETRY_NOW the moment the holder abandons). On the Harper side the wedge mechanism itself was reproduced live on a test cluster — see #2001 for the A/B — but it needs a leaked holder plus a conflicting commit, which is not something the unit suite can stage; this test pins the call contract and the surrounding invariants instead.

unitTests/resources/transaction.test.js: 30 passing. Cross-model review: codex + gemini + Harper domain pass.

Generated with Claude Fable 5.

[gemini-code-assist]

Code Review

This pull request introduces changes to release write intents on retained read transactions by calling abandonWrites() if available on the transaction handle. It also adds a corresponding unit test that dynamically resolves the installed version of @harperfast/rocksdb-js to verify this behavior. Feedback on the changes includes: 1) Preventing a potential TypeError and misleading warning log in DatabaseTransaction.ts by using optional chaining when accessing this.transaction in case it is null. 2) Applying optional chaining when accessing properties of the parsed package.json in the test helper to ensure robust parsing.

[claude]

Reviewed; no blockers found. Both prior findings (duplicate abandonWrites() call across retry rounds, and null-handle throw) are fixed in 83571b2 and confirmed by @kriszyp. The latest commit (6c0dfcc) only re-arms the test warning flag between suite files — no new concerns.

[kriszyp]

Guided tour

Two behavior changes, both inside commit()'s outstanding-iterators branch in resources/DatabaseTransaction.ts — the branch 8d69f1bd1 introduced.

1. Warn once about the pattern (before the replay is built)

if (!replayedWritesWarned) { replayedWritesWarned = true; harperLogger.warn?.(...) }

Committing while iterators are open means every staged write is re-staged and committed a second time. That cost is invisible to the application author today. The warning names the count and the startedFrom resource/method, and fires once per process — this is a report about a code pattern, not about an individual commit, so per-commit logging would be noise. Follows the existing outstandingCommitLogged precedent a few lines up.

2. Release the dead intents (after the replay commit is submitted)

if (!this.writesAbandoned) {
  this.writesAbandoned = true;
  try { (this.transaction as {...} | null)?.abandonWrites?.(); }
  catch (error) { harperLogger.warn?.(...); }
}

Four deliberate properties, each from a review finding:

• Placement — after transaction.commit() at :571, so the replay has already staged its own intents on the same VT slots. Publish-blocking never gaps; a writer parked on the original's tracker wakes once both are released.
• Once — a coordinated-retry or backoff round re-enters this same branch on the same retained handle (readTxnsUsed is only decremented once, guarded by baseReadRefConsumed). The flag makes the once-only invariant explicit rather than leaning on the callee's idempotency.
• Fenced — the replay commit is already in flight. An unfenced throw here would skip the onCommit hook (the aftercommit replication/txn-log notify) and the this.next chain-store commit, for a write the caller was already told succeeded. Matches how onCommit and leaveWriteQueue are treated in the same function.
• Optional + null-safe — ?.abandonWrites?.(), because of the dependency situation below.

Read this before reviewing the diff

abandonWrites() does not exist in the pinned rocksdb-js 2.6.1. This change is inert until rocksdb-js#747 lands, releases, and the dependency is bumped. That is why the call is feature-detected, and why the test version-gates an assertion that the native method must be present at ≥2.7 — so it cannot quietly stay a no-op after the bump.

What the test proves, and what it doesn't

It pins Harper's side: exactly one abandonWrites call, exactly one warning, the replayed write durable, and the iterator still producing entries afterward. It does not prove intents are released — that is rocksdb-js#747's park/wake test, in the other repo. Worth knowing when judging how much this suite guarantees on its own.

Where to look hardest

• Placement of the release relative to the replay's staging — that ordering is the whole safety argument.
• Whether once-per-process is the right cardinality for the warning, or whether it should be per-resource/method.

Review coverage

codex + gemini + Harper-domain adjudication pre-push, plus the CI bots on the pushed branch. Their findings produced the once-guard, the null-check, and the fence. grok was version-gated and did not run.

Posted by Claude Fable 5 on behalf of @kriszyp.

[cb1kenobi]

High: writesAbandoned is never reset, so the release is defeated on a reused transaction object

writesAbandoned is a per-native-handle once-guard (like baseReadRefConsumed), but unlike baseReadRefConsumed it is never cleared. getReadTxn() resets baseReadRefConsumed = false when it creates a fresh this.transaction, precisely because a DatabaseTransaction is reused across handle lifecycles (see the // this transaction be reused and committed again note in the success handler; doneReadTxn() nulls this.transaction, then a later getReadTxn() builds a new one). writesAbandoned stays true across that boundary.

Failure sequence on a reused object (e.g. a long-lived context that read-iterates-writes-commits more than once):

1. Batch 1 commits under open iterators → replay + abandonWrites(), writesAbandoned = true; iterators drain → doneReadTxn() aborts and nulls the handle.
2. Same object, batch 2: getReadTxn() mints handle H2 (resets baseReadRefConsumed, not writesAbandoned). Commit under open iterators re-enters this branch, but if (!this.writesAbandoned) is now false → abandonWrites() is skipped for H2. H2's VT write intents leak exactly as before this PR — the harper#2001 per-thread write wedge recurs on the second and later occurrences.

The fix mirrors the sibling guard — reset it where the fresh handle is created, in getReadTxn() next to this.baseReadRefConsumed = false;:

		this.readTxnsUsed = 1;
		this.baseReadRefConsumed = false; // fresh handle, fresh base reference for commit() to consume
		this.writesAbandoned = false; // fresh handle: its write intents have not been abandoned yet

This keeps the intended once-per-commit-cycle guard (retry rounds re-enter with this.transaction unchanged, so it still won't double-fire) while re-arming for the next reuse. Worth adding a test for the two-cycle reuse path — the current test only exercises a single abandon within one cycle, which is exactly the case that passes regardless.

—
Generated by Barber AI