Skip to content

fix(middleware): run on missing build assets#2242

Open
james-elicx wants to merge 1 commit into
mainfrom
codex/fix-middleware-node-runtime-parity
Open

fix(middleware): run on missing build assets#2242
james-elicx wants to merge 1 commit into
mainfrom
codex/fix-middleware-node-runtime-parity

Conversation

@james-elicx

Copy link
Copy Markdown
Member

Summary

  • let missing /_next/static/* requests reach middleware instead of returning from the static-file fast path
  • preserve the fast path for existing build assets so middleware still does not run for successful chunk requests
  • restore the canonical plain-text 404 Not Found response when middleware/routing does not handle the missing asset
  • keep App Router and Pages Router production server behavior aligned

Failure cluster

The latest Next.js deploy-suite artifact for test/e2e/middleware-general/test/node-runtime.test.ts showed the non-cache assertion:

  • should be able to rewrite on _next/static/chunks/pages/ 404: expected 200, received 404

The related original-request-shape assertion also observed no middleware header for a missing /_next/static/* URL. Data-request assertions from the same suite are owned separately by #2239; the Suspense/app-static cluster is also actively owned elsewhere.

Root cause

The Node production servers served existing build assets before middleware, which is correct, but also returned a static-layer 404 immediately for missing asset-shaped paths. Next.js allows middleware to rewrite or respond to a missing /_next/static/* request before falling back to its plain-text asset 404.

Next.js parity

Regression coverage is ported from:

The local fixture verifies all three parts of the contract:

  1. existing emitted assets return 200 without middleware headers
  2. middleware can handle a missing /_next/static/* request
  3. an unhandled missing asset remains 404 text/plain; charset=utf-8 with body Not Found

Validation

Targeted one-worker tests only:

  • vp test run tests/asset-prefix.test.ts -t "Pages Router with basePath alone serves assets" --maxWorkers=1
  • vp test run tests/asset-prefix.test.ts -t "unset: emits URLs under /_next/static/" --maxWorkers=1
  • vp check packages/vinext/src/server/prod-server.ts tests/asset-prefix.test.ts

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@pkg-pr-new

pkg-pr-new Bot commented Jun 21, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2242
npm i https://pkg.pr.new/vinext@2242

commit: e1882dd

@github-actions

Copy link
Copy Markdown
Contributor
Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original
workers-cache preview production
web preview production

@github-actions

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared e1882dd against base cf9d601 using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 132.8 KB 132.8 KB ⚫ +0.0%
Client entry size (gzip) vinext 118.7 KB 118.7 KB ⚫ +0.0%
Dev server cold start vinext 2.43 s 2.43 s ⚫ -0.0%
Production build time vinext 3.05 s 3.03 s ⚫ -0.6%
RSC entry closure size (gzip) vinext 92.2 KB 92.2 KB ⚫ +0.0%
Server bundle size (gzip) vinext 166.9 KB 166.9 KB ⚫ +0.0%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

@ask-bonk

ask-bonk Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

/bigbonk review

I reviewed the diff against the four request-handling sites that AGENTS.md says must stay in sync. The Node prod-server change itself is correct and well-targeted, but the same fix is missing from the two Cloudflare Workers entries — which is the primary deployment target.

Blocking: Workers entries still short-circuit missing assets before middleware

The PR description says it keeps "App Router and Pages Router production server behavior aligned," and the goal is for middleware to run on missing /_next/static/* requests. But that fix was only applied to server/prod-server.ts (the Node prod servers). The two Cloudflare Workers entries still return a plain-text 404 for any asset-shaped path before middleware runs:

  • App Router (Workers): packages/vinext/src/server/app-router-entry.ts:104-106

    if (isNextStaticPath(url.pathname, __workerBasePath, __workerAssetPathPrefix)) {
      return notFoundStaticAssetResponse();
    }

    This executes before rscHandler(request) (line 132), so middleware never sees a missing /_next/static/* request on Workers.

  • Pages Router (Workers): packages/vinext/src/deploy.ts:625-627

    if (isNextStaticPath(pathname, basePath, assetPathPrefix)) {
      return notFoundStaticAssetResponse();
    }

    Same problem — this runs before runPagesRequest (and therefore before middleware) on line 717.

Per AGENTS.md, Cloudflare Workers is the primary deployment target and dev/prod parity is required; the parity checklist explicitly names cloudflare/worker-entry.ts/deploy.ts alongside the prod servers. As written, the exact bug this PR fixes for Node still exists on Workers: a middleware that rewrites a missing /_next/static/chunks/... request will work in the Node prod server but not when deployed to Workers. That is a silent dev/prod divergence — precisely the class of bug AGENTS.md warns about ("Do not leave known bugs as 'follow-ups' — fix them in the same PR").

These two sites need the same treatment: drop the early notFoundStaticAssetResponse() for missing assets, let the request reach middleware/routing, and convert a resulting 404 back to the canonical plain-text static 404. (Valid assets are still served by the ASSETS binding before the worker runs, so the fast path is preserved automatically on Workers.)

Test coverage gap

The new assertions are good and correctly model the three-part contract, but they only exercise one of the four handlers — the Pages Router Node prod server (startProdServer with disableAppRouter: true, test at tests/asset-prefix.test.ts:691). There is no coverage for:

  1. the App Router Node prod server path that this PR also modified (startAppRouterServer, prod-server.ts:1437), and
  2. either Cloudflare Workers entry.

Because the only new test runs against the one handler that was fixed, CI stays green while the Workers regression ships unnoticed. At minimum, please add an App Router prod-server case mirroring this one; ideally add a Workers-entry case so the parity is actually enforced.

Things that look correct

  • Node Pages Router (prod-server.ts:1674, :1863) and Node App Router (prod-server.ts:1336, :1437) changes are sound: the missingBuildAsset flag is only consulted on the static-miss path (successful assets early-return before middleware, preserving the no-middleware fast path), and cancelResponseBody(response) correctly drains the routed 404 body before emitting the canonical plain-text response.
  • The behavioral premise matches Next.js — the cited middleware-general deploy-suite assertion (should be able to rewrite on _next/static/chunks/pages/ 404) confirms Next.js lets middleware rewrite a missing static chunk before its plain-text asset 404.
  • Targeted test passes locally (tests/asset-prefix.test.ts -t "Pages Router with basePath alone serves assets").

Summary

The Node-side fix is correct, but shipping it without the matching app-router-entry.ts and deploy.ts changes makes the PR title ("run on missing build assets") true only off-Workers. Recommend updating both Workers entries (and adding coverage that actually exercises them) before merge.

@ask-bonk

ask-bonk Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Posted my review to PR #2242. Summary of findings:

Blocking issue: incomplete fix for the primary deployment target

The PR fixes the Node prod servers (prod-server.ts) so middleware runs on missing /_next/static/* requests, but the same short-circuit still exists in both Cloudflare Workers entries — which AGENTS.md designates as the primary deployment target and part of the required parity set:

  • app-router-entry.ts:104-106 returns notFoundStaticAssetResponse() before rscHandler runs middleware.
  • deploy.ts:625-627 returns notFoundStaticAssetResponse() before runPagesRequest runs middleware.

Result: a middleware rewrite of a missing static chunk works on the Node server but silently fails when deployed to Workers — exactly the dev/prod divergence AGENTS.md warns against, left as an in-scope "known bug."

Test gap

The new assertions only exercise the Pages Router Node prod server. There's no coverage for the App Router prod server (also modified here) or either Workers entry, so CI stays green while the Workers regression ships unnoticed.

Correct parts

  • The Node-side missingBuildAsset flag logic, fast-path preservation, and cancelResponseBody draining are all sound.
  • The behavioral premise matches Next.js (confirmed by the cited deploy-suite assertion).
  • The targeted test passes locally.

Recommendation: apply the same change to both Workers entries and add coverage that actually exercises them before merge.

github run

@james-elicx james-elicx marked this pull request as ready for review June 21, 2026 20:13
@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

/bigbonk review

Re-reviewed e1882dd (unchanged since the prior review). I independently verified every claim against the four parity sites in AGENTS.md. The prior /bigbonk findings are accurate and still stand — the Node fix is correct, but the same fix is missing from both Cloudflare Workers entries (the primary deployment target).

Blocking: Workers entries still short-circuit missing assets before middleware

Both Workers entries return a plain-text 404 for any asset-shaped path before middleware runs, so the exact bug this PR fixes for Node still ships on Workers:

  • App Router (Workers): packages/vinext/src/server/app-router-entry.ts:104-106

    if (isNextStaticPath(url.pathname, __workerBasePath, __workerAssetPathPrefix)) {
      return notFoundStaticAssetResponse();
    }

    This runs before rscHandler(request, ctx) at line 132, so middleware never sees a missing /_next/static/* request.

  • Pages Router (Workers): packages/vinext/src/deploy.ts:625-627

    if (isNextStaticPath(pathname, basePath, assetPathPrefix)) {
      return notFoundStaticAssetResponse();
    }

    This runs before runPagesRequest(request, deps) at line 717 (and therefore before middleware).

A middleware that rewrites or responds to a missing /_next/static/chunks/... request will work in the Node prod server but silently fail on Workers — exactly the dev/prod divergence AGENTS.md warns against ("Do not leave known bugs as 'follow-ups' — fix them in the same PR"). The PR title ("run on missing build assets") and the description's "keep App Router and Pages Router production server behavior aligned" are only true off-Workers as written.

Both sites need the same treatment as the Node fix: drop the early notFoundStaticAssetResponse() for missing assets, let the request reach middleware/routing, then convert a resulting 404 back to the canonical plain-text static 404. Valid assets are served by the ASSETS binding before the worker runs, so the no-middleware fast path is preserved automatically.

Test coverage gap

The new assertions correctly model the three-part contract (existing assets 200 with no middleware header, middleware can handle a missing asset, unhandled miss is 404 text/plain "Not Found"), but they only exercise one of the four handlers — the Pages Router Node prod server (tests/asset-prefix.test.ts:691, startProdServer with disableAppRouter: true). There is no coverage for:

  1. the App Router Node prod server path this PR also modified (prod-server.ts:1437), and
  2. either Cloudflare Workers entry.

Because the only new test hits the one handler that was fixed, CI stays green while the Workers regression ships unnoticed. At minimum add an App Router prod-server case; ideally add a Workers-entry case so the parity is actually enforced.

Verified correct

  • Node fix is sound. App Router (prod-server.ts:1325, :1437) sets missingBuildAsset only after a confirmed static miss; Pages Router (prod-server.ts:1674, :1860) computes it from pagesAssetLookup !== null but a successful serve early-returns first, so the net behavior matches. In both, the flag is consulted only on the static-miss path, the request now reaches rscHandler / runPagesRequest (and thus middleware), and cancelResponseBody(response) correctly drains the routed 404 body before emitting the canonical plain-text response.
  • Behavioral premise matches Next.js — the cited middleware-general assertion (should be able to rewrite on _next/static/chunks/pages/ 404) confirms Next.js lets middleware rewrite a missing static chunk before its plain-text asset 404.

Recommendation

Apply the same fix to app-router-entry.ts and deploy.ts, and add coverage that exercises the App Router prod server and at least one Workers entry, before merge.

@ask-bonk

ask-bonk Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Posted my review to PR #2242. I independently verified every finding against the four parity sites in AGENTS.md (the PR is unchanged at e1882dd).

Summary

Blocking: incomplete fix for the primary deployment target (Workers)

The Node prod-server fix is correct, but the same short-circuit still exists in both Cloudflare Workers entries:

  • app-router-entry.ts:104-106 returns notFoundStaticAssetResponse() before rscHandler (line 132) runs middleware.
  • deploy.ts:625-627 returns notFoundStaticAssetResponse() before runPagesRequest (line 717) runs middleware.

Result: a middleware rewrite of a missing /_next/static/* request works on Node but silently fails when deployed to Workers — the dev/prod divergence AGENTS.md explicitly warns against leaving as a follow-up.

Test gap

The new assertions only exercise the Pages Router Node prod server (tests/asset-prefix.test.ts:691). No coverage for the App Router Node prod server (also modified) or either Workers entry, so CI stays green while the Workers regression ships.

Verified correct

  • The Node-side missingBuildAsset flag logic, fast-path preservation, and cancelResponseBody draining are all sound in both App Router (prod-server.ts:1437) and Pages Router (prod-server.ts:1860).
  • The behavioral premise matches Next.js (the cited middleware-general deploy-suite assertion).

Recommendation: Apply the fix to both Workers entries and add coverage that exercises them before merge.

github run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant