fix: don't rebase just-created batches (#18117)

It's possible to rebase just-created batches.

Case A:
- batch A runs effects
- one of these effects writes to a source. This creates a new batch B
- an effect _after_ that (still part of "flush effects of batch A")
executes a derived. This creates an entry in the `current` Map in batch
B
- batch A commits after processing batch B (`next_batch` etc logic),
batch B is pending. Due to derived being part of batchB.current batch A
can wrongfully think these are connected and try to rerun/add effects
etc on batch B

Case B:
- like case A but with an additional await inside a pending snippet

Case C:
- batch A with source a and b, it flushes effects
- one of these effects schedules batch B with b and c scheduling an
async effect
- batch B is deferred
- batch A commits. Due to the a/b/c partial overlap it will needlessly
rerun the just scheduled async effect

All these cases are wrong. We fix it like this:
1. we call `this.#commit()` _before_ running the new batches, which may
stick around due to having pending work, and we don't want to rebase
these. This fixes case A and C
2. we capture derived values in `previous_batch` if it exists, because
it means we're currently flushing effects, and derived writes belong to
that batch and not a new one that might have been scheduled already.
This fixes case B

Discovered this while working on #18097

---------

Co-authored-by: Rich Harris <rich.harris@vercel.com>

.changeset/easy-singers-retire.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: don't rebase just-created batches

packages/svelte/src/internal/client/reactivity/batch.js

@@ -342,6 +342,14 @@ export class Batch {
 			this.#deferred?.resolve();
 		}
 
+		// Order matters here - we need to commit and THEN continue flushing new batches, not the other way around,
+		// else we could start flushing a new batch and then, if it has pending work, rebase it right afterwards, which is wrong.
+		// In sync mode flushSync can cause #commit to wrongfully think that there needs to be a rebase, so we only do it in async mode
+		// TODO fix the underlying cause, otherwise this will likely regress when non-async mode is removed
+		if (async_mode_flag && !batches.has(this)) {
+			this.#commit();
+		}
+
 		var next_batch = /** @type {Batch | null} */ (/** @type {unknown} */ (current_batch));
 
 		// Edge case: During traversal new branches might create effects that run immediately and set state,
@@ -363,12 +371,6 @@ export class Batch {
 
 			next_batch.#process();
 		}
-
-		// In sync mode flushSync can cause #commit to wrongfully think that there needs to be a rebase, so we only do it in async mode
-		// TODO fix the underlying cause, otherwise this will likely regress when non-async mode is removed
-		if (async_mode_flag && !batches.has(this)) {
-			this.#commit();
-		}
 	}
 
 	/**
@@ -575,9 +577,12 @@ export class Batch {
 
 				checked = new Map();
 				var current_unequal = [...batch.current.keys()].filter((c) =>
-					this.current.has(c) ? /** @type {[any, boolean]} */ (this.current.get(c))[0] !== c : true
+					this.current.has(c)
+						? /** @type {[any, boolean]} */ (this.current.get(c))[0] !== c.v
+						: true
 				);
 
