fix: harden RSC initialization and remote module fetches - #3070
Merged
Conversation
Registry and cache-only consumers should not start project package detection. Resolve the React version on first render or page use and memoize that promise. Constraint: Constructor-only registry operations must not start filesystem work Rejected: Configure React only in tests | hides the production side effect Rejected: Add asynchronous cache teardown | spreads lifecycle complexity without owning the work Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep RSC handler construction side-effect free Tested: focused RSC suites with --trace-leaks; coverage shard 2 Not-tested: staging runtime before release
SSR HTTP module loading failed on the first temporary upstream response. Retry retryable statuses and network failures with bounded backoff while preserving immediate failure for permanent responses, and publish the fixes as 0.1.1126. Constraint: Remote module loading depends on third-party CDN availability Rejected: Retry the complete binary test only | leaves production renders exposed to the same outage Rejected: Retry permanent 4xx responses | adds latency without a recovery path Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep retries bounded and preserve immediate permanent-response failures Tested: HTTP bundle cache suite; focused RSC suites; coverage shard 2; deno task verify:quick Not-tested: live CDN outage longer than the bounded retry window
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves runtime reliability around RSC initialization and remote ESM module fetching, addressing leaked/pending filesystem ops during handler construction and transient upstream fetch failures during render.
Changes:
- Defer project React version discovery in RSC until a render path actually needs it (via a lazy callback).
- Add bounded retry with backoff for transient HTTP module fetch failures, with new tests covering retry vs permanent failure behavior.
- Bump package/version constants to
0.1.1126.
Verification status (from PR description):
- Focused RSC and HTTP cache tests with
--trace-leaks: passed - Coverage shard 2/8: passed
verify:quick: passed- Full unit suite: passed
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/version-constant.ts | Bumps shared version constant to 0.1.1126. |
| src/transforms/esm/http-cache.ts | Adds retrying fetchHttpModule() wrapper for transient HTTP failures and uses it in the cache path. |
| src/transforms/esm/http-cache.test.ts | Adds tests for transient retry behavior (502) and non-retry for permanent failure (404). |
| src/server/services/rsc/orchestrators/render-handler.ts | Switches reactVersion option from a promise to a lazy callback and updates call sites. |
| src/server/services/rsc/orchestrators/handler.ts | Lazily resolves and memoizes React version only when needed by renderer/page handling. |
| src/server/services/rsc/endpoints/handler-registry.test.ts | Adds regression test ensuring handler construction does not trigger dependency inspection (no Deno.stat). |
| deno.json | Bumps package version to 0.1.1126. |
Comments suppressed due to low confidence (1)
src/transforms/esm/http-cache.ts:331
- When the fetch returns a non-OK response, the code throws immediately without consuming or canceling the response body. In Deno, leaving the body unread can keep the underlying connection/resource open longer than necessary and may show up as a leak under
--trace-leaks(especially on repeated failures). Cancel the body before throwing to release resources promptly.
const response = await fetchHttpModule(normalizedUrl);
if (!response.ok) {
throw BUILD_FAILED.create({ detail: `Failed to fetch ${normalizedUrl}: ${response.status}` });
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: f0d51ff3e6
ℹ️ 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".
The new bounded HTTP retry loop can outlive the previous in-flight waiter deadline. Give callers a complete wait window, align the HTTP cache waiter with the retry budget, and discard final error response bodies before surfacing BUILD_FAILED. Constraint: A three-attempt fetch may consume roughly 90 seconds before backoff and scheduling overhead. Rejected: Remove the in-flight deadline | a fetch implementation that ignores abort could block every waiter indefinitely. Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep the in-flight wait budget at least as large as the owning HTTP retry budget. Tested: focused in-flight and HTTP cache suites with trace-leaks (54 steps), verify:quick Not-tested: wall-clock 90-second upstream timeout path; policy is covered with an injected short wait window.
Remote module failures can occur after response headers, so the shared retry boundary now covers fetch and body consumption together. Each failed response is discarded, transient failures retain one bounded owner, and observability only receives credential-safe URL details. Constraint: HTTP response bodies can fail or stall after headers have already succeeded. Rejected: Keep the manual retry loop | it duplicated shared backoff behavior and left body reads outside the attempt timeout. Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep fetch, body consumption, and failed-body cancellation inside the same HTTP module retry attempt. Tested: deno task verify:quick; 58 trace-leak-checked HTTP cache and in-flight steps; red-green coverage for network, body-read, status exhaustion, cancellation, and credential safety. Not-tested: Live upstream CDN fault injection.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/transforms/esm/http-cache.ts:408
normalizedUrlmay include credential-bearing query params (for example?access_token=...). This block logs and surfacesnormalizedUrldirectly in log context and theBUNDLE_ERRORdetail, which can leak secrets. Use the already-importedsanitizeUrlForSpan()(or another URL-sanitizer) when emitting the URL in logs/errors, while still fetching with the full normalized URL.
httpCacheLog.debug("Fetching from network", { url: normalizedUrl });
const fetchedModule = await fetchHttpModule(normalizedUrl);
let code = fetchedModule.code;
const contentType = fetchedModule.contentType;
const isHtmlContent = contentType.includes("text/html") || looksLikeHtmlNotJs(code);
if (isHtmlContent) {
logger.error(
"[HTTP-CACHE] Received HTML instead of JavaScript, likely an esm.sh error page",
{
url: normalizedUrl,
contentType,
preview: code.slice(0, 200),
},
);
throw BUNDLE_ERROR.create({
detail:
`Received HTML instead of JavaScript from ${normalizedUrl}. The package may not exist or failed to build on esm.sh.`,
});
The lint gate checks new test files more strictly than the no-check unit runner, so rejected values must be narrowed before their messages are inspected. Constraint: Preserve the existing runtime assertions and retry coverage. Confidence: high Scope-risk: narrow Tested: deno check --no-lock src/transforms/esm/http-cache.test.ts; deno fmt --check; focused test with --trace-leaks Not-tested: Full repository suite for this assertion-only follow-up
Sanitize credential-bearing module URLs before logs and user-visible bundle errors, and scope the lazy React version assertion to its project path so unrelated Deno.stat calls cannot make the regression test flaky. Constraint: HTTP cache URLs may contain provider credentials and test processes may perform unrelated filesystem probes. Rejected: Keep the HTML response preview in error logs | upstream bodies can contain secrets and do not add actionable cache diagnostics. Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep externally supplied URLs sanitized in every diagnostic and count only project-owned probes in global API stubs. Tested: deno test -A --no-check focused cache and handler suites; deno fmt --check; deno task verify:quick Not-tested: live upstream HTML failure carrying real provider credentials
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The release candidate was functionally correct, but review and complete validation uncovered an unnecessary jitter call, an imprecise security assertion, three load-sensitive tests, vulnerable transitive packages, internal public-doc wording, and a broken documented Storybook build. Keep each correction at its existing boundary and add regression evidence where behavior changed. Constraint: Preserve the v0.1.1126 release scope and introduce no new dependency. Rejected: Ignore failures outside the original RSC regression | would ship known security and developer-workflow defects. Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep caller timeout overrides deterministic and keep the Storybook Deno config alias explicit. Tested: verify:quick; 82 focused test steps; Storybook boundary test and production build; frozen OpenTelemetry tests; npm audit. Not-tested: Full pre-push suite after this final commit; GitHub CI is the authoritative full-suite rerun.
Sync the release branch with current main, advance its patch version beyond the already-published 0.1.1126, and remove cache identities from in-flight timeout logging so credential-bearing URLs cannot reach logs. Constraint: v0.1.1126 was published from main while this PR was still pending. Rejected: Redact selected query parameters | omitting the unused cache identity closes the entire logging channel. Confidence: high Scope-risk: narrow Reversibility: clean Directive: Do not add raw HTTP cache identities back to logs. Tested: focused in-flight and HTTP cache tests (60 steps); deno task verify:quick Not-tested: staging release deployment pending merge
kojiwakayama
disabled auto-merge
July 24, 2026 17:48
kojiwakayama
enabled auto-merge
July 24, 2026 17:48
Contributor
Author
|
@kwakayama All CI checks are green and every review thread is addressed/resolved. This v0.1.1127 gate now only needs the required code-owner/last-push approval; #3077 is stacked behind it for v0.1.1128. |
kwakayama
approved these changes
Jul 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Release target
This PR releases v0.1.1127. Version v0.1.1126 already exists from the preceding main commit, so both
deno.jsonandsrc/utils/version-constant.tsintentionally advance to0.1.1127.Why
The 0.1.1125 release exposed two production reliability issues: constructing an RSC handler could leave a pending Deno.stat operation, and a transient esm.sh failure could abort a render immediately. Review and full-suite verification then exposed adjacent security, determinism, and developer-workflow defects. This PR fixes each at its existing boundary rather than leaving known regressions for a later release.
Verification