Reuse insecure HTTP wait agent across retries

packages/testcontainers/src/wait-strategies/http-wait-strategy.agent-lifecycle.test.ts

@@ -0,0 +1,106 @@
+import { Readable } from "stream";
+import { Agent, request } from "undici";
+import { HttpWaitStrategy } from "./http-wait-strategy";
+
+// Tracks every undici Agent the strategy constructs. Hoisted so it is initialised
+// before the mock factories below run.
+const { agentInstances } = vi.hoisted(() => ({ agentInstances: [] as import("undici").Agent[] }));
+
+// Spy on the Agent constructor and stub `request` so these tests exercise the wait
+// strategy without any real HTTP or container runtime. Mocking at the top level
+// (hoisted) ensures HttpWaitStrategy binds these mocks when it is imported.
+vi.mock("undici", async (importOriginal) => {
+  const actual = await importOriginal<typeof import("undici")>();
+  return {
+    ...actual,
+    Agent: vi.fn(function (...args: ConstructorParameters<typeof actual.Agent>) {
+      const agent = new actual.Agent(...args);
+      agentInstances.push(agent);
+      return agent;
+    }),
+    request: vi.fn(),
+  };
+});
+
+vi.mock("../container-runtime", async (importOriginal) => {
+  const actual = await importOriginal<typeof import("../container-runtime")>();
+  return {
+    ...actual,
+    getContainerRuntimeClient: async () => ({
+      info: { containerRuntime: { host: "localhost" } },
+      container: { inspect: vi.fn() },
+    }),
+  };
+});
+
+const boundPorts = { getBinding: () => 12345 } as never;
+const container = { id: "container-id" } as never;
+
+const passingResponse = () =>
+  ({ statusCode: 200, headers: {}, body: Readable.from(["ok"]) }) as unknown as Awaited<ReturnType<typeof request>>;
+
+// Sequential: the tests share the module-level Agent spy and instance list.
+describe.sequential("HttpWaitStrategy insecure agent lifecycle", () => {
+  beforeEach(() => {
+    agentInstances.length = 0;
+    vi.mocked(Agent).mockClear();
+    vi.mocked(request).mockReset();
+  });
+
+  it("constructs a single insecure agent across retries and disposes it on completion", async () => {
+    let attempts = 0;
+    vi.mocked(request).mockImplementation(async () => {
+      attempts++;
+      // Fail the first couple of attempts so the retry loop runs more than once.
+      if (attempts < 3) {
+        throw new Error("connection refused");
+      }
+      return passingResponse();
+    });
+
+    const strategy = new HttpWaitStrategy("/health", 8443, {})
+      .usingTls()
+      .allowInsecure()
+      .withReadTimeout(10)
+      .withStartupTimeout(5000);
+
+    await strategy.waitUntilReady(container, boundPorts);
+
+    expect(attempts).toBeGreaterThan(1);
+    // A single Agent is constructed and reused across every retry attempt...
+    expect(vi.mocked(Agent)).toHaveBeenCalledTimes(1);
+    expect(agentInstances).toHaveLength(1);
+    // ...and it is disposed once the wait completes.
+    expect(agentInstances[0].destroyed).toBe(true);
+  });
+
+  it("creates a separate insecure agent per wait so concurrent waits stay isolated", async () => {
+    vi.mocked(request).mockImplementation(async () => passingResponse());
+
+    // A single strategy instance can drive multiple concurrent waits (e.g. a compose
+    // default wait strategy passed to every service). Each wait must get its own agent
+    // so one finishing wait cannot destroy a dispatcher another wait is still using.
+    const strategy = new HttpWaitStrategy("/health", 8443, {})
+      .usingTls()
+      .allowInsecure()
+      .withReadTimeout(10)
+      .withStartupTimeout(5000);
+
+    await Promise.all([strategy.waitUntilReady(container, boundPorts), strategy.waitUntilReady(container, boundPorts)]);
+
+    expect(vi.mocked(Agent)).toHaveBeenCalledTimes(2);
+    expect(agentInstances).toHaveLength(2);
+    expect(agentInstances.every((agent) => agent.destroyed)).toBe(true);
+  });
+
+  it("never constructs an agent when allowInsecure is not set", async () => {
+    vi.mocked(request).mockImplementation(async () => passingResponse());
+
+    const strategy = new HttpWaitStrategy("/health", 8080, {}).withReadTimeout(10).withStartupTimeout(5000);
+
+    await strategy.waitUntilReady(container, boundPorts);
+
+    expect(vi.mocked(Agent)).not.toHaveBeenCalled();
+    expect(agentInstances).toHaveLength(0);
+  });
+});

packages/testcontainers/src/wait-strategies/http-wait-strategy.ts

