microsoft / microsoft/FluidFramework
Invalid transaction customMetadata breaks the whole checkout instead of throwing recoverably
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 4.9k
- Forks
- 586
- Avg merge
- 1d 15h
- Merged PRs (30d)
- 146
Description
Describe the bug
Rejecting an invalid customMetadata value passed to TreeViewAlpha.runTransaction / runTransactionAsync puts the entire checkout into a broken state, rather than being a recoverable usage error. Every subsequent operation on the view then fails with Invalid use of ... after it was put into an invalid state by another error.
This is unfortunate because a malformed customMetadata is documented user input error — the same class of mistake as passing a bad value to any other API — and the value is validated before any state is mutated, so nothing is actually left inconsistent.
To Reproduce
Steps to reproduce the behavior:
- Create a
TreeViewAlpha. - Call
view.runTransaction(() => { ... }, { customMetadata: cyclic })wherecyclicis an object containing a cycle (or any non-JSON-serializable value). - Observe the expected
UsageError: Transaction "customMetadata" must be JSON-serializable: Converting circular structure to JSON. - Perform any further valid edit on
view— e.g.view.runTransaction(() => view.root.insertAtEnd("x")). - See
UsageError: Invalid use of SchematizingSimpleTreeView after it was put into an invalid state by another error.
Expected behavior
The invalid value is rejected with a UsageError and the view remains fully usable, the same way a rejected direct edit throws recoverably.
Cause and suggested fix
snapshotCustomMetadata runs inside TreeCheckout.mountTransaction, which is reached through @breakingMethod-decorated entry points. Any throw inside a breaking method marks the shared Breakable as broken.
Fixing it at the TreeCheckout layer alone is not sufficient: SchematizingSimpleTreeView is decorated with @breakingClass, so all of its methods break on throw, and it shares the checkout's Breakable (this.breaker = checkout.breaker). The view is what applications actually call.
The likely fix is to have the view's two runTransaction methods opt out of automatic breaking with @throwIfBroken (which breakingClass deliberately skips) and validate the metadata before delegating to the checkout. That appears safe for the transaction path itself, since the checkout's inner method remains @breakingMethod and still breaks on genuine failures — but it also means calling runTransaction on a disposed view would no longer poison the shared breaker, so the change needs its own review and test pass.
This was deliberately deferred out of PR #28064 to keep that change scoped to the custom commit metadata feature; the limitation is noted in a comment on TreeCheckout.mountTransaction.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start at TreeCheckout.mountTransaction and the SchematizingSimpleTreeView.runTransaction and runTransactionAsync entry points, then inspect the breakingClass, breakingMethod, and throwIfBroken behavior. Reproduce with cyclic customMetadata and verify that the view remains usable after the UsageError. Confirm that genuine transaction failures still break the view and review the disposed-view behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- api
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100