fix(txn): retry ERR_TRY_AGAIN on the same transaction (native in-place reset) - #1823
Merged
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates @harperfast/rocksdb-js to version 2.5.0 and refactors transaction retry logic in DatabaseTransaction.ts so that both ERR_BUSY and ERR_TRY_AGAIN errors recommit the same native transaction, which is now reset in place onto a fresh snapshot. It also adds transaction aborting when retries are exhausted to prevent handle leaks. The reviewer recommends that when aborting a chain of linked transactions, all links in the chain should be poisoned and closed first before aborting each individual link to avoid leaking native handles for other transactions in the chain.
Contributor
|
Reviewed; no blockers found. |
kriszyp
force-pushed
the
kris/tryagain-align
branch
from
July 21, 2026 23:35
d965630 to
c94a952
Compare
kriszyp
marked this pull request as ready for review
July 21, 2026 23:55
…e reset) Pairs with rocksdb-js "align ERR_TRY_AGAIN with the IsBusy reset path". #1696 worked around a stranded-snapshot ERR_TRY_AGAIN by replaying the writes onto a *fresh* RocksTransaction, because the native layer left the stranded snapshot in place so recommitting the same transaction spun forever. That fix also leaned on rocksdb-js publishing the change-feed entry on the failed commit (the fresh replay carries isRetry and never re-stages it). rocksdb-js now resets the transaction onto a fresh snapshot on a failed TryAgain commit — exactly as it always did for IsBusy — and defers the log publish until a real commit. So the fresh-transaction replay is both unnecessary and wrong: a fresh transaction with isRetry would never publish the now-unpublished entry, re-losing the change-feed entry (#1695). Recommit the SAME transaction instead, like ERR_BUSY: its committedPosition survives the reset (WAL write-once) and its onCommit hook stays attached, so the staged entry publishes exactly once, only when the retry commits. Updates the regression test to assert the retry reuses the same transaction id (reset in place) rather than running on a fresh one; the real compact()-induced ERR_TRY_AGAIN, commutative-increment, and co-batched-duplicate cases all still converge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The RETRY_NOW exhaustion path threw ServerError without releasing the native transaction, leaving the handle — and, for a logged transaction, its unpublished transaction-log position pinning the committed-read watermark — lingering until GC. The parallel ERR_BUSY/ERR_TRY_AGAIN rejection give-up already aborts; mirror it here. Pre-existing (not introduced by the TryAgain alignment), surfaced by cross-model review of this function. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The two coordinated-retry give-up paths (RETRY_NOW exhaustion and the ERR_BUSY/ERR_TRY_AGAIN rejection fallback) aborted only the head's native transaction, leaving every this.next link holding its native handle and read snapshot until GC. Mirror abortDueToTimeout's two-pass approach via a shared abortChainAfterRetries helper: poison every link (open = CLOSED) first, then abort each link's native transaction and run DatabaseTransaction-level cleanup, so a throw while aborting one link can't strand the rest. Clearing each link's native handle + read-snapshot bookkeeping before the per-link abort() prevents a double native abort (which throws) and abort()'s doneReadTxn loop from spinning on a nulled handle. The head's blobs are now released too (the old path skipped them). Co-Authored-By: Claude Opus <noreply@anthropic.com>
The two-pass design detaches/natively-aborts each link before calling its DatabaseTransaction-level abort(), specifically so one link's failure can't strand the rest. But that wrapper abort() call was itself unguarded: it synchronously walks savedBlobs and calls write.store.getEntry(), which can throw (closed store, decode error). A throw there exited the loop early, leaving every later link's native handle/read snapshot un-aborted and still tracked despite already being detached from the DatabaseTransaction object — exactly the partial-chain leak the two-pass comment says it prevents. Catch and log per link, mirroring abortDueToTimeout()'s established pattern, so cleanup always completes the full chain. Adds a focused unit test that forces one link's cleanup to throw and asserts later links are still detached, untracked, and natively aborted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
kriszyp
force-pushed
the
kris/tryagain-align
branch
from
July 22, 2026 03:26
c94a952 to
e0e78f0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pairs with HarperFast/rocksdb-js#710. rocksdb-js 2.5.0 is now published; rebased onto latest
main(which had independently picked up the same dependency bump) and verified against the real native build — see Validation below.Problem
#1696 fixed the #1695 source-apply
ERR_TRY_AGAINspin by replaying the writes onto a freshRocksTransaction, because the native layer left the stranded snapshot in place so recommitting the same transaction never converged. But that workaround leaned on a rocksdb-js defect: the transaction-log publish gate (!IsBusy) published the change-feed entry on the failed commit, and the fresh replay (markedisRetry) relied on that premature publish to keep the entry from being lost — an entry visible ahead of its data for every reader in the window, and a permanent phantom if the retry was ultimately abandoned.Fix
rocksdb-js#710 makes the native layer reset the transaction onto a fresh snapshot on a failed
TryAgaincommit — exactly as it always did forIsBusy— and publish log entries only on a real commit. So the fresh-transaction replay becomes both unnecessary and wrong (a fresh transaction withisRetrywould never publish the now-unpublished entry, re-manifesting #1695). This PR:ERR_TRY_AGAIN, likeERR_BUSYalways did. ItscommittedPositionsurvives the native reset (WAL write-once, rocksdb-js#668) and itsonCommithook stays attached, so the staged change-feed entry publishes exactly once, only when the retry really commits.RETRY_NOWexhaustion path threw without releasing the handle, leaving an unpublished log position pinning the committed-read watermark until GC; the rejection-path give-up already aborted).abortChainAfterRetries()helper mirroringabortDueToTimeout()'s two-pass pattern — and guards each link's wrapper cleanup (txn.abort(), which can throw fromsavedBlobs/getEntry()) so one link's failure can't strand later links' native handles/read snapshots.@harperfast/rocksdb-jsto^2.5.0— the paired-fix floor.The
Table.tsappendedAuditEntryguards from #1696 are deliberately kept: they already govern theERR_BUSYsame-transaction recommit, and the scenario they defend against is less reachable now (failed attempts never publish). Whether the TryAgain-specific rationale is now dead code deserves its own analysis — follow-up issue to come.Validation
Against the real, published rocksdb-js 2.5.0 build:
unitTests/resources/sourceApplyConflictRetry.test.js— including the realcompact()-inducedERR_TRY_AGAINrepro, with the assertion inverted from fresh-transaction-id to same-transaction-id — plus a new focused unit test forcing one chain link's wrapper cleanup to throw and asserting later links are still detached/untracked/natively aborted. 5/5 passing.npm run test:unit:resources: 1249/1249 passing.npm ci,format:check,lint:required: clean.Unit Testhad one unrelated flaky failure (caching.test.js, timing-based) that passed on rerun. The advisory, non-blockingNext.js Integration Tests (downstream)check fails on a pre-existingmaintype error (unrelated to this diff, already being fixed on a separate branch).Review history
Two review passes, both addressed:
Cross-model review (Codex ×2 + Harper-domain adjudication; Gemini leg failed — agy hang):
^2.5.0floor.appendedAuditEntrydouble-apply scenario requires a failed attempt's entry to be visible during retry — exactly what the new publish gate removes.Live discussion review (Codex + Harper domain pass) after 2.5.0 published:
main's independent bump of the same dependency to restore a clean merge (which had also silently stalledpull_request-triggered CI).abortChainAfterRetries()'s per-link wrappertxn.abort()was unguarded — a throw on an early link (fromsavedBlobs/getEntry()) exited the cleanup loop before later chain links were touched, contradicting the two-pass design's own guarantee (also independently flagged by Gemini earlier in review — see inline thread). Fixed by guarding that call too, with a regression test.KrAIs, via Claude
🤖 Generated with Claude Code