fix: skip derived re-evaluation inside destroyed branch effects#17862
fix: skip derived re-evaluation inside destroyed branch effects#17862mtsgrd wants to merge 7 commits intosveltejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 489cc44 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Thank you. Do you have a repro of this bug? The |
|
|
|
mtsgrd
force-pushed
the
gitbutler-bug-2
branch
2 times, most recently
from
049ccbb to
0d7b8a6
Compare
@Rich-Harris if you could have another look, I've replaced the fix with a version that doesn't loop, and I've replaced the unit test with a component test. Hope this is helpful! |
|
Unfortunately this still isn't quite right — if the block in question is transitioned out, instead of immediately destroyed, e.g. because you have this in the <div transition:fade></div>...you run into the same thing. I thought I had it fixed by giving {#if value}
- {@const result = value}
- <Inner data={result} bind:this={innerComp} />
+ <Inner data={value} bind:this={innerComp} />
{/if}The best bet might be to null out |
|
Okay I've tried everything and it doesn't seem possible to get this test passing without making another test, specifically Here's the thing: this test has been failing for a loooong time: The PR that initially caused it to fail was #15137, which fixed an issue @gyzerok was experiencing. And to be honest I'm just not sure what to do here. This will probably sound like I'm being dismissive, which isn't my intent, but I think these issues are occurring because you're writing code at the limits of what we can reasonably define behaviour for. It's just not possible, as far as I can tell, to design a system that satisfies all these extreme edge cases. |
@Rich-Harris not at all, I understand I'm at the boundary here so I appreciate you taking time to debug! 🙏 I'll come back to this thread if I learn anything new. |
|
@Rich-Harris not sure if you've missed my repro in another PR, that is an issue that have definitely appeared in the latest version. There is also this issue that seems to be very similar. When I looked at your effect related refactoring, I was immediately thinking there will be some issues 😄. The one in the repro is what breaks us in the hot path UX-wise, but there might be more. And unfortunately I can't roll out new version even internally (to search for other regressions), because app will become somewhat unusable. |
mtsgrd
force-pushed
the
gitbutler-bug-2
branch
2 times, most recently
from
8ff7bcb to
e24ce28
Compare
|
I'm also still keen to find a path forward, and I pushed a new commit to this branch in case it helps the investigation. What I'm trying to do is to address the concern where a transitioned-out block (INERT, not yet DESTROYED) still allows external readers to pull deriveds with stale deps.
I had a closer look at my setup and just want to flag two things:
It was always the case that it only fails on the second time.. and the more I debug this issue the more suspicious it feels. I'd like to be able to rely on the if contract, that code inside |
|
Update: I found another way to trigger this bug that doesn't rely on the external reader pattern from the original test. <script>
let value = $state('hello');
let elements = {};
</script>
{#if value}
<span bind:this={elements[value.toUpperCase()]}>{value}</span>
{/if}
<button onclick={() => (value = undefined)}>clear</button>
This is actually the pattern that was breaking things in GitButler: bind:this={triggers[change.path]}. The bind:this isn't on a component via an exported method — it's on a plain DOM element with a computed key. In my case it only crashes on the second destroy cycle (the first stale evaluation doesn't visibly throw, but the second one freezes the UI). The runtime guard in The three commits in this PR are now:
|
When a {#if} or {#each} branch is destroyed, derived values inside it
must not be re-evaluated — the closure captures state that is now gone,
and calling methods on undefined values throws.
The crash path:
- A parent $derived outside the block reads a child's exported reactive
value via bind:this, keeping the inner derived connected in the
reactive graph after the branch is torn down
- On the next flush, is_dirty() walks the dep chain and calls
execute_derived() on the destroyed branch's derived — which crashes
The fix checks whether the nearest non-derived ancestor has both
DESTROYED and BRANCH_EFFECT flags set, and returns the cached value
instead of re-evaluating.
This also simplifies get_derived_parent_effect() — it now returns the
raw ancestor unconditionally, and execute_derived handles the destroyed
case by setting parent_effect to null to avoid parenting new effects
into a dead scope.
When a branch is transitioning out (INERT), an external $derived can
still pull inner deriveds via bind:this, causing re-evaluation with
stale dependency values (e.g. value.toUpperCase() where value is now
undefined). This violates the {#if} contract — code inside {#if value}
should never run with a falsy value.
The fix adds a unified guard in update_derived that returns the cached
value instead of re-evaluating:
- Non-async mode: blocks unconditionally (INERT branches are never
walked by the scheduler, so any read is external)
- Async mode: blocks only during effect flushing (detected via
collected_effects === null && active_effect === null), allowing
branch-internal traversal to still update deriveds for transitions
This replaces a narrower guard that was previously in execute_derived
(non-async only) and extends coverage to async mode where the same
crash occurs but was previously unguarded.
Includes a component test (if-block-derived-inert-external-reader)
that reproduces the crash: an Inner component inside {#if value} with
out:fade exposes a $derived via bind:this; clearing value starts the
out-transition and triggers the external read path.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
While investigating the INERT branch guard from the previous commit, a
different trigger for the same class of bug was found: bind:this with a
dynamic key (e.g. `bind:this={elements[key]}`) inside an {#if} block.
When the branch is destroyed, bind:this queues a microtask to null out
the binding. That teardown calls get_value() which re-reads the dynamic
key expression — but the reactive context is gone, so it crashes
(e.g. value.toUpperCase() on undefined).
The fix is in the compiler (build_bind_this): computed member expression
keys and block-scoped variables used in bind:this expressions are now
captured as "parts" via get_parts, which the runtime evaluates during
the render_effect (while values are still valid) and reuses during
teardown. This extends an existing mechanism that previously only
captured {#each} context variables.
Two changes in build_bind_this:
- Broadened the identifier capture to include all block-scoped variables
(not just {#each} context), so {@const} variables are captured too
- Added extraction of computed MemberExpression properties into parts,
so inline expressions like bind:this={obj[expr]} are also safe
Test: if-block-bind-this-dynamic-key-destroyed covers the inline case
(bind:this={elements[value.toUpperCase()]}) which is only fixable via
the compiler change. The {#if} + transition variant
(if-block-bind-this-dynamic-key-inert) is covered by either this fix or
the runtime guard, so it was removed to avoid redundant coverage.
| // In non-async mode, INERT branches are never walked by the scheduler, | ||
| // so any read is necessarily external — block unconditionally. | ||
| // | ||
| // In async mode, INERT branches ARE walked (to keep transitions alive), |
There was a problem hiding this comment.
this is outdated now, might simplify some stuff
This removes the `queue_micro_task`-workaround we employed in `bind:this` in favor of a search for the nearest component effect / effect that is still getting destroyed, whichever comes first. We used `queue_micro_task` mainly due to timing issues with components wanting to access the bound property on teardown still, and when nulling it out on cleanup of the bind-this-effect itself, that was too early. The microtask is too late though in some cases, when accessing properties of objects that are no longer there. The targeted upwards-walk solves this while keeping the binding around as long as needed. For that I had to add a new `DESTROYING` flag. We _could_ have done it without one and by deleting code in `props.js` where we don't do `get(d)` when the prop derived is destroyed, but I wanted to keep that because you could still run into an access error if you e.g. access the property in a timeout. Alternative to #17862
|
The approach to the <span bind:this={elements[value.toUpperCase()][0]}>{value}</span>...and second, the case where it uses a function binding: <span bind:this={() => elements[value.toUpperCase()], (v) => elements[value.toUpperCase()] = v}>{value}</span>I spent way too long trying to solve this more generally, with both compiler- and runtime-oriented fixes, but couldn't get it to behave. And then @dummdidumm casually rolled up with #17885 which I think is a better fix — basically, it gets rid of the I'll update this PR so that it only addresses the other bug, which is distinct AFAICT. |
This removes the `queue_micro_task`-workaround we employed in `bind:this` in favor of a search for the nearest component effect / effect that is still getting destroyed, whichever comes first. We used `queue_micro_task` mainly due to timing issues with components wanting to access the bound property on teardown still, and when nulling it out on cleanup of the bind-this-effect itself, that was too early. The microtask is too late though in some cases, when accessing properties of objects that are no longer there. The targeted upwards-walk solves this while keeping the binding around as long as needed. For that I had to add a new `DESTROYING` flag. We _could_ have done it without one and by deleting code in `props.js` where we don't do `get(d)` when the prop derived is destroyed, but I wanted to keep that because you could still run into an access error if you e.g. access the property in a timeout. Alternative to #17862
I really appreciate the efforts to help out here 🙏 Is there anything more I can do at the moment or you guys got it from here? |
| // don't update deriveds inside a destroyed branch (e.g. {#if} or {#each}) — | ||
| // the branch scope is invalid and evaluating could trigger side effects | ||
| // with stale values. | ||
| if ( | ||
| !is_destroying_effect && | ||
| raw_parent !== null && | ||
| (raw_parent.f & (DESTROYED | BRANCH_EFFECT)) === (DESTROYED | BRANCH_EFFECT) | ||
| ) { | ||
| return derived.v; | ||
| } | ||
|
|
There was a problem hiding this comment.
appears we never reach this code, because of the guard in update_derived
| // don't update deriveds inside a destroyed branch (e.g. {#if} or {#each}) — | |
| // the branch scope is invalid and evaluating could trigger side effects | |
| // with stale values. | |
| if ( | |
| !is_destroying_effect && | |
| raw_parent !== null && | |
| (raw_parent.f & (DESTROYED | BRANCH_EFFECT)) === (DESTROYED | BRANCH_EFFECT) | |
| ) { | |
| return derived.v; | |
| } |
|
Investigated a bit more. It felt a bit odd that we keep deriveds alive inside destroyed effects, precisely because it makes this sort of situation inevitable, and to my mind there's nothing particularly special about branch effects — in other words it feels wrong to fix the case this PR addresses but not, say, this. So I dug into why we keep deriveds alive inside destroyed effects, and it turns out it comes from #17171. We changed the behaviour there because of a quirk of how remote functions work in SvelteKit: you can invoke a That results in a subtle bug: if we read a query in component A, we create the instance (along with its deriveds) inside the context of the current effect. If we then show component B which also reads the query, and destroy component A, the object is still alive in the cache but the deriveds belong to a now-destroyed effect, and refreshing the query will only work if we've kept it alive. But that's the wrong way to think about it. The underlying shared instance should be just that — shared. It shouldn't belong to component A or B. So the proper fix, we think, is to create it inside an Once we've done that, we can revert #17171 which should fix this issue. Part of me wonders if Svelte should warn when you try to read an inert derived, though that conversation can happen separately. |
4 participants
This is a follow-up to #17852. I was expecting it to resolve an issue that manifests in production, but somehow between my initial debugging and the merged fix something diverged and the issue still reproduces.
Video of issue that prompted the original pr as well as this one:
Before:
bug2.mp4
With this fix:
bug2-after.mp4
AI Explanation
When a branch effect (
{#if},{#each}) is destroyed, deriveds inside it must not re-evaluate — the branch scope is gone and computed values will be stale or throw.The immediate trigger was bind:this teardown: destroying a component with bind:this schedules a microtask that calls get_value on the element binding. If the bound element was inside a child component rendered from a
{@const}derived in an{#if}block that just became false, the derived would re-run with undefined and crash (e.g. calling.toUpperCase()on undefined).Root cause:
get_derived_parent_effect()returns null for destroyed parent effects, soexecute_derivedseesparent_effect === nulland proceeds to re-evaluate. The fix walks the .parent chain directly to detect whether the nearest non-derived ancestor is aBRANCH_EFFECTthat has beenDESTROYED, and returns the cached value instead.Tests: adds a component integration test (runtime-runes) that exercises the exact
{#if}/{@const}/bind:thispattern, and a low-level signal unit test that directly sets up the block → branch → derived teardown sequence to confirm no re-evaluation occurs after destruction.Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint