fix(server-renderer): cleanup component effect scopes after SSR render#14548
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughServer renderer now records per-component effect scopes in the SSR context and ensures those scopes (and watcher handles) are cleaned up after render (both stream and string). Tests for effectScope lifecycle, concurrent render isolation, and detached-scope behavior were added (duplicated blocks present). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Renderer
participant SSRContext
participant Component
participant TeleportResolver
participant Cleaner
Client->>Renderer: request render (stream/string)
Renderer->>Component: render component VNode
Component->>SSRContext: push instance scope into __instanceScopes
Renderer->>TeleportResolver: resolve teleports / unroll buffer
TeleportResolver-->>Renderer: teleports resolved
Renderer->>Cleaner: finalize / finally -> cleanupContext(context)
Cleaner->>SSRContext: call watcher unwatch callbacks (if any)
Cleaner->>SSRContext: for each __instanceScopes -> call stop()
Cleaner->>SSRContext: clear __instanceScopes
Cleaner-->>Renderer: cleanup complete
Renderer-->>Client: return stream/string (then close)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/server-renderer/__tests__/render.spec.ts (1)
1007-1083: Add one failing-render regression for the new cleanup path.These cases only cover successful renders. Since the renderer change is specifically about
finally-based teardown, I'd add a component that registersonScopeDisposeand then throws duringsetup()or render, and assert that cleanup still fires.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-renderer/__tests__/render.spec.ts` around lines 1007 - 1083, Add a failing-render regression test that ensures onScopeDispose handlers still run when setup() or the render function throws: create an app via createApp where setup registers onScopeDispose (pushing to an array or setting a flag) then deliberately throws (or returns a render that throws), call render(app) expecting it to reject, and assert the cleanup ran (array contains entry or flag true). Place tests alongside the existing ones (use the same deferred pattern for concurrency if needed) so they exercise the finally-based teardown path and reference render, createApp, setup, onScopeDispose, and effectScope to locate the relevant test files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server-renderer/src/renderToStream.ts`:
- Around line 76-97: The current Promise chain calls renderComponentVNode(vnode)
before Promise.resolve(), so synchronous throws escape the chain; change the
call to invoke renderComponentVNode inside a thenable (e.g.,
Promise.resolve().then(() => renderComponentVNode(vnode))) so any sync
exceptions are routed to the chain's .catch and .finally; ensure the rest of the
chain still calls unrollBuffer(buffer, stream), resolveTeleports(context),
stream.push(null), and that .catch uses stream.destroy(error) and .finally
invokes cleanup.
---
Nitpick comments:
In `@packages/server-renderer/__tests__/render.spec.ts`:
- Around line 1007-1083: Add a failing-render regression test that ensures
onScopeDispose handlers still run when setup() or the render function throws:
create an app via createApp where setup registers onScopeDispose (pushing to an
array or setting a flag) then deliberately throws (or returns a render that
throws), call render(app) expecting it to reject, and assert the cleanup ran
(array contains entry or flag true). Place tests alongside the existing ones
(use the same deferred pattern for concurrency if needed) so they exercise the
finally-based teardown path and reference render, createApp, setup,
onScopeDispose, and effectScope to locate the relevant test files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61f8e0a7-eaab-4a37-b08e-9aefb19bd856
📒 Files selected for processing (4)
packages/server-renderer/__tests__/render.spec.tspackages/server-renderer/src/render.tspackages/server-renderer/src/renderToStream.tspackages/server-renderer/src/renderToString.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server-renderer/src/renderToStream.ts`:
- Around line 77-81: The loop calling each unwatch from context.__watcherHandles
drains the watchers but leaves the array intact which retains references; after
iterating and calling each unwatch() (context.__watcherHandles), clear the
handles to release request-local references — e.g. set context.__watcherHandles
to an empty array or delete the property once all unwatch() calls complete so
the stopped watcher state is no longer reachable.
- Around line 90-98: The current Promise chain puts cleanup in .finally(), which
can throw and bypass the .catch() that calls stream.destroy(error); instead,
declare a guarded flag (e.g., let cleaned = false) before the chain and remove
.finally(cleanup). Insert a guarded cleanup step into the main chain (e.g.,
.then(() => { if (!cleaned) { cleaned = true; cleanup(); } })) and also call the
same guarded cleanup from the .catch(error => { if (!cleaned) { cleaned = true;
cleanup(); } stream.destroy(error); throw error; }) so that any cleanup errors
are part of the same chain and all errors flow through the .catch that calls
stream.destroy; reference the existing symbols renderComponentVNode,
unrollBuffer, resolveTeleports, cleanup, stream.push(null), and stream.destroy
when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 176181c7-af66-4930-a0fd-a82eac97f8f3
📒 Files selected for processing (1)
packages/server-renderer/src/renderToStream.ts
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
Size ReportBundles
Usages
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server-renderer/src/render.ts`:
- Around line 65-77: The cleanup loop in cleanupContext currently stops early if
a single unwatch() or scope.stop() throws, leaving other resources uncleaned;
update cleanupContext so each call to functions on context.__watcherHandles
(unwatch) and context.__instanceScopes (scope.stop) is wrapped in its own
try/catch to ensure remaining handles/scopes are processed even if one
throws—optionally capture/log the exception (but do not rethrow) and continue;
keep the existing length resets (context.__watcherHandles.length = 0 and
context.__instanceScopes.length = 0) after iteration to ensure arrays are
cleared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42c7b372-0f26-4180-87b5-36a2b70ac23b
📒 Files selected for processing (5)
packages/server-renderer/__tests__/render.spec.tspackages/server-renderer/__tests__/ssrWatch.spec.tspackages/server-renderer/src/render.tspackages/server-renderer/src/renderToStream.tspackages/server-renderer/src/renderToString.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server-renderer/tests/render.spec.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/server-renderer/src/renderToStream.ts (1)
96-103:⚠️ Potential issue | 🟡 MinorOriginal render error is lost when cleanup also throws.
If
finalize()throws after a render failure, the original error is replaced withcleanupError, making the root cause harder to debug. Consider usingfinallyto preserve the original error:💡 Suggested fix
.catch(error => { try { finalize() - } catch (cleanupError) { - error = cleanupError + } finally { + stream.destroy(error) } - stream.destroy(error) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-renderer/src/renderToStream.ts` around lines 96 - 103, The current catch block replaces the original render error when finalize() throws; preserve the original error by capturing it into a separate variable (e.g., originalError = error) before calling finalize(), then call stream.destroy with the originalError if present, falling back to cleanupError only if originalError is falsy; alternatively, move finalize() into a finally block so stream.destroy(error) is always invoked with the original render error. Update the code around finalize() and stream.destroy() to stop assigning cleanupError to the original error variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/server-renderer/src/renderToStream.ts`:
- Around line 96-103: The current catch block replaces the original render error
when finalize() throws; preserve the original error by capturing it into a
separate variable (e.g., originalError = error) before calling finalize(), then
call stream.destroy with the originalError if present, falling back to
cleanupError only if originalError is falsy; alternatively, move finalize() into
a finally block so stream.destroy(error) is always invoked with the original
render error. Update the code around finalize() and stream.destroy() to stop
assigning cleanupError to the original error variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 217b17ba-ab17-4092-aef5-7ae8c4ee6d83
📒 Files selected for processing (1)
packages/server-renderer/src/renderToStream.ts
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server-renderer/src/renderToStream.ts`:
- Around line 96-103: The catch block currently calls stream.destroy twice when
finalize() throws (once with cleanupError and again with the original error),
which can mask the root cause; modify the error handling in the .catch handler
around finalize() so you only call stream.destroy once—prefer passing the most
relevant error (e.g., cleanupError if finalize() throws, otherwise the original
error) or combine them (attach the original error as a property/cause) before a
single stream.destroy call; adjust the closure that references finalize() and
stream.destroy to implement this single-destroy strategy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a211adb2-d1be-44f6-913d-b78276125655
📒 Files selected for processing (2)
packages/server-renderer/src/render.tspackages/server-renderer/src/renderToStream.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@SaeedNezafat The current tests in this PR mainly validate behavior caused by calling Also, If there is still a real memory leak on the Vue side after that fix, please provide a minimal reproduction that demonstrates it clearly. Ideally this should show either:
Without such a reproduction, I think we should revert #14548 and revisit this with a narrower fix. |
|
Following up on this issue, I took some time to re-investigate it more thoroughly across different Vue/Nuxt versions, using stronger instrumentation. In earlier versions, I observed problematic SSR behavior in a real Nuxt application: reactive resources appeared to accumulate across requests. Applying a custom patch (based on AsyncLocalStorage + explicit Looking back, that behavior makes sense in context:
However, after re-testing on newer versions (including upstream async-context fixes), and upgrading the probe to use GC-based liveness tracking (WeakRef + FinalizationRegistry + forced GC), the picture is now clearer:
At this point, I no longer have strong evidence of a persistent SSR heap leak in the current setup. The earlier signal was largely influenced by callback-based tracking ( My previous patch did improve behavior in practice, but it effectively introduced an explicit teardown phase and triggered InterpretationThe most accurate interpretation seems to be:
Open questionGiven this, I’d be interested to clarify:
Additional observationFrom experimenting with explicit scope cleanup, it seems that introducing an internal, request-scoped cleanup phase could potentially provide some practical benefits in certain scenarios:
Of course, this would need to be carefully designed to avoid changing user-observable behavior (e.g. not triggering Thanks again for the earlier feedback — it helped narrow down the problem much more precisely. |
|
@SaeedNezafat Based on what you shared here, my understanding is that there is no longer strong evidence of a persistent SSR heap leak in the current setup. If that is the case, then this seems closer to an SSR lifecycle / semantics difference than to a confirmed memory leak. On the open questions you raised, my current view is:
Given the current conclusion, I still think the fix in #14548 is too broad, because using If there is still a real Vue-side SSR leak after #14445 and the async-context fixes, I think the better path would be to provide a minimal reproduction first and then design a narrower fix around that. Otherwise, I would lean toward reverting #14548 and discussing any potential internal request-scoped cleanup mechanism separately. |
|
Hello @edison1105 I have a minimal reproduction of a memory leak in vue 3.5.32 |
Problem
SSR component instance scopes are created during render, but unlike client rendering they are not explicitly stopped because SSR does not run the normal unmount path.
This can retain scope-bound effects and cleanup callbacks beyond the request lifetime.
Related to nuxt/nuxt#33644, which reports the issue as an upstream Vue bug.
Solution
Track SSR component instance scopes on the request-local SSR context and stop them after render finalization.
This keeps cleanup:
Tests
Added tests for:
Related
A workaround based on SSR scope cleanup was previously suggested in that issue:
nuxt/nuxt#33644 (comment)
Another user reported that the workaround resolved the issue in their Nuxt setup:
nuxt/nuxt#33644 (comment)
This PR attempts to address the underlying problem directly in Vue's server renderer.
If needed, I can rebase or adapt this against any existing upstream work linked from nuxt/nuxt#33644.
Summary by CodeRabbit
Tests
Bug Fixes