fix: don't rerun async effects unnecessarily

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,
@@ -335,31 +335,43 @@ 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;
-					const revert = Batch.apply(batch);
-
-					for (const root of queued_root_effects) {
-						batch.#traverse_effect_tree(root);
+				// Only reschedule an async effect if it depends on something else than
+				// the sources that were updated in this batch and that something else changed.
+				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 = [];
-					revert();
+					if (queued_root_effects.length > 0) {
+						current_batch = batch;
+						const revert = Batch.apply(batch);
+
+						for (const root of queued_root_effects) {
+							batch.#traverse_effect_tree(root);
+						}
+
+						queued_root_effects = [];
+						revert();
+					}
 				}
 			}
 
@@ -640,17 +652,34 @@ 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 one of the 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) {
+		/** @param {Reaction} reaction */
+		const depends_on_sources = (reaction) => {
+			if (reaction.deps !== null) {
+				for (const dep of reaction.deps) {
+					if (sources.includes(dep)) {
+						return true;
+					}
+					if ((dep.f & DERIVED) !== 0 && depends_on_sources(/** @type {Derived} */ (dep))) {
+						return true;
+					}
+				}
+			}
+			return false;
+		};
+
 		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_sources(reaction)) {
 				set_signal_status(reaction, DIRTY);
 				schedule_effect(/** @type {Effect} */ (reaction));
 			}

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');
 	}
 });