Repository navigation
Never drop source-applied writes on conflict; backpressure the apply loop - #1252
Merged
Merged
Conversation
…loop Writes applied from a canonical source of truth (a replication peer or an external caching source) flow through Table.sourcedFrom's subscription loop, which committed them fire-and-forget. Under sustained conflict a commit could exhaust the retry cap and the rejection was swallowed as an unhandled rejection — the write was silently dropped, and because there is no re-subscribe / sequence-id-resume path the node diverged permanently from its peers (harper-pro#348). - Mark source-applied transactions (context.sourceApply); their commits retry transient conflicts (ERR_BUSY / ERR_TRY_AGAIN) without the MAX_RETRIES cap, with a capped backoff. Request-path transactions keep the cap and surface a loud error. - Backpressure the apply loop: await each commit before recording the sequence id / pulling the next event. This removes the fire-and-forget pipelining that creates the conflict storm and guarantees the sequence id never advances past an uncommitted write. - Fold the success-path onCommit result into the commit completions so an async onCommit rejection can't become a silent unhandled rejection either. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Await the prior transaction's commit when a beginTxn closes it, not only at the final end_txn. The internal subscribe path delimits consecutive transactions with beginTxn and emits end_txn only after the last, so without this the sequence id could still advance past earlier uncommitted transactions. (Codex P1) - Pass the CommitResolution through to onCommit (was dropped when switching from .then(event.onCommit) to await), since callbacks may use the committed txn time. (Codex P2) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Reset txnInProgress to undefined (via try/finally) after a transaction completes at end_txn and at a beginTxn that closes a prior txn. Without this: (a) a standalone write after end_txn was appended to the completed transaction and lost, and (b) a non-transient commit rejection left a rejected promise that the next beginTxn re-awaited forever, bricking the apply loop. (agy blockers) - Log an async onCommit rejection instead of folding it into the commit completions, so commit success isn't coupled to the post-commit handler (the write is already durable). (agy) - Type context.sourceApply on the Context interface. (agy) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces backpressure and uncapped retries for replication-applied transactions (sourceApply) to prevent permanent node divergence on transient conflicts. Key changes include capping retry delays, awaiting transaction commits and onCommit callbacks in the subscription apply loop, and clearing transaction states in finally blocks. The review identified critical correctness issues in Table.ts where a failed commit could break transaction atomicity for subsequent writes or incorrectly advance the sequence ID. Additionally, improvements were suggested in DatabaseTransaction.ts to handle synchronous errors in onCommit and add warning logs for infinite retries under sourceApply.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Contributor
|
Reviewed; no blockers found. |
The end_txn handler resets the txnInProgress loop variable once the transaction completes, but the updateRecordedSequenceId closure reads txnInProgress.nodeId to record lastTxnTime for proxied replication. Capture the transaction in a stable local (committingTxn) used by the closure and commit await, so resetting the loop variable no longer breaks per-node sequence metadata. (Codex) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… failure - Track never-drop-on-conflict as a dedicated transaction field (sourceApply), set from the context in transaction() and propagated to chained multi-store transactions created in txnForContext. Without this, a source-applied transaction touching a second store committed the secondary through transaction.next with no sourceApply, so a transient conflict there still threw after MAX_RETRIES and dropped the canonical write. (Codex) - At a beginTxn that closes a prior transaction, catch a non-retryable commit failure locally (log + continue) instead of rethrowing, so the current beginTxn still starts a fresh transaction with correct boundaries rather than having its writes applied as standalone. (Codex) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Wrap the post-commit onCommit call in try/catch so a synchronous throw (not just a rejected promise) is logged rather than failing an already-durable commit. (Gemini) - Log a periodic warning when a source-applied transaction retries past MAX_RETRIES, so an uncapped-retry stall is visible in production where debug logging is off. (Gemini) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kriszyp
marked this pull request as ready for review
June 11, 2026 22:51
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
4 tasks
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
1 participant
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.
Summary
Writes applied from a canonical source of truth — a replication peer or an external caching source — flow through
Table.sourcedFrom's subscription apply loop, which committed them fire-and-forget. Under sustained write conflict a commit could exhaust theMAX_RETRIEScap and its rejection was swallowed as an unhandled rejection, so the write was silently dropped. Because there is no re-subscribe / sequence-id-resume path, the node then diverged permanently from its peers.This makes two coordinated changes:
context.sourceApply); their commits retryERR_BUSY/ERR_TRY_AGAINwithout theMAX_RETRIEScap (with a capped backoff). Request-path transactions are unchanged — they keep the cap and surface a loud error.awaits each commit before recording the sequence id / pulling the next event (atend_txn, at abeginTxnthat closes a prior transaction, and for standalone writes). This removes the fire-and-forget pipelining that creates the conflict storm and guarantees the sequence id never advances past an uncommitted write.Also: an async success-path
onCommitrejection is now logged instead of becoming a silent unhandled rejection.Purpose
The data-safety + throughput halves of HarperFast/harper-pro#348 (bulk full-copy / v4→v5 migration losing records at scale). Companion to #1251 (WBM defaults), which provides the memtable backpressure on the producer side.
Where to look
resources/Table.ts— the apply-loop backpressure awaits and thetxnInProgresslifecycle (reset viatry/finallyafter each transaction completes).resources/DatabaseTransaction.ts— the uncapped-retry-for-sourceApplybranch and theonCommitrejection handling.Reviewer attention (open items / deliberate trade-offs)
MIG_RECORDS=80000 …). This has not been run yet — it should gate the merge. Local fulltransaction.test.jsis pre-existing-flaky (RocksDB-lock/state); relevant suites pass in isolation (caching, subscriptionReplay, transaction merge).Cross-model reviewed (Codex + Gemini/agy) pre-PR; a couple of backpressure/lifecycle bugs they caught are fixed in the commit history.
🤖 Generated with Claude Code — model: Claude Sonnet 4.6
Known limitation
A genuinely non-transient commit failure during source-apply (not a transient conflict — those now retry without a cap) is logged and the loop continues; a later successful transaction's
end_txncan then advance the recorded sequence id past the failed one, so on restart that write is skipped. This is pre-existing behavior and out of scope for #348 (whose scenario is transient conflict, now fully covered). Hardening it (freeze the checkpoint or halt-loud on non-transient apply failure, so a restart re-delivers) is a possible defense-in-depth follow-up that depends on the re-subscribe/restart-resume path. The new periodic warn log surfaces a stalled apply loop in the meantime.