Repository navigation
fix(crdt): redeliver rows a late-stamped push changes, and pull counters per node - #36
Merged
Merged
Conversation
…ll counters per node A shadow row now has two clocks. The state's own clock, stored in crdt_state, is what merges and LWW compare and what a pull hands out as the change HLC. The hlc_ts and hlc_counter columns are the cursor position pulls page by. Every writer so far kept them equal, and rows written that way read back exactly as before, so nothing moves until a writer asks for a different position through WriteFieldStateAt or WriteTombstoneAt. Tombstone rows store their delete clock in crdt_state as well, and ReadState keeps the latest delete clock across nodes, not whichever row it happened to read last. A counter row is pulled as one change per node it holds. The server merges an increment into whichever node's row has the newer clock, so a pull that only sent the row's own node dropped the other node's totals. That is why the Dart convergence test left device B at 7.
…ps reach every peer A device that was offline pushes changes stamped hours ago. The server merged them, but the row kept the old clock as its cursor position, so a peer that had already pulled past that clock never got the merged state. Now a push that changes a row's stored state moves the row to a fresh cursor position: past the plugin clock, past every position this process handed out, past the highest one in the table, and past the row's own clock. Only the position moves. The change keeps its own clock on the wire, so you get the record you would have got by pulling earlier, and every client picks the winner it would have picked then. An LWW value that loses writes nothing. Neither does an older tombstone or a retried push. Pulls and streams resume from cursor positions, including the multi-table window from #35. Allocation and the write share one lock per plugin, which also stops two concurrent pushes to one field from losing each other's merge.
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.
This one stacks on #35 (
fix/crdt-sync-defects), because both change the push path incrdt/server.go. Merge #35 first, then retarget this tomain.It fixes edits that never reach other devices. Say device A goes offline, bumps a counter, and comes back a day later. The server merges A's change fine, but B already pulled past A's old timestamp, so B never sees the new total. In the Dart convergence test B sat at 7 when it should have been 10. With this branch it reaches 10.
What was wrong
There were two bugs behind it.
The pull cursor pages by each shadow row's stored HLC. When a push merges a late-stamped change into a row, the state changes but the row kept its older HLC. Any peer whose cursor was already past that HLC skipped the row for good. This hits counters, sets, lists, text and documents. LWW mostly escaped, because an older write loses anyway.
Counters had a second problem. A counter row merged from another node was pulled back as that row's own node delta only, so a peer pulling from scratch still undercounted.
What changed
A shadow row now keeps its own clock (in
crdt_state, which is what pulls deliver) apart from its cursor position (hlc_ts,hlc_counter). When a push changes a row's stored state, the server restamps the cursor with a fresh server-clock HLC, so every earlier cursor sorts before it and the row gets redelivered. The state's own clock doesn't move, so LWW and tombstone results come out the same on every replica.A push that changes nothing writes nothing, so nothing is restamped. That covers an LWW loser, an older tombstone and a retried push. An LWW winner is restamped but keeps its own clock and node. Counters are pulled as one record per node, so a peer always gets every node's delta. The wire shape is the same, there are just more records for a busy counter.
Running it
go test -race ./crdt/...There are nine new tests covering set, list, text, document and counter late stamps, an LWW loser and winner, tombstones, cursor monotonicity, the multi-table window from #35, and the stream. The ones that check the fix fail on the base commit. The Dart client in #34 also passes its full conformance suite against this branch, with the skipped late-stamp test turned on.
Things you should know before merging
HandlePushwould deadlock.Syncerand localAfterMutationwrites still put the state's clock in the cursor columns. A node that runs a Syncer and also serves clients still has the bug on that path. That's a follow-up.