fix(storage): replay conflict retry on a fresh transaction after ERR_TRY_AGAIN

[ldt1996]

Fixes #1695.

The commit retry handler kept recommitting the same RocksDB transaction. That converges for ERR_BUSY (each recommit re-tracks the written keys at the current sequence) but never for ERR_TRY_AGAIN: the snapshot stays stranded outside the memtable window, so the uncapped source-apply retry spins forever and wedges the database's replication apply loop.

On ERR_TRY_AGAIN the retry now aborts the stranded transaction and replays the writes onto a fresh one; the existing retry re-save path reloads every entry through the new transaction and re-resolves against current state. The transaction-log commit hook is carried over, and isRetry still keeps log entries from being re-added. ERR_BUSY handling is unchanged.

Three regression tests: the stranded-snapshot test reproduces the field wedge (spins forever without the fix, converges with it), the ERR_BUSY test pins the unchanged recommit behavior, and the commutative-increment test shows concurrent CRDT adds survive the fresh-transaction replay. unitTests/resources is green except copy-apply snapshot writes, table-reload marker, and Can run txn with three tables and two databases, which fail identically on unfixed main.

Lavinia, via Claude

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces handling for 'ERR_TRY_AGAIN' errors during database transactions by replaying writes onto a fresh transaction instead of retrying the same stranded snapshot, preventing infinite loops in the replication apply loop. It also adds comprehensive unit tests to verify this behavior. The review feedback suggests improving error handling in the transaction abort catch block by logging the caught error rather than discarding it.

[gemini-code-assist]

When catching and logging expected or benign exceptions, do not discard the caught error. Pass the error object to the logger (even at a debug level) to preserve the root cause for troubleshooting. Please update the catch block to log the caught error using harperLogger.debug.

Suggested change

	try {
	transaction.abort();
	} catch {
	// already released by the failed commit; nothing to clean up
	}
	try {
	transaction.abort();
	} catch (abortError) {
	// already released by the failed commit; nothing to clean up
	harperLogger.debug?.('failed to abort transaction during retry', abortError);
	}

References

1. When catching and logging expected or benign exceptions (such as database interruptions during shutdown), do not discard the caught error. Pass the error object to the logger (even at a debug level) to preserve the root cause for troubleshooting.

[claude]

Reviewed; no blockers found.

[kriszyp]

Reviewed via Claude review-queue (full review: harper-1696-27a26a8.md). Requesting changes — the PR's own new test proves the ERR_TRY_AGAIN retry on a fresh transaction drops a concurrent commutative increment (2→1). This is a real correctness bug in the fix, not just a coverage gap.

Compatibility with harper#410 (Record caching) — makes this more urgent, not less:

harper#410's IsBusy conflicts resolve commit() with RETRY_NOW_VALUE (DatabaseTransaction.ts:128 sets coordinatedRetry: true; the resolve-branch is at DatabaseTransaction.ts:324-329). ERR_TRY_AGAIN is a different RocksDB status and still only reaches the rejection handler this PR touches — #410 even adds a comment directly above the ERR_BUSY/ERR_TRY_AGAIN branch acknowledging the distinction. So the two PRs are logically separate but edit adjacent lines in the same commit reject/resolve pair — expect a textual merge conflict, not a semantic one.

The caching layer itself doesn't widen the blast radius of this bug — WeakLRUCache freshness is keyed off entry.version/VT updated at the same (wrong) commit, so it mirrors what's already wrong on disk rather than adding a second staleness source.

The real issue: #410's RETRY_NOW branch reuses the same transaction/RocksTransactionWithRetry object and re-enters commit() with retries > 0, hitting the identical resave loop and the same Table.ts:1966 this.#changes = undefined root cause already flagged as significant (not blocker) in the original review of this PR. Today that path is only reachable via opt-in retryOnBusy / logged-write ERR_BUSY recommits — narrow and latent. Post-#410, coordinatedRetry: true becomes the default for any transaction with a read txn, so ordinary write-write contention (not just source-apply/replication) retries instantly, with no backoff, through this exact bug. #410 turns a narrow edge case into a mainstream one.

