fix: settle in-flight async work before running onDestroy callbacks on failure

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

@@ -44,6 +44,12 @@ export class Renderer {
 	 */
 	#on_destroy = undefined;
 
+	/**
+	 * Whether the `onDestroy` callbacks of this renderer tree have been run.
+	 * @type {boolean}
+	 */
+	#destroyed = false;
+
 	/**
 	 * Whether this renderer is a component body.
 	 * @type {boolean}
@@ -628,15 +634,54 @@ export class Renderer {
 	}
 
 	/**
-	 * Runs the `onDestroy` callbacks of this renderer tree,
+	 * Runs the `onDestroy` callbacks of this renderer tree at most once,
 	 * whether the render succeeded or failed.
 	 */
 	#run_on_destroy() {
+		if (this.#destroyed) return;
+		this.#destroyed = true;
+
 		for (const cleanup of this.#collect_on_destroy()) {
 			cleanup();
 		}
 	}
 
+	/**
+	 * Waits until every promise in the tree has settled, including promises created
+	 * while waiting. This makes `#collect_on_destroy` safe to call after a failed
+	 * async render, where siblings of the rejected renderer are still in flight.
+	 */
+	async #settle() {
+		/** @type {Set<Promise<void>>} */
+		const seen = new Set();
+
+		/** @type {Promise<void>[]} */
+		let pending;
+
+		do {
+			pending = [];
+			this.#collect_pending(seen, pending);
+			await Promise.allSettled(pending);
+		} while (pending.length > 0);
+	}
+
+	/**
+	 * @param {Set<Promise<void>>} seen
+	 * @param {Promise<void>[]} pending
+	 */
+	#collect_pending(seen, pending) {
+		if (this.promise !== undefined && !seen.has(this.promise)) {
+			seen.add(this.promise);
+			pending.push(this.promise);
+		}
+
+		for (const child of this.#out) {
+			if (typeof child !== 'string') {
+				child.#collect_pending(seen, pending);
+			}
+		}
+	}
+
 	/**
 	 * @param {'sync' | 'async'} mode
 	 * @param {{ idPrefix?: string; csp?: Csp; transformError?: (error: unknown) => unknown }} options
@@ -672,9 +717,19 @@ export class Renderer {
 			Renderer.#open_render(renderer, component, options);
 
 			const content = renderer.#collect_content();
-			return Renderer.#close_render(content, renderer);
-		} finally {
+			const result = Renderer.#close_render(content, renderer);
+
 			renderer.#run_on_destroy();
+			return result;
+		} catch (error) {
+			try {
+				renderer.#run_on_destroy();
+			} catch {
+				// a throwing cleanup must not mask the error that failed the render
+			}
+
+			throw error;
+		} finally {
 			abort();
 			set_ssr_context(previous_context);
 		}
@@ -699,9 +754,23 @@ export class Renderer {
 			if (hydratables !== null) {
 				content.head = hydratables + content.head;
 			}
-			return Renderer.#close_render(content, renderer);
-		} finally {
+			const result = Renderer.#close_render(content, renderer);
+
 			renderer.#run_on_destroy();
+			return result;
+		} catch (error) {
+			// in-flight siblings of the rejected renderer must finish initialising
+			// before their cleanup runs, and may register more callbacks after resuming
+			await renderer.#settle();
+
+			try {
+				renderer.#run_on_destroy();
+			} catch {
+				// a throwing cleanup must not mask the error that failed the render
+			}
+
+			throw error;
+		} finally {
 			set_ssr_context(previous_context);
 			abort();
 		}

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

@@ -497,4 +497,62 @@ describe('async', () => {
 		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('a throwing on_destroy callback does not mask a sync render error', () => {
+		const component = (renderer: Renderer) => {
+			renderer.component((renderer) => {
+				renderer.on_destroy(() => {
+					throw new Error('cleanup failed');
+				});
+				renderer.child(() => {
+					throw new Error('boom');
+				});
+			});
+		};
+
+		expect(() => Renderer.render(component as unknown as Component).body).toThrow('boom');
+	});
+
+	test('a throwing on_destroy callback does not mask an async render error', async () => {
+		const component = (renderer: Renderer) => {
+			renderer.component((renderer) => {
+				renderer.on_destroy(() => {
+					throw new Error('cleanup failed');
+				});
+				renderer.child(async () => {
+					await Promise.resolve();
+					throw new Error('boom');
+				});
+			});
+		};
+
+		await expect(Renderer.render(component as unknown as Component)).rejects.toThrow('boom');
+	});
 });