fix(webapp): eliminate SSE abort-signal memory leak#3430
Conversation
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a server changelog entry and fixes an SSE memory leak by reworking abort/cancellation wiring: removes Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
AbortSignal.any() combined with abort(reason) on Node 20 pinned the full Express request/response graph via a DOMException stack trace and a FinalizationRegistry. Every aborted SSE connection leaked ~60KB (ServerResponse, IncomingMessage, Socket, 4x AbortController, SpanImpl) that survived GC indefinitely. - sse.ts: collapse the 4-signal stack to a single internalController.signal; replace AbortSignal.timeout() with a plain setTimeout cleared on abort; drop every string arg from .abort() so no DOMException stack is captured; removeEventListener the request-abort handler on cleanup. - entry.server.tsx: clear the setTimeout(abort, ABORT_DELAY) in every terminal callback so the abort closure does not pin the React render tree + remixContext for 30s per successful request (react-router #14200). - tracer.server.ts: gate HttpInstrumentation and ExpressInstrumentation behind DISABLE_HTTP_INSTRUMENTATION=true for isolation testing; defaults to enabled. Verified locally (500 aborted SSE connections + GC): - unpatched: +16.0 MB heap, 158 memlab leak clusters - patched: +3.3 MB heap, 0 app-code leaks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ericallam
force-pushed
the
fix/webapp-sse-memory-leak
branch
from
4e9d0c5 to
5a9563a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/webapp/app/v3/tracer.server.ts (1)
310-312:⚠️ Potential issue | 🟡 MinorRoute
DISABLE_HTTP_INSTRUMENTATIONthroughenv.server.ts.Line 310 still bypasses the webapp env schema. Add
DISABLE_HTTP_INSTRUMENTATIONto theenvexport, then useenv.DISABLE_HTTP_INSTRUMENTATIONhere.Suggested local change
- if (process.env.DISABLE_HTTP_INSTRUMENTATION !== "true") { + if (env.DISABLE_HTTP_INSTRUMENTATION !== "true") { instrumentations.unshift(new HttpInstrumentation(), new ExpressInstrumentation()); }As per coding guidelines, “Access environment variables via
envexport fromapp/env.server.ts. Never useprocess.envdirectly”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/v3/tracer.server.ts` around lines 310 - 312, Add DISABLE_HTTP_INSTRUMENTATION to the env export in app/env.server.ts (the same env object used throughout the app) and then replace the direct process.env access in tracer.server.ts with env.DISABLE_HTTP_INSTRUMENTATION; update any associated types or schema in env.server.ts (e.g., the exported Env interface or zod/schema validator) so the new key is typed/validated, and change the conditional in instrumentations.unshift(...) to check env.DISABLE_HTTP_INSTRUMENTATION === "true".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/webapp/app/v3/tracer.server.ts`:
- Around line 310-312: Add DISABLE_HTTP_INSTRUMENTATION to the env export in
app/env.server.ts (the same env object used throughout the app) and then replace
the direct process.env access in tracer.server.ts with
env.DISABLE_HTTP_INSTRUMENTATION; update any associated types or schema in
env.server.ts (e.g., the exported Env interface or zod/schema validator) so the
new key is typed/validated, and change the conditional in
instrumentations.unshift(...) to check env.DISABLE_HTTP_INSTRUMENTATION ===
"true".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b2a38674-b80b-4e0a-876b-26c8e6f88736
📒 Files selected for processing (4)
.server-changes/fix-sse-memory-leak.mdapps/webapp/app/entry.server.tsxapps/webapp/app/utils/sse.tsapps/webapp/app/v3/tracer.server.ts
✅ Files skipped from review due to trivial changes (1)
- apps/webapp/app/entry.server.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- .server-changes/fix-sse-memory-leak.md
- apps/webapp/app/utils/sse.ts
📜 Review details
⏰ 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). (28)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
apps/webapp/app/v3/tracer.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/v3/tracer.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Add crumbs as you write code using
//@Crumbscomments or `// `#region` `@crumbsblocks. These are temporary debug instrumentation and must be stripped usingagentcrumbs stripbefore merge.
Files:
apps/webapp/app/v3/tracer.server.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/v3/tracer.server.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/v3/tracer.server.ts
**/*.ts{,x}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from
@trigger.dev/sdkwhen writing Trigger.dev tasks. Never use@trigger.dev/sdk/v3or deprecatedclient.defineJob.
Files:
apps/webapp/app/v3/tracer.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: Access environment variables through theenvexport ofenv.server.tsinstead of directly accessingprocess.env
Use subpath exports from@trigger.dev/corepackage instead of importing from the root@trigger.dev/corepathUse named constants for sentinel/placeholder values (e.g.
const UNSET_VALUE = '__unset__') instead of raw string literals scattered across comparisons
Files:
apps/webapp/app/v3/tracer.server.ts
apps/webapp/**/*.server.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
apps/webapp/**/*.server.ts: Never userequest.signalfor detecting client disconnects. UsegetRequestAbortSignal()fromapp/services/httpAsyncStorage.server.tsinstead, which is wired directly to Expressres.on('close')and fires reliably
Access environment variables viaenvexport fromapp/env.server.ts. Never useprocess.envdirectly
Always usefindFirstinstead offindUniquein Prisma queries.findUniquehas an implicit DataLoader that batches concurrent calls and has active bugs even in Prisma 6.x (uppercase UUIDs returning null, composite key SQL correctness issues, 5-10x worse performance).findFirstis never batched and avoids this entire class of issues
Files:
apps/webapp/app/v3/tracer.server.ts
🧠 Learnings (19)
📓 Common learnings
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3399
File: apps/webapp/app/services/realtime/redisRealtimeStreams.server.ts:282-291
Timestamp: 2026-04-16T14:07:46.808Z
Learning: In `apps/webapp/app/services/realtime/redisRealtimeStreams.server.ts` (`streamResponse`), the pattern `signal.addEventListener("abort", cleanup, { once: true })` does NOT need an explicit `removeEventListener` call in the non-abort cleanup paths (inactivity, cancel). The `AbortController` is per-request, scoped to `httpAsyncStorage` (created in `apps/webapp/server.ts` per-request middleware), so it gets GC'd when the request ends — taking the listener and closure with it. The `isCleanedUp` guard prevents double-execution, and `redis.disconnect()` is called before the request ends. Do not flag this as a listener/closure leak.
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/**/*.server.ts : Never use `request.signal` for detecting client disconnects. Use `getRequestAbortSignal()` from `app/services/httpAsyncStorage.server.ts` instead, which is wired directly to Express `res.on('close')` and fires reliably
📚 Learning: 2026-03-25T15:29:25.889Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2026-03-25T15:29:25.889Z
Learning: Applies to trigger.config.ts : Configure telemetry instrumentations and exporters in the `telemetry` option
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2026-03-02T12:43:25.254Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/run-engine/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:25.254Z
Learning: Applies to internal-packages/run-engine/src/engine/systems/**/*.ts : Integrate OpenTelemetry tracer and meter instrumentation in RunEngine systems for observability
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2026-04-13T21:44:00.032Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/app/services/taskIdentifierRegistry.server.ts:24-67
Timestamp: 2026-04-13T21:44:00.032Z
Learning: In `apps/webapp/app/services/taskIdentifierRegistry.server.ts`, the sequential upsert/updateMany/findMany writes in `syncTaskIdentifiers` are intentionally NOT wrapped in a Prisma transaction. This function runs only during deployment-change events (low-concurrency path), and any partial `isInLatestDeployment` state is acceptable because it self-corrects on the next deployment. Do not flag this as a missing-transaction/atomicity issue in future reviews.
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2026-04-15T15:39:31.575Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2026-04-15T15:39:31.575Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2026-04-16T14:19:16.330Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/**/*.server.ts : Access environment variables via `env` export from `app/env.server.ts`. Never use `process.env` directly
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2026-04-15T15:39:31.575Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2026-04-15T15:39:31.575Z
Learning: Applies to apps/webapp/**/*.test.{ts,tsx} : Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2025-08-14T18:35:44.370Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2390
File: apps/webapp/app/env.server.ts:764-765
Timestamp: 2025-08-14T18:35:44.370Z
Learning: The BoolEnv helper in apps/webapp/app/utils/boolEnv.ts uses z.preprocess with inconsistent default value types across the codebase - some usages pass boolean defaults (correct) while others pass string defaults (incorrect), leading to type confusion. The helper should enforce boolean-only defaults or have clearer documentation.
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2026-03-27T18:11:57.032Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3114
File: apps/supervisor/src/index.ts:83-98
Timestamp: 2026-03-27T18:11:57.032Z
Learning: In `apps/supervisor/src/index.ts`, `RESOURCE_MONITOR_ENABLED` (env var in `apps/supervisor/src/env.ts`) defaults to `false`. As a result, the local `ResourceMonitor`-based `maxResources`/`skipDequeue` gating in `preDequeue` is inactive in compute mode deployments. Do not flag local resource monitor usage in compute mode as a live bug; it has no practical impact unless `RESOURCE_MONITOR_ENABLED` is explicitly set to `true`.
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2025-08-19T09:49:07.011Z
Learnt from: julienvanbeveren
Repo: triggerdotdev/trigger.dev PR: 2417
File: apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.import.ts:56-61
Timestamp: 2025-08-19T09:49:07.011Z
Learning: In the Trigger.dev codebase, environment variables should default to `isSecret: false` when not explicitly marked as secrets in the syncEnvVars functionality. This is the intended behavior for both regular variables and parent variables.
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2026-04-16T14:19:16.330Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/**/*.test.{ts,tsx} : For testable code, never import `env.server.ts` in test files. Pass configuration as options instead (e.g., `realtimeClient.server.ts` takes config as constructor arg, `realtimeClientGlobal.server.ts` creates singleton with env config)
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2026-04-16T13:45:22.317Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/test/engine/taskIdentifierRegistry.test.ts:3-19
Timestamp: 2026-04-16T13:45:22.317Z
Learning: In `apps/webapp/test/engine/taskIdentifierRegistry.test.ts`, the `vi.mock` calls for `~/services/taskIdentifierCache.server` (stubbing `getTaskIdentifiersFromCache` and `populateTaskIdentifierCache`), `~/models/task.server` (stubbing `getAllTaskIdentifiers`), and `~/db.server` (stubbing `prisma` and `$replica`) are intentional. The suite uses real Postgres via testcontainers for all `TaskIdentifier` DB operations, but isolates the Redis cache layer and legacy query fallback as separate concerns not exercised in this test file. Do not flag these mocks as violations of the no-mocks policy in future reviews.
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2026-03-02T12:42:47.652Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/supervisor/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:42:47.652Z
Learning: Applies to apps/supervisor/src/env.ts : Environment configuration should be defined in `src/env.ts`
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2025-11-10T09:09:07.399Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2663
File: apps/webapp/app/env.server.ts:1205-1206
Timestamp: 2025-11-10T09:09:07.399Z
Learning: In the trigger.dev webapp, S2_ACCESS_TOKEN and S2_DEPLOYMENT_LOGS_BASIN_NAME environment variables must remain optional until an OSS version of S2 is available, to avoid breaking environments that don't have S2 provisioned.
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2026-04-07T14:12:18.946Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3331
File: apps/webapp/test/engine/batchPayloads.test.ts:5-24
Timestamp: 2026-04-07T14:12:18.946Z
Learning: In `apps/webapp/test/engine/batchPayloads.test.ts`, using `vi.mock` for `~/v3/objectStore.server` (stubbing `hasObjectStoreClient` and `uploadPacketToObjectStore`), `~/env.server` (overriding offload thresholds), and `~/v3/tracer.server` (stubbing `startActiveSpan`) is intentional and acceptable. Simulating controlled transient upload failures (e.g., fail N times then succeed) to verify `p-retry` behavior cannot be reproduced with real services or testcontainers. This file is an explicit exception to the repo's general no-mocks policy.
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2026-04-16T14:19:16.330Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : Use named constants for sentinel/placeholder values (e.g. `const UNSET_VALUE = '__unset__'`) instead of raw string literals scattered across comparisons
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2026-03-22T13:26:12.060Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/code/TextEditor.tsx:81-86
Timestamp: 2026-03-22T13:26:12.060Z
Learning: In the triggerdotdev/trigger.dev codebase, do not flag `navigator.clipboard.writeText(...)` calls for `missing-await`/`unhandled-promise` issues. These clipboard writes are intentionally invoked without `await` and without `catch` handlers across the project; keep that behavior consistent when reviewing TypeScript/TSX files (e.g., usages like in `apps/webapp/app/components/code/TextEditor.tsx`).
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2026-03-22T19:24:14.403Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts:200-204
Timestamp: 2026-03-22T19:24:14.403Z
Learning: In the triggerdotdev/trigger.dev codebase, webhook URLs are not expected to contain embedded credentials/secrets (e.g., fields like `ProjectAlertWebhookProperties` should only hold credential-free webhook endpoints). During code review, if you see logging or inclusion of raw webhook URLs in error messages, do not automatically treat it as a credential-leak/secrets-in-logs issue by default—first verify the URL does not contain embedded credentials (for example, no username/password in the URL, no obvious secret/token query params or fragments). If the URL is credential-free per this project’s conventions, allow the logging.
Applied to files:
apps/webapp/app/v3/tracer.server.ts
📚 Learning: 2026-03-29T19:16:28.864Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3291
File: apps/webapp/app/v3/featureFlags.ts:53-65
Timestamp: 2026-03-29T19:16:28.864Z
Learning: When reviewing TypeScript code that uses Zod v3, treat `z.coerce.*()` schemas as their direct Zod type (e.g., `z.coerce.boolean()` returns a `ZodBoolean` with `_def.typeName === "ZodBoolean"`) rather than a `ZodEffects`. Only `.preprocess()`, `.refine()`/`.superRefine()`, and `.transform()` are expected to wrap schemas in `ZodEffects`. Therefore, in reviewers’ logic like `getFlagControlType`, do not flag/unblock failures that require unwrapping `ZodEffects` when the input schema is a `z.coerce.*` schema.
Applied to files:
apps/webapp/app/v3/tracer.server.ts
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Switch DISABLE_HTTP_INSTRUMENTATION from z.string().default("false") to
BoolEnv.default(false) for consistency with other boolean env flags, and
update the tracer check to use the parsed boolean directly.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The same internalController is exposed to handlers via context.controller.
RunStreamPresenter was still calling .abort("Send error"), which creates
a DOMException with a stack trace on signal.reason. Without AbortSignal.any
this no longer pins the closure graph, but it's inconsistent with the rest
of the fix and worth cleaning up.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
After verifying the mechanism with a standalone isolation test (see PR),
AbortSignal.any's dependent-signal tracking is the sole cause of the leak;
the reason type (.abort() vs .abort("string")) is not. Keeping the signal
chain collapsed to a single AbortController is what eliminates retention.
- Add named sentinel constants (ABORT_REASON_*) for readability and to
satisfy the CLAUDE.md named-constant rule.
- Route RunStreamPresenter's send-error abort through the shared
ABORT_REASON_SEND_ERROR sentinel.
- Update the sse.ts comment to cite nodejs/node#54614 (production shape
reported by ChainSafe Lodestar in the same retention pattern) and
nodejs/node#55351 (mechanism confirmed by @jasnell, narrow fix
shipped in 22.12.0 via #55354).
- Correct the .server-changes entry: the leak is caused by AbortSignal.any,
not by the string abort reasons (the earlier phrasing conflated the
two).
2 participants
Summary
Fixes a server-side memory leak in the webapp's SSE helper. Every aborted SSE connection (client tab close, navigation, timeout) was pinning its full request/response graph indefinitely on Node 20, so any long-running webapp process accumulated retained memory proportional to streaming-request churn.
Root cause
apps/webapp/app/utils/sse.tscombined four abort signals viaAbortSignal.any([requestAbortSignal, timeoutSignal, internalController.signal]). The composite signal tracks its source signals in an internalSet<WeakRef>registered against aFinalizationRegistry; under sustained traffic those entries accumulate faster than they're cleaned up, pinning every source signal (and its listeners, and anything those listeners close over) until the parent signal itself is GC'd or aborts.This is a long-standing Node issue with multiple open reports:
AbortSignal.any, compose manually.A separate issue in
apps/webapp/app/entry.server.tsx—setTimeout(abort, ABORT_DELAY)with noclearTimeouton success paths — kept the React render tree +remixContextalive for 30s per successful HTML request. Same pattern fixed upstream in React Router templates (react-router#14200), never backported to Remix v2.What changed
apps/webapp/app/utils/sse.ts— single-signal abort chain.AbortSignal.anyremoved;AbortSignal.timeoutreplaced by a plainsetTimeoutcleared when the controller aborts; named sentinel constants used as stackless abort reasons; request-abort handler explicitly removed on cleanup.apps/webapp/app/entry.server.tsx— clears thesetTimeout(abort, ABORT_DELAY)timer inonShellReady/onAllReady/onShellError.apps/webapp/app/v3/tracer.server.ts+env.server.ts— gates OpenTelemetryHttpInstrumentationandExpressInstrumentationbehindDISABLE_HTTP_INSTRUMENTATION=trueas an escape hatch for future OTel-listener retention patterns. Defaults to enabled.apps/webapp/app/presenters/v3/RunStreamPresenter.server.ts— uses the sharedABORT_REASON_SEND_ERRORsentinel.Verification
Full-app reproduction (memlab)
Isolated local harness, 500 abrupt SSE disconnects against a dev-presence route, GC between passes, heap snapshot diff with memlab:
ServerResponse, 1000AbortController, 250SpanImplretainedStandalone mechanism isolation
To confirm which axis of the change is load-bearing, a separate standalone Node script (
/tmp/abort-leak-test.mjs) ran 2000 requests × 200 KB payload per variant:AbortSignal.any+ string abort reasonAbortSignal.anyonly (no reason)AbortSignal.any)AbortSignal.anywith no listener on the compositeThis proves
AbortSignal.anyis the sole mechanism. The reason type (.abort()vs.abort("string")) is irrelevant for retention — V3 is clean, V5 leaks even without a listener on the composite.Risk
sse.tsis used by the dev-presence routes. Behaviour is equivalent — timeouts and client disconnects still abort the stream.signal.reasonis now a named string sentinel ("timeout","request_aborted", etc.) instead of the previous string arg or defaultAbortError. No in-tree reader ofsignal.reasonexists.entry.server.tsxchange is a standard cleanup of an abort timer, matches upstream React Router guidance.tracer.server.tschange is env-gated and defaults to current behaviour.AbortSignal.timeout()callsites (alert delivery, remote-build status) are fire-and-forget passed directly tofetch— not composed with anything long-lived, no retention risk, untouched.Test plan
Follow-up
The same
AbortSignal.any([userSignal, internalSignal])pattern exists in several SDK/core callsites that ship to customers (packages/core/src/v3/realtimeStreams/manager.ts,packages/trigger-sdk/src/v3/{ai,chat,chat-client,sessions}.ts,packages/core/src/v3/workers/warmStartClient.ts). Whether those leak in practice depends on the user passing a long-lived signal. Tracked separately.