fix: run `onDestroy` callbacks when a server render throws (#18585)

Fixes #18584.

There's multiple parts to this
- abort signal was buggy. It wasn't scoped per render, so cross-talk was possible. Fix by scoping to renderer
- onDestroy callbacks were skipped when something throws. Fix by carefully aborting the rest of the tree, waiting for settle, collect all callbacks and then call them

---------

Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com>

.changeset/great-hoops-tickle.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: run `onDestroy` callbacks when a server render throws

packages/svelte/src/internal/server/abort-signal.js

@@ -1,13 +1,12 @@
-import { STALE_REACTION } from '#client/constants';
+import { ssr_context } from './context.js';
 
-/** @type {AbortController | null} */
+export function getAbortSignal() {
-let controller = null;
+	let context = ssr_context;
 
-export function abort() {
+	while (context !== null) {
-	controller?.abort(STALE_REACTION);
+		if (context.r !== null) return context.r.global.get_abort_signal();
-	controller = null;
+		context = context.p;
 	}
 
-export function getAbortSignal() {
+	return new AbortController().signal;
-	return (controller ??= new AbortController()).signal;
 }

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

@@ -3,7 +3,7 @@
 /** @import { Csp, RenderOutput, SyncRenderOutput, Sha256Source } from '../../server/public.js' */
 /** @import { MaybePromise } from '#shared' */
 import { async_mode_flag } from '../flags/index.js';
-import { abort } from './abort-signal.js';
+import { STALE_REACTION } from '../client/constants.js';
 import { pop, push, set_ssr_context, ssr_context } from './context.js';
 import * as e from './errors.js';
 import * as w from './warnings.js';
@@ -180,7 +180,7 @@ export class Renderer {
 		// prevent unhandled rejections, and attach the promise to the renderer instance
 		// so that rejections correctly cause rendering to fail
 		promise.catch(noop);
-		this.promise = promise;
+		this.promise = this.global.track(promise);
 
 		return promises;
 	}
@@ -225,7 +225,7 @@ export class Renderer {
 				e.await_invalid();
 			}
 
-			child.promise = result;
+			child.promise = child.global.track(result);
 		}
 
 		return child;
@@ -273,7 +273,7 @@ export class Renderer {
 					e.await_invalid();
 				}
 				result.catch(noop);
-				child.promise = result;
+				child.promise = child.global.track(result);
 			}
 		} catch (error) {
 			// synchronous errors are handled here, async errors will be handled in #collect_content_async
@@ -293,12 +293,14 @@ export class Renderer {
 					e.await_invalid();
 				}
 
-				child.promise = /** @type {Promise<unknown>} */ (result).then((transformed) => {
+				child.promise = child.global.track(
+					/** @type {Promise<unknown>} */ (result).then((transformed) => {
 						set_ssr_context(parent_context);
 						child.#out.push(Renderer.#serialize_failed_boundary(transformed));
 						failed_snippet(child, transformed, noop);
 						child.#out.push(BLOCK_CLOSE);
-				});
+					})
+				);
 				child.promise.catch(noop);
 			} else {
 				child.#out.push(Renderer.#serialize_failed_boundary(result));
@@ -317,8 +319,11 @@ export class Renderer {
 	 */
 	component(fn, component_fn) {
 		push(component_fn);
-		const child = this.child(fn);
+		// mark before running so `onDestroy` callbacks are still collected if `fn` throws
-		child.#is_component_body = true;
+		this.child((renderer) => {
+			renderer.#is_component_body = true;
+			return fn(renderer);
+		});
 		pop();
 	}
 
@@ -626,6 +631,49 @@ export class Renderer {
 		}
 	}
 
+	/**
+	 * Runs every `onDestroy` callback in this renderer tree. On a failed render,
+	 * cleanup errors are suppressed so they do not mask the render error.
+	 * @param {boolean} suppress_errors
+	 */
+	#run_on_destroy(suppress_errors) {
+		let first_error;
+		let has_error = false;
+
+		for (const cleanup of this.#collect_on_destroy()) {
+			try {
+				cleanup();
+			} catch (error) {
+				if (!suppress_errors && !has_error) {
+					first_error = error;
+					has_error = true;
+				}
+			}
+		}
+
+		if (has_error) throw first_error;
+	}
+
+	/**
+	 * @param {'sync' | 'async'} mode
+	 * @param {{ idPrefix?: string; csp?: Csp; transformError?: (error: unknown) => unknown }} options
+	 * @returns {Renderer}
+	 */
+	static #create(mode, options) {
+		if (options.idPrefix?.includes('--')) {
+			e.invalid_id_prefix();
+		}
+
+		return new Renderer(
+			new SSRState(
+				mode,
+				options.idPrefix ? options.idPrefix + '-' : '',
+				options.csp,
+				options.transformError
+			)
+		);
+	}
+
 	/**
 	 * Render a component. Throws if any of the children are performing asynchronous work.
 	 *
@@ -636,13 +684,27 @@ export class Renderer {
 	 */
 	static #render(component, options) {
 		var previous_context = ssr_context;
