fix: don't use the request signal in the agent

[thewilkybarkid]

Following an upgrade from 13.0.1 to 14.0.1 (which included upgrading @npmcli/agent from 2.0.0 to 3.0.0), we've seen strange behaviour: a request works the first time, then immediately times out after that.

We're setting a signal on each request, which due to changes in @npmcli/agent is then also used by the agent for connections. The problem is that the original signal is used for all requests due to agent caching. Once it's been used (i.e. after the first time a request is made), no connections are possible (as they're immediately rejected). This change resolves our issue by removing the signal from the agent.

I looked into adding a test, but I don't think it's possible without making real network requests.

[wraithgar]

So you are passing signal as a parameter to make-fetch-happen? I think we need to understand a little bit more about what you're passing in here and where it's ending up in @npmcli/agent to be sure this is the right fix.

Your explanation of the root cause "reusing a signal" makes sense though, so fixing that is definitely the right approach. We just need to be sure we fix it correctly.

[wraithgar]

If you pass in a signal It does end up being given to the underlying agent that is created.

From options to normalizedOptions directly into the new Agent. Node.js's Agent accepts the same parameters as http.request, which includes signal. So there is where it's getting attached and reused.

What we need to somehow do here is differentiate when a user does want an agent attached to a request, versus when they don't. For reasons shown in this very bug it's not always wanted to attach and reuse a signal across multiple requests. But there may be reasons to still attach a signal for a given request. I suppose it does make sense that if I pass agent to a make-fetch-happen fetch call, I intend it only for that request and do not want it reused.

I'm gonna have to think a little more of use cases here but I think the fix may want to be in @npmcli/agent itself. The case may be that we don't want it to attach signals to the base agent.

We do still, however, want to attach a signal given to this module to the individual request. That's the equivalent of passing a signal to http.request.

[wraithgar]

After giving it more thought this is in fact the best solution. There's no need to try to 'guard' the signal option in @npmcli/agent. This module itself can be where the decision is made that when making a fetch, any passed signal is intended for the request, not the agent.

Currently testing this is pretty unrealistic, as there are no traces of this in the returned response. minipass-fetch returns new Response(data, { headers }). We'd have to mock the internals, which usually just tests how the code is written, not what it actually does. We're just gonna have to be ok w/ this as-is.

In order to give ourselves a better chance of not accidentally regressing here, we should add a comment explaining why signal is being forefully omitted from getAgent, in the absence of a test.