Skip to content

Fix container leak when start fails after the container is created - #1588

Draft
cristianrgreco wants to merge 5 commits into
mainfrom
claude/cranky-cori-2d7dc7
Draft

cristianrgreco wants to merge 5 commits into
mainfrom
claude/cranky-cori-2d7dc7

Conversation

@cristianrgreco

@cristianrgreco cristianrgreco commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

If anything failed between creating a container and start() returning, start() rejected but the container was left behind and the caller had no handle to stop it. This covers a throwing containerCreated, containerStarting or containerStarted hook, a failed Docker start call, and failed copies or network connects. Only a failed wait strategy cleaned up, in waitForContainer.

startContainer now removes the container, with its volumes, before rethrowing the original error.

The removal is a new removeFailedContainer helper that waitForContainer now uses too, so both paths clean up the same way: one force-remove with volumes, where an already removed container counts as done. That matters because waitForContainer may already have removed the container before startContainer gets to it, including when a module calls it from containerStarted (Kafka, Redpanda). waitForContainer previously stopped the container and then removed it; the outcome is the same. Its client parameter is now unused and is kept because the function is exported.

A reusable container created by the failing call is removed too. Hooks do not run for a reused container, so the next start() would otherwise receive one whose setup never completed, and Ryuk never removes it. An existing container found for reuse is left alone.

This follows testcontainers-java, where tryStart() wraps the whole start sequence and calls stop() on failure: https://github.com/testcontainers/testcontainers-java/blob/9bb3c2dc175672ad33a2eafd7133212c7173ac43/core/src/main/java/org/testcontainers/containers/GenericContainer.java#L369-L548. testcontainers-go and testcontainers-dotnet do not clean up internally; they leave the container object with the caller to terminate or dispose, which start() here cannot do because it returns the only handle.

Found while verifying #1514.

Verification

  • npx vitest run packages/testcontainers/src/generic-container/generic-container-lifecycle.test.ts
    • Before the fix: 4 failed, 2 passed. The three hook tests failed with promise resolved "{ …(23) }" instead of rejecting because the container still existed. The reuse test failed because the second start() returned the container whose hook had failed.
    • After the fix: 6 passed, no containers left behind.
    • After merging main: 6 passed in 6 of 9 runs. One run had 2 failures whose output I did not capture, one run's output was cut off before the summary, and in one run the two existing tests timed out waiting for ports (Port 8080/tcp not bound after 60000ms, Test timed out in 180000ms) while the four new tests passed. Other test runs were using the same Docker daemon each time.
  • After moving the cleanup into the shared helper: the lifecycle tests passed 6 of 6, packages/testcontainers/src/wait-strategies passed 71 of 71 and packages/testcontainers/src/docker-compose-environment passed 29 of 29. Those include the existing "should stop the container when ... times out" tests for waitForContainer.
  • With Wait for Couchbase KV readiness before start resolves #1514 merged in, before and after the helper change: npx vitest run packages/modules/couchbase/src/couchbase-container.test.ts -t "should throw error|should fail when setup exceeds" passed 3 of 3, and the three Couchbase containers those tests created no longer exist afterwards.
  • npm run format, npm run lint, npm run knip, npm run check-compiles: clean, before and after merging main (lint prints one info about the biome.json schema version, which comes from main).
  • Running the generic-container, wait-strategies, reaper and port-forwarder suites together locally (Docker Desktop, engine 29.6.1) is not reliably green with the new tests enabled: generic-container-restart.test.ts and the "reuse stopped container" tests failed with fetch failed in 6 of 7 runs, and in none of 5 runs without them. In those failures the library holds the pre-restart host port after restart() returns and Docker reports a different port a few seconds later. I saw the same port change once in 3 runs with the new tests skipped. That is being looked at separately and has not been checked on Linux.

Not breaking

No API change. A failed wait strategy already removed the container; this extends the same outcome to the other failures after the container is created. waitForContainer keeps its signature and still leaves no container behind when the wait fails.

A failure between container creation and start() returning, such as a throwing containerCreated, containerStarting or containerStarted hook, rejected start() but left the container behind with no handle to stop it. startContainer now removes the container before rethrowing, as waitForContainer already does for a failed wait strategy.
@cristianrgreco cristianrgreco added bug Something isn't working patch Backward compatible bug fix labels Oct 12, 2026
startContainer and waitForContainer each removed the container in their own way. Both now call removeFailedContainer, which force-removes the container with its volumes and treats an already removed container as done. waitForContainer keeps its client parameter because it is exported.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working patch Backward compatible bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant