From 9bd44762b4e21afa904881c7841e7a5fa689088f Mon Sep 17 00:00:00 2001 From: Nic <162764842+Nic-Polumeyv@users.noreply.github.com> Date: Fri, 24 Jul 2026 19:55:15 -0400 Subject: [PATCH] fix: settle in-flight async work before running onDestroy callbacks on failure --- .../svelte/src/internal/server/renderer.js | 79 +++++++++++++++++-- .../src/internal/server/renderer.test.ts | 58 ++++++++++++++ 2 files changed, 132 insertions(+), 5 deletions(-) diff --git a/packages/svelte/src/internal/server/renderer.js b/packages/svelte/src/internal/server/renderer.js index c79e6dfe30..0ad03c63fc 100644 --- a/packages/svelte/src/internal/server/renderer.js +++ b/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>} */ + const seen = new Set(); + + /** @type {Promise[]} */ + let pending; + + do { + pending = []; + this.#collect_pending(seen, pending); + await Promise.allSettled(pending); + } while (pending.length > 0); + } + + /** + * @param {Set>} seen + * @param {Promise[]} 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(); } diff --git a/packages/svelte/src/internal/server/renderer.test.ts b/packages/svelte/src/internal/server/renderer.test.ts index 44c96f75b4..e67a47f107 100644 --- a/packages/svelte/src/internal/server/renderer.test.ts +++ b/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'); + }); });