fix: don't rerun async effects unnecessarily (#16944)

Since #16866, when an async effect runs multiple times, we rebase older batches and rerun those effects. This can have unintended consequences: In a case where an async effect only depends on a single source, and that single source was updated in a later batch, we know that we don't need to / should not rerun the older batch.

This PR makes it so: We collect all the sources of older batches that are not part of the current batch that just committed, and then only mark those async effects as dirty which depend on one of those other sources. Fixes the bug I noticed while working on #16935

Author: dummdidumm

.changeset/wild-mirrors-take.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: don't rerun async effects unnecessarily

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

@@ -1,4 +1,4 @@
-/** @import { Derived, Effect, Source, Value } from '#client' */
+/** @import { Derived, Effect, Reaction, Source, Value } from '#client' */
 import {
 	BLOCK_EFFECT,
 	BRANCH_EFFECT,
@@ -342,31 +342,46 @@ export class Batch {
 					continue;
 				}
 
+				/** @type {Source[]} */
+				const sources = [];
+
 				for (const [source, value] of this.current) {
 					if (batch.current.has(source)) {
-						if (is_earlier) {
+						if (is_earlier && value !== batch.current.get(source)) {
 							// bring the value up to date
 							batch.current.set(source, value);
 						} else {
-							// later batch has more recent value,
+							// same value or later batch has more recent value,
 							// no need to re-run these effects
 							continue;
 						}
 					}
 
-					mark_effects(source);
+					sources.push(source);
 				}
 
-				if (queued_root_effects.length > 0) {
-					current_batch = batch;
-					batch.apply();
+				if (sources.length === 0) {
+					continue;
+				}
 
-					for (const root of queued_root_effects) {
-						batch.#traverse_effect_tree(root);
+				// Re-run async/block effects that depend on distinct values changed in both batches
+				const others = [...batch.current.keys()].filter((s) => !this.current.has(s));
+				if (others.length > 0) {
+					for (const source of sources) {
+						mark_effects(source, others);
 					}
 
-					queued_root_effects = [];
-					batch.deactivate();
+					if (queued_root_effects.length > 0) {
+						current_batch = batch;
+						batch.apply();
+
+						for (const root of queued_root_effects) {
+							batch.#traverse_effect_tree(root);
+						}
+
+						queued_root_effects = [];
+						batch.deactivate();
+					}
 				}
 			}
 
@@ -621,24 +636,46 @@ function flush_queued_effects(effects) {
 
 /**
  * This is similar to `mark_reactions`, but it only marks async/block effects
- * so that these can re-run after another batch has been committed
+ * depending on `value` and at least one of the other `sources`, so that
+ * these effects can re-run after another batch has been committed
  * @param {Value} value
+ * @param {Source[]} sources
  */
-function mark_effects(value) {
+function mark_effects(value, sources) {
 	if (value.reactions !== null) {
 		for (const reaction of value.reactions) {
 			const flags = reaction.f;
 
 			if ((flags & DERIVED) !== 0) {
-				mark_effects(/** @type {Derived} */ (reaction));
-			} else if ((flags & (ASYNC | BLOCK_EFFECT)) !== 0) {
+				mark_effects(/** @type {Derived} */ (reaction), sources);
+			} else if ((flags & (ASYNC | BLOCK_EFFECT)) !== 0 && depends_on(reaction, sources)) {
 				set_signal_status(reaction, DIRTY);
 				schedule_effect(/** @type {Effect} */ (reaction));
 			}
 		}
 	}
 }
 
+/**
+ * @param {Reaction} reaction
+ * @param {Source[]} sources
+ */
+function depends_on(reaction, sources) {
+	if (reaction.deps !== null) {
+		for (const dep of reaction.deps) {
+			if (sources.includes(dep)) {
+				return true;
+			}
+
+			if ((dep.f & DERIVED) !== 0 && depends_on(/** @type {Derived} */ (dep), sources)) {
+				return true;
+			}
+		}
+	}
+
+	return false;
+}
+
 /**
  * @param {Effect} signal
  * @returns {void}

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

@@ -171,6 +171,13 @@ export function async_derived(fn, location) {
 
 				internal_set(signal, value);
 
+				// All prior async derived runs are now stale
+				for (const [b, d] of deferreds) {
+					deferreds.delete(b);
+					if (b === batch) break;
+					d.reject(STALE_REACTION);
+				}
+
 				if (DEV && location !== undefined) {
 					recent_async_deriveds.add(signal);
 

packages/svelte/tests/runtime-runes/samples/async-resolve-stale/_config.js

@@ -20,7 +20,6 @@ export default test({
 		input.value = '12';
 		input.dispatchEvent(new Event('input', { bubbles: true }));
 		await macrotask(6);
-		// TODO this is wrong (separate bug), this should be 3 | 12
-		assert.htmlEqual(target.innerHTML, '<input> 5 | 12');
+		assert.htmlEqual(target.innerHTML, '<input> 3 | 12');
 	}
 });