From f1be6e3bf1c5a9919482f5c22b1412cdb537f0b3 Mon Sep 17 00:00:00 2001 From: Noah Encke <78610362+noencke@users.noreply.github.com> Date: Tue, 17 Dec 2024 11:53:47 -0800 Subject: [PATCH] Add explicit mechanism for preventing the submission of dangerous SharedTree 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 | 37 ++--- .../dds/tree/src/shared-tree-core/branch.ts | 66 +++----- .../dds/tree/src/shared-tree-core/index.ts | 1 + .../src/shared-tree-core/sharedTreeCore.ts | 20 +-- .../dds/tree/src/shared-tree/sharedTree.ts | 52 ++++--- .../dds/tree/src/shared-tree/treeCheckout.ts | 145 +++++++++++------- .../editManagerCorrectness.test.ts | 3 +- packages/dds/tree/src/test/util/utils.spec.ts | 22 ++- packages/dds/tree/src/util/index.ts | 1 + packages/dds/tree/src/util/utils.ts | 26 ++++ 10 files changed, 220 insertions(+), 153 deletions(-) diff --git a/packages/dds/tree/src/core/rebase/utils.ts b/packages/dds/tree/src/core/rebase/utils.ts index d91382873df6..bce4c1f85d15 100644 --- a/packages/dds/tree/src/core/rebase/utils.ts +++ b/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( 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), + ); } /** diff --git a/packages/dds/tree/src/shared-tree-core/branch.ts b/packages/dds/tree/src/shared-tree-core/branch.ts index 4cf137a80b4e..9f8080a375e4 100644 --- a/packages/dds/tree/src/shared-tree-core/branch.ts +++ b/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 = type: "append"; kind: CommitKind; change: TaggedChange; + /** The commits appended to the head of the branch by this operation */ newCommits: readonly [GraphCommit, ...GraphCommit[]]; } | { type: "remove"; - change: TaggedChange | undefined; + change: TaggedChange; + /** The commits removed from the head of the branch by this operation */ removedCommits: readonly [GraphCommit, ...GraphCommit[]]; } | { type: "rebase"; change: TaggedChange | undefined; - removedCommits: readonly GraphCommit[]; - newCommits: readonly GraphCommit[]; }; /** @@ -148,35 +143,31 @@ export class SharedTreeBranch { /** * 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, - kind: CommitKind = CommitKind.Default, - ): [change: TChange, newCommit: GraphCommit] { + public apply(change: TaggedChange, 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 { public rebaseOnto( branch: SharedTreeBranch, upTo = branch.getHead(), - ): BranchRebaseResult | 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,7 +234,6 @@ export class SharedTreeBranch { this.#events.emit("beforeChange", changeEvent); this.head = newSourceHead; this.#events.emit("afterChange", changeEvent); - return rebaseResult; } /** @@ -251,11 +241,9 @@ export class SharedTreeBranch { * @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, - ): [change: TaggedChange | undefined, removedCommits: GraphCommit[]] { + public removeAfter(commit: GraphCommit): void { if (commit === this.head) { - return [undefined, []]; + return; } const removedCommits: GraphCommit[] = []; @@ -288,7 +276,6 @@ export class SharedTreeBranch { this.#events.emit("beforeChange", changeEvent); this.head = commit; this.#events.emit("afterChange", changeEvent); - return [change, removedCommits]; } /** @@ -298,9 +285,7 @@ export class SharedTreeBranch { * @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, - ): [change: TChange, newCommits: GraphCommit[]] | undefined { + public merge(branch: SharedTreeBranch): void { this.assertNotDisposed(); branch.assertNotDisposed(); @@ -317,21 +302,20 @@ export class SharedTreeBranch { // 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 { - 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 */ diff --git a/packages/dds/tree/src/shared-tree-core/index.ts b/packages/dds/tree/src/shared-tree-core/index.ts index 7109cf0ce14a..6686f941ce52 100644 --- a/packages/dds/tree/src/shared-tree-core/index.ts +++ b/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"; diff --git a/packages/dds/tree/src/shared-tree-core/sharedTreeCore.ts b/packages/dds/tree/src/shared-tree-core/sharedTreeCore.ts index cea776fe668a..712512e68284 100644 --- a/packages/dds/tree/src/shared-tree-core/sharedTreeCore.ts +++ b/packages/dds/tree/src/shared-tree-core/sharedTreeCore.ts @@ -97,13 +97,6 @@ export class SharedTreeCore 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 // 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 } protected async loadCore(services: IChannelStorageService): Promise { + 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 protected submitCommit( commit: GraphCommit, schemaAndPolicy: ClonableSchemaAndPolicy, - isResubmit = false, + isResubmit: boolean, ): void { assert( this.isAttached() === (this.detachedRevision === undefined), @@ -377,9 +372,6 @@ export class SharedTreeCore this.editManager.advanceMinimumSequenceNumber(brand(message.minimumSequenceNumber)); } - /** - * @returns the head commit of the root local branch - */ protected getLocalBranch(): SharedTreeBranch { return this.editManager.localBranch; } diff --git a/packages/dds/tree/src/shared-tree/sharedTree.ts b/packages/dds/tree/src/shared-tree/sharedTree.ts index 4e458b401d13..e0a7b458686f 100644 --- a/packages/dds/tree/src/shared-tree/sharedTree.ts +++ b/packages/dds/tree/src/shared-tree/sharedTree.ts @@ -4,13 +4,7 @@ */ import { assert, unreachableCase } from "@fluidframework/core-utils/internal"; -import type { - HasListeners, - IEmitter, - IFluidHandle, - Listenable, -} from "@fluidframework/core-interfaces/internal"; -import { createEmitter } from "@fluid-internal/client-utils"; +import type { IFluidHandle } from "@fluidframework/core-interfaces/internal"; import type { IChannelAttributes, IChannelFactory, @@ -23,6 +17,7 @@ import { UsageError } from "@fluidframework/telemetry-utils/internal"; import { type ICodecOptions, noopValidator } from "../codec/index.js"; import { + type GraphCommit, type IEditableForest, type ITreeCursor, type JsonableTree, @@ -56,6 +51,7 @@ import { makeTreeChunker, } from "../feature-libraries/index.js"; import { + type ClonableSchemaAndPolicy, DefaultResubmitMachine, type ExplicitCoreCodecVersions, SharedTreeCore, @@ -85,12 +81,7 @@ import { SharedTreeReadonlyChangeEnricher } from "./sharedTreeChangeEnricher.js" import { SharedTreeChangeFamily } from "./sharedTreeChangeFamily.js"; import type { SharedTreeChange } from "./sharedTreeChangeTypes.js"; import type { SharedTreeEditBuilder } from "./sharedTreeEditBuilder.js"; -import { - type CheckoutEvents, - type TreeCheckout, - type BranchableTree, - createTreeCheckout, -} from "./treeCheckout.js"; +import { type TreeCheckout, type BranchableTree, createTreeCheckout } from "./treeCheckout.js"; import { breakingClass, fail, throwIfBroken } from "../util/index.js"; import type { IIdCompressor } from "@fluidframework/id-compressor"; @@ -229,9 +220,6 @@ export class SharedTree extends SharedTreeCore implements ISharedTree { - private readonly _events: Listenable & - IEmitter & - HasListeners; public readonly checkout: TreeCheckout; public get storedSchema(): TreeStoredSchemaRepository { return this.checkout.storedSchema; @@ -330,7 +318,6 @@ export class SharedTree ), changeEnricher, ); - this._events = createEmitter(); const localBranch = this.getLocalBranch(); this.checkout = createTreeCheckout( runtime.idCompressor, @@ -342,7 +329,6 @@ export class SharedTree schema, forest, fieldBatchCodec, - events: this._events, removedRoots, chunkCompressionStrategy: options.treeEncodeType, logger: this.logger, @@ -368,10 +354,10 @@ export class SharedTree this.commitEnricher.commitTransaction(); } }); - this.checkout.events.on("beforeBatch", (newCommits) => { - if (this.isAttached()) { + this.checkout.events.on("beforeBatch", (event) => { + if (event.type === "append" && this.isAttached()) { if (this.checkout.transaction.isInProgress()) { - this.commitEnricher.addTransactionCommits(newCommits); + this.commitEnricher.addTransactionCommits(event.newCommits); } } }); @@ -440,8 +426,7 @@ export class SharedTree protected override async loadCore(services: IChannelStorageService): Promise { await super.loadCore(services); - this.checkout.setTipRevisionForLoadedData(this.trunkHeadRevision); - this._events.emit("afterBatch", []); + this.checkout.load(); } protected override didAttach(): void { @@ -466,6 +451,27 @@ export class SharedTree ); super.applyStashedOp(...args); } + + protected override submitCommit( + commit: GraphCommit, + schemaAndPolicy: ClonableSchemaAndPolicy, + isResubmit: boolean, + ): void { + assert( + !this.checkout.transaction.isInProgress(), + "Cannot submit a commit while a transaction is in progress", + ); + if (isResubmit) { + return super.submitCommit(commit, schemaAndPolicy, isResubmit); + } + + // Refrain from submitting new commits until they are validated by the checkout. + // This is not a strict requirement for correctness in our system, but in the event that there is a bug when applying commits to the checkout + // that causes a crash (e.g. in the forest), this will at least prevent this client from sending the problematic commit to any other clients. + this.checkout.onCommitValid(commit, () => + super.submitCommit(commit, schemaAndPolicy, isResubmit), + ); + } } /** diff --git a/packages/dds/tree/src/shared-tree/treeCheckout.ts b/packages/dds/tree/src/shared-tree/treeCheckout.ts index dc81e9bf71b8..19e28e2056d9 100644 --- a/packages/dds/tree/src/shared-tree/treeCheckout.ts +++ b/packages/dds/tree/src/shared-tree/treeCheckout.ts @@ -3,12 +3,8 @@ * Licensed under the MIT License. */ -import { assert } from "@fluidframework/core-utils/internal"; -import type { - HasListeners, - IEmitter, - Listenable, -} from "@fluidframework/core-interfaces/internal"; +import { assert, unreachableCase } from "@fluidframework/core-utils/internal"; +import type { Listenable } from "@fluidframework/core-interfaces/internal"; import { createEmitter } from "@fluid-internal/client-utils"; import type { IIdCompressor } from "@fluidframework/id-compressor"; import { @@ -63,7 +59,7 @@ import { type SharedTreeBranchChange, type Transactor, } from "../shared-tree-core/index.js"; -import { Breakable, disposeSymbol, fail, getLast, hasSome } from "../util/index.js"; +import { Breakable, disposeSymbol, fail, getOrCreate } from "../util/index.js"; import { SharedTreeChangeFamily, hasSchemaChange } from "./sharedTreeChangeFamily.js"; import type { SharedTreeChange } from "./sharedTreeChangeTypes.js"; @@ -86,21 +82,23 @@ import { getCheckout, SchematizingSimpleTreeView } from "./schematizingTreeView. export interface CheckoutEvents { /** * The view is currently in a consistent state, but a batch of changes is about to be processed. - * @remarks After this event fires, it is safe to access the FlexTree, Forest and AnchorSet again until {@link CheckoutEvents.afterBatch} fires. - * @param appendedCommits - Any commits that were appended to the checkout's branch in order to produce the changes in this batch. + * @remarks Once this event fires, it is not safe to access the FlexTree, Forest and AnchorSet again until the corresponding {@link CheckoutEvents.afterBatch} fires. + * Every call to `beforeBatch` will be followed by a corresponding call to `afterBatch` (before any more calls to `beforeBatch`). + * @param change - The {@link SharedTreeBranchChange | change} to the checkout's active branch that is about to be processed. * May be empty if the changes were produced by e.g. a rebase or the initial loading of the document. */ - beforeBatch(appendedCommits: readonly GraphCommit[]): void; + beforeBatch(change: SharedTreeBranchChange): void; /** * A batch of changes has finished processing and the view is in a consistent state. * @remarks It is once again safe to access the FlexTree, Forest and AnchorSet. - * @param appendedCommits - Any commits that were appended to the checkout's branch in order to produce the changes in this batch. - * May be empty if the changes were produced by e.g. a rebase or the initial loading of the document. + * + * While every call to `beforeBatch` will be followed by a corresponding call to `afterBatch`, the converse is not true. + * This event may be fired without a preceding `beforeBatch` event if the checkout's branch and forest were directly updated via e.g. a summary load rather than via normal application of changes. * @remarks * This is mainly useful for knowing when to do followup work scheduled during events from Anchors. */ - afterBatch(appendedCommits: readonly GraphCommit[]): void; + afterBatch(): void; /** * Fired when a change is made to the branch. Includes data about the change that is made which listeners @@ -269,9 +267,6 @@ export function createTreeCheckout( schema?: TreeStoredSchemaRepository; forest?: IEditableForest; fieldBatchCodec?: FieldBatchCodec; - events?: Listenable & - IEmitter & - HasListeners; removedRoots?: DetachedFieldIndex; chunkCompressionStrategy?: TreeCompressionStrategy; logger?: ITelemetryLoggerExt; @@ -302,7 +297,6 @@ export function createTreeCheckout( changeFamily, () => idCompressor.generateCompressedId(), ); - const events = args?.events ?? createEmitter(); return new TreeCheckout( branch, @@ -310,7 +304,6 @@ export function createTreeCheckout( changeFamily, schema, forest, - events, mintRevisionTag, revisionTagCodec, idCompressor, @@ -372,6 +365,9 @@ export class TreeCheckout implements ITreeCheckoutFork { */ public static readonly revertTelemetryEventName = "RevertRevertible"; + readonly #events = createEmitter(); + public events: Listenable = this.#events; + public constructor( branch: SharedTreeBranch, /** True if and only if this checkout is for a forked branch and not the "main branch" of the tree. */ @@ -379,9 +375,6 @@ export class TreeCheckout implements ITreeCheckoutFork { private readonly changeFamily: ChangeFamily, public readonly storedSchema: TreeStoredSchemaRepository, public readonly forest: IEditableForest, - public readonly events: Listenable & - IEmitter & - HasListeners, private readonly mintRevisionTag: () => RevisionTag, private readonly revisionTagCodec: RevisionTagCodec, private readonly idCompressor: IIdCompressor, @@ -418,8 +411,18 @@ export class TreeCheckout implements ITreeCheckoutFork { // When each transaction is started, take a snapshot of the current state of removed roots const removedRootsSnapshot = this.removedRoots.clone(); return (result) => { - if (result === TransactionResult.Abort) { - this.removedRoots = removedRootsSnapshot; + switch (result) { + case TransactionResult.Abort: + this.removedRoots = removedRootsSnapshot; + break; + case TransactionResult.Commit: + if (!this.transaction.isInProgress()) { + // The changes in a transaction squash commit have already applied to the checkout and are known to be valid, so we can validate the squash commit automatically. + this.validateCommit(this.#transaction.branch.getHead()); + } + break; + default: + unreachableCase(result); } forks.forEach((fork) => fork.dispose()); @@ -467,39 +470,26 @@ export class TreeCheckout implements ITreeCheckoutFork { }; let withinEventContext = true; - this.events.emit("changed", { isLocal: true, kind }, getRevertible); + this.#events.emit("changed", { isLocal: true, kind }, getRevertible); withinEventContext = false; } } else if (this.isRemoteChangeEvent(event)) { // TODO: figure out how to plumb through commit kind info for remote changes - this.events.emit("changed", { isLocal: false, kind: CommitKind.Default }); + this.#events.emit("changed", { isLocal: false, kind: CommitKind.Default }); } }); - this.#transaction.activeBranchEvents.on("beforeChange", this.onBeforeChange); + this.#transaction.activeBranchEvents.on("afterChange", this.onAfterChange); this.#transaction.activeBranchEvents.on("ancestryTrimmed", this.onAncestryTrimmed); } - private readonly onBeforeChange = ( - event: SharedTreeBranchChange, - ): void => { - // We subscribe to `beforeChange` rather than `afterChange` here because it's possible that the change is invalid WRT our forest. - // For example, a bug in the editor might produce a malformed change object and thus applying the change to the forest will throw an error. - // In such a case we will crash here, preventing the change from being added to the commit graph, and preventing `afterChange` from firing. - // One important consequence of this is that we will not submit the op containing the invalid change, since op submissions happens in response to `afterChange`. + private readonly onAfterChange = (event: SharedTreeBranchChange): void => { + this.#events.emit("beforeBatch", event); if (event.change !== undefined) { - this.events.emit("beforeBatch", event.type === "append" ? event.newCommits : []); - - let revision: RevisionTag | undefined; - if (event.type === "rebase") { - assert( - hasSome(event.newCommits), - 0xa96 /* Expected new commit for non no-op change event */, - ); - revision = getLast(event.newCommits).revision; - } else { - revision = event.change.revision; - } + const revision = + event.type === "rebase" + ? this.#transaction.activeBranch.getHead().revision + : event.change.revision; // Conflicts due to schema will be empty and thus are not applied. for (const change of event.change.change.changes) { @@ -529,7 +519,10 @@ export class TreeCheckout implements ITreeCheckoutFork { fail("Unknown Shared Tree change type."); } } - this.events.emit("afterBatch", event.type === "append" ? event.newCommits : []); + } + this.#events.emit("afterBatch"); + if (event.type === "append") { + event.newCommits.forEach((commit) => this.validateCommit(commit)); } }; @@ -709,7 +702,6 @@ export class TreeCheckout implements ITreeCheckoutFork { this.changeFamily, storedSchema, forest, - createEmitter(), this.mintRevisionTag, this.revisionTagCodec, this.idCompressor, @@ -717,7 +709,7 @@ export class TreeCheckout implements ITreeCheckoutFork { this.logger, this.breaker, ); - this.events.emit("fork", checkout); + this.#events.emit("fork", checkout); return checkout; } @@ -790,7 +782,7 @@ export class TreeCheckout implements ITreeCheckoutFork { for (const view of this.views) { view.dispose(); } - this.events.emit("dispose"); + this.#events.emit("dispose"); } public getRemovedRoots(): [string | number | undefined, number, JsonableTree][] { @@ -811,11 +803,16 @@ export class TreeCheckout implements ITreeCheckoutFork { } /** - * This sets the tip revision as the latest relevant revision for any removed roots that are loaded from a summary. - * This needs to be called right after loading {@link this.removedRoots} from a summary to allow loaded data to be garbage collected. + * This must be called on the root/main checkout after loading from a summary. + * @remarks This pattern is necessary because the EditManager skips the normal process of applying commits to branches when loading a summary. + * Instead, it simply {@link SharedTreeBranch#setHead | mutates} the branches directly which does not propagate the typical events throughout the rest of the system. */ - public setTipRevisionForLoadedData(revision: RevisionTag): void { - this.removedRoots.setRevisionsForLoadedData(revision); + public load(): void { + // Set the tip revision as the latest relevant revision for any removed roots that are loaded from a summary - this allows them to be garbage collected later. + // When a load happens, the head of the trunk and the head of the local/main branch must be the same (this is enforced by SharedTree). + this.removedRoots.setRevisionsForLoadedData(this.#transaction.branch.getHead().revision); + // The content of the checkout (e.g. the forest) has (maybe) changed, so fire an afterBatch event. + this.#events.emit("afterBatch"); } private purgeRevertibles(): void { @@ -915,4 +912,46 @@ export class TreeCheckout implements ITreeCheckoutFork { event.type === "rebase" ); } + + // #region Commit Validation + + /** Used to maintain the contract of {@link onCommitValid}(). */ + #validatedCommits = new WeakMap< + GraphCommit, + ((commit: GraphCommit) => void)[] | true + >(); + + /** + * Registers a function to be called when the given commit is validated. + * @remarks A commit is validated by the checkout after it has been applied to the checkout's state (e.g. it has an effect on the forest). + * If the commit applies successfully (i.e. it does not raise any unexpected errors), the commit is considered valid and the registered function is called. + * If the commit does not apply successfully (because it causes an unexpected error), the function is not called (and the checkout will left in an error state). + * + * If the commit has already been validated when this function is called, the function is called immediately and this function returns `true`. + * Otherwise, the function is registered to be called later and this function returns `false`. + */ + public onCommitValid( + commit: GraphCommit, + fn: (commit: GraphCommit) => void, + ): boolean { + const validated = getOrCreate(this.#validatedCommits, commit, () => []); + if (validated === true) { + fn(commit); + return true; + } + + validated.push(fn); + return false; + } + + /** Mark the given commit as "validated" according to the contract of {@link onCommitValid}(). */ + private validateCommit(commit: GraphCommit): void { + const validated = getOrCreate(this.#validatedCommits, commit, () => []); + if (validated !== true) { + validated.forEach((fn) => fn(commit)); + this.#validatedCommits.set(commit, true); + } + } + + // #endregion Commit Validation } diff --git a/packages/dds/tree/src/test/shared-tree-core/edit-manager/editManagerCorrectness.test.ts b/packages/dds/tree/src/test/shared-tree-core/edit-manager/editManagerCorrectness.test.ts index 308a6b4d8a8d..a2a0aba010ed 100644 --- a/packages/dds/tree/src/test/shared-tree-core/edit-manager/editManagerCorrectness.test.ts +++ b/packages/dds/tree/src/test/shared-tree-core/edit-manager/editManagerCorrectness.test.ts @@ -722,10 +722,11 @@ function applyBranchCommit( intention: number | number[] = [], ): Commit { const revision = mintRevisionTag(); - const [_, commit] = branch.apply({ + branch.apply({ change: TestChange.mint(inputContext, intention), revision, }); + const commit = branch.getHead(); return { change: commit.change, revision: commit.revision, diff --git a/packages/dds/tree/src/test/util/utils.spec.ts b/packages/dds/tree/src/test/util/utils.spec.ts index 830040e1b805..ebfb79081817 100644 --- a/packages/dds/tree/src/test/util/utils.spec.ts +++ b/packages/dds/tree/src/test/util/utils.spec.ts @@ -5,7 +5,12 @@ import { strict as assert } from "node:assert"; -import { capitalize, mapIterable, transformObjectMap } from "../../util/index.js"; +import { + capitalize, + defineLazyCachedProperty, + mapIterable, + transformObjectMap, +} from "../../util/index.js"; import { benchmark } from "@fluid-tools/benchmark"; describe("Utils", () => { @@ -50,4 +55,19 @@ describe("Utils", () => { const m = new Map(mapIterable(testMap, ([k, v]) => [k, v] as const)); }, }); + + it("defineLazyCachedProperty", () => { + const obj = {}; + let count = 0; + const objWithProperty = defineLazyCachedProperty(obj, "prop", () => { + count += 1; + return 3; + }); + + assert.equal(count, 0); + assert.equal(objWithProperty.prop, 3); + assert.equal(count, 1); + assert.equal(objWithProperty.prop, 3); + assert.equal(count, 1); + }); }); diff --git a/packages/dds/tree/src/util/index.ts b/packages/dds/tree/src/util/index.ts index 2c1c14a9dae1..efa0b6960a90 100644 --- a/packages/dds/tree/src/util/index.ts +++ b/packages/dds/tree/src/util/index.ts @@ -93,6 +93,7 @@ export { getLast, hasSome, hasSingle, + defineLazyCachedProperty, } from "./utils.js"; export { ReferenceCountedBase, type ReferenceCounted } from "./referenceCounting.js"; diff --git a/packages/dds/tree/src/util/utils.ts b/packages/dds/tree/src/util/utils.ts index 8fa6985d136d..3407441c83d3 100644 --- a/packages/dds/tree/src/util/utils.ts +++ b/packages/dds/tree/src/util/utils.ts @@ -537,3 +537,29 @@ export function capitalize(s: S): Capitalize { export function compareStrings(a: T, b: T): number { return a > b ? 1 : a === b ? 0 : -1; } + +/** + * Defines a property on an object that is lazily initialized and cached. + * @remarks This is useful for properties that are expensive to compute and it is not guaranteed that they will be accessed. + * This function initially defines a getter on the object, but after first read it replaces the getter with a value property. + * @param obj - The object on which to define the property. + * @param key - The key of the property to define. + * @param get - The function (called either once or not at all) to compute the value of the property. + * @returns `obj`, typed such that it has the new property. + * This allows for the new property to be read off of `obj` in a type-safe manner after calling this function. + */ +export function defineLazyCachedProperty< + T extends object, + K extends string | number | symbol, + V, +>(obj: T, key: K, get: () => V): typeof obj & { [P in K]: V } { + Reflect.defineProperty(obj, key, { + get() { + const value = get(); + Reflect.defineProperty(obj, key, { value }); + return value; + }, + configurable: true, + }); + return obj as typeof obj & { [P in K]: V }; +}