fork fix

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

@@ -27,7 +27,8 @@ import {
 	get,
 	increment_write_version,
 	is_dirty,
-	update_effect
+	update_effect,
+	write_version
 } from '../runtime.js';
 import * as e from '../errors.js';
 import { flush_tasks, queue_micro_task } from '../dom/task.js';
@@ -259,7 +260,7 @@ export class Batch {
 	 * true indicates that this branch is new to the eyes of this fork but was already created before.
 	 * @type {Map<Effect, boolean>}
 	 */
-	#unskipped_branches = new Map();
+	unskipped_branches = new Map();
 
 	is_fork = false;
 
@@ -332,7 +333,7 @@ export class Batch {
 		if (!this.#skipped_branches.has(effect)) {
 			this.#skipped_branches.set(effect, { d: [], m: [] });
 		}
-		this.#unskipped_branches.delete(effect);
+		this.unskipped_branches.delete(effect);
 	}
 
 	/**
@@ -357,7 +358,7 @@ export class Batch {
 				callback(e);
 			}
 		}
-		if (!this.#unskipped_branches.has(effect)) this.#unskipped_branches.set(effect, is_fork_init);
+		if (!this.unskipped_branches.has(effect)) this.unskipped_branches.set(effect, is_fork_init);
 	}
 
 	/**
@@ -445,14 +446,6 @@ export class Batch {
 			set_signal_status(d, MAYBE_DIRTY);
 		}
 
-		if (!this.is_fork) {
-			for (const e of this.#unskipped_branches.keys()) {
-				if (e.f & FORK_ONLY_BRANCH) {
-					e.f ^= FORK_ONLY_BRANCH;
-				}
-			}
-		}
-
 		// An earlier batch might have created new branches which contain effects that we need
 		// to mark as dirty to also execute them.
 		// TODO does this make the similar logic in fork.commit below obsolete?
@@ -619,7 +612,7 @@ export class Batch {
 			var skip = is_skippable_branch || (flags & INERT) !== 0 || this.#skipped_branches.has(effect);
 
 			if ((flags & FORK_ONLY_BRANCH) !== 0) {
-				var first_time = this.#unskipped_branches.get(effect);
+				var first_time = this.unskipped_branches.get(effect);
 
 				if (first_time === undefined) {
 					skip = true;
@@ -632,7 +625,7 @@ export class Batch {
 					// We're seeing a fork-only branch for the first time in another fork. We need to traverse
 					// all effects inside it (they're all marked MAYBE_DIRTY). This is necessary because
 					// dependencies of the effects inside could've updated since the last time this branch ran.
-					this.#unskipped_branches.set(effect, false);
+					this.unskipped_branches.set(effect, false);
 					all_dirty ??= effect;
 					if (effect.f & CLEAN) effect.f ^= CLEAN;
 					skip = false;
@@ -789,10 +782,10 @@ export class Batch {
 
 		for (const [s, v] of batch.#skipped_branches) {
 			this.#skipped_branches.set(s, v);
-			this.#unskipped_branches.delete(s);
+			this.unskipped_branches.delete(s);
 		}
 
-		for (const s of batch.#unskipped_branches.keys()) {
+		for (const s of batch.unskipped_branches.keys()) {
 			const v = this.#skipped_branches.get(s);
 			// TODO i do wonder at this point if it's less code / easier / more robust to do what mark() below does
 			// instead and just rerun all the block effects. Though it will certainly overrun some blocks, potentially
@@ -1155,7 +1148,7 @@ export class Batch {
 
 				// A batch was unskipped in a later batch -> tell prior batches to unskip it, too
 				if (is_earlier) {
-					for (const unskipped of this.#unskipped_branches) {
+					for (const unskipped of this.unskipped_branches) {
 						batch.unskip_effect(unskipped, (e) => {
 							if ((e.f & (BLOCK_EFFECT | ASYNC)) !== 0) {
 								batch.schedule(e);
@@ -1868,10 +1861,12 @@ export function fork(fn) {
 				batch.id = prev.id;
 				prev.id = id;
 				prev.next = batch.next;
+				if (prev.next) prev.next.prev = prev;
+				else last_batch = prev;
 				batch.prev = prev.prev;
-				if (prev.prev) {
-					prev.prev.next = batch;
-				}
+				if (batch.prev) batch.prev.next = batch;
+				else first_batch = batch;
+				batch.next = prev;
 				prev.prev = batch;
 			}
 
@@ -1897,6 +1892,11 @@ export function fork(fn) {
 				}
 			}
 
+			for (const effect of batch.stale_effects.keys()) {
+				effect.wv = effect.wv > write_version ? effect.wv : write_version;
+			}
+			batch.stale_effects.clear();
+
 			// trigger any `$state.eager(...)` expressions with the new state.
 			// eager effects don't get scheduled like other effects, so we
 			// can't just encounter them during traversal, we need to
@@ -1914,6 +1914,16 @@ export function fork(fn) {
 				flush_eager_effects();
 			});
 
+			// Promote fork-only branches to the real world
+			for (const e of batch.unskipped_branches.keys()) {
+				if (e.f & FORK_ONLY_BRANCH) {
+					e.f ^= FORK_ONLY_BRANCH;
+				}
+			}
+
+			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?)
 			let next_batch = batch.next;
 			while (next_batch) {
@@ -1947,7 +1957,6 @@ export function fork(fn) {
 			// for (const run of batch.on_fork_commit.values()) {
 			// 	run();
 			// }
-			batch.flush();
 			await settled;
 		},
 		discard: () => {

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

@@ -798,6 +798,7 @@ export function get(signal) {
 				// TODO can overfire when two stale reads within one effect, because no "already scheduled this" logic.
 				// 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(() => {
 						const b = Batch.ensure();
 						set_signal_status(effect, DIRTY);

packages/svelte/tests/runtime-runes/samples/async-read-pending-value-stale-reader/main.svelte

@@ -0,0 +1,49 @@
+<script>
+	let a = $state(0);
+	let show1 = $state(false);
+	let show2 = $state(false);
+
+	let deferreds = [];
+
+	function push(key, v) {
+		const d = Promise.withResolvers();
+		deferreds.push({ key, v, d });
+		return d.promise;
+	}
+
+	function shift(key, override) {
+		const i = deferreds.findIndex((d) => d.key === key);
+		if (i === -1) return;
+		const [{ v, d }] = deferreds.splice(i, 1);
+		d.resolve(override ?? v);
+	}
+</script>
+
+<button onclick={() => a++}>up</button>
+<button onclick={() => a--}>down</button>
+<button onclick={() => (show1 = true)}>show1</button>
+<button onclick={() => (show2 = true)}>show2</button>
+<button onclick={() => shift('a')}>shift a</button>
+<button onclick={() => shift('t', 1)}>shift t</button>
+
+{#if show1}
+	<svelte:boundary>
+		<!-- no state deps initially; reads `a` for the first time only when
+		     the awaited value (controlled by the test) says so -->
+		<p>late read: {(await push('t', 0)) > 0 ? a : -1}</p>
+
+		{#snippet pending()}
+			<p>loading 1...</p>
+		{/snippet}
+	</svelte:boundary>
+{/if}
+
+{#if show2}
+	<svelte:boundary>
+		<p>async a: {await push('a', a)}</p>
+
+		{#snippet pending()}
+			<p>loading 2...</p>
+		{/snippet}
+	</svelte:boundary>
+{/if}

packages/svelte/tests/runtime-runes/samples/async-state-new-branch-all-combinations/_config.js

@@ -2,7 +2,6 @@ import { tick } from 'svelte';
 import { test } from '../../test';
 
 export default test({
-	skip: true, // TODO more combinations pass on https://github.com/sveltejs/svelte/pull/17971
 	timeout: 20_000,
 	async test({ assert, target }) {
 		const [x, fork_x, y, fork_y, shift, pop, commit_x, commit_y, reset] =