Skip to content

Conversation

dummdidumm
Copy link
Member

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

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
Copy link

changeset-bot bot commented Oct 13, 2025

🦋 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

Copy link
Contributor

Playground

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

Copy link
Member

@paoloricciuti paoloricciuti left a comment

Choose a reason for hiding this comment

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

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

@Rich-Harris
Copy link
Member

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
Copy link
Member

(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)

Promise.resolve(fn()).then((v) => {
if (d.rejected) {
// If we rejected this stale promise, d.resolve
// is a noop (d.promise.then(handler) below will never run).
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@dummdidumm dummdidumm Oct 13, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

…16936)

* slightly different approach to #16935

* move unset_context call
@svelte-docs-bot
Copy link

@Rich-Harris Rich-Harris merged commit 23e2bb3 into main Oct 13, 2025
18 checks passed
@Rich-Harris Rich-Harris deleted the promise-stale-fix branch October 13, 2025 21:22
@github-actions github-actions bot mentioned this pull request Oct 13, 2025
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.

3 participants