fix: resource clean up for tests and examples (#2738)

Ensure that all resources are cleaned up for tests and examples even
if they fail.

This leverages new helpers in testcontainers:
* TerminateContainer for examples
* CleanupContainer and CleanupNetwork for tests

These are required ensuring that containers that are created but fail
in later actions are returned alongside the error so that clean up can
be performed.

Consistently clean up created networks using a new context to ensure
that the removal gets run even if original context has timed out or
been cancelled.

Use fmt.Print instead of log.Fatal to ensure that defers are run in
all examples again ensuring that clean up is processed.

Call Stop from Terminate to ensure that child containers are shutdown
correctly on clean up as the hard coded timeout using by ContainerRemove
is too short to allow this to happen correctly.

Clean up of test logic replacing manual checks and asserts with require
to make them more concise and hence easier to understand.

Quiet test output by either capturing or disabling output so it's easier
to identify issues when tests are run in non verbose mode.

Clarify source of errors with wrapping and update tests to handle.

Ensure that port forwarding container is shutdown if an error occurs
during setup so it isn't orphaned.

Shutdown the port forwarding container on both stop and terminate to
prevent it being orphaned when the Stop is used.

Add missing error checks to tests.

Remove unused nolint directives and enable the nolintlint to catch any
regressions.

Don't use container as a variable as its overused.

Author: stevenh

.gitignore

@@ -14,4 +14,7 @@ TEST-*.xml
 
 tcvenv
 
-**/go.work
+**/go.work
+
+# VS Code settings
+.vscode

.golangci.yml

@@ -8,6 +8,7 @@ linters:
     - nonamedreturns
     - testifylint
     - errcheck
+    - nolintlint
 
 linters-settings:
   errorlint:

cleanup.go

@@ -0,0 +1,107 @@
+package testcontainers
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"reflect"
+	"time"
+)
+
+// terminateOptions is a type that holds the options for terminating a container.
+type terminateOptions struct {
+	ctx     context.Context
+	timeout *time.Duration
+	volumes []string
+}
+
+// TerminateOption is a type that represents an option for terminating a container.
+type TerminateOption func(*terminateOptions)
+
+// StopContext returns a TerminateOption that sets the context.
+// Default: context.Background().
+func StopContext(ctx context.Context) TerminateOption {
+	return func(c *terminateOptions) {
+		c.ctx = ctx
+	}
+}
+
+// StopTimeout returns a TerminateOption that sets the timeout.
+// Default: See [Container.Stop].
+func StopTimeout(timeout time.Duration) TerminateOption {
+	return func(c *terminateOptions) {
+		c.timeout = &timeout
+	}
+}
+
+// RemoveVolumes returns a TerminateOption that sets additional volumes to remove.
+// This is useful when the container creates named volumes that should be removed
+// which are not removed by default.
+// Default: nil.
+func RemoveVolumes(volumes ...string) TerminateOption {
+	return func(c *terminateOptions) {
+		c.volumes = volumes
+	}
+}
+
+// TerminateContainer calls [Container.Terminate] on the container if it is not nil.
+//
+// This should be called as a defer directly after [GenericContainer](...)
+// or a modules Run(...) to ensure the container is terminated when the
+// function ends.
+func TerminateContainer(container Container, options ...TerminateOption) error {
+	if isNil(container) {
+		return nil
+	}
+
+	c := &terminateOptions{
+		ctx: context.Background(),
+	}
+
+	for _, opt := range options {
+		opt(c)
+	}
+
+	// TODO: Add a timeout when terminate supports it.
+	err := container.Terminate(c.ctx)
+	if !isCleanupSafe(err) {
+		return fmt.Errorf("terminate: %w", err)
+	}
+
+	// Remove additional volumes if any.
+	if len(c.volumes) == 0 {
+		return nil
+	}
+
+	client, err := NewDockerClientWithOpts(c.ctx)
+	if err != nil {
+		return fmt.Errorf("docker client: %w", err)
+	}
+
+	defer client.Close()
+
+	// Best effort to remove all volumes.
+	var errs []error
+	for _, volume := range c.volumes {
+		if errRemove := client.VolumeRemove(c.ctx, volume, true); errRemove != nil {
+			errs = append(errs, fmt.Errorf("volume remove %q: %w", volume, errRemove))
+		}
+	}
+
+	return errors.Join(errs...)
+}
+
+// isNil returns true if val is nil or an nil instance false otherwise.
+func isNil(val any) bool {
+	if val == nil {
+		return true
+	}
+
+	valueOf := reflect.ValueOf(val)
+	switch valueOf.Kind() {
+	case reflect.Chan, reflect.Func, reflect.Map, reflect.Ptr, reflect.UnsafePointer, reflect.Interface, reflect.Slice:
+		return valueOf.IsNil()
+	default:
+		return false
+	}
+}

