Avoid passing non garbage collectable composite signal to tool call - #27986
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
ykmsd
marked this pull request as ready for review
June 25, 2026 21:02
matteotrab
approved these changes
Jun 26, 2026
flvndvd
approved these changes
Jun 26, 2026
|
|
||
| if (compositeSignal.aborted) { | ||
| perToolCallController.abort(compositeSignal.reason); | ||
| return callTool(perToolCallController.signal); |
Contributor
There was a problem hiding this comment.
If plan is to let MCP handles the abort signal can we add a comment right above this line, please?
Contributor
Author
|
I tested it locally (called wait tool and then sent a signal to mimic shutdown) and it seems to be working. I will merge it and monitor 🫡 Also note that most likely we need more work in memory management, I'm not sure how much of spikes we saw is related to this memory leak because we seem to load something huge like mp4, pptx etc. I will continue my investigation 🫡 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR is my attempt to fix memory leak in agent loop worker.
I have multiple snapshots from agent loop worker, and most of them are retained by
gcPersistentSignals. For example in one of my snapshots:Below is Yuka's current understanding of what is happening, I might be wrong so be extra careful when you read it 🫡
What is an AbortController/AbortSignal?
AbortControlleris the way to cancel fetch requests or other asynchronous operations.For example, the following fetch will be cancelled if it doesn't finish within 5000ms:
(note: you can now also use AbortSignal.timeout instead of setTimeout:
await fetch(url, { signal: AbortSignal.timeout(5000) }))You can attach an event listener to the signal:
Why there is a memory leak and what is gcPersistentSignals?
There is a bug in MCP sdk, they attach an event listener to the abort signal but never remove it, and also it doesn't have
{ once: true }(meaning that even if it's aborted the listener will not be removed, you can read more aboutoncehere).https://github.com/modelcontextprotocol/typescript-sdk/blob/v1.27.1/src/shared/protocol.ts#L1202-L1204
The event listener they attach captures
cancel, which carries a reject function and it is a closure that points back at the whole Promise. A settled promise retains its fulfillment value([[PromiseResult]])in V8, because the abort listener is never removed, reject stays reachable indefinitely, and the result of every past tool call accumulates in memory for the lifetime of the pod.This bug is fixed in v2 alpha version but not in v1. If you use a single abort signal and it's garbage collectable, this bug is harmless, because once V8 cannot reach the abort signal, everything (the listener, handler, and its closure) will be garbage collected.
However, the problem is we pass a composite signal and one of them is non garbage collectable:
https://github.com/dust-tt/dust/blob/main/front/temporal/agent_loop/activities/run_tool.ts#L210-L214
getShutdownSignalis a singleton that is declared at module level which will never be garbaged collected during the entire pod lifecycle.When you call
AbortSignal.any([toolCallSignal, getShutdownSignal()]), Node.js wires up two pointers between each source and the composite signal:It's a WeakRef, so even if one of the sources is still alive, the composite signal can be garbage collected once nothing strongly references it.
However, if you add an event listener to a composite signal, you don't want the signal to be garbage collected (otherwise nothing happens when abort is fired). So Node.js will add a composite signal to
gcPersistentSignalswhile it has an abort listener AND still has at least one live source. This will strongly reference the signal:https://github.com/nodejs/node/blob/ccdfb374383a3b0126693089780314f967881d20/lib/internal/abort_controller.js#L238-L252
It will be removed from
gcPersistentSignalsonly when the listener count drops back to 0 or both sources are gone. It will never happen in our case because sdk doesn't remove the listener when it's completed or aborted, and one of sources is alive forever.So the retention chain is:
gcPersistentSignals → composite signal → abort listener → cancel closure → reject → Promise → [[PromiseResult]]How can we fix it?
The MCP SDK attaches an abort listener that we have no way to remove. So instead of handing it the composite signal, we hand it a signal from a locally scoped AbortController. Because that signal is a non-composite signal, Node.js never adds it to `gcPersistentSignals` and because it's only referenced locally, it is garbage collected once the tool call ends (along with the SDK's listener and the retained tool result). if the composite signal receives an abort event (pod shutdown or workflow cancellation), our bridge listener forwards the abort, with its reason, to the per-call AbortController, so the in-flight tool call is still cancellable (I think??).Tests
Risk
Deploy Plan