bug: async-to-async relationships may error when using unloadAll on both types

[elfin-sbreuers]

Reproduction

I am observing the following error with ember-data version 4.12.4

vendor.js:16753 Uncaught (in promise) Error: Assertion Failed: Expected a stable identifier
    at assert (vendor.js:16753:15)
    at LegacyWrapper.disconnectRecord (vendor.js:87530:73)
    at JSONAPICache.unloadRecord (vendor.js:70505:26)
    at vendor.js:87974:19
    at Store._join (vendor.js:90492:11)
    at InstanceCache.unloadRecord (vendor.js:87959:20)
    at vendor.js:88012:22
    at Map.forEach (<anonymous>)
    at InstanceCache.clear (vendor.js:88010:25)
    at vendor.js:91938:36

It happens when using unloadRecord and some of the models are only partly loaded by being referenced in the relationships of an already loaded model.

import Model from '@ember-data/model';
import { hasMany } from '@ember-data/model';

export default class FooModel extends Model {
  @hasMany('bar', { async: true, inverse: 'foos' }) bars;
}

//app/models/bar.js
import Model from '@ember-data/model';
import { hasMany } from '@ember-data/model';

export default class BarModel extends Model {
  @hasMany('foo', { async: true, inverse: 'bars' }) foos;
}

import Route from '@ember/routing/route';
import { inject as service } from '@ember/service';

export default class TestRoute extends Route {
  @service store;
  model() {
    this.store.push({
      data: [
        {
          id: 1,
          type: 'foo',
          relationships: { bars: { data: [{ id: 1, type: 'bar' }] } },
        },
        // Commenting that in would solve the issue.
        // Meaning having the related resource fully loaded.
        /*{
          id: 1,
          type: 'bar',
          relationships: { foos: { data: [{ id: 1, type: 'foo' }] } },
        },*/
      ],
    });
    ['foo', 'bar'].forEach((modelName) => this.store.unloadAll(modelName));
    return this.store.peekAll('foo');
  }
}

The problem can be solved (or worked around) if the model the relationship points to is fully loaded by using include in the request or just loading the partly loaded model pointed to by the relationship directly.

It would be nice if the related models do not need to be fully loaded first before being able to clean the store from models, that are just partly loaded.

Version

bug@0.0.0 /home/breuerss/devel/flexypage/bug
├─┬ @ember/test-helpers@2.9.4
│ ├─┬ @embroider/util@1.12.1
│ │ └── ember-source@4.12.3 deduped
│ └── ember-source@4.12.3 deduped
├─┬ ember-data@4.12.4
│ └─┬ @ember-data/model@4.12.4
│   └─┬ ember-cached-decorator-polyfill@1.0.2
│     └── ember-source@4.12.3 deduped
└── ember-source@4.12.3

bug@0.0.0 /home/breuerss/devel/flexypage/bug
├─┬ ember-cli-dependency-checker@3.3.2
│ └── ember-cli@4.12.2 deduped
└── ember-cli@4.12.2

bug@0.0.0 /home/breuerss/devel/flexypage/bug
└── ember-data@4.12.4

A fix for the 4.12 LTS would be much appreciated. If you need any help reproducing the issue I would gladly assist.

[runspired]

this is an error in your payload, ids should be strings. If that is fixed the error will go away.

[elfin-sbreuers]

Could you point me in the direction of the document or discussion, where you decided that you do no longer support numeric IDs? I would like to understand this decision.

