Skip to content

Invalid transaction customMetadata breaks the whole checkout instead of throwing recoverably #28085

Description

@noencke

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions