Keep-Alive max parameter prevents connection reuse

[BrainTR]

Bug Description

When the server has a request limit per connection via the Keep-Alive max parameter, Undici reuses the initial connection until the limit is reached.
After the limit is reached, the server sends a "Connection: close" header, and Undici closes the connection.
In subsequent requests, despite the "Connection: Keep-Alive" header, Undici closes the connection.

Reproducible By

import { request, Agent } from 'undici';

const dispatcher = new Agent({ connections: 1 });

async function testRequest(id) {
    const url = 'https://tgftp.nws.noaa.gov/testfile';
    const { body, headers } = await request(url, { dispatcher });
    await body.dump();
    console.log({
        id,
        connection: headers['connection'],
        keepAlive: headers['keep-alive'],
    });
}

async function run() {
    for (let id = 1; id <= 110; id++) {
        await testRequest(id);
    }
}

run();

Expected Behavior

Undici should reuse connections when possible.

Logs

{ id: 1, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=100' } // new connection
{ id: 2, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=99' } // reused connection
{ id: 3, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=98' } // reused connection
{ id: 4, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=97' } // reused connection
...
{ id: 99, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=2' } // reused connection
{ id: 100, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=1' } // reused connection
{ id: 101, connection: 'close', keepAlive: undefined } // reused connection
{ id: 102, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=100' } // new connection
{ id: 103, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=100' } // new connection
{ id: 104, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=100' } // new connection
{ id: 105, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=100' } // new connection
...

Environment

• Node.js: v24.14.0 (undici 8.0.1)
• Platform: Ubuntu 24.04.4 LTS

[mcollina]

Thanks for reporting!

I'm not sure I follow. Can you also add the matching server to reproduce?

[BrainTR]

The URL I provided in the code already points to the matching server and targets an empty file for a faster response.
It is a well-known public FTP/HTTP server provided by NOAA.
The reproduction code should run as is.

[BrainTR]

I further investigated.
undici.Client doesn't have this issue and behaves as expected.

I think the issue is inside the undici.Agent
Removing this line partially resolves the issue, but not completely. Also it's probably breaking something else. I'm not familiar with the codebase so I don't know the optimal solution.

undici/lib/dispatcher/agent.js

Line 99 in 6824eed

result.dispatcher.close()

Logs after removing the line:

{ id: 1, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=100' } // new connection
{ id: 2, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=99' } // reused connection
...
{ id: 99, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=2' } // reused connection
{ id: 100, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=1' } // reused connection
{ id: 101, connection: 'close', keepAlive: undefined } // reused connection
{ id: 102, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=100' } // new connection
{ id: 103, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=100' } // new connection (shouldn't be)
{ id: 104, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=99' }  // reused connection
{ id: 105, connection: 'Keep-Alive', keepAlive: 'timeout=300, max=98' }  // reused connection

@mcollina could you take a look at this?

[metcoder95]

This one is tricky, I don't believe that deleting such as it can cause to do not preempt those clients who are not in usage; tho we might need to distinguish between a Client who's under keep-alive and stills not yet stale so it doesn't get preempted.

Nevertheless, opens another question, how do we preempt them then?

Overall, I'm on the side of not preempting if keep alive, but once expired tear it down (a bit more smart multi-origin Pooling)

[BrainTR]

Looks like this issue was introduced in v7.10.0 (probably related to #4223 and #4425). No issue in v7.9.0.
And it seems like a more general issue than I initially thought.

Here's another example:

import { request, Agent } from 'undici';

const dispatcher = new Agent({ connections: 1, maxRequestsPerClient: 3, allowH2: false });
dispatcher.on('connect', () => { console.log('new connection') });

async function testRequest(id) {
    const url = 'https://example.com/';
    const { body } = await request(url, { dispatcher });
    await body.dump();
    console.log(`request:${id} completed.`);
}

(async () => {
    const useDelay = false;
    for (let id = 1; id <= 10; id++) {
        await testRequest(id);
        useDelay && await new Promise(r => setTimeout(r, 1_000));
    }
})();

Logs (useDelay: false) - has issue

new connection
request:1 completed.
request:2 completed.
request:3 completed.
request:4 completed.
new connection
request:5 completed.
new connection
request:6 completed.
new connection
request:7 completed.

Logs (useDelay: true) - no issue

new connection
request:1 completed.
request:2 completed.
request:3 completed.
request:4 completed.
new connection
request:5 completed.
request:6 completed.
request:7 completed.
request:8 completed.
new connection
request:9 completed.
request:10 completed.

Also, maxRequestsPerClient has a bug causing one extra request before the connection closes, fixed in #5033.

undici/lib/dispatcher/agent.js

Lines 120 to 124 in 6824eed

	.on('connect', (origin, targets) => {
	const result = this[kClients].get(key)
	if (result) {
	result.count += 1
	}

result is always undefined after the first connection close.