Skip to content

merge-tree: split reset arms + previousProps accessor backed by WeakMap #27414

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.

Sign up for GitHub

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

Draft
anthony-murphy wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

merge-tree: split reset arms + previousProps accessor backed by WeakMap #27414

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0e7f5ab
refactor(merge-tree): extract resetAnnotateOp and resetInsertOp helpe…
anthony-murphy May 27, 2026
8d34c55
feat(merge-tree): add SegmentGroupCollection.previousPropsForSegment …
anthony-murphy May 27, 2026
5c914f9
refactor(merge-tree): use WeakMap for SegmentGroup.previousProps keye…
anthony-murphy May 27, 2026
0c08285
chore(merge-tree): biome format
anthony-murphy May 27, 2026
7ac273f
test(merge-tree): direct coverage for SegmentGroupCollection.previous…
anthony-murphy May 28, 2026
c976b85
fix(merge-tree): build/lint fixes for regression tests
anthony-murphy May 28, 2026
3138872
Merge remote-tracking branch 'upstream/main' into prep-merge-tree
anthony-murphy Jun 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 94 additions & 73 deletions packages/dds/merge-tree/src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -960,6 +960,89 @@ export class Client extends TypedEventEmitter<IClientEvents> {
};
}

private resetAnnotateOp(
resetOp: IMergeTreeAnnotateMsg | IMergeTreeAnnotateAdjustMsg,
segment: ISegmentLeaf,
segmentPosition: number,
): IMergeTreeDeltaOp | undefined {
assert(
segment.propertyManager?.hasPendingProperties(resetOp.props ?? resetOp.adjust) === true,
0x036 /* "Segment has no pending properties" */,
);
// if the segment has been removed or obliterated, there's no need to send the annotate op
// unless the remove was local, in which case the annotate must have come
// before the remove
if (!isRemovedAndAcked(segment)) {
return resetOp.props === undefined
? createAdjustRangeOp(
segmentPosition,
segmentPosition + segment.cachedLength,
resetOp.adjust,
)
: createAnnotateRangeOp(
segmentPosition,
segmentPosition + segment.cachedLength,
resetOp.props,
);
}
return undefined;
}

private resetInsertOp(
resetOp: IMergeTreeInsertMsg,
segment: ISegmentLeaf,
segmentPosition: number,
squash: boolean,
): IMergeTreeDeltaOp | undefined {
if (isInserted(segment) && opstampUtils.isSquashedOp(segment.insert)) {
return undefined;
}
assert(
isInserted(segment) && opstampUtils.isLocal(segment.insert),
0x037 /* "Segment already has assigned sequence number" */,
);
const removeInfo = toRemovalInfo(segment);

const unusedStamp: OperationStamp = { seq: 0, clientId: 0 };
if (removeInfo !== undefined && squash) {
assert(
removeInfo.removes.length === 1 ||
opstampUtils.isAcked(removeInfo.removes[removeInfo.removes.length - 2]),
0xbaf /* Expected only one local remove */,
);
this.squashInsertion(segment);
return undefined;
} else if (removeInfo !== undefined && opstampUtils.isAcked(removeInfo.removes[0])) {
assert(
removeInfo.removes[0].type === "sliceRemove",
0xb5c /* Remove on insertion must be caused by obliterate. */,
);
errorIfOptionNotTrue(this._mergeTree.options, "mergeTreeEnableObliterateReconnect");
// the segment was remotely obliterated, so is considered removed
// we set the seq to the universal seq and remove the local seq,
// so its length is not considered for subsequent local changes
// this allows us to not send the op as even the local client will ignore the segment
overwriteInfo<IHasInsertionInfo>(segment, {
insert: {
type: "insert",
seq: UniversalSequenceNumber,
localSeq: undefined,
clientId: NonCollabClient,
},
});
this._mergeTree.blockUpdatePathLengths(segment.parent, unusedStamp, true);
return undefined;
}

const segInsertOp: ISegment = segment.clone();
const opProps =
isObject(resetOp.seg) && "props" in resetOp.seg && isObject(resetOp.seg.props)
? { ...resetOp.seg.props }
: undefined;
segInsertOp.properties = opProps;
return createInsertSegmentOp(segmentPosition, segInsertOp);
}

