Dynamic IO: Improve error handling #73607

unstubbable · 2024-12-06T15:53:09Z

This PR improves error handling when dynamicIO is enabled.

prevents well-known errors from being logged
avoids unhandled rejections being logged during prerendering in dev mode
avoids hanging responses when well-known or unexpected errors are thrown
ensures pages with notFound used in the top level can be prerendered statically

addresses #73210 (comment)
closes NAR-48

unstubbable · 2024-12-06T15:53:21Z

Dynamic IO: Improve error handling #73607 👈 (View in Graphite)
Fix missing client reference manifest error when using route groups #73606
canary

This stack of pull requests is managed by Graphite. Learn more about stacking.

ijjk · 2024-12-06T16:02:16Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	vercel/next.js hl/dynamic-io-improve-error-handling	Change
buildDuration	20.4s	17.6s	N/A
buildDurationCached	16.9s	14.8s	N/A
nodeModulesSize	409 MB	409 MB	⚠️ +171 kB
nextStartRea..uration (ms)	475ms	476ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	vercel/next.js hl/dynamic-io-improve-error-handling	Change
1187-HASH.js gzip	50.1 kB	50.1 kB	N/A
8276.HASH.js gzip	169 B	168 B	N/A
8377-HASH.js gzip	5.3 kB	5.3 kB	N/A
bccd1874-HASH.js gzip	53 kB	53 kB	N/A
framework-HASH.js gzip	57.5 kB	57.5 kB	N/A
main-app-HASH.js gzip	232 B	233 B	N/A
main-HASH.js gzip	33.7 kB	33.7 kB	N/A
webpack-HASH.js gzip	1.71 kB	1.71 kB	N/A
Overall change	0 B	0 B	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	vercel/next.js hl/dynamic-io-improve-error-handling	Change
polyfills-HASH.js gzip	39.4 kB	39.4 kB	✓
Overall change	39.4 kB	39.4 kB	✓

Client Pages

	vercel/next.js canary	vercel/next.js hl/dynamic-io-improve-error-handling	Change
_app-HASH.js gzip	193 B	193 B	✓
_error-HASH.js gzip	193 B	193 B	✓
amp-HASH.js gzip	513 B	511 B	N/A
css-HASH.js gzip	343 B	342 B	N/A
dynamic-HASH.js gzip	1.84 kB	1.84 kB	N/A
edge-ssr-HASH.js gzip	265 B	265 B	✓
head-HASH.js gzip	363 B	362 B	N/A
hooks-HASH.js gzip	393 B	392 B	N/A
image-HASH.js gzip	4.44 kB	4.43 kB	N/A
index-HASH.js gzip	268 B	268 B	✓
link-HASH.js gzip	2.35 kB	2.34 kB	N/A
routerDirect..HASH.js gzip	328 B	328 B	✓
script-HASH.js gzip	397 B	397 B	✓
withRouter-HASH.js gzip	323 B	326 B	N/A
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	1.75 kB	1.75 kB	✓

Client Build Manifests

	vercel/next.js canary	vercel/next.js hl/dynamic-io-improve-error-handling	Change
_buildManifest.js gzip	746 B	747 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	vercel/next.js hl/dynamic-io-improve-error-handling	Change
index.html gzip	523 B	524 B	N/A
link.html gzip	537 B	537 B	✓
withRouter.html gzip	519 B	521 B	N/A
Overall change	537 B	537 B	✓

Edge SSR bundle Size Overall increase ⚠️

	vercel/next.js canary	vercel/next.js hl/dynamic-io-improve-error-handling	Change
edge-ssr.js gzip	128 kB	128 kB	N/A
page.js gzip	202 kB	203 kB	⚠️ +577 B
Overall change	202 kB	203 kB	⚠️ +577 B

Middleware size

	vercel/next.js canary	vercel/next.js hl/dynamic-io-improve-error-handling	Change
middleware-b..fest.js gzip	667 B	665 B	N/A
middleware-r..fest.js gzip	155 B	156 B	N/A
middleware.js gzip	31 kB	31 kB	N/A
edge-runtime..pack.js gzip	844 B	844 B	✓
Overall change	844 B	844 B	✓

Next Runtimes Overall increase ⚠️

	vercel/next.js canary	vercel/next.js hl/dynamic-io-improve-error-handling	Change
