fix: unset context on stale promises (#16935)

* fix: unset context on stale promises

When a stale promise is rejected in `async_derived`, and the promise eventually resolves, `d.resolve` will be noop and `d.promise.then(handler, ...)` will never run. That in turns means any restored context (via `(await save(..))()`) will never be unset. We have to handle this case and unset the context to prevent errors such as false-positive state mutation errors

* fix: unset context on stale promises (slightly different approach) (#16936)

* slightly different approach to #16935

* move unset_context call

* get rid of logs

---------

Co-authored-by: Rich Harris <[email protected]>

Author: dummdidumm

.changeset/major-beans-fry.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: unset context on stale promises

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

@@ -126,9 +126,11 @@ export function async_derived(fn, location) {
 		try {
 			// If this code is changed at some point, make sure to still access the then property
 			// of fn() to read any signals it might access, so that we track them as dependencies.
-			Promise.resolve(fn()).then(d.resolve, d.reject);
+			// We call `unset_context` to undo any `save` calls that happen inside `fn()`
+			Promise.resolve(fn()).then(d.resolve, d.reject).then(unset_context);
 		} catch (error) {
 			d.reject(error);
+			unset_context();
 		}
 
 		if (DEV) current_async_effect = null;
@@ -185,8 +187,6 @@ export function async_derived(fn, location) {
 				boundary.update_pending_count(-1);
 				if (!pending) batch.decrement();
 			}
-
-			unset_context();
 		};
 
 		d.promise.then(handler, (e) => handler(null, e || 'unknown'));

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

@@ -0,0 +1,26 @@
+import { test } from '../../test';
+
+export default test({
+	async test({ assert, target }) {
+		// We gotta wait a bit more in this test because of the macrotasks in App.svelte
+		function macrotask(t = 3) {
+			return new Promise((r) => setTimeout(r, t));
+		}
+
+		await macrotask();
+		assert.htmlEqual(target.innerHTML, '<input> 1 | ');
+
+		const [input] = target.querySelectorAll('input');
+
+		input.value = '1';
+		input.dispatchEvent(new Event('input', { bubbles: true }));
+		await macrotask();
+		assert.htmlEqual(target.innerHTML, '<input> 1 | ');
+
+		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');
+	}
+});

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

@@ -0,0 +1,34 @@
+<script>
+	let count = $state(0);
+	let value = $state('');
+	let prev;
+
+	function asd(v) {
+		const r = Promise.withResolvers();
+
+		if (prev || v === '') {
+			Promise.resolve().then(async () => {
+				count++;
+				r.resolve(v);
+				await new Promise(r => setTimeout(r, 0));
+				// TODO with a microtask like below it still throws a mutation error
+				// await Promise.resolve();
+				prev?.resolve();
+			});
+		} else {
+			prev = Promise.withResolvers();
+			prev.promise.then(() => {
+				count++;
+				r.resolve(v)
+			});
+		}
+
+		return r.promise;
+	}
+
+	const x = $derived(await asd(value))
+</script>
+
+<input bind:value />
+
+{count} | {x}