Skip to content

Commit 6ae1a1f

Browse files
committed
fix(webapp): named sentinel abort reasons + cite Node issues
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).
1 parent c2e6a2d commit 6ae1a1f

3 files changed

Lines changed: 39 additions & 18 deletions

File tree

.server-changes/fix-sse-memory-leak.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,4 @@ area: webapp
33
type: fix
44
---
55

6-
Fix memory leak where every aborted SSE connection and every successful HTML render pinned the full request/response graph on Node 20, caused by `AbortSignal.any` + string abort reasons in `sse.ts` and an un-cleared `setTimeout(abort)` in `entry.server.tsx`.
6+
Fix memory leak where every aborted SSE connection pinned the full request/response graph on Node 20, caused by `AbortSignal.any()` in `sse.ts` retaining its source signals indefinitely (see nodejs/node#54614, nodejs/node#55351). Also clear the `setTimeout(abort)` timer in `entry.server.tsx` so successful HTML renders don't pin the React tree for 30s per request.

apps/webapp/app/presenters/v3/RunStreamPresenter.server.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { type PrismaClient, prisma } from "~/db.server";
22
import { logger } from "~/services/logger.server";
33
import { singleton } from "~/utils/singleton";
4-
import { createSSELoader, SendFunction } from "~/utils/sse";
4+
import { ABORT_REASON_SEND_ERROR, createSSELoader, SendFunction } from "~/utils/sse";
55
import { throttle } from "~/utils/throttle";
66
import { tracePubSub } from "~/v3/services/tracePubSub.server";
77

@@ -66,10 +66,10 @@ export class RunStreamPresenter {
6666
});
6767
}
6868
}
69-
// Abort the stream on send error. No reason string — string reasons
70-
// create a DOMException whose stack trace captures the surrounding
71-
// closure (see sse.ts comment).
72-
context.controller.abort();
69+
// Abort the stream on send error. Uses a stackless string sentinel
70+
// from sse.ts — a no-arg abort() would create a DOMException with a
71+
// stack trace, which is unnecessary retention on the signal.reason.
72+
context.controller.abort(ABORT_REASON_SEND_ERROR);
7373
}
7474
},
7575
1000

apps/webapp/app/utils/sse.ts

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,20 @@ type SSEOptions = {
3838
// This is used to track the open connections, for debugging
3939
const connections: Set<string> = new Set();
4040

41+
// Stackless sentinel reasons passed to AbortController#abort. Calling .abort()
42+
// with no argument produces a DOMException that captures a ~500-byte stack
43+
// trace; a string reason is stored verbatim with no stack. The choice of
44+
// reason type does not cause the retention we saw in prod (that was the
45+
// AbortSignal.any composite — see comment near the timeoutTimer below for the
46+
// Node issue refs), but naming the sentinels keeps call sites readable and
47+
// lets future signal.reason consumers branch on the cause.
48+
export const ABORT_REASON_REQUEST = "request_aborted";
49+
export const ABORT_REASON_TIMEOUT = "timeout";
50+
export const ABORT_REASON_SEND_ERROR = "send_error";
51+
export const ABORT_REASON_INIT_STOP = "init_requested_stop";
52+
export const ABORT_REASON_ITERATOR_STOP = "iterator_requested_stop";
53+
export const ABORT_REASON_ITERATOR_ERROR = "iterator_error";
54+
4155
export function createSSELoader(options: SSEOptions) {
4256
const { timeout, interval = 500, debug = false, handler } = options;
4357

@@ -70,7 +84,7 @@ export function createSSELoader(options: SSEOptions) {
7084
// up immediately. Otherwise a send-failure in initStream leaves them
7185
// alive until `timeout` fires.
7286
if (!internalController.signal.aborted) {
73-
internalController.abort();
87+
internalController.abort(ABORT_REASON_SEND_ERROR);
7488
}
7589
throw error;
7690
}
@@ -97,18 +111,25 @@ export function createSSELoader(options: SSEOptions) {
97111

98112
log("Start");
99113

100-
// Single-signal abort chain: everything rolls up into internalController with NO
101-
// string reasons (string reasons create a DOMException whose stack trace pins the
102-
// closure graph). Timeout is a plain setTimeout cleared on abort rather than an
103-
// AbortSignal.timeout() combined via AbortSignal.any(); both of those patterns
104-
// leak on Node 20 due to FinalizationRegistry tracking of dependent signals.
114+
// Single-signal abort chain: everything rolls up into internalController.
115+
// Timeout is a plain setTimeout cleared on abort rather than an
116+
// AbortSignal.timeout() combined via AbortSignal.any() — AbortSignal.any
117+
// keeps its source signals in an internal Set<WeakRef> managed by a
118+
// FinalizationRegistry, and under sustained request traffic those entries
119+
// accumulate faster than they get cleaned up, pinning every source signal
120+
// (and its listeners, and anything those listeners close over) until the
121+
// parent signal is GC'd or aborts. Reproduced locally in isolation; shape
122+
// matches the ChainSafe Lodestar production case described in
123+
// nodejs/node#54614. See also nodejs/node#55351 (mechanism confirmed by
124+
// @jasnell, narrow fix in 22.12.0 via #55354) and nodejs/node#57584
125+
// (circular-dep variant, still open).
105126
const timeoutTimer = setTimeout(() => {
106-
if (!internalController.signal.aborted) internalController.abort();
127+
if (!internalController.signal.aborted) internalController.abort(ABORT_REASON_TIMEOUT);
107128
}, timeout);
108129

109130
const onRequestAbort = () => {
110131
log("request signal aborted");
111-
if (!internalController.signal.aborted) internalController.abort();
132+
if (!internalController.signal.aborted) internalController.abort(ABORT_REASON_REQUEST);
112133
};
113134

114135
internalController.signal.addEventListener(
@@ -133,7 +154,7 @@ export function createSSELoader(options: SSEOptions) {
133154
const shouldContinue = await handlers.beforeStream();
134155
if (shouldContinue === false) {
135156
log("beforeStream returned false, so we'll exit before creating the stream");
136-
internalController.abort();
157+
internalController.abort(ABORT_REASON_INIT_STOP);
137158
return;
138159
}
139160
}
@@ -149,7 +170,7 @@ export function createSSELoader(options: SSEOptions) {
149170
const shouldContinue = await handlers.initStream({ send: safeSend });
150171
if (shouldContinue === false) {
151172
log("initStream returned false, so we'll stop the stream");
152-
internalController.abort();
173+
internalController.abort(ABORT_REASON_INIT_STOP);
153174
return;
154175
}
155176
}
@@ -167,7 +188,7 @@ export function createSSELoader(options: SSEOptions) {
167188
const shouldContinue = await handlers.iterator({ date, send: safeSend });
168189
if (shouldContinue === false) {
169190
log("iterator return false, so we'll stop the stream");
170-
internalController.abort();
191+
internalController.abort(ABORT_REASON_ITERATOR_STOP);
171192
break;
172193
}
173194
} catch (error) {
@@ -176,7 +197,7 @@ export function createSSELoader(options: SSEOptions) {
176197
if (error instanceof Error && error.name !== "AbortError") {
177198
log(`iterator error: ${error.message}`);
178199
}
179-
internalController.abort();
200+
internalController.abort(ABORT_REASON_ITERATOR_ERROR);
180201
// No need to re-throw as we're handling it by aborting
181202
return; // Exit the run function immediately
182203
}

0 commit comments

Comments
 (0)