From 6c4c3f963f1ebc7fcf037a78b8c0e948b3d4bec4 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 3 Jul 2026 11:55:05 -0500 Subject: [PATCH 1/3] fix(docker): handling for exited containers that fail to start Signed-off-by: Samuel K --- pkg/devcontainer/config/container_details.go | 7 +- pkg/docker/helper.go | 125 +++++++++++++------ pkg/docker/helper_test.go | 51 ++++++++ pkg/driver/docker/docker_test.go | 1 + pkg/driver/docker/lifecycle.go | 43 +++++-- pkg/driver/docker/lifecycle_test.go | 95 ++++++++++++++ 6 files changed, 276 insertions(+), 46 deletions(-) create mode 100644 pkg/driver/docker/lifecycle_test.go diff --git a/pkg/devcontainer/config/container_details.go b/pkg/devcontainer/config/container_details.go index d4fd610b6..508b6836d 100644 --- a/pkg/devcontainer/config/container_details.go +++ b/pkg/devcontainer/config/container_details.go @@ -43,6 +43,9 @@ type ContainerDetailsConfig struct { } type ContainerDetailsState struct { - Status string `json:"Status,omitempty"` - StartedAt string `json:"StartedAt,omitempty"` + Status string `json:"Status,omitempty"` + StartedAt string `json:"StartedAt,omitempty"` + FinishedAt string `json:"FinishedAt,omitempty"` + ExitCode int `json:"ExitCode,omitempty"` + Error string `json:"Error,omitempty"` } diff --git a/pkg/docker/helper.go b/pkg/docker/helper.go index e996c4b48..1b6116228 100644 --- a/pkg/docker/helper.go +++ b/pkg/docker/helper.go @@ -30,6 +30,17 @@ const ( DockerBuilderBuildKit ) +const ( + containerRunningPollInterval = 500 * time.Millisecond + containerRunningTimeout = 30 * time.Second + containerExitGrace = 2 * time.Second +) + +var ( + ErrContainerTerminal = errors.New("container in terminal state") + ErrContainerExited = errors.New("container exited after start") +) + func (db DockerBuilder) String() string { return [...]string{"", "buildx", "buildkit"}[db] } @@ -232,20 +243,11 @@ func (r *DockerHelper) StartContainer(ctx context.Context, containerId string) e return nil } -// ErrContainerTerminal indicates a container entered an unrecoverable state -// (e.g. "dead" or "removing") and cannot be restarted. -var ErrContainerTerminal = errors.New("container in terminal state") - -const ( - containerRunningPollInterval = 500 * time.Millisecond - containerRunningTimeout = 30 * time.Second -) - -// WaitContainerRunning polls docker inspect until the container reports -// status "running" or the context/timeout expires. It does not start -// the container — the caller is responsible for that. +// WaitContainerRunning waits for the given container to be running, returning an error if +// it is in a terminal state or does not become running within a timeout. func (r *DockerHelper) WaitContainerRunning(ctx context.Context, containerID string) error { var lastErr error + start := time.Now() pollErr := wait.PollUntilContextTimeout( ctx, containerRunningPollInterval, containerRunningTimeout, true, func(ctx context.Context) (bool, error) { @@ -255,31 +257,8 @@ func (r *DockerHelper) WaitContainerRunning(ctx context.Context, containerID str log.Debugf("WaitContainerRunning: inspect error (will retry): %v", err) return false, nil } - if len(details) == 0 { - return false, fmt.Errorf( - "container %s disappeared while waiting for it to start", - containerID, - ) - } lastErr = nil - status := strings.ToLower(details[0].State.Status) - if status == "running" { - return true, nil - } - if status == "removing" || status == "dead" { - return false, fmt.Errorf( - "%w: container %s is %q", - ErrContainerTerminal, - containerID, - status, - ) - } - log.Debugf( - "WaitContainerRunning: container %s status=%s, waiting", - containerID, - status, - ) - return false, nil + return r.evaluateContainerState(ctx, containerID, details, time.Since(start)) }, ) if pollErr != nil && lastErr != nil { @@ -464,6 +443,80 @@ func (r *DockerHelper) GetContainerLogs( return cmd.Run() } +// containerStateError returns an error describing the container's state, including its +// exit code and logs. +func (r *DockerHelper) containerStateError( + ctx context.Context, + containerID string, + state config.ContainerDetailsState, + sentinel error, +) error { + detail := fmt.Sprintf("exit code %d", state.ExitCode) + if state.Error != "" { + detail += fmt.Sprintf(", error: %s", state.Error) + } + if logs := r.tailContainerLogs(ctx, containerID, 20); logs != "" { + detail += fmt.Sprintf("\nlogs:\n%s", logs) + } + return fmt.Errorf( + "%w: container %s is %q (%s)", + sentinel, + containerID, + strings.ToLower(state.Status), + detail, + ) +} + +// tailContainerLogs returns the tail of the container's logs for diagnostics, +// or "" if the logs cannot be retrieved. +func (r *DockerHelper) tailContainerLogs(ctx context.Context, containerID string, lines int) string { + out, err := r.buildCmd(ctx, "logs", containerID, "--tail", fmt.Sprintf("%d", lines)).CombinedOutput() + if err != nil { + return "" + } + return strings.TrimSpace(string(out)) +} + +// evaluateContainerState checks the container's state and returns true if it is running. +// If the container is in a terminal state or has exited after the grace period, it returns an error. +func (r *DockerHelper) evaluateContainerState( + ctx context.Context, + containerID string, + details []config.ContainerDetails, + elapsed time.Duration, +) (bool, error) { + if len(details) == 0 { + return false, fmt.Errorf( + "container %s disappeared while waiting for it to start", + containerID, + ) + } + state := details[0].State + status := strings.ToLower(state.Status) + if status == "running" { + return true, nil + } + if sentinel := failedBootSentinel(status, elapsed > containerExitGrace); sentinel != nil { + return false, r.containerStateError(ctx, containerID, state, sentinel) + } + log.Debugf("WaitContainerRunning: container %s status=%s, waiting", containerID, status) + return false, nil +} + +// failedBootSentinel returns an error if the container is in a terminal state or has exited +// after the grace period, or nil if it is still booting. +func failedBootSentinel(status string, graceElapsed bool) error { + switch status { + case "dead", "removing": + return ErrContainerTerminal + case "exited", "created": + if graceElapsed { + return ErrContainerExited + } + } + return nil +} + func (r *DockerHelper) buildCmd(ctx context.Context, args ...string) *exec.Cmd { cmd := exec.CommandContext(ctx, r.DockerCommand, args...) if r.Environment != nil { diff --git a/pkg/docker/helper_test.go b/pkg/docker/helper_test.go index 67f15630f..a747e8a6f 100644 --- a/pkg/docker/helper_test.go +++ b/pkg/docker/helper_test.go @@ -2,11 +2,13 @@ package docker import ( "context" + "errors" "io" "os" "path/filepath" "strings" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -146,6 +148,55 @@ esac assert.Equal(t, []string{"c1"}, got) } +func TestWaitContainerRunning_RunningSucceeds(t *testing.T) { + tmp := t.TempDir() + bin := writeScript(t, tmp, "docker-fake", `#!/bin/sh +case "$1" in + inspect) echo '[{"ID":"c1","State":{"Status":"running"}}]' ;; +esac +`) + + h := &DockerHelper{DockerCommand: bin} + require.NoError(t, h.WaitContainerRunning(context.Background(), "c1")) +} + +func TestWaitContainerRunning_ExitedFailsFast(t *testing.T) { + tmp := t.TempDir() + bin := writeScript(t, tmp, "docker-fake", `#!/bin/sh +case "$1" in + inspect) echo '[{"ID":"c1","State":{"Status":"exited","ExitCode":137,"Error":"OOM"}}]' ;; + logs) echo 'boom' ;; +esac +`) + + h := &DockerHelper{DockerCommand: bin} + start := time.Now() + err := h.WaitContainerRunning(context.Background(), "c1") + + require.Error(t, err) + assert.True(t, errors.Is(err, ErrContainerExited), "expected exited sentinel, got %v", err) + assert.False(t, errors.Is(err, ErrContainerTerminal), "exited must not be terminal") + assert.Contains(t, err.Error(), "137") + assert.Contains(t, err.Error(), "OOM") + assert.Less(t, time.Since(start), 10*time.Second) +} + +func TestWaitContainerRunning_DeadFailsFast(t *testing.T) { + tmp := t.TempDir() + bin := writeScript(t, tmp, "docker-fake", `#!/bin/sh +case "$1" in + inspect) echo '[{"ID":"c1","State":{"Status":"dead","ExitCode":1}}]' ;; + logs) echo '' ;; +esac +`) + + h := &DockerHelper{DockerCommand: bin} + err := h.WaitContainerRunning(context.Background(), "c1") + + require.Error(t, err) + assert.True(t, errors.Is(err, ErrContainerTerminal), "expected terminal sentinel, got %v", err) +} + func TestGPUSupportEnabled_CommandFailure(t *testing.T) { tmp := t.TempDir() bin := writeScript(t, tmp, "bad-runtime", `#!/bin/sh diff --git a/pkg/driver/docker/docker_test.go b/pkg/driver/docker/docker_test.go index b9d43e77d..fc4ef28fd 100644 --- a/pkg/driver/docker/docker_test.go +++ b/pkg/driver/docker/docker_test.go @@ -15,6 +15,7 @@ const ( testOSLinux = "linux" testRemoteUser = "vscode" testRunArg = "run" + testDockerCmd = "docker" testContainerUser = "container" ) diff --git a/pkg/driver/docker/lifecycle.go b/pkg/driver/docker/lifecycle.go index af038f445..fb358ecb6 100644 --- a/pkg/driver/docker/lifecycle.go +++ b/pkg/driver/docker/lifecycle.go @@ -2,6 +2,7 @@ package docker import ( "context" + "errors" "fmt" "io" "strings" @@ -12,6 +13,8 @@ import ( "github.com/devsy-org/devsy/pkg/log" ) +const containerRestartAttempts = 3 + func (d *dockerDriver) CommandDevContainer( ctx context.Context, params *driver.CommandParams, @@ -35,8 +38,9 @@ func (d *dockerDriver) CommandDevContainer( return d.Docker.Run(ctx, args, params.Stdin, params.Stdout, params.Stderr) } -// ensureContainerRunning restarts a stopped container and waits for it to come -// up. A container in a terminal state (dead/removing) is reported as an error. +// ensureContainerRunning checks that the given container is running, and if +// not, attempts to start it and wait for it to be running. If the container is +// in a terminal state (dead or removing), it returns an error. func (d *dockerDriver) ensureContainerRunning( ctx context.Context, container *config.ContainerDetails, @@ -54,17 +58,40 @@ func (d *dockerDriver) ensureContainerRunning( return nil } - log.Infof( - "container %s is not running (status=%s), restarting", - container.ID, status, + var lastErr error + for attempt := 1; attempt <= containerRestartAttempts; attempt++ { + if err := ctx.Err(); err != nil { + return err + } + log.Infof( + "container %s is not running (status=%s), restarting (attempt %d/%d)", + container.ID, status, attempt, containerRestartAttempts, + ) + err := d.restartAndWait(ctx, container.ID) + if err == nil { + log.Infof("container %s is now running", container.ID) + return nil + } + if errors.Is(err, docker.ErrContainerTerminal) { + return err + } + lastErr = err + log.Debugf("container %s restart attempt %d failed: %v", container.ID, attempt, err) + } + + return fmt.Errorf( + "%w: container %s did not stay running after %d restart attempts: %v", + docker.ErrContainerTerminal, container.ID, containerRestartAttempts, lastErr, ) - if err := d.Docker.StartContainer(ctx, container.ID); err != nil { +} + +func (d *dockerDriver) restartAndWait(ctx context.Context, containerID string) error { + if err := d.Docker.StartContainer(ctx, containerID); err != nil { return fmt.Errorf("restart container: %w", err) } - if err := d.Docker.WaitContainerRunning(ctx, container.ID); err != nil { + if err := d.Docker.WaitContainerRunning(ctx, containerID); err != nil { return fmt.Errorf("wait for container to be running: %w", err) } - log.Infof("container %s is now running", container.ID) return nil } diff --git a/pkg/driver/docker/lifecycle_test.go b/pkg/driver/docker/lifecycle_test.go new file mode 100644 index 000000000..6e03de111 --- /dev/null +++ b/pkg/driver/docker/lifecycle_test.go @@ -0,0 +1,95 @@ +package docker + +import ( + "context" + "os" + "path/filepath" + "strconv" + "testing" + + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/docker" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeDocker writes an executable shell script that emulates the docker CLI +// subcommands used by ensureContainerRunning. `inspect` reports "exited" until +// the number of prior `start` invocations reaches startsUntilRunning, after +// which it reports "running". A startsUntilRunning of -1 keeps it exited +// forever (a container that never boots). +func fakeDocker(t *testing.T, startsUntilRunning int) string { + t.Helper() + dir := t.TempDir() + state := filepath.Join(dir, "starts") + script := `#!/bin/sh +STATE="` + state + `" +THRESHOLD=` + strconv.Itoa(startsUntilRunning) + ` +case "$1" in + start) + n=$(cat "$STATE" 2>/dev/null || echo 0) + n=$((n + 1)) + echo "$n" > "$STATE" + echo started + ;; + inspect) + n=$(cat "$STATE" 2>/dev/null || echo 0) + if [ "$THRESHOLD" -ge 0 ] && [ "$n" -ge "$THRESHOLD" ]; then + echo '[{"ID":"c1","State":{"Status":"running"}}]' + else + echo '[{"ID":"c1","State":{"Status":"exited","ExitCode":1}}]' + fi + ;; + logs) echo 'boot failed' ;; +esac +` + path := filepath.Join(dir, "docker-fake") + //nolint:gosec // test helper script needs exec bit + require.NoError(t, os.WriteFile(path, []byte(script), 0o755)) + return path +} + +func newExitedContainer() *config.ContainerDetails { + return &config.ContainerDetails{ + ID: "c1", + State: config.ContainerDetailsState{Status: "exited"}, + } +} + +func TestEnsureContainerRunning_AlreadyRunning(t *testing.T) { + d := &dockerDriver{Docker: &docker.DockerHelper{DockerCommand: testDockerCmd}} + container := &config.ContainerDetails{ + ID: "c1", + State: config.ContainerDetailsState{Status: "running"}, + } + + require.NoError(t, d.ensureContainerRunning(context.Background(), container)) +} + +func TestEnsureContainerRunning_TerminalNotRestarted(t *testing.T) { + d := &dockerDriver{Docker: &docker.DockerHelper{DockerCommand: testDockerCmd}} + container := &config.ContainerDetails{ + ID: "c1", + State: config.ContainerDetailsState{Status: "dead"}, + } + + err := d.ensureContainerRunning(context.Background(), container) + require.Error(t, err) + assert.ErrorIs(t, err, docker.ErrContainerTerminal) +} + +func TestEnsureContainerRunning_RecoversOnRetry(t *testing.T) { + bin := fakeDocker(t, 2) + d := &dockerDriver{Docker: &docker.DockerHelper{DockerCommand: bin}} + + require.NoError(t, d.ensureContainerRunning(context.Background(), newExitedContainer())) +} + +func TestEnsureContainerRunning_NeverStartsFailsFast(t *testing.T) { + bin := fakeDocker(t, -1) + d := &dockerDriver{Docker: &docker.DockerHelper{DockerCommand: bin}} + + err := d.ensureContainerRunning(context.Background(), newExitedContainer()) + require.Error(t, err) + assert.ErrorIs(t, err, docker.ErrContainerTerminal) +} From 8ce29ca110299c2124fdcb9ba27acfc873d37cbb Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 3 Jul 2026 12:00:28 -0500 Subject: [PATCH 2/3] chore: remove unused FinishedAt Signed-off-by: Samuel K --- pkg/devcontainer/config/container_details.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/pkg/devcontainer/config/container_details.go b/pkg/devcontainer/config/container_details.go index 508b6836d..8f018a8d1 100644 --- a/pkg/devcontainer/config/container_details.go +++ b/pkg/devcontainer/config/container_details.go @@ -43,9 +43,8 @@ type ContainerDetailsConfig struct { } type ContainerDetailsState struct { - Status string `json:"Status,omitempty"` - StartedAt string `json:"StartedAt,omitempty"` - FinishedAt string `json:"FinishedAt,omitempty"` - ExitCode int `json:"ExitCode,omitempty"` - Error string `json:"Error,omitempty"` + Status string `json:"Status,omitempty"` + StartedAt string `json:"StartedAt,omitempty"` + ExitCode int `json:"ExitCode,omitempty"` + Error string `json:"Error,omitempty"` } From 17f3a55aa9214c5f334f797aacbf1c013d3b7d22 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 3 Jul 2026 12:07:46 -0500 Subject: [PATCH 3/3] style: format tailContainerLogs Signed-off-by: Samuel K --- pkg/docker/helper.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/docker/helper.go b/pkg/docker/helper.go index 1b6116228..e2c574242 100644 --- a/pkg/docker/helper.go +++ b/pkg/docker/helper.go @@ -469,8 +469,13 @@ func (r *DockerHelper) containerStateError( // tailContainerLogs returns the tail of the container's logs for diagnostics, // or "" if the logs cannot be retrieved. -func (r *DockerHelper) tailContainerLogs(ctx context.Context, containerID string, lines int) string { - out, err := r.buildCmd(ctx, "logs", containerID, "--tail", fmt.Sprintf("%d", lines)).CombinedOutput() +func (r *DockerHelper) tailContainerLogs( + ctx context.Context, + containerID string, + lines int, +) string { + out, err := r.buildCmd(ctx, "logs", containerID, "--tail", fmt.Sprintf("%d", lines)). + CombinedOutput() if err != nil { return "" }