Conversation
…pruning, comments - Pass skipAlreadyRegistered: true in buildIndex full-import fallback so user extensions that shadow built-in type names don't fail on first-run bootstrap. - Prune resolved promises from typeLoadPromises in the success handler instead of keeping them forever; remove dead lazyTypes.delete call that duplicated promoteFromLazy cleanup. - Add clarifying comment on extension-takes-priority classification in populateCatalogFromDir explaining it matches the loader's if/else if semantics.
There was a problem hiding this comment.
Code Review
Blocking Issues
None.
Suggestions
None.
Clean, well-scoped bugfix. All three changes are correct:
-
Promise pruning (
model.ts) — The old.then()handler calledthis.lazyTypes.delete(key)(duplicatingpromoteFromLazy()) while never pruningtypeLoadPromises, causing unbounded map growth. The fix correctly deletes the resolved promise, matching the existing.catch()cleanup pattern. -
skipAlreadyRegisteredon bootstrap path (user_model_loader.ts) — ThebuildIndex()full-import fallback now passesskipAlreadyRegistered: true, preventing failures when user extensions shadow built-in type names during first-run bootstrap. This brings the fallback path in line with the lazy-load path. -
Clarifying comment in
populateCatalogFromDir— Explains the extension-takes-priority classification and mutual exclusivity of exports. Appropriate context for a best-effort regex.
No DDD concerns — changes are internal to the ModelRegistry aggregate and UserModelLoader domain service. No libswamp import boundary violations. No new public behavior requiring additional tests.
There was a problem hiding this comment.
Adversarial Review
Critical / High
None.
Medium
None.
Low
model.ts:779-784—.then()handler deletes fromtypeLoadPromisesbefore the outerawaitresumes: Between the.then()microtask and the resumption of theawaitat line 791, a concurrent caller could callensureTypeLoadedfor the same key, seethis.models.has(key)astrue(becausepromoteFromLazyalready ran during the loader), and return early — which is correct. However, if the loader ever resolved without callingpromoteFromLazy, the promise would be pruned andlazyTypeswould still have the entry, causing an infinite retry loop. This is actually better than the old behavior (which deleted fromlazyTypesand silently lost the type), and the production type loader always callspromoteFromLazyvialoadSingleType, so this is purely theoretical.
Verdict
PASS — All three changes are correct. The typeLoadPromises leak fix is the substantive change; the old code deleted from lazyTypes (already handled by promoteFromLazy) instead of cleaning up the resolved promise. The skipAlreadyRegistered addition on the bootstrap path correctly prevents collisions when user extensions shadow built-in types during first-run catalog bootstrap. The comment addition is accurate.
Summary
Fixes three issues identified in post-merge review of #1063 (lazy per-bundle loading):
skipAlreadyRegisteredmissing on bootstrap path —buildIndex()full-import fallback now passesskipAlreadyRegistered: true, preventing failures when user extensions shadow built-in type names during first-run catalog bootstrap.typeLoadPromisesnever pruned on success — Resolved promises are now deleted from the map in the.then()handler. Removes the deadlazyTypes.delete(key)call that duplicatedpromoteFromLazy()cleanup.populateCatalogFromDir— Added clarifying comment explaining that extension-takes-priority matches the loader's ownif/else ifsemantics and that the exports are mutually exclusive by design.Test Plan
deno check,deno lint,deno fmtall clean🤖 Generated with Claude Code