Merge middleware response headers on all App Router response paths#290
Merge middleware response headers on all App Router response paths#290southpolesteve merged 2 commits intomainfrom
Conversation
|
/bigbonk review this |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Middleware Response Header Merging
Overall Assessment
The approach is correct and well-motivated. Before this PR, only 2 out of ~50 response paths in _handleRequest() merged middleware headers. Moving the merge to the handler() wrapper is the right architectural fix.
Concurrency Issue (Blocking)
Module-level let variables are not safe for concurrent requests. This is the single most important issue.
The generated RSC entry runs on Cloudflare Workers (via app-router-entry.ts importing virtual:vinext-rsc-entry). Workers can handle concurrent requests on the same isolate. Two overlapping requests will race on __pendingMiddlewareHeaders and __pendingMiddlewareRewriteStatus:
Request A: handler() sets __pendingMiddlewareHeaders = null
Request A: _handleRequest() sets __pendingMiddlewareHeaders = <A's headers>
Request B: handler() sets __pendingMiddlewareHeaders = null // ← clobbers A's headers
Request A: handler() reads __pendingMiddlewareHeaders // ← null, headers lost
The old code used local variables (_middlewareResponseHeaders) scoped to _handleRequest(), which was inherently safe for concurrency. The per-request state modules already use AsyncLocalStorage for isolation, but these new module-level variables bypass that.
Suggested fix: Return __pendingMiddlewareHeaders from _handleRequest() instead of using module-level state. For example, have _handleRequest() return { response, middlewareHeaders, middlewareRewriteStatus } (or wrap the response in a tuple), and let handler() destructure it. This preserves the centralized merge point without introducing shared mutable state.
Alternatively, if you want to keep the module-level variables, wrap them in an AsyncLocalStorage store (similar to how runWithHeadersContext works). But returning from _handleRequest() is simpler.
Behavioral Concern: Immutable Headers on Redirect Responses
The handler() wrapper correctly skips 3xx responses (line 1265: !(response.status >= 300 && response.status < 400)). But there are several Response.redirect() calls inside _handleRequest() that create responses with immutable headers. If the status check were removed or accidentally broken, response.headers.append() would throw on those responses. The current guard is correct, just worth noting for future maintainers.
Behavior Change: headers.set() vs headers.append() on RSC Path
The old RSC path (line ~2157) used responseHeaders[key] = value (effectively set semantics — last value wins). The new centralized merge uses response.headers.append(key, value). For single-valued headers this is identical, but for multi-valued headers like Set-Cookie, append is actually more correct (preserves all cookies). This is a positive change, but worth documenting in the PR description since it's a subtle behavioral difference.
Missing Test Coverage
There's no test that validates middleware headers appear on route handler responses, server action responses, metadata route responses, or error page responses. Without tests, there's no way to verify the fix works and no regression protection. Consider adding at least one integration test that:
- Configures a middleware that sets a
Set-Cookieheader - Makes requests to a route handler (
app/api/test/route.ts) - Asserts the
Set-Cookieheader appears in the response
Cross-File Parity (AGENTS.md Requirement)
Per AGENTS.md: "When fixing a bug in any of these files, check whether the same bug exists in the others."
dev-server.ts(Pages Router dev): Not affected — middleware runs at the Vite layer and headers are set viares.appendHeader()before the handler executes.prod-server.ts(Pages Router prod): Uses the same per-path merge pattern that this PR fixes for the App Router. The prod server mergesmiddlewareHeadersinto API route responses (line ~895) and SSR page responses (line ~944), but drops them on 404 responses (line ~935). This is a separate bug, but noting it for tracking.app-router-entry.ts(Cloudflare Workers): Delegates to the same virtual RSC entry, so this fix automatically applies there.
Nit: Dead Comment
Line 1382-1384 leaves a comment explaining that middleware headers are stored in module-level variables, but the comment just describes the code structure rather than adding insight. Could be simplified to a single line or removed since the module-level declaration already has a thorough comment block at lines 1238-1242.
Summary
The centralized merge approach is the right direction. The concurrency issue with module-level variables needs to be addressed before merging — it's a correctness bug on Workers. Switching to a return value from _handleRequest() would fix it cleanly.
|
Review posted. Here's a summary: Review SummaryThe approach is correct — centralizing middleware header merging in the One blocking issue: concurrency safety. The module-level Other findings:
|
Middleware response headers were only merged into RSC streaming responses and HTML rendering responses, but were dropped on route handlers, server actions, metadata routes, image optimization, error pages, and other response paths (~30 out of ~37 return points). Move middleware header merging from individual response paths to the handler() wrapper, which runs after _handleRequest() returns. This ensures all response paths receive middleware headers. The previous per-path merges in the RSC response and attachMiddlewareContext() are replaced by a single merge point at the top level.
Module-level __pendingMiddlewareHeaders and __pendingMiddlewareRewriteStatus variables could race between concurrent requests on Cloudflare Workers. Replace with a per-request _mwCtx object passed from handler() into _handleRequest(). Each request gets its own container, eliminating the shared mutable state.
southpolesteve
force-pushed
the
fix/merge-middleware-headers-all-paths
branch
from
584a6e0 to
d4b2693
Compare
|
Pushed fix for the concurrency issue. Replaced module-level The approach avoids having to change any of the 104 return statements in /bigbonk review this |
|
The concurrency fix was pushed in a previous update (replacing module-level vars with a per-request _mwCtx container). Could you re-review? The latest code no longer has any module-level mutable state. /bigbonk review this |
|
@southpolesteve Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review this |
1 participant
Summary
_handleRequest()).handler()wrapper function, which runs after_handleRequest()returns. This ensures all response paths (except early redirects/errors before middleware runs) receive middleware headers.Changes
packages/vinext/src/server/app-dev-server.ts:_middlewareResponseHeadersand_middlewareRewriteStatusto module-level variables (__pendingMiddlewareHeaders,__pendingMiddlewareRewriteStatus) so the handler wrapper can access them.handler()after_handleRequest()returns, usingresponse.headers.append()to preserve existing header values.attachMiddlewareContext()) to prevent header duplication.