diff --git a/docs/features/cli/index.md b/docs/features/cli/index.md index e3052e0476..d7eec10590 100644 --- a/docs/features/cli/index.md +++ b/docs/features/cli/index.md @@ -423,7 +423,7 @@ $ docker agent eval | [|./evals] [flags] | `--output ` | `/results` | Directory for results, logs, and session databases | | `--only ` | (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 ` | `1` | Number of times to repeat each evaluation (useful for computing baselines) | diff --git a/docs/features/evaluation/index.md b/docs/features/evaluation/index.md index 19f00a4742..865bd60f43 100644 --- a/docs/features/evaluation/index.md +++ b/docs/features/evaluation/index.md @@ -165,6 +165,11 @@ $ docker agent eval | [|./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: diff --git a/pkg/evaluation/eval.go b/pkg/evaluation/eval.go index 7974c425dd..2af8b5bd70 100644 --- a/pkg/evaluation/eval.go +++ b/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 { diff --git a/pkg/evaluation/eval_test.go b/pkg/evaluation/eval_test.go index 29f38cf2cc..eb81beef47 100644 --- a/pkg/evaluation/eval_test.go +++ b/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") +}