797-experime...dev.js gzip	322 B	322 B	✓
797.runtime.dev.js gzip	314 B	314 B	✓
app-page-exp...dev.js gzip	322 kB	322 kB	⚠️ +128 B
app-page-exp..prod.js gzip	127 kB	127 kB	N/A
app-page-tur..prod.js gzip	139 kB	140 kB	N/A
app-page-tur..prod.js gzip	135 kB	135 kB	N/A
app-page.run...dev.js gzip	312 kB	312 kB	⚠️ +122 B
app-page.run..prod.js gzip	122 kB	122 kB	N/A
app-route-ex...dev.js gzip	36.8 kB	37.1 kB	⚠️ +308 B
app-route-ex..prod.js gzip	25 kB	25.1 kB	⚠️ +143 B
app-route-tu..prod.js gzip	25 kB	25.1 kB	⚠️ +143 B
app-route-tu..prod.js gzip	24.8 kB	24.9 kB	⚠️ +135 B
app-route.ru...dev.js gzip	38.4 kB	38.7 kB	⚠️ +306 B
app-route.ru..prod.js gzip	24.8 kB	24.9 kB	⚠️ +135 B
pages-api-tu..prod.js gzip	9.56 kB	9.56 kB	✓
pages-api.ru...dev.js gzip	11.4 kB	11.4 kB	✓
pages-api.ru..prod.js gzip	9.56 kB	9.56 kB	✓
pages-turbo...prod.js gzip	21.3 kB	21.3 kB	✓
pages.runtim...dev.js gzip	27 kB	27 kB	✓
pages.runtim..prod.js gzip	21.3 kB	21.3 kB	✓
server.runti..prod.js gzip	916 kB	916 kB	✓
Overall change	1.83 MB	1.83 MB	⚠️ +1.42 kB

build cache Overall increase ⚠️

	vercel/next.js canary	vercel/next.js hl/dynamic-io-improve-error-handling	Change
0.pack gzip	2.03 MB	2.04 MB	⚠️ +1.1 kB
index.pack gzip	146 kB	146 kB	N/A
Overall change	2.03 MB	2.04 MB	⚠️ +1.1 kB

Diff details

Diff for main-HASH.js

Diff too large to display

Diff for app-page-exp..ntime.dev.js

Diff too large to display

Diff for app-page-exp..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page.runtime.dev.js

Diff too large to display

Diff for app-page.runtime.prod.js

Diff too large to display

Diff for app-route-ex..ntime.dev.js

Diff too large to display

Diff for app-route-ex..time.prod.js

Diff too large to display

Diff for app-route-tu..time.prod.js

Diff too large to display

Diff for app-route-tu..time.prod.js

Diff too large to display

Diff for app-route.runtime.dev.js

Diff too large to display

Diff for app-route.ru..time.prod.js

Diff too large to display

Commit: f0fd675

ijjk · 2024-12-06T16:20:44Z

Failing test suites

Commit: f0fd675

pnpm test-dev test/development/acceptance-app/ReactRefreshLogBox.test.ts

ReactRefreshLogBox app default > server component can recover from error thrown in the module

Expand output

● ReactRefreshLogBox app default › server component can recover from error thrown in the module

Expected Redbox but found none

  1060 |
  1061 |       await next.patchFile('index.js', "throw new Error('module error')")