container_test.go

@@ -290,8 +290,7 @@ func Test_BuildImageWithContexts(t *testing.T) {
 				ContainerRequest: req,
 				Started:          true,
 			})
-
-			defer terminateContainerOnEnd(t, ctx, c)
+			testcontainers.CleanupContainer(t, c)
 
 			if testCase.ExpectedError != "" {
 				require.EqualError(t, err, testCase.ExpectedError)
@@ -317,7 +316,7 @@ func Test_GetLogsFromFailedContainer(t *testing.T) {
 		ContainerRequest: req,
 		Started:          true,
 	})
-	terminateContainerOnEnd(t, ctx, c)
+	testcontainers.CleanupContainer(t, c)
 	require.Error(t, err)
 	require.Contains(t, err.Error(), "container exited with code 0")
 
@@ -417,25 +416,21 @@ func TestImageSubstitutors(t *testing.T) {
 				ImageSubstitutors: test.substitutors,
 			}
 
-			container, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{
+			ctr, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{
 				ContainerRequest: req,
 				Started:          true,
 			})
+			testcontainers.CleanupContainer(t, ctr)
 			if test.expectedError != nil {
 				require.ErrorIs(t, err, test.expectedError)
 				return
 			}
 
-			if err != nil {
-				t.Fatal(err)
-			}
-			defer func() {
-				terminateContainerOnEnd(t, ctx, container)
-			}()
+			require.NoError(t, err)
 
 			// enforce the concrete type, as GenericContainer returns an interface,
 			// which will be changed in future implementations of the library
-			dockerContainer := container.(*testcontainers.DockerContainer)
+			dockerContainer := ctr.(*testcontainers.DockerContainer)
 			assert.Equal(t, test.expectedImage, dockerContainer.Image)
 		})
 	}
@@ -455,21 +450,17 @@ func TestShouldStartContainersInParallel(t *testing.T) {
 				ExposedPorts: []string{nginxDefaultPort},
 				WaitingFor:   wait.ForHTTP("/").WithStartupTimeout(10 * time.Second),
 			}
-			container, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{
+			ctr, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{
 				ContainerRequest: req,
 				Started:          true,
 			})
-			if err != nil {
-				t.Fatalf("could not start container: %v", err)
-			}
+			testcontainers.CleanupContainer(t, ctr)
+			require.NoError(t, err)
+
 			// mappedPort {
-			port, err := container.MappedPort(ctx, nginxDefaultPort)
+			port, err := ctr.MappedPort(ctx, nginxDefaultPort)
 			// }
-			if err != nil {
-				t.Fatalf("could not get mapped port: %v", err)
-			}
-
-			terminateContainerOnEnd(t, ctx, container)
+			require.NoError(t, err)
 
 			t.Logf("Parallel container [iteration_%d] listening on %d\n", i, port.Int())
 		})