+		const renderer = Renderer.#create('sync', options);
+		/** @type {AccumulatedContent | undefined} */
+		let result;
+		let render_error;
+		let failed = false;
+
+		try {
 			try {
-			const renderer = Renderer.#open_render('sync', component, options);
+				Renderer.#open_render(renderer, component, options);
+				result = Renderer.#close_render(renderer.#collect_content(), renderer);
+			} catch (error) {
+				render_error = error;
+				failed = true;
+			}
+
+			renderer.#run_on_destroy(failed);
+			if (failed) throw render_error;
 
-			const content = renderer.#collect_content();
+			return /** @type {AccumulatedContent} */ (result);
-			return Renderer.#close_render(content, renderer);
 		} finally {
-			abort();
+			renderer.global.abort();
 			set_ssr_context(previous_context);
 		}
 	}
@@ -657,18 +719,35 @@ export class Renderer {
 	 */
 	static async #render_async(component, options) {
 		const previous_context = ssr_context;
+		const renderer = Renderer.#create('async', options);
+		/** @type {(AccumulatedContent & { hashes: { script: Sha256Source[] } }) | undefined} */
+		let result;
+		let render_error;
+		let failed = false;
 
 		try {
-			const renderer = Renderer.#open_render('async', component, options);
+			try {
+				Renderer.#open_render(renderer, component, options);
 				const content = await renderer.#collect_content_async();
 				const hydratables = await renderer.#collect_hydratables();
 				if (hydratables !== null) {
 					content.head = hydratables + content.head;
 				}
-			return Renderer.#close_render(content, renderer);
+				result = Renderer.#close_render(content, renderer);
+			} catch (error) {
+				render_error = error;
+				failed = true;
+				renderer.global.abort();
+				await renderer.global.settle();
+			}
+
+			renderer.#run_on_destroy(failed);
+			if (failed) throw render_error;
+
+			return /** @type {AccumulatedContent & { hashes: { script: Sha256Source[] } }} */ (result);
 		} finally {
 			set_ssr_context(previous_context);
-			abort();
+			renderer.global.abort();
 		}
 	}
 
