more fixes

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

@@ -130,6 +130,14 @@ export class Batch {
 	/** @type {Map<Effect, number>} */
 	stale_effects = new Map();
 
+	/**
+	 * Effects that, while running in an earlier batch, read a value that this batch holds
+	 * a newer version of, and that are therefore scheduled to re-run in this batch. Used to
+	 * avoid scheduling the same effect multiple times when it reads more than one such value.
+	 * @type {Set<Effect>}
+	 */
+	stale_readers = new Set();
+
 	/** @type {Set<Effect>} */
 	seen_effects = new Set();
 
@@ -160,14 +168,6 @@ export class Batch {
 	 */
 	previous = new Map();
 
-	/**
-	 * TODO run this on fork commit, put in all the effects we have decided we need to rerun,
-	 * it's a map so that we can delete entries if later batches have runs in it; though
-	 * how do we know that this no longer counts for batch 3 but still for batch 2?
-	 * @type {Map<Effect, () => void>}
-	 */
-	on_fork_commit = new Map();
-
 	/**
 	 * When the batch is committed (and the DOM is updated), we need to remove old branches
 	 * and append new ones by calling the functions added inside (if/each/key/etc) blocks
@@ -772,6 +772,10 @@ export class Batch {
 			if (d) deferred.promise.then(d.resolve).catch(d.reject);
 		}
 
+		for (const b of batch.dependent) {
+			if (b !== this) this.dependent.add(b);
+		}
+
 		for (const c of batch.#commit_callbacks) {
 			this.oncommit(() => c(batch));
 		}
@@ -1880,6 +1884,7 @@ export function fork(fn) {
 				// TODO we need to ensure that the version bumps happen "in order", e.g. in case of source1->derived2 we need to bump S last
 				// }
 
+				var changed = source.v !== content[0];
 				source.v = content[0];
 
 				if (!content[1]) {
@@ -1889,6 +1894,13 @@ export function fork(fn) {
 					// batch.mark(source, ...) TODO re-maybe-dirty- everything?
 					// dirty those effects the fork did not see yet, e.g. because a later batch created new branches
 					batch.mark(source, MAYBE_DIRTY, true);
+				} else if (changed) {
+					// A derived that was evaluated inside the fork: bump its version too, so that reactions
+					// which read the (then still old) real value _after_ the fork evaluated it — and which are
+					// therefore not in `stale_effects` — see a newer dependency version and re-run.
+					// We deliberately use a fresh version rather than the fork-time `content[2]`, because the
+					// real world may have run reactions since then that would otherwise outrank it.
+					content[2] = source.wv = increment_write_version();
 				}
 			}
 
@@ -1924,7 +1936,7 @@ export function fork(fn) {
 			batch.flush();
 
 			// Other forks might need to rerun now with the updated state.
-			// TODO reuse batch.capture() logic here (maybe we can just call it?)
+			// TODO reuse batch.capture() logic here (delete values from fork and possibly discard, depend on other batches etc) (maybe we can just call it?)
 			let next_batch = batch.next;
 			while (next_batch) {
 				for (const [source, [, is_derived]] of batch.current) {

packages/svelte/src/internal/client/runtime.js

@@ -544,21 +544,15 @@ export function update_effect(effect) {
 		if (is_latest_value) {
 			effect.wv = write_version;
 		} else {
-			// console.log('setting', effect.wv, effect, 'to', write_version, is_latest_value);
+			// The effect ran with values that are not the latest ones (it saw its own batch's view).
-			// effect.wv = write_version;
+			// Don't update its write version — instead remember it so that the batch can bring it
-			// set_signal_status(effect, MAYBE_DIRTY);
+			// up to date on commit, and tell all subsequent batches that it may need to re-run in their view.
-			// debugger;
+			/** @type {Batch} */ (own_batch).stale_effects.set(effect, write_version);
-			if (!is_latest_value) {
+			var batch = /** @type {Batch} */ (own_batch).next;
-				/** @type {Batch} */ (own_batch).stale_effects.set(effect, write_version);
+			while (batch) {
-				var batch = /** @type {Batch} */ (own_batch).next;
+				batch.maybe_dirty_effects.add(effect);
-				while (batch) {
+				batch = batch.next;
-					batch.maybe_dirty_effects.add(effect);
-					batch = batch.next;
-				}
 			}
-
-			// TODO add to maybe_dirty_effects in all subsequent batches here,
-			// removing need for other cross-batch rerun mechanisms / remove need for adding blocks to maybe_dirty?
 		}
 
 		// In DEV, increment versions of any sources that were written to during the effect,
@@ -646,12 +640,17 @@ export function get(signal) {
 					// rather than updating `new_deps`, which creates GC cost
 					if (new_deps === null && deps !== null && deps[skipped_deps] === signal) {
 						skipped_deps++;
-					} else if (new_deps === null) {
-						new_deps = [signal];
-						first_time = true;
 					} else {
-						new_deps.push(signal);
+						if (new_deps === null) {
-						first_time = true;
+							new_deps = [signal];
+						} else {
+							new_deps.push(signal);
+						}
+
+						// Only a signal that wasn't a dependency of this reaction before counts as new —
+						// reading existing dependencies in a different order must not (it would make
+						// the reaction see the latest value instead of its batch's view, see below)
+						first_time = deps === null || !includes.call(deps, signal);
 					}
 				}
 			} else {
@@ -793,19 +792,26 @@ export function get(signal) {
 		if (batch) {
 			if (!current.is_eager) batch.dependent.add(current);
 			// TODO do we only need this for async/block effects?
-			if (active_effect && (is_updating_effect || active_effect.f & ASYNC)) {
+			if (
+				active_effect &&
+				(is_updating_effect || active_effect.f & ASYNC) &&
+				// an effect can read several stale values in one run — only schedule the re-run once
+				!batch.stale_readers.has(active_effect)
+			) {
 				const effect = active_effect;
-				// TODO can overfire when two stale reads within one effect, because no "already scheduled this" logic.
+				batch.stale_readers.add(effect);
-				// TODO how to know "ok we already did this now"
+
 				if (current.is_eager) {
 					// TODO only do this if we can see that the batch doesn't have this already scheduled in (maybe)dirty effects.
 					batch.oncommit(() => {
+						batch.stale_readers.delete(effect);
 						const b = Batch.ensure();
 						set_signal_status(effect, DIRTY);
 						b.schedule(effect);
 					});
 				} else {
 					queue_micro_task(() => {
+						batch.stale_readers.delete(effect);
 						set_signal_status(effect, DIRTY);
 						batch.schedule(effect);
 						batch.flush();

packages/svelte/tests/runtime-runes/samples/async-fork-commit-derived-new-reader/_config.js

@@ -0,0 +1,21 @@
+import { tick } from 'svelte';
+import { test } from '../../test';
+
+const buttons = `<button>fork</button><button>show</button><button>commit</button>`;
+
+export default test({
+	async test({ assert, target }) {
+		const [fork, show, commit] = target.querySelectorAll('button');
+
+		fork.click();
+		await tick();
+
+		show.click();
+		await tick();
+		assert.htmlEqual(target.innerHTML, `${buttons}<p>0</p>`);
+
+		commit.click();
+		await tick();
+		assert.htmlEqual(target.innerHTML, `${buttons}<span>two</span><p>2</p>`);
+	}
+});

packages/svelte/tests/runtime-runes/samples/async-fork-commit-derived-new-reader/main.svelte

@@ -0,0 +1,17 @@
+<script>
+	import { fork } from 'svelte';
+
+	let s1 = $state(0);
+	let show = $state(false);
+	let d2 = $derived(s1 * 2);
+	let f;
+</script>
+
+<button onclick={() => (f = fork(() => (s1 = 1)))}>fork</button>
+<button onclick={() => (show = true)}>show</button>
+<button onclick={() => f.commit()}>commit</button>
+
+{#if d2 === 2}<span>two</span>{/if}
+{#if show}
+	<p>{d2}</p>
+{/if}

packages/svelte/tests/runtime-runes/samples/async-merge-transitive-dependency/Child.svelte

@@ -0,0 +1,8 @@
+<script>
+	import { untrack } from 'svelte';
+	let { a, b, c, slow, fast } = $props();
+</script>
+
+<p>{await slow(a)}</p>
+<p>{await slow(b)}</p>
+<p>{await fast(`${c}:${untrack(() => `${a}${b}`)}`)}</p>

packages/svelte/tests/runtime-runes/samples/async-merge-transitive-dependency/_config.js

@@ -0,0 +1,43 @@
+import { tick } from 'svelte';
+import { test } from '../../test';
+
+const buttons = `
+	<button>a</button>
+	<button>b</button>
+	<button>c</button>
+	<button>resolve</button>
+	<button>resolve last</button>
+`;
+
+// B3 reads values held by both B1 and B2 and therefore depends on both. When B3 merges
+// into B2 (the nearest earlier related batch), B2 must inherit B3's dependency on B1,
+// otherwise B2+B3 commit before B1 and leak B1's uncommitted value into the DOM
+export default test({
+	async test({ assert, target }) {
+		const [a, b, c, resolve, resolve_last] = target.querySelectorAll('button');
+
+		resolve.click();
+		await tick();
+		resolve.click();
+		await tick();
+		assert.htmlEqual(target.innerHTML, `${buttons}<p>0</p><p>0</p><p>0:00</p>`);
+
+		a.click(); // B1: slow(a=1) pending
+		await tick();
+		b.click(); // B2: slow(b=1) pending
+		await tick();
+		c.click(); // B3: reads a=1 (held by B1) and b=1 (held by B2) -> depends on both, merges into B2
+		await tick();
+		assert.htmlEqual(target.innerHTML, `${buttons}<p>0</p><p>0</p><p>0:00</p>`);
+
+		// B2 finishes first: it must still wait for B1
+		resolve_last.click();
+		await tick();
+		assert.htmlEqual(target.innerHTML, `${buttons}<p>0</p><p>0</p><p>0:00</p>`);
+
+		// B1 finishes: everything commits together
+		resolve.click();
+		await tick();
+		assert.htmlEqual(target.innerHTML, `${buttons}<p>1</p><p>1</p><p>1:11</p>`);
+	}
+});

packages/svelte/tests/runtime-runes/samples/async-merge-transitive-dependency/main.svelte

@@ -0,0 +1,25 @@
+<script>
+	import Child from './Child.svelte';
+	let a = $state(0);
+	let b = $state(0);
+	let c = $state(0);
+
+	const resolvers = [];
+	function slow(v) {
+		return new Promise((r) => resolvers.push(() => r(v)));
+	}
+	function fast(v) {
+		return Promise.resolve(v);
+	}
+</script>
+
+<button onclick={() => a++}>a</button>
+<button onclick={() => b++}>b</button>
+<button onclick={() => c++}>c</button>
+<button onclick={() => resolvers.shift()?.()}>resolve</button>
+<button onclick={() => resolvers.pop()?.()}>resolve last</button>
+
+<svelte:boundary>
+	{#snippet pending()}loading{/snippet}
+	<Child {a} {b} {c} {slow} {fast} />
+</svelte:boundary>

packages/svelte/tests/runtime-runes/samples/async-reordered-deps-not-new-reader/_config.js

@@ -0,0 +1,40 @@
+import { tick } from 'svelte';
+import { test } from '../../test';
+
+const buttons = `
+	<button>flip</button>
+	<button>a</button>
+	<button>b</button>
+	<button>resolve</button>
+`;
+
+// A reaction that reads its existing dependencies in a different order than last time
+// is not a "new reader" of those dependencies — it must keep seeing its own batch's view,
+// not the latest (uncommitted) value of a later pending batch
+export default test({
+	async test({ assert, target }) {
+		const [flip, a, , resolve] = target.querySelectorAll('button');
+
+		resolve.click();
+		await tick();
+		resolve.click();
+		await tick();
+		assert.htmlEqual(target.innerHTML, `${buttons}<p>a0b0</p><p>true</p><p>a0</p>`);
+
+		flip.click(); // B1: cond = false, slow(cond) pending
+		await tick();
+		a.click(); // B2: a = 'a1', slow(a) pending
+		await tick();
+		assert.htmlEqual(target.innerHTML, `${buttons}<p>a0b0</p><p>true</p><p>a0</p>`);
+
+		// B1 commits while B2 is still pending. The flipped branch reads `b` then `a` (reordered
+		// deps) — `a` must still resolve to B1's view (a0), not B2's uncommitted a1
+		resolve.click();
+		await tick();
+		assert.htmlEqual(target.innerHTML, `${buttons}<p>b0a0</p><p>false</p><p>a0</p>`);
+
+		resolve.click();
+		await tick();
+		assert.htmlEqual(target.innerHTML, `${buttons}<p>b0a1</p><p>false</p><p>a1</p>`);
+	}
+});

packages/svelte/tests/runtime-runes/samples/async-reordered-deps-not-new-reader/main.svelte

@@ -0,0 +1,22 @@
+<script>
+	let cond = $state(true);
+	let a = $state('a0');
+	let b = $state('b0');
+
+	const resolvers = [];
+	function slow(v) {
+		return new Promise((r) => resolvers.push(() => r(v)));
+	}
+</script>
+
+<button onclick={() => (cond = !cond)}>flip</button>
+<button onclick={() => (a = "a1")}>a</button>
+<button onclick={() => (b = "b1")}>b</button>
+<button onclick={() => resolvers.shift()?.()}>resolve</button>
+
+<svelte:boundary>
+	{#snippet pending()}loading{/snippet}
+	<p>{cond ? a + b : b + a}</p>
+	<p>{await slow(cond)}</p>
+	<p>{await slow(a)}</p>
+</svelte:boundary>

packages/svelte/tests/runtime-runes/samples/async-stale-read-multiple-no-double-run/Child.svelte

@@ -0,0 +1,8 @@
+<script>
+	let { a, b, c, slow, track } = $props();
+	let sa = $derived(await slow(a));
+</script>
+
+<p>{sa}</p>
+<p>{await slow(b + c)}</p>
+<p>{await track(sa, b, c)}</p>

packages/svelte/tests/runtime-runes/samples/async-stale-read-multiple-no-double-run/_config.js

@@ -0,0 +1,44 @@
+import { tick } from 'svelte';
+import { test } from '../../test';
+
+const buttons = `<button>a</button><button>bc</button><button>resolve</button>`;
+
+// An effect that runs in an earlier batch and reads _several_ values that a later batch
+// holds newer versions of must only be re-run once in that later batch, not once per value
+export default test({
+	async test({ assert, target, instance }) {
+		const [a, bc, resolve] = target.querySelectorAll('button');
+
+		resolve.click();
+		await tick();
+		resolve.click();
+		await tick();
+		resolve.click();
+		await tick();
+		assert.deepEqual(instance.get_calls(), ['0/0/0']);
+		assert.htmlEqual(target.innerHTML, `${buttons}<p>0</p><p>0</p><p>0/0/0</p>`);
+
+		// B1: a++ -> `sa` pending
+		a.click();
+		await tick();
+		assert.deepEqual(instance.get_calls(), ['0/0/0']);
+
+		// B2: b++, c++ -> `slow(b + c)` pending, `track` re-runs with B2's view
+		bc.click();
+		await tick();
+		assert.deepEqual(instance.get_calls(), ['0/0/0', '0/1/1']);
+
+		// resolve `sa` in B1: `track` re-runs in B1 (reading B1's stale view of b/c) and,
+		// because it read two values held by B2, exactly once more in B2
+		resolve.click();
+		await tick();
+		assert.deepEqual(instance.get_calls(), ['0/0/0', '0/1/1', '1/0/0', '1/1/1']);
+
+		for (let i = 0; i < 6; i++) {
+			resolve.click();
+			await tick();
+		}
+		assert.deepEqual(instance.get_calls(), ['0/0/0', '0/1/1', '1/0/0', '1/1/1']);
+		assert.htmlEqual(target.innerHTML, `${buttons}<p>1</p><p>2</p><p>1/1/1</p>`);
+	}
+});

packages/svelte/tests/runtime-runes/samples/async-stale-read-multiple-no-double-run/main.svelte

@@ -0,0 +1,30 @@
+<script>
+	import Child from './Child.svelte';
+	let a = $state(0);
+	let b = $state(0);
+	let c = $state(0);
+	let calls = [];
+
+	const resolvers = [];
+	function slow(v) {
+		return new Promise((r) => resolvers.push(() => r(v)));
+	}
+
+	export function get_calls() {
+		return calls;
+	}
+
+	function track(...args) {
+		calls.push(args.join('/'));
+		return slow(args.join('/'));
+	}
+</script>
+
+<button onclick={() => a++}>a</button>
+<button onclick={() => { b++; c++; }}>bc</button>
+<button onclick={() => resolvers.shift()?.()}>resolve</button>
+
+<svelte:boundary>
+	{#snippet pending()}loading{/snippet}
+	<Child {a} {b} {c} {slow} {track} />
+</svelte:boundary>