Repository navigation
perf(edge): hand requests only the Function answers on before the middleware - #443
soleil-colza wants to merge 6 commits into
Conversation
…ddleware A router's request for a page's whole payload is classified `rsc-payload` for any App Router route with a runtime-cache record, build payload or none. Two kinds of them the edge can only pass on to the Function: a member of a class with a shell, under a policy that reads no member's own record (`concreteUpgrade`), and a page the build wrote no payload of whose record holds a resume, asked for other than as a static prefetch. Under a matching middleware each ran it at the edge (a dispatch to its Worker), then went to the Function anyway: two round trips in series where a plain `rsc` request takes one. `payloadAhead` now judges both on the request as it came and hands them on before the middleware runs (`rsc-payload-ahead`), with no outcome, so the Function runs the middleware itself. The second kind is known from the record this isolate holds, looked at with a new non-touching `SharedByteLru.peek`, or, holding none, from the build's `delivery`, with the record read behind the response so the next request decides by it; an expired held record still has its read-again scheduled as `partFrom` does. Under a hold of zero nothing is judged. Build payloads, partial fallbacks, upgraded members, shell-less members, static prefetches and held whole records keep the middleware's run at the edge. What a visitor receives is the Function's answer either way, but for a middleware rewrite onto a large shipped file, as for a plain `rsc` request already. The local check accepts `rsc-payload-ahead` only where the path is covered, generations are served, there is no partial fallback, nothing of the middleware was kept, and one of the two kinds holds; the stack now records a record the edge peeked as evidence of the response.
…on before it A static Pages page or a Pages fallback under a matching middleware ran the middleware at the edge, then went to the Function as `static-page-private` whatever it decided: shared HTML cannot depend on a visitor's decision. A route body did the same where what made it the Function's was already known before the middleware ran. Each cost a dispatch to the middleware's Worker in series with the Function's. `HostedGate` gains `handOn`, asked once a matcher covers the request and before the policy, `start` or the middleware, with the rules already judged on the request as it came (judging them again would spend its routing allowance twice). Static pages and Pages fallbacks hand on there with `static-page-private` and no middleware outcome, so the Function runs the middleware itself. A route body hands on there only where a veto is knowable before the middleware: a private project or a validation, application credentials on the classified headers, header rules that are conditioned, refuse sharing or set a cookie, or a policy that serves no body from records. The vetoes that depend on what the middleware decides stay after it. For the bodies that still run the middleware, their record read now begins beside it (`start`), and is taken by the answer or let go to finish into memory (`abandon`); a record read is counted nowhere among an answer's requests, as before. When the middleware answers itself, behaviour for requests that still run it is unchanged.
The README said a request the middleware redirects, answers or rewrites to a file in storage need never start the application. That no longer holds for a request the edge would hand to the application whatever the middleware decides: a static Pages page a matcher covers, a route body only one visitor may have, a page's Flight only the application renders. Those go to the application at once, which runs the middleware itself. The core readme says the same of route bodies and static pages.
|
Arkor — Preview deployments for arkorlab-upwind Use the checkbox below to choose whether this pull request gets preview deployments.
Checking this builds the latest commit and future pushes. Unchecking stops queued and future builds; a running build can finish and an existing preview stays available. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 30 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. CodeRabbit Verify
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
 [Medium impact] The latest change appears safe to merge. SummaryThe PR skips a separate edge middleware call for requests sent straight to the Function. Route-body record reads now start alongside middleware.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Request covered by middleware] --> B{Request kind}