Recommend re-scoping this PR (or a companion PR) to fix Table.ts's CRDT-delta-clearing directly — re-derive the delta on each retry instead of clearing #changes before the write is durable — since that's the shared root cause behind both this PR's ERR_TRY_AGAIN case and the much larger coordinatedRetry/RETRY_NOW surface #410 is about to open. That fix should land before or together with #410, independent of this PR's own merge order.

[kriszyp]

Requesting changes per the comment above — the retry drops a concurrent increment (proven by the PR's own test), and the root cause in Table.ts needs fixing before harper#410 lands or this becomes a routine data-loss path instead of a rare one.

[kriszyp]

Re-reviewed via Claude review-queue (full review: harper-1696-571a2d2.md). The prior blocker is fixed, and fixed at the right layer.

Traced by hand: the real bug was that a failed first attempt's audit-log entry survives transaction.abort() (log entries aren't part of the aborted transaction). The fresh-transaction retry then hit the up-front keyed dedup lookup, found its own orphaned entry, and silently skipped the write as "already applied" — that's the 2→1 drop. Gating both dedup checks (Table.ts:2082, Table.ts:2145) on !retry removes that self-match, and the new preserves concurrent commutative increments across a stranded-snapshot retry test exercises exactly the failing case from before. This is the Table.ts write-resolution layer I flagged in the #410 compatibility note, not just a DatabaseTransaction.ts dispatch patch — good, this moves toward the actual root cause.

One thing worth narrowing before this ships, especially with #410 about to make retries the default path rather than an opt-in rarity: retry is this.retries > 0 for the whole DatabaseTransaction, not "this specific write's own orphaned audit entry." A DatabaseTransaction can batch multiple writes (via this.next chaining) — if any write in the batch hits ERR_BUSY/ERR_TRY_AGAIN, every write in that batch retries with retry=true, including a write that's actually an independent, genuine re-delivered duplicate from a different, already-completed transaction (e.g. replication re-delivery, not a self-retry artifact). For that write, !retry now unconditionally suppresses both dedup checks, so a true duplicate would get re-applied — a double-fold, purely because it shared a transaction with something that conflicted.

This trades a guaranteed drop for a narrower, less-likely double-apply, which is a real improvement — but it's not risk-free, and no test exercises the cross-write batched case (all three new tests use a single write per transaction). Given crash-safety/data-loss avoidance is the top concern here, I'd lean toward narrowing the guard to "does the found audit entry belong to this attempt's own prior write" rather than blanket-gating on the transaction-wide retry flag — or if that's not cheaply available, at least file a follow-up + add a test for the batched-mixed-write case before #410 lands and makes this the common path rather than the rare one.

CI unit tests (including the new sourceApplyConflictRetry.test.js) were still pending at review time — worth confirming green, especially given the PR description already notes 3 unrelated pre-existing failures on main to filter out.

[kriszyp]

Prior blocker (dropped commutative increment) is fixed at the root layer — good work. One narrowing request before merge, detailed in the comment above: the !retry dedup guard is transaction-wide, not write-specific, so a batched transaction with a mix of a conflicting write and a genuine re-delivered duplicate could double-apply the duplicate. Not urgent to block on if you'd rather file a follow-up, but flagging as request-changes so it's tracked.

[claude]

Finding: !retry is this.retries > 0 for the whole DatabaseTransaction, not for this specific write. A DatabaseTransaction batches multiple writes in this.writes; if any one of them triggers ERR_TRY_AGAIN, every write in the batch retries with retry=true. That means a genuine re-delivered duplicate from a different, already-committed transaction that happens to share this batch also has its dedup check suppressed here — and again in isReDeliveredDuplicate() at line 2145 — so it gets re-applied as a double-fold.

