fix(proxy): clean up combined abort signal listeners#1121
Conversation
The AbortSignal.any polyfill in forwarder.ts attached `abort` listeners
to source signals (responseController.signal and session.clientAbortSignal)
with `{ once: true }`, but `once: true` only auto-detaches when the abort
event actually fires. When a request completes normally — the common case —
those listeners were never removed and their closures kept holding the
combinedController, the cleanup array, and (transitively) the session and
the request body, preventing GC.
This is the same leak pattern as ding113#1113 fixed for the client abort listener
in `_handleStreamingHedge`; the polyfill branch in
`forwardSingleAttempt` was not covered by that PR.
Extract the polyfill into `combine-abort-signals.ts` returning an explicit
`{ signal, cleanup }` pair. The native `AbortSignal.any` branch returns a
noop cleanup since V8 owns the listener lifecycle.
In `forwardSingleAttempt`:
- Call cleanup in the fetch failure catch path (response-handler never runs).
- Call cleanup in the inner `try { throw fromUpstreamResponse } finally`
path used for HTTP 4xx/5xx errors (response-handler also skipped here).
- Compose cleanup into the `sessionWithTimeout.releaseAgent` callback so
the existing response-handler finally invocation point handles the
successful path without touching every call site. Cleanup is idempotent
via an internal `cleaned` flag.
Add unit tests covering both branches: native delegation, polyfill abort
propagation, listener detachment after explicit cleanup, automatic
cleanup on source abort, and pre-aborted source signals.
📝 WalkthroughWalkthrough此 PR 引入了一个集中式的 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 unit tests (beta)
⚔️ Resolve merge conflicts
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 |
github-actions
Bot
added
bug
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d89f91c5ef
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Polyfill 路径上需要主动解绑源信号的 abort listener（response-handler 不会执行）。 | ||
| cleanupCombinedSignal(); |
There was a problem hiding this comment.
In forwardSingleAttempt, this unconditional cleanupCombinedSignal() runs at catch entry even though the same catch block can continue with HTTP/2→HTTP/1.1 or proxy→direct fallback fetches. In the polyfill path, cleanup detaches responseController/clientAbortSignal listeners from the already-created init.signal, so those fallback requests are no longer abortable by timeout or client disconnect and can hang instead of returning the expected timeout/499 behavior. This should only clean up on terminal exits, or a fresh combined signal should be created before retrying.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a combineAbortSignals utility to merge multiple AbortSignal instances, utilizing the native AbortSignal.any when available and providing a polyfill for other environments. The ProxyForwarder is updated to use this utility, ensuring that listeners are cleaned up to prevent memory leaks. Review feedback identifies potential resource leaks in the hedging logic, specifically regarding discarded attempts and shadow session synchronization. It is also recommended that the polyfill propagate the abort reason from the source signal to maintain consistency with native behavior.
| sessionWithTimeout.releaseAgent = () => { | ||
| if (pool && agentCacheKeyToRelease && agentDispatcherIdToRelease) { | ||
| pool.releaseAgent(agentCacheKeyToRelease, agentDispatcherIdToRelease); | ||
| }; | ||
| } | ||
| } | ||
| cleanupCombinedSignal(); | ||
| }; |
There was a problem hiding this comment.
虽然将 cleanupCombinedSignal 合并到 releaseAgent 很好地复用了 response-handler 的清理点，但目前实现仍存在两处潜在的泄漏/失效风险：
- Hedge 失败者泄漏：在
sendStreamingWithHedge的runAttempt中，被丢弃的尝试（losers）不会进入response-handler。目前代码仅对 losers 调用了body.cancel()，未调用releaseAgent()，这会导致 polyfill 路径下的 listener 泄漏，且所有路径下的 agent 引用计数也无法释放。 - Shadow Session 同步问题：如果 Hedge 的胜出者是 shadow session，
syncWinningAttemptSession目前没有同步releaseAgent回主 session。这会导致response-handler在主 session 上调用releaseAgent时，实际上清理的是第一轮尝试（或为空），而胜出尝试的 listener 依然残留在源信号上。
建议在 runAttempt 的 loser 处理路径中显式调用 releaseAgent()，并在 syncWinningAttemptSession 中同步该回调。注意：由于此处于性能敏感的 Hedge 路径，且 releaseAgent 可能涉及 Redis 会话释放等非关键 I/O，根据项目规则，应将其作为 fire-and-forget 任务执行，以避免阻塞主逻辑。
References
- In performance-sensitive code paths like provider failover, non-critical I/O operations (e.g., releasing a session in Redis) should be executed as fire-and-forget tasks to avoid blocking the main logic.
| if (signal.aborted) { | ||
| combinedController.abort(); | ||
| cleanup(); | ||
| break; | ||
| } | ||
|
|
||
| const abortHandler = () => { | ||
| combinedController.abort(); | ||
| cleanup(); | ||
| }; | ||
|
|
||
| signal.addEventListener("abort", abortHandler, { once: true }); | ||
| detachers.push(() => { | ||
| signal.removeEventListener("abort", abortHandler); | ||
| }); | ||
| } |
There was a problem hiding this comment.
为了与原生的 AbortSignal.any 行为保持一致，建议在 polyfill 路径中传播源信号的 reason。原生实现会将组合信号的 reason 设置为第一个触发中断的信号的 reason。
for (const signal of signals) {
if (signal.aborted) {
combinedController.abort(signal.reason);
cleanup();
break;
}
const abortHandler = () => {
combinedController.abort(signal.reason);
cleanup();
};
signal.addEventListener("abort", abortHandler, { once: true });
detachers.push(() => {
signal.removeEventListener("abort", abortHandler);
});
}![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| logger.debug("ProxyForwarder: Combined abort signals", { | ||
| signalCount: signals.length, | ||
| }); |
There was a problem hiding this comment.
The original code emitted distinct logger.debug messages for the native path ("Using native AbortSignal.any") and the polyfill path ("Using AbortSignal.any polyfill"). The consolidated message no longer tells you which branch was actually taken, which matters when debugging the Next.js-standalone override scenario that motivated this fix.
Consider tagging the log entry with a usingPolyfill boolean so the information is preserved without restoring two separate log calls:
| logger.debug("ProxyForwarder: Combined abort signals", { | |
| signalCount: signals.length, | |
| }); | |
| logger.debug("ProxyForwarder: Combined abort signals", { | |
| signalCount: signals.length, | |
| usingPolyfill: !("any" in AbortSignal && typeof AbortSignal.any === "function"), | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/forwarder.ts
Line: 2719-2721
Comment:
**Debug log loses native/polyfill visibility**
The original code emitted distinct `logger.debug` messages for the native path (`"Using native AbortSignal.any"`) and the polyfill path (`"Using AbortSignal.any polyfill"`). The consolidated message no longer tells you which branch was actually taken, which matters when debugging the Next.js-standalone override scenario that motivated this fix.
Consider tagging the log entry with a `usingPolyfill` boolean so the information is preserved without restoring two separate log calls:
```suggestion
logger.debug("ProxyForwarder: Combined abort signals", {
signalCount: signals.length,
usingPolyfill: !("any" in AbortSignal && typeof AbortSignal.any === "function"),
});
```
How can I resolve this? If you propose a fix, please make it concise.![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
No significant issues identified. This is a well-structured memory leak fix that correctly extracts the AbortSignal.any polyfill into a dedicated module with proper lifecycle management. The cleanup is idempotent, all execution paths (fetch failure, HTTP error, success) are covered, and the behavioral contract change (releaseAgent now always set on session) is safe because the response-handler guards pool release internally.
PR Size: M
- Lines changed: 218 (168 additions, 50 deletions)
- Files changed: 3
Review Coverage
- Logic and correctness - Clean. All three exit paths (fetch failure, HTTP error, success) correctly call cleanup. Mutually exclusive paths prevent double-cleanup. Idempotent
cleanedflag provides additional safety. - Security (OWASP Top 10) - Clean. No user input handling or external data flows affected.
- Error handling - Clean. Cleanup is called in catch blocks and finally blocks. The response-handler's
releaseSessionAgentwraps the call in try/catch. - Type safety - Clean. No
anyusage.CombinedAbortSignalinterface is clean.MutableAbortSignalin tests is appropriately scoped. - Documentation accuracy - Clean. Comments accurately describe the cleanup strategy and rationale for merging cleanup into
releaseAgent. - Test coverage - Adequate. 5 tests covering native delegation, polyfill abort propagation, explicit cleanup, auto-cleanup on source abort, and pre-aborted signal edge case.
- Code clarity - Good. Extraction reduces inline complexity in forwarder.ts by ~40 lines.
Automated review by Claude AI
![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
🧹 Nitpick comments (1)
src/app/v1/_lib/proxy/forwarder.ts (1)
3286-3291:pool.releaseAgent抛错时cleanupCombinedSignal会被跳过。
response-handler.ts中的releaseSessionAgent用 try/catch 包裹了s.releaseAgent()并把字段置空，所以一旦pool.releaseAgent在某些边界场景（agent 已被驱逐等）抛错，下方的cleanupCombinedSignal()就再也不会被调用，polyfill 上的 listener 闭包又泄漏回去了。建议用 try/finally 把两步解耦：♻️ 建议改动
sessionWithTimeout.releaseAgent = () => { - if (pool && agentCacheKeyToRelease && agentDispatcherIdToRelease) { - pool.releaseAgent(agentCacheKeyToRelease, agentDispatcherIdToRelease); - } - cleanupCombinedSignal(); + try { + if (pool && agentCacheKeyToRelease && agentDispatcherIdToRelease) { + pool.releaseAgent(agentCacheKeyToRelease, agentDispatcherIdToRelease); + } + } finally { + cleanupCombinedSignal(); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 3286 - 3291, sessionWithTimeout.releaseAgent currently calls pool.releaseAgent and then cleanupCombinedSignal, but if pool.releaseAgent throws the cleanup is skipped; wrap the pool.releaseAgent call in a try/finally inside sessionWithTimeout.releaseAgent so cleanupCombinedSignal is always invoked (and ensure any agent-cache fields like agentCacheKeyToRelease and agentDispatcherIdToRelease are cleared/handled similarly as releaseSessionAgent in response-handler.ts); use the same naming (sessionWithTimeout.releaseAgent, pool.releaseAgent, cleanupCombinedSignal, releaseSessionAgent) to locate and update the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/v1/_lib/proxy/combine-abort-signals.ts`:
- Around line 36-52: The polyfill currently aborts the combinedController
without forwarding the source signal's reason, causing combinedSignal.reason to
differ from native AbortSignal.any; update the logic in combine-abort-signals
(the loop that checks signal.aborted and the abortHandler closure) to read the
originating signal.reason and call combinedController.abort(reason) so the
source reason is propagated both for already-aborted signals and for the event
handler path, keeping the existing cleanup() call and detacher removal behavior.
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 2815-2816: The cleanupCombinedSignal() call is currently invoked
immediately at the top of the catch block which unbinds abort listeners from the
original signal and breaks the later fallback flows that reuse
init/combinedSignal (e.g. the HTTP/2→HTTP/1.1 path that creates
http1FallbackInit and the proxy→direct fallback that creates fallbackInit and
calls fetchWithoutAutoDecode). Move the cleanupCombinedSignal() invocation out
of the catch head and instead call it right before any final throw or when a
branch truly finishes (responses timed out, streaming idle abort, client abort,
SSL/general fatal errors), and also ensure each inner fallback catch calls
cleanupCombinedSignal() before re-throwing; rely on releaseAgent’s idempotent
cleanup for successful fallback paths. Use the existing symbols
cleanupCombinedSignal, combinedSignal, init/http1FallbackInit/fallbackInit,
fetchWithoutAutoDecode, responseController.abort, combinedController and
session.clientAbortSignal to locate where to add/remove the calls.
---
Nitpick comments:
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 3286-3291: sessionWithTimeout.releaseAgent currently calls
pool.releaseAgent and then cleanupCombinedSignal, but if pool.releaseAgent
throws the cleanup is skipped; wrap the pool.releaseAgent call in a try/finally
inside sessionWithTimeout.releaseAgent so cleanupCombinedSignal is always
invoked (and ensure any agent-cache fields like agentCacheKeyToRelease and
agentDispatcherIdToRelease are cleared/handled similarly as releaseSessionAgent
in response-handler.ts); use the same naming (sessionWithTimeout.releaseAgent,
pool.releaseAgent, cleanupCombinedSignal, releaseSessionAgent) to locate and
update the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4bf83db-68e2-4c44-ae4d-22491c4526c7
📒 Files selected for processing (3)
src/app/v1/_lib/proxy/combine-abort-signals.tssrc/app/v1/_lib/proxy/forwarder.tstests/unit/proxy/combine-abort-signals.test.ts
| for (const signal of signals) { | ||
| if (signal.aborted) { | ||
| combinedController.abort(); | ||
| cleanup(); | ||
| break; | ||
| } | ||
|
|
||
| const abortHandler = () => { | ||
| combinedController.abort(); | ||
| cleanup(); | ||
| }; | ||
|
|
||
| signal.addEventListener("abort", abortHandler, { once: true }); | ||
| detachers.push(() => { | ||
| signal.removeEventListener("abort", abortHandler); | ||
| }); | ||
| } |
There was a problem hiding this comment.
polyfill 未传播源信号的 abort reason，与原生 AbortSignal.any 行为不一致。
原生 AbortSignal.any 在某个源 abort 时会把该源的 reason 传给组合信号；当前 polyfill 调用 combinedController.abort() 不带参数，会丢失上游传入的 reason（例如 responseController.signal.reason、clientAbortSignal.reason）。这会导致两条路径下 combinedSignal.reason 行为不同，下游若读取 signal.reason 做诊断/分类会拿到不一致的结果。
♻️ 建议改动
for (const signal of signals) {
if (signal.aborted) {
- combinedController.abort();
+ combinedController.abort(signal.reason);
cleanup();
break;
}
const abortHandler = () => {
- combinedController.abort();
+ combinedController.abort(signal.reason);
cleanup();
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/v1/_lib/proxy/combine-abort-signals.ts` around lines 36 - 52, The
polyfill currently aborts the combinedController without forwarding the source
signal's reason, causing combinedSignal.reason to differ from native
AbortSignal.any; update the logic in combine-abort-signals (the loop that checks
signal.aborted and the abortHandler closure) to read the originating
signal.reason and call combinedController.abort(reason) so the source reason is
propagated both for already-aborted signals and for the event handler path,
keeping the existing cleanup() call and detacher removal behavior.
| // Polyfill 路径上需要主动解绑源信号的 abort listener（response-handler 不会执行）。 | ||
| cleanupCombinedSignal(); |
There was a problem hiding this comment.
polyfill 路径上 cleanup 调用过早，会破坏 HTTP/2→HTTP/1.1 与代理→直连 fallback 的信号取消。
本 catch 块下方还有两条 fallback 路径会复用同一个 init（因此复用同一个 combinedSignal）：
- 第 ~2991-3026 行 HTTP/2→HTTP/1.1 fallback：
const http1FallbackInit = { ...init }，随后fetchWithoutAutoDecode(proxyUrl, http1FallbackInit, ...) - 第 ~3097-3109 行 代理→直连 fallback：
const fallbackInit = { ...init }，随后fetchWithoutAutoDecode(proxyUrl, fallbackInit, ...)
进入 catch 后第 2816 行立刻执行 cleanupCombinedSignal()。在 polyfill 分支这会把源信号上的 abort listener 全部解绑，且 cleaned 标志位永不复位。之后 H1/直连 fallback 在第 3040-3048、3126-3134 行重新启动响应超时（setTimeout(() => responseController.abort(), ...)），但 responseController.signal 上已没有 listener 把 abort 传播给 combinedController——combinedSignal.aborted 始终为 false，fallback 的 fetch 不会被首字节/总响应超时取消，session.clientAbortSignal 触发的客户端中断同样无法传到 fallback fetch，最终只能依赖 undici 的 headersTimeout/bodyTimeout 兜底，明显退化于 PR 之前的行为。原生 AbortSignal.any 路径因为 cleanup 是 noop 不受影响。
建议把 cleanup 从 catch 顶部移除，改为在每个真正抛出的分支前调用（响应超时、流式 idle、客户端中断、SSL/通用错误的最终 throw 处），fallback 成功路径已经由 releaseAgent 内的幂等 cleanup 兜底；fallback 自身的内层 catch 在 re-throw 前也需要补一次 cleanup。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 2815 - 2816, The
cleanupCombinedSignal() call is currently invoked immediately at the top of the
catch block which unbinds abort listeners from the original signal and breaks
the later fallback flows that reuse init/combinedSignal (e.g. the
HTTP/2→HTTP/1.1 path that creates http1FallbackInit and the proxy→direct
fallback that creates fallbackInit and calls fetchWithoutAutoDecode). Move the
cleanupCombinedSignal() invocation out of the catch head and instead call it
right before any final throw or when a branch truly finishes (responses timed
out, streaming idle abort, client abort, SSL/general fatal errors), and also
ensure each inner fallback catch calls cleanupCombinedSignal() before
re-throwing; rely on releaseAgent’s idempotent cleanup for successful fallback
paths. Use the existing symbols cleanupCombinedSignal, combinedSignal,
init/http1FallbackInit/fallbackInit, fetchWithoutAutoDecode,
responseController.abort, combinedController and session.clientAbortSignal to
locate where to add/remove the calls.
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
Summary
Fixes a memory leak in the
AbortSignal.anypolyfill branch offorwardSingleAttemptwhere abort listeners on source signals were never detached on normal request completion, preventing GC of session and request body objects.Related Issues & PRs:
forwardSingleAttemptthat was not addressed in that PRBackground
forwardSingleAttempt(around line 2728) registered{ once: true }abort listeners on source signals (responseController.signalandsession.clientAbortSignal) inside theAbortSignal.anypolyfill branch.once: trueonly auto-detaches when the abort event fires — on normal request completion (the common case), listeners were never removed.The
abortHandlerclosure heldcombinedController,cleanupHandlers, and transitively the session and request body, preventing GC. This is the same leak pattern #1113 fixed for_handleStreamingHedge, but the polyfill branch inforwardSingleAttemptwas not covered.Production impact: Node 20.3+ uses native
AbortSignal.anyand is unaffected, but Next.js standalone may override the globalAbortSignal, causing the fallback path to accumulate leaks.Changes
src/app/v1/_lib/proxy/combine-abort-signals.ts— returns{ signal, cleanup }. Native branch returns noop cleanup (V8 manages listeners); polyfill branch requires explicit cleanup by the caller.forwardSingleAttemptcleanup integration:cleanupCombinedSignal()(response-handler never runs)sessionWithTimeout.releaseAgentcallback, reusing response-handler's existing idempotent finally invocation point. Internalcleanedflag guarantees idempotency.combineAbortSignalscall.Verification
Files Changed
src/app/v1/_lib/proxy/combine-abort-signals.tssrc/app/v1/_lib/proxy/forwarder.tstests/unit/proxy/combine-abort-signals.test.tsDescription enhanced by Claude AI