From 41ede2cbfcf2e1b4aa01b4e582954d6a74ce523c Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Thu, 7 May 2026 21:58:35 +0200 Subject: [PATCH] fix: reschedule effect + avoid stale values This fixes the failing redirect test in SvelteKit. It consists of two parts, both needed: - reschedule new effects: when a batch commits, it needs to tell other batches about its new effects. We had this logic already but didn't apply it often enough. To avoid overfiring we add an additional set to track which of the new effects of a batch another batch already saw - avoid persisting stale async values: when an async derived commits a value of a later batch, and then a value of an earlier batch is committed, we're persistent a stale value. We need to avoid that to not "go back in time" - these two make the new test in this PR pass but not the redirect test in SvelteKit. For that I had to add the hack in `deriveds.js` to also unblock on the surrounding `flatten` effect For the first part I also had a different approach, but it was more LOC and felt a bit scary with respects to deadlocks so I discarded it. In `deriveds.js` I had this before `decrement_pending?.()`: ```ts if (!error) { let blocked = false; // All prior async derived runs are now stale for (const [b, _d] of deferreds) { if (b.id < batch.id) { batch.unblocked.add(effect); if (parent !== null) { // Terrible hack: this way we unblock ourselves from `flatten` // which can block us on initial run batch.unblocked.add(parent); } batch.oncommit(() => _d.resolve(value)); if (batch.is_blocked_by(b)) { // We do not want to commit just yet because it means we could write a newer // value to the source before an older value, and then the older value is the // latest one, creating stale UI / UI that "travels backwards". const resume = () => queue_micro_task(() => handler(value, error)); _d.promise.then(resume, resume); b.ondiscard(resume); blocked = true; } } } if (blocked) return; } ``` (and the related logic further down got removed) --- .changeset/beige-beans-arrive.md | 5 + .changeset/mean-windows-march.md | 5 + .../internal/client/dom/blocks/branches.js | 10 +- .../src/internal/client/reactivity/batch.js | 101 ++++++++++++------ .../internal/client/reactivity/deriveds.js | 20 +++- .../samples/async-stale-derived-8/_config.js | 20 ++++ .../samples/async-stale-derived-8/main.svelte | 23 ++++ 7 files changed, 141 insertions(+), 43 deletions(-) create mode 100644 .changeset/beige-beans-arrive.md create mode 100644 .changeset/mean-windows-march.md create mode 100644 packages/svelte/tests/runtime-runes/samples/async-stale-derived-8/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/async-stale-derived-8/main.svelte diff --git a/.changeset/beige-beans-arrive.md b/.changeset/beige-beans-arrive.md new file mode 100644 index 0000000000..1a5a36000c --- /dev/null +++ b/.changeset/beige-beans-arrive.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: reschedule new effects from other branches diff --git a/.changeset/mean-windows-march.md b/.changeset/mean-windows-march.md new file mode 100644 index 0000000000..620a140d36 --- /dev/null +++ b/.changeset/mean-windows-march.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: avoid persisting stale async values diff --git a/packages/svelte/src/internal/client/dom/blocks/branches.js b/packages/svelte/src/internal/client/dom/blocks/branches.js index 33c34f58bb..8127eb3651 100644 --- a/packages/svelte/src/internal/client/dom/blocks/branches.js +++ b/packages/svelte/src/internal/client/dom/blocks/branches.js @@ -109,13 +109,15 @@ export class BranchManager { } for (const [b, k] of this.#batches) { - this.#batches.delete(b); - - if (b === batch) { + if (b.id > batch.id) { // keep values for newer batches - break; + continue; } + this.#batches.delete(b); + + if (b === batch) continue; + const offscreen = this.#offscreen.get(k); if (offscreen) { diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 8f14cb4437..07d3fe49c7 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -92,6 +92,21 @@ let uid = 1; export class Batch { id = uid++; + /** + * Effects this batch has seen already or that were created before it (and there it is able to know about them). + * Used to filter out #new_effects of other batches during #commit. + */ + #seen_effects = new Set(); + + constructor() { + // This batch doesn't care about new batches before it was created; it knows about these already + for (const batch of batches) { + for (const e of batch.#new_effects) { + this.#seen_effects.add(e); + } + } + } + /** True as soon as `#process()` was called */ #started = false; @@ -206,24 +221,20 @@ export class Batch { #is_blocked() { for (const batch of this.#blockers) { - for (const effect of batch.#blocking_pending.keys()) { + outer: for (const effect of batch.#blocking_pending.keys()) { if (this.unblocked.has(effect)) continue; - var skipped = false; var e = effect; while (e.parent !== null) { if (this.#skipped_branches.has(e)) { - skipped = true; - break; + continue outer; } e = e.parent; } - if (!skipped) { - return true; - } + return true; } } @@ -415,6 +426,7 @@ export class Batch { } else if (async_mode_flag && (flags & (RENDER_EFFECT | MANAGED_EFFECT)) !== 0) { render_effects.push(effect); } else if (is_dirty(effect)) { + this.#seen_effects.add(effect); // This effect was definitely touched by this batch now if ((flags & BLOCK_EFFECT) !== 0) this.#maybe_dirty_effects.add(effect); update_effect(effect); } @@ -521,7 +533,9 @@ export class Batch { * @param {Effect} effect */ register_created_effect(effect) { - this.#new_effects.push(effect); + if ((effect.f & (EAGER_EFFECT | BRANCH_EFFECT)) === 0) { + this.#new_effects.push(effect); + } } #commit() { @@ -534,6 +548,44 @@ export class Batch { /** @type {Source[]} */ var sources = []; + /** @type {Map} */ + var checked = new Map(); + + var scheduled = false; + + var current_unequal = [...batch.current] + .filter(([k, c]) => { + const current = this.current.get(k); + // unequal if value is different and it's not a derived in both + return current ? current[0] !== c[0] && (!current[1] || !c[1]) : true; + }) + .map(([k]) => k); + + if (current_unequal.length > 0) { + // New effects created in this batch another batch doesn't know about yet + // need to be checked against the sources that are changing in this batch, + // as they might need to be rescheduled now. If we don't do this, we can + // end up with stale values in the UI. + for (const effect of this.#new_effects) { + if ( + !batch.#seen_effects.has(effect) && + (effect.f & (INERT | EAGER_EFFECT)) === 0 && + depends_on(effect, current_unequal, checked) + ) { + if ((effect.f & (ASYNC | BLOCK_EFFECT)) !== 0) { + scheduled = true; + set_signal_status(effect, DIRTY); + batch.schedule(effect); + } else { + // TODO this isn't quite right, maybe the source is different + // but the effect is only reading it indirectly through a derived + // that didn't change, so it doesn't need to re-run. + batch.#dirty_effects.add(effect); + } + // TODO skipped branches needs to be taken into account here? + } + } + } for (const [source, [value, is_derived]] of this.current) { if (batch.current.has(source)) { @@ -562,8 +614,8 @@ export class Batch { // this batch is now obsolete and can be discarded batch.discard(); } - } else if (sources.length > 0) { - if (DEV) { + } else if (sources.length > 0 || scheduled) { + if (DEV && !scheduled) { invariant(batch.#roots.length === 0, 'Batch has scheduled roots'); } @@ -585,36 +637,12 @@ export class Batch { /** @type {Set} */ var marked = new Set(); - /** @type {Map} */ - var checked = new Map(); + checked = new Map(); for (var source of sources) { mark_effects(source, others, marked, checked); } - checked = new Map(); - var current_unequal = [...batch.current.keys()].filter((c) => - this.current.has(c) - ? /** @type {[any, boolean]} */ (this.current.get(c))[0] !== c.v - : true - ); - - if (current_unequal.length > 0) { - for (const effect of this.#new_effects) { - if ( - (effect.f & (DESTROYED | INERT | EAGER_EFFECT)) === 0 && - depends_on(effect, current_unequal, checked) - ) { - if ((effect.f & (ASYNC | BLOCK_EFFECT)) !== 0) { - set_signal_status(effect, DIRTY); - batch.schedule(effect); - } else { - batch.#dirty_effects.add(effect); - } - } - } - } - // Only apply and traverse when we know we triggered async work with marking the effects if (batch.#roots.length > 0) { batch.apply(); @@ -1024,6 +1052,9 @@ function mark_effects(value, sources, marked, checked) { (flags & DIRTY) === 0 && depends_on(reaction, sources, checked) ) { + // TODO this isn't quite right, maybe the source is different + // but the effect is only reading it indirectly through a derived + // that didn't change, so it doesn't need to re-run. set_signal_status(reaction, DIRTY); schedule_effect(/** @type {Effect} */ (reaction)); } diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index 794da4f2b0..3281ad1fba 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -47,6 +47,7 @@ import { batch_values, current_batch, previous_batch } from './batch.js'; import { increment_pending, unset_context } from './async.js'; import { deferred, includes, noop } from '../../shared/utils.js'; import { set_signal_status, update_derived_status } from './status.js'; +import { queue_micro_task } from '../dom/task.js'; /** * This allows us to track 'reactivity loss' that occurs when signals @@ -125,7 +126,7 @@ export function async_derived(fn, label, location) { // only suspend in async deriveds created on initialisation var should_suspend = !active_reaction; - /** @type {Map>>} */ + /** @type {Map> & {temp?: true}>} */ var deferreds = new Map(); async_effect(() => { @@ -135,7 +136,7 @@ export function async_derived(fn, label, location) { reactivity_loss_tracker = { effect, effect_deps: new Set(), warned: false }; } - /** @type {ReturnType>} */ + /** @type {ReturnType> & {temp?: true}} */ var d = deferred(); promise = d.promise; @@ -226,17 +227,28 @@ export function async_derived(fn, label, location) { signal.f ^= ERROR_VALUE; } + const v = signal.v; internal_set(signal, value); + if (d.temp) signal.v = v; // All prior async derived runs are now stale for (const [b, d] of deferreds) { if (b.id < batch.id) { - // Don't delete + resolve directly, instead only do that once - // the current batch commits. This way we avoid tearing when + // Don't resolve directly, instead only do that once the + // current batch commits. This way we avoid tearing when // `b` is rendering through the early resolve while `batch` is // still pending. batch.unblocked.add(effect); + if (parent !== null) { + // Terrible hack: this way we unblock ourselves from `flatten` + // which can block us on initial run + batch.unblocked.add(parent); + } batch.oncommit(() => d.resolve(value)); + // We don't want the value to be written to the underlying source because + // else it will be considered the latest value even thought it's outdated. + // This can happen if the earlier derived resolves before this batch commits. + d.temp = true; } } diff --git a/packages/svelte/tests/runtime-runes/samples/async-stale-derived-8/_config.js b/packages/svelte/tests/runtime-runes/samples/async-stale-derived-8/_config.js new file mode 100644 index 0000000000..fd0fc0de48 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-stale-derived-8/_config.js @@ -0,0 +1,20 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target }) { + const [increment, pop] = target.querySelectorAll('button'); + + increment.click(); + await tick(); + increment.click(); + await tick(); + pop.click(); + await tick(); + assert.htmlEqual(target.innerHTML, ' 2 2 1'); // showing nothing here yet would also be ok + + pop.click(); + await tick(); + assert.htmlEqual(target.innerHTML, ' 2 2 1'); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-stale-derived-8/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-stale-derived-8/main.svelte new file mode 100644 index 0000000000..42b7206b56 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-stale-derived-8/main.svelte @@ -0,0 +1,23 @@ + + + + + + +{#if count > 0} + {await push(count)} {count} {other} +{/if}