@@ -57,8 +57,8 @@ export class HttpWaitStrategy extends AbstractWaitStrategy {
     return this;
   }
 
-  public withReadTimeout(startupTimeoutMs: number): this {
-    this.readTimeoutMs = startupTimeoutMs;
+  public withReadTimeout(readTimeoutMs: number): this {
+    this.readTimeoutMs = readTimeoutMs;
     return this;
   }
 
@@ -79,61 +79,73 @@ export class HttpWaitStrategy extends AbstractWaitStrategy {
     let containerExited = false;
     const client = await getContainerRuntimeClient();
     const { abortOnContainerExit } = this.options;
+    // Scope the insecure agent to this invocation rather than the strategy instance: a single
+    // strategy object can drive multiple concurrent waits (e.g. a compose default wait strategy
+    // passed to every service), and a shared dispatcher would let one finished wait destroy the
+    // agent another wait is still using.
+    const agent = this.createInsecureAgent();
 
-    await new IntervalRetry<Response | undefined, Error>(this.readTimeoutMs).retryUntil(
-      async () => {
-        try {
-          const url = `${this.protocol}://${client.info.containerRuntime.host}:${boundPorts.getBinding(this.port)}${
-            this.path
-          }`;
-
-          if (abortOnContainerExit) {
-            const containerStatus = (await client.container.inspect(container)).State.Status;
-
-            if (containerStatus === exitStatus) {
-              containerExited = true;
-              return;
+    try {
+      await new IntervalRetry<Response | undefined, Error>(this.readTimeoutMs).retryUntil(
+        async () => {
+          try {
+            const url = `${this.protocol}://${client.info.containerRuntime.host}:${boundPorts.getBinding(this.port)}${
+              this.path
+            }`;
+
+            if (abortOnContainerExit) {
+              const containerStatus = (await client.container.inspect(container)).State.Status;
+
+              if (containerStatus === exitStatus) {
+                containerExited = true;
+                return;
+              }
             }
+
+            return undiciResponseToFetchResponse(
+              await request(url, {
+                method: this.method,
+                signal: AbortSignal.timeout(this.readTimeoutMs),
+                headers: this.headers,
+                dispatcher: agent,
+              })
+            );
+          } catch {
+            return undefined;
+          }
+        },
+        async (response) => {
+          if (abortOnContainerExit && containerExited) {
+            return true;
           }
 
-          return undiciResponseToFetchResponse(
-            await request(url, {
-              method: this.method,
-              signal: AbortSignal.timeout(this.readTimeoutMs),
-              headers: this.headers,
-              dispatcher: this.getAgent(),
-            })
-          );
-        } catch {
-          return undefined;
-        }
-      },
-      async (response) => {
-        if (abortOnContainerExit && containerExited) {
-          return true;
-        }
-
-        if (response === undefined) {
-          return false;
-        } else if (!this.predicates.length) {
-          return response.ok;
-        } else {
-          for (const predicate of this.predicates) {
-            const result = await predicate(response);
-            if (!result) {
-              return false;
+          if (response === undefined) {
+            return false;
+          } else if (!this.predicates.length) {
+            return response.ok;
+          } else {
+            for (const predicate of this.predicates) {
+              const result = await predicate(response);
+              if (!result) {
+                return false;
+              }
             }
+            return true;
           }
-          return true;
-        }
-      },
-      () => {
-        const message = `URL ${this.path} not accessible after ${this.startupTimeoutMs}ms`;
-        log.error(message, { containerId: container.id });
-        throw new Error(message);
-      },
-      this.startupTimeoutMs
-    );
+        },
+        () => {
+          const message = `URL ${this.path} not accessible after ${this.startupTimeoutMs}ms`;
+          log.error(message, { containerId: container.id });
+          throw new Error(message);
+        },
+        this.startupTimeoutMs
+      );
+    } finally {
+      // Force-close rather than graceful close(): status-only predicates never consume the
+      // response body, so close() could hang waiting for those connections to be released.
+      // The wait has finished by this point, so there is nothing left worth draining.
+      await agent?.destroy();
+    }
 
     if (abortOnContainerExit && containerExited) {
       return this.handleContainerExit(container);
@@ -165,13 +177,15 @@ export class HttpWaitStrategy extends AbstractWaitStrategy {
     throw new Error(message);
   }
 
-  private getAgent(): Agent | undefined {
-    if (this._allowInsecure) {
-      return new Agent({
-        connect: {
-          rejectUnauthorized: false,
-        },
-      });
+  private createInsecureAgent(): Agent | undefined {
+    if (!this._allowInsecure) {
+      return undefined;
     }
+
+    return new Agent({
+      connect: {
+        rejectUnauthorized: false,
+      },
+    });
   }
 }