fix: differentiate between 404 and 500 when rendering an error page

[eltigerchino]

fixes #10556

Differentiating between 404 and 500 when rendering an error page prevents the load fetch from returning an internal error when it should be making a fetch request.

This fails because SvelteKit tries to render an error page, fetch the prerendered endpoint, but the fetch returns a 500 when it detects state.error is true (set to true because we are rendering a 404 error page).

I couldn't include a test without it affecting other tests. This is because the reproduction requires adding a fetch to a prerendered endpoint in the root layout (causing other tests to fail).

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

[changeset-bot]

🦋 Changeset detected

Latest commit: 13aea04

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[dummdidumm]

Mhhm this is a tough one. It feels a bit weird to use the status code for this - could you end up in the root layout with state.error in other circumstances where you would also want to load a prerendered file? Is something like "try real fetch first if state.error and state.depth > 0 (which indicates an internal fetch request" better?

[eltigerchino]

Is something like "try real fetch first if state.error and state.depth > 0 (which indicates an internal fetch request" better?

Replaced the fix with this and I think it works well.

[gtm-nayan]

I think this could cause an infinite chain of requests.

Eg:
A request comes for kit.svelte.dev/non-existent, which is a 404 so it tries to render the error page, which makes a request to /nav.json but maybe that had a typo in the path and then that response also tries to do the same... and so on?

[gtm-nayan]

I borked it

[dummdidumm]

Gah there's no easy solution, is there? 😓 How could we catch this case of "error from within error state", which should probably reroute to the fallback error.html?

[eltigerchino]

oh whoops. For some reason I thought the below would kick in. Would adding something similar solve this issue?

kit/packages/kit/src/runtime/server/page/index.js

Lines 19 to 22 in 8c642cf

	/**
	* The maximum request depth permitted before assuming we're stuck in an infinite loop
	*/
	const MAX_DEPTH = 10;

kit/packages/kit/src/runtime/server/page/index.js

Lines 34 to 39 in 8c642cf

	if (state.depth > MAX_DEPTH) {
	// infinite request cycle detected
	return text(`Not found: ${event.url.pathname}`, {
	status: 404 // TODO in some cases this should be 500. not sure how to differentiate
	});
	}

[gtm-nayan]

That'd kick in if the state was saved, but with the fetch it ends up being a completely new request that the server doesn't know it's related to the previous one in any way.

[eltigerchino]

Hmm, maybe a custom header to identify the request? Or do we generally avoid adding extra headers?

[eltigerchino]

I've added a header to the fetch request so that the server can identify when we're coming from an error state. The error.html will get returned in this case to prevent the infinite request chain.

[benmccann]

@s3812497 a heads up that this fix may not have worked: sveltejs/svelte#9337 (comment)

[gtm-nayan]

I checked the Kit site while reviewing this PR and it worked on that, not sure what's going on with the Svelte site.
random PR preview a few days before the merge: https://kit-svelte-n5vpuv07n-svelte.vercel.app/wat which is a 500, whereas the preview for this PR is a 404

[eltigerchino]

I tested the svelte site locally and it's happening because of an error while rendering the root layout.
https://github.com/sveltejs/svelte/blob/28d6b34598266d352be999697a4588aae3772a23/sites/svelte.dev/src/routes/%2Blayout.svelte#L27

TypeError: Cannot read properties of null (reading 'startsWith')

I think in this case, it's working correctly. Since the error occurred while rendering the default error page, we get the static fallback error page. However, the error message is incorrect. It should be "Internal Error" although the route we are navigating to doesn't exist.

kit/packages/kit/src/runtime/server/utils.js

Line 107 in fb611ef

message: event.route.id != null ? 'Internal Error' : 'Not Found'

This PR never actually fixed the issue as a whole, but only fixed one of the causes (fetching prerendered endpoints in the root layout while rendering an error page).