Repository navigation
Fix container leak when start fails after the container is created - #1588
Draft
cristianrgreco wants to merge 5 commits into
Draft
cristianrgreco wants to merge 5 commits into
cristianrgreco wants to merge 5 commits into
Conversation
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.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 throwingcontainerCreated,containerStartingorcontainerStartedhook, a failed Docker start call, and failed copies or network connects. Only a failed wait strategy cleaned up, inwaitForContainer.startContainernow removes the container, with its volumes, before rethrowing the original error.The removal is a new
removeFailedContainerhelper thatwaitForContainernow 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 becausewaitForContainermay already have removed the container beforestartContainergets to it, including when a module calls it fromcontainerStarted(Kafka, Redpanda).waitForContainerpreviously stopped the container and then removed it; the outcome is the same. Itsclientparameter 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 callsstop()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, whichstart()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.tspromise resolved "{ …(23) }" instead of rejectingbecause the container still existed. The reuse test failed because the secondstart()returned the container whose hook had failed.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.packages/testcontainers/src/wait-strategiespassed 71 of 71 andpackages/testcontainers/src/docker-compose-environmentpassed 29 of 29. Those include the existing "should stop the container when ... times out" tests forwaitForContainer.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 mergingmain(lint prints one info about thebiome.jsonschema version, which comes frommain).generic-container,wait-strategies,reaperandport-forwardersuites together locally (Docker Desktop, engine 29.6.1) is not reliably green with the new tests enabled:generic-container-restart.test.tsand the "reuse stopped container" tests failed withfetch failedin 6 of 7 runs, and in none of 5 runs without them. In those failures the library holds the pre-restart host port afterrestart()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.
waitForContainerkeeps its signature and still leaves no container behind when the wait fails.