fix(http): stop destroying superseded agents, which could abort live requests

Follow-up to the agent caching change. When the trust material changes the caches
are cleared, and that path called agent.destroy() on the node https agents.

Node's documentation for agent.destroy() is "Destroy any sockets that are
currently in use by the agent" - so an admin uploading a certificate could have
aborted an HTTPS request that was already in flight, for instance a nextcloud
calendar sync. The agents run without keepAlive (verified: new https.Agent({})
reports keepAlive false on node 24), so destroying bought nothing anyway; each
socket closes on its own once its response completes.

undici's close() is kept, because it waits for enqueued requests to finish and
therefore cannot interrupt one in flight.

Also verified the correctness premise of sharing a single agent across every
integration, which the previous commit asserted but did not test: three origins
served by one agent each got their own pooled connection, four requests over one
connection apiece, with no response misrouted between them.
This commit is contained in:
Thomas Camlong committed 2026-08-13 15:36:12 +02:00
1 parent 09df2910c5
commit 320c51ac89
2 files changed
+28 -2

No files matched your search

@@ -65,9 +65,14 @@ const getTrustMaterialAsync = async () => {
.digest("hex");
if (fingerprint !== cachedTrustFingerprint) {
// Superseded agents hold their idle sockets open until they are closed.
// undici's close() waits for enqueued requests to finish, so releasing the pool cannot interrupt
// a request already in flight.
//
// The node agents are deliberately *not* destroyed: agent.destroy() destroys sockets that are
// currently in use, so an admin adding a certificate could abort an in-flight calendar sync.
// They run without keepAlive, so dropping the reference is enough - each socket closes once its
// own response completes.
for (const agent of undiciAgentCache.values()) void agent.close();
for (const agent of httpsAgentCache.values()) agent.destroy();
undiciAgentCache.clear();
httpsAgentCache.clear();
cachedTrustFingerprint = fingerprint;
@@ -138,6 +138,27 @@ describe("createHttpsAgentAsync", () => {
expect(after).not.toBe(before);
});
test("should not destroy a superseded agent, which would abort requests in flight", async () => {
// agent.destroy() destroys sockets that are currently in use, so destroying the old agent when an
// admin adds a certificate could abort an in-flight calendar sync. These agents run without
// keepAlive, so dropping the reference is enough.
const { createHttpsAgentAsync } = await importRequestModuleAsync();
const before = await createHttpsAgentAsync();
// keepAlive is a real runtime property but is absent from @types/node's public Agent surface.
// It is asserted because it is the premise of not destroying: without keepAlive, a dropped agent's
// sockets close on their own once their response completes.
expect((before as unknown as { keepAlive: boolean }).keepAlive).toBe(false);
const destroySpy = vi.spyOn(before, "destroy");
// Act
trustMaterial.certificates = [{ fileName: "added.crt", content: "-----BEGIN CERTIFICATE-----" }];
const after = await createHttpsAgentAsync();
// Assert
expect(after).not.toBe(before);
expect(destroySpy).not.toHaveBeenCalled();
});
test("should give a caller supplying an override a dedicated agent", async () => {
// Arrange
const { createHttpsAgentAsync } = await importRequestModuleAsync();