feat(agent): add wall-clock idle time check in reuseSocket for serverless environments - #122
feat(agent): add wall-clock idle time check in reuseSocket for serverless environments#122grn621 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe agent records when sockets enter the free pool and stores their effective timeout. ChangesFree-socket timeout handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Sequence Diagram(s)sequenceDiagram
participant HTTPClient
participant Agent
participant FreeSocketPool
participant HTTPServer
HTTPClient->>Agent: addRequest(req, options)
Agent->>FreeSocketPool: Check free-pool timestamp and effective timeout
alt Socket exceeds effective timeout
Agent->>FreeSocketPool: Remove and destroy stale socket
Agent->>HTTPServer: Create replacement connection
else Socket remains within timeout
Agent->>FreeSocketPool: Reuse pooled socket
end
HTTPServer-->>HTTPClient: Return response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
index.d.tsParsing error: 'import' and 'export' may appear only with 'sourceType: module' 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
test/wall-clock-free-socket-timeout.test.js (1)
139-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test does not cover the second socket, and duplicate sockets can make it flaky.
The test name states "multiple stale sockets", but only
sockets[0]is passed toreuseSocket().sockets[1]is never validated after the simulated freeze.
socketscan also contain the same socket twice. The two requests run in parallel, but if the first response completes before the second request is dispatched, the agent reuses the same socket.assert.strictEqual(sockets.length, 2)still passes in that case.💚 Proposed change
assert.strictEqual(sockets.length, 2); + assert.notStrictEqual(sockets[0], sockets[1], 'should use two distinct sockets'); assert(sockets[0][SOCKET_ENTER_FREE_POOL_TIME] > 0); assert(sockets[1][SOCKET_ENTER_FREE_POOL_TIME] > 0); @@ - const mockReq = { reusedSocket: false }; - agent.reuseSocket(sockets[0], mockReq); - assert(sockets[0].destroyed, 'first stale socket should be destroyed'); - assert.strictEqual(mockReq.reusedSocket, true, - 'req.reusedSocket should be set to true for retry'); + sockets.forEach((socket, i) => { + const mockReq = { reusedSocket: false }; + agent.reuseSocket(socket, mockReq); + assert(socket.destroyed, `stale socket ${i} should be destroyed`); + assert.strictEqual(mockReq.reusedSocket, true, + 'req.reusedSocket should be set to true for retry'); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/wall-clock-free-socket-timeout.test.js` around lines 139 - 160, Update the multiple-stale-sockets test around agent.reuseSocket to ensure the two collected sockets are distinct before validating them. Invoke reuseSocket with separate mock requests for both sockets, then assert each socket is destroyed and each request has reusedSocket set to true; retain cleanup and completion behavior.lib/constants.js (1)
14-14: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd
SOCKET_ENTER_FREE_POOL_TIMEto theConstantsinterface.
lib/constants.jsexports the symbol, butindex.d.tsstill declares only the previous constants, so TypeScript consumers importing constants can get a compile error.♻️ Proposed change in `index.d.ts`
SOCKET_REQUEST_COUNT: Symbol; SOCKET_REQUEST_FINISHED_COUNT: Symbol; + SOCKET_ENTER_FREE_POOL_TIME: Symbol; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/constants.js` at line 14, Add SOCKET_ENTER_FREE_POOL_TIME to the Constants interface in index.d.ts, matching the symbol already exported by lib/constants.js and preserving the existing constant declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/agent.js`:
- Around line 195-199: Remove the stale-socket destruction and early return from
reuseSocket(); instead, perform the wall-clock staleness check while free
sockets are still being selected, such as in an addRequest() override before
delegating to the parent implementation, and purge stale entries from
this.sockets[name]. Preserve req.reusedSocket only as informational state and
ensure stale sockets are removed before request assignment.
- Around line 188-194: Update the stale-socket cleanup logic to derive
freeSocketTimeout via this.calcSocketTimeout(socket), preserving per-socket and
keep-alive timeout behavior. In the adjacent debug call, replace both %.1f
placeholders with supported %s or %d formatters so idleMs and freeSocketTimeout
are rendered correctly.
In `@test/wall-clock-free-socket-timeout.test.js`:
- Around line 94-117: Update test/wall-clock-free-socket-timeout.test.js:94-117
to remove the retry fallback from the first request and have its req error
handler destroy agent and call done(err), preventing failed requests from
passing. At test/wall-clock-free-socket-timeout.test.js:237-256, apply the same
failure behavior while closing httpsApp, move the socket1.destroyed assertion
into the success callback, and ensure completion cannot call done twice.
- Around line 205-223: Update the “should work correctly with HTTPS agent” test
to register cleanup that always closes httpsApp, including assertion and
request-error paths, rather than only closing it in success callbacks. Also move
the HttpsAgent, https, and fs imports to the file’s top-level imports.
---
Nitpick comments:
In `@lib/constants.js`:
- Line 14: Add SOCKET_ENTER_FREE_POOL_TIME to the Constants interface in
index.d.ts, matching the symbol already exported by lib/constants.js and
preserving the existing constant declarations.
In `@test/wall-clock-free-socket-timeout.test.js`:
- Around line 139-160: Update the multiple-stale-sockets test around
agent.reuseSocket to ensure the two collected sockets are distinct before
validating them. Invoke reuseSocket with separate mock requests for both
sockets, then assert each socket is destroyed and each request has reusedSocket
set to true; retain cleanup and completion behavior.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 72cc6cb6-7e83-49dd-b495-b162cbb76156
📒 Files selected for processing (3)
lib/agent.jslib/constants.jstest/wall-clock-free-socket-timeout.test.js
879d92f to
e0d4161
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/agent.js`:
- Around line 196-201: Update the free-socket cleanup loop in the agent request
flow before super.addRequest(req, options): remove each stale socket from its
freeSockets pool before calling socket.destroy(), then delete the pool bucket
when it becomes empty so the base implementation cannot select a destroyed
socket.
- Around line 154-158: Update the lifecycle comment near the free-pool timestamp
assignment to state that addRequest() reads the timestamp to detect sockets idle
longer than freeSocketTimeout, replacing the inaccurate reference to
reuseSocket().
- Around line 183-195: The free-socket cleanup in the agent’s reuse path
currently compares idle time only against this.options.freeSocketTimeout.
Capture the positive effective timeout from calcSocketTimeout(socket) when the
socket enters the free pool, preserve that entry-time value on the socket, and
use it in the stale-socket comparison alongside the recorded
SOCKET_ENTER_FREE_POOL_TIME; do not recalculate dynamic TTL values during reuse.
- Around line 182-183: Update Agent.addRequest() to preserve the inherited (req,
options, port, localAddress) signature. Normalize string options into an object
containing host, port, and localAddress, use the merged options for getName()
and purge lookup, and forward all four arguments to super.addRequest().
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f15de168-a21b-4705-9de1-4f4e3a51ed07
📒 Files selected for processing (2)
lib/agent.jstest/wall-clock-free-socket-timeout.test.js
1c550a2 to
a802fed
Compare
…less environments In environments where the process is frozen between invocations (e.g., AWS Lambda, Azure Functions), socket.setTimeout() callbacks do not fire because the event loop is suspended. When the process unfreezes, stale sockets whose connections were closed by the server during the freeze are reused, causing EPIPE and ECONNRESET errors. This change adds a wall-clock timestamp (Date.now()) to each socket when it enters the free pool (keepSocketAlive), and checks elapsed real time against freeSocketTimeout when the socket is grabbed for reuse (reuseSocket). If the idle time exceeds the threshold, the socket is destroyed and req.reusedSocket is set to true so Node's http client retries on a fresh connection. This fix is backward-compatible: - No new configuration options required - Uses the existing freeSocketTimeout value as the threshold - No behavioral change for long-running server processes where timers work - Only activates when actual wall-clock idle time exceeds freeSocketTimeout Closes #xxx
a802fed to
a6d11be
Compare
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Adds an opt-in testOnBorrow feature to agentkeepalive to make free-socket reuse resilient in serverless “freeze/thaw” environments where timer-based socket.setTimeout() callbacks don’t run during suspension. The change stamps wall-clock timestamps when sockets enter the free pool and purges stale free sockets on borrow.
Changes:
- Add
testOnBorrowoption to stamp wall-clock enter-time + effective timeout on free sockets and purge stale sockets before reuse. - Expose new internal socket metadata symbols in
lib/constants.jsand TypeScript definitions. - Add documentation and a dedicated test suite covering HTTP/HTTPS and multiple stale sockets.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/wall-clock-free-socket-timeout.test.js | Adds coverage for wall-clock stamping and stale-free-socket purging behavior. |
| README.md | Documents testOnBorrow and provides a serverless usage example. |
| lib/constants.js | Introduces symbols for free-pool enter time and effective timeout metadata. |
| lib/agent.js | Implements stamping in keepSocketAlive() and stale-socket purging in addRequest(). |
| index.d.ts | Adds testOnBorrow to options and exports new constants in typings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: a6d11bebfe
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- README: drain response body in serverless example before resolving - test: relax timestamp recency assertion to 5s for CI stability - agent: merge this.options into getName() lookup to match Node's key (P1) - agent: increment timeoutSocketCount in _purgeStaleFreeSockets (P2)
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/wall-clock-free-socket-timeout.test.js (1)
195-235: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose the HTTPS server on every test exit path.
httpsApp.close(done)runs only after the second successful response. If either request fails, or an assertion throws, the test leaveshttpsAppandagentopen. Register teardown outside the request callbacks so Mocha runs it after failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/wall-clock-free-socket-timeout.test.js` around lines 195 - 235, Update the test setup around the should work correctly with HTTPS agent case to register teardown independently of the nested request callbacks, ensuring httpsApp is closed and agent is destroyed on success, request errors, and assertion failures. Remove reliance on the final httpsApp.close(done) callback path and preserve the existing assertions and response handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@test/wall-clock-free-socket-timeout.test.js`:
- Around line 195-235: Update the test setup around the should work correctly
with HTTPS agent case to register teardown independently of the nested request
callbacks, ensuring httpsApp is closed and agent is destroyed on success,
request errors, and assertion failures. Remove reliance on the final
httpsApp.close(done) callback path and preserve the existing assertions and
response handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a994692-cd47-4a30-add6-495ab94aebd3
📒 Files selected for processing (4)
README.mdindex.d.tslib/agent.jstest/wall-clock-free-socket-timeout.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- index.d.ts
- lib/agent.js
|
@fengmk2 I have addressed all the automated feedback from the AI reviewers in my latest commit (c43d511). Summary of updates:
All pre-merge checks are passing. Ready for your final human review and merge! |
In environments where the process is frozen between invocations (e.g., AWS Lambda, Azure Functions), socket.setTimeout() callbacks do not fire because the event loop is suspended. When the process unfreezes, stale sockets whose connections were closed by the server during the freeze are reused, causing EPIPE and ECONNRESET errors.
This adds a new
testOnBorrowoption. When enabled, a wall-clock timestamp (Date.now()) is stamped on each socket when it enters the free pool (keepSocketAlive), and elapsed real time is checked against the effectivefreeSocketTimeoutwhen a new request arrives (addRequest). If the idle time exceeds the threshold, the socket is destroyed and a fresh connection is created.Usage:
Problem:
Fix (with testOnBorrow: true):
Design:
testOnBorrow: false(default) — no change, zero overhead, existing timer-based behaviortestOnBorrow: true— adds wall-clock validation inaddRequest()before socket selectionBackward-compatible:
Checklist
npm testpassesAffected core subsystem(s)
lib/agent.js,lib/constants.js,index.d.ts,README.mdDescription of change
Added
testOnBorrowoption that validates free sockets using wall-clock time before reuse:keepSocketAlive(): whentestOnBorrowis enabled, stampsDate.now()and the effective timeout on each socket entering the free pooladdRequest(): whentestOnBorrowis enabled, calls_purgeStaleFreeSockets()before the base implementation selects a socket_purgeStaleFreeSockets(name): extracted helper that iterates the pool in reverse, removes and destroys sockets idle beyond their effective timeoutThis is essential for serverless environments (AWS Lambda, Azure Functions, Google Cloud Functions) where
socket.setTimeout()does not fire during process freeze.Summary by CodeRabbit
testOnBorrowoption for serverless environments (AWS Lambda, Azure Functions)testOnBorrowis enabled.Summary by CodeRabbit
New Features
testOnBorrowsupport to validate idle sockets before reuse.Documentation
testOnBorrow, its default behavior, HTTPS Lambda usage, and potential connection errors when disabled.