Is bug? is bugfix? :: affecting ember-source 5.6+ with false-negatives w/ backtracking re-render assertions

[NullVoxPopuli]

Reproduction in Limber (locally running on port 4301)

Notes:

• PR in ember-source that caused the issue: Upgrade VM @glimmer/* packages from 0.84.3 to 0.85.13 emberjs/ember.js#20561
  • This PR is mostly type/TS updates
  • Logical changes do exist:
    • https://github.com/emberjs/ember.js/pull/20561/files#diff-8f5db53a98378f58f0b4aba1b1f12f38cd3872c97d540860e86a3fd781627b18R365-R373
    • https://github.com/emberjs/ember.js/pull/20561/files#diff-cdf95f0c6c10226a54fbac271478aaa1defd908f0f998fec0f484e52d2544184R339
    • maybe: https://github.com/emberjs/ember.js/pull/20561/files#diff-efd3a6bdc0bb9c370a49d76a9b94e3415d2c1f2fe9c5882c3d2f53c70d9ff158R13
• There are two places where the backtracking rerendering message is defined:
  • in Glimmer VM
  • and in ember-source (overrides the VM message)

---

Examples of real code that "broke" in ember 5.6+ (which, imo, is legit problematic, and should be migrated away from)

• creating records in class properties

  @tracked createdModel = this.store.createRecord('model-name');

  this is problematic because createRecord interacts with "Live arrays", which have previously been used on the pages (imagine a table view with a navigatable create modal).
  • What should happen instead: folks should only call createRecord as the result of a user action, such as submitting a form.
• Setting data outside of the current context / component as a result of construction

  constructor(owner, args) {
    super(owner, args);
    
    if (this.args.options.length === 1) {
      this.setType(this.args.options[0]);
    }
  }
  
  selectedRelatedData = this.args.relatedData;
  
  setType = (option) => {
    // option is tracked
    this.selectedRelatedData.option = option;
  }

  • What should happen instead: is that selectedRelatedData should be derived, as should the type, potentially using localCopy from tracked-toolbox if local state forking needing.
    for example:

    get _type() {
      if (this.args.options.length === 1) {
        return this.args.options[0];
      }
    }
    
    @localCopy('_type') type;

• making requests on reactive data in the constructor

  constructor() {
    super(...arguments);
    this.setup();
  }
  
  setup = () => {
    this.args.model.data?.reload();
  }

  This has the same problem as above, where the data can be a part of a live array (or other structure) elsewhere on the page.
  • What to do instead: fetch data earlier (such as in the model hook), or wait to fetch until a user interaction.
• Changing the route upon construction of a component

  constructor() {
    super(...arguments);
    
    this.router.transitionTo(`route.name.here`);
  }

  • What to do instead: either don't even have this component in the first place, or have whatever is wanting to render this component just do the transition instead (for example, as the result of a user interaction)

Examples of real code that "broke" in ember 5.6+ which maybe shouldn't have

... trying to find reproductions ...

[evoactivity]

Reproduction repo https://github.com/evoactivity/backtracking-repro/blob/main/app/components/thing/index.js

[krisselden]

@NullVoxPopuli this is as designed, you are reading and writing to a tracked property in one render pass. This makes it so that there are potentially 2 versions of the same tracked property. There isn't any way for it to know you are not using the prior version you read.

There are also simple refactors to avoid it. Most of this is because you want a tracked property for local changes based on derived state. You can create a new instance with a tracked property and cache it to the source tracked. Like tracked url to derived class State tracked isLoading = true, cached on url ==. This lets you set isLoading to false when it finishes and if the url changes, the cache invalidates and you get a new State for the new url initialized to isLoading = true.

[krisselden]

This issue comes up because the old assertion was tied to a chain of state tied all the way into the rendering. As things move to tracked, rendering is basically cached on the source versions of tracked and doesn't book keep all the way to rendering. so the more systems that refactor to rely on autotracking the more you hit cases where people are reading during render and even if they aren't using that read value in any derived state, autotracking can't assume that.

[NullVoxPopuli]

I think the reproductions I have are not reproductions, because they fail on 0.84.3 as well.

[boris-petrov]

I'm not sure I understand correctly what you're talking about here but could this also be what you're looking for as a reproduction?

[wycats]

the more systems that refactor to rely on autotracking the more you hit cases where people are reading during render and even if they aren't using that read value in any derived state, autotracking can't assume that

@krisselden A lot of the time, we're talking about patterns where people are reading values as part of a larger process that doesn't actually depend on the value (e.g. lazy initialization, memoization).

An Aside About Autotracking Safety (and untracked)

In these cases, the value is guaranteed to be read exactly once per render transaction, and always for the purpose of producing the same answer for all calls during that render transaction. Many common use-cases for this kind of pattern are captured in tracked-toolbox, although the code in tracked-toolbox is subtle enough that it's often hard to tell what's going on by reading the code.

These patterns work because they follow the basic safety invariants of autotracking that we haven't ever fully formalized. It's something like this:

When internal storage for a reactive value is accessed, it must either consume the tag associated with the value or:

1. guarantee that it returns an identical JavaScript value for all accesses to the reactive value in the same transaction.
2. consume a tag that will invalidate when a future action changes modifies the reactive value.

Our built-in storage types (@tracked, @cached, Tracked Builtins and the lower-level APIs like createCache) all present a public API surface that satisfies these invariants.

The decorators in tracked-toolbox are implementations of additional (more ad-hoc) patterns that also satisfy these safety invariants.

In general, all of these APIs could be implemented in terms of a "Peekable Cell" primitive (and tracked-toolbox is largely implemented in terms of a subtle version of a peekable cell).

In contrast, using untrack generally cannot satisfy these safety invariants in any cases where "Peekable Cell" wouldn't work, because untrack wraps an entire call stack, and applies to code that you don't control. If it only applied to code you did control, the peek pattern (and manual verification of the safety invariants) would be sufficient.

But if you turn off tracking on code you don't control, then you can't be sure that any accesses to those reactive values "don't matter".

The peekable pattern uses untracked internal state internally and then wraps it in an explicit tag that invalidates whenever the higher-level construct actually changes.

The untracked pattern uses tracked storage as an internal implementation details, and then disables tracking for completely unrelated reactive values so that accesses to the internal state are sometimes treated as internal.

What I think is going on

I think that @krisselden is right to say that this problem is caused by a more consistent use of auto-tracking in our internals and ecosystem. This is surfacing situations where people are accessing tracked storage and were relying on implementation details of the rendering engine to swallow those accesses.

In some cases, these changes are just catching bugs where certain changes to reactive storage didn't end up being reflected in the DOM at all.

In other cases, a human reader of the code in question would feel very justified in saying that the access "doesn't matter!" and want the rendering engine to swallow it. But these cases are rather brittle: small refactorings to the code in question could result in backtracking assertions without a way for the user to rationalize why the error suddenly appeared.

What I think we should do

In the short-term, I think that we should:

1. Document the safety rule that applies to peekable storage and verify that it applies to all of the tracked-toolbox APIs.
2. Help people identify and remedy situations where the backtracking error represented a straight-up bug (i.e. the DOM doesn't always represent the current state of the reactive value)
3. Evangelize the use of tracked-toolbox and its patterns for situations where the old behavior wasn't hiding a bug. Using tracked-toolbox in these situations is much less mysterious and won't turn seemingly innocuous refactors into a confusing game of whack-a-mole.

In the medium-term, I think that we should create and document a PeekableCell primitive, refactor tracked-toolbox to use it, and offer it as a power tool for people building their own reactive primitives.

The audience for PeekableCell is not the general Ember audience, and people who use it will need to be prepared to understand and enforce the safety guarantees of our reactivity system. But it will give people who are not working on blessed reactive primitives the tools and documentation necessary to implement patterns that are not already captured in tracked-toolbox and feel confident that they've done it effectively.

(It's already possible to implement PeekableCell in userspace. That's how tracked-toolbox works. But there are many subtly different implementations of the pattern out there, and it's difficult to clearly document the invariants in terms of an abstract pattern implemented in so many ways).

In a sense, this is the same kind of problem that Rust faces with its unsafe features, except that the consequence of "unsafe reactivity" is "the DOM didn't update when it should" and the consequence of unsafe memory access is a dangerous segfault.

---

I really like the fact that it's possible to understand our reactivity system so coherently. Unlike similar systems, we managed to phase out our observers (what other systems call watch) and almost nobody ever thinks about untrack.

It's a real achievement that we can call it a clear bug if the DOM doesn't reflect the current value of reactive state that was used to build it.

That achievement includes tracked-builtins and tracked-toolbox. If you use state from one of those sources in your component and the DOM doesn't reflect the current state, it's a clear bug. Both of them are doing fairly novel things and neither of them uses watch or untrack internally.

In my opinion, the path forward is:

• clearly documenting the safety invariants that make this possible
• evangelizing the high-level patterns that satisfy those invariants
• creating a new public API to make it easy for people to build new reactive primitives that behave safely

When I build apps, I treasure the simplicity of the Ember reactivity model, and I think we should take this bug as a challenge to smooth out the remaining rough edges.

[wycats]

One additional invariant that I remembered when re-reading tracked-toolbox just now to verify compliance.

3. Internal state may be freely accessed and mutated while the reactive value is initializing. Other reactive state must not be accessed or mutated during initialization.

A reactive value is "initializing" if you can prove that the value was not yet accessed by external code. A reactive value's revision is initialized to the current global revision at the end of initialization.

Since reactive state must not be mutated during initialization, the revision at the beginning of initialization is the same as the revision at the end of initialization.