fix(transforms/esm): never cache a degraded module artifact - #3016
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d40f0f52fc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| hash, | ||
| degraded, | ||
| }); | ||
| return cachePath; |
There was a problem hiding this comment.
In the SSR case where a statically imported HTTP module contains a lazy absolute HTTP import that fails to prefetch, this branch returns the degraded file path to cacheHttpImportsToLocal() without propagating any degraded state. The pipeline then caches transformed app code that points at file://...http-<hash>.mjs; on a cache hit, validateCachedBundlesByManifestOrCode() only checks the http-*.mjs file, or any partial manifest, exists, so it returns the cached transform and never re-enters cacheHttpModuleInternal() to retry the child fetch. That means the degradation can survive until transform-cache eviction, and indefinitely in the local fallback, despite this block saying the next render retries.
Useful? React with 👍 / 👎.
Soft-failing dynamic HTTP dependency prefetches can produce a module that still points at an unresolved remote child. Treating that output as a normal artifact lets one upstream blip propagate through local memoization and the distributed bundle cache. The resolver now reports unresolved runtime-resolvable specifiers, and the HTTP cache marks those artifacts for the current render while keeping them out of durable caches so the next render retries.\n\nConstraint: Absolute dynamic http(s) imports may still be runtime-resolved; relative, npm:, and bare dynamic imports cannot be safely left in cached esm.sh modules.\nRejected: Shortening the distributed TTL | it would still publish a known-bad artifact fleet-wide and only reduce the exposure window.\nConfidence: high\nScope-risk: moderate\nDirective: Keep degraded artifacts useful for the render that produced them, but never let them acquire the same cache identity as complete artifacts.\nTested: deno test --allow-all src/transforms/esm/http-cache.test.ts src/transforms/esm/specifier-resolver.test.ts\nTested: deno task lint:sanitizer-baseline\nTested: deno task lint\nTested: deno task typecheck\nNot-tested: full repository CI locally
d40f0f5 to
28cbab9
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Score: 94/100.
This fixes a real cache correctness bug from the dynamic import soft-fail path: degraded HTTP module artifacts are no longer promoted into the distributed cache or in-memory path map, and marked local artifacts are retried on the next render. The restriction to absolute dynamic http(s) specifiers is the right boundary; relative, npm:, and bare dynamic specifiers cannot be safely left for runtime resolution in cached esm.sh modules.
Local validation after rebasing on current main:
deno test --allow-all src/transforms/esm/http-cache.test.ts src/transforms/esm/specifier-resolver.test.tsdeno task lint:sanitizer-baselinedeno task lintdeno task typecheck
Recommended next step: merge after GitHub required checks finish green.
Why
Follow-up to #2999. That PR made nested esm.sh specifier resolution soft-fail instead of throwing, but a degraded artifact could then become indistinguishable from a complete artifact. A transient child-module fetch failure could be written locally, pushed to the distributed bundle cache, and memoized for later renders.
The previous runtime-resolution premise is also only safe for absolute
http(s)dynamic imports. Relative dynamic imports inside an esm.sh bundle resolve against the local bundle-cache directory, andnpm:or bare dynamic imports need resolver/import-map context that the cached module no longer has.What
buildReplacementsandrewriteModuleImportsnow return both replacements and adegradedsignal.http(s)specifiers; relative,npm:, and bare dynamic resolution failures are fatal again.Verification
deno test --allow-all src/transforms/esm/http-cache.test.ts src/transforms/esm/specifier-resolver.test.tsdeno task lint:sanitizer-baselinedeno task lintdeno task typecheckReview status
Score: 94/100. Recommended next step: merge after GitHub required checks finish green.
Operational note
Already-written unmarked degraded artifacts cannot be distinguished in code. If the remaining 24-hour distributed cache window matters during rollout, flush the HTTP module cache prefix.