Skip to content

[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

Conversation

Abe27342
Copy link
Contributor

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

Cherry-pick of #23934 into 2.22 release branch.

…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>
@Copilot Copilot AI review requested due to automatic review settings February 26, 2025 23:43
@Abe27342 Abe27342 requested a review from a team as a code owner February 26, 2025 23:43
@github-actions github-actions bot added area: dds Issues related to distributed data structures base: release PRs targeted against a release branch and removed area: dds Issues related to distributed data structures labels Feb 26, 2025
Copy link
Contributor

Warning

WARNING: This PR is targeting a release branch!

All changes must first be merged into main and then backported to the target release branch.
Please include a link to the main PR in the description of this PR.

Changes to release branches require approval from the Patch Triage group before merging.
You should have already discussed this change with them so they know to expect it.

For more details, see our internal documentation for the patch policy and processes for
patch releases.

Copy link
Contributor

@Copilot Copilot AI left a 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,

@anthony-murphy anthony-murphy requested a review from a team February 27, 2025 01:02
Copy link

@frankmueller-msft frankmueller-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@Abe27342 Abe27342 merged commit 636bd77 into microsoft:release/client/2.22 Feb 27, 2025
39 checks passed
@Abe27342 Abe27342 deleted the cherry-pick-last-obliterate-fix branch February 27, 2025 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
base: release PRs targeted against a release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants