fix(txn): align ERR_TRY_AGAIN with the IsBusy reset path — reset in place, publish only on real commit#710
Open
kriszyp wants to merge 5 commits into
Open
fix(txn): align ERR_TRY_AGAIN with the IsBusy reset path — reset in place, publish only on real commit#710kriszyp wants to merge 5 commits into
kriszyp wants to merge 5 commits into
Conversation
An optimistic commit returns TryAgain (not Busy) when the transaction's snapshot is stranded outside the memtable window — the sequence history the conflict check needs was flushed. Two problems followed: 1. commitFinished (which advances the committed-read watermark, publishing the log/change-feed entries) was gated on !IsBusy. TryAgain is not Busy, so a *failed* TryAgain commit published its entries while the record was rolled back — a change-feed entry ahead of its data, and a permanent phantom if the retry was ultimately abandoned (commitAborted cannot pull the watermark back past an already-advanced position). 2. TryAgain was not retried at all: resetTransaction fired only on IsBusy, so recommitting re-checked the same stranded snapshot forever (harper#1695). Treat TryAgain like Busy: publish only on a real commit (gate on status.ok()), and resetTransaction on IsBusy || IsTryAgain. The reset preserves committedPosition (WAL stays write-once, #668) but takes a fresh snapshot, so the caller's re-run validates against current state and converges. TS adds TransactionTryAgainError and wires it into both db.transaction() retry loops. Regression test drives the exact path via a gated native test seam (forceTryAgainForTesting; inert in production) since a real TryAgain is finicky to stage through the public API (OCC retains flushed-memtable history). It asserts convergence on the *same* transaction, WAL written once, and — the discriminating check — that the entry is not visible mid-retry (fails under the old !IsBusy gate with "expected 1 to be 0"). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both codes now mean the same thing to callers — the native layer reset the transaction in place (fresh snapshot, committedPosition preserved) and the body should be re-run — so express that contract as a common base class and collapse the two-clause retry predicates to one instanceof check. TransactionAbandonedError shares the field shape but deliberately does not extend the base: retryability is the boundary the hierarchy expresses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t does not unwrite bytes Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2.5.0 is the paired-fix floor for HarperFast/harper's ERR_TRY_AGAIN same-transaction retry (harper must require >=2.5.0 so an old native that neither resets on TryAgain nor defers the failed-commit publish can never be installed underneath it). The RETRY_NOW comment records that TryAgain is deliberately excluded from coordinated-retry signalling: it has no conflicting lock holder to park on, so it takes the reject→reset→retry path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
Paired harper PR: HarperFast/harper#1823 (fix(txn): retry ERR_TRY_AGAIN on the same transaction) — draft until this releases as 2.5.0. KrAIs, via Claude |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for retrying transactions that fail with a TryAgain status (due to stranded snapshots) similarly to how IsBusy failures are handled. It refactors error classes by introducing a common TransactionRetryableError base class, updates the native transaction commit paths to reset and retry on TryAgain, ensures log entries are only published upon successful commits, and adds a deterministic test seam along with a regression test. The reviewer suggested checking the return status of txnHandle->txn->Rollback() in the test seam block to prevent ignoring potential severe errors during rollback.
Contributor
📊 Benchmark Resultsget-sync.bench.tsgetSync() > random keys - small key size (100 records)
getSync() > sequential keys - small key size (100 records)
ranges.bench.tsgetRange() > small range (100 records, 50 range)
realistic-load.bench.tsRealistic write load with workers > write variable records with transaction log
transaction-log.bench.tsTransaction log > read 100 iterators while write log with 100 byte records
Transaction log > read one entry from random position from log with 1000 100 byte records
worker-put-sync.bench.tsputSync() > random keys - small key size (100 records, 10 workers)
worker-transaction-log.bench.tsTransaction log with workers > write log with 100 byte records
Results from commit c1ba4d4 |
… write-once assertion Windows-only CI failure across every runner (Node 22/24/26, Bun, Deno): `expected 16777216 to be 154`. Root cause is in the new test, not the fix — the mid-transaction discriminating query (asserting the entry isn't visible yet on attempt 2) maps the still-active log file, which on Windows pre-extends the on-disk file to its configured max size via SetEndOfFile (only ever shrunk back by a reopen recovery scan, never on close — see transaction_log_file_windows.cpp). The existing #668 test never queries the active file before its own stat-based check, so it never trips this. Swap the raw filesystem byte-count assertion for getStats().totals.entriesWritten, which counts entries actually written by writeBatch() (skipped entirely by the write-once retry no-op) — a logical, platform-independent counter unaffected by Windows' file pre-extension. Also addresses the Gemini PR review comment: check Rollback()'s return status in the test seam rather than ignoring it — a failed rollback (e.g. underlying corruption) is a real error and should surface, not be masked behind the simulated TryAgain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
heskew
added
claude-review
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
2 participants
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 the HarperFast/harper PR "retry ERR_TRY_AGAIN on the same transaction" (linked in first comment) — the two must land together; this side releases first as 2.5.0, which is harper's new dependency floor.
Problem
An optimistic commit returns
TryAgain(notBusy) when the transaction's snapshot is stranded outside the memtable window — the sequence history the conflict check needs was flushed away (bulk-ingest bursts, compaction). Two defects followed:commitFinished— which advances the committed-read watermark, making log/change-feed entries visible — was gated on!IsBusy().TryAgainis notBusy, so a failed TryAgain commit published its entries while the data was rolled back: a change-feed entry visible ahead of its data for every reader in that window.resetTransaction()fired only onIsBusy, so recommitting afterTryAgainre-checked the same stranded snapshot forever. harper#1695's source-apply wedge (uncapped retry spinning, replication apply loop frozen) is the field manifestation; harper#1696 worked around it in the JS layer by replaying onto a fresh transaction — which depended on the premature publish of defect 1.Fix
Treat
TryAgainexactly likeBusy:commitFinishedis now gated onstatus.ok()— entries publish only when the data commit actually succeeds. On a hard error,close()'scommitAbortedstops the position pinning the watermark (bytes cannot be unwritten; abandoning a logged transaction remains the loudly-flaggedERR_TRANSACTION_ABANDONEDcase, unchanged).resetTransaction()fires onIsBusy() || IsTryAgain()— preservescommittedPosition(WAL stays write-once, Data loss: IsBusy commit retry orphans an uncommitted log position, pinning the committed watermark → committed reads truncate #668) but takes a fresh snapshot, so the caller's re-run validates against current state and converges.TransactionRetryableErrorbase (shared byTransactionIsBusyError/ newTransactionTryAgainError) expressing the contract: the native layer reset the handle; re-run the body and retry. Bothdb.transaction()retry loops now use oneinstanceofcheck.TransactionAbandonedErrordeliberately does not extend the base — retryability is the boundary.RETRY_NOWfast path staysIsBusy-only by design: it parks on the conflicting holder's VT-slot lock, and a stranded snapshot has no lock holder to park on (documented in-code).Validation
test/transaction-log-tryagain-retry.test.ts: converges on the same transaction id, WAL written exactly once, and — the discriminating assertion — the entry is not visible mid-retry (fails under the old!IsBusygate withexpected 1 to be +0).TryAgainis injected via a gated native test seam (forceTryAgainForTesting, inert unless called) because a real stranded snapshot can't be staged deterministically through the public API (OCC retains flushed-memtable history). The real repro lives in the paired harper PR: itscompact()-inducedERR_TRY_AGAINtest converges on the same transaction with this build linked.Cross-model review (Codex ×2 + Harper-domain adjudication; Gemini leg failed — agy hang)
^2.5.0and stays draft until publish.transactionSyncfresh-Transaction-per-attempt lifecycle, worker-threadresetTransactionenvelope, and operation-vs-state log semantics are all the pre-existingIsBusy/Data loss: IsBusy commit retry orphans an uncommitted log position, pinning the committed watermark → committed reads truncate #668 design thatTryAgainnow joins — no new hazard; the publish-gate change is strictly safer.transactionLogMapCount).attempt < maxRetriesvs<=) — present forIsBusybefore this change; aligning it changes user-visible retry counts, so it deserves its own decision.KrAIs, via Claude
🤖 Generated with Claude Code