edit lock for transactions in callbacks

.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(() => {
+			// ...
+		});
+	});
+});
+```

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.",

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<unknown> | 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", () => {