Repository: sveltejs/kit (js)

Issue:
Differing behavior of exception in shadow endpoint vs ssr
### Describe the bug

In hooks.js I wrap the `await resolve(event)` in a try/catch in order to do things like rollback a DB transaction.

This works fine when the exception is thrown from the shadow endpoint but the exception fails to bubble up to `handle()` when the same page is rendered on the server.



### Reproduction


```js
// hooks.js

export const handle = async ({ event, resolve }) => {
  let response
  try {
    response = await resolve(event);
  } catch (err) {
    console.log('@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@')
    console.log(err)
    throw err
  }
}
```

```js
// routes/endpoint.js

export const get = async () => {
  throw new Error("I should see some @'s")
}
```

### Logs

*From a full page load (SSR)*

```shell
I should see some @'s!
Error: I should see some @'s!
    at get (/private/tmp/my-app/src/routes/todos/index.ts:2:10)
    at load_shadow_data (file:///private/tmp/my-app/.svelte-kit/runtime/server/index.js:2530:25)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async load_node (file:///private/tmp/my-app/.svelte-kit/runtime/server/index.js:2127:5)
    at async respond$1 (file:///private/tmp/my-app/.svelte-kit/runtime/server/index.js:2804:15)
    at async resolve (file:///private/tmp/my-app/.svelte-kit/runtime/server/index.js:3309:11)
    at async Object.handle (/private/tmp/my-app/src/hooks.ts:5:13)
    at async respond (file:///private/tmp/my-app/.svelte-kit/runtime/server/index.js:3253:20)
    at async file:///private/tmp/my-app/node_modules/@sveltejs/kit/dist/chunks/index.js:294:24
```

*From a shadow endpoint fetch (`__data.json`)*

```
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
Error: I should see some @'s!
    at get (/private/tmp/my-app/src/routes/todos/index.ts:4:9)
    at render_endpoint (file:///private/tmp/my-app/.svelte-kit/runtime/server/index.js:171:25)
    at resolve (file:///private/tmp/my-app/.svelte-kit/runtime/server/index.js:3286:24)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Object.handle (//private/tmp/my-app/src/hooks.ts:6:16)
    at async respond (file:///private/tmp/my-app/.svelte-kit/runtime/server/index.js:3253:20)
    at async file:///private/tmp/my-app/node_modules/@sveltejs/kit/dist/chunks/index.js:294:24
```


### System Info

```shell
System:
    OS: macOS 12.2.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 240.88 MB / 32.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 17.6.0 - /opt/homebrew/bin/node
    Yarn: 1.22.18 - ~/.yarn/bin/yarn
    npm: 8.5.2 - /opt/homebrew/bin/npm
  Browsers:
    Chrome: 100.0.4896.127
    Safari: 15.3
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.40
    @sveltejs/kit: next => 1.0.0-next.324
    svelte: ^3.46.0 => 3.48.0
```


### Severity

serious, but I can work around it

### Additional Information

I'm not 100% sure how to work around it...

Pull request description:
[breaking] render raw response when unexpected error occurs in endpoint
..instead of failing completely. Also catch possible fetch error (which would occur when network fails).
Closes #4801
Related to #1931 (maybe even closing it? Can't tell from reading the issue)

There are still some code paths which would bubble up to the handler, but these are of the kind "you use a wrong SvelteKit option" or during prerendering ("cannot use mutative methods .."). The other all look like they have a try-catch around code paths where errors could occur.

Not sure if a test is needed here. We could theoretically do one for the endpoint error.

This is a breaking change insofar as people could have relied on this behavior somehow.

### Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
- [x] 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
- [x] This message body should clearly illustrate what problems it solves.
- [ ] Ideally, include a test that fails without this PR but passes with it.

### Tests
- [x] Run the tests with `pnpm test` and lint the project with `pnpm lint` and `pnpm check`

### Changesets
- [x] 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. All changesets should be `patch` until SvelteKit 1.0


Patch:
diff --git a/.changeset/few-ghosts-attend.md b/.changeset/few-ghosts-attend.md
new file mode 100644
index 000000000..3c51929f6
--- /dev/null
+++ b/.changeset/few-ghosts-attend.md
@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+[breaking] catch and render raw response when unexpected error occurs in endpoint
diff --git a/.changeset/pre.json b/.changeset/pre.json
index d96656044..0e1d5632c 100644
--- a/.changeset/pre.json
+++ b/.changeset/pre.json
@@ -681,6 +681,7 @@
     "odd-ligers-march",
     "odd-ligers-swim",
     "odd-pumas-grow",
