Change loader module cache to use canonical RRI#5540
Conversation
moduleCacheKey now folds every spelling of a module (real URL, virtual alias, RRI prefix) to one RRI via unresolveURL, so the internal module cache is RRI-native rather than keyed by the virtual-alias URL. All spellings still collapse to one key (preserving single-class identity); user-realm and bare-package identifiers pass through unchanged. The RRI->URL relationship is only stable between realm-mapping changes, so VirtualNetwork now exposes onMappingChange and the loader subscribes to discard its RRI-keyed caches (modules, moduleCanonicalURLs, knownDepsCache) whenever a mapping is added or removed. Realm-prefix mappings are a small fixed set frozen after setup, so this fires rarely. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Preview deploymentsHost Test Results 1 files ±0 1 suites ±0 2h 59m 19s ⏱️ + 24m 20s Results for commit 2023cfa. ± Comparison against earlier commit 392b387. Realm Server Test Results 1 files ±0 1 suites ±0 16m 13s ⏱️ ±0s Results for commit 2023cfa. ± Comparison against earlier commit 392b387. |
![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: 392b387c02
ℹ️ 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".
Each Loader subscribes to the VirtualNetwork's realm-mapping-change notifications so it can discard its RRI-keyed caches on a remap. The VirtualNetwork outlives any single loader — LoaderService replaces the loader on every module edit, session boundary, and clearFetchCache — so a loader that never unsubscribes stays pinned, along with its entire compiled-module cache, by the listener the network still holds. Over a long session these accumulate, and a later remap invokes every stale callback. Loader now retains the unsubscribe and exposes dispose() to release it; LoaderService calls dispose() on the outgoing loader before replacing it (and on service teardown). dispose() only detaches the listener — it does not clear caches, since a clone made from the loader carries them forward. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR makes the loader’s module cache key use the canonical RRI form (via VirtualNetwork.unresolveURL) so that different spellings of the same module (resolved URL, virtual alias URL, and prefix-form RRI) share a single cached module/class identity. Because realm-prefix mappings can change at runtime, it also introduces a realm-mapping change notification hook and uses it to discard affected loader caches, plus adds a Loader.dispose() mechanism to avoid leaking discarded loaders via long-lived VirtualNetwork listeners.
Changes:
- Add
VirtualNetwork.onMappingChange()(with unsubscribe) and notify listeners on realm-mapping add/remove. - Change
Loadermodule-cache keying to canonical RRI (unresolveURL) and clear module caches on mapping changes. - Ensure discarded loaders unsubscribe via
Loader.dispose(), and updateLoaderService+ unit tests accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/runtime-common/virtual-network.ts | Adds mapping-change listener subscription surface and emits notifications on mapping add/remove. |
| packages/runtime-common/loader.ts | Switches module cache key to canonical RRI, clears caches on mapping changes, and adds dispose() to unsubscribe. |
| packages/host/tests/unit/loader-test.ts | Adds unit tests for cache discard on mapping change and for dispose() unsubscribing behavior. |
| packages/host/app/services/loader-service.ts | Disposes previous loaders when replacing/resetting to prevent listener-pinned loader/cache leaks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Notified whenever a realm-prefix mapping is added or removed. Consumers | ||
| // that key caches by the RRI form a mapping produces (e.g. the Loader's | ||
| // module cache) subscribe here to discard those entries when the mapping | ||
| // set changes — the RRI→URL relationship is only stable between changes. | ||
| private mappingChangeListeners = new Set<() => void>(); |
| // Release the realm-mapping-change subscription so this loader can be | ||
| // garbage-collected once discarded. Only detaches the listener — it does not | ||
| // clear the caches, since a clone made from this loader carries them forward. | ||
| dispose() { | ||
| this.unsubscribeMappingChange?.(); | ||
| this.unsubscribeMappingChange = undefined; | ||
| } |
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Review focus: the semantics of the new mapping-sensitive cache key (unresolveURL in moduleCacheKey), the invalidation/subscription lifecycle, and whether the leak fix covers every loader-replacement site. All claims below were verified against the checked-out branch, not the diff view.
Bottom line: no blocking issues. The RRI-keyed cache, the clear-on-mapping-change subscription, and the dispose() plumbing are correct as landed. One code comment misdescribes cloneLoader's behavior and should be fixed before merge (one-line change, thread on dispose() in loader.ts).
What lands right:
- The cache key now matches the canonical identifier form the loader already emits everywhere else (
canonicalIdentifierfeedsidentify()output and dependency lists) — one canonicalization scheme instead of two coexisting ones (virtual-alias for keys, RRI for outputs). I traced all three spellings through the new key: resolved real URL and RRI prefix collapse via therealmMappingsscan, and the virtual-alias spelling collapses via theresolveURLMappingchase — single class identity holds across every spelling. - Clear-on-change converts the documented instability (RRI→URL is only stable between mapping changes) into an invalidation event rather than a stale-key hazard, and it's strictly safer than the orphaning the previous stable-key design accepted — detail and the boot-time-only production evidence in the thread on the constructor subscription.
- The unsubscribe fix is complete:
dispose()is called at all three loader-replacement sites inLoaderService(resetSessionBoundary, bothresetLoaderbranches) plus the destructor. I grepped the tree for other productionnew Loader(/cloneLoadersites — there are none outside the host service (the realm server registers mappings at boot but constructs no VN-attached loaders); test-created loaders subscribe to per-test VNs that die with the test owner. - Both new tests ran in CI (confirmed in the merged host test report) and the net-zero add+remove probe in the dispose test is exactly the right observable for "subscription released".
Recommendations:
- Fix the
dispose()comment — clones do not carry the caches forward; corrected wording suggested in the thread onpackages/runtime-common/loader.ts. (Non-blocking but cheap and worth doing here.) - Optional: reword the mapping-add assertion message in
loader-test.tsso it doesn't overclaim — thread there has the detail.
Adjacent, out of scope:
moduleCacheKeynow runsunresolveURLon everygetModule/setModule. Cost is at parity with the removed path (both payresolveURLMapping— URL construction plusRealmPaths— on the common user-realm miss; the base-realm real-URL case actually got cheaper since therealmMappingsscan hits before any URL construction). If loader ops ever show up in render profiles again, a VN-level memo forunresolveURLmirroringtoURLHrefCache(cleared on mapping change, whichonMappingChangenow makes trivial) is the natural follow-up.knownDepsCachekeys remain resolved-URL-form (resolveImportoutput) rather thanmoduleCacheKeyform, so two spellings of one root can hold duplicate entries. Correctness is covered by the clear-on-change; unifying the keying is a nicety for whoever continues the RRI-native migration.- Percy reports 5 visual diffs awaiting review — worth a glance before merge; nothing in this diff should change rendering, so they're likely environmental, but that's for Percy review to confirm.
| // Module caches are keyed by canonical RRI form (see moduleCacheKey), whose | ||
| // relationship to a real URL is only stable between realm-mapping changes. | ||
| // Discard the RRI-keyed caches whenever a mapping is added or removed so an | ||
| // entry can't outlive the spelling it was keyed under. | ||
| this.unsubscribeMappingChange = this.virtualNetwork?.onMappingChange(() => { | ||
| this.modules.clear(); | ||
| this.moduleCanonicalURLs.clear(); | ||
| this.knownDepsCache.clear(); | ||
| }); |
There was a problem hiding this comment.
[Claude Code 🤖] Confirmation — the clear-on-change design is correct as landed, and here are the conditions under which it stays that way. No change requested; this documents what I verified so the trade-off is on record.
What this replaces. The previous key was deliberately mapping-insensitive (virtual-alias URL via mapURL(real-to-virtual)), precisely so a mapping change couldn't orphan cached entries. This change inverts that: the key is now mapping-sensitive (unresolveURL), and the instability is handled by discarding the caches on every mapping change. The discard is strictly better than orphaning — an orphaned URL-form entry would become reachable again after a mapping remove restores the key form, re-serving a stale module copy; the discard makes that impossible.
Why the blast radius is acceptable in production. I enumerated every production caller of addRealmMapping/removeRealmMapping: host — makeVirtualNetwork() in packages/host/app/services/network.ts (base/catalog/skills/openrouter prefixes at service construction) and shimExternals in packages/host/app/lib/externals.ts (boxel-ui prefix); server — boot-time registration in packages/realm-server/main.ts and worker.ts (and the server constructs no VN-attached Loaders in production code at all — I grepped for every new Loader( site). RealmServerService.setAvailableRealmIdentifiers — the one path that changes the realm list at runtime — only mutates its in-memory availableRealms array, never the VN. So in production the listener fires only during boot, before any meaningful module cache exists; all mid-session add/remove traffic is test-scoped.
The condition under which this stops being safe. If realm-prefix mappings ever become runtime-dynamic in the host (e.g. registering user-realm prefixes at login or on workspace creation — a plausible direction for RRI-native work), each add wipes every live loader's compiled modules. Already-rendered card instances keep holding classes evaluated from the discarded cache while fresh imports re-evaluate new class objects, so instanceof / polymorphic-field identity checks across that boundary diverge. Whoever makes mappings dynamic needs to pair it with a loader-generation story, not just rely on this listener.
Mid-flight safety, verified. A clear during an in-flight import degrades to a refetch, not a wedge: advanceToState's loop re-reads getModule every iteration and its undefined arm calls fetchModule again, and Loader-level shims re-serve through _fetch's moduleShims lookup (keyed by URL, untouched by the clear), with fetchModule's shimmed branch re-establishing the module entry and canonical URL.
One caveat worth knowing. unresolveURL's second step chases resolveURLMapping(url, 'virtual-to-real'), so the key derivation also depends on urlMappings — and addURLMapping does not notify. That's fine under the current contract (urlMappings is append-only with no removal API and is populated only at boot, so it's outside the instability window), but if a removal API is ever added it needs to notify too.
| // Release the realm-mapping-change subscription so this loader can be | ||
| // garbage-collected once discarded. Only detaches the listener — it does not | ||
| // clear the caches, since a clone made from this loader carries them forward. | ||
| dispose() { | ||
| this.unsubscribeMappingChange?.(); | ||
| this.unsubscribeMappingChange = undefined; | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] The comment's rationale for not clearing the caches is factually wrong: clones do not carry the caches forward. Non-blocking, but worth fixing before merge — it misdescribes cloneLoader's contract in exactly the place a future reader will look when deciding whether dispose() may clear.
What cloneLoader actually does. It constructs a fresh Loader (empty modules / moduleCanonicalURLs / knownDepsCache) and copies only moduleShims, re-registering each via clone.shimModule(...). The compiled-module state is deliberately not inherited — the clone rebuilds it from the HTTP fetch cache, which is the whole point of the swap (resetLoader's comment in packages/host/app/services/loader-service.ts says exactly this: "we keep the fetch cache so we can take advantage of HTTP caching when rebuilding the loader state"). So "a clone made from this loader carries them forward" can't be the reason not to clear; clearing the parent's maps would be invisible to any clone.
The real reasons not to clear, which the code gets right. (1) The discarded loader may still be draining in-flight work — LoaderService swaps this.loader while async imports against the old instance are mid-flight, and clearing under them would force re-evaluation and split class identity within those operations. (2) Clearing buys nothing for memory: once the listener is detached and the owner drops its reference, the maps are collected wholesale.
Suggested rewording:
| // Release the realm-mapping-change subscription so this loader can be | |
| // garbage-collected once discarded. Only detaches the listener — it does not | |
| // clear the caches, since a clone made from this loader carries them forward. | |
| dispose() { | |
| this.unsubscribeMappingChange?.(); | |
| this.unsubscribeMappingChange = undefined; | |
| } | |
| // Release the realm-mapping-change subscription so this loader can be | |
| // garbage-collected once discarded. Only detaches the listener — the caches | |
| // are left intact because a discarded loader may still be draining in-flight | |
| // imports, and once nothing references it the maps are collected wholesale. | |
| dispose() { | |
| this.unsubscribeMappingChange?.(); | |
| this.unsubscribeMappingChange = undefined; | |
| } |
| assert.false( | ||
| loader.isModuleLoaded(`${testRealmURL}f`), | ||
| 'adding a realm mapping discards the module cache', | ||
| ); |
There was a problem hiding this comment.
[Claude Code 🤖] Nit, non-blocking: this assertion's message overclaims what it observes — it would pass even if the add discarded nothing. The dispose test's own comment explains the mechanism: a mapping add shifts the cache-key form itself. After addRealmMapping('@test-loader-discard/', testRealmURL), isModuleLoaded unresolves the lookup to @test-loader-discard/f, while an undiscarded entry would still sit under its import-time key ${testRealmURL}f — a miss either way. I traced both paths through moduleCacheKey → unresolveURL to confirm.
The assertion that actually pins the discard is the final one: after the remove restores the key form, a surviving entry would resurface and isModuleLoaded would return true. (Strictly, the pair pins "at least one of add/remove discards" — a remove-only implementation would also pass — but that disjunction is the observable contract; the stale-resurrection scenario is exactly what the last assertion catches, and there's no public-API probe that can distinguish which event did the discarding.)
Suggested message that matches what's observed:
| assert.false( | |
| loader.isModuleLoaded(`${testRealmURL}f`), | |
| 'adding a realm mapping discards the module cache', | |
| ); | |
| assert.false( | |
| loader.isModuleLoaded(`${testRealmURL}f`), | |
| 'module is not visible under its pre-mapping spelling after a mapping add', | |
| ); |
The test comment at the top is accurate as written; only this message oversells. Also: the net-zero add+remove probe in the dispose test below, with its explanatory comment, is genuinely good test design — that comment will save the next person real time.
If a prefix mapping exists, the prefix RRI is used as the key.
This adds
VirtualNetwork#onMappingChangeso the cache can be thrown away if prefix mappings are added or removed.