Exception thrown in hydratable promise crashes entire node process

[kylejrp]

Describe the bug

Hey, thanks for launching the new hydratable feature! We're already taking advantage of it here at Stack Overflow.

We've noticed one problem with the current implementation though. If a promise rejects (eg. through a thrown exception), hydratable reasonably fails to serialize the Error object returned by the promise. However, it then throws an exception outside of the promise chain. This causes the entire node process to crash since I think the exception was thrown in a background thread, not the main thread.

cc @elliott-with-the-longest-name-on-github @Rich-Harris @kaysef

I'm happy to open a PR to help fix this, I just need some guidance on what the intent is on these promise chains below.

Source of the problem

I think the problematic bit is in the entry.serialized assignment:

svelte/packages/svelte/src/internal/server/hydratable.js

Lines 58 to 88 in 46603d9

	entry.serialized = devalue.uneval(entry.value, (value, uneval) => {
	if (is_promise(value)) {
	const p = value
	.then((v) => `r(${uneval(v)})`)
	.catch((devalue_error) =>
	e.hydratable_serialization_failed(
	key,
	serialization_stack(entry.stack, devalue_error?.stack)
	)
	);
	// prevent unhandled rejections from crashing the server
	p.catch(() => {});
	// track which promises are still resolving when render is complete
	unresolved?.set(p, key);
	p.finally(() => unresolved?.delete(p));
	// we serialize promises as `"${i}"`, because it's impossible for that string
	// to occur 'naturally' (since the quote marks would have to be escaped)
	const placeholder = `"${uid++}"`;
	(entry.promises ??= []).push(
	p.then((s) => {
	entry.serialized = entry.serialized.replace(placeholder, s);
	})
	);
	return placeholder;
	}
	});

I think that this accidentally introduces three branches of promise chains, only one of which is actually used:

