fix(replication): prevent out-of-order full update from reverting newer record - #1172
Merged
Conversation
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
|
Reviewed; no blockers found. |
kriszyp
marked this pull request as ready for review
June 9, 2026 03:49
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
kriszyp
force-pushed
the
kris/crdt-ooo-revert
branch
from
June 9, 2026 23:00
2f5eecf to
e534eca
Compare
…r record In the out-of-order resequencing path (precedesExisting <= 0), when audit is on and the audit walk finds no succeeding updates to resequence around, a full update fell through to the shared commit and stored the older record over the newer one (recordToStore = recordUpdate), leaving the cluster non-convergent. Skip the losing full update instead — the existing record is newer, so this older update is superseded. Uses a bare return (no writeCommit) to match the superseded-by-newer-put branch above, so no audit record is written referencing this losing update's pre-saved blobs. Reproduced via deployment-tracking: a burst of same-key full puts commits out of order on a loaded peer and the row gets stuck at a non-terminal status. Fixes #1170 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
kriszyp
force-pushed
the
kris/crdt-ooo-revert
branch
from
June 9, 2026 23:21
e534eca to
b8d9558
Compare
cb1kenobi
approved these changes
Jun 10, 2026
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.
Summary
In the out-of-order resequencing path in
resources/Table.ts(precedesExisting <= 0), when audit is enabled and the audit walk finds no succeeding updates to resequence around, a full update fell through to the shared commit line and stored the older record over the newer one (recordToStore = recordUpdate) — leaving the cluster permanently non-convergent.This skips the losing full update instead: the existing record is newer, so the older update is superseded. It uses a bare
return(nowriteCommit) to match the existing superseded-by-newer-putbranch a few lines above, so no audit record is written referencing this losing update's pre-saved blobs.Purpose
Fixes #1170. Reproduced via deployment tracking: a burst of same-key full puts replicates to a loaded peer, commits out of order, and the row gets stuck at a non-terminal status forever (origin + other peers converge; one peer does not). Surfaced as the intermittent harper-pro
deployTrackingReplication.test.mjsfailure onmain.Where to look — please scrutinize
This is a hot, critical replication path; it deserves careful core review:
write.skipped = true+writeCommit(false)with audit on would write an audit entry referencing the losing update's blobs and then clean them up (orphaning them). The fix avoids this by using a barereturn— matching the existingput/deletesupersede branch at the top of this block. Please confirm this is the correct discard path and that no blob/audit bookkeeping is skipped that the existing supersede branch relies on.fullUpdate && !incrementalUpdateToApplyat the end of the audit branch — i.e. an out-of-order full update with no reconstructable succeeding updates. Worth confirming there is no legitimate case where we should still write here.Open items for the reviewer
resources/replication suites locally (no native DB build in this environment). Guidance on the right harness (or a follow-up commit) welcome.Cross-model review: Codex (raised the blob concern above; addressed).
🤖 Generated by Claude (Opus 4.x) per the Harper engineering DLC.