From e4dd6c8d2656f7ac19356f13f258556f9614b43f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20Sert?= Date: Wed, 15 Apr 2026 19:00:53 +0300 Subject: [PATCH 1/7] fix: use pre-increment for request counter --- lib/dispatcher/client-h1.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/dispatcher/client-h1.js b/lib/dispatcher/client-h1.js index ce8b2e0f627..4d21f504706 100644 --- a/lib/dispatcher/client-h1.js +++ b/lib/dispatcher/client-h1.js @@ -1106,7 +1106,7 @@ function writeH1 (client, request) { socket[kReset] = reset } - if (client[kMaxRequests] && socket[kCounter]++ >= client[kMaxRequests]) { + if (client[kMaxRequests] && ++socket[kCounter] >= client[kMaxRequests]) { socket[kReset] = true } From 759edcf260c5d86bb58cd148f171d83a4c51253a Mon Sep 17 00:00:00 2001 From: bienzaaron Date: Wed, 15 Apr 2026 23:02:20 -0400 Subject: [PATCH 2/7] test: add reproduction test case for #5022 --- test/issue-4244.js | 92 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/test/issue-4244.js b/test/issue-4244.js index 489557fe495..268a81ebe09 100644 --- a/test/issue-4244.js +++ b/test/issue-4244.js @@ -62,4 +62,96 @@ describe('Agent should close inactive clients', () => { await p }) + + test('should reuse replacement keep-alive connection after server closes the previous one', async (t) => { + let nextSocketId = 0 + const socketIds = new Map() + const requestsPerSocket = new Map() + + const server = createServer((req, res) => { + const socket = req.socket + if (!socketIds.has(socket)) { + socketIds.set(socket, ++nextSocketId) + } + + const count = (requestsPerSocket.get(socket) || 0) + 1 + requestsPerSocket.set(socket, count) + + const remaining = 3 - count + res.setHeader('x-socket-id', String(socketIds.get(socket))) + + if (remaining > 0) { + res.setHeader('connection', 'Keep-Alive') + res.setHeader('keep-alive', `timeout=30, max=${remaining}`) + } else { + res.setHeader('connection', 'close') + } + + res.writeHead(200) + res.end('ok') + }).listen(0) + + t.after(() => { + server.closeAllConnections?.() + server.close() + }) + + const agent = new Agent({ connections: 1 }) + t.after(() => agent.close()) + + const socketSequence = [] + for (let i = 0; i < 5; i++) { + const { statusCode, headers, body } = await request(`http://localhost:${server.address().port}`, { + dispatcher: agent + }) + + assert.equal(statusCode, 200) + await body.dump() + socketSequence.push(headers['x-socket-id']) + } + + assert.deepEqual(socketSequence.slice(0, 3), ['1', '1', '1']) + assert.deepEqual(socketSequence.slice(3), ['2', '2']) + }) + + test('should reuse replacement connection after keep-alive max closes the previous one', async (t) => { + let nextSocketId = 0 + const socketIds = new Map() + + const server = createServer((req, res) => { + const socket = req.socket + if (!socketIds.has(socket)) { + socketIds.set(socket, ++nextSocketId) + } + + res.setHeader('x-socket-id', String(socketIds.get(socket))) + res.setHeader('connection', 'Keep-Alive') + res.setHeader('keep-alive', 'timeout=30') + + res.writeHead(200) + res.end('ok') + }).listen(0) + + t.after(() => { + server.closeAllConnections?.() + server.close() + }) + + const agent = new Agent({ connections: 1, maxRequestsPerClient: 3 }) + t.after(() => agent.close()) + + const socketSequence = [] + for (let i = 0; i < 5; i++) { + const { statusCode, headers, body } = await request(`http://localhost:${server.address().port}`, { + dispatcher: agent + }) + + assert.equal(statusCode, 200) + await body.dump() + socketSequence.push(headers['x-socket-id']) + } + + assert.deepEqual(socketSequence.slice(0, 3), ['1', '1', '1']) + assert.deepEqual(socketSequence.slice(3), ['2', '2']) + }) }) From 3f057c7aae689a1bde89a8fecf7bfca2e5886812 Mon Sep 17 00:00:00 2001 From: bienzaaron Date: Wed, 15 Apr 2026 23:05:51 -0400 Subject: [PATCH 3/7] fix: minimal fix for #5022 --- lib/dispatcher/agent.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/dispatcher/agent.js b/lib/dispatcher/agent.js index a1cc7fd6817..140c2c58077 100644 --- a/lib/dispatcher/agent.js +++ b/lib/dispatcher/agent.js @@ -1,7 +1,7 @@ 'use strict' const { InvalidArgumentError, MaxOriginsReachedError } = require('../core/errors') -const { kClients, kRunning, kClose, kDestroy, kDispatch, kUrl } = require('../core/symbols') +const { kBusy, kClients, kRunning, kClose, kDestroy, kDispatch, kUrl } = require('../core/symbols') const DispatcherBase = require('./dispatcher-base') const Pool = require('./pool') const Client = require('./client') @@ -93,23 +93,23 @@ class Agent extends DispatcherBase { const result = this[kClients].get(key) if (result) { if (connected) result.count -= 1 - if (result.count <= 0) { + if (result.count <= 0 && !result.dispatcher[kBusy]) { this[kClients].delete(key) if (!result.dispatcher.destroyed) { result.dispatcher.close() } - } - let hasOrigin = false - for (const entry of this[kClients].values()) { - if (entry.origin === origin) { - hasOrigin = true - break + let hasOrigin = false + for (const entry of this[kClients].values()) { + if (entry.origin === origin) { + hasOrigin = true + break + } } - } - if (!hasOrigin) { - this[kOrigins].delete(origin) + if (!hasOrigin) { + this[kOrigins].delete(origin) + } } } } From f0927a8e7d37ccfc8d5ef2512f8fa6313e007732 Mon Sep 17 00:00:00 2001 From: bienzaaron Date: Wed, 15 Apr 2026 23:19:54 -0400 Subject: [PATCH 4/7] fix: clean up connection bookkeeping --- lib/dispatcher/agent.js | 80 ++++++++++++++++++++--------------------- lib/mock/mock-agent.js | 16 ++++----- 2 files changed, 47 insertions(+), 49 deletions(-) diff --git a/lib/dispatcher/agent.js b/lib/dispatcher/agent.js index 140c2c58077..858a5f248f7 100644 --- a/lib/dispatcher/agent.js +++ b/lib/dispatcher/agent.js @@ -1,7 +1,7 @@ 'use strict' const { InvalidArgumentError, MaxOriginsReachedError } = require('../core/errors') -const { kBusy, kClients, kRunning, kClose, kDestroy, kDispatch, kUrl } = require('../core/symbols') +const { kBusy, kClients, kConnected, kRunning, kClose, kDestroy, kDispatch, kUrl } = require('../core/symbols') const DispatcherBase = require('./dispatcher-base') const Pool = require('./pool') const Client = require('./client') @@ -65,7 +65,7 @@ class Agent extends DispatcherBase { get [kRunning] () { let ret = 0 - for (const { dispatcher } of this[kClients].values()) { + for (const dispatcher of this[kClients].values()) { ret += dispatcher[kRunning] } return ret @@ -86,54 +86,52 @@ class Agent extends DispatcherBase { throw new MaxOriginsReachedError() } - const result = this[kClients].get(key) - let dispatcher = result && result.dispatcher + let dispatcher = this[kClients].get(key) if (!dispatcher) { - const closeClientIfUnused = (connected) => { - const result = this[kClients].get(key) - if (result) { - if (connected) result.count -= 1 - if (result.count <= 0 && !result.dispatcher[kBusy]) { - this[kClients].delete(key) - if (!result.dispatcher.destroyed) { - result.dispatcher.close() - } - - let hasOrigin = false - for (const entry of this[kClients].values()) { - if (entry.origin === origin) { - hasOrigin = true - break - } - } - - if (!hasOrigin) { - this[kOrigins].delete(origin) - } - } - } - } dispatcher = this[kFactory](opts.origin, allowH2 === false ? { ...this[kOptions], allowH2: false } : this[kOptions]) - .on('drain', this[kOnDrain]) - .on('connect', (origin, targets) => { - const result = this[kClients].get(key) - if (result) { - result.count += 1 + + const closeClientIfUnused = () => { + if (this[kClients].get(key) !== dispatcher) { + return + } + + if (dispatcher[kConnected] > 0 || dispatcher[kBusy]) { + return + } + + this[kClients].delete(key) + if (!dispatcher.destroyed) { + dispatcher.close() + } + + let hasOrigin = false + for (const client of this[kClients].values()) { + if (client[kUrl].origin === dispatcher[kUrl].origin) { + hasOrigin = true + break } - this[kOnConnect](origin, targets) - }) + } + + if (!hasOrigin) { + this[kOrigins].delete(dispatcher[kUrl].origin) + } + } + + dispatcher + .on('drain', this[kOnDrain]) + .on('connect', this[kOnConnect]) .on('disconnect', (origin, targets, err) => { - closeClientIfUnused(true) + closeClientIfUnused() this[kOnDisconnect](origin, targets, err) }) .on('connectionError', (origin, targets, err) => { - closeClientIfUnused(false) + closeClientIfUnused() this[kOnConnectionError](origin, targets, err) }) - this[kClients].set(key, { count: 0, dispatcher, origin }) + this[kClients].set(key, dispatcher) this[kOrigins].add(origin) } @@ -142,7 +140,7 @@ class Agent extends DispatcherBase { [kClose] () { const closePromises = [] - for (const { dispatcher } of this[kClients].values()) { + for (const dispatcher of this[kClients].values()) { closePromises.push(dispatcher.close()) } this[kClients].clear() @@ -152,7 +150,7 @@ class Agent extends DispatcherBase { [kDestroy] (err) { const destroyPromises = [] - for (const { dispatcher } of this[kClients].values()) { + for (const dispatcher of this[kClients].values()) { destroyPromises.push(dispatcher.destroy(err)) } this[kClients].clear() @@ -162,7 +160,7 @@ class Agent extends DispatcherBase { get stats () { const allClientStats = {} - for (const { dispatcher } of this[kClients].values()) { + for (const dispatcher of this[kClients].values()) { if (dispatcher.stats) { allClientStats[dispatcher[kUrl].origin] = dispatcher.stats } diff --git a/lib/mock/mock-agent.js b/lib/mock/mock-agent.js index 61449e077ea..17a7b717c21 100644 --- a/lib/mock/mock-agent.js +++ b/lib/mock/mock-agent.js @@ -167,7 +167,7 @@ class MockAgent extends Dispatcher { } [kMockAgentSet] (origin, dispatcher) { - this[kClients].set(origin, { count: 0, dispatcher }) + this[kClients].set(origin, dispatcher) } [kFactory] (origin) { @@ -179,9 +179,9 @@ class MockAgent extends Dispatcher { [kMockAgentGet] (origin) { // First check if we can immediately find it - const result = this[kClients].get(origin) - if (result?.dispatcher) { - return result.dispatcher + const dispatcher = this[kClients].get(origin) + if (dispatcher) { + return dispatcher } // If the origin is not a string create a dummy parent pool and return to user @@ -192,11 +192,11 @@ class MockAgent extends Dispatcher { } // If we match, create a pool and assign the same dispatches - for (const [keyMatcher, result] of Array.from(this[kClients])) { - if (result && typeof keyMatcher !== 'string' && matchValue(keyMatcher, origin)) { + for (const [keyMatcher, nonExplicitDispatcher] of Array.from(this[kClients])) { + if (nonExplicitDispatcher && typeof keyMatcher !== 'string' && matchValue(keyMatcher, origin)) { const dispatcher = this[kFactory](origin) this[kMockAgentSet](origin, dispatcher) - dispatcher[kDispatches] = result.dispatcher[kDispatches] + dispatcher[kDispatches] = nonExplicitDispatcher[kDispatches] return dispatcher } } @@ -210,7 +210,7 @@ class MockAgent extends Dispatcher { const mockAgentClients = this[kClients] return Array.from(mockAgentClients.entries()) - .flatMap(([origin, result]) => result.dispatcher[kDispatches].map(dispatch => ({ ...dispatch, origin }))) + .flatMap(([origin, dispatcher]) => dispatcher[kDispatches].map(dispatch => ({ ...dispatch, origin }))) .filter(({ pending }) => pending) } From 6e2a7da30a69c805ae95368d9d6dbcc77cbeeaf7 Mon Sep 17 00:00:00 2001 From: bienzaaron Date: Sun, 19 Apr 2026 18:30:28 -0400 Subject: [PATCH 5/7] fix: give test more appropriate name --- test/{issue-4244.js => agent-connection-management.js} | 3 +++ 1 file changed, 3 insertions(+) rename test/{issue-4244.js => agent-connection-management.js} (97%) diff --git a/test/issue-4244.js b/test/agent-connection-management.js similarity index 97% rename from test/issue-4244.js rename to test/agent-connection-management.js index 268a81ebe09..34f42790ffd 100644 --- a/test/issue-4244.js +++ b/test/agent-connection-management.js @@ -62,7 +62,10 @@ describe('Agent should close inactive clients', () => { await p }) +}) +// https://github.com/nodejs/undici/issues/5022 +describe('Agent should not close active clients', () => { test('should reuse replacement keep-alive connection after server closes the previous one', async (t) => { let nextSocketId = 0 const socketIds = new Map() From 5d75f78a85d11d008cebb6136da9fb6ccee9ce36 Mon Sep 17 00:00:00 2001 From: bienzaaron Date: Mon, 20 Apr 2026 09:00:30 -0400 Subject: [PATCH 6/7] test: update assertions for `maxRequestsPerClient` to accurately assert number of requests --- test/close-and-destroy.js | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/test/close-and-destroy.js b/test/close-and-destroy.js index e52c0072553..582bd8cbaba 100644 --- a/test/close-and-destroy.js +++ b/test/close-and-destroy.js @@ -265,17 +265,20 @@ test('close after and destroy should error', async (t) => { test('close socket and reconnect after maxRequestsPerClient reached', async (t) => { t = tspl(t, { plan: 1 }) + let nextConnectionId = 0 + const socketToIdMap = new Map() + const connectionUsedForRequest = [] const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + connectionUsedForRequest.push(socketToIdMap.get(req.socket)) res.end(req.url) }) after(() => server.close()) server.listen(0, async () => { - let connections = 0 - server.on('connection', () => { - connections++ + server.on('connection', (sock) => { + socketToIdMap.set(sock, nextConnectionId++) }) const client = new Client( `http://localhost:${server.address().port}`, @@ -287,7 +290,7 @@ test('close socket and reconnect after maxRequestsPerClient reached', async (t) await makeRequest() await makeRequest() await makeRequest() - t.strictEqual(connections, 2) + t.deepEqual(connectionUsedForRequest, [0, 0, 1, 1]) function makeRequest () { return client.request({ path: '/', method: 'GET' }) @@ -299,17 +302,20 @@ test('close socket and reconnect after maxRequestsPerClient reached', async (t) test('close socket and reconnect after maxRequestsPerClient reached (async)', async (t) => { t = tspl(t, { plan: 1 }) + let nextConnectionId = 0 + const socketToIdMap = new Map() + const connectionUsedForRequest = [] const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + connectionUsedForRequest.push(socketToIdMap.get(req.socket)) res.end(req.url) }) after(() => server.close()) server.listen(0, async () => { - let connections = 0 - server.on('connection', () => { - connections++ + server.on('connection', (sock) => { + socketToIdMap.set(sock, nextConnectionId++) }) const client = new Client( `http://localhost:${server.address().port}`, @@ -323,7 +329,7 @@ test('close socket and reconnect after maxRequestsPerClient reached (async)', as makeRequest(), makeRequest() ]) - t.strictEqual(connections, 2) + t.deepEqual(connectionUsedForRequest, [0, 0, 1, 1]) function makeRequest () { return client.request({ path: '/', method: 'GET' }) From d897e6e09238d7fc7324df716b60897084c6b465 Mon Sep 17 00:00:00 2001 From: bienzaaron Date: Mon, 20 Apr 2026 09:05:45 -0400 Subject: [PATCH 7/7] test: replace `Promise` constructor with `Promise.withResolvers` --- test/agent-connection-management.js | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/test/agent-connection-management.js b/test/agent-connection-management.js index 34f42790ffd..cd520a304de 100644 --- a/test/agent-connection-management.js +++ b/test/agent-connection-management.js @@ -17,17 +17,15 @@ describe('Agent should close inactive clients', () => { server.close() }) + /** @type {Promise} */ let p const agent = new Agent({ factory: (origin, opts) => { const pool = new Pool(origin, opts) - let _resolve, _reject - p = new Promise((resolve, reject) => { - _resolve = resolve - _reject = reject - }) + const { promise, resolve, reject } = Promise.withResolvers() + p = promise pool.on('disconnect', () => { - setImmediate(() => pool.destroyed ? _resolve() : _reject(new Error('client not destroyed'))) + setImmediate(() => pool.destroyed ? resolve() : reject(new Error('client not destroyed'))) }) return pool } @@ -39,17 +37,15 @@ describe('Agent should close inactive clients', () => { }) test('in case of connection error', async (t) => { + /** @type {Promise} */ let p const agent = new Agent({ factory: (origin, opts) => { const pool = new Pool(origin, opts) - let _resolve, _reject - p = new Promise((resolve, reject) => { - _resolve = resolve - _reject = reject - }) + const { promise, resolve, reject } = Promise.withResolvers() + p = promise pool.on('connectionError', () => { - setImmediate(() => pool.destroyed ? _resolve() : _reject(new Error('client not destroyed'))) + setImmediate(() => pool.destroyed ? resolve() : reject(new Error('client not destroyed'))) }) return pool }