fix(app-router): don't clear RSC context before lazy stream is consumed#352
fix(app-router): don't clear RSC context before lazy stream is consumed#352james-elicx merged 2 commits intomainfrom
Conversation
commit: |
|
987d785 to
d93b7bb
Compare
|
/bigbonk review this pr please |
There was a problem hiding this comment.
Review
The fix is correct and the test is well-constructed. Removing the early setNavigationContext(null) / setHeadersContext(null) calls from the RSC branches of renderHTTPAccessFallbackPage and renderErrorBoundaryPage is the right approach — the AsyncLocalStorage scope from runWithHeadersContext() (line 1409) naturally handles cleanup when all async continuations complete, including lazy stream consumption.
The explanation in the PR body and the inline comments are excellent — they clearly describe the mechanism (ALS store mutation vs. ALS scope unwinding), the symptom, and why the HTML branches are unaffected.
Test quality
The test fixture is well-designed:
- layout.tsx — async layout that calls
headers()during stream consumption (reproduces theNextIntlClientProviderServerpattern) - page.tsx — calls
notFound()to triggerrenderHTTPAccessFallbackPage(the buggy code path) - not-found.tsx — simple component to verify the correct code path was taken
- Tests cover both the HTML baseline (confirms the fixture works) and the RSC regression path with explicit positive and negative assertions
Same bug exists elsewhere in this file
Per AGENTS.md: "When fixing a bug in any of these files, check whether the same bug exists in the others. Do not leave known bugs as 'follow-ups' — fix them in the same PR."
I checked prod-server.ts and app-router-entry.ts — they delegate to the built RSC entry and don't manage context directly, so they're clean. However, there are two more instances of the same bug pattern in app-dev-server.ts itself that this PR doesn't fix:
1. Intercepting route RSC branch (lines 2154-2159):
const interceptStream = renderToReadableStream(interceptElement, { onError: interceptOnError });
setHeadersContext(null); // ← same bug
setNavigationContext(null); // ← same bug
return new Response(interceptStream, {
headers: { "Content-Type": "text/x-component; charset=utf-8", "Vary": "RSC, Accept" },
});This is the same pattern: renderToReadableStream() → immediate context clear → return unconsumed stream. If an intercepting route has an async layout that calls headers(), it will hit the exact same bug.
2. Server action RSC re-render branch (lines 1828-1837):
const rscStream = renderToReadableStream(
{ root: element, returnValue },
{ temporaryReferences, onError: onRenderError },
);
const actionPendingCookies = getAndClearPendingCookies();
const actionDraftCookie = getDraftModeCookieHeader();
setHeadersContext(null); // ← same bug
setNavigationContext(null); // ← same bugAfter a server action, the page tree is re-rendered to an RSC stream that's returned to the client. Context is cleared before the stream is consumed. If any async component in the re-render calls headers(), it will see null context.
Both of these should be fixed in this PR to avoid leaving known bugs as follow-ups. The fix is the same: remove the setHeadersContext(null) / setNavigationContext(null) calls from these RSC response branches.
Minor: the getAndClearPendingCookies() + getDraftModeCookieHeader() calls in the server action branch
Note that getAndClearPendingCookies() and getDraftModeCookieHeader() on lines 1834-1835 read from the headers context synchronously and are called before the stream is consumed. This is fine — they extract data that was set during action.apply(), not during stream rendering. So those calls should stay; only the two context-clear calls below them need to be removed.
Summary
The core fix is sound and the test coverage for the renderHTTPAccessFallbackPage path is good. The two additional instances of the same bug (intercepting routes and server action re-renders) should be fixed in this PR.
|
I've posted a review on PR #352 requesting changes. Here's the summary: The core fix is correct — removing early However, there are two more instances of the same bug in
I also noted that |
…nd intercepting route RSC branches Same bug as fixed for renderHTTPAccessFallbackPage/renderErrorBoundaryPage: context was cleared immediately after renderToReadableStream() returned, before the stream was lazily consumed by the client. Async server components running during stream consumption would see null context and fail. Two additional instances fixed: - Server action RSC re-render branch (after action.apply()) - Intercepting route RSC response branch The getAndClearPendingCookies() / getDraftModeCookieHeader() calls in the server action branch are intentionally preserved — they read synchronously from context set during action execution (not during stream rendering) and must run before the response is sent. Context cleanup is handled naturally by the AsyncLocalStorage scope from runWithHeadersContext() unwinding after all async continuations complete.
|
/bigbonk re-review this pr please |
There was a problem hiding this comment.
Re-review
All feedback from the first review has been addressed. The second commit (af053d6) fixes the two additional instances that were flagged:
-
Server action RSC re-render branch (line ~1833) —
setHeadersContext(null)/setNavigationContext(null)removed. ThegetAndClearPendingCookies()/getDraftModeCookieHeader()calls correctly remain since they read synchronously-set data from the action execution, not from stream consumption. -
Intercepting route RSC branch (line ~2157) — context clears removed.
Both fixes include the same clear explanatory comment as the original two sites.
Remaining setHeadersContext(null) calls are safe
I audited all ~50 remaining setHeadersContext(null) / setNavigationContext(null) calls in app-dev-server.ts. They all fall into safe categories:
- HTML branches (lines 596, 716) —
await handleSsr(rscStream, ...)fully consumes the stream before clearing - Redirect responses (lines 2185, 2216, 2264) — no unconsumed stream exists
- Fallback text responses (lines 2193, 2224) —
renderHTTPAccessFallbackPagereturned null, so a plain text response is sent with no stream
No additional instances of the bug remain.
CI
All 26 checks pass — Lint, Typecheck, Vitest, all 7 Playwright E2E projects, all 7 deploy previews, smoke tests, CodeQL.
LGTM.
|
PR #352 re-reviewed and approved. The author addressed both issues from the first review:
I audited all ~50 remaining |
When handling RSC requests (client-side navigation), the handler was calling
setNavigationContext(null)andsetHeadersContext(null)immediately afterrenderToReadableStream()returned — before the stream was actually consumed.The problem:
renderToReadableStream()returns as soon as the shell is ready, but the stream is consumed lazily by the client. Async server components that run during stream consumption (e.g.NextIntlClientProviderServer, which is rendered as a child ofNextIntlClientProviderin a layout) would execute after the handler had already cleared context, causing them to see null headers/navigation context and throw. The symptom for users wasuseTranslations()failing with "context from NextIntlClientProvider was not found" during client-side navigation.Fix: Remove the early
setNavigationContext(null)/setHeadersContext(null)calls on the RSC response branches inrenderHTTPAccessFallbackPageandrenderErrorBoundaryPage. Context is already cleaned up correctly when theAsyncLocalStoragescope fromrunWithHeadersContextunwinds — no manual clear is needed.The HTML response branches are unaffected: those
awaitthe full SSR pipeline (handleSsr) before responding, so context is fully consumed by the time the clears run.