fix(signals): a removed row's cleanup can write during teardown (#4165) by ryansolid · Pull Request #4168 · solidjs/solid

ryansolid · 2026-10-20T04:51:14Z

Summary

Before: removing a keyed <For> row disposed it inside the list's pass, so the row's cleanups ran while the list computation was the current scope. A setter in an onCleanup or onSettled(() => () => ...) cleanup was rejected with REACTIVE_WRITE_IN_OWNED_SCOPE, and reactivity halted in development. The row stayed mounted and later list updates were ignored. The report hit this in a real app: deleting a row from its own open menu, where the menu's teardown publishes shared state.

After: a cleanup's signal, store or refresh write is allowed during teardown, the same way an effect callback's is. Cleanups still run under their owner, so getOwner() and context lookups inside them work as before, and errors thrown from a cleanup still reach the nearest boundary.

How: owner teardown already raises the dev-only disposalDepth counter around cleanups. The three owned-scope write checks (setSignal, markRefresh and the store setter guard) now also skip when it is raised. Everything is behind __DEV__, so production output is unchanged (0 B).

An earlier version of this PR ran cleanups through runWithOwner(null, ...). That fixed the write but made getOwner() return null and getContext throw inside cleanups, so it was replaced.

How did you test this change?

New packages/signals/tests/for-row-cleanup-write-4165.test.ts: a mapArray row cleanup writing a signal, a branch-memo cleanup writing a signal, and a row cleanup writing a store all fail on next and pass here. A fourth test checks that a row cleanup still sees its owner and a provided context.
New packages/web/test/for-row-cleanup-write-4165.spec.tsx is the report's shape: a <For> row with an onSettled cleanup or an onCleanup that calls a setter, then removing the row and appending another. It fails on next and passes here.
Probed cleanups that throw under an Errored boundary (a <For> row and a memo branch): same result on next and with this change.
Full suites after a build: signals 5409 passed, web 1394 passed, solid 877 passed. Size gate passes, and dist/prod has no reference to the counter.

https://claude.ai/code/session_018LY7F9czoRqdMEvk2QwPr1

Generated by Claude Code

