Change dependency tracker to use canonical RRI form - #5776
Conversation
Host Test Results 1 files 1 suites 2h 25m 8s ⏱️ Results for commit 2f89811. Realm Server Test Results 1 files ±0 1 suites ±0 16m 30s ⏱️ +8s Results for commit 2f89811. ± Comparison against earlier commit a6314e6. |
3aa3933 to
7ce848a
Compare
The dependency tracker's `canonicalURL` guard dropped any identifier without an `http(s)` scheme, so prefix-form identifiers could not be tracked at all. Every caller therefore converted to a URL first: `card-api` through `Loader.dependencyTrackingKey`, and the loader internally through `canonicalizeTrackingKey`, which folded each module onto its virtual-alias URL. The forms were then converted back — three of the four snapshot consumers run the deps through `unresolveURLs`, and the index writer folds them again before persisting. The tracker was a URL-form island inside an RRI-form pipeline. Accept both canonical remote forms in the guard instead. A URL starts with `http://`/`https://` and a prefix-form RRI starts with `@`, which distinguishes them without a VirtualNetwork — the same syntactic test `isLocalId` uses — so the tracker stays free of realm mappings. Bare specifiers, relative references and other schemes are still dropped. That lets the conversions go: - `card-api` records `identity.module` and the loader's consumed-module list as they are, both already canonical RRI. - The loader's two internal tracking calls use `moduleCacheKey`, the same fold its module cache uses, so a module's dependency identity and its class identity are one key and cannot diverge. - `dependencyTrackingKey` and `canonicalizeTrackingKey` are removed. Deps on disk are unchanged: they were already normalized to prefix form by the index writer, which now finds them in that form to begin with. The `unresolveURLs` calls at the snapshot consumers are left in place — they also normalize deps from other sources merged into the same arrays. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`moduleCacheKey` folds a module onto its canonical RRI, which is right for a realm reached through a registered prefix but wrong for one that has only a URL mapping: `unresolveURL` maps an alias *to* the real URL and never back, so a module in such a realm was recorded under the host actually serving it rather than the alias the index names it by. In CI that meant a dep on `https://realm-test.ci.localhost/test/person` where every index row says `https://localhost:4202/test/person` — a dep no invalidation scan can match. Give tracking its own fold: canonical RRI when a prefix mapping claims the module, the virtual alias when only a URL mapping does. The module cache keeps `moduleCacheKey`, which needs internal consistency rather than agreement with the index, and the two requirements are not the same. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The dependency walk re-derives a tracking key for every module in a root's transitive set on every import of that root, so a realm's modules are folded repeatedly during a from-scratch index — and the url-mapped branch allocates a URL each time. Cache the result per module identifier. Cleared alongside the other mapping-derived caches when a realm mapping is added or removed, since the key is only stable between those changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7ce848a to
359b9a1
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f89811e59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| !url || | ||
| (!url.startsWith('http://') && | ||
| !url.startsWith('https://') && | ||
| !url.startsWith('@')) |
There was a problem hiding this comment.
When a prefix-mapped card is prerendered, packages/host/app/routes/render.ts starts the tracking session with the URL-form route id, while deserialization and field contexts in card-api.gts use the canonical RRI-form resource.id or instance.id as their consumer. Admitting @... here causes #normalizeConsumer to record that same root under its RRI spelling, but #rootCandidates only excludes string-equivalent URL/.json variants, so the rendered row is emitted as its own dependency. Normalize roots and consumers to the same form, or make root exclusion recognize both spellings.
Useful? React with 👍 / 👎.
| let cached = this.trackingKeyCache.get(moduleIdentifier); | ||
| if (cached !== undefined) { | ||
| return cached; |
There was a problem hiding this comment.
If VirtualNetwork.addURLMapping() is called after this loader has already derived a key, this cache continues returning the pre-mapping real URL instead of the newly available virtual alias. addURLMapping() clears the VirtualNetwork's own mapping caches but does not call notifyMappingChange(), so the loader's onMappingChange subscription never clears trackingKeyCache; subsequent cache-hit imports can therefore persist a dependency spelling that invalidation will not find. Either notify loaders about URL-mapping changes or avoid retaining alias-derived keys across them.
Useful? React with 👍 / 👎.
1 participant
Previously it was converting everything to a URL first, now it can handle prefix RRIs.
Claude’s explanation
The dependency tracker's `canonicalURL` guard dropped any identifier without an `http(s)` scheme, so prefix-form identifiers could not be tracked at all. Every caller therefore converted to a URL first: `card-api` through `Loader.dependencyTrackingKey`, and the loader internally through `canonicalizeTrackingKey`, which folded each module onto its virtual-alias URL. The forms were then converted back — three of the four snapshot consumers run the deps through `unresolveURLs`, and the index writer folds them again before persisting. The tracker was a URL-form island inside an RRI-form pipeline.Accept both canonical remote forms in the guard instead. A URL starts with
http:///https://and a prefix-form RRI starts with@, which distinguishes them without a VirtualNetwork — the same syntactic testisLocalIduses — so the tracker stays free of realm mappings. Bare specifiers, relative references and other schemes are still dropped.That lets the conversions go:
card-apirecordsidentity.moduleand the loader's consumed-module list as they are, both already canonical RRI.moduleCacheKey, the same fold its module cache uses, so a module's dependency identity and its class identity are one key and cannot diverge.dependencyTrackingKeyandcanonicalizeTrackingKeyare removed.Deps on disk are unchanged: they were already normalized to prefix form by the index writer, which now finds them in that form to begin with. The
unresolveURLscalls at the snapshot consumers are left in place — they also normalize deps from other sources merged into the same arrays.