I am a bit confused, especially since everything else continues to work except for the unloadRecord, and the JSONAPI standard (https://jsonapi.org/) we developed our API against five years ago still supports numeric IDs.

Any pointers would be appreciated.

[runspired]

EmberData has never supported numeric IDs internally, we’ve only ever supported them in a few APIs where we immediately recast them to string form (the signatures of those APIs now deprecate if they receive a number). Push was not one of these, push expects the final normalized form which means it must exactly match cache format. Cache format for the JSON:API cache is string IDs.

also note that JSON:API itself does not support numeric IDs. Here’s the relevant portion of the spec: https://jsonapi.org/format/#document-resource-object-identification

[runspired]

also fwiw I pulled down and ran the reproduction locally. I encountered an error (but not the one you've reported here) due to an extraneous comma, which once fixed resulted in the whole thing working as expected with no errors.

[elfin-sbreuers]

Thank you for your response. I will check again.

[elfin-sbreuers]

@runspired We looked further into it. The usage of stringified IDs actually solved the described use case and the repro repo, I guess the caching fooled my.

Unfortunately, when we updated our API to provide IDs as strings the error message did not vanish as expected. Looking further into it we discovered that we have circular relationships. For example a university has a study-group and a student is member of the university and of a study-group. All relationships are expressed with inverses.

If the models are fully loaded the unloading works properly. But if we load a university and a study-group and they both reference a student the unloading fails.

I updated the repository accordingly. Do you have another tip how this issue can be solved (without loading the models completely). Thanks again for your time.

[runspired]

I think at this point a good question is why are you unloading in the first place? The unload APIs are (broad strokes) a mega-hack when used outside of very specific contexts and often leave your app in an incorrect state if you don't perform the right surgery. Knowing the use case there's a decent probability I can point you to a better pattern.

Short of that, I would need to know which record you are unloading (student, university, or study-group) and relationships are sync vs async, as we treat those differently during unload.

[elfin-sbreuers]

Thanks for your answer. I guess the use case we have is comparable to an impersonate user feature. When I am for example an admin and I want to impersonate another user I do not want to see the content I have normally access to (and was already loaded) but the content the impersonated user could see. We want to keep the model hooks in the routes the same, i.e. using findAll and the API decides the data the requesting user has access to.

I don't know anymore when we introduced the unloading of records if it was with our Ember 2 or Ember 3.4 implementation of the app. But it made sure that records the user should not have access to are not still accidentally in the store. This worked beautifully at least up to version 4.6 of ember-data. That is the version we now fell back on although we wanted to upgrade to the LTS version.

We defined the relationships as async. And to be honest it would be all the records. That is also reflected in the repro repo though the problem is condensed to the error case. If there won't be a solution that could be provided centrally it would be helpful to know how we can implement a workaround for the different cases you have in mind (student, university, and study-group).
I think the current problematic case is university and study-group are loaded, both reference the same student, which is not loaded. When unloading university everything is fine, when we unload study-group the problem occurs. It feels like ember-data is unloading or cleaning an identifier from the IDENTIFIER set that is still in use as partial relationship by another model.

// app/models/bar.js
import Model from '@ember-data/model';
import { hasMany } from '@ember-data/model';

export default class BarModel extends Model {
  @hasMany('foo', { async: true, inverse: 'bars' }) foos;
  @hasMany('baz', { async: true, inverse: 'bars' }) bazs;
}

// app/models/baz.js
import Model from '@ember-data/model';
import { hasMany } from '@ember-data/model';

export default class BazModel extends Model {
  @hasMany('bar', { async: true, inverse: 'bazs' }) bars;
  @hasMany('foo', { async: true, inverse: 'bazs' }) foos;
}

// app/models/foo.js
import Model from '@ember-data/model';
import { hasMany, belongsTo } from '@ember-data/model';

export default class FooModel extends Model {
  @belongsTo('bar', { async: true, inverse: 'foos' }) bar;
  @hasMany('baz', { async: true, inverse: 'foos' }) bazs;
}

// app/routes/test.js
import Route from '@ember/routing/route';
import { inject as service } from '@ember/service';

export default class TestRoute extends Route {
  @service store;
  model() {
    this.store.push({
      data: [
        {
          id: '1',
          type: 'foo',
          relationships: {
              bar: { data: { id: '1', type: 'bar' } },
          },
        },
        {
          id: '1',
          type: 'baz',
          relationships: {
              bars: { data: [{ id: '1', type: 'bar' }] },
          },
        }
       // Addind this would work aorund the issue
        /*,{
          id: '1',
          type: 'bar',
          relationships: {
              foos: { data: [{ id: '1', type: 'foo' }] },
              fazs: { data: [{ id: '1', type: 'faz' }] }
          },
        }*/
      ],
    });
    ['foo', 'bar', 'baz'].forEach((modelName) => this.store.unloadAll(modelName));
    return this.store.peekAll('foo');
  }
}

Your input is much appreciated. Thanks for your efforts.

[runspired]

Not making any promises but I'll open this issue and check the provided test against main and 4.12.

TBH the behavior as described may (or may not) be intended to be supported and a bit of it will have to be a judgement call based on the mechanism of failure.

Some back context:

unload is not a "get rid of this thing" API. It is a poorly named method for "attempt to GC this record" (and in that spirit we are likely to get rid of it when we introduce #8162). Specifically, unloadRecord and unloadAll() mark records for the GC, and then a sweep attempts to cleanup anything that is de-referencable. If a record turns out to not be de-referencable, we clean up most cache state but not the relationship graph, as we will need it to be able to re-fetch the record later.

This process considers async relationships to be "retainers", meaning that its not possible to fully unload a record that is a member of an async relationship without first clearing all such relationship data. Typically this happens via a persisted delete though there are ways to simulate this via push+unload. Since all your relationships are async, all records in theory ought to be retained, so this process will get rid of the record state for them but not clear the relationship graph of their entries.

Hence unload of foo and baz appears to you to remove all the records (but references to them stay in the graph), then later bar is unloaded and the GC tries to access some information about foo/baz to see if they are able to be released too and blows up.

This might be a bug with async-to-async relationship handling (generally speaking a retained relationship should still have an entry in the identifiers map even if cache and record entries are removed, but it also might not be because the heuristics around this are complicated.

I would largely recommend that only one of two specific approached be used to attempt to unload large quantities of records from the store today (I say today as there is another approach that works with forking, but we haven't implemented forking in the JSON:API cache yet though the library in general has introduced support).

Approach 1: reload the page. Any time you change session this is best practice.
Approach 2: unloadAll() with no arguments. Yes, this means a few other records might be cleared out too, but this approach is special cased since we don't need to do the mark/sweep, we can jump straight to clearing all the things since we know there should be nothing left at the end.

[elfin-sbreuers]

Thank you for looking further into it and for your recommendations if the issue can't be adressed.

We wanted to avoid disruptive/interrupting events for the user experience, meaning the SPA should always be there after being loaded, which is why we avoided the page reloading. But we will give the unloadAll a shot for now, since it solves our test case.

[runspired]

@elfin-sbreuers

RE

meaning the SPA should always be there after being loaded

FWIW if you make sure your cache headers on your static assets are set to enable the browser to cache them until you've re-released, those reloads are extremely cheap. The only cost you really make tends to be whatever network requests you need for data to assemble the page, and you're going to be making those anyway with the setup you've described.

[runspired]

I wrote some tests that replicated this issue (you can hit it even in the case where you do load all the relationships btw). After spending some time debugging, I don't think there's a path forward on this any time soon. Issues like this have existed as long as the unloadAll API has existed: its an inherently unsafe operation with edge cases that will sometimes really blow up in your face. Unfortunately, this is one of them.

I poked around what it would take to make this work. Disregarding performance costs (which could be severe... ), it requires a complete rewrite of how we do GC. We've been intending to do such a rewrite at some point, but I don't think anyone actively contributing to the project currently has the time to do so.

To be clear this is not a WONTFIX because with time or the right contributor I'd love to invest more in this area of the library. If you happen to really like graph traversal optimization problems and would like to take a stab at a new GC ... I'd be happy to pair on this even though it would otherwise be a low priority for me.