Summary
TransactionContext.rebaseEdgeAppends resolves a commit-time page conflict by dropping this transaction's whole modified page and replaying only the tracked in-chunk appends on top of the reloaded committed image:
// Drop the stale copies so the reload observes the current committed version of the page.
modifiedPages.remove(pageId);
immutablePages.remove(pageId);
That is sound only if every byte this transaction wrote to that page is accounted for by a tracked append. Nothing verifies it. The invariant is instead maintained by hand, through poisonEdgeAppendPage calls scattered across the writers - currently 9 call sites in LocalDatabase, EdgeLinkedList, StripedEdgeList and (since #5569) LocalBucket. Miss one, and the merge silently commits a page from which this transaction's other writes have vanished. No exception, no log line: the write is simply gone.
This has already bitten three times
Three independent instances of one bug class, each found by a human or a reviewer reading the code rather than by a test. The fourth is a matter of time: the merge is opt-out, so a new writer is rebasable by default and has to remember to exclude itself.
Proposed change
Flip it to opt-in: before accepting a rebase, require the page to prove that its modified byte range is fully explained by the tracked appends.
MutablePage.getModifiedRange() already gives the [from, to) span this transaction dirtied, and the commit path reads it a few lines above the rebase decision. For each tracked segment on the page, the append knows the record's extent (slot-table entry -> record position, plus its on-page size) and the page-header fields it may touch. The union of those extents is the span the merge can legitimately reproduce.
In isRebasableEdgeAppendPage, accept the page only when its modified range is contained in that union. A delete elsewhere on the page, an untracked inline record-table write, a new chunk, a stripe-directory update - all of them widen the modified range past the appends' extents, so the check refuses and the transaction falls back to a normal retry. Conservative in exactly the right direction: the worst outcome of a false negative is a retry that would have happened anyway before the merge existed.
The existing poisonEdgeAppendPage calls stay as the fast, precise path; the containment check is the backstop that makes forgetting one non-fatal.
The same guard is nearly free on the slot merge
rebaseSlots has the identical shape and would benefit from the identical check, comparing the modified range against the union of the tracked slots' extents. It is better covered today - all three hooks live in the central LocalBucket.create/update/deleteRecordInternal, so a new writer has to go out of its way to bypass them - but the check costs one comparison on a path that only runs on a conflict, and it would have caught the multi-page-writer gap above without anyone noticing it by reading.
Tests
- The three historical bugs, as regression tests, with their poison call removed: each must now fall back cleanly instead of dropping the write.
Issue5381FalseConflictTest.multiPageRecordCoLocatedWithTrackedInsertStaysIntact and stripedDirectoryUpdateCoLocatedWithSegmentAppendFallsBack are the templates.
- A synthetic writer that dirties a byte outside every tracked extent: the merge must refuse.
- The existing merge-counter assertions must still show the merge firing on the genuinely-clean workloads, so the containment check is not so tight that it refuses everything.
Why it is worth doing
Every bug in this class is a silent lost write on the leader, which then replicates faithfully to every follower - the failure mode with the worst blast radius and the least chance of being noticed. The check converts it into a retry.
Summary
TransactionContext.rebaseEdgeAppendsresolves a commit-time page conflict by dropping this transaction's whole modified page and replaying only the tracked in-chunk appends on top of the reloaded committed image:That is sound only if every byte this transaction wrote to that page is accounted for by a tracked append. Nothing verifies it. The invariant is instead maintained by hand, through
poisonEdgeAppendPagecalls scattered across the writers - currently 9 call sites inLocalDatabase,EdgeLinkedList,StripedEdgeListand (since #5569)LocalBucket. Miss one, and the merge silently commits a page from which this transaction's other writes have vanished. No exception, no log line: the write is simply gone.This has already bitten three times
writeMultiPageRecord/updateMultiPageRecordwroteNEXT_CHUNKrecords to reused pages through inline record-table writes that bypassedcreate/update/deleteRecordInternal, so they were neither tracked nor poisoned. Fixed by making both writers poison.StripeDirectorywrite made the page look rebasable. Fixed by poisoning on the non-candidate update.poisonEdgeAppendPage(fileId, pageNumber)todeleteRecordInternal.Three independent instances of one bug class, each found by a human or a reviewer reading the code rather than by a test. The fourth is a matter of time: the merge is opt-out, so a new writer is rebasable by default and has to remember to exclude itself.
Proposed change
Flip it to opt-in: before accepting a rebase, require the page to prove that its modified byte range is fully explained by the tracked appends.
MutablePage.getModifiedRange()already gives the[from, to)span this transaction dirtied, and the commit path reads it a few lines above the rebase decision. For each tracked segment on the page, the append knows the record's extent (slot-table entry -> record position, plus its on-page size) and the page-header fields it may touch. The union of those extents is the span the merge can legitimately reproduce.In
isRebasableEdgeAppendPage, accept the page only when its modified range is contained in that union. A delete elsewhere on the page, an untracked inline record-table write, a new chunk, a stripe-directory update - all of them widen the modified range past the appends' extents, so the check refuses and the transaction falls back to a normal retry. Conservative in exactly the right direction: the worst outcome of a false negative is a retry that would have happened anyway before the merge existed.The existing
poisonEdgeAppendPagecalls stay as the fast, precise path; the containment check is the backstop that makes forgetting one non-fatal.The same guard is nearly free on the slot merge
rebaseSlotshas the identical shape and would benefit from the identical check, comparing the modified range against the union of the tracked slots' extents. It is better covered today - all three hooks live in the centralLocalBucket.create/update/deleteRecordInternal, so a new writer has to go out of its way to bypass them - but the check costs one comparison on a path that only runs on a conflict, and it would have caught the multi-page-writer gap above without anyone noticing it by reading.Tests
Issue5381FalseConflictTest.multiPageRecordCoLocatedWithTrackedInsertStaysIntactandstripedDirectoryUpdateCoLocatedWithSegmentAppendFallsBackare the templates.Why it is worth doing
Every bug in this class is a silent lost write on the leader, which then replicates faithfully to every follower - the failure mode with the worst blast radius and the least chance of being noticed. The check converts it into a retry.