From 322ae71c809b91aa403c8a0b6fffde566c5c478a Mon Sep 17 00:00:00 2001 From: daesunp <138815173+daesunp@users.noreply.github.com> Date: Tue, 12 May 2026 16:27:21 +0000 Subject: [PATCH 1/3] edit lock for transactions in callbacks --- .../dds/tree/src/shared-tree/treeCheckout.ts | 5 ++ .../src/test/shared-tree/treeCheckout.spec.ts | 81 +++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/packages/dds/tree/src/shared-tree/treeCheckout.ts b/packages/dds/tree/src/shared-tree/treeCheckout.ts index 13ba631cafae..7503293ae51f 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", () => { From 7dbdaddd2bf64d7c56a0a0eaf4b97856e07fe541 Mon Sep 17 00:00:00 2001 From: daesunp <138815173+daesunp@users.noreply.github.com> Date: Tue, 12 May 2026 17:03:05 +0000 Subject: [PATCH 2/3] comment change --- packages/dds/tree/src/shared-tree/treeCheckout.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dds/tree/src/shared-tree/treeCheckout.ts b/packages/dds/tree/src/shared-tree/treeCheckout.ts index 7503293ae51f..b6218a65784a 100644 --- a/packages/dds/tree/src/shared-tree/treeCheckout.ts +++ b/packages/dds/tree/src/shared-tree/treeCheckout.ts @@ -891,7 +891,7 @@ export class TreeCheckout implements ITreeCheckout { // 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. + // 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( From a897dc3235f0b2906881566f6f412e73e960de5e Mon Sep 17 00:00:00 2001 From: daesunp <138815173+daesunp@users.noreply.github.com> Date: Mon, 18 May 2026 17:29:06 +0000 Subject: [PATCH 3/3] Changeset --- .changeset/tough-lies-say.md | 43 ++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 .changeset/tough-lies-say.md 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(() => { + // ... + }); + }); +}); +```