From 2ec428487b0aca24ba689ed53ba0d0dc8e8e9f8b Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Mon, 16 Jun 2025 13:53:15 -0700 Subject: [PATCH 1/7] rebase ref, not pos --- .../dds/sequence/src/intervalCollection.ts | 75 +++++++------------ .../src/intervalCollectionMapInterfaces.ts | 10 ++- .../overlappingIntervalsIndex.ts | 2 +- .../dds/sequence/src/test/fuzz/fuzzUtils.ts | 7 ++ 4 files changed, 40 insertions(+), 54 deletions(-) diff --git a/packages/dds/sequence/src/intervalCollection.ts b/packages/dds/sequence/src/intervalCollection.ts index c671750a7600..654c802b28bb 100644 --- a/packages/dds/sequence/src/intervalCollection.ts +++ b/packages/dds/sequence/src/intervalCollection.ts @@ -739,7 +739,9 @@ function clearEmptyPendingEntry(pendingChanges: PendingChanges, id: string) { } } -function hasEndpointChanges(serialized: SerializedIntervalDelta) { +function hasEndpointChanges( + serialized: SerializedIntervalDelta, +): serialized is ISerializedInterval { return serialized.start !== undefined && serialized.end !== undefined; } @@ -932,7 +934,7 @@ export class IntervalCollection const rebasedValue = localOpMetadata.endpointChangesNode === undefined ? value - : this.rebaseLocalInterval(localOpMetadata); + : this.rebaseLocalInterval(value, localOpMetadata); if (rebasedValue === undefined) { const { id } = getSerializedProperties(value); @@ -974,68 +976,45 @@ export class IntervalCollection } } - private rebasePositionWithSegmentSlide( - pos: number | "start" | "end", - seqNumberFrom: number, + private rebaseReferenceWithSegmentSlide( + ref: LocalReferencePosition, localSeq: number, - ): number | "start" | "end" | undefined { + ): number | "start" | "end" { if (!this.client) { throw new LoggingError("mergeTree client must exist"); } - if (pos === "start" || pos === "end") { - return pos; - } - const { clientId } = this.client.getCollabWindow(); - const { segment, offset } = - this.client.getContainingSegment( - pos, - { - referenceSequenceNumber: seqNumberFrom, - clientId: this.client.getLongClientId(clientId), - }, - localSeq, - ) ?? {}; - - // if segment is undefined, it slid off the string - assert(segment !== undefined && offset !== undefined, 0x54e /* No segment found */); + const segment = ref.getSegment(); + const offset = ref.getOffset(); const segoff = getSlideToSegoff( - { segment, offset }, - undefined, + segment === undefined ? undefined : { segment, offset }, + ref.slidingPreference, createLocalReconnectingPerspective(this.client.getCurrentSeq(), clientId, localSeq), this.options.mergeTreeReferencesCanSlideToEndpoint, ); // case happens when rebasing op, but concurrently entire string has been deleted - if (segoff?.segment === undefined || segoff.offset === undefined) { + if (segoff === undefined) { return DetachedReferencePosition; } - - assert( - offset !== undefined && 0 <= offset && offset < segment.cachedLength, - 0x54f /* Invalid offset */, - ); return this.client.findReconnectionPosition(segoff.segment, localSeq) + segoff.offset; } private computeRebasedPositions( localOpMetadata: IntervalAddLocalMetadata | IntervalChangeLocalMetadata, - ): ISerializedInterval | SerializedIntervalDelta { + ): Pick { assert( this.client !== undefined, 0x550 /* Client should be defined when computing rebased position */, ); - const { localSeq, original } = localOpMetadata; - const rebased = { ...original }; - const { start, end, sequenceNumber } = original; - if (start !== undefined) { - rebased.start = this.rebasePositionWithSegmentSlide(start, sequenceNumber, localSeq); - } - if (end !== undefined) { - rebased.end = this.rebasePositionWithSegmentSlide(end, sequenceNumber, localSeq); - } + + const { localSeq, interval } = localOpMetadata; + const rebased = { + start: this.rebaseReferenceWithSegmentSlide(interval.start, localSeq), + end: this.rebaseReferenceWithSegmentSlide(interval.end, localSeq), + }; return rebased; } @@ -1216,7 +1195,7 @@ export class IntervalCollection { type: "add", localSeq, - original: serializedInterval, + interval, }, ); } @@ -1345,7 +1324,7 @@ export class IntervalCollection type: "change", localSeq, previous: interval.serialize(), - original: serializedInterval, + interval: newInterval ?? interval, }; this.submitDelta( @@ -1369,8 +1348,6 @@ export class IntervalCollection } if (newInterval) { this.emitChange(newInterval, interval, true, false); - this.client?.removeLocalReferencePosition(interval.start); - this.client?.removeLocalReferencePosition(interval.end); } return newInterval; } @@ -1477,14 +1454,14 @@ export class IntervalCollection * */ public rebaseLocalInterval( + original: SerializedIntervalDelta, localOpMetadata: IntervalAddLocalMetadata | IntervalChangeLocalMetadata, ): SerializedIntervalDelta | undefined { - const original = localOpMetadata.original; - if (!this.client) { + if (!this.client || !hasEndpointChanges(original)) { // If there's no associated mergeTree client, the originally submitted op is still correct. return original; } - if (!this.attached) { + if (!this.attached || this.localCollection === undefined) { throw new LoggingError("attachSequence must be called"); } @@ -1494,7 +1471,7 @@ export class IntervalCollection const { start: startRebased, end: endRebased } = (localOpMetadata.rebased ??= this.computeRebasedPositions(localOpMetadata)); - const localInterval = this.localCollection?.idIntervalIndex.getIntervalById(id); + const localInterval = this.localCollection.idIntervalIndex.getIntervalById(id); const rebased: SerializedIntervalDelta = { start: startRebased, @@ -1521,7 +1498,7 @@ export class IntervalCollection if (localInterval !== undefined) { // The rebased op may place this interval's endpoints on different segments. Calling `changeInterval` here // updates the local client's state to be consistent with the emitted op. - this.localCollection?.changeInterval( + localOpMetadata.interval = this.localCollection?.changeInterval( localInterval, toOptionalSequencePlace(startRebased, startSide ?? Side.Before), toOptionalSequencePlace(endRebased, endSide ?? Side.Before), diff --git a/packages/dds/sequence/src/intervalCollectionMapInterfaces.ts b/packages/dds/sequence/src/intervalCollectionMapInterfaces.ts index adaea5dca094..4d0680352d25 100644 --- a/packages/dds/sequence/src/intervalCollectionMapInterfaces.ts +++ b/packages/dds/sequence/src/intervalCollectionMapInterfaces.ts @@ -15,28 +15,30 @@ import { ISerializedInterval, IntervalDeltaOpType, SerializedIntervalDelta, + type SequenceIntervalClass, } from "./intervals/index.js"; export interface IntervalAddLocalMetadata { type: typeof IntervalDeltaOpType.ADD; localSeq: number; endpointChangesNode?: ListNode; - rebased?: ISerializedInterval; - original: ISerializedInterval; + rebased?: Pick; + interval: SequenceIntervalClass; } export interface IntervalChangeLocalMetadata { type: typeof IntervalDeltaOpType.CHANGE; localSeq: number; previous: ISerializedInterval; endpointChangesNode?: ListNode; - rebased?: SerializedIntervalDelta; - original: SerializedIntervalDelta; + rebased?: Pick; + interval: SequenceIntervalClass; } export interface IntervalDeleteLocalMetadata { type: typeof IntervalDeltaOpType.DELETE; localSeq: number; previous: ISerializedInterval; endpointChangesNode?: undefined; + interval?: undefined; } export type IntervalMessageLocalMetadata = | IntervalAddLocalMetadata diff --git a/packages/dds/sequence/src/intervalIndex/overlappingIntervalsIndex.ts b/packages/dds/sequence/src/intervalIndex/overlappingIntervalsIndex.ts index d3998af88115..205e4f1b3619 100644 --- a/packages/dds/sequence/src/intervalIndex/overlappingIntervalsIndex.ts +++ b/packages/dds/sequence/src/intervalIndex/overlappingIntervalsIndex.ts @@ -162,7 +162,7 @@ export class OverlappingIntervalsIndex implements ISequenceOverlappingIntervalsI } public remove(interval: SequenceIntervalClass) { - this.intervalTree.removeExisting(interval); + this.intervalTree.remove(interval); } public add(interval: SequenceIntervalClass) { diff --git a/packages/dds/sequence/src/test/fuzz/fuzzUtils.ts b/packages/dds/sequence/src/test/fuzz/fuzzUtils.ts index 988b856541bb..f8e87f67a066 100644 --- a/packages/dds/sequence/src/test/fuzz/fuzzUtils.ts +++ b/packages/dds/sequence/src/test/fuzz/fuzzUtils.ts @@ -483,6 +483,13 @@ export const baseModel: Omit< break; case "removeRange": case "annotateRange": + if (op.start > 0) { + op.start--; + } + if (op.end > 0) { + op.end--; + } + break; case "addInterval": case "changeInterval": { const { startPos, endPos, startSide, endSide } = endpointPosAndSide( From 2c7561626b5c930547bc55fe5c95f5b3e5437780 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Tue, 17 Jun 2025 09:27:40 -0700 Subject: [PATCH 2/7] exports --- packages/dds/merge-tree/src/client.ts | 15 +++++++-- packages/dds/merge-tree/src/index.ts | 2 +- .../src/intervals/sequenceInterval.ts | 33 +++++++++++++++++++ 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/packages/dds/merge-tree/src/client.ts b/packages/dds/merge-tree/src/client.ts index fa6947f3d173..91378e9ef30d 100644 --- a/packages/dds/merge-tree/src/client.ts +++ b/packages/dds/merge-tree/src/client.ts @@ -108,7 +108,10 @@ import * as opstampUtils from "./stamps.js"; type IMergeTreeDeltaRemoteOpArgs = Omit & Required>; -interface RebasedObliterateEndpoint { +/** + * @internal + */ +export interface RebasedObliterateEndpoint { segment: ISegmentLeaf; offset: number; side: Side; @@ -910,8 +913,14 @@ export class Client extends TypedEventEmitter { return { segment: newSegment, offset: newOffset, side: newSide }; } - private computeNewObliterateEndpoints( - obliterateInfo: ObliterateInfo, + public computeNewObliterateEndpoints( + obliterateInfo: { + start: LocalReferencePosition; + end: LocalReferencePosition; + stamp: OperationStamp; + startSide: Side; + endSide: Side; + }, squash: boolean, ): { start: RebasedObliterateEndpoint; diff --git a/packages/dds/merge-tree/src/index.ts b/packages/dds/merge-tree/src/index.ts index 9ed98ab94504..0501c152b34f 100644 --- a/packages/dds/merge-tree/src/index.ts +++ b/packages/dds/merge-tree/src/index.ts @@ -15,7 +15,7 @@ export { createPropertyTrackingAttributionPolicyFactory, createPropertyTrackingAndInsertionAttributionPolicyFactory, } from "./attributionPolicy.js"; -export { Client, IClientEvents } from "./client.js"; +export { Client, IClientEvents, RebasedObliterateEndpoint } from "./client.js"; export { ConflictAction, Dictionary, diff --git a/packages/dds/sequence/src/intervals/sequenceInterval.ts b/packages/dds/sequence/src/intervals/sequenceInterval.ts index 8263dfd21568..a3d1e1357868 100644 --- a/packages/dds/sequence/src/intervals/sequenceInterval.ts +++ b/packages/dds/sequence/src/intervals/sequenceInterval.ts @@ -31,6 +31,7 @@ import { type ISegmentInternal, UnassignedSequenceNumber, UniversalSequenceNumber, + type RebasedObliterateEndpoint, } from "@fluidframework/merge-tree/internal"; import { UsageError } from "@fluidframework/telemetry-utils/internal"; import { v4 as uuid } from "uuid"; @@ -498,6 +499,38 @@ export class SequenceIntervalClass implements SequenceInterval, ISerializableInt return endPos > bstart && startPos < bend; } + public rebaseEndpoints(rebased: Record<"start" | "end", RebasedObliterateEndpoint>) { + const startRef = createPositionReferenceFromSegoff( + this.client, + rebased.start, + this.start.refType, + undefined, + undefined, + undefined, + startReferenceSlidingPreference(this.stickiness), + startReferenceSlidingPreference(this.stickiness) === SlidingPreference.BACKWARD, + ); + if (this.start.properties) { + startRef.addProperties(this.start.properties); + } + this.start = startRef; + + const endRef = createPositionReferenceFromSegoff( + this.client, + rebased.end, + this.end.refType, + undefined, + undefined, + undefined, + endReferenceSlidingPreference(this.stickiness), + endReferenceSlidingPreference(this.stickiness) === SlidingPreference.FORWARD, + ); + if (this.end.properties) { + endRef.addProperties(this.end.properties); + } + this.end = endRef; + } + /** * {@inheritDoc IInterval.modify} */ From e5a8f05fbc6ded6e26259228361530fc1e9f8bda Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Tue, 17 Jun 2025 11:35:32 -0700 Subject: [PATCH 3/7] fix sides --- packages/dds/sequence/src/intervals/sequenceInterval.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/dds/sequence/src/intervals/sequenceInterval.ts b/packages/dds/sequence/src/intervals/sequenceInterval.ts index a3d1e1357868..7b2053e868a4 100644 --- a/packages/dds/sequence/src/intervals/sequenceInterval.ts +++ b/packages/dds/sequence/src/intervals/sequenceInterval.ts @@ -286,8 +286,8 @@ export class SequenceIntervalClass implements SequenceInterval, ISerializableInt public end: LocalReferencePosition, public intervalType: IntervalType, props?: PropertySet, - public readonly startSide: Side = Side.Before, - public readonly endSide: Side = Side.Before, + public startSide: Side = Side.Before, + public endSide: Side = Side.Before, ) { if (props) { this.#props.properties = addProperties(this.#props.properties, props); @@ -500,6 +500,7 @@ export class SequenceIntervalClass implements SequenceInterval, ISerializableInt } public rebaseEndpoints(rebased: Record<"start" | "end", RebasedObliterateEndpoint>) { + this.startSide = rebased.start.side; const startRef = createPositionReferenceFromSegoff( this.client, rebased.start, @@ -515,6 +516,7 @@ export class SequenceIntervalClass implements SequenceInterval, ISerializableInt } this.start = startRef; + this.endSide = rebased.end.side; const endRef = createPositionReferenceFromSegoff( this.client, rebased.end, From 0c9a0ab8e09e118b24b50a8fdc2761749a758252 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Tue, 17 Jun 2025 11:37:44 -0700 Subject: [PATCH 4/7] clear rebase --- packages/dds/sequence/src/intervalCollection.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/dds/sequence/src/intervalCollection.ts b/packages/dds/sequence/src/intervalCollection.ts index 654c802b28bb..8d19d14f3b47 100644 --- a/packages/dds/sequence/src/intervalCollection.ts +++ b/packages/dds/sequence/src/intervalCollection.ts @@ -783,6 +783,7 @@ export class IntervalCollection if (md.type === "add" || (md.type === "change" && hasEndpointChanges(op.value))) { const endpointChanges = (pending.endpointChanges ??= new DoublyLinkedList()); md.endpointChangesNode = endpointChanges.push(md).last; + md.rebased = undefined; } submitDelta(op, pending.local.push(md).last); }; From 96c6417b6b6a2c6911016a0d85bbbcd6d193b730 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Tue, 17 Jun 2025 11:58:17 -0700 Subject: [PATCH 5/7] fix some tests --- packages/dds/sequence/src/intervalCollection.ts | 7 +++++-- .../src/intervalIndex/overlappingIntervalsIndex.ts | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/dds/sequence/src/intervalCollection.ts b/packages/dds/sequence/src/intervalCollection.ts index 8d19d14f3b47..73de31d48ef7 100644 --- a/packages/dds/sequence/src/intervalCollection.ts +++ b/packages/dds/sequence/src/intervalCollection.ts @@ -986,12 +986,15 @@ export class IntervalCollection } const { clientId } = this.client.getCollabWindow(); - const segment = ref.getSegment(); + const segment: ISegmentInternal | undefined = ref.getSegment(); + if (segment?.endpointType) { + return segment.endpointType; + } const offset = ref.getOffset(); const segoff = getSlideToSegoff( segment === undefined ? undefined : { segment, offset }, - ref.slidingPreference, + undefined, createLocalReconnectingPerspective(this.client.getCurrentSeq(), clientId, localSeq), this.options.mergeTreeReferencesCanSlideToEndpoint, ); diff --git a/packages/dds/sequence/src/intervalIndex/overlappingIntervalsIndex.ts b/packages/dds/sequence/src/intervalIndex/overlappingIntervalsIndex.ts index 205e4f1b3619..d3998af88115 100644 --- a/packages/dds/sequence/src/intervalIndex/overlappingIntervalsIndex.ts +++ b/packages/dds/sequence/src/intervalIndex/overlappingIntervalsIndex.ts @@ -162,7 +162,7 @@ export class OverlappingIntervalsIndex implements ISequenceOverlappingIntervalsI } public remove(interval: SequenceIntervalClass) { - this.intervalTree.remove(interval); + this.intervalTree.removeExisting(interval); } public add(interval: SequenceIntervalClass) { From aab8781bf9f95daf19816a93ff9cc6d98871ffb6 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Tue, 17 Jun 2025 12:41:31 -0700 Subject: [PATCH 6/7] unlink local refs from interval for undo/redo --- packages/dds/sequence/src/intervalCollection.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/dds/sequence/src/intervalCollection.ts b/packages/dds/sequence/src/intervalCollection.ts index 73de31d48ef7..d4c070fa99b2 100644 --- a/packages/dds/sequence/src/intervalCollection.ts +++ b/packages/dds/sequence/src/intervalCollection.ts @@ -1352,6 +1352,10 @@ export class IntervalCollection } if (newInterval) { this.emitChange(newInterval, interval, true, false); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + interval.start.properties!.interval = undefined; + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + interval.end.properties!.interval = undefined; } return newInterval; } From ac106c85f2eff136fbeff470d44a9c7891ed173b Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Tue, 17 Jun 2025 12:45:17 -0700 Subject: [PATCH 7/7] reverts --- packages/dds/merge-tree/src/client.ts | 15 ++----- packages/dds/merge-tree/src/index.ts | 2 +- .../src/intervals/sequenceInterval.ts | 39 +------------------ 3 files changed, 6 insertions(+), 50 deletions(-) diff --git a/packages/dds/merge-tree/src/client.ts b/packages/dds/merge-tree/src/client.ts index 91378e9ef30d..fa6947f3d173 100644 --- a/packages/dds/merge-tree/src/client.ts +++ b/packages/dds/merge-tree/src/client.ts @@ -108,10 +108,7 @@ import * as opstampUtils from "./stamps.js"; type IMergeTreeDeltaRemoteOpArgs = Omit & Required>; -/** - * @internal - */ -export interface RebasedObliterateEndpoint { +interface RebasedObliterateEndpoint { segment: ISegmentLeaf; offset: number; side: Side; @@ -913,14 +910,8 @@ export class Client extends TypedEventEmitter { return { segment: newSegment, offset: newOffset, side: newSide }; } - public computeNewObliterateEndpoints( - obliterateInfo: { - start: LocalReferencePosition; - end: LocalReferencePosition; - stamp: OperationStamp; - startSide: Side; - endSide: Side; - }, + private computeNewObliterateEndpoints( + obliterateInfo: ObliterateInfo, squash: boolean, ): { start: RebasedObliterateEndpoint; diff --git a/packages/dds/merge-tree/src/index.ts b/packages/dds/merge-tree/src/index.ts index 0501c152b34f..9ed98ab94504 100644 --- a/packages/dds/merge-tree/src/index.ts +++ b/packages/dds/merge-tree/src/index.ts @@ -15,7 +15,7 @@ export { createPropertyTrackingAttributionPolicyFactory, createPropertyTrackingAndInsertionAttributionPolicyFactory, } from "./attributionPolicy.js"; -export { Client, IClientEvents, RebasedObliterateEndpoint } from "./client.js"; +export { Client, IClientEvents } from "./client.js"; export { ConflictAction, Dictionary, diff --git a/packages/dds/sequence/src/intervals/sequenceInterval.ts b/packages/dds/sequence/src/intervals/sequenceInterval.ts index 7b2053e868a4..8263dfd21568 100644 --- a/packages/dds/sequence/src/intervals/sequenceInterval.ts +++ b/packages/dds/sequence/src/intervals/sequenceInterval.ts @@ -31,7 +31,6 @@ import { type ISegmentInternal, UnassignedSequenceNumber, UniversalSequenceNumber, - type RebasedObliterateEndpoint, } from "@fluidframework/merge-tree/internal"; import { UsageError } from "@fluidframework/telemetry-utils/internal"; import { v4 as uuid } from "uuid"; @@ -286,8 +285,8 @@ export class SequenceIntervalClass implements SequenceInterval, ISerializableInt public end: LocalReferencePosition, public intervalType: IntervalType, props?: PropertySet, - public startSide: Side = Side.Before, - public endSide: Side = Side.Before, + public readonly startSide: Side = Side.Before, + public readonly endSide: Side = Side.Before, ) { if (props) { this.#props.properties = addProperties(this.#props.properties, props); @@ -499,40 +498,6 @@ export class SequenceIntervalClass implements SequenceInterval, ISerializableInt return endPos > bstart && startPos < bend; } - public rebaseEndpoints(rebased: Record<"start" | "end", RebasedObliterateEndpoint>) { - this.startSide = rebased.start.side; - const startRef = createPositionReferenceFromSegoff( - this.client, - rebased.start, - this.start.refType, - undefined, - undefined, - undefined, - startReferenceSlidingPreference(this.stickiness), - startReferenceSlidingPreference(this.stickiness) === SlidingPreference.BACKWARD, - ); - if (this.start.properties) { - startRef.addProperties(this.start.properties); - } - this.start = startRef; - - this.endSide = rebased.end.side; - const endRef = createPositionReferenceFromSegoff( - this.client, - rebased.end, - this.end.refType, - undefined, - undefined, - undefined, - endReferenceSlidingPreference(this.stickiness), - endReferenceSlidingPreference(this.stickiness) === SlidingPreference.FORWARD, - ); - if (this.end.properties) { - endRef.addProperties(this.end.properties); - } - this.end = endRef; - } - /** * {@inheritDoc IInterval.modify} */