[release/2.22] fix(merge-tree): Correctly bookkeep insert into local-only obliterate (#23937)

## 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)

Cherry-pick of #23934 into 2.22 release branch.

Co-authored-by: Abram Sanderson <[email protected]>

Author: Abe27342

packages/dds/merge-tree/src/mergeTree.ts

@@ -1611,6 +1611,8 @@ export class MergeTree {
 			let normalizedNewestSeq: number = 0;
 			const movedClientIds: number[] = [];
 			const movedSeqs: number[] = [];
+			let newestAcked: ObliterateInfo | undefined;
+			let oldestUnacked: ObliterateInfo | undefined;
 			for (const ob of this.obliterates.findOverlapping(newSegment)) {
 				// compute a normalized seq that takes into account local seqs
 				// but is still comparable to remote seqs to keep the checks below easy
@@ -1641,6 +1643,23 @@ export class MergeTree {
 						normalizedNewestSeq = normalizedObSeq;
 						newest = ob;
 					}
+
+					if (
+						ob.seq !== UnassignedSequenceNumber &&
+						(newestAcked === undefined || newestAcked.seq < ob.seq)
+					) {
+						newestAcked = ob;
+					}
+
+					if (
+						ob.seq === UnassignedSequenceNumber &&
+						(oldestUnacked === undefined || oldestUnacked.localSeq! > ob.localSeq!)
+					) {
+						// There can be one local obliterate surrounding a segment if a client repeatedly obliterates
+						// a region (ex: in the text ABCDEFG, obliterate D, then obliterate CE, then BF). In this case,
+						// the first one that's applied will be the one that actually removes the segment.
+						oldestUnacked = ob;
+					}
 				}
 			}
 
@@ -1649,23 +1668,41 @@ export class MergeTree {
 			// by the same client that's inserting this segment, we let them insert into this range and therefore don't
 			// mark it obliterated.
 			if (oldest && newest?.clientId !== clientId) {
-				const moveInfo: IMoveInfo = {
-					movedClientIds,
-					movedSeq: oldest.seq,
-					movedSeqs,
-					localMovedSeq: oldest.localSeq,
-					wasMovedOnInsert: oldest.seq !== UnassignedSequenceNumber,
-				};
+				let moveInfo: IMoveInfo;
+				if (newestAcked === newest || newestAcked?.clientId !== clientId) {
+					moveInfo = {
+						movedClientIds,
+						movedSeq: oldest.seq,
+						movedSeqs,
+						localMovedSeq: oldestUnacked?.localSeq,
+						wasMovedOnInsert: oldest.seq !== UnassignedSequenceNumber,
+					};
+				} else {
+					assert(
+						oldestUnacked !== undefined,
+						"Expected local obliterate to be defined if newestAcked is not equal to newest",
+					);
+					// There's a pending local obliterate for this range, so it will be marked as obliterated by us. However,
+					// all other clients are under the impression that the most recent acked obliterate won the right to insert
+					// in this range.
+					moveInfo = {
+						movedClientIds: [oldestUnacked.clientId],
+						movedSeq: oldestUnacked.seq,
+						movedSeqs: [oldestUnacked.seq],
+						localMovedSeq: oldestUnacked.localSeq,
+						wasMovedOnInsert: false,
+					};
+				}
 
 				overwriteInfo(newSegment, moveInfo);
 
 				if (moveInfo.localMovedSeq !== undefined) {
 					assert(
-						oldest.segmentGroup !== undefined,
+						oldestUnacked?.segmentGroup !== undefined,
 						0x86c /* expected segment group to exist */,
 					);
 
-					this.addToPendingList(newSegment, oldest.segmentGroup);
+					this.addToPendingList(newSegment, oldestUnacked?.segmentGroup);
 				}
 
 				if (newSegment.parent) {

packages/dds/merge-tree/src/test/obliterate.concurrent.spec.ts

@@ -1911,6 +1911,86 @@ for (const incremental of [true, false]) {
 
 				helper.logger.validate({ baseText: "8m" });
 			});
+
+			// See 'Local obliterate wins post-insertion of segment previously thought to have won' (below) for a simpler
+			// to understand version of this test. This test is the original partial synchronization fuzz variant which
+			// demonstrated that issue, and has been preserved for now in case it catches additional related problems.
+			// Once fuzz testing more meaninfully leverages ops being sent to different clients at different types (partial
+			// synchronization), it's probably fine to remove this.
+			it("fuzz regression: Local obliterate wins post-insertion of segment previously thought to have won", () => {
+				const helper = new PartialSyncTestHelper();
+
+				helper.insertText("A", 0, "Hx15J");
+				helper.processAllOps();
+				helper.insertText("A", 0, "9T");
+				helper.insertText("B", 0, "c8v");
+				helper.advanceClients("A", "C");
+				helper.removeRange("A", 2, 5);
+				helper.removeRange("A", 0, 1);
+				helper.obliterateRange("A", 0, 3);
+				helper.obliterateRange(
+					"C",
+					{ pos: 1, side: Side.After },
+					{ pos: 4, side: Side.Before },
+				);
+				helper.insertText("B", 0, "4qpo");
+				helper.insertText("C", 2, "fP");
+				helper.insertText("A", 0, "hn");
+				helper.advanceClients("A", "C");
+				helper.obliterateRange(
+					"A",
+					{ pos: 5, side: Side.After },
+					{ pos: 9, side: Side.After },
+				);
+				helper.obliterateRange(
+					"B",
+					{ pos: 3, side: Side.Before },
+					{ pos: 9, side: Side.After },
+				);
+				// At the time of the original bug, this would hit 0xa3f.
+				helper.processAllOps();
+
+				helper.logger.validate({ baseText: "hn4qpJ" });
+			});
+
+			// Simpler version of the above test which has the same root cause but reproduces a slightly different failure mode.
+			it("Local obliterate wins post-insertion of segment previously thought to have won", () => {
+				const helper = new PartialSyncTestHelper();
+				helper.insertText("A", 0, "1xx2");
+				helper.processAllOps();
+				// A and B both obliterate the 'xx' segment with an expanding obliterate, then try to insert
+				// into the gap that it leaves.
+				helper.obliterateRange(
+					"A",
+					{ pos: 0, side: Side.After },
+					{ pos: 3, side: Side.Before },
+				);
+				helper.insertText("A", 1, "aaaa");
+				helper.obliterateRange(
+					"B",
+					{ pos: 0, side: Side.After },
+					{ pos: 3, side: Side.Before },
+				);
+				helper.insertText("B", 1, "bbb");
+				helper.advanceClients("A", "B");
+				// Meanwhile, C attempts the same thing without seeing A or B's obliterate & insertions
+				helper.obliterateRange(
+					"C",
+					{ pos: 0, side: Side.After },
+					{ pos: 3, side: Side.Before },
+				);
+				helper.insertText("C", 1, "ccc");
+				// Before seeing C's ops, B attempts to insert more content. Since B hasn't yet seen C's obliterate,
+				// A and B are under the impression that B's obliterate has won and the string contents are '1bbb2'.
+				helper.insertText("B", 5, "B");
+				helper.insertText("A", 5, "A");
+				helper.processAllOps();
+				// By now, all clients realize C actually won the obliterate and should have additionally applied B's
+				// op correctly as it was outside of the obliterated range.
+				// At the time this test was written, client C had trouble recognizing that A and B think that B has won
+				// and merged incorrectly, hitting 'MergeTree insert failed'.
+				helper.logger.validate({ baseText: "1ccc2AB" });
+			});
 		});
 	});
 }