From 5f15b442f3a947cac393116a3ba7a3985ce0e039 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 17 Apr 2026 10:37:34 +0000 Subject: [PATCH 1/5] fix: preserve MockAgent interception for native fetch --- lib/dispatcher/agent.js | 11 ++++++- lib/mock/mock-agent.js | 24 +++++++++++--- test/node-test/global-dispatcher-version.js | 36 +++++++++++++++++++++ 3 files changed, 65 insertions(+), 6 deletions(-) diff --git a/lib/dispatcher/agent.js b/lib/dispatcher/agent.js index a1cc7fd6817..7c743b76027 100644 --- a/lib/dispatcher/agent.js +++ b/lib/dispatcher/agent.js @@ -15,12 +15,18 @@ const kFactory = Symbol('factory') const kOptions = Symbol('options') const kOrigins = Symbol('origins') +const HTTP1_ONLY_SUFFIX = '#http1-only' + function defaultFactory (origin, opts) { return opts && opts.connections === 1 ? new Client(origin, opts) : new Pool(origin, opts) } +function getOriginKey (origin, allowH2) { + return allowH2 === false ? `${origin}${HTTP1_ONLY_SUFFIX}` : origin +} + class Agent extends DispatcherBase { constructor ({ factory = defaultFactory, maxOrigins = Infinity, connect, ...options } = {}) { if (typeof factory !== 'function') { @@ -80,7 +86,7 @@ class Agent extends DispatcherBase { } const allowH2 = opts.allowH2 ?? this[kOptions].allowH2 - const key = allowH2 === false ? `${origin}#http1-only` : origin + const key = getOriginKey(origin, allowH2) if (this[kOrigins].size >= this[kOptions].maxOrigins && !this[kOrigins].has(origin)) { throw new MaxOriginsReachedError() @@ -171,4 +177,7 @@ class Agent extends DispatcherBase { } } +Agent.getOriginKey = getOriginKey +Agent.HTTP1_ONLY_SUFFIX = HTTP1_ONLY_SUFFIX + module.exports = Agent diff --git a/lib/mock/mock-agent.js b/lib/mock/mock-agent.js index 61449e077ea..22e99e99748 100644 --- a/lib/mock/mock-agent.js +++ b/lib/mock/mock-agent.js @@ -73,7 +73,7 @@ class MockAgent extends Dispatcher { opts.origin = normalizeOrigin(opts.origin) // Call MockAgent.get to perform additional setup before dispatching as normal - this.get(opts.origin) + const baseDispatcher = this.get(opts.origin) this[kMockAgentAddCallHistoryLog](opts) @@ -87,7 +87,20 @@ class MockAgent extends Dispatcher { dispatchOpts.path = `${path}?${normalizedSearchParams}` } - return this[kAgent].dispatch(dispatchOpts, handler) + if (dispatchOpts.allowH2 === false && typeof dispatchOpts.origin === 'string') { + const originKey = Agent.getOriginKey(dispatchOpts.origin, false) + let dispatcher = this[kMockAgentGet](originKey) + + if (!dispatcher) { + dispatcher = this[kFactory](dispatchOpts.origin, { ...this[kOptions], allowH2: false }) + this[kMockAgentSet](originKey, dispatcher) + } + + dispatcher[kDispatches] = baseDispatcher[kDispatches] + return dispatcher.dispatch(dispatchOpts, handler) + } + + return baseDispatcher.dispatch(dispatchOpts, handler) } async close () { @@ -170,9 +183,9 @@ class MockAgent extends Dispatcher { this[kClients].set(origin, { count: 0, dispatcher }) } - [kFactory] (origin) { - const mockOptions = Object.assign({ agent: this }, this[kOptions]) - return this[kOptions] && this[kOptions].connections === 1 + [kFactory] (origin, options = this[kOptions]) { + const mockOptions = Object.assign({ agent: this }, options) + return options && options.connections === 1 ? new MockClient(origin, mockOptions) : new MockPool(origin, mockOptions) } @@ -210,6 +223,7 @@ class MockAgent extends Dispatcher { const mockAgentClients = this[kClients] return Array.from(mockAgentClients.entries()) + .filter(([origin]) => typeof origin !== 'string' || !origin.endsWith(Agent.HTTP1_ONLY_SUFFIX)) .flatMap(([origin, result]) => result.dispatcher[kDispatches].map(dispatch => ({ ...dispatch, origin }))) .filter(({ pending }) => pending) } diff --git a/test/node-test/global-dispatcher-version.js b/test/node-test/global-dispatcher-version.js index e20a181a4dd..dd6db9e772b 100644 --- a/test/node-test/global-dispatcher-version.js +++ b/test/node-test/global-dispatcher-version.js @@ -97,6 +97,42 @@ test('setGlobalDispatcher mirrors a v1-compatible dispatcher that Node.js global assert.strictEqual(payload.mirroredV2, true) }) +test('setGlobalDispatcher mirrors a MockAgent that Node.js global fetch uses', () => { + const script = ` + const { MockAgent, setGlobalDispatcher } = require('./index.js') + const http = require('node:http') + const { once } = require('node:events') + + ;(async () => { + const server = http.createServer((req, res) => res.end('real')) + server.listen(0) + await once(server, 'listening') + + const origin = 'http://127.0.0.1:' + server.address().port + const agent = new MockAgent() + agent.disableNetConnect() + agent.get(origin).intercept({ + path: '/v1/test', + method: 'GET' + }).reply(200, 'mock') + + setGlobalDispatcher(agent) + + const res = await fetch(origin + '/v1/test') + process.stdout.write(await res.text()) + + server.close() + })().catch((err) => { + console.error(err?.cause?.stack || err?.stack || err) + process.exit(1) + }) + ` + + const result = runNode(script) + assert.strictEqual(result.status, 0, result.stderr) + assert.strictEqual(result.stdout, 'mock') +}) + test('Dispatcher1Wrapper bridges legacy handlers to a new Agent', () => { const script = ` const { Agent, Dispatcher1Wrapper } = require('./index.js') From b115e9f20e0959fc2a100d4ff55f56c6c06f78ac Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 17 Apr 2026 14:01:39 +0000 Subject: [PATCH 2/5] refactor: document Agent origin key helper --- lib/dispatcher/agent.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/dispatcher/agent.js b/lib/dispatcher/agent.js index 7c743b76027..86971987548 100644 --- a/lib/dispatcher/agent.js +++ b/lib/dispatcher/agent.js @@ -24,6 +24,8 @@ function defaultFactory (origin, opts) { } function getOriginKey (origin, allowH2) { + // Keep a separate cache entry for HTTP/1-only dispatchers so they do not + // share a pool with requests that may negotiate HTTP/2. return allowH2 === false ? `${origin}${HTTP1_ONLY_SUFFIX}` : origin } From ccab3f61373d178b2551c7db908d1897d9498c34 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 17 Apr 2026 14:19:39 +0000 Subject: [PATCH 3/5] refactor: register MockAgent protocol buckets eagerly --- DESIGN.md | 362 ++++++++++++++++++++++++++++++++++++++++ lib/mock/mock-agent.js | 37 ++-- lib/mock/mock-client.js | 7 +- lib/mock/mock-pool.js | 7 +- test/mock-agent.js | 23 ++- 5 files changed, 421 insertions(+), 15 deletions(-) create mode 100644 DESIGN.md diff --git a/DESIGN.md b/DESIGN.md new file mode 100644 index 00000000000..406adea75bc --- /dev/null +++ b/DESIGN.md @@ -0,0 +1,362 @@ +# Design proposal: make `Agent` protocol-aware without synthetic origin suffixes + +## Audience and assumptions + +This note is written for undici maintainers. + +It assumes the current context around: +- the legacy global dispatcher wrapper forcing `allowH2: false` +- `Agent` currently distinguishing HTTP/1-only dispatchers by appending `#http1-only` to the origin key +- `MockAgent` and other internal consumers needing to reason about the same dispatcher registry + +## Summary + +The current `#http1-only` key suffix works as a short-term fix, but it encodes routing state into a string key that other internals can accidentally depend on. + +As an incremental starting step, `MockAgent.get(origin)` can eagerly register both protocol buckets for string origins and make them share the same interceptor list. That removes the lazy repair currently needed during dispatch, while keeping behaviour correct for native `fetch()`. + +That step is still transitional. The longer-term goal is to make `Agent` explicitly protocol-aware without exposing its pool registry shape to `MockAgent` or any other internal consumer. + +A less brittle design is to make `Agent` explicitly protocol-aware: +- keep separate dispatcher maps for + - normal protocol-negotiating dispatchers + - HTTP/1-only dispatchers +- provide internal methods to query and mutate those maps by `(origin, allowH2)` +- stop requiring other modules to know how `Agent` encodes that distinction internally + +In particular, `MockAgent` should not need to know that HTTP/1-only dispatchers were historically represented as `origin + '#http1-only'`. + +## Problem statement + +Today, `Agent` stores dispatchers in a single map and synthesizes the map key from both: +- the request origin +- the routing constraint `allowH2 === false` + +Conceptually, the key space is: +- `https://example.com` +- `https://example.com#http1-only` + +That has a few problems. + +### 1. Routing semantics are encoded into a string + +The suffix is not part of the origin. It is an internal transport-selection detail. + +Encoding that detail into a string key makes it easy for other code to accidentally couple to: +- the exact suffix value +- the fact that the distinction is implemented as string concatenation +- suffix-based filtering such as `endsWith('#http1-only')` + +That is brittle because the representation leaks across module boundaries. + +### 2. Other internals need to reimplement `Agent` knowledge + +The regression behind #5036 happened because: +- `Dispatcher1Wrapper` forced `allowH2: false` +- `Agent` routed native `fetch()` through the HTTP/1-only bucket +- `MockAgent` had registered interceptors under the plain origin bucket only + +The direct bug was fixed, but the deeper issue remains: `MockAgent` still needs protocol-bucket knowledge from `Agent`. + +Even if the suffix string is centralized, the architectural dependency is still present. + +### 3. One map now mixes two different dimensions + +The current map combines: +- identity: `origin` +- policy: whether the dispatcher may negotiate HTTP/2 + +Those are different concerns. + +The implementation is simpler if the registry explicitly models both. + +## Goals + +1. Remove synthetic suffix parsing from internal consumers. +2. Make the dispatcher registry explicitly protocol-aware. +3. Let other internals query dispatcher state through methods, not key conventions. +4. Avoid exposing `Agent`'s underlying pool registry shape to `MockAgent`. +5. Preserve the current observable behaviour. +6. Keep the implementation internal; this does not need to become public API. + +## Non-goals + +- Redesigning the external `Agent` API +- Changing how ALPN or `allowH2` semantics work +- Unifying `MockAgent` and `Agent` into one type + +## Proposed design + +## 1. Split `Agent` storage into two maps + +Instead of one `kClients` map with encoded keys, keep two explicit maps: + +- `kClients` + - dispatchers for the normal path + - key: plain origin + - meaning: requests that may use the agent's configured protocol behaviour +- `kHttp1OnlyClients` + - dispatchers for requests forced to HTTP/1.1 + - key: plain origin + - meaning: requests with `allowH2 === false` + +This preserves the current behaviour while making the distinction explicit. + +Example shape: + +```js +const kClients = Symbol('clients') +const kHttp1OnlyClients = Symbol('http1-only clients') +``` + +Then `Agent` chooses the map directly: + +```js +const clients = allowH2 === false ? this[kHttp1OnlyClients] : this[kClients] +const result = clients.get(origin) +``` + +This removes the need for: +- synthetic origin keys +- suffix concatenation +- suffix filtering + +## 2. Add internal query/mutation methods on `Agent` + +The important change is not only storing entries separately, but also making `Agent` the owner of the lookup rules. + +These methods should not expose the raw registry maps or require `MockAgent` to manipulate pool entries directly. The point is to hide the storage shape behind internal semantics. + +Internal consumers should ask `Agent` questions like: +- "give me the dispatcher entry for this origin and routing mode" +- "set a dispatcher entry for this origin and routing mode" +- "delete the dispatcher entry for this origin and routing mode" +- "does this origin still have any dispatchers in either map?" + +A reasonable internal surface would be symbol-based methods rather than new public methods. + +For example: + +```js +const kGetDispatcherEntry = Symbol('get dispatcher entry') +const kSetDispatcherEntry = Symbol('set dispatcher entry') +const kDeleteDispatcherEntry = Symbol('delete dispatcher entry') +const kHasDispatcherForOrigin = Symbol('has dispatcher for origin') +const kForEachDispatcherEntry = Symbol('for each dispatcher entry') +``` + +Conceptually: + +```js +agent[kGetDispatcherEntry](origin, { allowH2 }) +agent[kSetDispatcherEntry](origin, { allowH2 }, entry) +agent[kDeleteDispatcherEntry](origin, { allowH2 }) +agent[kHasDispatcherForOrigin](origin) +agent[kForEachDispatcherEntry]((entry, meta) => { ... }) +``` + +Why symbol methods are preferable here: +- they keep the surface internal +- they avoid blessing these methods as supported public API +- they let `MockAgent` and similar internals depend on semantics, not representation + +## 3. Make `MockAgent` depend on the query methods, not the storage shape + +`MockAgent` currently reaches into agent internals and effectively shares the underlying client registry. + +That coupling is what made the suffix matter. + +With the proposed design, `MockAgent` should ask `Agent` for the correct bucket by routing mode rather than constructing keys itself. + +At minimum, that means: +- no string concatenation to build HTTP/1-only keys +- no suffix-based filtering when enumerating pending interceptors +- no assumptions about how `Agent` partitions dispatcher entries + +A better internal structure for `MockAgent` is: +- interceptor registries keyed by normalized origin only +- dispatcher instances resolved through `Agent` protocol-aware queries + +That separates two concerns cleanly: +- **mock matching**: keyed by logical origin and request properties +- **transport routing**: keyed by origin plus protocol policy + +### Why this matters + +The interceptors should be logically attached to the origin, not to an implementation-specific registry key. + +Whether the request took: +- the default path +- the HTTP/1-only path + +should not change how interceptors are matched or reported. + +## 4. Iterate across both maps through one helper + +Several `Agent` operations need to consider all dispatcher entries: +- `close()` +- `destroy()` +- `stats` +- origin cleanup + +Those should not manually duplicate logic. + +Add one internal iterator/helper that visits both maps. + +Conceptually: + +```js +forEachDispatcherEntry (fn) { + for (const [origin, entry] of this[kClients]) { + fn(entry, { origin, allowH2: this[kOptions].allowH2 !== false }) + } + + for (const [origin, entry] of this[kHttp1OnlyClients]) { + fn(entry, { origin, allowH2: false }) + } +} +``` + +That gives the implementation one place to define what counts as "all dispatchers". + +## Alternative shapes considered + +## A. Keep the suffix, but centralize it + +This is the current stopgap direction. + +Pros: +- minimal code churn +- fixes the immediate regression + +Cons: +- still leaks implementation details across modules +- still requires suffix-aware filtering and reasoning +- still mixes identity and routing policy in one string key + +This is acceptable as a short-term patch, but not ideal as the long-term design. + +## B. Nested map: `Map` + +This is also a valid design. + +Pros: +- models both dispatchers under the same origin naturally +- origin cleanup becomes straightforward + +Cons: +- slightly more bookkeeping in the hot path +- more complex mutation code than two direct maps + +This would be more explicit than the suffix approach and is arguably the cleanest data model. + +I still slightly prefer two maps plus query methods because: +- it keeps hot-path lookups very direct +- it maps closely to the current implementation +- it minimizes migration cost + +That said, nested maps would also solve the brittleness problem. + +## Detailed behaviour proposal + +## Dispatcher lookup + +Given `(origin, allowH2)`: +- if `allowH2 === false`, use the HTTP/1-only registry +- otherwise use the default registry + +No encoded key should be constructed. + +## Dispatcher creation + +When a dispatcher is missing: +- create it using the appropriate options +- store it in the selected registry +- keep origin tracking separate from storage shape + +## Origin tracking + +`kOrigins` should reflect whether an origin exists in either registry. + +That means: +- adding an entry in either map adds the origin +- deleting an entry removes the origin only if neither map still contains it + +This is more robust than implicitly inferring it from encoded keys. + +## Mock dispatcher association + +If `MockAgent` needs both protocol-specific dispatchers to share the same interceptor list, that sharing should happen explicitly, not via key convention. + +For example, `MockAgent` can maintain: + +```js +Map +``` + +and then create protocol-specific mock dispatchers that each reference the same `dispatches` array. + +This is preferable to storing duplicate logical state under two registry keys and then trying to filter one copy out of diagnostics. + +## Migration plan + +### Phase 0: simplify the current `MockAgent` workaround + +As a starting step, when `MockAgent.get(origin)` receives a concrete string origin: +- create the default mock dispatcher bucket +- create the HTTP/1-only mock dispatcher bucket as well +- make both dispatchers share the same interceptor list + +This is still a stopgap, but it is better than lazily creating the HTTP/1-only bucket during dispatch because: +- the mirrored state is established at registration time +- native `fetch()` and normal undici requests see the same mock setup immediately +- `MockAgent.dispatch()` no longer needs to repair missing protocol buckets on the fly + +This phase should not be treated as the final design because `MockAgent` still relies on `Agent`'s current registry convention. + +### Phase 1: introduce protocol-aware storage in `Agent` + +- add `kHttp1OnlyClients` +- add internal query/mutation helpers +- make `Agent`'s own lookup path use those helpers +- keep behaviour unchanged + +### Phase 2: migrate internal consumers + +Update `MockAgent` and any similar internal users to stop depending on: +- `origin + suffix` +- `endsWith(suffix)` +- direct assumptions about the shape of `agent[kClients]` + +### Phase 3: remove suffix compatibility logic + +Once all internal users are converted: +- delete key-suffix helpers +- delete any code that filters synthetic entries in diagnostics + +## Testing plan + +At minimum: + +1. Existing regression test for native `fetch()` + `MockAgent` +2. Native `WebSocket` regression tests that motivated the HTTP/1-only routing +3. `MockAgent` pending interceptor diagnostics +4. `close()` / `destroy()` coverage for both dispatcher registries +5. Stats/origin cleanup behaviour when only one of the two registries is populated + +Additional targeted tests: +- same origin can have both a default dispatcher and an HTTP/1-only dispatcher simultaneously +- deleting one does not remove the other +- deleting the last one removes the origin from origin tracking +- `MockAgent` interceptor reporting does not duplicate shared interceptor state + +## Recommendation + +Treat the current suffix-based fix as the compatibility patch, not the final architecture. + +The long-term direction should be: +- explicit protocol-aware dispatcher registries in `Agent` +- internal query methods for dispatcher lookup and iteration +- `MockAgent` depending on those methods instead of key conventions + +That design is less brittle because it makes the transport distinction explicit in the data model instead of hiding it inside a string. diff --git a/lib/mock/mock-agent.js b/lib/mock/mock-agent.js index 22e99e99748..9ba56fbc2af 100644 --- a/lib/mock/mock-agent.js +++ b/lib/mock/mock-agent.js @@ -23,11 +23,13 @@ const { const MockClient = require('./mock-client') const MockPool = require('./mock-pool') const { matchValue, normalizeSearchParams, buildAndValidateMockOptions, normalizeOrigin } = require('./mock-utils') -const { InvalidArgumentError, UndiciError } = require('../core/errors') +const { ClientDestroyedError, InvalidArgumentError, UndiciError } = require('../core/errors') const Dispatcher = require('../dispatcher/dispatcher') const PendingInterceptorsFormatter = require('./pending-interceptors-formatter') const { MockCallHistory } = require('./mock-call-history') +const kClosed = Symbol('closed') + class MockAgent extends Dispatcher { constructor (opts = {}) { super(opts) @@ -36,6 +38,7 @@ class MockAgent extends Dispatcher { this[kNetConnect] = true this[kIsMockActive] = true + this[kClosed] = false this[kMockAgentIsCallHistoryEnabled] = mockOptions.enableCallHistory ?? false this[kMockAgentAcceptsNonStandardSearchParameters] = mockOptions.acceptNonStandardSearchParameters ?? false this[kIgnoreTrailingSlash] = mockOptions.ignoreTrailingSlash ?? false @@ -66,10 +69,31 @@ class MockAgent extends Dispatcher { dispatcher = this[kFactory](originKey) this[kMockAgentSet](originKey, dispatcher) } + + if (typeof originKey === 'string') { + const http1OriginKey = Agent.getOriginKey(originKey, false) + let http1Dispatcher = this[kClients].get(http1OriginKey)?.dispatcher + + if (!http1Dispatcher) { + http1Dispatcher = this[kFactory](originKey, { ...this[kOptions], allowH2: false }) + this[kMockAgentSet](http1OriginKey, http1Dispatcher) + http1Dispatcher[kDispatches] = dispatcher[kDispatches] + } + } + return dispatcher } dispatch (opts, handler) { + if (this[kClosed]) { + const err = new ClientDestroyedError() + if (typeof handler?.onResponseError === 'function') { + handler.onResponseError(null, err) + return false + } + throw err + } + opts.origin = normalizeOrigin(opts.origin) // Call MockAgent.get to perform additional setup before dispatching as normal @@ -88,15 +112,7 @@ class MockAgent extends Dispatcher { } if (dispatchOpts.allowH2 === false && typeof dispatchOpts.origin === 'string') { - const originKey = Agent.getOriginKey(dispatchOpts.origin, false) - let dispatcher = this[kMockAgentGet](originKey) - - if (!dispatcher) { - dispatcher = this[kFactory](dispatchOpts.origin, { ...this[kOptions], allowH2: false }) - this[kMockAgentSet](originKey, dispatcher) - } - - dispatcher[kDispatches] = baseDispatcher[kDispatches] + const dispatcher = this[kClients].get(Agent.getOriginKey(dispatchOpts.origin, false))?.dispatcher return dispatcher.dispatch(dispatchOpts, handler) } @@ -104,6 +120,7 @@ class MockAgent extends Dispatcher { } async close () { + this[kClosed] = true this.clearCallHistory() await this[kAgent].close() this[kClients].clear() diff --git a/lib/mock/mock-client.js b/lib/mock/mock-client.js index b3be7ab3b91..c5dd9daf300 100644 --- a/lib/mock/mock-client.js +++ b/lib/mock/mock-client.js @@ -61,7 +61,12 @@ class MockClient extends Client { async [kClose] () { await promisify(this[kOriginalClose])() this[kConnected] = 0 - this[kMockAgent][Symbols.kClients].delete(this[kOrigin]) + + for (const [origin, result] of this[kMockAgent][Symbols.kClients]) { + if (result?.dispatcher === this) { + this[kMockAgent][Symbols.kClients].delete(origin) + } + } } } diff --git a/lib/mock/mock-pool.js b/lib/mock/mock-pool.js index 2121e3c99a3..200aed73635 100644 --- a/lib/mock/mock-pool.js +++ b/lib/mock/mock-pool.js @@ -61,7 +61,12 @@ class MockPool extends Pool { async [kClose] () { await promisify(this[kOriginalClose])() this[kConnected] = 0 - this[kMockAgent][Symbols.kClients].delete(this[kOrigin]) + + for (const [origin, result] of this[kMockAgent][Symbols.kClients]) { + if (result?.dispatcher === this) { + this[kMockAgent][Symbols.kClients].delete(origin) + } + } } } diff --git a/test/mock-agent.js b/test/mock-agent.js index 500a8833116..c01ef08f92c 100644 --- a/test/mock-agent.js +++ b/test/mock-agent.js @@ -9,7 +9,7 @@ const { kClients, kConnected } = require('../lib/core/symbols') const { InvalidArgumentError, ClientDestroyedError } = require('../lib/core/errors') const MockClient = require('../lib/mock/mock-client') const MockPool = require('../lib/mock/mock-pool') -const { kAgent, kMockAgentIsCallHistoryEnabled } = require('../lib/mock/mock-symbols') +const { kAgent, kDispatches, kMockAgentIsCallHistoryEnabled } = require('../lib/mock/mock-symbols') const Dispatcher = require('../lib/dispatcher/dispatcher') const { MockNotMatchedError } = require('../lib/mock/mock-errors') const { fetch } = require('..') @@ -171,6 +171,23 @@ describe('MockAgent - get', () => { const mockPool2 = mockAgent.get(baseUrl) t.assert.strictEqual(mockPool1, mockPool2) }) + + test('should register both protocol buckets for string origins', (t) => { + t.plan(5) + + const baseUrl = 'http://localhost:9999' + const mockAgent = new MockAgent() + after(() => mockAgent.close()) + + const mockPool = mockAgent.get(baseUrl) + const http1OnlyPool = mockAgent[kClients].get(Agent.getOriginKey(baseUrl, false)).dispatcher + + t.assert.strictEqual(mockAgent[kClients].size, 2) + t.assert.ok(http1OnlyPool instanceof MockPool) + t.assert.notStrictEqual(http1OnlyPool, mockPool) + t.assert.strictEqual(mockAgent[kClients].get(baseUrl).dispatcher, mockPool) + t.assert.strictEqual(http1OnlyPool[kDispatches], mockPool[kDispatches]) + }) }) describe('MockAgent - dispatch', () => { @@ -243,7 +260,7 @@ test('MockAgent - .close should clean up registered pools', async (t) => { t.assert.ok(mockPool instanceof MockPool) t.assert.strictEqual(mockPool[kConnected], 1) - t.assert.strictEqual(mockAgent[kClients].size, 1) + t.assert.strictEqual(mockAgent[kClients].size, 2) await mockAgent.close() t.assert.strictEqual(mockPool[kConnected], 0) t.assert.strictEqual(mockAgent[kClients].size, 0) @@ -261,7 +278,7 @@ test('MockAgent - .close should clean up registered clients', async (t) => { t.assert.ok(mockClient instanceof MockClient) t.assert.strictEqual(mockClient[kConnected], 1) - t.assert.strictEqual(mockAgent[kClients].size, 1) + t.assert.strictEqual(mockAgent[kClients].size, 2) await mockAgent.close() t.assert.strictEqual(mockClient[kConnected], 0) t.assert.strictEqual(mockAgent[kClients].size, 0) From b1386384fa7c92c794d2548cdd6d3fa4da04513c Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 17 Apr 2026 14:31:06 +0000 Subject: [PATCH 4/5] refactor: hide Agent protocol registries behind helpers --- DESIGN.md | 9 ++++ lib/core/symbols.js | 6 +++ lib/dispatcher/agent.js | 112 ++++++++++++++++++++++++++------------- lib/mock/mock-agent.js | 93 +++++++++++++++++++++++--------- lib/mock/mock-client.js | 8 +-- lib/mock/mock-pool.js | 8 +-- lib/mock/mock-symbols.js | 1 + test/mock-agent.js | 31 ++++++----- 8 files changed, 180 insertions(+), 88 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 406adea75bc..3eeb491f3fc 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -13,6 +13,15 @@ It assumes the current context around: The current `#http1-only` key suffix works as a short-term fix, but it encodes routing state into a string key that other internals can accidentally depend on. +## Implementation status + +This branch implements the first substantial slice of the design: +- `Agent` now keeps separate default and HTTP/1-only dispatcher registries +- `Agent` exposes internal symbol-based query/mutation helpers for those registries +- `MockAgent` uses those helpers instead of sharing `Agent`'s raw pool map + +The remaining design goal is to continue reducing coupling around mock dispatcher lifecycle and diagnostics. + As an incremental starting step, `MockAgent.get(origin)` can eagerly register both protocol buckets for string origins and make them share the same interceptor list. That removes the lazy repair currently needed during dispatch, while keeping behaviour correct for native `fetch()`. That step is still transitional. The longer-term goal is to make `Agent` explicitly protocol-aware without exposing its pool registry shape to `MockAgent` or any other internal consumer. diff --git a/lib/core/symbols.js b/lib/core/symbols.js index ff37adc0448..ea34f0db5ae 100644 --- a/lib/core/symbols.js +++ b/lib/core/symbols.js @@ -42,6 +42,12 @@ module.exports = { kPendingIdx: Symbol('pending index'), kError: Symbol('error'), kClients: Symbol('clients'), + kHttp1OnlyClients: Symbol('http1-only clients'), + kGetDispatcherEntry: Symbol('get dispatcher entry'), + kSetDispatcherEntry: Symbol('set dispatcher entry'), + kDeleteDispatcherEntry: Symbol('delete dispatcher entry'), + kHasDispatcherForOrigin: Symbol('has dispatcher for origin'), + kForEachDispatcherEntry: Symbol('for each dispatcher entry'), kClient: Symbol('client'), kParser: Symbol('parser'), kOnDestroyed: Symbol('destroy callbacks'), diff --git a/lib/dispatcher/agent.js b/lib/dispatcher/agent.js index 86971987548..576da75388b 100644 --- a/lib/dispatcher/agent.js +++ b/lib/dispatcher/agent.js @@ -1,7 +1,20 @@ 'use strict' const { InvalidArgumentError, MaxOriginsReachedError } = require('../core/errors') -const { kClients, kRunning, kClose, kDestroy, kDispatch, kUrl } = require('../core/symbols') +const { + kClients, + kHttp1OnlyClients, + kRunning, + kClose, + kDestroy, + kDispatch, + kUrl, + kGetDispatcherEntry, + kSetDispatcherEntry, + kDeleteDispatcherEntry, + kHasDispatcherForOrigin, + kForEachDispatcherEntry +} = require('../core/symbols') const DispatcherBase = require('./dispatcher-base') const Pool = require('./pool') const Client = require('./client') @@ -15,18 +28,14 @@ const kFactory = Symbol('factory') const kOptions = Symbol('options') const kOrigins = Symbol('origins') -const HTTP1_ONLY_SUFFIX = '#http1-only' - function defaultFactory (origin, opts) { return opts && opts.connections === 1 ? new Client(origin, opts) : new Pool(origin, opts) } -function getOriginKey (origin, allowH2) { - // Keep a separate cache entry for HTTP/1-only dispatchers so they do not - // share a pool with requests that may negotiate HTTP/2. - return allowH2 === false ? `${origin}${HTTP1_ONLY_SUFFIX}` : origin +function shouldUseHttp1OnlyClients (allowH2) { + return allowH2 === false } class Agent extends DispatcherBase { @@ -52,6 +61,7 @@ class Agent extends DispatcherBase { this[kOptions] = { ...util.deepClone(options), maxOrigins, connect } this[kFactory] = factory this[kClients] = new Map() + this[kHttp1OnlyClients] = new Map() this[kOrigins] = new Set() this[kOnDrain] = (origin, targets) => { @@ -73,12 +83,45 @@ class Agent extends DispatcherBase { get [kRunning] () { let ret = 0 - for (const { dispatcher } of this[kClients].values()) { + + this[kForEachDispatcherEntry](({ dispatcher }) => { ret += dispatcher[kRunning] - } + }) + return ret } + [kGetDispatcherEntry] (origin, { allowH2 } = {}) { + return (shouldUseHttp1OnlyClients(allowH2) ? this[kHttp1OnlyClients] : this[kClients]).get(origin) + } + + [kSetDispatcherEntry] (origin, { allowH2 } = {}, entry) { + ;(shouldUseHttp1OnlyClients(allowH2) ? this[kHttp1OnlyClients] : this[kClients]).set(origin, entry) + this[kOrigins].add(origin) + } + + [kDeleteDispatcherEntry] (origin, { allowH2 } = {}) { + ;(shouldUseHttp1OnlyClients(allowH2) ? this[kHttp1OnlyClients] : this[kClients]).delete(origin) + + if (!this[kHasDispatcherForOrigin](origin)) { + this[kOrigins].delete(origin) + } + } + + [kHasDispatcherForOrigin] (origin) { + return this[kClients].has(origin) || this[kHttp1OnlyClients].has(origin) + } + + [kForEachDispatcherEntry] (callback) { + for (const [origin, entry] of this[kClients]) { + callback(entry, { origin }) + } + + for (const [origin, entry] of this[kHttp1OnlyClients]) { + callback(entry, { origin, allowH2: false }) + } + } + [kDispatch] (opts, handler) { let origin if (opts.origin && (typeof opts.origin === 'string' || opts.origin instanceof URL)) { @@ -88,45 +131,34 @@ class Agent extends DispatcherBase { } const allowH2 = opts.allowH2 ?? this[kOptions].allowH2 - const key = getOriginKey(origin, allowH2) + const registry = { allowH2 } if (this[kOrigins].size >= this[kOptions].maxOrigins && !this[kOrigins].has(origin)) { throw new MaxOriginsReachedError() } - const result = this[kClients].get(key) + const result = this[kGetDispatcherEntry](origin, registry) let dispatcher = result && result.dispatcher if (!dispatcher) { const closeClientIfUnused = (connected) => { - const result = this[kClients].get(key) + const result = this[kGetDispatcherEntry](origin, registry) if (result) { if (connected) result.count -= 1 if (result.count <= 0) { - this[kClients].delete(key) + this[kDeleteDispatcherEntry](origin, registry) 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) + const result = this[kGetDispatcherEntry](origin, registry) if (result) { result.count += 1 } @@ -141,8 +173,7 @@ class Agent extends DispatcherBase { this[kOnConnectionError](origin, targets, err) }) - this[kClients].set(key, { count: 0, dispatcher, origin }) - this[kOrigins].add(origin) + this[kSetDispatcherEntry](origin, registry, { count: 0, dispatcher, origin }) } return dispatcher.dispatch(opts, handler) @@ -150,36 +181,43 @@ class Agent extends DispatcherBase { [kClose] () { const closePromises = [] - for (const { dispatcher } of this[kClients].values()) { + + this[kForEachDispatcherEntry](({ dispatcher }) => { closePromises.push(dispatcher.close()) - } + }) + this[kClients].clear() + this[kHttp1OnlyClients].clear() + this[kOrigins].clear() return Promise.all(closePromises) } [kDestroy] (err) { const destroyPromises = [] - for (const { dispatcher } of this[kClients].values()) { + + this[kForEachDispatcherEntry](({ dispatcher }) => { destroyPromises.push(dispatcher.destroy(err)) - } + }) + this[kClients].clear() + this[kHttp1OnlyClients].clear() + this[kOrigins].clear() return Promise.all(destroyPromises) } get stats () { const allClientStats = {} - for (const { dispatcher } of this[kClients].values()) { + + this[kForEachDispatcherEntry](({ dispatcher }) => { if (dispatcher.stats) { allClientStats[dispatcher[kUrl].origin] = dispatcher.stats } - } + }) + return allClientStats } } -Agent.getOriginKey = getOriginKey -Agent.HTTP1_ONLY_SUFFIX = HTTP1_ONLY_SUFFIX - module.exports = Agent diff --git a/lib/mock/mock-agent.js b/lib/mock/mock-agent.js index 9ba56fbc2af..3561d2597b8 100644 --- a/lib/mock/mock-agent.js +++ b/lib/mock/mock-agent.js @@ -1,11 +1,18 @@ 'use strict' -const { kClients } = require('../core/symbols') +const { + kClients, + kGetDispatcherEntry, + kSetDispatcherEntry, + kDeleteDispatcherEntry, + kForEachDispatcherEntry +} = require('../core/symbols') const Agent = require('../dispatcher/agent') const { kAgent, kMockAgentSet, kMockAgentGet, + kMockAgentUnregisterDispatches, kDispatches, kIsMockActive, kNetConnect, @@ -50,7 +57,7 @@ class MockAgent extends Dispatcher { const agent = opts?.agent ? opts.agent : new Agent(opts) this[kAgent] = agent - this[kClients] = agent[kClients] + this[kClients] = new Map() this[kOptions] = mockOptions if (this[kMockAgentIsCallHistoryEnabled]) { @@ -65,19 +72,32 @@ class MockAgent extends Dispatcher { let dispatcher = this[kMockAgentGet](originKey) + if (!dispatcher && typeof originKey === 'string') { + dispatcher = this[kAgent][kGetDispatcherEntry](originKey)?.dispatcher + } + + if (!dispatcher && typeof originKey === 'string') { + for (const [keyMatcher, result] of Array.from(this[kClients])) { + if (result && typeof keyMatcher !== 'string' && matchValue(keyMatcher, originKey)) { + dispatcher = this[kFactory](originKey) + dispatcher[kDispatches] = result.dispatcher[kDispatches] + break + } + } + } + if (!dispatcher) { dispatcher = this[kFactory](originKey) this[kMockAgentSet](originKey, dispatcher) } if (typeof originKey === 'string') { - const http1OriginKey = Agent.getOriginKey(originKey, false) - let http1Dispatcher = this[kClients].get(http1OriginKey)?.dispatcher + this[kAgent][kSetDispatcherEntry](originKey, {}, { count: 0, dispatcher, origin: originKey }) - if (!http1Dispatcher) { - http1Dispatcher = this[kFactory](originKey, { ...this[kOptions], allowH2: false }) - this[kMockAgentSet](http1OriginKey, http1Dispatcher) + if (!this[kAgent][kGetDispatcherEntry](originKey, { allowH2: false })) { + const http1Dispatcher = this[kFactory](originKey, { ...this[kOptions], allowH2: false }) http1Dispatcher[kDispatches] = dispatcher[kDispatches] + this[kAgent][kSetDispatcherEntry](originKey, { allowH2: false }, { count: 0, dispatcher: http1Dispatcher, origin: originKey }) } } @@ -97,7 +117,7 @@ class MockAgent extends Dispatcher { opts.origin = normalizeOrigin(opts.origin) // Call MockAgent.get to perform additional setup before dispatching as normal - const baseDispatcher = this.get(opts.origin) + this.get(opts.origin) this[kMockAgentAddCallHistoryLog](opts) @@ -111,17 +131,21 @@ class MockAgent extends Dispatcher { dispatchOpts.path = `${path}?${normalizedSearchParams}` } - if (dispatchOpts.allowH2 === false && typeof dispatchOpts.origin === 'string') { - const dispatcher = this[kClients].get(Agent.getOriginKey(dispatchOpts.origin, false))?.dispatcher - return dispatcher.dispatch(dispatchOpts, handler) - } - - return baseDispatcher.dispatch(dispatchOpts, handler) + return this[kAgent].dispatch(dispatchOpts, handler) } async close () { this[kClosed] = true this.clearCallHistory() + + const closePromises = [] + for (const [origin, result] of this[kClients]) { + if (typeof origin !== 'string') { + closePromises.push(result.dispatcher.close()) + } + } + + await Promise.all(closePromises) await this[kAgent].close() this[kClients].clear() } @@ -197,7 +221,7 @@ class MockAgent extends Dispatcher { } [kMockAgentSet] (origin, dispatcher) { - this[kClients].set(origin, { count: 0, dispatcher }) + this[kClients].set(origin, { dispatcher }) } [kFactory] (origin, options = this[kOptions]) { @@ -220,27 +244,44 @@ class MockAgent extends Dispatcher { this[kMockAgentSet](origin, dispatcher) return 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)) { - const dispatcher = this[kFactory](origin) - this[kMockAgentSet](origin, dispatcher) - dispatcher[kDispatches] = result.dispatcher[kDispatches] - return dispatcher - } - } } [kGetNetConnect] () { return this[kNetConnect] } + async [kMockAgentUnregisterDispatches] (dispatches, closingDispatcher) { + for (const [origin, result] of Array.from(this[kClients])) { + if (result?.dispatcher?.[kDispatches] === dispatches) { + this[kClients].delete(origin) + } + } + + const dispatchersToClose = new Set() + const entriesToDelete = [] + + this[kAgent][kForEachDispatcherEntry]((entry, meta) => { + if (entry.dispatcher[kDispatches] === dispatches) { + entriesToDelete.push(meta) + dispatchersToClose.add(entry.dispatcher) + } + }) + + for (const { origin, allowH2 } of entriesToDelete) { + this[kAgent][kDeleteDispatcherEntry](origin, { allowH2 }) + } + + for (const dispatcher of dispatchersToClose) { + if (dispatcher !== closingDispatcher && !dispatcher.closed && !dispatcher.destroyed) { + await dispatcher.close() + } + } + } + pendingInterceptors () { const mockAgentClients = this[kClients] return Array.from(mockAgentClients.entries()) - .filter(([origin]) => typeof origin !== 'string' || !origin.endsWith(Agent.HTTP1_ONLY_SUFFIX)) .flatMap(([origin, result]) => result.dispatcher[kDispatches].map(dispatch => ({ ...dispatch, origin }))) .filter(({ pending }) => pending) } diff --git a/lib/mock/mock-client.js b/lib/mock/mock-client.js index c5dd9daf300..57364ecfd79 100644 --- a/lib/mock/mock-client.js +++ b/lib/mock/mock-client.js @@ -6,6 +6,7 @@ const { buildMockDispatch } = require('./mock-utils') const { kDispatches, kMockAgent, + kMockAgentUnregisterDispatches, kClose, kOriginalClose, kOrigin, @@ -61,12 +62,7 @@ class MockClient extends Client { async [kClose] () { await promisify(this[kOriginalClose])() this[kConnected] = 0 - - for (const [origin, result] of this[kMockAgent][Symbols.kClients]) { - if (result?.dispatcher === this) { - this[kMockAgent][Symbols.kClients].delete(origin) - } - } + await this[kMockAgent][kMockAgentUnregisterDispatches](this[kDispatches], this) } } diff --git a/lib/mock/mock-pool.js b/lib/mock/mock-pool.js index 200aed73635..df0785f30c3 100644 --- a/lib/mock/mock-pool.js +++ b/lib/mock/mock-pool.js @@ -6,6 +6,7 @@ const { buildMockDispatch } = require('./mock-utils') const { kDispatches, kMockAgent, + kMockAgentUnregisterDispatches, kClose, kOriginalClose, kOrigin, @@ -61,12 +62,7 @@ class MockPool extends Pool { async [kClose] () { await promisify(this[kOriginalClose])() this[kConnected] = 0 - - for (const [origin, result] of this[kMockAgent][Symbols.kClients]) { - if (result?.dispatcher === this) { - this[kMockAgent][Symbols.kClients].delete(origin) - } - } + await this[kMockAgent][kMockAgentUnregisterDispatches](this[kDispatches], this) } } diff --git a/lib/mock/mock-symbols.js b/lib/mock/mock-symbols.js index 9b23e8e3cf0..4410adbce9b 100644 --- a/lib/mock/mock-symbols.js +++ b/lib/mock/mock-symbols.js @@ -12,6 +12,7 @@ module.exports = { kMockAgent: Symbol('mock agent'), kMockAgentSet: Symbol('mock agent set'), kMockAgentGet: Symbol('mock agent get'), + kMockAgentUnregisterDispatches: Symbol('mock agent unregister dispatches'), kMockDispatch: Symbol('mock dispatch'), kClose: Symbol('close'), kOriginalClose: Symbol('original agent close'), diff --git a/test/mock-agent.js b/test/mock-agent.js index c01ef08f92c..3d4dfc95736 100644 --- a/test/mock-agent.js +++ b/test/mock-agent.js @@ -5,7 +5,7 @@ const { createServer } = require('node:http') const { once } = require('node:events') const { request, setGlobalDispatcher, MockAgent, Agent } = require('..') const { getResponse } = require('../lib/mock/mock-utils') -const { kClients, kConnected } = require('../lib/core/symbols') +const { kClients, kConnected, kGetDispatcherEntry, kHttp1OnlyClients } = require('../lib/core/symbols') const { InvalidArgumentError, ClientDestroyedError } = require('../lib/core/errors') const MockClient = require('../lib/mock/mock-client') const MockPool = require('../lib/mock/mock-pool') @@ -172,21 +172,24 @@ describe('MockAgent - get', () => { t.assert.strictEqual(mockPool1, mockPool2) }) - test('should register both protocol buckets for string origins', (t) => { - t.plan(5) + test('should register protocol-specific dispatchers without exposing agent pools', (t) => { + t.plan(6) const baseUrl = 'http://localhost:9999' - const mockAgent = new MockAgent() + const agent = new Agent() + const mockAgent = new MockAgent({ agent }) after(() => mockAgent.close()) const mockPool = mockAgent.get(baseUrl) - const http1OnlyPool = mockAgent[kClients].get(Agent.getOriginKey(baseUrl, false)).dispatcher + const defaultPool = agent[kGetDispatcherEntry](baseUrl).dispatcher + const http1OnlyPool = agent[kGetDispatcherEntry](baseUrl, { allowH2: false }).dispatcher - t.assert.strictEqual(mockAgent[kClients].size, 2) + t.assert.strictEqual(mockAgent[kClients].size, 1) t.assert.ok(http1OnlyPool instanceof MockPool) t.assert.notStrictEqual(http1OnlyPool, mockPool) - t.assert.strictEqual(mockAgent[kClients].get(baseUrl).dispatcher, mockPool) + t.assert.strictEqual(defaultPool, mockPool) t.assert.strictEqual(http1OnlyPool[kDispatches], mockPool[kDispatches]) + t.assert.strictEqual(agent[kHttp1OnlyClients].size, 1) }) }) @@ -260,7 +263,7 @@ test('MockAgent - .close should clean up registered pools', async (t) => { t.assert.ok(mockPool instanceof MockPool) t.assert.strictEqual(mockPool[kConnected], 1) - t.assert.strictEqual(mockAgent[kClients].size, 2) + t.assert.strictEqual(mockAgent[kClients].size, 1) await mockAgent.close() t.assert.strictEqual(mockPool[kConnected], 0) t.assert.strictEqual(mockAgent[kClients].size, 0) @@ -278,14 +281,14 @@ test('MockAgent - .close should clean up registered clients', async (t) => { t.assert.ok(mockClient instanceof MockClient) t.assert.strictEqual(mockClient[kConnected], 1) - t.assert.strictEqual(mockAgent[kClients].size, 2) + t.assert.strictEqual(mockAgent[kClients].size, 1) await mockAgent.close() t.assert.strictEqual(mockClient[kConnected], 0) t.assert.strictEqual(mockAgent[kClients].size, 0) }) -test('MockAgent - [kClients] should match encapsulated agent', async (t) => { - t.plan(1) +test('MockAgent - [kClients] should not expose encapsulated agent pools', async (t) => { + t.plan(3) const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { res.setHeader('content-type', 'text/plain') @@ -312,8 +315,10 @@ test('MockAgent - [kClients] should match encapsulated agent', async (t) => { method: 'GET' }).reply(200, 'hello') - // The MockAgent should encapsulate the input agent clients - t.assert.strictEqual(mockAgent[kClients].size, agent[kClients].size) + // The MockAgent should not expose the encapsulated agent pool registry + t.assert.notStrictEqual(mockAgent[kClients], agent[kClients]) + t.assert.strictEqual(mockAgent[kClients].size, 1) + t.assert.strictEqual(agent[kClients].size, 1) }) test('MockAgent - basic intercept with MockAgent.request', async (t) => { From 72c3bad47046da483dc505b03db3fa4da46b94f1 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 17 Apr 2026 14:36:02 +0000 Subject: [PATCH 5/5] chore: remove design document --- DESIGN.md | 371 ------------------------------------------------------ 1 file changed, 371 deletions(-) delete mode 100644 DESIGN.md diff --git a/DESIGN.md b/DESIGN.md deleted file mode 100644 index 3eeb491f3fc..00000000000 --- a/DESIGN.md +++ /dev/null @@ -1,371 +0,0 @@ -# Design proposal: make `Agent` protocol-aware without synthetic origin suffixes - -## Audience and assumptions - -This note is written for undici maintainers. - -It assumes the current context around: -- the legacy global dispatcher wrapper forcing `allowH2: false` -- `Agent` currently distinguishing HTTP/1-only dispatchers by appending `#http1-only` to the origin key -- `MockAgent` and other internal consumers needing to reason about the same dispatcher registry - -## Summary - -The current `#http1-only` key suffix works as a short-term fix, but it encodes routing state into a string key that other internals can accidentally depend on. - -## Implementation status - -This branch implements the first substantial slice of the design: -- `Agent` now keeps separate default and HTTP/1-only dispatcher registries -- `Agent` exposes internal symbol-based query/mutation helpers for those registries -- `MockAgent` uses those helpers instead of sharing `Agent`'s raw pool map - -The remaining design goal is to continue reducing coupling around mock dispatcher lifecycle and diagnostics. - -As an incremental starting step, `MockAgent.get(origin)` can eagerly register both protocol buckets for string origins and make them share the same interceptor list. That removes the lazy repair currently needed during dispatch, while keeping behaviour correct for native `fetch()`. - -That step is still transitional. The longer-term goal is to make `Agent` explicitly protocol-aware without exposing its pool registry shape to `MockAgent` or any other internal consumer. - -A less brittle design is to make `Agent` explicitly protocol-aware: -- keep separate dispatcher maps for - - normal protocol-negotiating dispatchers - - HTTP/1-only dispatchers -- provide internal methods to query and mutate those maps by `(origin, allowH2)` -- stop requiring other modules to know how `Agent` encodes that distinction internally - -In particular, `MockAgent` should not need to know that HTTP/1-only dispatchers were historically represented as `origin + '#http1-only'`. - -## Problem statement - -Today, `Agent` stores dispatchers in a single map and synthesizes the map key from both: -- the request origin -- the routing constraint `allowH2 === false` - -Conceptually, the key space is: -- `https://example.com` -- `https://example.com#http1-only` - -That has a few problems. - -### 1. Routing semantics are encoded into a string - -The suffix is not part of the origin. It is an internal transport-selection detail. - -Encoding that detail into a string key makes it easy for other code to accidentally couple to: -- the exact suffix value -- the fact that the distinction is implemented as string concatenation -- suffix-based filtering such as `endsWith('#http1-only')` - -That is brittle because the representation leaks across module boundaries. - -### 2. Other internals need to reimplement `Agent` knowledge - -The regression behind #5036 happened because: -- `Dispatcher1Wrapper` forced `allowH2: false` -- `Agent` routed native `fetch()` through the HTTP/1-only bucket -- `MockAgent` had registered interceptors under the plain origin bucket only - -The direct bug was fixed, but the deeper issue remains: `MockAgent` still needs protocol-bucket knowledge from `Agent`. - -Even if the suffix string is centralized, the architectural dependency is still present. - -### 3. One map now mixes two different dimensions - -The current map combines: -- identity: `origin` -- policy: whether the dispatcher may negotiate HTTP/2 - -Those are different concerns. - -The implementation is simpler if the registry explicitly models both. - -## Goals - -1. Remove synthetic suffix parsing from internal consumers. -2. Make the dispatcher registry explicitly protocol-aware. -3. Let other internals query dispatcher state through methods, not key conventions. -4. Avoid exposing `Agent`'s underlying pool registry shape to `MockAgent`. -5. Preserve the current observable behaviour. -6. Keep the implementation internal; this does not need to become public API. - -## Non-goals - -- Redesigning the external `Agent` API -- Changing how ALPN or `allowH2` semantics work -- Unifying `MockAgent` and `Agent` into one type - -## Proposed design - -## 1. Split `Agent` storage into two maps - -Instead of one `kClients` map with encoded keys, keep two explicit maps: - -- `kClients` - - dispatchers for the normal path - - key: plain origin - - meaning: requests that may use the agent's configured protocol behaviour -- `kHttp1OnlyClients` - - dispatchers for requests forced to HTTP/1.1 - - key: plain origin - - meaning: requests with `allowH2 === false` - -This preserves the current behaviour while making the distinction explicit. - -Example shape: - -```js -const kClients = Symbol('clients') -const kHttp1OnlyClients = Symbol('http1-only clients') -``` - -Then `Agent` chooses the map directly: - -```js -const clients = allowH2 === false ? this[kHttp1OnlyClients] : this[kClients] -const result = clients.get(origin) -``` - -This removes the need for: -- synthetic origin keys -- suffix concatenation -- suffix filtering - -## 2. Add internal query/mutation methods on `Agent` - -The important change is not only storing entries separately, but also making `Agent` the owner of the lookup rules. - -These methods should not expose the raw registry maps or require `MockAgent` to manipulate pool entries directly. The point is to hide the storage shape behind internal semantics. - -Internal consumers should ask `Agent` questions like: -- "give me the dispatcher entry for this origin and routing mode" -- "set a dispatcher entry for this origin and routing mode" -- "delete the dispatcher entry for this origin and routing mode" -- "does this origin still have any dispatchers in either map?" - -A reasonable internal surface would be symbol-based methods rather than new public methods. - -For example: - -```js -const kGetDispatcherEntry = Symbol('get dispatcher entry') -const kSetDispatcherEntry = Symbol('set dispatcher entry') -const kDeleteDispatcherEntry = Symbol('delete dispatcher entry') -const kHasDispatcherForOrigin = Symbol('has dispatcher for origin') -const kForEachDispatcherEntry = Symbol('for each dispatcher entry') -``` - -Conceptually: - -```js -agent[kGetDispatcherEntry](origin, { allowH2 }) -agent[kSetDispatcherEntry](origin, { allowH2 }, entry) -agent[kDeleteDispatcherEntry](origin, { allowH2 }) -agent[kHasDispatcherForOrigin](origin) -agent[kForEachDispatcherEntry]((entry, meta) => { ... }) -``` - -Why symbol methods are preferable here: -- they keep the surface internal -- they avoid blessing these methods as supported public API -- they let `MockAgent` and similar internals depend on semantics, not representation - -## 3. Make `MockAgent` depend on the query methods, not the storage shape - -`MockAgent` currently reaches into agent internals and effectively shares the underlying client registry. - -That coupling is what made the suffix matter. - -With the proposed design, `MockAgent` should ask `Agent` for the correct bucket by routing mode rather than constructing keys itself. - -At minimum, that means: -- no string concatenation to build HTTP/1-only keys -- no suffix-based filtering when enumerating pending interceptors -- no assumptions about how `Agent` partitions dispatcher entries - -A better internal structure for `MockAgent` is: -- interceptor registries keyed by normalized origin only -- dispatcher instances resolved through `Agent` protocol-aware queries - -That separates two concerns cleanly: -- **mock matching**: keyed by logical origin and request properties -- **transport routing**: keyed by origin plus protocol policy - -### Why this matters - -The interceptors should be logically attached to the origin, not to an implementation-specific registry key. - -Whether the request took: -- the default path -- the HTTP/1-only path - -should not change how interceptors are matched or reported. - -## 4. Iterate across both maps through one helper - -Several `Agent` operations need to consider all dispatcher entries: -- `close()` -- `destroy()` -- `stats` -- origin cleanup - -Those should not manually duplicate logic. - -Add one internal iterator/helper that visits both maps. - -Conceptually: - -```js -forEachDispatcherEntry (fn) { - for (const [origin, entry] of this[kClients]) { - fn(entry, { origin, allowH2: this[kOptions].allowH2 !== false }) - } - - for (const [origin, entry] of this[kHttp1OnlyClients]) { - fn(entry, { origin, allowH2: false }) - } -} -``` - -That gives the implementation one place to define what counts as "all dispatchers". - -## Alternative shapes considered - -## A. Keep the suffix, but centralize it - -This is the current stopgap direction. - -Pros: -- minimal code churn -- fixes the immediate regression - -Cons: -- still leaks implementation details across modules -- still requires suffix-aware filtering and reasoning -- still mixes identity and routing policy in one string key - -This is acceptable as a short-term patch, but not ideal as the long-term design. - -## B. Nested map: `Map` - -This is also a valid design. - -Pros: -- models both dispatchers under the same origin naturally -- origin cleanup becomes straightforward - -Cons: -- slightly more bookkeeping in the hot path -- more complex mutation code than two direct maps - -This would be more explicit than the suffix approach and is arguably the cleanest data model. - -I still slightly prefer two maps plus query methods because: -- it keeps hot-path lookups very direct -- it maps closely to the current implementation -- it minimizes migration cost - -That said, nested maps would also solve the brittleness problem. - -## Detailed behaviour proposal - -## Dispatcher lookup - -Given `(origin, allowH2)`: -- if `allowH2 === false`, use the HTTP/1-only registry -- otherwise use the default registry - -No encoded key should be constructed. - -## Dispatcher creation - -When a dispatcher is missing: -- create it using the appropriate options -- store it in the selected registry -- keep origin tracking separate from storage shape - -## Origin tracking - -`kOrigins` should reflect whether an origin exists in either registry. - -That means: -- adding an entry in either map adds the origin -- deleting an entry removes the origin only if neither map still contains it - -This is more robust than implicitly inferring it from encoded keys. - -## Mock dispatcher association - -If `MockAgent` needs both protocol-specific dispatchers to share the same interceptor list, that sharing should happen explicitly, not via key convention. - -For example, `MockAgent` can maintain: - -```js -Map -``` - -and then create protocol-specific mock dispatchers that each reference the same `dispatches` array. - -This is preferable to storing duplicate logical state under two registry keys and then trying to filter one copy out of diagnostics. - -## Migration plan - -### Phase 0: simplify the current `MockAgent` workaround - -As a starting step, when `MockAgent.get(origin)` receives a concrete string origin: -- create the default mock dispatcher bucket -- create the HTTP/1-only mock dispatcher bucket as well -- make both dispatchers share the same interceptor list - -This is still a stopgap, but it is better than lazily creating the HTTP/1-only bucket during dispatch because: -- the mirrored state is established at registration time -- native `fetch()` and normal undici requests see the same mock setup immediately -- `MockAgent.dispatch()` no longer needs to repair missing protocol buckets on the fly - -This phase should not be treated as the final design because `MockAgent` still relies on `Agent`'s current registry convention. - -### Phase 1: introduce protocol-aware storage in `Agent` - -- add `kHttp1OnlyClients` -- add internal query/mutation helpers -- make `Agent`'s own lookup path use those helpers -- keep behaviour unchanged - -### Phase 2: migrate internal consumers - -Update `MockAgent` and any similar internal users to stop depending on: -- `origin + suffix` -- `endsWith(suffix)` -- direct assumptions about the shape of `agent[kClients]` - -### Phase 3: remove suffix compatibility logic - -Once all internal users are converted: -- delete key-suffix helpers -- delete any code that filters synthetic entries in diagnostics - -## Testing plan - -At minimum: - -1. Existing regression test for native `fetch()` + `MockAgent` -2. Native `WebSocket` regression tests that motivated the HTTP/1-only routing -3. `MockAgent` pending interceptor diagnostics -4. `close()` / `destroy()` coverage for both dispatcher registries -5. Stats/origin cleanup behaviour when only one of the two registries is populated - -Additional targeted tests: -- same origin can have both a default dispatcher and an HTTP/1-only dispatcher simultaneously -- deleting one does not remove the other -- deleting the last one removes the origin from origin tracking -- `MockAgent` interceptor reporting does not duplicate shared interceptor state - -## Recommendation - -Treat the current suffix-based fix as the compatibility patch, not the final architecture. - -The long-term direction should be: -- explicit protocol-aware dispatcher registries in `Agent` -- internal query methods for dispatcher lookup and iteration -- `MockAgent` depending on those methods instead of key conventions - -That design is less brittle because it makes the transport distinction explicit in the data model instead of hiding it inside a string.