Skip to content

Change req.on 'close' to req.socket.on 'close'#2978

Open
little-thing wants to merge 1 commit intoCopilotKit:mainfrom
little-thing:fix-express-runtime
Open

Change req.on 'close' to req.socket.on 'close'#2978
little-thing wants to merge 1 commit intoCopilotKit:mainfrom
little-thing:fix-express-runtime

Conversation

@little-thing
Copy link

@little-thing little-thing commented Jan 8, 2026

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.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed request cancellation to trigger when the underlying socket closes, ensuring more reliable request cleanup on connection termination.

✏️ Tip: You can customize this high-level summary in your review settings.

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.
@changesets-bot-copilotkit
Copy link

⏭️ Changeset Not Required

Latest commit: bd5fc03

No changes in this PR affected the @copilitkit/* packages. Merging this PR will not cause a version bump for any packages.

Changeset is not required for this PR.

@vercel
Copy link
Contributor

vercel bot commented Jan 8, 2026

@little-thing is attempting to deploy a commit to the CopilotKit Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 8, 2026

Walkthrough

Modified the abort event listener in Express utility to attach to the underlying socket object instead of the request object. Changed from req.on("close", ...) to req.socket.on("close", ...) to trigger AbortController cancellation when the socket closes.

Changes

Cohort / File(s) Summary
Socket abort listener
src/v2.x/packages/runtime/src/endpoints/express-utils.ts
Changed event listener attachment from request object to underlying socket object for abort signal handling

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: replacing req.on('close', ...) with req.socket.on('close', ...). It is specific, clear, and directly reflects the core modification in the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5e7372a and bd5fc03.

📒 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 use CopilotProvider from '@copilotkit/react-core' as the main provider component at the root of your React application
Use useCopilotChat hook from '@copilotkit/react-core' for implementing chat functionality
Use useCopilotAction hook from '@copilotkit/react-core' to define actions that agents can execute
Use useCopilotReadable hook from '@copilotkit/react-core' to expose application state to agents
Use CopilotChat component from '@copilotkit/react-ui' for implementing pre-built chat interface UI
Use CopilotPopup component from '@copilotkit/react-ui' for implementing popup chat interface
Use CopilotSidebar component from '@copilotkit/react-ui' for implementing sidebar chat interface
Use CopilotTextarea component from '@copilotkit/react-textarea' for implementing AI-enhanced textarea with auto-completion and suggestions

Always 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}: Prefer type aliases over interface for data structures in TypeScript
Avoid any in TypeScript; use explicit generics or unknown where necessary
Files should be named in kebab-case.ts or kebab-case.tsx with 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/ to dist/ 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

Comment on lines 68 to 70
const controller = new AbortController();
req.on("close", () => controller.abort());
req.socket.on("close", () => controller.abort());
init.signal = controller.signal;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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 -C2

Repository: CopilotKit/CopilotKit

Length of output: 1058


🏁 Script executed:

# Search for event listener cleanup patterns in the codebase
rg -n '\.removeListener|\.off\(' --type=ts -C2

Repository: CopilotKit/CopilotKit

Length of output: 1567


🏁 Script executed:

# Search for AbortController usage patterns in the codebase
rg -n 'AbortController' --type=ts -C3

Repository: 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant