fix(actions): return body-limit errors without closing responses - #2026
Conversation
commit: |
|
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
/bigbonk review
Reviewed the body-limit / backpressure changes against Next.js v16.2.6 behavior and traced the stream lifecycle through the entry, RSC handler, middleware, and prod-server paths. The Next.js parity for the HTTP-access-fallback restriction is correct, but I found one real correctness bug, one no-op/dead-code issue, and a couple of smaller notes.
1. new Uint8Array(chunk.buffer, …) aliases Node's pooled buffer — data corruption risk (blocking)
packages/vinext/src/server/prod-server.ts:165
controller.enqueue(new Uint8Array(chunk.buffer, chunk.byteOffset, chunk.byteLength));This enqueues a view into the chunk's underlying ArrayBuffer rather than a copy. Node's IncomingMessage/socket reads are backed by a shared internal buffer pool (Buffer.allocUnsafe), so consecutive data chunks frequently share the same ArrayBuffer. Demonstrated locally:
captured views share underlying buffer for chunk[0] and chunk[1]? true
v1 before slab reused: [1,1,1,1]
v1 after slab reused: [9,9,9,9] // queued view corrupted
The old code (new Uint8Array(chunk)) copied the bytes, so this was safe. The new backpressure design makes this worse, not better: chunks now sit queued in the ReadableStream (paused/resumed via pull) for longer, giving Node more opportunity to reuse the pooled slab region before the queued view is consumed. The result is intermittent body corruption that won't reproduce deterministically in tests.
Fix: copy the bytes when enqueuing, e.g. controller.enqueue(Uint8Array.prototype.slice.call(chunk)) or new Uint8Array(chunk) (the previous behavior), or controller.enqueue(new Uint8Array(chunk.subarray())) — anything that does not alias the pool.
2. sourceRequest.body.cancel() after cloneRequestWithHeaders is dead code (no-op)
packages/vinext/src/server/app-router-entry.ts:120-124 and packages/vinext/src/server/app-rsc-handler.ts:1067-1070
const sourceRequest = request;
request = cloneRequestWithHeaders(sourceRequest, filteredHeaders);
if (process.env.NODE_ENV !== "development" && sourceRequest.body) {
void sourceRequest.body.cancel().catch(() => {});
}cloneRequestWithHeaders does new Request(request, { headers }) (request-pipeline.ts:658). Constructing a Request from an existing one transfers/locks the source body into the new request. So by the time we reach the cancel() call, sourceRequest.body is already locked. Verified:
src.bodyUsed after new Request: true
src.body locked: true
src cancel threw: TypeError Invalid state: ReadableStream is locked
cloned text: "hello world" // clone still has the body
The TypeError is swallowed by .catch(() => {}), so this never cancels anything in the normal path. Two problems:
- It does nothing toward the stated goal (no body is drained here).
- In the rare fallback branch of
cloneRequestWithHeaders(thecatchthat passesbody: request.bodyby reference, request-pipeline.ts:663), the source and clone share the same stream — there, a successfulcancel()would destroy the clone's body and break every POST that reaches a route handler/Server Action. Today it's masked only because the standard path locks the stream first.
Either drop these two blocks, or restructure so the drain targets the branch you actually intend to discard (e.g. tee/clone explicitly and cancel the discarded branch before it's locked). As written it's confusing dead code at best and a latent footgun at worst.
The genuinely effective drain is the one in app-server-action-execution.ts:1040-1043 (cancels options.request.body on the content-length > maxActionBodySize short-circuit, before the body is locked) — that one is correct.
3. Middleware finally body cancel — verify the mwRequest === request aliasing case
packages/vinext/src/server/middleware-runtime.ts:317-321 cancels nextRequest.body in a finally that runs even when middleware returns NextResponse.next() and the request continues downstream.
This is currently safe because createNextRequest (line 214) clones when request.body && !request.bodyUsed, so nextRequest.body is an isolated tee branch. But when request.body is null or already used, mwRequest = request (same object), so nextRequest.body IS options.request.body. It happens to be safe only because that branch implies the body is null/consumed. Please add a comment documenting this invariant, or guard the cancel on the clone explicitly — it's fragile to a future refactor of createNextRequest.
4. Next.js parity (looks correct)
createServerActionHttpFallbackErrorrestricting to 401/403/404 (app-browser-action-result.ts:76) matches Next.jsALLOWED_CODESinhttp-access-fallback.ts(onlyNOT_FOUND/FORBIDDEN/UNAUTHORIZED). 500s correctly fall through to the serialized action error, matching the server-action-reducer behavior (res.status >= 400 && content-type === 'text/plain'→throw new Error(message)). Good.void reader.cancel()intext-stream.ts:64andapp-server-action-execution.ts:527avoids hanging on the unconsumed tee sibling;releaseLock()after a fire-and-forget cancel is safe (verified). Good, and the new sibling-branch tests cover it.
Summary
|
Review posted on PR #2026. Summary of findings: Review summaryI traced the body-limit/backpressure changes through the entry, RSC handler, middleware, and prod-server paths and validated stream semantics with standalone Node reproductions. Blocking — data corruption (#1): Dead code / footgun (#2): The Robustness note (#3): The middleware Parity confirmed (#4): Restricting HTTP-access-fallback to 401/403/404 matches Next.js The |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review
Re-reviewed after 2beea30 fix(server): own Node request stream chunks. The previously-blocking data-corruption bug is resolved. One issue from the prior round (sourceRequest.body.cancel() dead code / footgun) remains unaddressed.
1. ✅ Resolved — Node chunk aliasing (was blocking)
prod-server.ts:165 now copies bytes (new Uint8Array(chunk)) instead of aliasing Node's pooled ArrayBuffer (new Uint8Array(chunk.buffer, …)). The added regression test node-to-web-request.test.ts ("owns queued bytes when Node reuses a pooled chunk buffer") covers the slab-reuse case. Verified the test passes. Good fix.
2. ⚠️ Still present — sourceRequest.body.cancel() after cloneRequestWithHeaders is a no-op (and a latent footgun)
app-router-entry.ts:122-124 and app-rsc-handler.ts:1068-1070:
const sourceRequest = request;
request = cloneRequestWithHeaders(sourceRequest, filteredHeaders);
if (process.env.NODE_ENV !== "development" && sourceRequest.body) {
void sourceRequest.body.cancel().catch(() => {});
}cloneRequestWithHeaders (request-pipeline.ts:658) does new Request(request, { headers }), which transfers/locks the source body. By the time cancel() runs, sourceRequest.body is already locked. Reproduced in this runtime:
src.bodyUsed after new Request: true
src.body locked: true
src cancel threw: TypeError Invalid state: ReadableStream is locked // swallowed by .catch
cloned text: "hello world" // clone retains the full body
Two problems, unchanged from last round:
- Normal path: the
cancel()throws and is swallowed — it drains nothing, so it does nothing toward the stated goal. - Fallback path (request-pipeline.ts:663,
body: request.body ?? undefined— by reference): source and clone share the same stream. A successful cancel here destroys the clone's body. Reproduced:
src cancel succeeded (would destroy shared stream)
cloned.text() threw: TypeError Body is unusable: Body has already been read
Today this is masked only because the standard path locks the stream first. It's confusing dead code in the common case and a latent breakage of every POST that reaches a route handler / Server Action if the fallback path is ever taken. Recommend dropping both blocks (the body is owned by request after the clone and is consumed normally), or restructuring to explicitly tee and cancel the discarded branch before it's locked.
The genuinely effective drain remains app-server-action-execution.ts:1042-1043, which cancels options.request.body on the content-length > maxActionBodySize short-circuit before the body is locked — that one is correct.
3. Note — middleware finally body cancel relies on a clone invariant
middleware-runtime.ts:317-321 cancels nextRequest.body in a finally that also runs on the NextResponse.next() continue path. This is safe today because createNextRequest (line 214) takes the request.clone() branch precisely when request.body && !request.bodyUsed, so a non-null nextRequest.body is always an isolated tee branch. When mwRequest === request (the alias branch), the body is null/consumed, so the nextRequest.body guard is falsy. The invariant is correct but fragile to a future createNextRequest refactor — worth a comment documenting it.
4. ✅ Parity confirmed
createServerActionHttpFallbackErrorrestricting to 401/403/404 (app-browser-action-result.ts) matches Next.jsALLOWED_CODES(NOT_FOUND/FORBIDDEN/UNAUTHORIZED); 500s correctly fall through as serialized action errors.void reader.cancel()+releaseLock()changes intext-stream.tsandapp-server-action-execution.ts:527are safe and covered by the new sibling-branch tests.
Summary
- #1 fixed — no longer blocking.
- #2 should still be removed or restructured — non-functional in the normal path, latent footgun in the fallback path.
- #3 is a maintainability note.
Recommend addressing #2 before merge.
|
Review posted on PR #2026. Review summaryI re-reviewed the body-limit/backpressure changes after the latest commit ( #1 — Resolved (was blocking): The Node chunk-aliasing data-corruption bug is fixed. #2 — Still present (recommend fixing before merge): The
#3 — Robustness note: The middleware #4 — Parity confirmed: 401/403/404 HTTP-access-fallback restriction matches Next.js Net: the previously-blocking bug is fixed; #2 remains the one actionable item before merge. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review
Re-reviewed after the new commit d28ae28 fix(server): preserve cloned request bodies. Both previously-flagged issues are now resolved, and I re-verified the stream/parity behavior. No actionable findings remain.
1. ✅ Resolved — Node chunk aliasing (was blocking in round 1)
prod-server.ts:165 copies bytes via new Uint8Array(chunk) instead of aliasing Node's pooled ArrayBuffer. The regression test node-to-web-request.test.ts ("owns queued bytes when Node reuses a pooled chunk buffer") covers the slab-reuse case. Verified passing.
2. ✅ Resolved — sourceRequest.body.cancel() dead code / footgun (was flagged rounds 1 & 2)
d28ae28 removes both blocks:
app-router-entry.ts— thesourceRequestcapture + post-clonecancel()is gone; now justrequest = cloneRequestWithHeaders(request, filteredHeaders).app-rsc-handler.ts— therawRequest.body.cancel()block is gone.
This eliminates both the no-op (the clone locks the source body so cancel() always threw and was swallowed) and the latent fallback-path footgun (a successful cancel destroying the shared clone body). The body is owned by the clone and consumed normally downstream. Good.
3. Note (unchanged) — middleware finally body cancel relies on a clone invariant
middleware-runtime.ts:317-321 cancels nextRequest.body in a finally that also runs on the NextResponse.next() continue path. I re-traced createNextRequest:
request.body && !request.bodyUsed→mwRequest = request.clone(), so a non-nullnextRequest.bodyis always an isolated tee branch — safe to cancel.- Otherwise
mwRequest === request(alias), but the body is null/consumed, so thenextRequest.bodyguard is falsy — no cancel runs. - The
new Request(mwUrl, mwRequest)rebuild at line 232 transfers the (clone's) body into the new request, sonextRequest.bodynever aliasesoptions.request.body.
The invariant is correct but fragile to a future createNextRequest refactor. A one-line comment documenting it would help, but this is a maintainability note, not a blocker.
4. ✅ Parity confirmed
createServerActionHttpFallbackErrornow restricts to 401/403/404 (app-browser-action-result.ts:76), matching Next.jsALLOWED_CODES(UNAUTHORIZED/FORBIDDEN/NOT_FOUND); 500s correctly fall through as serialized action errors.void reader.cancel()+releaseLock()changes intext-stream.tsandapp-server-action-execution.ts:527are safe; the effective content-length short-circuit drain atapp-server-action-execution.ts:1042cancels the body before it's locked.readNodeStreamcancel()correctly drains (req.resume()) without destroying the Node request — covered by the new "cancels without destroying the Node request and drains the remainder" test.
Verification
vp test runonnode-to-web-request.test.ts,app-server-action-execution.test.ts,app-router-production-server.test.ts,app-browser-entry.test.ts→ 340 passed.vp checkon all 7 changed source files → clean (format, lint, types).
Note on the diff vs main
The branch is currently 3 commits behind main (#2031, #1940, #2023). A git diff main..HEAD shows some unrelated image-quality and dedup-params tests as "deleted" — those are main-only additions the branch hasn't merged yet, not regressions from this PR. A rebase/merge before landing will resolve it.
Summary
|
Review posted on PR #2026. Review summaryI re-reviewed after the new commit #1 — Resolved (was blocking): #2 — Resolved (this round): #3 — Maintainability note: The middleware #4 — Parity confirmed: 401/403/404 HTTP-access-fallback restriction matches Next.js Verification: 340 tests passed across the 4 relevant files; Net: No remaining blockers. The only open item is the optional #3 documentation comment. LGTM. I also noted that the branch is 3 commits behind |
PR #2026 local regression confirmationVerdictThe deploy-suite failure from run 27549611191 does not reproduce locally on PR head d28ae28. No code changes were made. Environment
Results
All three deployments completed successfully and reached the assertion. The full suite also passed every downstream action test that had cascaded after the CI timeout. The CI-only timeout is therefore best classified as a one-run flake/harness stall rather than a confirmed branch regression. |
…ng (#2741) * fix(server): transfer request bodies into NextRequest instead of teeing `Request.clone()` tees the body stream, and a tee branch that is never read buffers every chunk the other branch pulls. The NextRequest constructor cloned any body-bearing input, so wrapping the incoming request left an unread branch holding the whole body. A route handler that streams a 128 MiB upload to storage — O(1) memory by design — retained the full 128 MiB instead, letting an unauthenticated client exhaust the process or Worker isolate by repeating the request. Upstream Next.js does `super(input, init)` here, transferring the body rather than branching it. Match that. Callers that genuinely need two live branches (middleware vs. downstream routing) already clone explicitly, so nothing loses the isolation added in #1132. The clone also defeated the mitigation added in #2026: `executeMiddleware` cancels the middleware body branch in a `finally`, but the constructor's extra tee sat between that cancel and the branch actually accumulating chunks, so the cancel released a branch nobody was filling. With the clone gone the existing cancel reaches the real branch again. Drop the same dead tee from the basePath re-prefix path in `createTrackedAppRouteRequest`, where the source request is replaced outright and its body is never read again. Measured with a 128 MiB streamed body, reading through and discarding: before: arrayBuffers +128.0 MiB (route handler wrap, and middleware that does not touch the body) after: arrayBuffers +0.0 MiB, matching an unwrapped baseline * fix(server): avoid teeing normalized RSC request bodies * fix(middleware): release isolated request body branches --------- Co-authored-by: James <james@eli.cx>
1 participant
Summary
Next.js parity
Fixes the plaintext and multipart failures in:
test/e2e/app-dir/actions/app-action-size-limit-invalid.test.tsValidation
Something went wrong!and excludesNEXT_HTTP_ERROR_FALLBACK;500vp checkand vinext build passed