B -->|Eligible payload or static Pages page| C[Send straight to Function]
C --> D[Function runs middleware]
B -->|Route body| E[Run middleware alongside record read]
E --> F[Serve eligible bytes or forward request]
Reviews (4) · Last reviewed commit: "test(edge): check the body a conditioned..." · Reviewed by Greptile |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 22 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/edge/src/handlers/payload-ahead.ts">
<violation number="1" location="packages/edge/src/handlers/payload-ahead.ts:83">
P1: This unconditional class-member branch bypasses two existing exclusions: static prefetches and requests whose generation is already held. Preserve the prefetch check and only hand off when no generation is held; otherwise these requests lose saved Flight responses and complete-record edge answers.</violation>
</file>
<file name="packages/local/src/part-handoffs.ts">
<violation number="1" location="packages/local/src/part-handoffs.ts:499">
P2: The validation accepts `rsc-payload-ahead` from `delivery: 'resume'` even when the trace captured a complete held generation. The edge rejects that case before consulting `delivery`; distinguish an observed complete record from the no-record fallback so validation catches an ahead handoff that is not eligible.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| } | ||
| const settings = runtimeCacheOf(input.edge.policy); | ||
| const { pathname } = input.url; | ||
| if (memberFlightUnproved(settings, entry, pathname)) { |
There was a problem hiding this comment.
P1: This unconditional class-member branch bypasses two existing exclusions: static prefetches and requests whose generation is already held. Preserve the prefetch check and only hand off when no generation is held; otherwise these requests lose saved Flight responses and complete-record edge answers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/edge/src/handlers/payload-ahead.ts, line 83:
<comment>This unconditional class-member branch bypasses two existing exclusions: static prefetches and requests whose generation is already held. Preserve the prefetch check and only hand off when no generation is held; otherwise these requests lose saved Flight responses and complete-record edge answers.</comment>
<file context>
@@ -0,0 +1,143 @@
+ }
+ const settings = runtimeCacheOf(input.edge.policy);
+ const { pathname } = input.url;
+ if (memberFlightUnproved(settings, entry, pathname)) {
+ return true;
+ }
</file context>
| if (read.classified.get(NEXT_ROUTER_PREFETCH_HEADER) === '1') { | ||
| return `${BEFORE_MIDDLEWARE}, though it is a static prefetch, which saved partial Flight answers`; | ||
| } | ||
| return route.cache?.delivery === 'resume' || readResumable(stack, route) |
There was a problem hiding this comment.
P2: The validation accepts rsc-payload-ahead from delivery: 'resume' even when the trace captured a complete held generation. The edge rejects that case before consulting delivery; distinguish an observed complete record from the no-record fallback so validation catches an ahead handoff that is not eligible.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/local/src/part-handoffs.ts, line 499:
<comment>The validation accepts `rsc-payload-ahead` from `delivery: 'resume'` even when the trace captured a complete held generation. The edge rejects that case before consulting `delivery`; distinguish an observed complete record from the no-record fallback so validation catches an ahead handoff that is not eligible.</comment>
<file context>
@@ -404,6 +416,91 @@ function unheldHandoffFailure(stack: CheckedStack, part: Part, read: HandedOn):
+ if (read.classified.get(NEXT_ROUTER_PREFETCH_HEADER) === '1') {
+ return `${BEFORE_MIDDLEWARE}, though it is a static prefetch, which saved partial Flight answers`;
+ }
+ return route.cache?.delivery === 'resume' || readResumable(stack, route)
+ ? undefined
+ : `${BEFORE_MIDDLEWARE}, though the build finished the page, and no generation of its record the edge read holds a resume`;
</file context>
The core README listed what makes a route body the Function's before the middleware runs, and left out a validation request, which the edge hands on so too (`visitorSpecificAhead`); and the payload passage in `servedAtEdge` opened on a clause that did not read as one (cubic). Not changed: - A class member handed on ahead whatever its prefetch header, with no check for a held generation (cubic): `partGeneration` handed such a member to the Function for a static prefetch as for a navigation, and the only generation it waived the rule for is a shell-less member's own record, which `payloadAhead` never sees (`classification.entry` is undefined for one). - The local check accepting `rsc-payload-ahead` by the build's `delivery` where a complete generation was seen (cubic): the evidence is the stack's, not one request's, and a complete generation is held after a legitimate ahead handoff — the read behind it brings it in — so rejecting on it would fail handoffs the edge made rightly.
![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: 622e27f4dc
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| handOn: (configured) => { | ||
| const reason = refusedAhead(input, configured); | ||
| return reason === undefined ? undefined : input.passthrough(reason); |
There was a problem hiding this comment.
For a route-body request carrying application credentials, or with generations disabled, this early handoff also skips middleware that rewrites the request to a shipped static file. Previously, routedBody called serveHostedFile before evaluating body-sharing restrictions, so those rewrites were answered from edge storage. The Function cannot necessarily replace that behavior: packages/adapter/src/index.ts omits _next/static files from its bundled bytes, and rewriteTargetFiles only registers destinations of configuration rewrites, not arbitrary middleware rewrites. Consequently, a middleware-only rewrite to a file such as the /_next/static/media/large.png target covered by route-body.test.ts can now return a not-found response when the original request has credentials or generations are off. Keep these requests on the middleware path unless the rewrite target can also be answered by the Function.
Useful? React with 👍 / 👎.
…file A route body that its project, a validation, the request's credentials or a header rule made the Function's whatever the middleware decided was handed to the Function before the middleware ran. But a middleware can rewrite such a request onto a shipped file, which the edge serves from storage (`routedBody` serves it before judging the body) and which the Function need not carry — `_next/static`, or a large public file — so the rewrite could now be answered not found (Codex). The middleware runs for every covered route body again. What changes from before the earlier commit is only the record read: begun beside the middleware, but not for a body none of whose answers would take it. Static pages and Pages fallbacks keep being handed on before the middleware: their rewrites already went to the Function.
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
The test of conditioned header rules kept across a middleware checked the answer's headers and dispatches, not its bytes (cubic): it now checks the body the Function answered with too.
Summary
Hand requests the edge can only pass on to the Function on before running the application's middleware at the edge, so the Function runs the middleware itself: one dispatch instead of two in series.
Why
Under a matching middleware, several request kinds ran the middleware at the edge (a dispatch to its Worker) and were then handed to the Function anyway (a second dispatch, behind the first) — where a plain
rscrequest goes to the Function at once. Those kinds are:And a route body's record was read only once the middleware had answered, in series behind it.
Handing them on at once removes a serial round trip — and a cold middleware Worker's start — from time to first byte; reading a body's record beside the middleware takes the read off the path behind it.
Changes
handlers/payload-ahead.ts(payloadAhead, asked aftermiddlewareMatchesand before the header rules) hands on, withrsc-payload-aheadand no middleware outcome:memberFlightUnproved, now shared withpartGeneration);SharedByteLru.peek; an expired one is still read again behind, as answering it would have), or, holding none, from the build'sdelivery, with the record read behind the response so the next request is judged by the record. Not judged under a hold of zero.Build payloads, partial fallbacks, upgraded and shell-less members, static prefetches and complete held records still run the middleware at the edge.
HostedGate.handOn(configured), asked once a matcher covers the request and before anything else is begun, reusing the rules already judged:static-page-private); their rewrites already went to the Function.Route bodies still run the middleware — it may rewrite onto a shipped file, which the edge serves from storage and the Function need not carry — but their record read now begins beside it (
start) and is taken by the answer or let go to finish into memory (abandon); none is begun for a body no answer would take it for (private project or validation, application credentials on the classified request, header rules that are conditioned, refuse sharing or set a cookie).Local check: accepts
rsc-payload-aheadonly where it holds (covered path, generations served, no partial fallback, one of the two kinds, no middleware decision kept), and records peeked records as evidence.README, README.ja and the core README say which requests go to the application before its middleware.
Tests
payload-ahead.test.ts, detecting middleware dispatches byMIDDLEWARE_ONLY_HEADER: both kinds hand on with no dispatch; shell-less members, partial fallbacks, upgraded members, static prefetches and complete held records still run the middleware; a page with nothing held is handed on and the next request is answered at the edge.partial-fallback-wiring.test.ts.part-handoffs.test.ts), accept/reject cases incheck.test.ts, and evidence capture forpeek.part-handoffs/generation-evidence/checksuites, eslint, oxfmt,tsc(edge, local),check:vocabulary.Notes
rscrequests: a middleware rewrite onto a large shipped file the Function does not carry. Static pages and Pages fallbacks already handed a rewrite on to the Function, so nothing changes for them.Review fixes
The core README names a validation request among what hands a body on before the middleware; the payload passage in
servedAtEdgeis reworded (cubic).Codex (
route-body.ts, handing a private body on before the middleware loses a rewrite onto a shipped file): route bodies run the middleware again; only the record read moved beside it.Not changed
payload-ahead.ts, a class member handed on whatever its prefetch header and with no check for a held generation):partGenerationalready handed such a member to the Function for a static prefetch as for a navigation; the only generation it waived the rule for is a shell-less member's own record, which never reachespayloadAhead(classification.entryis undefined for one).part-handoffs.ts, acceptingrsc-payload-aheadbydeliverywhere a complete generation was seen): the evidence is the stack's, not one request's, and a complete generation is held after a legitimate ahead handoff — the read behind it brings it in — so rejecting on it would fail handoffs the edge made rightly.