fix(eval): reap orphaned eval containers on context cancellation

docs/features/cli/index.md

@@ -423,7 +423,7 @@ $ docker agent eval <agent-file>|<registry-ref> [<eval-dir>|./evals] [flags]
 | `--output <dir>`    | `<eval-dir>/results`                 | Directory for results, logs, and session databases                         |
 | `--only <pattern>`  | (all)                                | Only run evals with file names matching these patterns (repeatable)        |
 | `--base-image`      | (default)                            | Custom base Docker image for eval containers                               |
-| `--keep-containers` | `false`                              | Keep containers after evaluation (don't remove with `--rm`)                |
+| `--keep-containers` | `false`                              | Keep containers after evaluation, including cancelled/interrupted runs (don't remove with `--rm`) |
 | `-e, --env`         | (none)                               | Environment variables to pass to container (`KEY` or `KEY=VALUE`, repeatable) |
 | `--repeat <n>`      | `1`                                  | Number of times to repeat each evaluation (useful for computing baselines) |
 

docs/features/evaluation/index.md

@@ -165,6 +165,11 @@ $ docker agent eval <agent-file>|<registry-ref> [<eval-dir>|./evals]
 | `-e, --env`         | (none)                      | Environment variables to pass to container (`KEY` or `KEY=VALUE`) |
 | `--repeat`          | `1`                         | Number of times to repeat each evaluation (useful for computing baselines) |
 
+> [!NOTE]
+> **Container cleanup on cancellation**
+>
+> If the run is cancelled (Ctrl-C or a timeout), docker-agent force-removes the in-flight eval containers so they don't keep running in the background. An individual eval failing does not cancel the run — evaluation continues with the rest — so this cleanup only kicks in when the whole run is cancelled. Pass `--keep-containers` to opt out and preserve containers for debugging even when a run is cancelled.
+
 ### Custom Base Images
 
 When `--base-image` is set, the eval harness builds a derived image on top of your base image at evaluation time. Two things happen automatically:

pkg/evaluation/eval.go

@@ -3,6 +3,7 @@ package evaluation
 
 import (
 	"bufio"
+	"bytes"
 	"cmp"
 	"context"
 	"encoding/json"
@@ -471,19 +472,15 @@ func (r *Runner) runDockerAgentInContainer(ctx context.Context, imageID string,
 	if err != nil {
 		return nil, fmt.Errorf("creating stdout pipe: %w", err)
 	}
-	stderr, err := cmd.StderrPipe()
-	if err != nil {
-		return nil, fmt.Errorf("creating stderr pipe: %w", err)
-	}
+	var stderrBuf bytes.Buffer
+	cmd.Stderr = &stderrBuf
+
+	cmd.WaitDelay = 10 * time.Second
 
 	if err := cmd.Start(); err != nil {
 		return nil, fmt.Errorf("starting docker run: %w", err)
 	}
-
-	var stderrData []byte
-	go func() {
-		stderrData, _ = io.ReadAll(stderr)
-	}()
+	defer r.reapContainer(containerName)
 
 	var events []map[string]any
 	scanner := bufio.NewScanner(stdout)
@@ -508,6 +505,7 @@ func (r *Runner) runDockerAgentInContainer(ctx context.Context, imageID string,
 	}
 
 	waitErr := cmd.Wait()
+	stderrData := stderrBuf.Bytes()
 	if waitErr != nil {
 		slog.DebugContext(ctx, "Container exited with error", "stderr", string(stderrData), "error", waitErr)
 	}
@@ -526,6 +524,30 @@ func (r *Runner) runDockerAgentInContainer(ctx context.Context, imageID string,
 	return events, nil
 }
 
+// reapContainer force-removes the eval container so it never outlives the
+// run. --rm normally removes it on a clean exit, but exec.CommandContext
+// only kills the docker CLI on ctx cancellation; the container itself is
+// never signaled and would otherwise leak. Uses a fresh context because the
+// eval ctx may already be cancelled by the time this runs.
+func (r *Runner) reapContainer(containerName string) {
+	if r.KeepContainers {
+		return
+	}
+
+	// Detached on purpose: the eval's ctx may already be cancelled
+	// (timeout, Ctrl-C) by the time this runs, and cleanup must still happen.
+	//rubocop:disable Lint/ContextConnectivity
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancel()
+
+	out, err := exec.CommandContext(ctx, "docker", "rm", "-f", containerName).CombinedOutput()
+	if err == nil || strings.Contains(string(out), "No such container") {
+		return
+	}
+
+	slog.WarnContext(ctx, "Failed to reap eval container", "container", containerName, "error", err, "output", string(out))
+}
+
 func parseContainerEvents(events []map[string]any) (response string, cost float64, outputTokens int64, toolCalls []string) {
 	var responseBuf strings.Builder
 	for _, event := range events {

pkg/evaluation/eval_test.go

@@ -2,7 +2,9 @@ package evaluation
 
 import (
 	"bytes"
+	"context"
 	"encoding/json"
+	"fmt"
 	"os"
 	"path/filepath"
 	"regexp"
@@ -13,6 +15,8 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/docker/docker-agent/pkg/config"
+	"github.com/docker/docker-agent/pkg/environment"
 	"github.com/docker/docker-agent/pkg/session"
 )
 
@@ -1006,3 +1010,236 @@ func TestNeedsJudge(t *testing.T) {
 		})
 	}
 }
+
+// writeDockerStub installs a fake "docker" executable on PATH that appends
+// its invocation args (one line per call) to logFile, so tests can assert
+// on how reapContainer invoked it without a real Docker daemon.
+func writeDockerStub(t *testing.T, logFile string) {
+	t.Helper()
+
+	binDir := t.TempDir()
+	script := fmt.Sprintf("#!/bin/sh\necho \"$@\" >> %q\n", logFile)
+	require.NoError(t, os.WriteFile(filepath.Join(binDir, "docker"), []byte(script), 0o755))
+	t.Setenv("PATH", binDir)
+}
+
+func TestReapContainer_RunsRmDashF(t *testing.T) {
+	logFile := filepath.Join(t.TempDir(), "docker-calls.log")
+	writeDockerStub(t, logFile)
+
+	r := &Runner{}
+	r.reapContainer("docker-agent-eval-1234")
+
+	data, err := os.ReadFile(logFile)
+	require.NoError(t, err)
+	assert.Contains(t, string(data), "rm -f docker-agent-eval-1234")
+}
+
+func TestReapContainer_RunsEvenWhenCallerContextAlreadyCancelled(t *testing.T) {
+	logFile := filepath.Join(t.TempDir(), "docker-calls.log")
+	writeDockerStub(t, logFile)
+
+	// Simulate the eval's context (timeout, Ctrl-C, ...) already being
+	// cancelled by the time reapContainer runs. reapContainer must not
+	// derive its context from it, so the cleanup still happens.
+	ctx, cancel := context.WithCancel(t.Context())
+	cancel()
+	require.Error(t, ctx.Err())
+
+	r := &Runner{}
+	r.reapContainer("docker-agent-eval-5678")
+
+	data, err := os.ReadFile(logFile)
+	require.NoError(t, err)
+	assert.Contains(t, string(data), "rm -f docker-agent-eval-5678")
+}
+
+func TestReapContainer_SkippedWhenKeepContainers(t *testing.T) {
+	logFile := filepath.Join(t.TempDir(), "docker-calls.log")
+	writeDockerStub(t, logFile)
+
+	r := &Runner{Config: Config{KeepContainers: true}}
+	r.reapContainer("docker-agent-eval-9999")
+
+	_, err := os.ReadFile(logFile)
+	assert.ErrorIs(t, err, os.ErrNotExist, "docker should never be invoked when --keep-containers is set")
+}
+
+func TestReapContainer_IgnoresNoSuchContainer(t *testing.T) {
+	// Common path: --rm already removed the container by the time reap
+	// runs, so "docker rm -f" fails with "No such container". That must
+	// not be surfaced as a warning-worthy failure.
+	binDir := t.TempDir()
+	script := "#!/bin/sh\necho \"Error: No such container: $2\" >&2\nexit 1\n"
+	require.NoError(t, os.WriteFile(filepath.Join(binDir, "docker"), []byte(script), 0o755))
+	t.Setenv("PATH", binDir)
+
+	r := &Runner{}
+	// Should not panic and should complete quickly despite the non-zero exit.
+	r.reapContainer("docker-agent-eval-missing")
+}
+
+// --- Integration-level tests for runDockerAgentInContainer ---
+//
+// These go through the real function (not reapContainer directly) using a
+// fake "docker" on PATH that distinguishes "docker run" from "docker rm", so
+// they protect the actual regression: they fail if the
+// `defer r.reapContainer(containerName)` registration in
+// runDockerAgentInContainer is ever removed or short-circuited.
+
+// writeDockerStubWithRunBehavior installs a fake "docker" executable on PATH
+// that appends every invocation's args (one line per call) to logFile. For
+// "docker run" calls it executes runBehavior, a POSIX shell snippet standing
+// in for the containerized process; "docker rm" calls always succeed. This
+// lets tests exercise runDockerAgentInContainer end-to-end, including its
+// deferred cleanup, without a Docker daemon.
+func writeDockerStubWithRunBehavior(t *testing.T, logFile, runBehavior string) {
+	t.Helper()
+
+	binDir := t.TempDir()
+	script := fmt.Sprintf("#!/bin/sh\necho \"$@\" >> %q\ncase \"$1\" in\nrun)\n%s\n;;\nrm)\nexit 0\n;;\nesac\n", logFile, runBehavior)
+	require.NoError(t, os.WriteFile(filepath.Join(binDir, "docker"), []byte(script), 0o755))
+	// Keep the real PATH behind the stub: the run behaviors below shell out to
+	// real utilities (sleep, head, tr); only "docker" itself must be faked.
+	t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH"))
+}
+
+// dockerCallLines returns the recorded docker invocations, one per line, in
+// the order they occurred.
+func dockerCallLines(t *testing.T, logFile string) []string {
+	t.Helper()
+
+	data, err := os.ReadFile(logFile)
+	require.NoError(t, err)
+	return strings.Split(strings.TrimRight(string(data), "\n"), "\n")
+}
+
+// containerNameFromRunArgs extracts the --name value from a recorded "docker
+// run ..." invocation line, so tests can confirm the matching "docker rm -f"
+// call targeted the same container.
+func containerNameFromRunArgs(t *testing.T, runLine string) string {
+	t.Helper()
+
+	m := regexp.MustCompile(`--name (\S+)`).FindStringSubmatch(runLine)
+	require.Len(t, m, 2, "expected --name in docker run args: %s", runLine)
+	return m[1]
+}
+
+// newTestRunner builds a Runner suitable for exercising
+// runDockerAgentInContainer without touching a real agent file or the host
+// environment.
+func newTestRunner(cfg Config) *Runner {
+	return &Runner{
+		Config:      cfg,
+		agentSource: config.NewBytesSource("agent.yaml", []byte("name: test")),
+		runConfig: &config.RuntimeConfig{
+			EnvProviderForTests: environment.NewNoEnvProvider(),
+		},
+	}
+}
+
+func TestRunDockerAgentInContainer_CallerContextCancellation(t *testing.T) {
+	logFile := filepath.Join(t.TempDir(), "docker-calls.log")
+	// "exec sleep 5" replaces the shell's own process image instead of
+	// forking a child, so SIGKILLing the docker run process (what ctx
+	// cancellation does) terminates the sleep directly -- matching how the
+	// real docker CLI process (not a shell wrapper) behaves.
+	writeDockerStubWithRunBehavior(t, logFile, "exec sleep 5")
+
+	r := newTestRunner(Config{})
+
+	ctx, cancel := context.WithCancel(t.Context())
+	go func() {
+		// Cancel only once "docker run" has actually started and recorded its
+		// invocation, so this genuinely exercises mid-flight cancellation
+		// rather than racing (and possibly losing to) process-startup latency.
+		assert.Eventually(t, func() bool {
+			data, err := os.ReadFile(logFile)
+			return err == nil && strings.HasPrefix(string(data), "run ")
+		}, 5*time.Second, 5*time.Millisecond)
+		cancel()
+	}()
+
+	start := time.Now()
+	_, err := r.runDockerAgentInContainer(ctx, "image123", []string{"question"}, "")
+	require.Error(t, err)
+	assert.Less(t, time.Since(start), 5*time.Second, "context cancellation should kill docker run before its 5s sleep completes")
+
+	calls := dockerCallLines(t, logFile)
+	require.GreaterOrEqual(t, len(calls), 2, "expected both a docker run and a docker rm -f call, got: %v", calls)
+	require.True(t, strings.HasPrefix(calls[0], "run "))
+	containerName := containerNameFromRunArgs(t, calls[0])
+	assert.Contains(t, calls[len(calls)-1], "rm -f "+containerName, "container must still be reaped when the caller context is cancelled")
+}
+
+func TestRunDockerAgentInContainer_NonZeroExit(t *testing.T) {
+	logFile := filepath.Join(t.TempDir(), "docker-calls.log")
+	writeDockerStubWithRunBehavior(t, logFile, "echo boom >&2; exit 7")
+
+	r := newTestRunner(Config{})
+	_, err := r.runDockerAgentInContainer(t.Context(), "image123", []string{"question"}, "")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "container failed")
+	assert.Contains(t, err.Error(), "boom")
+
+	calls := dockerCallLines(t, logFile)
+	require.GreaterOrEqual(t, len(calls), 2)
+	require.True(t, strings.HasPrefix(calls[0], "run "))
+	containerName := containerNameFromRunArgs(t, calls[0])
+	assert.Contains(t, calls[len(calls)-1], "rm -f "+containerName, "container must be reaped after a non-zero docker run exit")
+}
+
+func TestRunDockerAgentInContainer_ScannerTokenTooLong(t *testing.T) {
+	logFile := filepath.Join(t.TempDir(), "docker-calls.log")
+	// A single line above the 10 MiB scanner buffer limit (no newline) trips
+	// bufio.ErrTooLong; the reader loop must handle that without a panic.
+	// Once the scanner gives up, nothing keeps draining stdout, so the
+	// still-writing "tr" can block on a full pipe forever -- bound the test
+	// with a timeout (as any real caller eventually would) so that residual
+	// hang is force-killed rather than wedging the test.
+	writeDockerStubWithRunBehavior(t, logFile, `head -c 11000000 /dev/zero | tr '\0' 'a'`)
+
+	r := newTestRunner(Config{})
+	ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second)
+	defer cancel()
+	_, err := r.runDockerAgentInContainer(ctx, "image123", []string{"question"}, "")
+	require.Error(t, err)
+
+	calls := dockerCallLines(t, logFile)
+	require.GreaterOrEqual(t, len(calls), 2)
+	require.True(t, strings.HasPrefix(calls[0], "run "))
+	containerName := containerNameFromRunArgs(t, calls[0])
+	assert.Contains(t, calls[len(calls)-1], "rm -f "+containerName, "container must be reaped after a scanner error")
+}
+
+func TestRunDockerAgentInContainer_Success(t *testing.T) {
+	logFile := filepath.Join(t.TempDir(), "docker-calls.log")
+	writeDockerStubWithRunBehavior(t, logFile, `echo '{"type":"agent_choice","content":"hi"}'`)
+
+	r := newTestRunner(Config{})
+	events, err := r.runDockerAgentInContainer(t.Context(), "image123", []string{"question"}, "")
+	require.NoError(t, err)
+	require.Len(t, events, 1)
+	assert.Equal(t, "agent_choice", events[0]["type"])
+
+	calls := dockerCallLines(t, logFile)
+	require.Len(t, calls, 2)
+	require.True(t, strings.HasPrefix(calls[0], "run "))
+	containerName := containerNameFromRunArgs(t, calls[0])
+	assert.Contains(t, calls[1], "rm -f "+containerName, "container must be reaped after a normal successful exit")
+}
+
+func TestRunDockerAgentInContainer_KeepContainersSkipsCleanup(t *testing.T) {
+	logFile := filepath.Join(t.TempDir(), "docker-calls.log")
+	writeDockerStubWithRunBehavior(t, logFile, `echo '{"type":"agent_choice","content":"hi"}'`)
+
+	r := newTestRunner(Config{KeepContainers: true})
+	events, err := r.runDockerAgentInContainer(t.Context(), "image123", []string{"question"}, "")
+	require.NoError(t, err)
+	require.Len(t, events, 1)
+
+	calls := dockerCallLines(t, logFile)
+	require.Len(t, calls, 1, "docker rm must never run when KeepContainers is set")
+	assert.True(t, strings.HasPrefix(calls[0], "run "))
+	assert.NotContains(t, calls[0], "--rm", "run args must omit --rm when KeepContainers is set")
+}