microsoft / microsoft/FluidFramework

Invalid transaction customMetadata breaks the whole checkout instead of throwing recoverably

Open
#28,085 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
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:

  1. Create a TreeViewAlpha.
  2. Call view.runTransaction(() => { ... }, { customMetadata: cyclic }) where cyclic is an object containing a cycle (or any non-JSON-serializable value).
  3. Observe the expected UsageError: Transaction "customMetadata" must be JSON-serializable: Converting circular structure to JSON.
  4. Perform any further valid edit on view — e.g. view.runTransaction(() => view.root.insertAtEnd("x")).
  5. 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.