…can write (#4165) Removing a keyed For row disposed it inside the list's pass, so its cleanups ran with the list computation as context. A setter there was rejected as an owned-scope write and halted reactivity in development. Cleanups (_disposal entries and the final effect-returned cleanup) now run through runWithOwner(null), untracked. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LY7F9czoRqdMEvk2QwPr1

changeset-bot · 2026-10-20T04:51:19Z

🦋 Changeset detected

Latest commit: c9745ac

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages

Name	Type
@solidjs/signals	Patch
test-integration	Patch
@solidjs/web	Patch
@solidjs/babel-plugin	Patch
@solidjs/compiler	Patch
@solidjs/diagnostics	Patch
@solidjs/element	Patch
@solidjs/h	Patch
@solidjs/html	Patch
solid-js	Patch
@solidjs/universal	Patch
board-example	Patch
server-dbmon-example	Patch
todos-server-example	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 · 2026-10-20T04:54:13Z

Size (brotli, eager entry chunk)

scenario	head	minified vs recorded	cap		lazy chunks (not counted)
signals: core floor (createSignal/Memo/Effect/Root/flush)	7.37 KB	0 B	7.39 KB	✅
signals: + createStore	14.93 KB	0 B	14.95 KB	✅
signals: + isPending/latest	10.14 KB	0 B	10.16 KB	✅
signals: + until/refresh (truth readers, no optimism)	7.97 KB	0 B	7.98 KB	✅
app: render + one signal (the simple-app floor)	9.68 KB	0 B	9.70 KB	✅
app: hydrating (no stores) with Show/For/Loading/Errored/lazy	18.31 KB	0 B	18.32 KB	✅	lazy-page.js 0.04 KB
app: hydrating + every store primitive family	30.31 KB	0 B	30.32 KB	✅	lazy-page.js 0.04 KB
app: CSR with Show/For/Loading/Errored/lazy	13.26 KB	0 B	13.28 KB	✅	lazy-page.js 0.04 KB
app: CSR, observe tier (same app on the `observe` artifacts)	15.02 KB	0 B	15.03 KB	✅	lazy-page.js 0.04 KB
app: CSR, observe tier + attribution engine enabled	29.61 KB	0 B	29.63 KB	✅	lazy-page.js 0.04 KB
app: compiled floor (one template, one text hole, one delegated click)	9.88 KB	0 B	9.89 KB	✅
app: compiled CSR (JSX todo app: spread/merge/omit, events, class/style, keyed For, Show, Loading + lazy, store)	26.09 KB	0 B	26.10 KB	✅	stats.js 0.18 KB
app: compiled hydrating (the same JSX todo app through hydrate(), compiled hydratable)	32.09 KB	0 B	32.13 KB	✅	stats.js 0.20 KB
frames: eager client consumer (frames client + transport, lazy codec)	11.75 KB	0 B	11.76 KB	✅
page: base server components (hydrating + dynamic + frames + sf reference)	33.19 KB	0 B	33.20 KB	✅	assets.js 0.78 KB, decode.js 6.25 KB, lazy-page.js 0.04 KB, regions.js 1.07 KB, trace.js 9.02 KB, wire.js 2.78 KB
page: live server components (base + live/GET + action + isPending/latest)	37.47 KB	0 B	37.48 KB	✅	assets.js 0.78 KB, decode.js 6.25 KB, lazy-page.js 0.04 KB, regions.js 1.06 KB, trace.js 9.06 KB, wire.js 2.78 KB
page: compiled base server components (the base page as JSX: templates with class/style/attributes/events, For/Show; no spread)	35.21 KB	0 B	35.23 KB	✅	assets.js 0.78 KB, decode.js 6.25 KB, regions.js 1.06 KB, sc-comments.js 0.20 KB, trace.js 9.06 KB, wire.js 2.77 KB
page: compiled live server components (the compiled base page + live/GET + action + isPending/latest)	41.30 KB	0 B	41.31 KB	✅	eager (counted): web.js 21.93 KB; assets.js 0.78 KB, decode.js 6.25 KB, regions.js 1.06 KB, sc-comments.js 0.19 KB, trace.js 9.08 KB, wire.js 2.77 KB
page: base + router (base page + @solidjs/router: createRouter, two routes, preload, useNavigate)	40.71 KB	0 B	40.72 KB	✅	assets.js 0.78 KB, decode.js 6.25 KB, lazy-page.js 0.04 KB, regions.js 1.06 KB, server.js 1.02 KB, serverForms.js 3.59 KB, trace.js 9.13 KB, wire.js 2.77 KB
page: live + router (live page + @solidjs/router: createRouter, two routes, preload, useNavigate)	46.90 KB	0 B	46.91 KB	✅	eager (counted): client.js 27.09 KB; assets.js 0.78 KB, decode.js 6.25 KB, lazy-page.js 0.04 KB, regions.js 1.07 KB, server.js 1.02 KB, serverForms.js 3.30 KB, trace.js 9.06 KB, wire.js 2.80 KB
server: floor (getRequestEvent + isServer)	1.33 KB	0 B	1.34 KB	✅
server: renderToString (the server-render floor)	20.62 KB	−30 B	20.59 KB	⚠️ over by 31 B, 50 B minified headroom

⚠️ Over the brotli cap within the minified allowance (passes)

server: renderToString (the server-render floor): over brotli cap by 31 B; minified 72,124 B vs 72,154 B recorded with the cap (−30 B) — 50 B of the 20 B minified allowance left; +0 B minified over this PR's base

_{Bundled with Rolldown (what Vite ships), brotli q11, decimal KB. A scenario fails only when it is over its brotli cap and its minified size is more than 20 B over the minified recorded with the cap; over the cap within that allowance is brotli layout noise and passes with a warning. Caps and their recorded minified in scripts/size/scenarios.js; the floor and page caps in floor-caps.json are frozen (lower only, or Size-Exception: in the PR body). npm run ratchet lowers caps per RC; it never raises one (scripts/size/README.md).}

coveralls · 2026-10-20T05:00:41Z

Coverage Report for CI Build 39391318236

Coverage remained the same at 77.555%

Details

Coverage remained the same as the base build.
Patch coverage: No coverable lines changed in this PR.
No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	1347
Covered Lines:	1103
Line Coverage:	81.89%
Relevant Branches:	1050
Covered Branches:	756
Branch Coverage:	72.0%
Branches in Coverage %:	Yes
Coverage Strength:	27.56 hits per line

💛 - Coveralls

codspeed · 2026-10-20T05:06:11Z

Merging this PR will degrade performance by 10.6%

⚠️

2 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

build + consume

readAllowed

⚠️

Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 1 regressed benchmark
✅ 195 untouched benchmarks
⏩ 5 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Benchmark	`BASE`	`HEAD`	Efficiency
❌ ⚠️	`build + consume`	694.3 µs	776.6 µs	-10.6%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

_{Comparing fix/for-row-cleanup-write-4165 (c9745ac) with next (71153bf)}

5 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

ryansolid · 2026-10-20T05:14:36Z

CodSpeed flags createRenderEffects:create1to1 (−23.9%) and projection derive NESTED (−25.4%). The projection benchmark is known to be noisy here: it swung +34% on #4164 the same day. On create1to1, this change can't be on the hot path, because those render effects register no cleanups and runDisposal returns before the new runWithOwner call. A local A/B on the built package, run twice alternately, shows the branch equal or faster: 117 and 105 ops/s with the fix, 89 and 96 on next. CodSpeed also warns that it compared different runtime environments. I'm treating both as noise.

— Claude via Claude Code

Generated by Claude Code

Running cleanups with no owner broke getOwner() and getContext inside them. Instead, the dev-only owned-scope write checks (setSignal, markRefresh, the store setter) stand down while disposalDepth is raised, so a cleanup's writes are allowed and production output is unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018LY7F9czoRqdMEvk2QwPr1

ryansolid · 2026-10-20T08:33:32Z

The CodSpeed failure is props chain depth 7 :: build + consume (−10.6%). That benchmark is known to be noisy, and CodSpeed flags it here for system-call time and for different runtime environments. This PR's source change only touches __DEV__ branches, so production output is byte-identical: the size report shows 0 B on every scenario, and dist/prod has no reference to the new check. Every other check is green. I'm not changing anything for this one, and I don't have the means to re-run CodSpeed from here.

— Claude via Claude Code

Generated by Claude Code

ryansolid changed the title ~~fix(signals): cleanups run with no owner, so a removed row's cleanup can write (#4165)~~ fix(signals): a removed row's cleanup can write during teardown (#4165) Oct 20, 2026

ryansolid merged commit a2743fa into next Oct 20, 2026
7 of 8 checks passed

ryansolid mentioned this pull request Oct 20, 2026

[2.0 rc.15 + next] For row lifecycle cleanup inherits the list computation and halts on a shared-state write #4165

Closed

Navigation Menu

Repository navigation

fix(signals): a removed row's cleanup can write during teardown (#4165) - #4168

fix(signals): a removed row's cleanup can write during teardown (#4165)#4168
ryansolid merged 2 commits into
nextfrom
fix/for-row-cleanup-write-4165

ryansolid commented Oct 20, 2026 •

edited

Loading

Uh oh!

changeset-bot Bot commented Oct 20, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 20, 2026 •

edited

Loading

Uh oh!

coveralls commented Oct 20, 2026 •

edited

Loading

Uh oh!

codspeed Bot commented Oct 20, 2026 •

edited

Loading

Uh oh!

ryansolid commented Oct 20, 2026

Uh oh!

ryansolid commented Oct 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ryansolid commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How did you test this change?

Uh oh!

changeset-bot Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size (brotli, eager entry chunk)

⚠️ Over the brotli cap within the minified allowance (passes)

Uh oh!

coveralls commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 39391318236

Coverage remained the same at 77.555%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

codspeed Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will degrade performance by 10.6%

Performance Changes

Footnotes

Uh oh!

ryansolid commented Oct 20, 2026

Uh oh!

ryansolid commented Oct 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ryansolid commented Oct 20, 2026 •

edited

Loading

changeset-bot Bot commented Oct 20, 2026 •

edited

Loading

github-actions Bot commented Oct 20, 2026 •

edited

Loading

coveralls commented Oct 20, 2026 •

edited

Loading

codspeed Bot commented Oct 20, 2026 •

edited

Loading