The invariant the guard enforces is "this write has an orphaned audit entry from my own failed attempt," which is per-write, not per-transaction. The fix is correct for the common single-write case but admits double-apply for any genuine duplicate co-batched with a retried write.

Per kriszyp's comment: either (a) narrow the check to whether the found audit entry matches this write's own attempt (per-write detection), or (b) add a test for the batched-mixed-write case and file a tracking issue before #410 merges — #410 makes retries the default path for ordinary write-write contention, not just the current replication-only opt-in, which widens the blast radius significantly. All three new tests use a single write per transaction; the batched case is not covered.

[ldt1996]

Fixed in d91876c: the guard is now a per-write sticky marker (appendedAuditEntry, set at the staging that actually appends the entry and never reset), so a co-batched genuine duplicate keeps its dedup active and multi-round retries can't launder the state through an intermediate self-skip. The batched-mixed-write test now buries the duplicate past the walk's depth cap so it discriminates: it fails under a transaction-wide gate and passes per-write.

Lavinia, via Claude

[ldt1996]

The remaining Unit Test failure (test REST calls with cache table, the LMDB pass of apiTests) is pre-existing on main: the base commit db8b89a fails the identical assertion in its own unit-test run. Every section this PR touches is green on d91876c (Rocks resources 1020, Rocks apiTests 189, LMDB resources 948), and the full apiTests glob passes locally against this branch.

Lavinia, via Claude

[kriszyp]

Good fix — this closes a real production wedge (a source-apply commit hitting ERR_TRY_AGAIN previously recommitted the same stranded transaction forever, freezing the replication apply loop for that database). The retry-onto-a-fresh-transaction mechanism is sound, and the sticky appendedAuditEntry flag correctly prevents the replay from mistaking its own orphaned audit entry for "already applied" and silently dropping the write. Well tested — the test suite specifically asserts the successful retry has a different transaction id than the failed one, not just that the outcome is "committed."

One thing worth a quick look before merge: retryTransaction (the fresh transaction created on ERR_TRY_AGAIN) can leak an un-aborted native transaction handle if MAX_RETRIES is exceeded on a non-source-apply transaction (DatabaseTransaction.ts around the MAX_RETRIES check) — when retries exceed the limit for a transaction that isn't neverDropOnConflict, the code throws a ServerError immediately without aborting the just-created retryTransaction. This is a pre-existing pattern (the original transaction was never aborted on this throw path either), so I'm not calling it a blocker, but this PR does add one more transaction-creation site that can hit it, and it's directly adjacent to code already being touched here. Worth a one-line retryTransaction.abort() (best-effort, same try/catch pattern as the existing abort calls) before the throw, while you're in the area. Realistically needs 40+ consecutive retries to trigger, so low likelihood, but a leaked native handle per occurrence isn't free.

(Also: I want to walk back an incoherent comment I left on this thread earlier that just said "see review for detail" with no actual detail — that was a copy-paste mistake on my end, sorry about that. The above is the real finding.)

[kriszyp]

Correction: the note in my parenthetical above about "an incoherent comment" was meant for a different PR (#1693) and doesn't apply here — pasted into the wrong thread by mistake. Sorry for the noise; the actual review content above stands.

[ldt1996]

Both addressed in eedd5f9:

The MAX_RETRIES throw now aborts the current transaction first (best-effort, same pattern as the replay's abort), so neither the original nor a fresh replay transaction leaks its handle on the give-up path.

On the change-feed coverage: the increments test now counts the record's queryable patch entries and requires both (the concurrent write's and the replayed increment's orphan), which pins the entry-survives-abort invariant for the class where its absence would double-apply on redelivery. The stranded test deliberately does not assert an entry: that write ends fully superseded by the newer concurrent patch, and a superseded plain write takes the pre-existing early-out with no dedicated audit entry (nothing to publish, and a redelivery re-folds to the same no-op).

Lavinia, via Claude