-
Notifications
You must be signed in to change notification settings - Fork 549
[release/2.22] fix(merge-tree): Correctly bookkeep insert into local-only obliterate #23937
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[release/2.22] fix(merge-tree): Correctly bookkeep insert into local-only obliterate #23937
Conversation
…microsoft#23934) ## Description Resolves some issues in obliterate's insert codepath related to unacked obliterates over the range being inserted into. The gist of the change is that we did not properly bookkeep a scenario where: - The local client has an outstanding obliterate for a range - Multiple other clients also obliterated this range and attempted to insert into it In this case, the local client knows that eventually, the remote clients won't have won the rights to insert into that range (since the local obliterate will remove the segment). However, remote clients won't know this until the local client's obliterate is acked. Therefore if remote clients make subsequent changes without seeing the local client's obliterate, the local client may interpret them incorrectly. The fix here is to fix the bookkeeping of the inserted segment on the local client in the case above, which can be done by being a bit more thorough on examining any overlapping obliterates (specifically, tracking the newest acked obliterate and oldest unacked obliterate separately from the newest overall). See unit tests for more details. [AB#31009](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/31009) --------- Co-authored-by: Abram Sanderson <absander@microsoft.com>
Warning WARNING: This PR is targeting a release branch! All changes must first be merged into Changes to release branches require approval from the Patch Triage group before merging. For more details, see our internal documentation for the patch policy and processes for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR fixes the bookkeeping in the obliterate insert codepath so that the local client correctly handles overlapping obliterates, ensuring that remote changes are processed properly.
- Introduces tracking of newest acknowledged and oldest unacknowledged obliterates.
- Updates moveInfo construction to correctly select between acknowledged and unacknowledged obliterate metadata.
Reviewed Changes
File | Description |
---|---|
packages/dds/merge-tree/src/test/obliterate.concurrent.spec.ts | Added additional tests to pinpoint regression scenarios involving overlapping obliterates and subsequent insert operations. |
packages/dds/merge-tree/src/mergeTree.ts | Enhanced bookkeeping logic by recording and using newestAcked and oldestUnacked obliterate info during insert processing. |
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
packages/dds/merge-tree/src/mergeTree.ts:1671
- The assignment of moveInfo uses oldestUnacked with optional chaining when setting localMovedSeq, whereas later in the else branch an assertion guarantees its existence. Consider adding an explicit check or removing optional chaining for consistency and clarity.
let moveInfo: IMoveInfo;
packages/dds/merge-tree/src/mergeTree.ts:1701
- The assertion uses optional chaining on oldestUnacked which may hide an unexpected undefined state. Consider ensuring that oldestUnacked is defined before this branch is reached, to make error reporting more explicit.
assert(oldestUnacked?.segmentGroup !== undefined,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
Resolves some issues in obliterate's insert codepath related to unacked obliterates over the range being inserted into. The gist of the change is that we did not properly bookkeep a scenario where:
In this case, the local client knows that eventually, the remote clients won't have won the rights to insert into that range (since the local obliterate will remove the segment). However, remote clients won't know this until the local client's obliterate is acked. Therefore if remote clients make subsequent changes without seeing the local client's obliterate, the local client may interpret them incorrectly.
The fix here is to fix the bookkeeping of the inserted segment on the local client in the case above, which can be done by being a bit more thorough on examining any overlapping obliterates (specifically, tracking the newest acked obliterate and oldest unacked obliterate separately from the newest overall). See unit tests for more details.
AB#31009
Cherry-pick of #23934 into 2.22 release branch.