Add explicit mechanism for preventing the submission of dangerous Sha…

…redTree commits (#23276)

## Description

Currently, if the application of a local commit crashes a SharedTree
client, that client will crash _before_ sending the op to other clients,
which prevents them from crashing in turn. This is good, but the way
that it is currently achieved is somewhat obfuscated, relies on bizarre
event ordering guarantees, and is preventing some future features from
being possible.

More specifically, the checkout currently updates its forest in response
to its branch's `"beforeChange"` event. This is because op submission
happens in response to the `"afterChange"` event, so if the checkout
crashes during `"beforeChange"`, we won't progress to `"afterChange"`.
However, that means that when the end user of SharedTree receives a
`"nodeChanged"` or `"treeChanged"` event, it will be in the context of
the `"beforeChange"` event - so the forest will be updated according to
their change, but the commit graph will not. Therefore, they cannot
(sanely) do operations that affect the commit graph - like forking or
merging their branches - in an event handler. This PR moves the forest
update to the `"afterChange"` event, so that the commit graph is updated
before the user's event handler is called. It does this by adding an
explicit mechanism to the checkout for monitoring when commits have been
"validated" - and SharedTree then uses this to determine when they
should be submitted to other clients. `SharedTreeCore` now attempts to
submit commits during `"beforeChange"`, not `"afterChange"`, but is
intercepted by `SharedTree` and then delayed until after validation.

This PR also does a smattering of other related cleanup, including:

* Removing the return values from all the branch operations, for
simplicity.
* Adjusting the arguments to `"beforeBatch"` and `"afterBatch"` to suit
their usage and adding documentation.
* Tightening the arguments to `SharedTreeBranchChange` and improving the
documentation.
* Adding a helper function for implementing lazy+cached properties, and
using it to optimize the `merge` operation of `SharedTreeBranch` as well
as for a cached property in the rebase logic.
* Removing the current (and confusing) injection of the event emitter
from the SharedTree into the checkout, and moving it into a more
explicit `load` function. While still ugly, it is at least
straightforward, and it combines with and cleans up the existing "on
load" function `setTipRevisionForLoadedData()`.

packages/dds/tree/src/core/rebase/utils.ts

@@ -5,7 +5,7 @@
 
 import { assert, oob } from "@fluidframework/core-utils/internal";
 
-import { hasSome, type Mutable } from "../../util/index.js";
+import { defineLazyCachedProperty, hasSome, type Mutable } from "../../util/index.js";
 
 import {
 	type ChangeRebaser,
@@ -312,26 +312,23 @@ export function rebaseBranch<TChange>(
 		revInfos.unshift({ revision: rollback.revision, rollbackOf: rollback.rollbackOf });
 	}
 
-	let netChange: TChange | undefined;
-	return {
-		newSourceHead: newHead,
-		get sourceChange(): TChange | undefined {
-			if (netChange === undefined) {
-				netChange = changeRebaser.compose(editsToCompose);
-			}
-			return netChange;
-		},
-		commits: {
-			deletedSourceCommits,
-			targetCommits,
-			sourceCommits,
-		},
-		telemetryProperties: {
-			sourceBranchLength,
-			rebaseDistance: targetCommits.length,
-			countDropped: sourceBranchLength - sourceSet.size,
+	return defineLazyCachedProperty(
+		{
+			newSourceHead: newHead,
+			commits: {
+				deletedSourceCommits,
+				targetCommits,
+				sourceCommits,
+			},
+			telemetryProperties: {
+				sourceBranchLength,
+				rebaseDistance: targetCommits.length,
+				countDropped: sourceBranchLength - sourceSet.size,
+			},
 		},
-	};
+		"sourceChange",
+		() => changeRebaser.compose(editsToCompose),
+	);
 }
 
 /**

packages/dds/tree/src/shared-tree-core/branch.ts

@@ -24,15 +24,10 @@ import {
 import type { Listenable } from "@fluidframework/core-interfaces";
 import { createEmitter } from "@fluid-internal/client-utils";
 
-import { hasSome } from "../util/index.js";
+import { hasSome, defineLazyCachedProperty } from "../util/index.js";
 
 /**
- * Describes a change to a `SharedTreeBranch`. Various operations can mutate the head of the branch;
- * this change format describes each in terms of the "removed commits" (all commits which were present
- * on the branch before the operation but are no longer present after) and the "new commits" (all
- * commits which are present on the branch after the operation that were not present before). Each of
- * the following event types also provides a `change` which contains the net change to the branch
- * (or is undefined if there was no net change):
+ * Describes a change to a `SharedTreeBranch`. Each of the following event types provides a `change` which contains the net change to the branch (or is undefined if there was no net change):
  * * Append - when one or more commits are appended to the head of the branch, for example via
  * a change applied by the branch's editor, or as a result of merging another branch into this one
  * * Remove - when one or more commits are removed from the head of the branch.
@@ -43,18 +38,18 @@ export type SharedTreeBranchChange<TChange> =
 			type: "append";
 			kind: CommitKind;
 			change: TaggedChange<TChange>;
+			/** The commits appended to the head of the branch by this operation */
 			newCommits: readonly [GraphCommit<TChange>, ...GraphCommit<TChange>[]];
 	  }
 	| {
 			type: "remove";
-			change: TaggedChange<TChange> | undefined;
+			change: TaggedChange<TChange>;
+			/** The commits removed from the head of the branch by this operation */
 			removedCommits: readonly [GraphCommit<TChange>, ...GraphCommit<TChange>[]];
 	  }
 	| {
 			type: "rebase";
 			change: TaggedChange<TChange> | undefined;
-			removedCommits: readonly GraphCommit<TChange>[];
-			newCommits: readonly GraphCommit<TChange>[];
 	  };
 
 /**
@@ -148,35 +143,31 @@ export class SharedTreeBranch<TEditor extends ChangeFamilyEditor, TChange> {
 
 	/**
 	 * Apply a change to this branch.
-	 * @param taggedChange - the change to apply
+	 * @param change - the change to apply
 	 * @param kind - the kind of change to apply
 	 * @returns the change that was applied and the new head commit of the branch
 	 */
-	public apply(
-		taggedChange: TaggedChange<TChange>,
-		kind: CommitKind = CommitKind.Default,
-	): [change: TChange, newCommit: GraphCommit<TChange>] {
+	public apply(change: TaggedChange<TChange>, kind: CommitKind = CommitKind.Default): void {
 		this.assertNotDisposed();
 
-		const revisionTag = taggedChange.revision;
+		const revisionTag = change.revision;
 		assert(revisionTag !== undefined, 0xa49 /* Revision tag must be provided */);
 
 		const newHead = mintCommit(this.head, {
 			revision: revisionTag,
-			change: taggedChange.change,
+			change: change.change,
 		});
 
 		const changeEvent = {
 			type: "append",
 			kind,
-			change: taggedChange,
+			change,
 			newCommits: [newHead],
 		} as const;
 
 		this.#events.emit("beforeChange", changeEvent);
 		this.head = newHead;
 		this.#events.emit("afterChange", changeEvent);
-		return [taggedChange.change, newHead];
 	}
 
 	/**
@@ -215,13 +206,13 @@ export class SharedTreeBranch<TEditor extends ChangeFamilyEditor, TChange> {
 	public rebaseOnto(
 		branch: SharedTreeBranch<TEditor, TChange>,
 		upTo = branch.getHead(),
-	): BranchRebaseResult<TChange> | undefined {
+	): void {
 		this.assertNotDisposed();
 
 		// Rebase this branch onto the given branch
 		const rebaseResult = this.rebaseBranch(this, branch, upTo);
 		if (rebaseResult === undefined) {
-			return undefined;
+			return;
 		}
 
 		// The net change to this branch is provided by the `rebaseBranch` API
@@ -243,19 +234,16 @@ export class SharedTreeBranch<TEditor extends ChangeFamilyEditor, TChange> {
 		this.#events.emit("beforeChange", changeEvent);
 		this.head = newSourceHead;
 		this.#events.emit("afterChange", changeEvent);
-		return rebaseResult;
 	}
 
 	/**
 	 * Remove a range of commits from this branch.
 	 * @param commit - All commits after (but not including) this commit will be removed.
 	 * @returns The net change to this branch and the commits that were removed from this branch.
 	 */
-	public removeAfter(
-		commit: GraphCommit<TChange>,
-	): [change: TaggedChange<TChange> | undefined, removedCommits: GraphCommit<TChange>[]] {
+	public removeAfter(commit: GraphCommit<TChange>): void {
 		if (commit === this.head) {
-			return [undefined, []];
+			return;
 		}
 
 		const removedCommits: GraphCommit<TChange>[] = [];
@@ -288,7 +276,6 @@ export class SharedTreeBranch<TEditor extends ChangeFamilyEditor, TChange> {
 		this.#events.emit("beforeChange", changeEvent);
 		this.head = commit;
 		this.#events.emit("afterChange", changeEvent);
-		return [change, removedCommits];
 	}
 
 	/**
@@ -298,9 +285,7 @@ export class SharedTreeBranch<TEditor extends ChangeFamilyEditor, TChange> {
 	 * @returns the net change to this branch and the commits that were added to this branch by the merge,
 	 * or undefined if nothing changed
 	 */
-	public merge(
-		branch: SharedTreeBranch<TEditor, TChange>,
-	): [change: TChange, newCommits: GraphCommit<TChange>[]] | undefined {
+	public merge(branch: SharedTreeBranch<TEditor, TChange>): void {
 		this.assertNotDisposed();
 		branch.assertNotDisposed();
 
@@ -317,21 +302,20 @@ export class SharedTreeBranch<TEditor extends ChangeFamilyEditor, TChange> {
 		// Compute the net change to this branch
 		const sourceCommits = rebaseResult.commits.sourceCommits;
 		assert(hasSome(sourceCommits), 0xa86 /* Expected source commits in non no-op merge */);
-		const change = this.changeFamily.rebaser.compose(sourceCommits);
-		const taggedChange = makeAnonChange(change);
-		const changeEvent = {
-			type: "append",
-			kind: CommitKind.Default,
-			get change(): TaggedChange<TChange> {
-				return taggedChange;
-			},
-			newCommits: sourceCommits,
-		} as const;
+		const { rebaser } = this.changeFamily;
+		const changeEvent = defineLazyCachedProperty(
+			{
+				type: "append",
+				kind: CommitKind.Default,
+				newCommits: sourceCommits,
+			} as const,
+			"change",
+			() => makeAnonChange(rebaser.compose(sourceCommits)),
+		);
 
 		this.#events.emit("beforeChange", changeEvent);
 		this.head = rebaseResult.newSourceHead;
 		this.#events.emit("afterChange", changeEvent);
-		return [change, sourceCommits];
 	}
 
 	/** Rebase `branchHead` onto `onto`, but return undefined if nothing changed */

packages/dds/tree/src/shared-tree-core/index.ts

@@ -26,6 +26,7 @@ export {
 	type Summarizable,
 	type SummaryElementParser,
 	type SummaryElementStringifier,
+	type ClonableSchemaAndPolicy,
 } from "./sharedTreeCore.js";
 
 export type { ResubmitMachine } from "./resubmitMachine.js";

packages/dds/tree/src/shared-tree-core/sharedTreeCore.ts

@@ -97,13 +97,6 @@ export class SharedTreeCore<TEditor extends ChangeFamilyEditor, TChange>
 		return this.getLocalBranch().editor;
 	}
 
-	/**
-	 * Gets the revision at the head of the trunk.
-	 */
-	protected get trunkHeadRevision(): RevisionTag {
-		return this.editManager.getTrunkHead().revision;
-	}
-
 	/**
 	 * Used to encode/decode messages sent to/received from the Fluid runtime.
 	 *
@@ -189,11 +182,9 @@ export class SharedTreeCore<TEditor extends ChangeFamilyEditor, TChange>
 				// Commit enrichment is only necessary for changes that will be submitted as ops, and changes issued while detached are not submitted.
 				this.commitEnricher.processChange(change);
 			}
-		});
-		this.editManager.localBranch.events.on("afterChange", (change) => {
 			if (change.type === "append") {
 				for (const commit of change.newCommits) {
-					this.submitCommit(commit, this.schemaAndPolicy);
+					this.submitCommit(commit, this.schemaAndPolicy, false);
 				}
 			}
 		});
@@ -266,6 +257,10 @@ export class SharedTreeCore<TEditor extends ChangeFamilyEditor, TChange>
 	}
 
 	protected async loadCore(services: IChannelStorageService): Promise<void> {
+		assert(
+			this.editManager.localBranch.getHead() === this.editManager.getTrunkHead(),
+			"All local changes should be applied to the trunk before loading from summary",
+		);
 		const [editManagerSummarizer, ...summarizables] = this.summarizables;
 		const loadEditManager = this.loadSummarizable(editManagerSummarizer, services);
 		const loadSummarizables = summarizables.map(async (s) =>
@@ -313,7 +308,7 @@ export class SharedTreeCore<TEditor extends ChangeFamilyEditor, TChange>
 	protected submitCommit(
 		commit: GraphCommit<TChange>,
 		schemaAndPolicy: ClonableSchemaAndPolicy,
-		isResubmit = false,
+		isResubmit: boolean,
 	): void {
 		assert(
 			this.isAttached() === (this.detachedRevision === undefined),
@@ -377,9 +372,6 @@ export class SharedTreeCore<TEditor extends ChangeFamilyEditor, TChange>
 		this.editManager.advanceMinimumSequenceNumber(brand(message.minimumSequenceNumber));
 	}
 
-	/**
-	 * @returns the head commit of the root local branch
-	 */
 	protected getLocalBranch(): SharedTreeBranch<TEditor, TChange> {
 		return this.editManager.localBranch;
 	}