1. The first promise is defined on line 60 with const p = value.then((v) => r(${uneval(v)})).catch((d) => e.hydratable_serialization_failed(...));. The promise we gave it throws an exception, so the uneval never runs and we jump to the catch. However, because catch is returning the error object instead of throwing it, the promise is now an accepted promise of type Promise<Error>. It is not the expected type of a rejected promise.
2. A second promise is created on line 70 with p.catch(() => {});. p is an accepted promise, so the catch handler is never called. Notably, this new promise created by p.catch(...) is not assigned back to p, it's just discarded, so even if it did run, it wouldn't ever transform the promise p.
3. A third promise is created on line 74: p.finally(() => unresolved?.delete(p));. This refers back to the first promise, not the second promise, because p was never reassigned. This third promise is then discarded and not reassigned to p. (It's unclear to me if the finally() block will ever execute, because this third promise is discarded and not assigned to p?)
4. A fourth promise is created on line 81 with p.then((s) => { entry.serialized = entry.serialized.replace(placeholder, s);}). This promise refers again to the first promise. p is an accepted promise containing an Error object, so this new fourth promise is happily (and incorrectly) pushed as a value to entry.promises.
5. The data has now been corrupted in entry.promises - it contains an Promise<Error> object. We probably should have thrown by now and not corrupted entry.promises. Presumably, something later tries to do something with that Error object and crashes in a background thread, taking the whole node process with it.

Diagram of promise chains

value = new Promise(() => { throw new Error("nope")})
p = value
1+4--> p.then(uneval).catch(hydratable_serialization_failed).then(entry.serialized = ...)
2  --> p.catch(() => {})
3  --> p.finally(() => unresolved?.delete(value))

Possible fix

I think if you did throw e.hydratable_serialization_failed(...) inside the first catch(...), it might work? However, the promise chains are still kind of messy - promises 2 and 3 aren't being assigned back to p and are thus being discarded (and possibly never having their extra then(...)/catch(...)/finally(...) executed?)

Reproduction

Here's my minimal repro project. It's a Fastify server that tries to render the Svelte component using a Vite-built SSR file.
https://github.com/kylejrp/vite-ssr-crash

Steps to reproduce:

npm install
npm run dev

Then, open http://localhost:3000/. The first request will show an error object, but then the server crashes in the background. Trying to reload the page will time out because the server never responds.

Failing component

You can reproduce this with the minimal Svelte component:

<script lang="ts">
  import { hydratable } from "svelte";
  const foo = await hydratable("foo", () => new Promise(() => { throw new Error("nope")}));
</script>

{foo}

This crashes the entire node process on the background thread. This exception is not caught by Fastify's global error handler and the entire server becomes unresponsive. Visiting http://localhost:3000/ will respond with an error message on the first request, but will then crash shortly after on the background thread and make the entire server unresponsive.

Working component

If you move the promise from being called by hydratable, it instead correctly bubbles up the exception on the main thread and is handled by Fastify's global error handler. You can make repeated requests to http://localhost:3000/ and Fastify responds with a 500 error each time.

<script lang="ts">
    const foo = await new Promise(() => { throw new Error("nope")});
</script>

{foo}

Logs

{"level":30,"time":1764963489130,"pid":80592,"hostname":"so-win-28552095","msg":"Server listening at http://127.0.0.1:3000"}
[1] {"level":30,"time":1764963489132,"pid":80592,"hostname":"so-win-28552095","msg":"Server listening at http://[::1]:3000"}
[1] {"level":30,"time":1764963493147,"pid":80592,"hostname":"so-win-28552095","reqId":"req-1","req":{"method":"GET","url":"/","host":"localhost:3000","remoteAddress":"::1","remotePort":53918},"msg":"incoming request"}
[1] {"level":50,"time":1764963493157,"pid":80592,"hostname":"so-win-28552095","reqId":"req-1","req":{"method":"GET","url":"/","host":"localhost:3000","remoteAddress":"::1","remotePort":53918},"res":{"statusCode":500},"err":{"type":"Error","message":"nope","stack":"Error: nope\n    at <anonymous> (C:\\code\\vite-ssr\\dist\\server\\entry-server.js:1029:15)\n    at new Promise (<anonymous>)\n    at <anonymous> (C:\\code\\vite-ssr\\dist\\server\\entry-server.js:1028:55)\n    at hydratable (C:\\code\\vite-ssr\\dist\\server\\entry-server.js:980:17)\n    at Array.<anonymous> (C:\\code\\vite-ssr\\dist\\server\\entry-server.js:1028:31)\n    at Renderer.run (C:\\code\\vite-ssr\\dist\\server\\entry-server.js:425:44)\n    at <anonymous> (C:\\code\\vite-ssr\\dist\\server\\entry-server.js:1027:34)\n    at Renderer.child (C:\\code\\vite-ssr\\dist\\server\\entry-server.js:456:20)\n    at Renderer.component (C:\\code\\vite-ssr\\dist\\server\\entry-server.js:477:24)\n    at App (C:\\code\\vite-ssr\\dist\\server\\entry-server.js:1025:14)"},"msg":"nope"}
[1] {"level":30,"time":1764963493163,"pid":80592,"hostname":"so-win-28552095","reqId":"req-1","res":{"statusCode":500},"responseTime":15.511900007724762,"msg":"request completed"}
[1] C:\code\vite-ssr\dist\server\entry-server.js:249
[1]   const error = new Error(`hydratable_serialization_failed
[1]                 ^
[1]
[1]
[1] Svelte error: hydratable_serialization_failed
[1] Failed to serialize `hydratable` data for key `foo`.
[1]
[1] `hydratable` can serialize anything [`uneval` from `devalue`](https://npmjs.com/package/uneval) can, plus Promises.
[1]
[1] Cause:
[1] Caused by:
[1] Error: nope
[1]     at <anonymous> (C:\code\vite-ssr\dist\server\entry-server.js:1029:15)
[1]     at new Promise (<anonymous>)
[1]     at <anonymous> (C:\code\vite-ssr\dist\server\entry-server.js:1028:55)
[1]     at hydratable (C:\code\vite-ssr\dist\server\entry-server.js:980:17)
[1]     at Array.<anonymous> (C:\code\vite-ssr\dist\server\entry-server.js:1028:31)
[1]     at Renderer.run (C:\code\vite-ssr\dist\server\entry-server.js:425:44)
[1]     at <anonymous> (C:\code\vite-ssr\dist\server\entry-server.js:1027:34)
[1]     at Renderer.child (C:\code\vite-ssr\dist\server\entry-server.js:456:20)
[1]     at Renderer.component (C:\code\vite-ssr\dist\server\entry-server.js:477:24)
[1]     at App (C:\code\vite-ssr\dist\server\entry-server.js:1025:14)
[1]
[1] https://svelte.dev/e/hydratable_serialization_failed
[1]     at hydratable_serialization_failed (C:\code\vite-ssr\dist\server\entry-server.js:249:17)      
[1]     at <anonymous> (C:\code\vite-ssr\dist\server\entry-server.js:991:28)
[1]
[1] Node.js v20.19.5

System Info

System:
    OS: Windows 11 10.0.26200
    CPU: (20) x64 13th Gen Intel(R) Core(TM) i9-13900H
    Memory: 28.90 GB / 63.68 GB
  Binaries:
    Node: 20.19.5 - C:\Program Files\nodejs\node.EXE
    npm: 10.9.3 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Chromium (141.0.3537.71)
    Firefox Developer Edition: 146.0 - C:\Program Files\Firefox Developer Edition\firefox.exe
  npmPackages:
    svelte: ^5.45.5 => 5.45.5

Severity

blocking an upgrade

[elliott-with-the-longest-name-on-github]

Thank you for the thorough report! I'll dig into this today -- I had discussed some vague fears of this with Rich during the development process but we thought we'd taken care of the edge cases. Clearly missed one 😅

[elliott-with-the-longest-name-on-github]

Just blogging my exploration as I go through, based on what you've written:

1. The first promise is defined on line 60 with const p = value.then((v) => r(${uneval(v)})).catch((d) => e.hydratable_serialization_failed(...));. The promise we gave it throws an exception, so the uneval never runs and we jump to the catch. However, because catch is returning the error object instead of throwing it, the promise is now an accepted promise of type Promise. It is not the expected type of a rejected promise.

I don't think this is accurate. e.hydratable_serialization_failed does not return a value -- it throws an error -- so the callback passed to .catch is actually just throwing an error, which should reject the promise.

I tested this in the browser console:

(() => {
    function throw_error() { throw new Error('gotcha') }

    const p = Promise.resolve().then(() => { throw new Error('this is like if serialization failed') }).catch(throw_error);
    p.catch(() => {}); // prevents "unhandled promise rejection" on the entire chain
})()

...if you comment out the p.catch bit, you'll see that we get an unhandled promise rejection error.

Basically, the way promises are designed, if you have a promise that both 1) rejects and 2) does not have a catch handler, the whole process will crash. The empty .catch handler we attach doesn't actually change the status of the promise we're attaching it to (it's still rejected), but it does prevent the freak-out JavaScript likes to have when there's an "unhandled" promise rejection.

Maybe to make it clearer:

• p is a Promise<string> that may reject
• p.catch creates a new promise that we do nothing with -- but by attaching the .catch handler to p we prevent the unhandled rejection
• If p rejects, await p will still throw, and p.catch (added later) would still run

2. A second promise is created on line 70 with p.catch(() => {});. p is an accepted promise, so the catch handler is never called. Notably, this new promise created by p.catch(...) is not assigned back to p, it's just discarded, so even if it did run, it wouldn't ever transform the promise p.

Yeah, expected -- as above, we're not transforming p, we're just attaching a .catch handler to prevent it from crashing the process if it rejects.

3. A third promise is created on line 74: p.finally(() => unresolved?.delete(p));. This refers back to the first promise, not the second promise, because p was never reassigned. This third promise is then discarded and not reassigned to p. (It's unclear to me if the finally() block will ever execute, because this third promise is discarded and not assigned to p?)

Same thing as p.catch -- we're not transforming p, we're attaching a side effect to it. (When p finishes, regardless of whether it rejected or passed, remove it from the unresolved set.) Which is the correct behavior.

4. A fourth promise is created on line 81 with p.then((s) => { entry.serialized = entry.serialized.replace(placeholder, s);}). This promise refers again to the first promise. p is an accepted promise containing an Error object, so this new fourth promise is happily (and incorrectly) pushed as a value to entry.promises.

This still isn't true -- p is either unresolved, resolved with string, or rejected. This is just attaching work to the end of it that says "hey, update the string we're tracking all of this in"

5. The data has now been corrupted in entry.promises - it contains an Promise object. We probably should have thrown by now and not corrupted entry.promises. Presumably, something later tries to do something with that Error object and crashes in a background thread, taking the whole node process with it.

Okay so this is all not actually what's happening -- the situation is really quite simple. We have p, which is a Promise<string> that might reject. We also have entry.promises, which is the accumulated work required to serialize this hydratable. It's full of Promise<void>s (this is what happens when you do p.then(/* function returning void */)).

And here's where I think the problem is. While we're attaching a .catch handler to p to prevent unhandled rejections, we're not attaching one when we do the followup "replace the string" work:

			(entry.promises ??= []).push(
				p.then((s) => {
					entry.serialized = entry.serialized.replace(placeholder, s);
-				})
+				}).catch(() => {})
			);

This is creating a new promise (based on p) -- so if p rejects, so does p.then, but p.then doesn't have a handler, so it's considered an unhandled rejection.

AFAICT, this is correct -- combined with catching render errors, this prevents crashing the process when something fails to serialize. I'll open a PR and you can let me know if it fixes your issue!

[elliott-with-the-longest-name-on-github]

For the curious, I actually reorganized everything a bit to reduce the number of promises created / make it easier to attach catch handlers:

	entry.serialized = devalue.uneval(entry.value, (value, uneval) => {
		if (is_promise(value)) {
			// we serialize promises as `"${i}"`, because it's impossible for that string
			// to occur 'naturally' (since the quote marks would have to be escaped)
			// this placeholder is returned synchronously from `uneval`, which includes it in the
			// serialized string. Later (at least one microtask from now), when `p.then` runs, it'll
			// be replaced.
			const placeholder = `"${uid++}"`;
			const p = value
				.then((v) => {
					entry.serialized = entry.serialized.replace(placeholder, `r(${uneval(v)})`);
				})
				.catch((devalue_error) =>
					e.hydratable_serialization_failed(
						key,
						serialization_stack(entry.stack, devalue_error?.stack)
					)
				);

			unresolved?.set(p, key);
			// prevent unhandled rejections from crashing the server, track which promises are still resolving when render is complete
			p.catch(() => {}).finally(() => unresolved?.delete(p));

			(entry.promises ??= []).push(p);
			return placeholder;
		}
	});

[kylejrp]

I don't think this is accurate. e.hydratable_serialization_failed does not return a value -- it throws an error -- so the callback passed to .catch is actually just throwing an error, which should reject the promise.

Ahh, I misread internal/server/errors.js, sorry. I thought it returned an Error, not threw an Error. That makes a lot of my explanation nonsensical, apologies! I'll continue to respond to your thoughts below, but it's all based on my faulty assumption.

Yeah, expected -- as above, we're not transforming p, we're just attaching a .catch handler to prevent it from crashing the process if it rejects.

Yeah, it's clear now that this is scheduling a function to be called if/when the promise is rejected. It's unclear to me how this provides any benefit though because it's not modifying the original promise chain? It's not useful if a separate background thread catches the exception if a different promise chain chains off the original promise that doesn't have the thrown exception caught. The returned {} isn't used anywhere...

Same thing as p.catch -- we're not transforming p, we're attaching a side effect to it. (When p finishes, regardless of whether it rejected or passed, remove it from the unresolved set.) Which is the correct behavior.

Yeah, I'm wrong. finally will always schedule a background task to remove it from the unresolved set. That behavior is correct, finally always executes.

This still isn't true -- p is either unresolved, resolved with string, or rejected. This is just attaching work to the end of it that says "hey, update the string we're tracking all of this in"

Yep, you're right.

Okay so this is all not actually what's happening -- the situation is really quite simple. We have p, which is a Promise that might reject. We also have entry.promises, which is the accumulated work required to serialize this hydratable. It's full of Promises (this is what happens when you do p.then(/* function returning void */)).
And here's where I think the problem is. While we're attaching a .catch handler to p to prevent unhandled rejections, we're not attaching one when we do the followup "replace the string" work:

Wow yeah, I was way off the mark on my analysis. You're right.

I think the confusion for me was that the line p.catch(() => {}) is essentially a no-op before. When you're building the promise here, you're building it off the original p.then(...).catch(throw), which didn't have that extra p.catch(() => {}). If it had been reassigned like p = p.catch(() => {}), then it would have prevented the error here. But like you said, you don't want to transform p, just attach handlers to it.

For the curious, I actually reorganized everything a bit to reduce the number of promises created / make it easier to attach catch handlers:

This looks nicer, thank you for the quick fix! I'll check out the PR now to see if it fixes the problem.

[elliott-with-the-longest-name-on-github]

Yeah, it's clear now that this is scheduling a function to be called if/when the promise is rejected. It's unclear to me how this provides any benefit though because it's not modifying the original promise chain? It's not useful if a separate background thread catches the exception if a different promise chain chains off the original promise that doesn't have the thrown exception caught. The returned {} isn't used anywhere...

There's this really crazy, weird property of promises where if they reject and don't have a catch handler attached to them, they crash the process in the background with an "unhandled promise rejection". Just attaching a .catch handler to them prevents the rejection from being "unhandled" and therefore prevents the out-of-bounds unhandled promise rejection from occurring. It's pretty weird to play around with:

(async () => {
    function throw_error() { throw new Error('gotcha') }

    const p = Promise.resolve().then(() => { throw new Error('this is like if serialization failed') });
    p.catch(() => {}); // prevents "unhandled promise rejection" on the entire chain

    // p is now safe to defer -- you can save it in an array or something, and `await` it later. If it rejected, it'll throw when `await`ed
    // however, if you were to uncomment the "useless" `.catch` above, this whole function  
    // would crash with an error to the console about 
    // "unhandled promise rejection", even if `p` is awaited and handled later
})()

p is now safe to defer -- you can save it in an array or something, and await it later. If it rejected, the await will throw with the error, just like you'd expect. However, if you comment out the "useless" .catch, the whole process will crash with "unhandled promise rejection".

Basically what actually happens:

• Promise.resolve() is created
• .then callback is attached to the promise (but crucially, not run yet)
• The resulting promise is assigned to p
• p.catch runs, which attaches a catch handler to p
• function scope exits, microtasks are picked up, which causes .then to run
• .then throws
  • If .catch hadn't been attached, this rejection would be considered "unhandled", and would crash in the background
• .catch, which is attached to p, runs. Because it runs, this rejection is not "unhandled"
  • However, because we didn't assign p.catch back to p, p is still the original (rejected) promise, which means we can still use it later to access its error value.

So... yeah. Promises are weird and if you're not careful they'll implode your entire process in the background.