From 1ebed6832270b7ae6abf5251d2e659a39687dd13 Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Fri, 13 Mar 2026 22:25:45 +0100 Subject: [PATCH] fix: avoid traversing clean roots (#17928) Before #17805, all batches drew from the same `queued_root_effects` and did reset them to the empty array when starting a flush. After the refactoring roots are scheduled per batch. This introduces a possible race condition where the same root is scheduled multiple times. It was possible because of the rebase logic in `#commit` not clearing the array of roots, so if you somehow flush that same batch later, you will end up traversing a clean root. (it is possible a bug like this always existed with rebasing it was just impossible hard to trigger it before because everyone drew from the same root effects array) The fix is a bit more complicated than just checking if new roots where added, we gotta check if we actually created async work before traversing. Fixes #17918 --------- Co-authored-by: Rich Harris --- .changeset/stupid-chefs-rescue.md | 5 ++ .../internal/client/dom/blocks/boundary.js | 18 +----- .../src/internal/client/reactivity/batch.js | 57 +++++++++++++------ .../async-pending-effect/Component.svelte | 11 ++++ .../samples/async-pending-effect/_config.js | 16 ++++++ .../samples/async-pending-effect/main.svelte | 19 +++++++ 6 files changed, 95 insertions(+), 31 deletions(-) create mode 100644 .changeset/stupid-chefs-rescue.md create mode 100644 packages/svelte/tests/runtime-runes/samples/async-pending-effect/Component.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/async-pending-effect/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/async-pending-effect/main.svelte diff --git a/.changeset/stupid-chefs-rescue.md b/.changeset/stupid-chefs-rescue.md new file mode 100644 index 0000000000..d21389e97a --- /dev/null +++ b/.changeset/stupid-chefs-rescue.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: avoid traversing clean roots diff --git a/packages/svelte/src/internal/client/dom/blocks/boundary.js b/packages/svelte/src/internal/client/dom/blocks/boundary.js index 8046f1e222..b440bb3ba4 100644 --- a/packages/svelte/src/internal/client/dom/blocks/boundary.js +++ b/packages/svelte/src/internal/client/dom/blocks/boundary.js @@ -271,21 +271,9 @@ export class Boundary { #resolve(batch) { this.is_pending = false; - // any effects that were previously deferred should be rescheduled — - // after the next traversal (which will happen immediately, due to the - // same update that brought us here) the effects will be flushed - for (const e of this.#dirty_effects) { - set_signal_status(e, DIRTY); - batch.schedule(e); - } - - for (const e of this.#maybe_dirty_effects) { - set_signal_status(e, MAYBE_DIRTY); - batch.schedule(e); - } - - this.#dirty_effects.clear(); - this.#maybe_dirty_effects.clear(); + // any effects that were previously deferred should be transferred + // to the batch, which will flush in the next microtask + batch.transfer_effects(this.#dirty_effects, this.#maybe_dirty_effects); } /** diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index ebaed93e9c..3a4872b0b6 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -38,6 +38,8 @@ import { defer_effect } from './utils.js'; import { UNINITIALIZED } from '../../../constants.js'; import { set_signal_status } from './status.js'; import { legacy_is_updating_store } from './store.js'; +import { invariant } from '../../shared/dev.js'; +import { log_effect_tree } from '../dev/debug.js'; /** @type {Set} */ const batches = new Set(); @@ -204,9 +206,25 @@ export class Batch { #process() { if (flush_count++ > 1000) { + batches.delete(this); infinite_loop_guard(); } + // we only reschedule previously-deferred effects if we expect + // to be able to run them after processing the batch + if (!this.#is_deferred()) { + for (const e of this.#dirty_effects) { + this.#maybe_dirty_effects.delete(e); + set_signal_status(e, DIRTY); + this.schedule(e); + } + + for (const e of this.#maybe_dirty_effects) { + set_signal_status(e, MAYBE_DIRTY); + this.schedule(e); + } + } + const roots = this.#roots; this.#roots = []; @@ -396,21 +414,6 @@ export class Batch { is_processing = true; current_batch = this; - // we only reschedule previously-deferred effects if we expect - // to be able to run them after processing the batch - if (!this.#is_deferred()) { - for (const e of this.#dirty_effects) { - this.#maybe_dirty_effects.delete(e); - set_signal_status(e, DIRTY); - this.schedule(e); - } - - for (const e of this.#maybe_dirty_effects) { - set_signal_status(e, MAYBE_DIRTY); - this.schedule(e); - } - } - this.#process(); } finally { flush_count = 0; @@ -470,6 +473,10 @@ export class Batch { // Re-run async/block effects that depend on distinct values changed in both batches var others = [...batch.current.keys()].filter((s) => !this.current.has(s)); if (others.length > 0) { + if (DEV) { + invariant(batch.#roots.length === 0, 'Batch has scheduled roots'); + } + batch.activate(); /** @type {Set} */ @@ -482,6 +489,7 @@ export class Batch { mark_effects(source, others, marked, checked); } + // Only apply and traverse when we know we triggered async work with marking the effects if (batch.#roots.length > 0) { batch.apply(); @@ -489,7 +497,7 @@ export class Batch { batch.#traverse(root, [], []); } - // TODO do we need to do anything with the dummy effect arrays? + batch.#roots = []; } batch.deactivate(); @@ -523,6 +531,23 @@ export class Batch { }); } + /** + * @param {Set} dirty_effects + * @param {Set} maybe_dirty_effects + */ + transfer_effects(dirty_effects, maybe_dirty_effects) { + for (const e of dirty_effects) { + this.#dirty_effects.add(e); + } + + for (const e of maybe_dirty_effects) { + this.#maybe_dirty_effects.add(e); + } + + dirty_effects.clear(); + maybe_dirty_effects.clear(); + } + /** @param {(batch: Batch) => void} fn */ oncommit(fn) { this.#commit_callbacks.add(fn); diff --git a/packages/svelte/tests/runtime-runes/samples/async-pending-effect/Component.svelte b/packages/svelte/tests/runtime-runes/samples/async-pending-effect/Component.svelte new file mode 100644 index 0000000000..b39ca3dfd4 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-pending-effect/Component.svelte @@ -0,0 +1,11 @@ + \ No newline at end of file diff --git a/packages/svelte/tests/runtime-runes/samples/async-pending-effect/_config.js b/packages/svelte/tests/runtime-runes/samples/async-pending-effect/_config.js new file mode 100644 index 0000000000..083608e9dc --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-pending-effect/_config.js @@ -0,0 +1,16 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target }) { + // This test causes two batches to be scheduled such that the same root is traversed multiple times, + // some of the time while it was already marked clean by a previous batch processing. It tests + // that the app stays reactive after, i.e. that the root is not improperly marked as unclean. + await tick(); + const [button] = target.querySelectorAll('button'); + + button.click(); + await tick(); + assert.htmlEqual(target.innerHTML, `

hello

`); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-pending-effect/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-pending-effect/main.svelte new file mode 100644 index 0000000000..d6c09803f9 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-pending-effect/main.svelte @@ -0,0 +1,19 @@ + + + + + + + + {#snippet pending()} + + {/snippet} + + +{#if condition} +

hello

+{/if}