Skip to content

fix: reschedule effect + avoid stale values#18189

Closed
dummdidumm wants to merge 3 commits into
mainfrom
stale-values-async-fix
Closed

fix: reschedule effect + avoid stale values#18189
dummdidumm wants to merge 3 commits into
mainfrom
stale-values-async-fix

Conversation

@dummdidumm

Copy link
Copy Markdown
Member

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?.():

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)

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-bot

changeset-bot Bot commented May 7, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: bb85f61

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@svelte-docs-bot

Copy link
Copy Markdown

@github-actions

github-actions Bot commented May 7, 2026

Copy link
Copy Markdown
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@18189

Comment thread packages/svelte/src/internal/client/reactivity/deriveds.js Outdated
Comment thread packages/svelte/src/internal/client/reactivity/batch.js Outdated
dummdidumm and others added 2 commits May 7, 2026 22:19
Co-authored-by: vercel[bot] <35613825+vercel[bot]@users.noreply.github.com>
@dummdidumm

Copy link
Copy Markdown
Member Author

Closing in favor of #18205

@dummdidumm dummdidumm closed this May 13, 2026
@dummdidumm dummdidumm deleted the stale-values-async-fix branch May 13, 2026 19:35
Rich-Harris added a commit that referenced this pull request May 13, 2026
This is another attempt to address some of the tricky edge cases that
arise when async batches resolve out of order. The idea is this:

Essentially, when a batch resolves:

1. we find the latest batch it shares changes with
2. if none exists (either because this is the earliest batch, or because
it is independent of any earlier batches):
    - we commit it: effects are flushed, `oncommit` callbacks are run
- we restart any async work in later batches that depends on both the
committed values and the later batch's changes
3. otherwise, we don't commit the batch. instead:
- we merge the changes from the later batch onto the earlier batch. in
some cases this may mean restarting async work on the earlier batch, but
we avoid doing so unnecessarily.
    - we then `#process()` the earlier batch. if it resolves, goto 1

This feels like it ought to work. There are still two failing tests,
which I'm currently looking into.

Notable changes:

- Instead of having a `batches` set, we have a linked list. When a batch
resolves, this makes it easy to find the batch that it should be merged
into
- The `#is_deferred` logic now takes account of skipped effects —
there's no need to wait for a promise inside a falsy `if` block (this
accounts for the change in the `async-inner-after-outer` test)
- We no longer care about `#blockers`

I would like to believe that this approach will allow us to simplify and
delete some code, for example the `rebase` logic, though that remains to
be seen. It also feels like some version of #18035 would be helpful.

- closes #18189
- closes #18162
- fixes sveltejs/kit#15431

### Before submitting the PR, please make sure you do the following

- [x] It's really useful if your PR references an issue where it is
discussed ahead of time. In many cases, features are absent for a
reason. For large changes, please create an RFC:
https://github.com/sveltejs/rfcs
- [x] Prefix your PR title with `feat:`, `fix:`, `chore:`, or `docs:`.
- [x] This message body should clearly illustrate what problems it
solves.
- [x] Ideally, include a test that fails without this PR but passes with
it.
- [x] If this PR changes code within `packages/svelte/src`, add a
changeset (`npx changeset`).

### Tests and linting

- [x] Run the tests with `pnpm test` and lint the project with `pnpm
lint`

---------

Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant