diff --git a/.changeset/tough-lies-say.md b/.changeset/tough-lies-say.md new file mode 100644 index 000000000000..f84e12024f8f --- /dev/null +++ b/.changeset/tough-lies-say.md @@ -0,0 +1,43 @@ +--- +"@fluidframework/tree": minor +"__section": tree +--- +Starting a transaction inside a tree-change event listener now produces a usage error + +Calling `runTransaction` or `runTransactionAsync` from inside a `nodeChanged` or `treeChanged` event listener now throws a `UsageError`. +This brings transactions into line with the existing rule that tree mutations from inside a change-event listener are forbidden. + +Before this change, starting a transaction from a listener would push a transaction frame onto the running outer transaction's bookkeeping before the inner edit was attempted, +leaving the tree's transaction labels in a corrupted state even when the inner edit itself threw. +The new check rejects the call before any state mutation, so the tree's transaction labels remain consistent. + +The error message follows the existing format used by the other listener-time guards: + +``` +Running a transaction is forbidden during a nodeChanged or treeChanged event +``` + +After this error is thrown, the affected tree view enters a broken state and subsequent operations on it will throw an `Invalid use of ... after it was put into an invalid state` error. +Discard the view and acquire a fresh one to recover. + +#### Migration + +If you start a transaction in response to a tree change, defer it out of the listener: + +```typescript +// Before — throws at runtime +Tree.on(view.root, "nodeChanged", () => { + view.runTransaction(() => { + // ... + }); +}); + +// After — schedule the transaction outside the listener +Tree.on(view.root, "nodeChanged", () => { + queueMicrotask(() => { + view.runTransaction(() => { + // ... + }); + }); +}); +``` diff --git a/packages/dds/tree/src/shared-tree/treeCheckout.ts b/packages/dds/tree/src/shared-tree/treeCheckout.ts index 13ba631cafae..b6218a65784a 100644 --- a/packages/dds/tree/src/shared-tree/treeCheckout.ts +++ b/packages/dds/tree/src/shared-tree/treeCheckout.ts @@ -888,6 +888,11 @@ export class TreeCheckout implements ITreeCheckout { private mountTransaction(params: RunTransactionParams | undefined, isAsync: boolean): void { this.checkNotDisposed(); + // Starting a transaction during a nodeChanged/treeChanged event is forbidden for the same + // reason direct edits are: it would push a label frame and (typically) commit edits while + // the tree's invariants are mid-flight. Without this guard, nested transactions started + // from a listener leak label frames into the running outer transaction's label tree + this.editLock.checkUnlocked("Running a transaction"); if (isAsync && this.transaction.size > 0) { throw new UsageError( "An asynchronous transaction cannot be started while another transaction is already in progress.", diff --git a/packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts b/packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts index 09ca13834efb..1678455b82c7 100644 --- a/packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts +++ b/packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts @@ -1357,6 +1357,87 @@ describe("sharedTreeView", () => { error: "Disposing a view is forbidden during a nodeChanged or treeChanged event", }); }); + + it("run a transaction", () => { + // Without the guard, a runTransaction call from inside a listener pushes a label + // frame and starts a nested transaction before the inner edit's lock check fires, + // corrupting the outer transaction's label tree. The guard rejects at mount time. + expectErrorDuringEdit({ + duringEdit: (view) => + view.runTransaction(() => { + view.root.number = 4; + }), + error: "Running a transaction is forbidden during a nodeChanged or treeChanged event", + }); + }); + + it("run an async transaction", async () => { + // runTransactionAsync is declared `async`, so the synchronous throw from + // mountTransaction surfaces as a rejected promise rather than a thrown error. + // Capture the promise in the listener and assert rejection after the outer edit. + const view = getView( + new TreeViewConfiguration({ enableSchemaValidation, schema: NumberNode }), + ); + view.initialize({ number: 3 }); + + let asyncResult: Promise | undefined; + Tree.on(view.root, "nodeChanged", () => { + asyncResult = view.runTransactionAsync(async () => { + view.root.number = 4; + }); + }); + + view.root.number = 0; + assert(asyncResult !== undefined, "Expected the listener to start a transaction"); + await assert.rejects( + asyncResult, + validateUsageError( + /Running a transaction is forbidden during a nodeChanged or treeChanged event/, + ), + ); + }); + + it("leaves the tree in a broken state after run-transaction-during-listener throws", () => { + // runTransaction is decorated with `@breakingMethod`, so a throw from inside it + // (here: the new `checkUnlocked` call in `mountTransaction`) puts the tree into + // the `Breakable` "broken" state. After that, every method decorated with + // `@breakingMethod` / `@throwIfBroken` rejects with "Invalid use of ... after it + // was put into an invalid state". This test documents that contract so future + // refactors that move `mountTransaction` outside the decorated method's reach + // (or remove the decorator) fail loudly. + const view = getView( + new TreeViewConfiguration({ enableSchemaValidation, schema: NumberNode }), + ); + view.initialize({ number: 3 }); + + Tree.on(view.root, "nodeChanged", () => { + view.runTransaction(() => { + view.root.number = 4; + }); + }); + + assert.throws( + () => (view.root.number = 0), + validateUsageError( + /Running a transaction is forbidden during a nodeChanged or treeChanged event/, + ), + ); + + // Subsequent operations should all throw with the broken-state message. + assert.throws( + () => (view.root.number = 5), + validateUsageError(/invalid state/), + "direct edit after listener throw should be blocked", + ); + assert.throws( + () => + view.runTransaction(() => { + view.root.number = 6; + }), + validateUsageError(/invalid state/), + "runTransaction after listener throw should be blocked", + ); + }); }); describe("Enrichment", () => {