From 5c75ea1b32b082c902aca9026a8737ded60ea191 Mon Sep 17 00:00:00 2001 From: Dmitry Meyer Date: Wed, 26 Aug 2026 10:45:29 +0000 Subject: [PATCH] [shim] Fix flaky Docker tests TestDocker_SSHServer became flaky after #4203, which stopped Run() from waiting for the container: the tests now wait for the completion themselves, and the wait was capped at 60 seconds, while the container installs openssh-server over the network, which was measured to take from 24 to 52 seconds. #4203 also added three more Docker tests, all running in parallel with each other. A timed out wait also leaves the container behind, as the deferred Remove() call rejects a task that is still running. The runner dir of such a container is deleted along with the test temporary dir, so the following test runs, which adopt every task container on the host, keep failing to save the state of a task whose dir is gone, reporting errors unrelated to the tests being run. That is, one flaky run poisons the runs that follow it. * The wait timeout is raised to 150 seconds, which is still below the timeout of the tests themselves. * The deferred cleanup call now terminates the task before removing it, so that a failed test does not leave its container behind. * Docker tests no longer run in parallel: they share the Docker daemon, and a DockerRunner adopts every task container on the host, including the containers of the other tests. Two runners processing the same task write over each other's task state files, and one of them may stop a container that the other still considers running. * The task info is no longer read inside the require.Eventually() condition, which is called in a goroutine that may outlive a timed out call, racing with the assertions that follow it. Note that the Docker tests now take about 64 instead of 26 seconds, almost entirely due to TestDocker_SSHServer, which no longer overlaps with the others. Co-Authored-By: Claude Opus 5 (1M context) --- runner/internal/shim/docker_test.go | 48 +++++++++++++++++++---------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/runner/internal/shim/docker_test.go b/runner/internal/shim/docker_test.go index f1f51eda5..b629a0d18 100644 --- a/runner/internal/shim/docker_test.go +++ b/runner/internal/shim/docker_test.go @@ -17,13 +17,17 @@ import ( "github.com/stretchr/testify/require" ) +/* +Docker tests are not parallel: they share the Docker daemon, and a DockerRunner adopts +every task container on the host, including the containers of the other tests. +*/ + // TestDocker_SSHServer pulls ubuntu image (without sshd), installs openssh-server and exits // Basically, it indirectly tests a shell script generated by getSSHShellCommands func TestDocker_SSHServer(t *testing.T) { if testing.Short() { t.Skip() } - t.Parallel() params := &dockerParametersMock{ commands: []string{"/usr/sbin/sshd -V 2>&1 | grep OpenSSH"}, @@ -39,7 +43,7 @@ func TestDocker_SSHServer(t *testing.T) { require.NoError(t, err) taskConfig := createTaskConfig(t) - defer dockerRunner.Remove(t.Context(), taskConfig.ID) + defer cleanupTask(t, dockerRunner, taskConfig.ID) assert.NoError(t, dockerRunner.Submit(ctx, taskConfig)) assert.NoError(t, dockerRunner.Start(ctx, taskConfig.ID)) @@ -50,7 +54,6 @@ func TestDocker_ShmNoexecByDefault(t *testing.T) { if testing.Short() { t.Skip() } - t.Parallel() params := &dockerParametersMock{ commands: []string{"mount | grep '/dev/shm .*size=65536k' | grep noexec"}, @@ -65,7 +68,7 @@ func TestDocker_ShmNoexecByDefault(t *testing.T) { require.NoError(t, err) taskConfig := createTaskConfig(t) - defer dockerRunner.Remove(t.Context(), taskConfig.ID) + defer cleanupTask(t, dockerRunner, taskConfig.ID) assert.NoError(t, dockerRunner.Submit(ctx, taskConfig)) assert.NoError(t, dockerRunner.Start(ctx, taskConfig.ID)) @@ -76,7 +79,6 @@ func TestDocker_ShmExecIfSizeSpecified(t *testing.T) { if testing.Short() { t.Skip() } - t.Parallel() params := &dockerParametersMock{ commands: []string{"mount | grep '/dev/shm .*size=1024k' | grep -v noexec"}, @@ -92,7 +94,7 @@ func TestDocker_ShmExecIfSizeSpecified(t *testing.T) { taskConfig := createTaskConfig(t) taskConfig.ShmSize = 1024 * 1024 - defer dockerRunner.Remove(t.Context(), taskConfig.ID) + defer cleanupTask(t, dockerRunner, taskConfig.ID) assert.NoError(t, dockerRunner.Submit(ctx, taskConfig)) assert.NoError(t, dockerRunner.Start(ctx, taskConfig.ID)) @@ -103,7 +105,6 @@ func TestDocker_ContainerExitedWithError(t *testing.T) { if testing.Short() { t.Skip() } - t.Parallel() params := &dockerParametersMock{ commands: []string{"echo failed for a reason", "exit 3"}, @@ -118,7 +119,7 @@ func TestDocker_ContainerExitedWithError(t *testing.T) { require.NoError(t, err) taskConfig := createTaskConfig(t) - defer dockerRunner.Remove(t.Context(), taskConfig.ID) + defer cleanupTask(t, dockerRunner, taskConfig.ID) require.NoError(t, dockerRunner.Submit(ctx, taskConfig)) require.NoError(t, dockerRunner.Start(ctx, taskConfig.ID)) @@ -135,7 +136,6 @@ func TestDocker_RestoredTaskIsTerminated(t *testing.T) { if testing.Short() { t.Skip() } - t.Parallel() params := &dockerParametersMock{ commands: []string{"sleep 3", "exit 7"}, @@ -156,7 +156,7 @@ func TestDocker_RestoredTaskIsTerminated(t *testing.T) { // The restarted shim restores the task from its container restartedRunner, err := NewDockerRunner(ctx, params) require.NoError(t, err) - defer restartedRunner.Remove(t.Context(), taskConfig.ID) + defer cleanupTask(t, restartedRunner, taskConfig.ID) require.Equal(t, taskConfig.ID, restartedRunner.TaskInfo(taskConfig.ID).ID) taskInfo := waitTaskTerminated(t, restartedRunner, taskConfig.ID) @@ -271,16 +271,32 @@ func assertTaskDone(t *testing.T, runner *DockerRunner, taskID string) { } // waitTaskTerminated processes tasks until the task is terminated, as Start() only -// starts the container and does not wait for it to exit +// starts the container and does not wait for it to exit. +// The timeout covers the whole container lifetime, which may be long, e.g., if the +// container installs packages func waitTaskTerminated(t *testing.T, runner *DockerRunner, taskID string) TaskInfo { t.Helper() - var taskInfo TaskInfo require.Eventually(t, func() bool { runner.ProcessTasks(t.Context()) - taskInfo = runner.TaskInfo(taskID) - return taskInfo.Status == TaskStatusTerminated - }, 60*time.Second, 200*time.Millisecond) - return taskInfo + return runner.TaskInfo(taskID).Status == TaskStatusTerminated + }, 150*time.Second, 200*time.Millisecond) + // Read outside of the condition, which Eventually runs in a separate goroutine + // that may outlive the call + return runner.TaskInfo(taskID) +} + +// cleanupTask terminates and removes the task, so that its container does not outlive +// the test even if the test fails. A container left behind is adopted by the runners of +// the following test runs, breaking them in confusing ways +func cleanupTask(t *testing.T, runner *DockerRunner, taskID string) { + t.Helper() + reason := string(types.TerminationReasonTerminatedByUser) + if err := runner.Terminate(t.Context(), taskID, 0, reason, "test cleanup"); err != nil { + t.Logf("failed to terminate task %s: %s", taskID, err) + } + if err := runner.Remove(t.Context(), taskID); err != nil { + t.Logf("failed to remove task %s: %s", taskID, err) + } } func createTaskConfig(t *testing.T) TaskConfig {