Skip to content

fix: correctly block on earlier batches#17971

Draft
dummdidumm wants to merge 20 commits intomainfrom
async-blocking-and-merging
Draft

fix: correctly block on earlier batches#17971
dummdidumm wants to merge 20 commits intomainfrom
async-blocking-and-merging

Conversation

@dummdidumm
Copy link
Copy Markdown
Member

@dummdidumm dummdidumm commented Mar 19, 2026

This tweaks the async batch algorithm:

  • If two batches touch distinct sources they can resolve independently (i.e. one started later can resolve earlier)
  • If a later batch is a superset of an earlier one it can ignore it because we know it will do all the work the earlier one does
  • Else if they overlap the later one is blocked on the earlier one

This also changes how batch_values is computed. During batch.#traverse we no longer use the previous values of earlier batches, in other words we see the latest value up to this batch. This is important to fix #17099, because the core problem of that bug is that when a later batch creates new effects with a previous value we have no good way to then update those effects after the earlier batch resolves (write versions are older for the earlier one).

But that means the blocking logic now also has to include new branches: If a later batch creates a new branch that then reads a value that an earlier batch wrote, we also have to block in that case, because we cannot show the new values of the earlier batch before that one has resolved.

It also includes logic of not outright rejecting earlier batches' promises once a later batch resolves in async_derived, because that batch might have other pending work that is either not superseeded by a later batch or will be superseeded by another batch. Instead we tell the batch which of its async work is obsolete, and if everything's obsolete we know we can savely discard it. This closes #17935 - I feel like a few more tests in that direction would be good though.

As a result batches resolve in the order they are started, if they are related, and the rebase algorithm becomes obsolete.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 19, 2026

⚠️ No Changeset found

Latest commit: 9fdcfe1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@github-actions
Copy link
Copy Markdown
Contributor

Playground

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

@dummdidumm
Copy link
Copy Markdown
Member Author

dummdidumm commented Mar 20, 2026

Found a case that works on main but not in this PR fixed

#get_blockers() {
const blockers = [];

for (const batch of [...this.blockers].sort((a, b) => b.id - a.id)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as far as I can see the sorting is unnecessary? the only thing we do with the resulting array is this:

Promise.all(blockers.map((b) => b.settled())).then(...);
Suggested change
for (const batch of [...this.blockers].sort((a, b) => b.id - a.id)) {
for (const batch of this.blockers) {

@svelte-docs-bot
Copy link
Copy Markdown

@dummdidumm dummdidumm marked this pull request as draft March 23, 2026 08:11
…ly reaching a more powerful version of `#commit()`? ugh)
…ere can still be other work (e.g. sources, new branches) that we need to merge into another batch)
dummdidumm added a commit that referenced this pull request Mar 24, 2026
extracting this from #17971 because even if that PR is not merged these tests show that there's still a lot of cases where we're buggy, particularly around forking and new branches
dummdidumm added a commit that referenced this pull request Mar 24, 2026
extracting this from #17971 because even if that PR is not merged these
tests show that there's still a lot of cases where we're buggy,
particularly around forking and new branches
dummdidumm added a commit that referenced this pull request Mar 27, 2026
If a batch creates a new branch (e.g. through an if block becoming true)
the previous batches so far do not know about the new effects created
through that. This can lead to stale values being shown. We therefore
schedule those new effects on prior batches if they are touched by a
`current` value of that batch

Fixes #17099

extracted from #17971
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.

Async: Values can get out of sync

2 participants