@@ -760,28 +839,15 @@ export class Renderer {
 
 	/**
 	 * @template {Record<string, any>} Props
-	 * @param {'sync' | 'async'} mode
+	 * @param {Renderer} renderer
 	 * @param {import('svelte').Component<Props>} component
 	 * @param {{ props?: Omit<Props, '$$slots' | '$$events'>; context?: Map<any, any>; idPrefix?: string; csp?: Csp; transformError?: (error: unknown) => unknown }} options
-	 * @returns {Renderer}
+	 * @returns {void}
 	 */
-	static #open_render(mode, component, options) {
+	static #open_render(renderer, component, options) {
-		if (options.idPrefix?.includes('--')) {
-			e.invalid_id_prefix();
-		}
-
 		var previous_context = ssr_context;
 
 		try {
-			const renderer = new Renderer(
-				new SSRState(
-					mode,
-					options.idPrefix ? options.idPrefix + '-' : '',
-					options.csp,
-					options.transformError
-				)
-			);
-
 			/** @type {SSRContext} */
 			const context = { p: null, c: options.context ?? null, r: renderer };
 			set_ssr_context(context);
@@ -790,8 +856,6 @@ export class Renderer {
 			// @ts-expect-error
 			component(renderer, options.props ?? {});
 			renderer.push(BLOCK_CLOSE);
-
-			return renderer;
 		} finally {
 			set_ssr_context(previous_context);
 		}
@@ -803,10 +867,6 @@ export class Renderer {
 	 * @returns {AccumulatedContent & { hashes: { script: Sha256Source[] } }}
 	 */
 	static #close_render(content, renderer) {
-		for (const cleanup of renderer.#collect_on_destroy()) {
-			cleanup();
-		}
-
 		let head = content.head + renderer.global.get_title();
 		let body = content.body;
 
@@ -890,6 +950,14 @@ export class SSRState {
 	/** @readonly @type {Set<{ hash: string; code: string }>} */
 	css = new Set();
 
+	/** @type {Set<Promise<unknown>>} */
+	#pending = new Set();
+
+	/** @type {AbortController | null} */
+	#controller = null;
+
+	#aborted = false;
+
 	/**
 	 * `transformError` passed to `render`. Called when an error boundary catches an error.
 	 * Throws by default if unset in `render`.
@@ -920,6 +988,38 @@ export class SSRState {
 		this.uid = () => `${id_prefix}s${uid++}`;
 	}
 
+	/**
+	 * @template T
+	 * @param {Promise<T>} promise
+	 * @returns {Promise<T>}
+	 */
+	track(promise) {
+		this.#pending.add(promise);
+		promise.then(
+			() => this.#pending.delete(promise),
+			() => this.#pending.delete(promise)
+		);
+		return promise;
+	}
+
+	async settle() {
+		while (this.#pending.size > 0) {
+			await Promise.allSettled([...this.#pending]);
+		}
+	}
+
+	abort() {
+		if (this.#aborted) return;
+		this.#aborted = true;
+		this.#controller?.abort(STALE_REACTION);
+	}
+
+	get_abort_signal() {
+		const controller = (this.#controller ??= new AbortController());
+		if (this.#aborted) controller.abort(STALE_REACTION);
+		return controller.signal;
+	}
+
 	get_title() {
 		return this.#title.value;
 	}

packages/svelte/src/internal/server/renderer.test.ts

@@ -2,6 +2,7 @@ import { afterAll, beforeAll, describe, expect, test } from 'vitest';
 import { Renderer, SSRState } from './renderer.js';
 import type { Component } from 'svelte';
 import { disable_async_mode_flag, enable_async_mode_flag } from '../flags/index.js';
+import { getAbortSignal } from './abort-signal.js';
 
 test('collects synchronous body content by default', () => {
 	const component = (renderer: Renderer) => {
@@ -466,4 +467,176 @@ describe('async', () => {
 		await Renderer.render(component as unknown as Component);
 		expect(destroyed).toEqual(['c', 'e', 'a', 'b', 'b*', 'd']);
 	});
+
+	test('on_destroy callbacks run when a sync render throws', () => {
+		const destroyed: string[] = [];
+		const component = (renderer: Renderer) => {
+			renderer.component((renderer) => {
+				renderer.on_destroy(() => destroyed.push('a'));
+				renderer.child(() => {
+					throw new Error('boom');
+				});
+			});
+		};
+
+		expect(() => Renderer.render(component as unknown as Component).body).toThrow('boom');
+		expect(destroyed).toEqual(['a']);
+	});
+
+	test('on_destroy callbacks run when an async render rejects', async () => {
+		const destroyed: string[] = [];
+		const component = (renderer: Renderer) => {
+			renderer.component((renderer) => {
+				renderer.on_destroy(() => destroyed.push('a'));
+				renderer.child(async () => {
+					await Promise.resolve();
+					throw new Error('boom');
+				});
+			});
+		};
+
+		await expect(Renderer.render(component as unknown as Component)).rejects.toThrow('boom');
+		expect(destroyed).toEqual(['a']);
+	});
+
+	test('on_destroy waits for in-flight renderers when an async render rejects', async () => {
+		const events: string[] = [];
+		let initialised = false;
+
+		const component = (renderer: Renderer) => {
+			renderer.component((renderer) => {
+				// rejects while the sibling component below is still in flight
+				renderer.child(async () => {
+					await Promise.resolve();
+					throw new Error('boom');
+				});
+
+				renderer.component((renderer) => {
+					renderer.on_destroy(() => events.push(`before-await (initialised: ${initialised})`));
+					renderer.child(async () => {
+						await new Promise((f) => setTimeout(f, 10));
+						initialised = true;
+						renderer.on_destroy(() => events.push('after-await'));
+					});
+				});
+			});
+		};
+
+		await expect(Renderer.render(component as unknown as Component)).rejects.toThrow('boom');
+		expect(events).toEqual(['before-await (initialised: true)', 'after-await']);
+	});
+
+	test('aborts in-flight renderers before waiting for them', async () => {
+		const events: string[] = [];
+		const component = (renderer: Renderer) => {
+			renderer.component((renderer) => {
+				renderer.child(async () => {
+					await Promise.resolve();
+					throw new Error('boom');
+				});
+
+				renderer.component((renderer) => {
+					renderer.on_destroy(() => events.push('destroyed'));
+					renderer.child(async () => {
+						const signal = getAbortSignal();
+						await new Promise((_, reject) => {
+							signal.addEventListener('abort', () => reject(signal.reason), { once: true });
+						});
+					});
+				});
+			});
+		};
+
+		await expect(Renderer.render(component as unknown as Component)).rejects.toThrow('boom');
+		expect(events).toEqual(['destroyed']);
+	});
+
+	test('on_destroy waits for every run invocation when an async render rejects', async () => {
+		const events: string[] = [];
+		let initialised = false;
+		const component = (renderer: Renderer) => {
+			renderer.component((renderer) => {
+				renderer.on_destroy(() => events.push(`destroyed (initialised: ${initialised})`));
+				renderer.run([
+					async () => {
+						await new Promise((resolve) => setTimeout(resolve, 10));
+						initialised = true;
+						renderer.on_destroy(() => events.push('destroyed after await'));
+					}
+				]);
+				renderer.run([
+					async () => {
+						await Promise.resolve();
+						throw new Error('boom');
+					}
+				]);
+			});
+		};
+
+		await expect(Renderer.render(component as unknown as Component)).rejects.toThrow('boom');
+		expect(events).toEqual(['destroyed (initialised: true)', 'destroyed after await']);
+	});
+
+	test('abort signals are scoped to a render', async () => {
+		let signal: AbortSignal;
+		let start!: () => void;
+		let resume!: () => void;
+		const started = new Promise<void>((resolve) => (start = resolve));
+		const resumed = new Promise<void>((resolve) => (resume = resolve));
+		const component = (renderer: Renderer) => {
+			renderer.child(async () => {
+				signal = getAbortSignal();
+				start();
+				await resumed;
+			});
+		};
+
+		const first_render = Promise.resolve(Renderer.render(component as unknown as Component));
+		await started;
+		await Renderer.render((() => {}) as unknown as Component);
+		expect(signal!.aborted).toBe(false);
+
+		resume();
+		await first_render;
+		expect(signal!.aborted).toBe(true);
+	});
+
+	test('a throwing on_destroy callback does not mask a sync render error', () => {
+		const destroyed: string[] = [];
+		const component = (renderer: Renderer) => {
+			renderer.component((renderer) => {
+				renderer.on_destroy(() => {
+					destroyed.push('a');
+					throw new Error('cleanup failed');
+				});
+				renderer.on_destroy(() => destroyed.push('b'));
+				renderer.child(() => {
+					throw new Error('boom');
+				});
+			});
+		};
+
+		expect(() => Renderer.render(component as unknown as Component).body).toThrow('boom');
+		expect(destroyed).toEqual(['a', 'b']);
+	});
+
+	test('a throwing on_destroy callback does not mask an async render error', async () => {
+		const destroyed: string[] = [];
+		const component = (renderer: Renderer) => {
+			renderer.component((renderer) => {
+				renderer.on_destroy(() => {
+					destroyed.push('a');
+					throw new Error('cleanup failed');
+				});
+				renderer.on_destroy(() => destroyed.push('b'));
+				renderer.child(async () => {
+					await Promise.resolve();
+					throw new Error('boom');
+				});
+			});
+		};
+
+		await expect(Renderer.render(component as unknown as Component)).rejects.toThrow('boom');
+		expect(destroyed).toEqual(['a', 'b']);
+	});
 });