From 6db93f67ac83eb10e39e72afc88c692ebfe41f73 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Mon, 19 Oct 2026 19:17:38 -0400 Subject: [PATCH 1/2] fix(docker): RunOneShot force-removes its container when the run is cancelled (D-0712, D-0724) Each one-shot now has a container name (the caller's or a generated one). When the context ends by signal or deadline, the named container is removed with docker rm -f in its own process group, so db export, backup, restore and pg_restore stop writing to the target after nself reports failure. --- internal/docker/run_oneshot.go | 43 ++++++++++++++- internal/docker/run_oneshot_test.go | 86 ++++++++++++++++++++++++++++- 2 files changed, 127 insertions(+), 2 deletions(-) diff --git a/internal/docker/run_oneshot.go b/internal/docker/run_oneshot.go index 95b5568d2..a9805d84b 100644 --- a/internal/docker/run_oneshot.go +++ b/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,31 @@ 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() + if ctx.Err() != 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") diff --git a/internal/docker/run_oneshot_test.go b/internal/docker/run_oneshot_test.go index 5308a1f63..6ffb6885e 100644 --- a/internal/docker/run_oneshot_test.go +++ b/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,85 @@ 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) + } +} From cdf102ec093be1a324020d57c17c9a0c85a0a3da Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Mon, 19 Oct 2026 19:27:47 -0400 Subject: [PATCH 2/2] fix(docker): remove a one-shot container only when the run started (D-0712) --- internal/docker/run_oneshot.go | 4 +++- internal/docker/run_oneshot_test.go | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/internal/docker/run_oneshot.go b/internal/docker/run_oneshot.go index a9805d84b..0c69a8fc0 100644 --- a/internal/docker/run_oneshot.go +++ b/internal/docker/run_oneshot.go @@ -153,7 +153,9 @@ func RunOneShot(ctx context.Context, spec RunSpec) (stdout, stderr string, err e var so, se bytes.Buffer cmd.Stdout, cmd.Stderr = &so, &se runErr := cmd.Run() - if ctx.Err() != nil { + // 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 { diff --git a/internal/docker/run_oneshot_test.go b/internal/docker/run_oneshot_test.go index 6ffb6885e..1f08edfc5 100644 --- a/internal/docker/run_oneshot_test.go +++ b/internal/docker/run_oneshot_test.go @@ -174,3 +174,17 @@ func TestRunOneShot_NoRemoveOnSuccess(t *testing.T) { 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") + } +}