chore: guard client[kUrl] in closeClientIfUnused loop

lib/dispatcher/agent.js

@@ -108,14 +108,14 @@ class Agent extends DispatcherBase {
 
         let hasOrigin = false
         for (const client of this[kClients].values()) {
-          if (client[kUrl].origin === dispatcher[kUrl].origin) {
+          if (client[kUrl]?.origin === dispatcher[kUrl]?.origin) {
             hasOrigin = true
             break
           }
         }
 
         if (!hasOrigin) {
-          this[kOrigins].delete(dispatcher[kUrl].origin)
+          this[kOrigins].delete(dispatcher[kUrl]?.origin)
         }
       }
 

test/agent-connection-management.js

@@ -1,7 +1,9 @@
 const { test, describe } = require('node:test')
 const assert = require('node:assert')
 const { createServer } = require('node:http')
+const { EventEmitter } = require('node:events')
 const { request, Agent, Pool } = require('..')
+const { kBusy, kConnected, kDispatch, kRunning, kUrl } = require('../lib/core/symbols')
 
 // https://github.com/nodejs/undici/issues/4424
 describe('Agent should close inactive clients', () => {
@@ -62,6 +64,156 @@ describe('Agent should close inactive clients', () => {
 
 // https://github.com/nodejs/undici/issues/5022
 describe('Agent should not close active clients', () => {
+  class FakeDispatcher extends EventEmitter {
+    constructor (origin, { withUrl = false } = {}) {
+      super()
+      this[kConnected] = 0
+      this[kBusy] = false
+      this[kRunning] = 0
+      this.destroyed = false
+      this.dispatchCalls = 0
+
+      if (withUrl) {
+        this[kUrl] = new URL(origin)
+      }
+    }
+
+    dispatch () {
+      this.dispatchCalls++
+      return true
+    }
+
+    close () {
+      this.destroyed = true
+      return Promise.resolve()
+    }
+
+    destroy () {
+      this.destroyed = true
+      return Promise.resolve()
+    }
+  }
+
+  test('should ignore stale disconnect from replaced client', async () => {
+    const origin = 'http://localhost:3000'
+    const created = []
+    const agent = new Agent({
+      factory: (factoryOrigin) => {
+        const dispatcher = new FakeDispatcher(String(factoryOrigin))
+        created.push(dispatcher)
+        return dispatcher
+      }
+    })
+
+    const handler = {}
+
+    agent[kDispatch]({ origin }, handler)
+    assert.equal(created.length, 1)
+
+    const stale = created[0]
+    stale.emit('disconnect', origin, [stale], new Error('first close'))
+
+    agent[kDispatch]({ origin }, handler)
+    assert.equal(created.length, 2)
+
+    // Simulate a late event from a stale dispatcher after replacement exists.
+    stale.emit('disconnect', origin, [stale], new Error('late close'))
+
+    agent[kDispatch]({ origin }, handler)
+    assert.equal(created.length, 2)
+    assert.equal(created[1].dispatchCalls, 2)
+
+    await agent.close()
+  })
+
+  test('should ignore stale connectionError from replaced client with kUrl', async () => {
+    const origin = 'http://localhost:3001'
+    const created = []
+    const agent = new Agent({
+      factory: (factoryOrigin) => {
+        const dispatcher = new FakeDispatcher(String(factoryOrigin), { withUrl: true })
+        created.push(dispatcher)
+        return dispatcher
+      }
+    })
+
+    const handler = {}
+
+    agent[kDispatch]({ origin }, handler)
+    assert.equal(created.length, 1)
+
+    const stale = created[0]
+    stale.emit('connectionError', origin, [stale], new Error('first error'))
+
+    agent[kDispatch]({ origin }, handler)
+    assert.equal(created.length, 2)
+
+    // Simulate a late connectionError from stale dispatcher after replacement exists.
+    stale.emit('connectionError', origin, [stale], new Error('late error'))
+
+    agent[kDispatch]({ origin }, handler)
+    assert.equal(created.length, 2)
+    assert.equal(created[1].dispatchCalls, 2)
+
+    await agent.close()
+  })
+
+  test('should handle remaining client with kUrl while cleaning replaced client', async () => {
+    const origin = 'http://localhost:3002'
+    const created = []
+    const agent = new Agent({
+      factory: (factoryOrigin) => {
+        const dispatcher = new FakeDispatcher(String(factoryOrigin), { withUrl: true })
+        created.push(dispatcher)
+        return dispatcher
+      }
+    })
+
+    const handler = {}
+
+    agent[kDispatch]({ origin }, handler)
+    agent[kDispatch]({ origin, allowH2: false }, handler)
+    assert.equal(created.length, 2)
+
+    assert.doesNotThrow(() => {
+      created[0].emit('disconnect', origin, [created[0]], new Error('disconnect'))
+    })
+
+    // A replacement for the non-http1-only key should still be creatable.
+    agent[kDispatch]({ origin }, handler)
+    assert.equal(created.length, 3)
+
+    await agent.close()
+  })
+
+  test('should handle remaining client without kUrl while cleaning replaced client', async () => {
+    const origin = 'http://localhost:3003'
+    const created = []
+    const agent = new Agent({
+      factory: (factoryOrigin) => {
+        const dispatcher = new FakeDispatcher(String(factoryOrigin))
+        created.push(dispatcher)
+        return dispatcher
+      }
+    })
+
+    const handler = {}
+
+    agent[kDispatch]({ origin }, handler)
+    agent[kDispatch]({ origin, allowH2: false }, handler)
+    assert.equal(created.length, 2)
+
+    assert.doesNotThrow(() => {
+      created[0].emit('disconnect', origin, [created[0]], new Error('disconnect'))
+    })
+
+    // A replacement for the non-http1-only key should still be creatable.
+    agent[kDispatch]({ origin }, handler)
+    assert.equal(created.length, 3)
+
+    await agent.close()
+  })
+
   test('should reuse replacement keep-alive connection after server closes the previous one', async (t) => {
     let nextSocketId = 0
     const socketIds = new Map()