Fix containers left running after wait strategies fail (#142)

Author: cristianrgreco

src/generic-container.test.ts

@@ -6,6 +6,7 @@ import { GenericContainer } from "./generic-container";
 import { AlwaysPullPolicy } from "./pull-policy";
 import { Wait } from "./wait";
 import { Readable } from "stream";
+import { RandomUuid } from "./uuid";
 
 describe("GenericContainer", () => {
   jest.setTimeout(180_000);
@@ -245,6 +246,69 @@ describe("GenericContainer", () => {
     await container.stop();
   });
 
+  it("should stop the container when the host port check wait strategy times out", async () => {
+    const containerName = `container-${new RandomUuid().nextUuid()}`;
+
+    await expect(
+      new GenericContainer("cristianrgreco/testcontainer", "1.1.12")
+        .withName(containerName)
+        .withExposedPorts(8081)
+        .withStartupTimeout(new Duration(0, TemporalUnit.MILLISECONDS))
+        .start()
+    ).rejects.toThrowError("Port 8081 not bound after 0ms");
+
+    expect(await getRunningContainerNames()).not.toContain(containerName);
+  });
+
+  it("should stop the container when the log message wait strategy times out", async () => {
+    const containerName = `container-${new RandomUuid().nextUuid()}`;
+
+    await expect(
+      new GenericContainer("cristianrgreco/testcontainer", "1.1.12")
+        .withName(containerName)
+        .withExposedPorts(8080)
+        .withWaitStrategy(Wait.forLogMessage("unexpected"))
+        .withStartupTimeout(new Duration(0, TemporalUnit.MILLISECONDS))
+        .start()
+    ).rejects.toThrowError(`Log message "unexpected" not received after 0ms`);
+
+    expect(await getRunningContainerNames()).not.toContain(containerName);
+  });
+
+  it("should stop the container when the health check wait strategy times out", async () => {
+    const containerName = `container-${new RandomUuid().nextUuid()}`;
+
+    const context = path.resolve(fixtures, "docker-with-health-check");
+    const customGenericContainer = await GenericContainer.fromDockerfile(context).build();
+    await expect(
+      customGenericContainer
+        .withName(containerName)
+        .withExposedPorts(8080)
+        .withWaitStrategy(Wait.forHealthCheck())
+        .withStartupTimeout(new Duration(0, TemporalUnit.MILLISECONDS))
+        .start()
+    ).rejects.toThrowError("Health check not healthy after 0ms");
+
+    expect(await getRunningContainerNames()).not.toContain(containerName);
+  });
+
+  it("should stop the container when the health check fails", async () => {
+    const containerName = `container-${new RandomUuid().nextUuid()}`;
+
+    const context = path.resolve(fixtures, "docker-with-health-check");
+    const customGenericContainer = await GenericContainer.fromDockerfile(context).build();
+    await expect(
+      customGenericContainer
+        .withName(containerName)
+        .withExposedPorts(8080)
+        .withHealthCheck({ test: "exit 1" })
+        .withWaitStrategy(Wait.forHealthCheck())
+        .start()
+    ).rejects.toThrowError("Health check failed");
+
+    expect(await getRunningContainerNames()).not.toContain(containerName);
+  });
+
   it("should honour .dockerignore file", async () => {
     const context = path.resolve(fixtures, "docker-with-dockerignore");
     const container = await GenericContainer.fromDockerfile(context).build();
@@ -312,4 +376,12 @@ describe("GenericContainer", () => {
       await startedContainer.stop();
     });
   });
+
+  const getRunningContainerNames = async (): Promise<string[]> => {
+    const containers = await dockerodeClient.listContainers();
+    return containers
+      .map((container) => container.Names)
+      .reduce((result, containerNames) => [...result, ...containerNames], [])
+      .map((containerName) => containerName.replace("/", ""));
+  };
 });

src/generic-container.ts

@@ -21,7 +21,7 @@ import {
   TmpFs,
 } from "./docker-client";
 import { DockerClientFactory, Host } from "./docker-client-factory";
-import { log, containerLog } from "./logger";
+import { containerLog, log } from "./logger";
 import { Port } from "./port";
 import { PortBinder } from "./port-binder";
 import { HostPortCheck, InternalPortCheck } from "./port-check";
@@ -235,8 +235,20 @@ export class GenericContainer implements TestContainer {
   ): Promise<void> {
     log.debug(`Waiting for container to be ready: ${container.getId()}`);
     const waitStrategy = this.getWaitStrategy(dockerClient, container);
-    await waitStrategy.withStartupTimeout(this.startupTimeout).waitUntilReady(container, containerState, boundPorts);
-    log.info("Container is ready");
+
+    try {
+      await waitStrategy.withStartupTimeout(this.startupTimeout).waitUntilReady(container, containerState, boundPorts);
+      log.info("Container is ready");
+    } catch (err) {
+      log.error("Container failed to be ready");
+      try {
+        await container.stop({ timeout: new Duration(0, TemporalUnit.MILLISECONDS) });
+        await container.remove({ removeVolumes: true });
+      } catch (stopErr) {
+        log.error("Failed to stop container after it failed to be ready");
+      }
+      throw err;
+    }
   }
 
   private getWaitStrategy(dockerClient: DockerClient, container: Container): WaitStrategy {

src/wait-strategy.ts

@@ -89,21 +89,30 @@ export class LogWaitStrategy extends AbstractWaitStrategy {
     const stream = await container.logs();
 
     return new Promise((resolve, reject) => {
+      const startupTimeout = this.startupTimeout.get(TemporalUnit.MILLISECONDS);
+      const timeout = setTimeout(
+        () => reject(new Error(`Log message "${this.message}" not received after ${startupTimeout}ms`)),
+        startupTimeout
+      );
+
       byline(stream)
         .on("data", (line) => {
           if (line.includes(this.message)) {
             stream.destroy();
+            clearTimeout(timeout);
             resolve();
           }
         })
         .on("err", (line) => {
           if (line.includes(this.message)) {
             stream.destroy();
+            clearTimeout(timeout);
             resolve();
           }
         })
         .on("end", () => {
           stream.destroy();
+          clearTimeout(timeout);
           reject();
         });
     });
@@ -118,14 +127,18 @@ export class HealthCheckWaitStrategy extends AbstractWaitStrategy {
       new Duration(100, TemporalUnit.MILLISECONDS)
     );
 
-    await retryStrategy.retryUntil(
+    const status = await retryStrategy.retryUntil(
       async () => (await container.inspect()).healthCheckStatus,
-      (status) => status === "healthy",
+      (status) => status === "healthy" || status === "unhealthy",
       () => {
         const timeout = this.startupTimeout.get(TemporalUnit.MILLISECONDS);
         throw new Error(`Health check not healthy after ${timeout}ms`);
       },
       this.startupTimeout
     );
+
+    if (status !== "healthy") {
+      throw new Error(`Health check failed`);
+    }
   }
 }