Skip to content

http: do not reuse expired free sockets - #67292

Open
dennev wants to merge 1 commit into
nodejs:mainfrom
dennev:http-agent-expired-free-socket
Open

dennev wants to merge 1 commit into
nodejs:mainfrom
dennev:http-agent-expired-free-socket

Conversation

@dennev

@dennev dennev commented Nov 6, 2026

Copy link
Copy Markdown

When the event loop is blocked long enough for a free socket's idle timeout
to expire, its timeout callback cannot run. The next request can then reuse
that socket from agent.freeSockets even though the server may already have
closed the connection, causing the request to fail with ECONNRESET.

This change addresses the timeout-expiry case discussed in #54293. It records
when a socket enters the free pool using the timers' clock
(timersBinding.getLibuvNow()). In addRequest(), expired candidates are
removed and destroyed until an unexpired socket is found or the pool is empty.

The clock call preserves the binding object as its receiver, as required by
the fast API call. The regression test also exercises an optimized
addRequest().

Not covered by this change:

Testing: I personally ran the regression test on a release build in a Linux
container, and it passed. I also ran a reproduction based on #54293, with
server.keepAliveTimeout = 5000 and a 6-second block between two requests
through http.globalAgent. It fails with ECONNRESET on v26.12.1 and succeeds
with this change, using a new socket.

Agent-run validation confirmed that the regression test fails without this
change and passes with it. It covers LIFO and FIFO expiry, reuse before expiry,
unchanged reuse without an idle timeout, maxSockets accounting after a block,
and optimized addRequest().

Agent-run validation on Linux also included the HTTP/HTTPS and agent-related
tests, and the full make -j12 test, all of which passed. The full-suite run
used a release build in a container started with --init.

AI disclosure: Claude Code (Claude Opus 5.5) was used to investigate the cause,
write and review the code, test and documentation changes, and run the
agent-run validation described above. This description started from a draft
by the agent, which I rewrote. Before opening this pull request, Claude and
Codex (gpt-6.1-sol) reviewed the final change, the commit message and this
description.

I read the change and the new test myself and checked them against the code
paths described above.

Refs: #54293

A free socket with an idle timeout is normally destroyed by its timer.
When the event loop is blocked long enough for that timeout to expire,
the timer cannot run, and `addRequest()` can reuse the socket even
though the server may already have closed the connection. This can
cause the request to fail with `ECONNRESET`.

Record when a socket enters the free pool and, before reuse, remove and
destroy a free socket whose idle timeout has expired according to the
timers' clock.

Refs: nodejs#54293
Assisted-by: a closed-source coding agent
Signed-off-by: dennev <dennev13@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues and PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Nov 6, 2026
@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@codecov

codecov Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.69%. Comparing base (bf246cf) to head (f9ed896).
⚠️ Report is 73 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #67292      +/-   ##
==========================================
- Coverage   90.69%   90.69%   -0.01%     
==========================================
  Files         797      797              
  Lines      278787   279632     +845     
  Branches    53610    53781     +171     
==========================================
+ Hits       252849   253604     +755     
+ Misses      16310    16298      -12     
- Partials     9628     9730     +102     
Files with missing lines Coverage Δ
lib/internal/http/agent.js 96.75% <100.00%> (+0.11%) ⬆️

... and 68 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@inoway46 inoway46 added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 6, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 6, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@MikeMcC399 MikeMcC399 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Nov 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. http Issues and PRs related to the http subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants