From 3343a28232e51e03c066ac39059d636903ad4268 Mon Sep 17 00:00:00 2001 From: Nic <162764842+Nic-Polumeyv@users.noreply.github.com> Date: Fri, 24 Jul 2026 15:10:36 -0400 Subject: [PATCH 1/4] fix: run `onDestroy` callbacks when a server render throws --- .changeset/great-hoops-tickle.md | 5 ++ .../svelte/src/internal/server/renderer.js | 63 ++++++++++++++----- .../src/internal/server/renderer.test.ts | 31 +++++++++ 3 files changed, 83 insertions(+), 16 deletions(-) create mode 100644 .changeset/great-hoops-tickle.md diff --git a/.changeset/great-hoops-tickle.md b/.changeset/great-hoops-tickle.md new file mode 100644 index 0000000000..94b32588f1 --- /dev/null +++ b/.changeset/great-hoops-tickle.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: run `onDestroy` callbacks when a server render throws diff --git a/packages/svelte/src/internal/server/renderer.js b/packages/svelte/src/internal/server/renderer.js index 35aac64721..4bdcc617c1 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 belonging to this renderer tree have run. + * @type {boolean} + */ + #on_destroy_ran = false; + /** * Whether this renderer is a component body. * @type {boolean} @@ -316,8 +322,11 @@ export class Renderer { */ component(fn, component_fn) { push(component_fn); - const child = this.child(fn); - child.#is_component_body = true; + // mark before running so `onDestroy` callbacks are still collected if `fn` throws + this.child((renderer) => { + renderer.#is_component_body = true; + return fn(renderer); + }); pop(); } @@ -624,6 +633,19 @@ export class Renderer { } } + /** + * Runs the `onDestroy` callbacks of this renderer tree exactly once, + * whether the render succeeded or failed. + */ + #run_on_destroy() { + if (this.#on_destroy_ran) return; + this.#on_destroy_ran = true; + + for (const cleanup of this.#collect_on_destroy()) { + cleanup(); + } + } + /** * Render a component. Throws if any of the children are performing asynchronous work. * @@ -634,12 +656,15 @@ export class Renderer { */ static #render(component, options) { var previous_context = ssr_context; + /** @type {Renderer | undefined} */ + var renderer; try { - const renderer = Renderer.#open_render('sync', component, options); + renderer = Renderer.#open_render('sync', component, options); const content = renderer.#collect_content(); return Renderer.#close_render(content, renderer); } finally { + renderer?.#run_on_destroy(); abort(); set_ssr_context(previous_context); } @@ -655,9 +680,11 @@ export class Renderer { */ static async #render_async(component, options) { const previous_context = ssr_context; + /** @type {Renderer | undefined} */ + let renderer; try { - const renderer = Renderer.#open_render('async', component, options); + renderer = Renderer.#open_render('async', component, options); const content = await renderer.#collect_content_async(); const hydratables = await renderer.#collect_hydratables(); if (hydratables !== null) { @@ -665,6 +692,7 @@ export class Renderer { } return Renderer.#close_render(content, renderer); } finally { + renderer?.#run_on_destroy(); set_ssr_context(previous_context); abort(); } @@ -770,16 +798,16 @@ export class Renderer { var previous_context = ssr_context; - try { - const renderer = new Renderer( - new SSRState( - mode, - options.idPrefix ? options.idPrefix + '-' : '', - options.csp, - options.transformError - ) - ); + const renderer = new Renderer( + new SSRState( + mode, + options.idPrefix ? options.idPrefix + '-' : '', + options.csp, + options.transformError + ) + ); + try { /** @type {SSRContext} */ const context = { p: null, c: options.context ?? null, r: renderer }; set_ssr_context(context); @@ -790,6 +818,11 @@ export class Renderer { renderer.push(BLOCK_CLOSE); return renderer; + } catch (error) { + // restore context first so callbacks run outside it, as on the success path + set_ssr_context(previous_context); + renderer.#run_on_destroy(); + throw error; } finally { set_ssr_context(previous_context); } @@ -801,9 +834,7 @@ export class Renderer { * @returns {AccumulatedContent & { hashes: { script: Sha256Source[] } }} */ static #close_render(content, renderer) { - for (const cleanup of renderer.#collect_on_destroy()) { - cleanup(); - } + renderer.#run_on_destroy(); let head = content.head + renderer.global.get_title(); let body = content.body; diff --git a/packages/svelte/src/internal/server/renderer.test.ts b/packages/svelte/src/internal/server/renderer.test.ts index 8e98c41796..44c96f75b4 100644 --- a/packages/svelte/src/internal/server/renderer.test.ts +++ b/packages/svelte/src/internal/server/renderer.test.ts @@ -466,4 +466,35 @@ 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']); + }); }); From 5de017554e51bcad7ca92140d4aa3439e2fbec82 Mon Sep 17 00:00:00 2001 From: Nic <162764842+Nic-Polumeyv@users.noreply.github.com> Date: Fri, 24 Jul 2026 15:21:19 -0400 Subject: [PATCH 2/4] refactor: single execution point for onDestroy callbacks --- .../svelte/src/internal/server/renderer.js | 72 ++++++++----------- 1 file changed, 30 insertions(+), 42 deletions(-) diff --git a/packages/svelte/src/internal/server/renderer.js b/packages/svelte/src/internal/server/renderer.js index 4bdcc617c1..26f2c0a5eb 100644 --- a/packages/svelte/src/internal/server/renderer.js +++ b/packages/svelte/src/internal/server/renderer.js @@ -44,12 +44,6 @@ export class Renderer { */ #on_destroy = undefined; - /** - * Whether the `onDestroy` callbacks belonging to this renderer tree have run. - * @type {boolean} - */ - #on_destroy_ran = false; - /** * Whether this renderer is a component body. * @type {boolean} @@ -634,18 +628,35 @@ export class Renderer { } /** - * Runs the `onDestroy` callbacks of this renderer tree exactly once, + * Runs the `onDestroy` callbacks of this renderer tree, * whether the render succeeded or failed. */ #run_on_destroy() { - if (this.#on_destroy_ran) return; - this.#on_destroy_ran = true; - for (const cleanup of this.#collect_on_destroy()) { cleanup(); } } + /** + * @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. * @@ -656,15 +667,14 @@ export class Renderer { */ static #render(component, options) { var previous_context = ssr_context; - /** @type {Renderer | undefined} */ - var renderer; + const renderer = Renderer.#create('sync', options); try { - renderer = Renderer.#open_render('sync', component, options); + Renderer.#open_render(renderer, component, options); const content = renderer.#collect_content(); return Renderer.#close_render(content, renderer); } finally { - renderer?.#run_on_destroy(); + renderer.#run_on_destroy(); abort(); set_ssr_context(previous_context); } @@ -680,11 +690,10 @@ export class Renderer { */ static async #render_async(component, options) { const previous_context = ssr_context; - /** @type {Renderer | undefined} */ - let renderer; + const renderer = Renderer.#create('async', options); try { - renderer = Renderer.#open_render('async', component, options); + Renderer.#open_render(renderer, component, options); const content = await renderer.#collect_content_async(); const hydratables = await renderer.#collect_hydratables(); if (hydratables !== null) { @@ -692,7 +701,7 @@ export class Renderer { } return Renderer.#close_render(content, renderer); } finally { - renderer?.#run_on_destroy(); + renderer.#run_on_destroy(); set_ssr_context(previous_context); abort(); } @@ -786,27 +795,14 @@ export class Renderer { /** * @template {Record} Props - * @param {'sync' | 'async'} mode + * @param {Renderer} renderer * @param {import('svelte').Component} component * @param {{ props?: Omit; context?: Map; idPrefix?: string; csp?: Csp; transformError?: (error: unknown) => unknown }} options - * @returns {Renderer} + * @returns {void} */ - static #open_render(mode, component, options) { - if (options.idPrefix?.includes('--')) { - e.invalid_id_prefix(); - } - + static #open_render(renderer, component, options) { var previous_context = ssr_context; - const renderer = new Renderer( - new SSRState( - mode, - options.idPrefix ? options.idPrefix + '-' : '', - options.csp, - options.transformError - ) - ); - try { /** @type {SSRContext} */ const context = { p: null, c: options.context ?? null, r: renderer }; @@ -816,13 +812,6 @@ export class Renderer { // @ts-expect-error component(renderer, options.props ?? {}); renderer.push(BLOCK_CLOSE); - - return renderer; - } catch (error) { - // restore context first so callbacks run outside it, as on the success path - set_ssr_context(previous_context); - renderer.#run_on_destroy(); - throw error; } finally { set_ssr_context(previous_context); } @@ -834,7 +823,6 @@ export class Renderer { * @returns {AccumulatedContent & { hashes: { script: Sha256Source[] } }} */ static #close_render(content, renderer) { - renderer.#run_on_destroy(); let head = content.head + renderer.global.get_title(); let body = content.body; From e119b4af5f1ab975a8f28b89653fe449aaa2b697 Mon Sep 17 00:00:00 2001 From: Nic <162764842+Nic-Polumeyv@users.noreply.github.com> Date: Fri, 24 Jul 2026 15:31:46 -0400 Subject: [PATCH 3/4] chore: format --- packages/svelte/src/internal/server/renderer.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/svelte/src/internal/server/renderer.js b/packages/svelte/src/internal/server/renderer.js index 26f2c0a5eb..c79e6dfe30 100644 --- a/packages/svelte/src/internal/server/renderer.js +++ b/packages/svelte/src/internal/server/renderer.js @@ -823,7 +823,6 @@ export class Renderer { * @returns {AccumulatedContent & { hashes: { script: Sha256Source[] } }} */ static #close_render(content, renderer) { - let head = content.head + renderer.global.get_title(); let body = content.body; 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 4/4] 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'); + }); });