@@ -480,28 +471,28 @@ func ExampleGenericContainer_withSubstitutors() {
 	ctx := context.Background()
 
 	// applyImageSubstitutors {
-	container, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{
+	ctr, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{
 		ContainerRequest: testcontainers.ContainerRequest{
 			Image:             "alpine:latest",
 			ImageSubstitutors: []testcontainers.ImageSubstitutor{dockerImageSubstitutor{}},
 		},
 		Started: true,
 	})
-	// }
-	if err != nil {
-		log.Fatalf("could not start container: %v", err)
-	}
-
 	defer func() {
-		err := container.Terminate(ctx)
-		if err != nil {
-			log.Fatalf("could not terminate container: %v", err)
+		if err := testcontainers.TerminateContainer(ctr); err != nil {
+			log.Printf("failed to terminate container: %s", err)
 		}
 	}()
 
+	// }
+	if err != nil {
+		log.Printf("could not start container: %v", err)
+		return
+	}
+
 	// enforce the concrete type, as GenericContainer returns an interface,
 	// which will be changed in future implementations of the library
-	dockerContainer := container.(*testcontainers.DockerContainer)
+	dockerContainer := ctr.(*testcontainers.DockerContainer)
 
 	fmt.Println(dockerContainer.Image)
 

docker.go

@@ -259,9 +259,14 @@ func (c *DockerContainer) Start(ctx context.Context) error {
 //
 // If the container is already stopped, the method is a no-op.
 func (c *DockerContainer) Stop(ctx context.Context, timeout *time.Duration) error {
+	// Note we can't check isRunning here because we allow external creation
+	// without exposing the ability to fully initialize the container state.
+	// See: https://github.com/testcontainers/testcontainers-go/issues/2667
+	// TODO: Add a check for isRunning when the above issue is resolved.
+
 	err := c.stoppingHook(ctx)
 	if err != nil {
-		return err
+		return fmt.Errorf("stopping hook: %w", err)
 	}
 
 	var options container.StopOptions
@@ -272,22 +277,38 @@ func (c *DockerContainer) Stop(ctx context.Context, timeout *time.Duration) erro
 	}
 
 	if err := c.provider.client.ContainerStop(ctx, c.ID, options); err != nil {
-		return err
+		return fmt.Errorf("container stop: %w", err)
 	}
+
 	defer c.provider.Close()
 
 	c.isRunning = false
 
 	err = c.stoppedHook(ctx)
 	if err != nil {
-		return err
+		return fmt.Errorf("stopped hook: %w", err)
 	}
 
 	return nil
 }
 
-// Terminate is used to kill the container. It is usually triggered by as defer function.
+// Terminate calls stops and then removes the container including its volumes.
+// If its image was built it and all child images are also removed unless
+// the [FromDockerfile.KeepImage] on the [ContainerRequest] was set to true.
+//
+// The following hooks are called in order:
+//   - [ContainerLifecycleHooks.PreTerminates]
+//   - [ContainerLifecycleHooks.PostTerminates]
 func (c *DockerContainer) Terminate(ctx context.Context) error {
+	// ContainerRemove hardcodes stop timeout to 3 seconds which is too short
+	// to ensure that child containers are stopped so we manually call stop.
+	// TODO: make this configurable via a functional option.
+	timeout := 10 * time.Second
+	err := c.Stop(ctx, &timeout)
+	if err != nil && !isCleanupSafe(err) {
+		return fmt.Errorf("stop: %w", err)
+	}
+
 	select {
 	// close reaper if it was created
 	case c.terminationSignal <- true:
@@ -296,6 +317,8 @@ func (c *DockerContainer) Terminate(ctx context.Context) error {
 
 	defer c.provider.client.Close()
 
+	// TODO: Handle errors from ContainerRemove more correctly, e.g. should we
+	// run the terminated hook?
 	errs := []error{
 		c.terminatingHook(ctx),
 		c.provider.client.ContainerRemove(ctx, c.GetContainerID(), container.RemoveOptions{

docker_auth_test.go

@@ -164,8 +164,8 @@ func TestBuildContainerFromDockerfile(t *testing.T) {
 	}
 
 	redisC, err := prepareRedisImage(ctx, req)
+	CleanupContainer(t, redisC)
 	require.NoError(t, err)
-	terminateContainerOnEnd(t, ctx, redisC)
 }
 
 // removeImageFromLocalCache removes the image from the local cache
@@ -202,16 +202,15 @@ func TestBuildContainerFromDockerfileWithDockerAuthConfig(t *testing.T) {
 			BuildArgs: map[string]*string{
 				"REGISTRY_HOST": &registryHost,
 			},
-			Repo:          "localhost",
-			PrintBuildLog: true,
+			Repo: "localhost",
 		},
 		AlwaysPullImage: true, // make sure the authentication takes place
 		ExposedPorts:    []string{"6379/tcp"},
 		WaitingFor:      wait.ForLog("Ready to accept connections"),
 	}
 
 	redisC, err := prepareRedisImage(ctx, req)
-	terminateContainerOnEnd(t, ctx, redisC)
+	CleanupContainer(t, redisC)
 	require.NoError(t, err)
 }
 
@@ -237,7 +236,7 @@ func TestBuildContainerFromDockerfileShouldFailWithWrongDockerAuthConfig(t *test
 	}
 
 	redisC, err := prepareRedisImage(ctx, req)
-	terminateContainerOnEnd(t, ctx, redisC)
+	CleanupContainer(t, redisC)
 	require.Error(t, err)
 }
 
@@ -259,7 +258,7 @@ func TestCreateContainerFromPrivateRegistry(t *testing.T) {
 		ContainerRequest: req,
 		Started:          true,
 	})
-	terminateContainerOnEnd(t, ctx, redisContainer)
+	CleanupContainer(t, redisContainer)
 	require.NoError(t, err)
 }
 
@@ -298,6 +297,7 @@ func prepareLocalRegistryWithAuth(t *testing.T) string {
 	}
 
 	registryC, err := GenericContainer(ctx, genContainerReq)
+	CleanupContainer(t, registryC)
 	require.NoError(t, err)
 
 	mappedPort, err := registryC.MappedPort(ctx, "5000/tcp")
@@ -310,9 +310,6 @@ func prepareLocalRegistryWithAuth(t *testing.T) string {
 	t.Cleanup(func() {
 		removeImageFromLocalCache(t, addr+"/redis:5.0-alpine")
 	})
-	t.Cleanup(func() {
-		require.NoError(t, registryC.Terminate(context.Background()))
-	})
 
 	_, cancel := context.WithCancel(context.Background())
 	t.Cleanup(cancel)