+    "odd-sloths-sit",
     "odd-tools-add",
     "odd-trees-visit",
     "old-beers-push",
diff --git a/packages/kit/CHANGELOG.md b/packages/kit/CHANGELOG.md
index 1c07dfe05..875c20d47 100644
--- a/packages/kit/CHANGELOG.md
+++ b/packages/kit/CHANGELOG.md
@@ -1,5 +1,11 @@
 # @sveltejs/kit
 
+## 1.0.0-next.457
+
+### Patch Changes
+
+- Print error if resolveOpts.ssr is set ([#6475](https://github.com/sveltejs/kit/pull/6475))
+
 ## 1.0.0-next.456
 
 ### Patch Changes
diff --git a/packages/kit/package.json b/packages/kit/package.json
index 7dcb69a5f..34520f71f 100644
--- a/packages/kit/package.json
+++ b/packages/kit/package.json
@@ -1,6 +1,6 @@
 {
 	"name": "@sveltejs/kit",
-	"version": "1.0.0-next.456",
+	"version": "1.0.0-next.457",
 	"repository": {
 		"type": "git",
 		"url": "https://github.com/sveltejs/kit",
diff --git a/packages/kit/src/runtime/server/endpoint.js b/packages/kit/src/runtime/server/endpoint.js
index 9585e027e..6eebd7c71 100644
--- a/packages/kit/src/runtime/server/endpoint.js
+++ b/packages/kit/src/runtime/server/endpoint.js
@@ -1,4 +1,4 @@
-import { HttpError, Redirect } from '../control.js';
+import { Redirect } from '../control.js';
 import { check_method_names, method_not_allowed } from './utils.js';
 
 /**
@@ -39,9 +39,8 @@ export async function render_endpoint(event, mod, state) {
 		);
 
 		if (!(response instanceof Response)) {
-			return new Response(
-				`Invalid response from route ${event.url.pathname}: handler should return a Response object`,
-				{ status: 500 }
+			throw new Error(
+				`Invalid response from route ${event.url.pathname}: handler should return a Response object`
 			);
 		}
 
@@ -52,15 +51,13 @@ export async function render_endpoint(event, mod, state) {
 
 		return response;
 	} catch (error) {
-		if (error instanceof HttpError) {
-			return new Response(error.message, { status: error.status });
-		} else if (error instanceof Redirect) {
+		if (error instanceof Redirect) {
 			return new Response(undefined, {
 				status: error.status,
-				headers: { Location: error.location }
+				headers: { location: error.location }
 			});
-		} else {
-			throw error;
 		}
+
+		throw error;
 	}
 }
diff --git a/packages/kit/src/runtime/server/index.js b/packages/kit/src/runtime/server/index.js
index e1f03785b..413903640 100644
--- a/packages/kit/src/runtime/server/index.js
+++ b/packages/kit/src/runtime/server/index.js
@@ -3,10 +3,9 @@ import { render_page } from './page/index.js';
 import { render_response } from './page/render.js';
 import { respond_with_error } from './page/respond_with_error.js';
 import { coalesce_to_error } from '../../utils/error.js';
-import { serialize_error, GENERIC_ERROR, static_error_page } from './utils.js';
+import { GENERIC_ERROR, handle_fatal_error } from './utils.js';
 import { decode_params, disable_search, normalize_path } from '../../utils/url.js';
 import { exec } from '../../utils/routing.js';
-import { negotiate } from '../../utils/http.js';
 import { render_data } from './data/index.js';
 import { DATA_SUFFIX } from '../../constants.js';
 
@@ -190,175 +189,151 @@ export async function respond(request, options, state) {
 		transformPageChunk: default_transform
 	};
 
-	try {
-		const response = await options.hooks.handle({
-			event,
-			resolve: async (event, opts) => {
-				if (opts) {
-					// TODO remove for 1.0
-					if ('transformPage' in opts) {
-						throw new Error(
-							'transformPage has been replaced by transformPageChunk — see https://github.com/sveltejs/kit/pull/5657 for more information'
-						);
-					}
-
-					if ('ssr' in opts) {
-						throw new Error(
-							'ssr has been removed, set it in the appropriate +layout.js instead. See the PR for more information: https://github.com/sveltejs/kit/pull/6197'
-						);
-					}
-
-					resolve_opts = {
-						transformPageChunk: opts.transformPageChunk || default_transform
-					};
+	/**
+	 *
+	 * @param {import('types').RequestEvent} event
+	 * @param {import('types').ResolveOptions} [opts]
+	 */
+	async function resolve(event, opts) {
+		try {
+			if (opts) {
+				// TODO remove for 1.0
+				if ('transformPage' in opts) {
+					throw new Error(
+						'transformPage has been replaced by transformPageChunk — see https://github.com/sveltejs/kit/pull/5657 for more information'
+					);
 				}
 
-				if (state.prerendering?.fallback) {
-					return await render_response({
-						event,
-						options,
-						state,
-						page_config: { ssr: false, csr: true },
-						status: 200,
-						error: null,
-						branch: [],
-						fetched: [],
-						validation_errors: undefined,
-						cookies: [],
-						resolve_opts
-					});
+				if ('ssr' in opts) {
+					throw new Error(
+						'ssr has been removed, set it in the appropriate +layout.js instead. See the PR for more information: https://github.com/sveltejs/kit/pull/6197'
+					);
 				}
 
-				if (route) {
-					/** @type {Response} */
-					let response;
-
-					if (is_data_request) {
-						response = await render_data(event, route, options, state);
-					} else if (route.page) {
-						response = await render_page(event, route, route.page, options, state, resolve_opts);
-					} else if (route.endpoint) {
-						response = await render_endpoint(event, await route.endpoint(), state);
-					} else {
-						// a route will always have a page or an endpoint, but TypeScript
-						// doesn't know that
-						throw new Error('This should never happen');
-					}
+				resolve_opts = {
+					transformPageChunk: opts.transformPageChunk || default_transform
+				};
+			}
 
-					if (!is_data_request) {
-						// we only want to set cookies on __data.js requests, we don't
-						// want to cache stuff erroneously etc
-						for (const key in headers) {
-							const value = headers[key];
-							response.headers.set(key, /** @type {string} */ (value));
-						}
-					}
+			if (state.prerendering?.fallback) {
+				return await render_response({
+					event,
+					options,
+					state,
+					page_config: { ssr: false, csr: true },
+					status: 200,
+					error: null,
+					branch: [],
+					fetched: [],
+					validation_errors: undefined,
+					cookies: [],
+					resolve_opts
+				});
+			}
+
+			if (route) {
+				/** @type {Response} */
+				let response;
 
-					for (const cookie of cookies) {
-						response.headers.append('set-cookie', cookie);
+				if (is_data_request) {
+					response = await render_data(event, route, options, state);
+				} else if (route.page) {
+					response = await render_page(event, route, route.page, options, state, resolve_opts);
+				} else if (route.endpoint) {
+					response = await render_endpoint(event, await route.endpoint(), state);
+				} else {
+					// a route will always have a page or an endpoint, but TypeScript
+					// doesn't know that
+					throw new Error('This should never happen');
+				}
+
+				if (!is_data_request) {
+					// we only want to set cookies on __data.js requests, we don't
+					// want to cache stuff erroneously etc
+					for (const key in headers) {
+						const value = headers[key];
+						response.headers.set(key, /** @type {string} */ (value));
 					}
+				}
 
-					// respond with 304 if etag matches
-					if (response.status === 200 && response.headers.has('etag')) {
-						let if_none_match_value = request.headers.get('if-none-match');
+				for (const cookie of cookies) {
+					response.headers.append('set-cookie', cookie);
+				}
 
-						// ignore W/ prefix https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-None-Match#directives
-						if (if_none_match_value?.startsWith('W/"')) {
-							if_none_match_value = if_none_match_value.substring(2);
-						}
+				// respond with 304 if etag matches
+				if (response.status === 200 && response.headers.has('etag')) {
+					let if_none_match_value = request.headers.get('if-none-match');
 
-						const etag = /** @type {string} */ (response.headers.get('etag'));
+					// ignore W/ prefix https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-None-Match#directives
+					if (if_none_match_value?.startsWith('W/"')) {
+						if_none_match_value = if_none_match_value.substring(2);
+					}
 
-						if (if_none_match_value === etag) {
-							const headers = new Headers({ etag });
+					const etag = /** @type {string} */ (response.headers.get('etag'));
 
-							// https://datatracker.ietf.org/doc/html/rfc7232#section-4.1
-							for (const key of ['cache-control', 'content-location', 'date', 'expires', 'vary']) {
-								const value = response.headers.get(key);
-								if (value) headers.set(key, value);
-							}
+					if (if_none_match_value === etag) {
+						const headers = new Headers({ etag });
 
-							return new Response(undefined, {
-								status: 304,
-								headers
-							});
+						// https://datatracker.ietf.org/doc/html/rfc7232#section-4.1
+						for (const key of ['cache-control', 'content-location', 'date', 'expires', 'vary']) {
+							const value = response.headers.get(key);
+							if (value) headers.set(key, value);
 						}
-					}
 
-					return response;
+						return new Response(undefined, {
+							status: 304,
+							headers
+						});
+					}
 				}
 
-				if (state.initiator === GENERIC_ERROR) {
-					return new Response('Internal Server Error', {
-						status: 500
-					});
-				}
+				return response;
+			}
 
-				// if this request came direct from the user, rather than
-				// via a `fetch` in a `load`, render a 404 page
-				if (!state.initiator) {
-					return await respond_with_error({
-						event,
-						options,
-						state,
-						status: 404,
-						error: new Error(`Not found: ${event.url.pathname}`),
-						resolve_opts
-					});
-				}
+			if (state.initiator === GENERIC_ERROR) {
+				return new Response('Internal Server Error', {
+					status: 500
+				});
+			}
 
-				if (state.prerendering) {
-					return new Response('not found', { status: 404 });
-				}
+			// if this request came direct from the user, rather than
+			// via a `fetch` in a `load`, render a 404 page
+			if (!state.initiator) {
+				return await respond_with_error({
+					event,
+					options,
+					state,
+					status: 404,
+					error: new Error(`Not found: ${event.url.pathname}`),
+					resolve_opts
+				});
+			}
 
-				// we can't load the endpoint from our own manifest,
-				// so we need to make an actual HTTP request
-				return await fetch(request);
-			},
+			if (state.prerendering) {
+				return new Response('not found', { status: 404 });
+			}
 
+			// we can't load the endpoint from our own manifest,
+			// so we need to make an actual HTTP request
+			return await fetch(request);
+		} catch (e) {
+			const error = coalesce_to_error(e);
+			return handle_fatal_error(event, options, error);
+		}
+	}
+
+	try {
+		return await options.hooks.handle({
+			event,
+			resolve,
 			// TODO remove for 1.0
 			// @ts-expect-error
 			get request() {
 				throw new Error('request in handle has been replaced with event' + details);
 			}
 		});
-
-		// TODO for 1.0, change the error message to point to docs rather than PR
-		if (response && !(response instanceof Response)) {
-			throw new Error('handle must return a Response object' + details);
-		}
-
-		return response;
 	} catch (/** @type {unknown} */ e) {
 		const error = coalesce_to_error(e);
-
-		options.handle_error(error, event);
-
-		const type = negotiate(event.request.headers.get('accept') || 'text/html', [
-			'text/html',
-			'application/json'
-		]);
-
-		if (is_data_request || type === 'application/json') {
-			return new Response(serialize_error(error, options.get_stack), {
-				status: 500,
-				headers: { 'content-type': 'application/json; charset=utf-8' }
-			});
-		}
-
-		// TODO is this necessary? should we just return a plain 500 at this point?
-		try {
-			return await respond_with_error({
-				event,
-				options,
-				state,
-				status: 500,
-				error,
-				resolve_opts
-			});
-		} catch (/** @type {unknown} */ e) {
-			const error = coalesce_to_error(e);
-			return static_error_page(options, 500, error.message);
-		}
+		return handle_fatal_error(event, options, error);
 	}
 }
diff --git a/packages/kit/src/runtime/server/page/index.js b/packages/kit/src/runtime/server/page/index.js
index 1d05961f9..c704cdc83 100644
--- a/packages/kit/src/runtime/server/page/index.js
+++ b/packages/kit/src/runtime/server/page/index.js
@@ -334,7 +334,7 @@ export async function render_page(event, route, page, options, state, resolve_op
 		});
 	} catch (error) {
 		// if we end up here, it means the data loaded successfull
-		// but the page failed to render
+		// but the page failed to render, or that a prerendering error occurred
 		options.handle_error(/** @type {Error} */ (error), event);
 
 		return await respond_with_error({
diff --git a/packages/kit/src/runtime/server/utils.js b/packages/kit/src/runtime/server/utils.js
index 26c173ad8..d4659cdbe 100644
--- a/packages/kit/src/runtime/server/utils.js
+++ b/packages/kit/src/runtime/server/utils.js
@@ -1,4 +1,6 @@
 import { devalue } from 'devalue';
+import { DATA_SUFFIX } from '../../constants.js';
+import { negotiate } from '../../utils/http.js';
 import { HttpError } from '../control.js';
 
 /** @param {any} body */
@@ -175,3 +177,33 @@ export function static_error_page(options, status, message) {
 		status
 	});
 }
+
+/**
+ * @param {import('types').RequestEvent} event
+ * @param {import('types').SSROptions} options
+ * @param {Error} error
+ */
+export function handle_fatal_error(event, options, error) {
+	let status = 500;
+
+	if (error instanceof HttpError) {
+		status = error.status;
+	} else {
+		options.handle_error(error, event);
+	}
+
+	// ideally we'd use sec-fetch-dest instead, but Safari — quelle surprise — doesn't support it
+	const type = negotiate(event.request.headers.get('accept') || 'text/html', [
+		'text/html',
+		'application/json'
+	]);
+
+	if (event.url.pathname.endsWith(DATA_SUFFIX) || type === 'application/json') {
+		return new Response(serialize_error(error, options.get_stack), {
+			status,
+			headers: { 'content-type': 'application/json; charset=utf-8' }
+		});
+	}
+
+	return static_error_page(options, status, error.message);
+}


Test patch:
diff --git a/packages/kit/test/apps/basics/test/server.test.js b/packages/kit/test/apps/basics/test/server.test.js
index 1cafac3ba..c409a799e 100644
--- a/packages/kit/test/apps/basics/test/server.test.js
+++ b/packages/kit/test/apps/basics/test/server.test.js
@@ -148,17 +148,19 @@ test.describe('Errors', () => {
 		);
 	});
 
-	test('throw error(..) in endpoint', async ({ page, read_errors }) => {
-		const res = await page.goto('/errors/endpoint-throw-error');
+	test('throw error(...) in endpoint', async ({ request, read_errors }) => {
+		const res = await request.get('/errors/endpoint-throw-error');
 
 		const error = read_errors('/errors/endpoint-throw-error');
 		expect(error).toBe(undefined);
 
-		expect(await res?.text()).toBe('You shall not pass');
+		expect(await res?.text()).toContain(
+			'This is the static error page with the following message: You shall not pass'
+		);
 		expect(res?.status()).toBe(401);
 	});
 
-	test('throw redirect(..) in endpoint', async ({ page, read_errors }) => {
+	test('throw redirect(...) in endpoint', async ({ page, read_errors }) => {
 		const res = await page.goto('/errors/endpoint-throw-redirect');
 		expect(res?.status()).toBe(200); // redirects are opaque to the browser
 
@@ -167,6 +169,15 @@ test.describe('Errors', () => {
 
 		expect(await page.textContent('h1')).toBe('the answer is 42');
 	});
+
+	test('error thrown in handle results in a rendered error page', async ({ request }) => {
+		const res = await request.get('/errors/error-in-handle');
+
+		expect(await res.text()).toContain(
+			'This is the static error page with the following message: Error in handle'
+		);
+		expect(res.status()).toBe(500);
+	});
 });
 
 test.describe('Load', () => {
diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js
index 6a9bc4cf0..be319a1f9 100644
--- a/packages/kit/test/apps/basics/test/test.js
+++ b/packages/kit/test/apps/basics/test/test.js
@@ -537,16 +537,6 @@ test.describe('Errors', () => {
 		);
 	});
 
-	test('error thrown in handle results in a rendered error page', async ({ page }) => {
-		await page.goto('/errors/error-in-handle');
-
-		expect(await page.textContent('footer')).toBe('Custom layout');
-		expect(await page.textContent('#message')).toBe(
-			'This is your custom error page saying: "Error in handle"'
-		);
-		expect(await page.innerHTML('h1')).toBe('500');
-	});
-
 	test('prerendering a page with a mutative page endpoint results in a catchable error', async ({
 		page
 	}) => {