fix: prevent reactivity loss during/after fork

[hmnd]

fixes #17197, fixes #17304, fixes #17301, fixes #17309, possibly others

Thanks to @dummdidumm's prior digging into this here

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: 1f7975c

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@17335

[soniquete0]

I tested this with my repro mentioned in this comment - sveltejs/kit#15059 (comment)
It does fix the hovering a link bug.
There seems to be one more bug related to async as well related to navigation also mentioned in the comment. I am leaving a comment about it here as I thought that you might find it interesting.

[svelte-docs-bot]

Preview: https://svelte-dev-git-preview-svelte-17335-svelte.vercel.app/

[paoloricciuti]

I think it makes sense...there's only one thing that it kinda irks me here: we are restoring ALL the branches to CLEAN but wouldn't this lead to over-running?

[hmnd]

Hmm I don't think that's a concern because we only do this for branches that had CLEAN before the fork. I'm pretty sure setting CLEAN doesn't necessarily mean branches will run either, it just allows them to be scheduled.

[Heniker]

I am not sure If I understand fork correctly, but it seems that just using fork(() => {}) can completely hang the app: REPL (offending line is commented)

This seems to be even execution order-dependent.

[hmnd]

@Heniker you're seeing a combination of the bug fixed in this pr, and the bug with deriveds (which {@const} compiles to) in my other pr #17362. Stacking the two prs fixes your repro and doesn't result in a freeze

[hmnd]

@teemingc let me know if you'd prefer for me to combine this PR with #17362. As you can see from the above comment, this PR is sort of dependent on that one to fully fix reactivity, although they touch different parts of the runtime.

[teemingc]

Thanks David. I think it's completely fine to keep the PRs separate as they make reviewing the changes easier. I'd recommend editing the descriptions to mention how the two are linked.

Unfortunately, I'm not too familiar with the Svelte codebase so I'd wait for one of the more experienced maintainers to review the two PRs once everyone's back from their holidays.

[hmnd]

Great catch! I've fixed it simply by not removing reactions during forks. This does mean we potentially keep stale deps around for a bit longer, buth they'll still get cleaned up after the fork.

[hmnd]

Fixed a slight bug in my fix to @dummdidumm's repro. There was previously an attempt to ensure deriveds get their value set if first initalizied in a fork, but this was not working properly because derived.deps always gets set downstream from execute_derived, before the guard around setting derived.v.

This hadn't shown itself yet because remove_reactions forced re-evaluation of the derived on every fork. Now that we don't have that, the derived remains connected and gets marked as CLEAN after the first fork, resulting in the error on the second fork.

[Rich-Harris]

Thank you! Another incredibly helpful investigation. It turns out the reactivity loss isn't fork-specific — you can get the same result without it. Essentially what's happening is that because skipped_effects don't get traversed, the effects inside them are never reset. I think the most correct solution is to do that traversal when the batch is deferred — I've opened #17581. Never would have got there without this starting point.

[Rich-Harris]

Merged #17581 so will close this

[hmnd]

Aha nice find! Sorry I wasn't quite on the right track, but glad to have helped and happy to see this fixed :)