private resetPendingDeltaToOps(
resetOp: IMergeTreeDeltaOp,

Expand Down Expand Up @@ -1174,82 +1257,12 @@ export class Client extends TypedEventEmitter<IClientEvents> {
let newOp: IMergeTreeDeltaOp | undefined;
switch (resetOp.type) {
case MergeTreeDeltaType.ANNOTATE: {
assert(
segment.propertyManager?.hasPendingProperties(resetOp.props ?? resetOp.adjust) ===
true,
0x036 /* "Segment has no pending properties" */,
);
// if the segment has been removed or obliterated, there's no need to send the annotate op
// unless the remove was local, in which case the annotate must have come
// before the remove
if (!isRemovedAndAcked(segment)) {
newOp =
resetOp.props === undefined
? createAdjustRangeOp(
segmentPosition,
segmentPosition + segment.cachedLength,
resetOp.adjust,
)
: createAnnotateRangeOp(
segmentPosition,
segmentPosition + segment.cachedLength,
resetOp.props,
);
}
newOp = this.resetAnnotateOp(resetOp, segment, segmentPosition);
break;
}

case MergeTreeDeltaType.INSERT: {
if (isInserted(segment) && opstampUtils.isSquashedOp(segment.insert)) {
break;
}
assert(
isInserted(segment) && opstampUtils.isLocal(segment.insert),
0x037 /* "Segment already has assigned sequence number" */,
);
const removeInfo = toRemovalInfo(segment);

const unusedStamp: OperationStamp = { seq: 0, clientId: 0 };
if (removeInfo !== undefined && squash) {
assert(
removeInfo.removes.length === 1 ||
opstampUtils.isAcked(removeInfo.removes[removeInfo.removes.length - 2]),
0xbaf /* Expected only one local remove */,
);
this.squashInsertion(segment);
break;
} else if (removeInfo !== undefined && opstampUtils.isAcked(removeInfo.removes[0])) {
assert(
removeInfo.removes[0].type === "sliceRemove",
0xb5c /* Remove on insertion must be caused by obliterate. */,
);
errorIfOptionNotTrue(
this._mergeTree.options,
"mergeTreeEnableObliterateReconnect",
);
// the segment was remotely obliterated, so is considered removed
// we set the seq to the universal seq and remove the local seq,
// so its length is not considered for subsequent local changes
// this allows us to not send the op as even the local client will ignore the segment
overwriteInfo<IHasInsertionInfo>(segment, {
insert: {
type: "insert",
seq: UniversalSequenceNumber,
localSeq: undefined,
clientId: NonCollabClient,
},
});
this._mergeTree.blockUpdatePathLengths(segment.parent, unusedStamp, true);
break;
}

const segInsertOp: ISegment = segment.clone();
const opProps =
isObject(resetOp.seg) && "props" in resetOp.seg && isObject(resetOp.seg.props)
? { ...resetOp.seg.props }
: undefined;
segInsertOp.properties = opProps;
newOp = createInsertSegmentOp(segmentPosition, segInsertOp);
newOp = this.resetInsertOp(resetOp, segment, segmentPosition, squash);
break;
}

Expand All @@ -1270,11 +1283,19 @@ export class Client extends TypedEventEmitter<IClientEvents> {
}

if (newOp) {
let newPreviousProps: WeakMap<ISegmentLeaf, PropertySet> | undefined;
if (segmentGroup.previousProps) {
const sourceProps = segmentGroup.previousProps.get(segment);
newPreviousProps = new WeakMap();
if (sourceProps !== undefined) {
newPreviousProps.set(segment, sourceProps);
}
}

@anthony-murphy anthony-murphy May 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Deep Review: This hunk replaces previousProps: segmentGroup.previousProps?.slice(0) with a freshly constructed WeakMap carrying only the current segment's entry — the PR description identifies the old .slice(0) as a latent inconsistency (defensive copy of the props array, but the surrounding loop only ever wrote a single segment to the new group, so the array shape was already mispaired post-copy). The PR ships with no test changes.

Closest existing coverage (resetPendingSegmentsToOp.spec.ts:189-211, client.rollback.spec.ts:206-245 and :658-688) doesn't inspect per-segment previousProps entries. Prior bugs in this exact reset/rebase path were fuzz-discovered, not unit-discovered (#11946, #22069), so existing unit tests are an unreliable safety net for semantic regressions here.

Add a focused regression in resetPendingSegmentsToOp.spec.ts (or client.rollback.spec.ts) that constructs a pending annotate group with multiple segments, triggers resetPendingDeltaToOps reissue, and asserts each new per-segment SegmentGroup has a previousProps WeakMap containing exactly the entry for its own segment.

const newSegmentGroup: SegmentGroup = {
segments: [],
localSeq: segmentGroup.localSeq,
refSeq: this.getCollabWindow().currentSeq,
previousProps: segmentGroup.previousProps?.slice(0),
previousProps: newPreviousProps,
};

segment.segmentGroups.enqueue(newSegmentGroup);
Expand Down
12 changes: 7 additions & 5 deletions packages/dds/merge-tree/src/mergeTree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1426,7 +1426,7 @@ export class MergeTree {
refSeq: this.collabWindow.currentSeq,
};
if (previousProps) {
_segmentGroup.previousProps = [];
_segmentGroup.previousProps = new WeakMap();
}
this.pendingSegments.push(_segmentGroup);
}
Expand All @@ -1438,7 +1438,7 @@ export class MergeTree {
throw new Error("All segments in group should have previousProps or none");
}
if (previousProps) {
_segmentGroup.previousProps!.push(previousProps);
_segmentGroup.previousProps!.set(segment, previousProps);
}

const segmentGroups = (segment.segmentGroups ??= new SegmentGroupCollection(segment));
Expand Down Expand Up @@ -2449,7 +2449,6 @@ export class MergeTree {
) {
throw new Error("Rollback op doesn't match last edit");
}
let i = 0;
for (const segment of pendingSegmentGroup.segments) {
const segmentSegmentGroup = segment?.segmentGroups?.pop();
assert(
Expand All @@ -2476,7 +2475,11 @@ export class MergeTree {
{ op: removeOp, rollback: true },
);
} /* op.type === MergeTreeDeltaType.ANNOTATE */ else {
const props = pendingSegmentGroup.previousProps![i];
const props = pendingSegmentGroup.previousProps!.get(segment);
assert(
props !== undefined,
"Segment missing previousProps entry on annotate rollback",
);

@anthony-murphy anthony-murphy May 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Deep Review: This new assert uses a raw string, but every other assert(...) in mergeTree.ts uses a numbered hex tag (0x036, 0x037, 0x043–0x04f, 0x751, 0x86c, 0xa40, 0xa6e, 0xb5c, 0xb5d, 0xb6e–0xb73, 0xbaf — 16+ examples). This very PR preserves 0x036, 0x037, 0xbaf, 0xb5c byte-identical in the extracted client.ts helpers, so the convention is intentional.

The repo ships policy-check:asserts (flub generate assertTags) that rewrites raw strings to hex tags, so this either gets rewritten before merge or trips the policy gate. Tags are how assertionShortCodesMap maps production failures back to messages without shipping the strings.

Run npm run policy-check:asserts before merge, or tag manually now:

Suggested change
);
const props = pendingSegmentGroup.previousProps!.get(segment);
assert(
props !== undefined,
0xXXXX /* Segment missing previousProps entry on annotate rollback */,
);


// If the segment has been removed by a concurrent operation, we can't use
// position-based annotateRange because findRollbackPosition returns a position
Expand Down Expand Up @@ -2505,7 +2508,6 @@ export class MergeTree {
{ op: annotateOp, rollback: true },
);
}
i++;
}
}
} else {
Expand Down
2 changes: 1 addition & 1 deletion packages/dds/merge-tree/src/mergeTreeNodes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ export interface ObliterateInfo {

export interface SegmentGroup {
segments: ISegmentLeaf[];
previousProps?: PropertySet[];
previousProps?: WeakMap<ISegmentLeaf, PropertySet>;
localSeq?: number;
refSeq: number;
obliterateInfo?: ObliterateInfo;
Expand Down
17 changes: 13 additions & 4 deletions packages/dds/merge-tree/src/segmentGroupCollection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import { DoublyLinkedList, walkList } from "@fluidframework/core-utils/internal";

import type { SegmentGroup, ISegmentLeaf } from "./mergeTreeNodes.js";
import type { PropertySet } from "./properties.js";

export class SegmentGroupCollection {
private readonly segmentGroups: DoublyLinkedList<SegmentGroup>;
Expand Down Expand Up @@ -48,13 +49,21 @@ export class SegmentGroupCollection {
walkList(this.segmentGroups, (sg) => segmentGroups.enqueueOnCopy(sg.data, this.segment));
}

/**
* Returns the previousProps entry paired with this collection's segment within the given
* segmentGroup, or undefined if the group has no previousProps or no entry exists for the segment.
*/
public previousPropsForSegment(segmentGroup: SegmentGroup): PropertySet | undefined {
return segmentGroup.previousProps?.get(this.segment);
}

@anthony-murphy anthony-murphy May 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Deep Review: The PR description states the accessor "encapsulates the … lookup" and that callers become a clean one-liner, but the two natural call sites that landed in the same PR continue to access previousProps directly — mergeTree.ts:2479 uses pendingSegmentGroup.previousProps!.get(segment) and client.ts:~1286 uses segmentGroup.previousProps.get(segment). grep previousPropsForSegment across packages/dds/merge-tree returns only this declaration; the encapsulation goal isn't realized in this commit.

Blast radius is narrow — index.ts does not re-export SegmentGroupCollection, so this isn't package-public surface — but the description-vs-diff inconsistency is real and PR #11961's review precedent was to keep copy-only previousProps plumbing private until a consumer exists.

Either (a) migrate mergeTree.ts:2479 and client.ts:~1286 to segment.segmentGroups.previousPropsForSegment(segmentGroup) so the encapsulation lands here, or (b) drop the accessor until the consumer lands in the squash PR.


private enqueueOnCopy(segmentGroup: SegmentGroup, sourceSegment: ISegmentLeaf): void {
this.enqueue(segmentGroup);
if (segmentGroup.previousProps) {
// duplicate the previousProps for this segment
const index = segmentGroup.segments.indexOf(sourceSegment);
if (index !== -1) {
segmentGroup.previousProps.push(segmentGroup.previousProps[index]);
// duplicate the previousProps entry for the destination segment, keyed off the source's entry
const sourceProps = segmentGroup.previousProps.get(sourceSegment);
if (sourceProps !== undefined) {
segmentGroup.previousProps.set(this.segment, sourceProps);
}

@anthony-murphy anthony-murphy May 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Deep Review: enqueueOnCopy is the structural successor to PR #11961 (scarlettjlee, 2022-09-15), which fixed the exact split-after-annotate rollback crash: "when a segment is split after an annotation, there was no corresponding property set for the new segment, causing rollback to crash." The rewritten copy path — segmentGroup.previousProps.get(sourceSegment) → segmentGroup.previousProps.set(this.segment, sourceProps) — is the load-bearing replacement for the prior indexOf + parallel-array push logic.

The new .copyTo spec in test/segmentGroupCollection.spec.ts only enqueues groups with previousProps === undefined (segmentGroups.enqueue({ segments: [], localSeq: 1, refSeq: 0 })). Three new tests were added for the trivial previousPropsForSegment read accessor; zero for the nontrivial copy logic. Prior bugs on this path (#11946, #22069, #24253) were fuzz-discovered, not unit-discovered — existing unit tests aren't a reliable safety net for a silent regression on the split-then-rollback case.

Distinct from the reissue-path concern on client.ts:1293 — that thread covers resetPendingDeltaToOps; this is the split-via-copyTo path.

Add a .copyTo (or sibling) case here that constructs a source SegmentGroup with a populated previousProps WeakMap entry for the source segment, runs the enqueue-on-copy path, and asserts the destination's SegmentGroup.previousProps.get(destSegment) === sourceProps. Ideally also exercise MergeTree.rollback ANNOTATE end-to-end against the split case PR #11961 originally regressed on.

}
}
Expand Down
Loading