+				if (current_unequal.length > 0) {
 					for (const effect of this.#new_effects) {
 						if (
 							(effect.f & (DESTROYED | INERT | EAGER_EFFECT)) === 0 &&
@@ -591,6 +596,7 @@ export class Batch {
 							}
 						}
 					}
+				}
 
 				// Only apply and traverse when we know we triggered async work with marking the effects
 				if (batch.#roots.length > 0) {

packages/svelte/src/internal/client/reactivity/deriveds.js

@@ -43,7 +43,7 @@ import { get_error } from '../../shared/dev.js';
 import { async_mode_flag, tracing_mode_flag } from '../../flags/index.js';
 import { component_context } from '../context.js';
 import { UNINITIALIZED } from '../../../constants.js';
-import { batch_values, current_batch } from './batch.js';
+import { batch_values, current_batch, previous_batch } from './batch.js';
 import { increment_pending, unset_context } from './async.js';
 import { deferred, includes, noop } from '../../shared/utils.js';
 import { set_signal_status, update_derived_status } from './status.js';
@@ -399,7 +399,14 @@ export function update_derived(derived) {
 		// change, `derived.equals` may incorrectly return `true`
 		if (!current_batch?.is_fork || derived.deps === null) {
 			if (current_batch !== null) {
+				// We also write to previous_batch because if it exists, it is a sign that we're
+				// currently in the process of flushing effects. These updates to deriveds may belong
+				// to the previous batch, not the new one (which can already exist if an earlier
+				// effect wrote to a source). This can cause bugs when running batch.#commit() later,
+				// but not adding it to current_batch can, too, so we add it to both.
+				// See https://github.com/sveltejs/svelte/pull/18117 for more details.
 				current_batch.capture(derived, value, true);
+				previous_batch?.capture(derived, value, true);
 			} else {
 				derived.v = value;
 			}

packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-1/_config.js

@@ -0,0 +1,27 @@
+import { tick } from 'svelte';
+import { test } from '../../test';
+
+// Tests that a newly created batch during an effect flush isn't rebased right away by the previous batch.#commit(),
+// rescheduling an effect on the new batch that shouldn't run.
+export default test({
+	async test({ assert, target, logs }) {
+		await tick();
+		const [increment, resolve] = target.querySelectorAll('button');
+
+		increment.click();
+		await tick();
+		assert.deepEqual(logs, []);
+
+		// This resolve
+		// - shouldn't result in the derived execution capturing the new derived value on the new batch, but on the previous batch which is currently flushing
+		// - shouldn't result in #commit() rebasing the new batch
+		resolve.click();
+		await tick();
+		assert.deepEqual(logs, [2]);
+
+		// As a result, this resolve shouldn't result in another execution of the effect depending on the derived
+		resolve.click();
+		await tick();
+		assert.deepEqual(logs, [2]);
+	}
+});

packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-1/main.svelte

@@ -0,0 +1,32 @@
+<script>
+	let count = $state(0);
+	let double = $derived(count * 2);
+	let count_mirror = $state(0);
+
+	const queued = [];
+	function delay(v) {
+		if (!v) return v;
+		return new Promise(resolve => {
+			queued.push(() => resolve(v));
+		});
+	}
+</script>
+
+<button onclick={() => count++}>count {await delay(count)} | count_mirror {await delay(count_mirror)}</button>
+<button onclick={() => queued.shift()?.()}>resolve</button>
+
+{#if count}
+	<!-- inside if block so effects are newly created and therefore added to batch.#new_effects -->
+	<!-- first $effect creates new batch ... -->
+	{(() => {
+		$effect(() => {
+			count_mirror = count;
+		})
+	})()}
+	<!-- ... which second $effect shouldn't write to because the derived execution belongs to the previous batch -->
+	{(() => {
+		$effect(() => {
+			console.log(double);
+		})
+	})()}
+{/if}

packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-2/_config.js

@@ -0,0 +1,25 @@
+import { tick } from 'svelte';
+import { test } from '../../test';
+
+// Tests that a newly created batch during an effect flush isn't rebased right away by the previous batch.#commit(),
+// rescheduling an effect on the new batch that shouldn't run.
+export default test({
+	async test({ assert, target, logs }) {
+		await tick();
+		const [increment, resolve] = target.querySelectorAll('button');
+		assert.deepEqual(logs, ['delay 0']);
+
+		increment.click();
+		await tick();
+		assert.deepEqual(logs, ['delay 0', 'delay 2']);
+
+		// This resolve should trigger the async effect only once
+		resolve.click();
+		await tick();
+		assert.deepEqual(logs, ['delay 0', 'delay 2', 'effect run', 'delay 4']);
+
+		resolve.click();
+		await tick();
+		assert.deepEqual(logs, ['delay 0', 'delay 2', 'effect run', 'delay 4']);
+	}
+});

packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-2/main.svelte

@@ -0,0 +1,29 @@
+<script>
+	import { untrack } from "svelte";
+
+	let a = $state(0);
+	let b = $state(0);
+	let c = $state(0);
+
+	const queued = [];
+	function delay(v) {
+		console.log('delay ' + v);
+		if (!v) return v;
+		return new Promise(resolve => {
+			queued.push(() => resolve(v));
+		});
+	}
+
+	$effect(() => {
+		if (b + c === 0 || b + c > 2) return;
+		console.log('effect run')
+		untrack(() => {
+			b++;
+			c++;
+		})
+	})
+</script>
+
+<button onclick={() => { a++; b++; }}>increment</button>
+<button onclick={() => queued.shift()?.()}>resolve</button>
+{await delay(a + b + c)}

packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-3/_config.js

@@ -0,0 +1,31 @@
+import { tick } from 'svelte';
+import { test } from '../../test';
+
+// Tests that a newly created batch during an effect flush isn't rebased right away by the previous batch.#commit(),
+// rescheduling an effect on the new batch that shouldn't run.
+export default test({
+	async test({ assert, target, logs }) {
+		await tick();
+		const [increment, shift, pop] = target.querySelectorAll('button');
+
+		increment.click();
+		await tick();
+		assert.deepEqual(logs, []);
+
+		// Resolve the blocking await which shouldn't result in the derived execution capturing
+		// the new derived value on the new batch, but on the previous batch which is currently flushing
+		pop.click();
+		await tick();
+		assert.deepEqual(logs, [2]);
+
+		// Resolve the non-blocking await which shouldn't result in #commit() rebasing the new batch
+		shift.click();
+		await tick();
+		assert.deepEqual(logs, [2]);
+
+		// Resolve the new batch's await
+		shift.click();
+		await tick();
+		assert.deepEqual(logs, [2]);
+	}
+});

packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-3/main.svelte

@@ -0,0 +1,37 @@
+<script>
+	let count = $state(0);
+	let double = $derived(count * 2);
+	let count_mirror = $state(0);
+
+	const queued = [];
+	function delay(v) {
+		if (!v) return v;
+		return new Promise(resolve => {
+			queued.push(() => resolve(v));
+		});
+	}
+</script>
+
+<button onclick={() => count++}>count {await delay(count)} | count_mirror {await delay(count_mirror)}</button>
+<button onclick={() => queued.shift()?.()}>shift</button>
+<button onclick={() => queued.pop()?.()}>pop</button>
+
+{#if count}
+	<svelte:boundary>
+		{await delay(count)}
+		{#snippet pending()}loading{/snippet}
+	</svelte:boundary>
+	<!-- inside if block so effects are newly created and therefore added to batch.#new_effects -->
+	<!-- first $effect creates new batch ... -->
+	{(() => {
+		$effect(() => {
+			count_mirror = count;
+		})
+	})()}
+	<!-- ... which second $effect shouldn't write to because the derived execution belongs to the previous batch -->
+	{(() => {
+		$effect(() => {
+			console.log(double);
+		})
+	})()}
+{/if}

packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-4/_config.js

@@ -0,0 +1,58 @@
+import { tick } from 'svelte';
+import { test } from '../../test';
+
+// Tests that a newly created batch during an effect flush isn't rebased right away by the previous batch.#commit(),
+// rescheduling an effect on the new batch that shouldn't run.
+export default test({
+	async test({ assert, target, logs }) {
+		await tick();
+		const [increment, unrelated, resolve] = target.querySelectorAll('button');
+
+		increment.click();
+		await tick();
+		assert.deepEqual(logs, []);
+
+		// This resolve
+		// - shouldn't result in the derived execution capturing the new derived value on the new batch, but on the previous batch which is currently flushing
+		// - shouldn't result in #commit() rebasing the new batch
+		resolve.click();
+		await tick();
+		assert.deepEqual(logs, [2]);
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<button>count 1 | count_mirror 0 | count_mirror_d 0 | unrelated 0</button>
+				<button>unrelated++</button>
+				<button>resolve</button>
+			`
+		);
+
+		// This resolve
+		// - shouldn't result in the derived execution capturing the new derived value on the new batch, but on the previous batch which is currently flushing
+		// - shouldn't result in #commit() rebasing the new batch
+		unrelated.click();
+		await tick();
+		assert.deepEqual(logs, [2]);
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<button>count 1 | count_mirror 0 | count_mirror_d 0 | unrelated 1</button>
+				<button>unrelated++</button>
+				<button>resolve</button>
+			`
+		);
+
+		// As a result, this resolve shouldn't result in another execution of the effect depending on the derived
+		resolve.click();
+		await tick();
+		assert.deepEqual(logs, [2]);
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<button>count 1 | count_mirror 1 | count_mirror_d 2 | unrelated 1</button>
+				<button>unrelated++</button>
+				<button>resolve</button>
+			`
+		);
+	}
+});

packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-4/main.svelte

@@ -0,0 +1,38 @@
+<script>
+	import { untrack } from "svelte";
+
+	let count = $state(0);
+	let double = $derived(count * 2);
+	let count_mirror = $state(0);
+	let unrelated = $state(0);
+	let count_mirror_d = $derived(count_mirror * 2);
+
+	const queued = [];
+	function delay(v) {
+		if (!v) return v;
+		return new Promise(resolve => {
+			queued.push(() => resolve(v));
+		});
+	}
+</script>
+
+<button onclick={() => count++}>count {await delay(count)} | count_mirror {await delay(count_mirror)} | count_mirror_d {count_mirror_d} | unrelated {unrelated}</button>
+<button onclick={() => unrelated++}>unrelated++</button>
+<button onclick={() => queued.shift()?.()}>resolve</button>
+
+{#if count}
+	<!-- inside if block so effects are newly created and therefore added to batch.#new_effects -->
+	<!-- first $effect creates new batch ... -->
+	{(() => {
+		$effect(() => {
+			count_mirror = count;
+			untrack(() => count_mirror_d); // execute derived; should associate value with the right batch
+		})
+	})()}
+	<!-- ... which second $effect shouldn't write to because the derived execution belongs to the previous batch -->
+	{(() => {
+		$effect(() => {
+			console.log(double);
+		})
+	})()}
+{/if}