Change req.on 'close' to req.socket.on 'close'#2978
Change req.on 'close' to req.socket.on 'close'#2978little-thing wants to merge 1 commit intoCopilotKit:mainfrom
Conversation
Node.js triggers the `req.close` event upon completing the request body parsing, which sets `request.signal.aborted` to true, preventing the result from being returned.
⏭️ Changeset Not RequiredLatest commit: bd5fc03 No changes in this PR affected the Changeset is not required for this PR. |
|
@little-thing is attempting to deploy a commit to the CopilotKit Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughModified the abort event listener in Express utility to attach to the underlying socket object instead of the request object. Changed from Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/v2.x/packages/runtime/src/endpoints/express-utils.ts:
- Around line 68-70: The request socket "close" listener currently added with
req.socket.on("close", ...) must be removed to avoid listener accumulation;
create a named handler (e.g., onClose) that calls controller.abort() and then
removes itself from req.socket (req.socket.off/removeListener) and register that
handler instead of an anonymous function, and also ensure you remove the
listener when the request completes (e.g., on 'finish'/'end' or equivalent) so
the handler is always cleaned up after init.signal is set on the AbortController
instance named controller.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/v2.x/packages/runtime/src/endpoints/express-utils.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/frontend-development.mdc)
**/*.{ts,tsx}: Import and useCopilotProviderfrom '@copilotkit/react-core' as the main provider component at the root of your React application
UseuseCopilotChathook from '@copilotkit/react-core' for implementing chat functionality
UseuseCopilotActionhook from '@copilotkit/react-core' to define actions that agents can execute
UseuseCopilotReadablehook from '@copilotkit/react-core' to expose application state to agents
UseCopilotChatcomponent from '@copilotkit/react-ui' for implementing pre-built chat interface UI
UseCopilotPopupcomponent from '@copilotkit/react-ui' for implementing popup chat interface
UseCopilotSidebarcomponent from '@copilotkit/react-ui' for implementing sidebar chat interface
UseCopilotTextareacomponent from '@copilotkit/react-textarea' for implementing AI-enhanced textarea with auto-completion and suggestionsAlways use the provided hooks and functions to modify suggestions state - never modify suggestions state directly
Files:
src/v2.x/packages/runtime/src/endpoints/express-utils.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/frontend-development.mdc)
Use TypeScript for component development to ensure better type safety
Files:
src/v2.x/packages/runtime/src/endpoints/express-utils.ts
**/*.{tsx,ts,jsx,js}
📄 CodeRabbit inference engine (.cursor/rules/quick-reference.mdc)
For building a simple Copilot, start with copilot-chat-with-your-data example, use react-core package for basic integration, and add react-ui package for pre-built components
Files:
src/v2.x/packages/runtime/src/endpoints/express-utils.ts
src/v2.x/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/v2.x/.cursor/rules/20-typescript.auto.mdc)
src/v2.x/**/*.{ts,tsx}: Prefertypealiases overinterfacefor data structures in TypeScript
Avoidanyin TypeScript; use explicit generics orunknownwhere necessary
Files should be named inkebab-case.tsorkebab-case.tsxwith a single export per file
Files:
src/v2.x/packages/runtime/src/endpoints/express-utils.ts
src/v2.x/packages/runtime/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/v2.x/packages/runtime/.cursor/rules/runtime.always.mdc)
Node-targeted code compiled from
src/todist/using TypeScript
Files:
src/v2.x/packages/runtime/src/endpoints/express-utils.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: CopilotKit/CopilotKit PR: 0
File: .cursor/rules/suggestions-development.mdc:0-0
Timestamp: 2025-12-18T18:11:20.367Z
Learning: Applies to **/suggestions.ts : Ensure abort controllers properly clean up when new suggestion requests are initiated or components unmount
📚 Learning: 2025-12-18T18:11:20.367Z
Learnt from: CR
Repo: CopilotKit/CopilotKit PR: 0
File: .cursor/rules/suggestions-development.mdc:0-0
Timestamp: 2025-12-18T18:11:20.367Z
Learning: Applies to **/suggestions.ts : Ensure abort controllers properly clean up when new suggestion requests are initiated or components unmount
Applied to files:
src/v2.x/packages/runtime/src/endpoints/express-utils.ts
📚 Learning: 2025-12-18T18:11:58.449Z
Learnt from: CR
Repo: CopilotKit/CopilotKit PR: 0
File: src/v2.x/packages/runtime/.cursor/rules/runtime.always.mdc:0-0
Timestamp: 2025-12-18T18:11:58.449Z
Learning: Applies to src/v2.x/packages/runtime/src/index.ts : Exported APIs live under `src/index.ts`
Applied to files:
src/v2.x/packages/runtime/src/endpoints/express-utils.ts
| const controller = new AbortController(); | ||
| req.on("close", () => controller.abort()); | ||
| req.socket.on("close", () => controller.abort()); | ||
| init.signal = controller.signal; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the actual file context around lines 68-70
cat -n src/v2.x/packages/runtime/src/endpoints/express-utils.ts | sed -n '60,80p'Repository: CopilotKit/CopilotKit
Length of output: 856
🏁 Script executed:
# Check for other req.socket usage patterns in the codebase
rg -n 'req\.socket' --type=ts -C2Repository: CopilotKit/CopilotKit
Length of output: 1058
🏁 Script executed:
# Search for event listener cleanup patterns in the codebase
rg -n '\.removeListener|\.off\(' --type=ts -C2Repository: CopilotKit/CopilotKit
Length of output: 1567
🏁 Script executed:
# Search for AbortController usage patterns in the codebase
rg -n 'AbortController' --type=ts -C3Repository: CopilotKit/CopilotKit
Length of output: 9312
🌐 Web query:
Express.js request socket always defined req.socket documentation
💡 Result:
Short answer: req.socket is provided by Node (http.IncomingMessage) and is expected to exist for an active HTTP request — it's the underlying net.Socket for that connection. Historically req.connection was an alias (now deprecated); use req.socket (or req.socket.remoteAddress to get client IP). [1][2]
Sources:
- Node/IncomingMessage socket docs (describes socket as the net.Socket for the request). [1]
- Discussion noting req.connection is deprecated and to use req.socket instead. [2]
🌐 Web query:
Node.js HTTP keep-alive socket persistence multiple requests
💡 Result:
Summary — how Node.js keeps sockets alive and reuses them across multiple requests
-
Who manages persistence: the http(s).Agent. It pools sockets per host:port (and TLS options for HTTPS) and hands them to client requests so one TCP connection can serve multiple HTTP/1.1 requests. [1][2]
-
Enable it: create an Agent with keepAlive: true (default Agent has keepAlive false). keepAlive keeps sockets in agent.freeSockets after a request completes so they can be reused. Example: new http.Agent({ keepAlive: true }). [1][2]
-
Key options:
- keepAlive (bool) — keep sockets around for reuse. [1]
- keepAliveMsecs — initial TCP keep-alive probe delay. [1]
- maxSockets — max concurrent sockets per host before queuing. [1]
- maxFreeSockets — max idle sockets to keep in the pool. [6]
-
When reuse happens: after a request finishes the socket is either returned to freeSockets (if keepAlive true) or destroyed (if false). When a new request for the same agent/host arrives the agent will reuse a free socket if compatible (same host:port:localAddress[:family], and for HTTPS same TLS options). [2][7]
-
When reuse fails or ends:
- Server may close idle connections (agent removes closed sockets). [1]
- If server refuses multiple requests on the same connection (or response has Connection: close), agent opens a new socket for the next request. [1][4]
- freeSockets are unref() so they don't keep the Node.js process alive; sockets are also removed on 'timeout' or 'close'. [1][6]
-
Best practices:
- Use a long-lived Agent instance (don’t recreate per request) so connections are reused. [3]
- Call agent.destroy() when you no longer need it to free OS resources. [2][6]
- Monitor/adjust maxSockets and maxFreeSockets for your workload and server limits. [1][6]
References
[1] Node.js HTTP docs — Agent options and behavior.
[2] Node.js HTTP docs — agent.keepSocketAlive / reuseSocket / destroy.
[3] AWS Developer Blog — explanation and recommendation to enable keep-alive for SDKs.
[4] StackOverflow discussion — Agent, sockets and request relationships.
[6] Node.js API (freeSockets / timeout behavior).
🌐 Web query:
Node.js event listener cleanup memory leaks socket reuse best practices
💡 Result:
Here are concise best practices for avoiding event-listener memory leaks and safely reusing sockets in Node.js, with sources.
Event-listener cleanup
- Prefer one-time listeners when appropriate: use emitter.once() for short‑lived handlers. [1]
- Always remove handlers you add to emitters you don’t own: emitter.off/removeListener or emitter.removeAllListeners(event) when the handler’s lifetime ends. Keep a reference to the handler so you can remove it. [1][3]
- Avoid adding per-request listeners to long‑lived emitters (or use scoped/temporary emitters). If you must, bound the number of listeners or call remove after use. [3]
- Use emitter.setMaxListeners(n) to surface leaks (MaxListenersExceededWarning) but don’t rely on it to hide real leaks. [1][3]
- Avoid closures that retain large objects in listeners; null out large references when done or use WeakRef/FinalizationRegistry for non‑critical caches/listener state where suitable. [4][3]
- Register 'error' handlers on streams/sockets to avoid unhandled errors keeping state or crashing flows. [1]
Socket/HTTP connection reuse and cleanup
- Use http(s).Agent with keepAlive for socket reuse, tune maxSockets/maxFreeSockets for your workload. Explicitly destroy/close the agent (agent.destroy()) when it will no longer be used to release sockets. [2]
- For long‑running servers, periodically monitor freeSockets/sockets and consider connection timeouts (socket.setTimeout) and proper close/destroy on client disconnects. [2]
- Avoid holding per‑connection large state on the socket object; detach references when the socket ends. [2][3]
Detection and debugging
- Monitor process.memoryUsage(), heap snapshots (Chrome DevTools, heapdump), and use tools like clinic/clinic.js or APMs to find retained objects and listener growth. Watch for MaxListenersExceededWarning and steadily increasing listener counts via emitter.listenerCount(event). [4][3]
Quick code patterns
- Add/remove pattern:
const handler = (data) => { ... };
emitter.on('event', handler);
// later
emitter.off('event', handler);
[1] - Use once:
emitter.once('event', handler);
[1] - Destroy agent when done:
agent.destroy();
[2]
Sources
[1] Node.js Events docs (removeListener, once, setMaxListeners, error listener).
[2] Node.js HTTP Agent docs (keepAlive, maxSockets, agent.destroy()).
[3] Articles on Node memory leaks / event emitter leaks (best practices for removing listeners and avoiding leaks).
[4] Guides on debugging leaks (heap snapshots, clinic.js, APM monitoring, WeakRef usage).
If you want, I can: (A) show a short checklist you can paste into code reviews; (B) scan your code (paste a snippet) and point out listener/leak risks.
Remove event listener to follow best practices for socket management.
The listener added to req.socket should be cleaned up after use. While req.socket is guaranteed to exist during active request handling per Node.js documentation, event listeners on long-lived emitters like sockets should be removed to prevent accumulation, especially in scenarios with HTTP keep-alive connection reuse.
Suggested cleanup pattern
const controller = new AbortController();
-req.socket.on("close", () => controller.abort());
+const closeHandler = () => controller.abort();
+req.socket.on("close", closeHandler);
+
+// Cleanup when controller is already aborted or on request completion
+const originalSignal = controller.signal;
+controller.signal.addEventListener("abort", () => {
+ req.socket.removeListener("close", closeHandler);
+});
init.signal = controller.signal;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/v2.x/packages/runtime/src/endpoints/express-utils.ts around lines 68 -
70, The request socket "close" listener currently added with
req.socket.on("close", ...) must be removed to avoid listener accumulation;
create a named handler (e.g., onClose) that calls controller.abort() and then
removes itself from req.socket (req.socket.off/removeListener) and register that
handler instead of an anonymous function, and also ensure you remove the
listener when the request completes (e.g., on 'finish'/'end' or equivalent) so
the handler is always cleaned up after init.signal is set on the AbortController
instance named controller.
Node.js triggers the
req.closeevent upon completing the request body parsing, which setsrequest.signal.abortedto true, preventing the result from being returned.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.