Repository navigation
fix: a throwing cleanup no longer aborts the rest of the teardown - #4213
Merged
Merged
Conversation
Every cleanup in the tree runs even when one throws: the ones registered before it on the same owner, unvisited siblings, ancestors and effect cleanups. The first error is rethrown once the teardown is over. Client (@solidjs/signals): one guarded call (`runCleanup`) covers both the `_disposal` list and the effect `_cleanup`, collecting the first error. Outside a flush the walk that began the teardown rethrows it; inside a flush (a commit retiring a frame, a pass replacing an uncommitted one, an effect disposing a root) `flush()` rethrows it after the drain completes, so the commit loop and the effect queues are never left half-applied and the pass that tears down a frame still drains the lazy releases it collected (#4176's follow-up case now passes without a dedicated drain). Server (solid-js): `disposeOwner` guards each child walk and each cleanup and rethrows the first error after the walk, so renderToStream's final dispose runs every cleanup and #4206 still reports the error (#4193). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015hPxMkC3jg3Uivh65gvaf2
🦋 Changeset detectedLatest commit: 8e262f9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
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 |
Size (brotli, eager entry chunk)
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 |
Every scenario the change grows, at CI's measured sizes. Hello world (app: compiled floor) is 9,928 B brotli, under 10 KB. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015hPxMkC3jg3Uivh65gvaf2
Coverage Report for CI Build 39686012859Coverage remained the same at 77.662%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Merging this PR will improve performance by 11.97%2 warnings
Performance Changes
Tip Curious why performance improved? Comment Comparing 
Footnotes
|
CodSpeed flagged createOwners at -10%: the end-of-walk rethrow check in disposeChildren slowed every disposal. Root dispose now rethrows after its walk outside a flush; every other teardown leaves it to the flush that runCleanup schedules. Drops the nested parameter too. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015hPxMkC3jg3Uivh65gvaf2
Hello world (app: compiled floor) is 9,935 B brotli, under 10 KB. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015hPxMkC3jg3Uivh65gvaf2
…halt Review follow-ups for #4213: - A throwing `unobserved` hook is collected like a cleanup, so the teardown around it carries on and a pass's release collector is always removed. - A halt drops a collected cleanup error, so it never surfaces from a later, unrelated dispose(). - Server disposeOwner goes back to the indexed reverse loop. - Tests for the mid-walk release, the halt, and throwing hooks. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015hPxMkC3jg3Uivh65gvaf2
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015hPxMkC3jg3Uivh65gvaf2
ryansolid
marked this pull request as ready for review
October 21, 2026 22:46
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before: a cleanup that throws (
onCleanupor an effect's returned cleanup) aborts the rest of disposal on both the client and the server. That skips the earlier cleanups on the same owner, any sibling subtrees not yet reached, every ancestor's cleanups, and effect cleanups. On the client the error also goes pastcreateErrorBoundaryand out offlush(). The pass that was disposing never commits, the view stays on the old branch, and the next flush throws again. On the server,renderToStream's final dispose skips the remaining cleanups (#4193's third expectation). Separately, a throw in an unfinished pass skipsrecompute's deferred-release drain, so lazy memos released before it stay linked (follow-up to #4176).After: every cleanup in the tree runs, and the first error is rethrown once teardown is done:
dispose()outside a flush rethrows it.flush()rethrows it after the flush has finished, so commits and effects are never left half-applied.unobservedhook is handled the same way, so it can't stop a teardown halfway or leave a pass's release collector installed.dispose().disposeOwnerruns every cleanup, and fix(web): renderToStream serializer and cleanup errors reach the ambient error hook (#4193) #4206'srender/failedreport still fires.Routing the error to the nearest boundary so its fallback shows was tried. It trips the infinite-loop guard, because the boundary re-reads a memo that then re-runs cleanly. So the error is rethrown rather than caught.
How: a single guarded
runCleanupinowner.tswraps the_disposallist, the effect_cleanup, and userunobservedhooks. It keeps the first error and schedules a flush. Only root dispose (disposeRootSelf) andflushrethrow, which keeps the check offdisposeChildren's hot path. Putting it there cost about 10% on CodSpeed'screateOwners.haltReactivitydrops a collected error. Thetry/finallyinrecomputeis gone. One try around the whole cleanup loop (resuming after a throw) was benchmarked against the per-cleanup guard: same speed in a create/dispose bench, and 33 B larger, so the per-cleanup guard stays. On the server,disposeOwnerguards each child walk and each cleanup.Tests:
packages/signals/tests/cleanup-throw-continues.test.ts(10 cases, including a lazy node released mid-walk, a halt dropping the error, and a throwingunobservedhook).deferred-release-cleanup-throw.test.ts: the fix(signals): restore deferred releases after cleanup throws #4176 follow-up case (from 9a6ca29, which cost +35 B on its own and is no longer needed) and a throwingunobservedhook during a pass's teardown.packages/solid/test/server/signals.spec.ts.Size-Exception: about +120 to +145 B minified / +4 to +100 B brotli on the client scenarios, approved by Ryan (2026-10-21) because a throwing cleanup must not leak or wedge the tree. Hello world (
app: compiled floor) is 9,919 B brotli in CI, under 10 KB. Caps for every grown scenario are re-recorded from CI's numbers.🤖 Generated with Claude Code
https://claude.ai/code/session_015hPxMkC3jg3Uivh65gvaf2