fix: memory leaks #5896
fix: memory leaks #5896
Conversation
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 8m 46s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 3s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-11-19 00:27:10 UTC
|
Caution Review failedThe pull request is closed. WalkthroughReworks the server start flow to build a per-request router via getRouter, merge startOptions with serialization adapters (including ServerFunctionSerializationAdapter), run a middleware pipeline that prioritizes server functions, perform SSR dehydration and header assembly, validate/serialize redirects (x-tsr-redirect), centralize error handling, and reset router on completion. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as createStartHandler
participant ReqOpts as requestStartOptions
participant Router
participant Context as runWithStartContext
participant Middleware as Middleware Pipeline
participant ServerFn as Server Functions
participant RouterExec as executeRouter
participant SSR as SSR Dehydrate
Client->>Handler: incoming HTTP request
Handler->>ReqOpts: compute requestStartOptions (merge adapters)
Handler->>Router: getRouter(ReqOpts)
Router-->>Handler: configured router
rect rgb(245,250,255)
Handler->>Context: runWithStartContext(request)
Context->>Middleware: execute middleware chain
Middleware->>ServerFn: attempt server-function handling first
alt server-fn handled
ServerFn-->>Middleware: Response
else
Middleware->>RouterExec: executeRouter -> route resolution
RouterExec-->>Middleware: route result / redirect / error
end
end
alt Response ready
Middleware->>SSR: dehydrate SSR state + collect headers
SSR-->>Handler: final body + headers
Handler-->>Client: final response
else Redirect / Error
Middleware-->>Handler: redirect/error response
Handler-->>Client: redirect/error
end
Note over Handler,Router: finally -> reset router / cleanup
sequenceDiagram
participant Stream as PassthroughStream
participant Listener as InjectedHTMLListener
Stream->>Listener: subscribe (deferred assignment)
Client->>Stream: consume
alt Consumer cancels or times out
Stream--xListener: onCancel -> stopListeningToInjectedHtml()
end
Listener-->>Stream: stop/cleanup invoked
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/start-server-core/src/createStartHandler.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
packages/start-server-core/src/createStartHandler.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/start-server-core/src/createStartHandler.ts
🧬 Code graph analysis (1)
packages/start-server-core/src/createStartHandler.ts (5)
packages/router-core/src/ssr/ssr-server.ts (2)
getOrigin(227-239)attachRouterServerSsrUtils(100-225)packages/start-client-core/src/client/ServerFunctionSerializationAdapter.ts (1)
ServerFunctionSerializationAdapter(5-16)packages/start-storage-context/src/async-local-storage.ts (1)
runWithStartContext(15-20)packages/start-server-core/src/router-manifest.ts (1)
getStartManifest(10-63)packages/start-client-core/src/createServerFn.ts (2)
flattenMiddlewares(582-606)executeMiddleware(172-241)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (1)
packages/start-server-core/src/createStartHandler.ts (1)
83-125: Router lifecycle, SSR wiring, and redirect handling look solid.The refactored flow—lazy
getRouterinitialisation with memory history, per-requestrunWithStartContext, SSR setup viaattachRouterServerSsrUtils+dehydrate, and centralised redirect handling (including manualx-tsr-redirectresponses androuter.resolveRedirect) is coherent and addresses the previous global/patchy setup. Thefinallyblock that clearsrouter.serverSsrand drops the localrouterreference is an appropriate cleanup step for avoiding per-request SSR state retention.Also applies to: 137-234, 251-314
| const startOptions: AnyStartInstanceOptions = | ||
| (await (await getEntries()).startEntry.startInstance?.getOptions()) || | ||
| ({} as AnyStartInstanceOptions) | ||
| startOptions.serializationAdapters = | ||
| startOptions.serializationAdapters || [] | ||
| // insert start specific default serialization adapters | ||
| startOptions.serializationAdapters.push( | ||
| ServerFunctionSerializationAdapter, | ||
| ) |
There was a problem hiding this comment.
Avoid per-request growth of serializationAdapters (risk of memory leak).
startOptions.serializationAdapters.push(ServerFunctionSerializationAdapter) mutates the options object on every request. If startInstance.getOptions() returns a shared object (which is likely), this will append the same adapter indefinitely, growing the array and causing duplicated work.
Guard the push with a dedupe check so the adapter is only added once per options object:
- startOptions.serializationAdapters =
- startOptions.serializationAdapters || []
- // insert start specific default serialization adapters
- startOptions.serializationAdapters.push(
- ServerFunctionSerializationAdapter,
- )
+ startOptions.serializationAdapters =
+ startOptions.serializationAdapters || []
+ // insert start specific default serialization adapter once
+ if (
+ !startOptions.serializationAdapters.includes(
+ ServerFunctionSerializationAdapter,
+ )
+ ) {
+ startOptions.serializationAdapters.push(
+ ServerFunctionSerializationAdapter,
+ )
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const startOptions: AnyStartInstanceOptions = | |
| (await (await getEntries()).startEntry.startInstance?.getOptions()) || | |
| ({} as AnyStartInstanceOptions) | |
| startOptions.serializationAdapters = | |
| startOptions.serializationAdapters || [] | |
| // insert start specific default serialization adapters | |
| startOptions.serializationAdapters.push( | |
| ServerFunctionSerializationAdapter, | |
| ) | |
| const startOptions: AnyStartInstanceOptions = | |
| (await (await getEntries()).startEntry.startInstance?.getOptions()) || | |
| ({} as AnyStartInstanceOptions) | |
| startOptions.serializationAdapters = | |
| startOptions.serializationAdapters || [] | |
| // insert start specific default serialization adapter once | |
| if ( | |
| !startOptions.serializationAdapters.includes( | |
| ServerFunctionSerializationAdapter, | |
| ) | |
| ) { | |
| startOptions.serializationAdapters.push( | |
| ServerFunctionSerializationAdapter, | |
| ) | |
| } |
🤖 Prompt for AI Agents
In packages/start-server-core/src/createStartHandler.ts around lines 127 to 135,
the code mutates startOptions.serializationAdapters on every request which can
grow a shared array and leak memory; instead avoid mutating a potentially shared
options object by either deduplicating before pushing or by replacing the array
with a new one that includes the adapter only once: check if
startOptions.serializationAdapters already contains
ServerFunctionSerializationAdapter (by identity) and only add it if missing, or
assign startOptions.serializationAdapters =
[...(startOptions.serializationAdapters || []),
ServerFunctionSerializationAdapter] only when not present so you never append
duplicates to a shared array.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/start-server-core/src/createStartHandler.ts (3)
348-350: Critical: Route middleware chain can terminate prematurely when.options.serveris missing.Similar to the request middleware issue, this maps
d.options.serverwithout filtering, which can produce undefined entries that prematurely stop middleware execution.Apply this diff:
const middlewares = flattenMiddlewares( matchedRoutes.flatMap((r) => r.options.server?.middleware).filter(Boolean), - ).map((d) => d.options.server) + ) + .map((d) => d.options.server) + .filter(Boolean)
376-378: Critical: Handler middleware chain can terminate prematurely when.options.serveris missing.This middleware mapping also lacks filtering for undefined
serverhandlers, creating the same chain termination risk.Apply this diff:
if (middleware && middleware.length) { middlewares.push( - ...flattenMiddlewares(middleware).map((d) => d.options.server), + ...flattenMiddlewares(middleware) + .map((d) => d.options.server) + .filter(Boolean), ) }
240-243: Critical: Middleware chain can terminate prematurely when.options.serveris missing.The middleware pipeline maps through
d.options.serverwithout filtering out undefined entries. SinceexecuteMiddleware(line 444) treats a falsymiddlewareas "end of chain" (if (!middleware) return ctx), any entry whereoptions.serverisundefinedwill stop the pipeline and preventrequestHandlerMiddlewarefrom running.Apply this diff:
const flattenedMiddlewares = startOptions.requestMiddleware ? flattenMiddlewares(startOptions.requestMiddleware) : [] - const middlewares = flattenedMiddlewares.map((d) => d.options.server) + const middlewares = flattenedMiddlewares + .map((d) => d.options.server) + .filter(Boolean)
🧹 Nitpick comments (1)
packages/start-server-core/src/createStartHandler.ts (1)
257-268: Consider extracting duplicate manual redirect handling.The manual redirect response logic (lines 257-267 and 298-307) is duplicated. While the control flow is correct (handling already-resolved vs newly-resolved redirects), extracting this to a helper function would improve maintainability.
Example helper:
function createManualRedirectResponse(response: any) { return json( { ...response.options, isSerializedRedirect: true, }, { headers: response.headers, }, ) }Then replace both blocks with:
if (request.headers.get('x-tsr-redirect') === 'manual') { return createManualRedirectResponse(response) }Also applies to: 298-308
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/start-server-core/src/createStartHandler.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/start-server-core/src/createStartHandler.ts
🧬 Code graph analysis (1)
packages/start-server-core/src/createStartHandler.ts (6)
packages/router-core/src/ssr/ssr-server.ts (2)
getOrigin(227-239)attachRouterServerSsrUtils(100-225)packages/start-client-core/src/client/ServerFunctionSerializationAdapter.ts (1)
ServerFunctionSerializationAdapter(5-16)packages/start-server-core/src/index.tsx (2)
HEADERS(16-16)attachRouterServerSsrUtils(4-4)packages/start-storage-context/src/async-local-storage.ts (1)
runWithStartContext(15-20)packages/start-server-core/src/router-manifest.ts (1)
getStartManifest(10-63)packages/start-client-core/src/createServerFn.ts (2)
flattenMiddlewares(582-606)executeMiddleware(172-241)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (2)
packages/start-server-core/src/createStartHandler.ts (2)
94-102: Excellent fix for the serializationAdapters memory leak!Creating a new array with the spread operator instead of mutating the shared
startOptions.serializationAdaptersproperly addresses the memory leak concern from the previous review.
314-318: Good practice: router cleanup in finally block.Ensuring
routeris set tonullin the finally block helps prevent memory leaks by allowing garbage collection after each request completes or errors.
| return json( | ||
| { | ||
| error: 'Only HTML requests are supported here', | ||
| }, | ||
| { | ||
| status: 500, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Use HTTP 406 instead of 500 for Accept header mismatch.
A 500 status indicates a server error, but rejecting an unsupported media type is a client request issue. HTTP 406 (Not Acceptable) is the standard status code when the server cannot satisfy the Accept header.
Apply this diff:
return json(
{
error: 'Only HTML requests are supported here',
},
{
- status: 500,
+ status: 406,
},
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return json( | |
| { | |
| error: 'Only HTML requests are supported here', | |
| }, | |
| { | |
| status: 500, | |
| }, | |
| ) | |
| return json( | |
| { | |
| error: 'Only HTML requests are supported here', | |
| }, | |
| { | |
| status: 406, | |
| }, | |
| ) |
🤖 Prompt for AI Agents
In packages/start-server-core/src/createStartHandler.ts around lines 179 to 186,
the response for requests that don't accept HTML currently returns HTTP 500;
change the response status to 406 (Not Acceptable) to reflect that the client's
Accept header cannot be satisfied. Update the json response call to use status:
406 and keep the error payload the same so the client receives the correct
semantic HTTP status for an unsupported media type.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/ssr/transformStreamWithRouter.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (3)
packages/router-core/src/ssr/transformStreamWithRouter.ts (3)
38-48: LGTM! Clean cancellation propagation.The
onCancelparameter andcancel()handler provide a proper hook for cleanup when the stream is canceled, marking the passthrough as destroyed and propagating the cancellation signal.
141-143: LGTM! Proper subscription cleanup.Capturing the unsubscribe function in
stopListeningToInjectedHtmlenables proper cleanup of the event listener, preventing memory leaks from lingering subscriptions.
178-178: LGTM! Ensures cleanup on promise settlement.The
.finally()block ensures the subscription is cleaned up when the injected HTML promise settles, complementing the cancellation cleanup on line 106.Note: This means cleanup can occur via two paths (cancellation or promise settlement), but the optional chaining and typical unsubscribe idempotency make this safe.
schiller-manuel
force-pushed
the
fix-memory-leaks
branch
from
01557d4 to
08391dd
Compare
Summary by CodeRabbit
Chores
Bug Fixes
Refactor
New Features
Tests