fix(docker): RunOneShot removes its container on cancel (D-0712, D-0724)

internal/docker/run_oneshot.go

@@ -17,11 +17,14 @@ package docker
 import (
 	"bytes"
 	"context"
+	"crypto/rand"
+	"encoding/hex"
 	"fmt"
 	"os"
 	"os/exec"
 	"sort"
 	"strings"
+	"time"
 )
 
 // RunSpec describes one `docker run --rm` invocation.
@@ -112,10 +115,31 @@ func buildRunArgs(spec RunSpec) ([]string, error) {
 	return append(args, spec.Args...), nil
 }
 
+// oneShotRemoveTimeout bounds the `docker rm -f` that follows a cancelled run.
+const oneShotRemoveTimeout = 30 * time.Second
+
+// newOneShotName returns a unique container name for a run whose caller set none.
+// It is a variable so tests can pin the name.
+var newOneShotName = func() string {
+	b := make([]byte, 6)
+	if _, err := rand.Read(b); err != nil {
+		return fmt.Sprintf("nself-oneshot-%d", time.Now().UnixNano())
+	}
+	return "nself-oneshot-" + hex.EncodeToString(b)
+}
+
 // RunOneShot runs spec to completion (`docker run --rm`) and returns what the
 // container wrote to stdout and stderr. A non-zero exit returns the captured
 // output together with an error that quotes the last stderr line.