> 1062 |       await session.assertHasRedbox()
       |       ^
  1063 |       await next.patchFile(
  1064 |         'index.js',
  1065 |         'export default function Page() {return <p>hello world</p>}'

  at Object.<anonymous> (development/acceptance-app/ReactRefreshLogBox.test.ts:1062:7)

Read more about building and testing Next.js in contributing.md.

packages/next/src/server/app-render/app-render.tsx

gnoff · 2024-12-06T17:07:07Z

packages/next/src/server/app-render/create-error-handler.tsx

+export function getDigestForWellKnownError(error: unknown): string | undefined {
+  // If we're bailing out to CSR, we don't need to log the error.
+  if (isBailoutToCSRError(error)) return error.digest
+
+  // If this is a navigation error, we don't need to log the error.
+  if (isNextRouterError(error)) return error.digest
+
+  // If this error occurs, we know that we should be stopping the static
+  // render. This is only thrown in static generation when PPR is not enabled,
+  // which causes the whole page to be marked as dynamic. We don't need to
+  // tell the user about this error, as it's not actionable.
+  if (isDynamicServerError(error)) return error.digest
+
+  return undefined
+}


I wonder if it's time to brand our errors so we don't have to do these delimited type checks. Is the semantic we're going for that we want to forward digests from errors that the framework created? This is important because we don't want a userland error with a digest property to hit this pathway. We can use instanceof checks I suppose. One thing to consider is that this function might only work in RSC layer b/c if any of these predicates check instance identity it won't work across the serialization boundary whereas a digest check will surivive since React forwards those.

I guess it is also used on the client so they all have to check digest exclusively. This does mean it is relatively easy to create your own errors that appear to match these error types. Unlikely that this would actually happen but one way we can defend against that is for errors in RSC to never respect a passed in digest, only set the digest from some other source. Then the client can trust that a matching digest was always produced by the framework

It's tricky, we currently also need to preserve digests from the Cache environment.

gnoff · 2024-12-06T17:13:58Z

packages/next/src/server/app-render/app-render.tsx

+
+      // This is intentionally using the readable datastream from the main
+      // render rather than the flight data from the error page render
+      const flightStream =
+        reactServerPrerenderResult instanceof ServerPrerenderStreamResult
+          ? reactServerPrerenderResult.asStream()
+          : reactServerPrerenderResult.consumeAsStream()
+


The error handling pathway needs to be moved into the implementations of DIO X PPR so we can avoid stuff like this. It's fine to land for now but it just shows how brittle the current construction is. Lmk if you want to take that on :)

I thought the 4-way branching is supposed to be temporary, so this prerender result union type would then also go away when we merge the branches again. But I guess that won't happen soon?

well I think the error handling is also imprecise now b/c we should probably handle a fatal error in the RSC render differentaly than just not producing a shell in the SSR pass. At the moment it's all a little confused. We also need to ideally render the not found and error responses using DIO semantics so we might need to extract each branch into it's own implementation that can be called into again from within certain error cases. We can denormalize into each branch so that when we finally delete the unused ones we know we have only the necessary error handling semantics whereas today it is all sort of jumbled together

ijjk added created-by: Next.js team PRs by the Next.js team. tests type: next labels Dec 6, 2024

unstubbable mentioned this pull request Dec 6, 2024

Fix missing client reference manifest error when using route groups #73606

Merged

unstubbable changed the title ~~Add failing tests for notFound with dynamicIO~~ Dynamic IO: Improve error handling Dec 6, 2024

unstubbable marked this pull request as ready for review December 6, 2024 16:43

lubieowoce reviewed Dec 6, 2024

View reviewed changes

packages/next/src/server/app-render/app-render.tsx Show resolved Hide resolved

gnoff approved these changes Dec 6, 2024

View reviewed changes

gnoff reviewed Dec 6, 2024

View reviewed changes

unstubbable mentioned this pull request Dec 6, 2024

Make notFound() work in "use cache" page #73210

Merged

unstubbable changed the base branch from hl/fix-missing-client-reference-manifest-route-groups to graphite-base/73607 December 6, 2024 19:43

unstubbable force-pushed the hl/dynamic-io-improve-error-handling branch from 8780549 to 886742a Compare December 6, 2024 19:43

unstubbable force-pushed the graphite-base/73607 branch from 1915f89 to 2772f8e Compare December 6, 2024 19:43

unstubbable changed the base branch from graphite-base/73607 to canary December 6, 2024 19:44

unstubbable force-pushed the hl/dynamic-io-improve-error-handling branch from 886742a to 3c39bcd Compare December 6, 2024 19:44

unstubbable added 2 commits December 6, 2024 21:09

Add failing tests for notFound with dynamicIO

957e3ed

Add failing test for top-level errors with dynamicIO

fd18c85

unstubbable force-pushed the hl/dynamic-io-improve-error-handling branch from 3c39bcd to 6500c85 Compare December 6, 2024 20:09

Dynamic IO: Improve error handling

f0fd675

unstubbable force-pushed the hl/dynamic-io-improve-error-handling branch from 6500c85 to f0fd675 Compare December 6, 2024 21:07

unstubbable enabled auto-merge (squash) December 6, 2024 21:28

unstubbable merged commit a28160a into canary Dec 6, 2024
107 of 112 checks passed

unstubbable deleted the hl/dynamic-io-improve-error-handling branch December 6, 2024 21:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic IO: Improve error handling #73607

Dynamic IO: Improve error handling #73607

unstubbable commented Dec 6, 2024 •

edited

Loading

unstubbable commented Dec 6, 2024 •

edited

Loading

ijjk commented Dec 6, 2024 •

edited

Loading

ijjk commented Dec 6, 2024 •

edited

Loading

gnoff Dec 6, 2024

gnoff Dec 6, 2024

unstubbable Dec 6, 2024

gnoff Dec 6, 2024

unstubbable Dec 6, 2024

gnoff Dec 6, 2024

Dynamic IO: Improve error handling #73607

Dynamic IO: Improve error handling #73607

Conversation

unstubbable commented Dec 6, 2024 • edited Loading

unstubbable commented Dec 6, 2024 • edited Loading

ijjk commented Dec 6, 2024 • edited Loading

Stats from current PR

ijjk commented Dec 6, 2024 • edited Loading

Failing test suites

gnoff Dec 6, 2024

Choose a reason for hiding this comment

gnoff Dec 6, 2024

Choose a reason for hiding this comment

unstubbable Dec 6, 2024

Choose a reason for hiding this comment

gnoff Dec 6, 2024

Choose a reason for hiding this comment

unstubbable Dec 6, 2024

Choose a reason for hiding this comment

gnoff Dec 6, 2024

Choose a reason for hiding this comment

unstubbable commented Dec 6, 2024 •

edited

Loading

unstubbable commented Dec 6, 2024 •

edited

Loading

ijjk commented Dec 6, 2024 •

edited

Loading

ijjk commented Dec 6, 2024 •

edited

Loading