fix: run batch until complete (#16971)

 at present we only call batch.increment() when something async happens if we're not inside a pending boundary, but that's incorrect — it means that a batch is committed before everything resolves. When work inside a pending boundary does resolve, the batch becomes a zombie.

At the same time, we don't handle effects inside pending boundaries correctly. They should be deferred until the boundary (and all its parents) are ready.

This PR attempts to fix that — during traversal, when we exit a pending boundary, any effects that were collected get deferred until the next flush. We also distinguish between batch.#pending (any ongoing async work) and batch.#blocking_pending (any async work that should prevent effects outside pending boundaries from being flushed).

Author: Rich-Harris

.changeset/mighty-mice-call.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: keep batches alive until all async work is complete

packages/svelte/src/internal/client/dom/blocks/async.js

@@ -1,5 +1,6 @@
 /** @import { TemplateNode, Value } from '#client' */
 import { flatten } from '../../reactivity/async.js';
+import { Batch, current_batch } from '../../reactivity/batch.js';
 import { get } from '../../runtime.js';
 import {
 	hydrate_next,
@@ -18,8 +19,11 @@ import { get_boundary } from './boundary.js';
  */
 export function async(node, expressions, fn) {
 	var boundary = get_boundary();
+	var batch = /** @type {Batch} */ (current_batch);
+	var blocking = !boundary.is_pending();
 
 	boundary.update_pending_count(1);
+	batch.increment(blocking);
 
 	var was_hydrating = hydrating;
 
@@ -44,6 +48,7 @@ export function async(node, expressions, fn) {
 			fn(node, ...values);
 		} finally {
 			boundary.update_pending_count(-1);
+			batch.decrement(blocking);
 		}
 
 		if (was_hydrating) {

packages/svelte/src/internal/client/dom/blocks/boundary.js

@@ -291,13 +291,6 @@ export class Boundary {
 				this.#anchor.before(this.#offscreen_fragment);
 				this.#offscreen_fragment = null;
 			}
-
-			// TODO this feels like a little bit of a kludge, but until we
-			// overhaul the boundary/batch relationship it's probably
-			// the most pragmatic solution available to us
-			queue_micro_task(() => {
-				Batch.ensure().flush();
-			});
 		}
 	}
 

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

@@ -218,10 +218,10 @@ export function unset_context() {
 export async function async_body(anchor, fn) {
 	var boundary = get_boundary();
 	var batch = /** @type {Batch} */ (current_batch);
-	var pending = boundary.is_pending();
+	var blocking = !boundary.is_pending();
 
 	boundary.update_pending_count(1);
-	if (!pending) batch.increment();
+	batch.increment(blocking);
 
 	var active = /** @type {Effect} */ (active_effect);
 
@@ -254,12 +254,7 @@ export async function async_body(anchor, fn) {
 		}
 
 		boundary.update_pending_count(-1);
-
-		if (pending) {
-			batch.flush();
-		} else {
-			batch.decrement();
-		}
+		batch.decrement(blocking);
 
 		unset_context();
 	}

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

@@ -11,7 +11,8 @@ import {
 	RENDER_EFFECT,
 	ROOT_EFFECT,
 	MAYBE_DIRTY,
-	DERIVED
+	DERIVED,
+	BOUNDARY_EFFECT
 } from '#client/constants';
 import { async_mode_flag } from '../../flags/index.js';
 import { deferred, define_property } from '../../shared/utils.js';
@@ -31,6 +32,16 @@ import { invoke_error_boundary } from '../error-handling.js';
 import { old_values, source, update } from './sources.js';
 import { inspect_effect, unlink_effect } from './effects.js';
 
+/**
+ * @typedef {{
+ *   parent: EffectTarget | null;
+ *   effect: Effect | null;
+ *   effects: Effect[];
+ *   render_effects: Effect[];
+ *   block_effects: Effect[];
+ * }} EffectTarget
+ */
+
 /** @type {Set<Batch>} */
 const batches = new Set();
 
@@ -65,6 +76,8 @@ let is_flushing = false;
 export let is_flushing_sync = false;
 
 export class Batch {
+	committed = false;
+
 	/**
 	 * The current values of any sources that are updated in this batch
 	 * They keys of this map are identical to `this.#previous`
@@ -91,33 +104,18 @@ export class Batch {
 	 */
 	#pending = 0;
 
+	/**
+	 * The number of async effects that are currently in flight, _not_ inside a pending boundary
+	 */
+	#blocking_pending = 0;
+
 	/**
 	 * A deferred that resolves when the batch is committed, used with `settled()`
 	 * TODO replace with Promise.withResolvers once supported widely enough
 	 * @type {{ promise: Promise<void>, resolve: (value?: any) => void, reject: (reason: unknown) => void } | null}
 	 */
 	#deferred = null;
 
-	/**
-	 * Template effects and `$effect.pre` effects, which run when
-	 * a batch is committed
-	 * @type {Effect[]}
-	 */
-	#render_effects = [];
-
-	/**
-	 * The same as `#render_effects`, but for `$effect` (which runs after)
-	 * @type {Effect[]}
-	 */
-	#effects = [];
-
-	/**
-	 * Block effects, which may need to re-run on subsequent flushes
-	 * in order to update internal sources (e.g. each block items)
-	 * @type {Effect[]}
-	 */
-	#block_effects = [];
-
 	/**
 	 * Deferred effects (which run after async work has completed) that are DIRTY
 	 * @type {Effect[]}
@@ -148,41 +146,37 @@ export class Batch {
 
 		this.apply();
 
+		/** @type {EffectTarget} */
+		var target = {
+			parent: null,
+			effect: null,
+			effects: [],
+			render_effects: [],
+			block_effects: []
+		};
+
 		for (const root of root_effects) {
-			this.#traverse_effect_tree(root);
+			this.#traverse_effect_tree(root, target);
 		}
 
-		// if there is no outstanding async work, commit
-		if (this.#pending === 0) {
-			// TODO we need this because we commit _then_ flush effects...
-			// maybe there's a way we can reverse the order?
-			var previous_batch_sources = batch_values;
+		this.#resolve();
 
-			this.#commit();
-
-			var render_effects = this.#render_effects;
-			var effects = this.#effects;
-
-			this.#render_effects = [];
-			this.#effects = [];
-			this.#block_effects = [];
+		if (this.#blocking_pending > 0) {
+			this.#defer_effects(target.effects);
+			this.#defer_effects(target.render_effects);
+			this.#defer_effects(target.block_effects);
+		} else {
+			// TODO append/detach blocks here, not in #commit
 
 			// If sources are written to, then work needs to happen in a separate batch, else prior sources would be mixed with
 			// newly updated sources, which could lead to infinite loops when effects run over and over again.
 			previous_batch = this;
 			current_batch = null;
 
-			batch_values = previous_batch_sources;
-			flush_queued_effects(render_effects);
-			flush_queued_effects(effects);
+			flush_queued_effects(target.render_effects);
+			flush_queued_effects(target.effects);
 
 			previous_batch = null;
-
-			this.#deferred?.resolve();
-		} else {
-			this.#defer_effects(this.#render_effects);
-			this.#defer_effects(this.#effects);
-			this.#defer_effects(this.#block_effects);
 		}
 
 		batch_values = null;
@@ -192,8 +186,9 @@ export class Batch {
 	 * Traverse the effect tree, executing effects or stashing
 	 * them for later execution as appropriate
 	 * @param {Effect} root
+	 * @param {EffectTarget} target
 	 */
-	#traverse_effect_tree(root) {
+	#traverse_effect_tree(root, target) {
 		root.f ^= CLEAN;
 
 		var effect = root.first;
@@ -205,15 +200,25 @@ export class Batch {
 
 			var skip = is_skippable_branch || (flags & INERT) !== 0 || this.skipped_effects.has(effect);
 
+			if ((effect.f & BOUNDARY_EFFECT) !== 0 && effect.b?.is_pending()) {
+				target = {
+					parent: target,
+					effect,
+					effects: [],
+					render_effects: [],
+					block_effects: []
+				};
+			}
+
 			if (!skip && effect.fn !== null) {
 				if (is_branch) {
 					effect.f ^= CLEAN;
 				} else if ((flags & EFFECT) !== 0) {
-					this.#effects.push(effect);
+					target.effects.push(effect);
 				} else if (async_mode_flag && (flags & RENDER_EFFECT) !== 0) {
-					this.#render_effects.push(effect);
+					target.render_effects.push(effect);
 				} else if (is_dirty(effect)) {
-					if ((effect.f & BLOCK_EFFECT) !== 0) this.#block_effects.push(effect);
+					if ((effect.f & BLOCK_EFFECT) !== 0) target.block_effects.push(effect);
 					update_effect(effect);
 				}
 
@@ -229,6 +234,17 @@ export class Batch {
 			effect = effect.next;
 
 			while (effect === null && parent !== null) {
+				if (parent === target.effect) {
+					// TODO rather than traversing into pending boundaries and deferring the effects,
+					// could we just attach the effects _to_ the pending boundary and schedule them
+					// once the boundary is ready?
+					this.#defer_effects(target.effects);
+					this.#defer_effects(target.render_effects);
+					this.#defer_effects(target.block_effects);
+
+					target = /** @type {EffectTarget} */ (target.parent);
+				}
+
 				effect = parent.next;
 				parent = parent.parent;
 			}
@@ -246,8 +262,6 @@ export class Batch {
 			// mark as clean so they get scheduled if they depend on pending async state
 			set_signal_status(e, CLEAN);
 		}
-
-		effects.length = 0;
 	}
 
 	/**
@@ -283,8 +297,8 @@ export class Batch {
 				// this can happen if a new batch was created during `flush_effects()`
 				return;
 			}
-		} else if (this.#pending === 0) {
-			this.#commit();
+		} else {
+			this.#resolve();
 		}
 
 		this.deactivate();
@@ -300,24 +314,37 @@ export class Batch {
 		}
 	}
 
-	/**
-	 * Append and remove branches to/from the DOM
-	 */
-	#commit() {
-		for (const fn of this.#callbacks) {
-			fn();
+	#resolve() {
+		if (this.#blocking_pending === 0) {
+			// append/remove branches
+			for (const fn of this.#callbacks) fn();
+			this.#callbacks.clear();
 		}
 
-		this.#callbacks.clear();
+		if (this.#pending === 0) {
+			this.#commit();
+		}
+	}
 
+	#commit() {
 		// If there are other pending batches, they now need to be 'rebased' —
 		// in other words, we re-run block/async effects with the newly
 		// committed state, unless the batch in question has a more
 		// recent value for a given source
 		if (batches.size > 1) {
 			this.#previous.clear();
 
-			let is_earlier = true;
+			var previous_batch_values = batch_values;
+			var is_earlier = true;
+
+			/** @type {EffectTarget} */
+			var dummy_target = {
+				parent: null,
+				effect: null,
+				effects: [],
+				render_effects: [],
+				block_effects: []
+			};
 
 			for (const batch of batches) {
 				if (batch === this) {
@@ -359,27 +386,43 @@ export class Batch {
 						batch.apply();
 
 						for (const root of queued_root_effects) {
-							batch.#traverse_effect_tree(root);
+							batch.#traverse_effect_tree(root, dummy_target);
 						}
 
+						// TODO do we need to do anything with `target`? defer block effects?
+
 						queued_root_effects = [];
 						batch.deactivate();
 					}
 				}
 			}
 
 			current_batch = null;
+			batch_values = previous_batch_values;
 		}
 
+		this.committed = true;
 		batches.delete(this);
+
+		this.#deferred?.resolve();
 	}
 
-	increment() {
+	/**
+	 *
+	 * @param {boolean} blocking
+	 */
+	increment(blocking) {
 		this.#pending += 1;
+		if (blocking) this.#blocking_pending += 1;
 	}
 
-	decrement() {
+	/**
+	 *
+	 * @param {boolean} blocking
+	 */
+	decrement(blocking) {
 		this.#pending -= 1;
+		if (blocking) this.#blocking_pending -= 1;
 
 		for (const e of this.#dirty_effects) {
 			set_signal_status(e, DIRTY);
@@ -391,6 +434,9 @@ export class Batch {
 			schedule_effect(e);
 		}
 
+		this.#dirty_effects = [];
+		this.#maybe_dirty_effects = [];
+
 		this.flush();
 	}