fix: unset context on stale promises

[dummdidumm]

When a stale promise is rejected in async_derived, and the promise eventually resolves, d.resolve will be noop and d.promise.then(handler, ...) will never run. That in turns means any restored context (via (await save(..))()) will never be unset. We have to handle this case and unset the context to prevent errors such as false-positive state mutation errors.

There's a separate error about the derived running multiple times when it shouldn't, but I haven't gotten around to that yet.

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: 2c0304f

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

[paoloricciuti]

LGTM! And it's definitely not because @dummdidumm has to explain it to me in a video call. 😅

[Rich-Harris]

I'm having a real hard time making sense of the test. Can we have a version that doesn't rely on setTimeout so that we can see the bug in the playground?

[Rich-Harris]

(I'd also love it if we didn't need to monkey-patch the deferred, since we presumably intend to use Promise.withResolvers when it's universally available)

[Rich-Harris]

we're not doing d.promise.then(handler), we're doing d.promise.then(onresolved, onrejected) — I'd expect onrejected (which does call unset_context) to run if d.promise rejects. what am I missing?

[paoloricciuti]

I had the same question, glad it didn't trick only me. The order of events is

1. async_effect creates the promise, and run fn which saves the context
2. Something changes, async_effect creates a new promise and reject the old one, handler is executed with null and the context is cleared
3. This new promise resolve, the context is restored by the return value of save
4. First promise resolves the context is restored, d.resolve is invoked and that should clear the context but since d was already rejected it does nothing

The context is never cleared and there's a hanging active_reaction

[dummdidumm]

The order of events is this:

1. you enter X, batch runs
2. you enter Y, X hasn't resolve yet, batch runs
3. Y resolves before X, X is stale -> you reject the promise for X -> d.promise.then below runs, also unsets context but that doesn't matter, because ...
4. some time later X resolves. The generated code looks something like $.async_derived((await save(x())())), which means the restore method of save runs. That means the context is now set again. But d.promise.then(handle) will NOT run, because that promise is already done (we called its reject), so the context is never unset (unless with my fix)

Here's a playground that shows you the error without the fix (type in 1234 fast enough): https://svelte.dev/playground/hello-world?version=5.39.11#H4sIAAAAAAAACm1RwWrjMBD9lWEoRGq9bnroxU0Kve1xWXpb7UG1J4lAGRlpbKcY__uiaNO00ItA781783gzI9sjYYM_yfsAU4i-A0WdE-o0VrhznhI2f2aU9z7PZQCri-ql7-s0kpeMvdlE3-FtYCGWhA1uUhtdL8-GjXgSaMPAAlu4SWKF1Fo_XZjR-oGuzGqlDWduN3ArLjDY1KlRw5zBIomwhV8xHF2ienJy-E0p-JFiUsXWSBs4BU-1D3u1SmKjON6vKhh14cvLNF18VITt82WHkUTy6o4UBlGxAvX4Y6w98V4OGm7hYb1e__dZdC0HYqX0F_nn9QbjOZ7jvcGc4Ok6NbDc3X38Y10mSY0f9p_jRpIhMsS6L5nPwqW0lTcKnHKPHUU3UqfsZJ2U9nLFWhve3F_PwhvH_SDw5rhryhHuz_h8zrUYnk8LVih0EmwkDrT8rVCs85PjDpud9YmWf-ytupNXAgAA

I don't know why my test does not show the error in the playground, I thought it did.
I didn't get this to show up with button clicks yet, maybe because our event handlers unset the context before flushing, masking the error.

As to monkey-patching: We can just do let d = { ...Promise.withResolvers(), rejected: false } later

[Rich-Harris]

I see. Couldn't we just do it like this though? #16936

[svelte-docs-bot]

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