+//
+// Cancelling ctx (signal or deadline) kills only the docker client, so the
+// container would keep running against its target. Every run therefore has a
+// name (spec.Name, or a generated one), and when ctx ends the container is
+// force-removed before RunOneShot returns.
 func RunOneShot(ctx context.Context, spec RunSpec) (stdout, stderr string, err error) {
+	if spec.Name == "" {
+		spec.Name = newOneShotName()
+	}
 	args, err := buildRunArgs(spec)
 	if err != nil {
 		return "", "", err
@@ -125,14 +149,33 @@ func RunOneShot(ctx context.Context, spec RunSpec) (stdout, stderr string, err e
 	for n, v := range spec.EnvPass {
 		cmd.Env = append(cmd.Env, n+"="+v)
 	}
+	cmd.WaitDelay = 5 * time.Second
 	var so, se bytes.Buffer
 	cmd.Stdout, cmd.Stderr = &so, &se
-	if runErr := cmd.Run(); runErr != nil {
+	runErr := cmd.Run()
+	// Remove only a container this run may have created: a run cancelled before
+	// docker started cannot have, and a caller-set name may belong to another.
+	if ctx.Err() != nil && cmd.Process != nil {
+		removeAfterCancel(spec.Name)
+	}
+	if runErr != nil {
 		return so.String(), se.String(), fmt.Errorf("docker run %s: %w: %s", spec.Image, runErr, lastLine(se.String()))
 	}
 	return so.String(), se.String(), nil
 }
 
+// removeAfterCancel force-removes the named container with a context that
+// outlives the cancelled one. It runs docker in its own process group so the
+// Ctrl-C that cancelled the run does not also interrupt the removal. An absent
+// container (the run was cancelled before docker created it) is not an error.
+func removeAfterCancel(name string) {
+	rmCtx, cancel := context.WithTimeout(context.Background(), oneShotRemoveTimeout)
+	defer cancel()
+	rm := exec.CommandContext(rmCtx, "docker", "rm", "-f", name)
+	setProcGroupAttr(rm)
+	_ = rm.Run()
+}
+
 // lastLine returns the last non-empty line of s, for error messages.
 func lastLine(s string) string {
 	lines := strings.Split(strings.TrimSpace(s), "\n")

internal/docker/run_oneshot_test.go

@@ -11,6 +11,7 @@ import (
 	"runtime"
 	"strings"
 	"testing"
+	"time"
 )
 
 // installRecordingDocker puts a `docker` script on PATH that records argv and
@@ -34,6 +35,7 @@ func installRecordingDocker(t *testing.T) string {
 
 func TestRunOneShot_SecretOnlyInEnv(t *testing.T) {
 	dir := installRecordingDocker(t)
+	pinOneShotName(t, "fixed-name")
 	const secret = "s3cr3t-token-value"
 	stdout, stderr, err := RunOneShot(context.Background(), RunSpec{
 		Image:   "example/img:1@sha256:abc",
@@ -50,7 +52,7 @@ func TestRunOneShot_SecretOnlyInEnv(t *testing.T) {
 		t.Errorf("output not captured: %q / %q", stdout, stderr)
 	}
 	argv, _ := os.ReadFile(filepath.Join(dir, "argv"))
-	want := "run --rm --user 1000:1000 --network acme-net -v /srv/ssl:/ssl -v /x/s.sh:/s.sh:ro -e B_NAME -e SECRET_VAL example/img:1@sha256:abc run --domains a.example.org"
+	want := "run --rm --name fixed-name --user 1000:1000 --network acme-net -v /srv/ssl:/ssl -v /x/s.sh:/s.sh:ro -e B_NAME -e SECRET_VAL example/img:1@sha256:abc run --domains a.example.org"
 	if got := strings.TrimSpace(string(argv)); got != want {
 		t.Errorf("argv = %q, want %q", got, want)
 	}
@@ -90,3 +92,99 @@ func TestBuildRunArgs_Hardening(t *testing.T) {
 		t.Errorf("argv = %q, want %q", got, want)
 	}
 }
+
+// pinOneShotName fixes the generated container name for the test.
+func pinOneShotName(t *testing.T, name string) {
+	t.Helper()
+	prev := newOneShotName
+	newOneShotName = func() string { return name }
+	t.Cleanup(func() { newOneShotName = prev })
+}
+
+// installHangingDocker puts a `docker` on PATH whose `run` records its argv
+// and blocks, and whose `rm` records its argv to $FAKE_DIR/rm.
+func installHangingDocker(t *testing.T) string {
+	t.Helper()
+	if runtime.GOOS == "windows" {
+		t.Skip("fake docker is a POSIX shell script")
+	}
+	dir := t.TempDir()
+	script := "#!/bin/sh\ncase \"$1\" in\nrun) echo \"$@\" > \"$FAKE_DIR/run\"; exec sleep 30;;\nrm) echo \"$@\" > \"$FAKE_DIR/rm\";;\nesac\n"
+	if err := os.WriteFile(filepath.Join(dir, "docker"), []byte(script), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH"))
+	t.Setenv("FAKE_DIR", dir)
+	return dir
+}
+
+// TestRunOneShot_CancelRemovesContainer: a cancelled run force-removes the
+// named container (D-0712, D-0724), whether the name was generated or the
+// caller's.
+func TestRunOneShot_CancelRemovesContainer(t *testing.T) {
+	cases := map[string]struct{ name, want string }{
+		"generated name": {"", "gen-name"},
+		"caller's name":  {"caller-name", "caller-name"},
+	}
+	for label, tc := range cases {
+		t.Run(label, func(t *testing.T) {
+			dir := installHangingDocker(t)
+			pinOneShotName(t, "gen-name")
+			ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
+			defer cancel()
+			go func() { // cancel once the fake docker has seen the run
+				for i := 0; i < 600; i++ {
+					if _, err := os.Stat(filepath.Join(dir, "run")); err == nil {
+						time.Sleep(100 * time.Millisecond)
+						cancel()
+						return
+					}
+					time.Sleep(50 * time.Millisecond)
+				}
+				cancel()
+			}()
+			start := time.Now()
+			_, _, err := RunOneShot(ctx, RunSpec{Image: "img", Name: tc.name})
+			if err == nil {
+				t.Fatal("want an error from the cancelled run")
+			}
+			if time.Since(start) > 10*time.Second {
+				t.Errorf("cancelled run took %v", time.Since(start))
+			}
+			run, _ := os.ReadFile(filepath.Join(dir, "run"))
+			if !strings.Contains(string(run), "--name "+tc.want+" ") {
+				t.Errorf("run argv %q lacks --name %s", run, tc.want)
+			}
+			rm, _ := os.ReadFile(filepath.Join(dir, "rm"))
+			if got := strings.TrimSpace(string(rm)); got != "rm -f "+tc.want {
+				t.Errorf("docker rm argv = %q, want %q", got, "rm -f "+tc.want)
+			}
+		})
+	}
+}
+
+// TestRunOneShot_NoRemoveOnSuccess: a run that finishes on its own is left to --rm.
+func TestRunOneShot_NoRemoveOnSuccess(t *testing.T) {
+	dir := installRecordingDocker(t)
+	if _, _, err := RunOneShot(context.Background(), RunSpec{Image: "i"}); err != nil {
+		t.Fatal(err)
+	}
+	argv, _ := os.ReadFile(filepath.Join(dir, "argv"))
+	if !strings.HasPrefix(string(argv), "run ") {
+		t.Errorf("last docker call = %q, want only the run", argv)
+	}
+}
+
+// TestRunOneShot_PreCancelledRemovesNothing: a context that ended before docker
+// started leaves a caller-named container (which may be another run's) alone.
+func TestRunOneShot_PreCancelledRemovesNothing(t *testing.T) {
+	dir := installHangingDocker(t)
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+	if _, _, err := RunOneShot(ctx, RunSpec{Image: "img", Name: "someone-elses"}); err == nil {
+		t.Fatal("want an error")
+	}
+	if _, err := os.Stat(filepath.Join(dir, "rm")); err == nil {
+		t.Error("docker rm ran although the run never started")
+	}
+}