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 <rich.harris@vercel.com>

.changeset/stupid-chefs-rescue.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: avoid traversing clean roots

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 —
+		// any effects that were previously deferred should be transferred
-		// after the next traversal (which will happen immediately, due to the
+		// to the batch, which will flush in the next microtask
-		// same update that brought us here) the effects will be flushed
+		batch.transfer_effects(this.#dirty_effects, this.#maybe_dirty_effects);
-		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();
 	}
 
 	/**

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<Batch>} */
 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<Value>} */
@@ -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<Effect>} dirty_effects
+	 * @param {Set<Effect>} 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);

packages/svelte/tests/runtime-runes/samples/async-pending-effect/Component.svelte

@@ -0,0 +1,11 @@
+<script lang="ts">
+	let x = $state(false);
+
+	$effect(() => {
+		x = true;
+
+		return () => {
+			x = false;
+		}
+	});
+</script>

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, `<button>toggle</button><p>hello</p>`);
+	}
+});

packages/svelte/tests/runtime-runes/samples/async-pending-effect/main.svelte

@@ -0,0 +1,19 @@
+<script lang="ts">
+	import Component from './Component.svelte';
+
+	let condition = $state(false);
+</script>
+
+<button onclick={() => (condition = !condition)}>toggle</button>
+
+<svelte:boundary>
+	<Component whatever={await 1} />
+
+	{#snippet pending()}
+		<Component />
+	{/snippet}
+</svelte:boundary>
+
+{#if condition}
+	<p>hello</p>
+{/if}