fix: prevent re-activated stale batches from break reactivity

[paoloricciuti]

Tweaked @dummdidumm test a bit to also show how after the stale batch was deactivated it was basically freezing reactivity.

The problem is that if a previous promise resolves after another one the previous is restored and that re-activate an already committed branch and that just lingers around preventing new batches from being created.

The check was suggested by codex but I spent a bit of time figuring out if it was correct, and it seems it should work since once is committed the batch is removed from batches so if a current_branch is not in there it should mean that it's been committed, and it's now being re-activated.

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

• 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
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.
• If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

🦋 Changeset detected

Latest commit: bf7581c

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

[github-actions]

Playground

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

[dummdidumm]

I still wonder if we are missing a cleanup earlier somewhere which would make this check unnecessary

[paoloricciuti]

I still wonder if we are missing a cleanup earlier somewhere which would make this check unnecessary

I don't think we are necessarily missing a clean-up...at most, we are doing a clean-up at the wrong time? Also weird tests are failing on node 18 while 22 and 24 works. Did something changed regarding the timing of setTimeout?

[Rich-Harris]

This definitely feels like it's working around a bug rather than fixing it. But it's not just a cleanup issue — if you mount a component with a pending boundary, and all the await expressions are inside the boundary, then the batch will commit (because those await expressions are linked to the boundary, rather than the batch) but when the work resolves it will restore the committed batch.

This points at a design flaw in the relationship between boundaries and batches, which I've been trying to wrap my head round for a while now (but took a break from in the interests of shipping a conference talk). #16743 is another manifestation of the same flaw. I guess we need to get serious about fixing that.

[paoloricciuti]

but when the work resolves it will restore the committed batch.

Isn't this exactly the problem? I agree this might require some different fixing that solves the issues for good, just asking to understand.

[Rich-Harris]

Yeah, I'm just saying that the problem isn't that we're forgetting to unset current_batch somewhere after it's removed from the batches set, or something like that (which I would categorise as a 'cleanup issue'). It's not a bug that we can trivially work around, it's a more fundamental flaw

[paoloricciuti]

Gotcha... I think this should be a p0 (maybe p1 since it's experimental) because if you happen to have a situation where promise B started after promise A resolves first, then you basically freeze reactivity without ANY errors.