GO-7357 Fix state-apply correctness bugs in editor object sync - #3195
Open
requilence wants to merge 4 commits into
Open
GO-7357 Fix state-apply correctness bugs in editor object sync#3195requilence wants to merge 4 commits into
requilence wants to merge 4 commits into
Conversation
A move whose target was concurrently moved inside one of the moved blocks created a cycle detached from the root, and apply() garbage-collected both subtrees on every device. Reject such moves so the previously applied concurrent move wins deterministically. A move whose target was concurrently removed left the moved blocks unlinked, so they were garbage-collected as unreachable. Reattach them to the end of the root instead of losing them.
When the derived state had no own details, normalization mutated the parent (committed) state's details in place, bypassing copy-on-write. Read through Details() and write through SetDetail instead, so the committed state stays untouched and the normalized values participate in the regular diff.
…snapshot treeSource.Update captured prevSnapshot after overwriting lastSnapshotId, so the reset branch was unreachable and the counter only accumulated, inflating snapshot probability and with it the rate of conflicting concurrent snapshots. The root-change proxy could not work anyway: in append mode the root does not change in the same update that delivers a snapshot change. BuildState now reports whether a snapshot change was applied, and Update restarts the counter from it.
NewUnmarshalTreeChange kept the snapshot only for the first converted change. A snapshot change arriving in an append batch after another new change was unmarshalled without its snapshot; objecttree caches the result as Change.Model and discards the raw data, so when an in-memory tree reduce later made that change the root, BuildState failed with a nil snapshot and the object stayed stale until reopened. Keep the snapshot for changes flagged IsSnapshot. Also log Update/Rebuild listener failures at error level: they leave the live state behind the tree heads, and at debug level frozen objects were invisible.
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
Four confirmed-by-test correctness fixes in the smartblock state-build / change-apply pipeline, found during an offline-collaboration review of editor objects. Each has a test that fails before the fix and passes after. Linear: GO-7357.
Fixes
Prevent block loss on conflicting concurrent moves (
state/change.go)Cross-moves (device A:
x→undery, device B:y→underx) created a cycle detached from the root;apply()then garbage-collected both subtrees on every device. Move-into-a-concurrently-deleted-target lost the moved subtree the same way. Now a move whose target sits inside the moved blocks is rejected (the earlier concurrent move wins deterministically), and blocks whose target was removed are reattached to the root instead of being GC'd.Keep copy-on-write in
normalizeRecommendedRelations(state/normalize.go)When the derived state had no own details, normalization mutated the committed parent state's details in place, bypassing copy-on-write. Now reads via
Details()and writes viaSetDetailonly when values actually change.Reset
changesSinceSnapshotwhen an appended batch contains a snapshot (sourceimpl/source.go)treeSource.UpdatecapturedprevSnapshotafter overwritinglastSnapshotId, so the reset branch was dead code and the counter only accumulated — inflating snapshot probability and, with it, the rate of conflicting concurrent snapshots.BuildStatenow reports whether a snapshot change was applied, andUpdaterestarts the counter from it.Keep snapshot data when unmarshalling snapshot changes (
sourceimpl/source.go)NewUnmarshalTreeChangekept the snapshot only for the first converted change.objecttreecaches the result asChange.Modeland discards the raw data, so a snapshot change converted second lost its snapshot; a later in-memory tree reduce that made it the root leftBuildStatewith a nil snapshot and the object stayed stale until reopened. Now the snapshot is kept for changes flaggedIsSnapshot. Also raisedUpdate/Rebuildlistener failures from debug to error level so frozen objects are visible.Testing
go test ./core/block/editor/state/ ./core/block/source/sourceimpl/— greenTestState_ApplyChange_MoveConflicts,TestNormalizeRecommendedRelationsDoesNotMutateParent,TestTreeSource_Update_ChangesSinceSnapshot,TestNewUnmarshalTreeChange_KeepsSnapshotForSnapshotChangesNotes
These fixes change replay semantics for conflict cases; mixed old/new client versions will disagree on the outcome until the next snapshot bakes one side in — inherent to any replay fix. Broader design-level findings from the review (receive-path normalization divergence, version flip-flop via snapshots, whole-value LWW on list relations) are intentionally out of scope here.