From 29558bd5b8bc565eb9277421568b359cf344199f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 21 Aug 2026 20:29:36 +0000 Subject: [PATCH 1/5] feat: wire DEVSY_AGENT_PATH to entrypoint Signed-off-by: Samuel K --- pkg/agent/delivery/local_docker.go | 2 +- pkg/config/env.go | 11 ++- pkg/devcontainer/compose_test.go | 2 +- pkg/devcontainer/single.go | 11 ++- pkg/devcontainer/single_test.go | 9 +- pkg/docker/helper.go | 32 ++++++-- pkg/docker/helper_test.go | 20 +++++ pkg/driver/docker/build.go | 3 +- pkg/driver/docker/lifecycle.go | 122 +++++++++++++++++++--------- pkg/driver/docker/lifecycle_test.go | 53 +++++++++++- 10 files changed, 207 insertions(+), 58 deletions(-) diff --git a/pkg/agent/delivery/local_docker.go b/pkg/agent/delivery/local_docker.go index fd29a1388..8b23e403c 100644 --- a/pkg/agent/delivery/local_docker.go +++ b/pkg/agent/delivery/local_docker.go @@ -66,7 +66,7 @@ func (d *LocalDockerDelivery) DeliverPreStart(ctx context.Context, opts PreStart if opts.RunOptions.Env == nil { opts.RunOptions.Env = make(map[string]string) } - opts.RunOptions.Env["DEVSY_AGENT_PATH"] = volumeMountPath + "/" + binaryName() + opts.RunOptions.Env[pkgconfig.EnvAgentPath] = volumeMountPath + "/" + binaryName() return nil } diff --git a/pkg/config/env.go b/pkg/config/env.go index 48ab7ef75..b42a69d3d 100644 --- a/pkg/config/env.go +++ b/pkg/config/env.go @@ -60,10 +60,15 @@ const ( // EnvAgentPreferDownload forces agent binary download even if a local copy exists. EnvAgentPreferDownload = "DEVSY_AGENT_PREFER_DOWNLOAD" - // EnvOS is set to the host operating system (runtime.GOOS). + // EnvAgentPath is the path to the agent binary inside the workspace + // container, set by agent delivery so the container entrypoint can locate + // it (defaults to /usr/local/bin/devsy). + EnvAgentPath = "DEVSY_AGENT_PATH" + + // EnvOS is set to the host operating system. EnvOS = "DEVSY_OS" - // EnvArch is set to the host architecture (runtime.GOARCH). + // EnvArch is set to the host architecture. EnvArch = "DEVSY_ARCH" // EnvLogLevel is set to the current log level. @@ -121,8 +126,6 @@ const ( // EnvProviderPrefix is the prefix for provider-specific option env vars (append provider name + "_"). EnvProviderPrefix = EnvPrefix + "PROVIDER_" - // --- Provider-scoped env vars (set when running provider commands) ---. - // EnvProviderWorkspaceID is the workspace identifier passed to providers. EnvProviderWorkspaceID = "WORKSPACE_ID" diff --git a/pkg/devcontainer/compose_test.go b/pkg/devcontainer/compose_test.go index 516096cad..41009c0d6 100644 --- a/pkg/devcontainer/compose_test.go +++ b/pkg/devcontainer/compose_test.go @@ -781,7 +781,7 @@ func TestBuildOverrideEntrypointAppendsUserEntrypoint(t *testing.T) { func TestBuildOverrideEntrypointKeepsDefaultEntrypointReachable(t *testing.T) { script := buildOverrideEntrypoint(&config.MergedDevContainerConfig{}, nil) body := script[2] - if !strings.Contains(body, "devsy internal agent container daemon") { + if !strings.Contains(body, "internal agent container daemon") { t.Errorf("expected default entrypoint invocation in script, got %q", body) } } diff --git a/pkg/devcontainer/single.go b/pkg/devcontainer/single.go index 26936b825..d03a81354 100644 --- a/pkg/devcontainer/single.go +++ b/pkg/devcontainer/single.go @@ -44,10 +44,17 @@ func joinShellStatements(statements ...string) string { // DefaultEntrypoint waits for the devsy agent binary to become available // before handing off to the container daemon. var DefaultEntrypoint = joinShellStatements( - `while ! command -v /usr/local/bin/devsy >/dev/null 2>&1; do echo "waiting for devsy agent to be available"`, + fmt.Sprintf( + `while ! command -v "${%s:-/usr/local/bin/devsy}" >/dev/null 2>&1`, + pkgconfig.EnvAgentPath, + ), + `do echo "waiting for devsy agent to be available"`, "sleep 1", "done", - "exec /usr/local/bin/devsy internal agent container daemon", + fmt.Sprintf( + `exec "${%s:-/usr/local/bin/devsy}" internal agent container daemon`, + pkgconfig.EnvAgentPath, + ), ) // resolvedContainer holds the outputs that every code path through diff --git a/pkg/devcontainer/single_test.go b/pkg/devcontainer/single_test.go index ec425b617..079fe67fe 100644 --- a/pkg/devcontainer/single_test.go +++ b/pkg/devcontainer/single_test.go @@ -209,9 +209,12 @@ func TestDefaultEntrypointSingleLine(t *testing.T) { if strings.Contains(DefaultEntrypoint, "\n") { t.Fatalf("DefaultEntrypoint must be single-line, got %q", DefaultEntrypoint) } - if !strings.Contains(DefaultEntrypoint, "devsy internal agent container daemon") { + if !strings.Contains(DefaultEntrypoint, "internal agent container daemon") { t.Errorf("DefaultEntrypoint must invoke the agent daemon, got %q", DefaultEntrypoint) } + if !strings.Contains(DefaultEntrypoint, `"${DEVSY_AGENT_PATH:-/usr/local/bin/devsy}"`) { + t.Errorf("DefaultEntrypoint must honor DEVSY_AGENT_PATH, got %q", DefaultEntrypoint) + } } func TestGetStartScriptSingleLine(t *testing.T) { @@ -227,7 +230,7 @@ func TestGetStartScriptSingleLine(t *testing.T) { if !strings.Contains(got, `exec "$@"`) { t.Fatalf("GetStartScript() must keep the shell exec passthrough, got %q", got) } - if !strings.Contains(got, "devsy internal agent container daemon") { + if !strings.Contains(got, "internal agent container daemon") { t.Fatalf("GetStartScript() must invoke the agent, got %q", got) } } @@ -245,7 +248,7 @@ func TestGetStartScriptPreservesStatementOrder(t *testing.T) { `exec "$@"`, "first-entrypoint", "second-entrypoint", - "devsy internal agent container daemon", + "internal agent container daemon", } lastIdx := -1 for _, want := range wantOrder { diff --git a/pkg/docker/helper.go b/pkg/docker/helper.go index f9f190c19..e6e451d1d 100644 --- a/pkg/docker/helper.go +++ b/pkg/docker/helper.go @@ -32,7 +32,7 @@ const ( DockerBuilderBuildKit ) -const ( +var ( containerRunningPollInterval = 500 * time.Millisecond containerRunningTimeout = 30 * time.Second containerExitGrace = 2 * time.Second @@ -42,6 +42,12 @@ var ( ErrContainerTerminal = errors.New("container in terminal state") ErrContainerExited = errors.New("container exited after start") ErrImageNotFound = errors.New("image not found") + + // podmanMachineStartTimeout is the maximum time to wait for a Podman machine to start. + podmanMachineStartTimeout = 90 * time.Second + + // pingTimeout is the maximum time to wait for a ping to the runtime daemon. + pingTimeout = 30 * time.Second ) var imageNotFoundMarkers = []string{ @@ -158,11 +164,6 @@ func (r *DockerHelper) ClientVersion(ctx context.Context) string { return strings.TrimSpace(string(out)) } -// podmanMachineStartTimeout bounds a Podman machine boot, which spins up a VM. -var podmanMachineStartTimeout = 90 * time.Second - -var pingTimeout = 30 * time.Second - func runCmdCombined(ctx context.Context, cmd *exec.Cmd) error { var out bytes.Buffer cmd.Stdout = &out @@ -439,6 +440,16 @@ func (r *DockerHelper) StartContainer(ctx context.Context, containerId string) e return nil } +// UnpauseContainer unpauses a paused container. +func (r *DockerHelper) UnpauseContainer(ctx context.Context, containerId string) error { + out, err := r.buildCmd(ctx, "unpause", containerId).CombinedOutput() + if err != nil { + return fmt.Errorf("failed to unpause container: %s: %w", string(out), err) + } + + return nil +} + // 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 { @@ -450,7 +461,7 @@ func (r *DockerHelper) WaitContainerRunning(ctx context.Context, containerID str details, err := r.InspectContainers(ctx, []string{containerID}) if err != nil { lastErr = err - log.Debugf("WaitContainerRunning: inspect error (will retry): %v", err) + log.Debugf("inspecting container %s: %v", containerID, err) return false, nil } lastErr = nil @@ -458,7 +469,12 @@ func (r *DockerHelper) WaitContainerRunning(ctx context.Context, containerID str }, ) if pollErr != nil && lastErr != nil { - return fmt.Errorf("%w (last inspect error: %v)", pollErr, lastErr) + return fmt.Errorf( + "waiting for container %s to be running: %w (last inspect error: %v)", + containerID, + pollErr, + lastErr, + ) } return pollErr } diff --git a/pkg/docker/helper_test.go b/pkg/docker/helper_test.go index 940622b33..f4a41c16f 100644 --- a/pkg/docker/helper_test.go +++ b/pkg/docker/helper_test.go @@ -516,3 +516,23 @@ sleep 5 require.Error(t, err) assert.ErrorIs(t, err, context.DeadlineExceeded) } + +func TestWaitContainerRunning_PreservesDeadlineExceededWithInspectError(t *testing.T) { + original := containerRunningTimeout + containerRunningTimeout = 50 * time.Millisecond + t.Cleanup(func() { containerRunningTimeout = original }) + + bin := writeScript(t, t.TempDir(), "docker-fake", `#!/bin/sh +case "$1" in + inspect) echo "Cannot connect to the Docker daemon" >&2; exit 1 ;; +esac +`) + h := &DockerHelper{DockerCommand: bin} + err := h.WaitContainerRunning(context.Background(), "c1") + + require.Error(t, err) + assert.ErrorIs(t, err, context.DeadlineExceeded, + "poll timeout must remain in the error chain even when inspect also fails") + assert.Contains(t, err.Error(), "Cannot connect to the Docker daemon", + "inspect error should be included for diagnostics") +} diff --git a/pkg/driver/docker/build.go b/pkg/driver/docker/build.go index f68703bd4..34366a2b6 100644 --- a/pkg/driver/docker/build.go +++ b/pkg/driver/docker/build.go @@ -415,13 +415,14 @@ func (d *dockerDriver) executeBuild( return nil } +// createBuildInfo constructs the BuildInfo after a successful build. When pushing, +// the image may not be available locally, so ImageDetails may be nil. func (d *dockerDriver) createBuildInfo( ctx context.Context, imageName string, req driver.BuildRequest, buildOptions *build.BuildOptions, ) (*config.BuildInfo, error) { - // When pushing, image may not be available locally var imageDetails *config.ImageDetails if !buildOptions.Push { var err error diff --git a/pkg/driver/docker/lifecycle.go b/pkg/driver/docker/lifecycle.go index 0713733b1..64dcca585 100644 --- a/pkg/driver/docker/lifecycle.go +++ b/pkg/driver/docker/lifecycle.go @@ -15,17 +15,42 @@ import ( "k8s.io/apimachinery/pkg/util/wait" ) -const containerRestartAttempts = 3 +type containerState string + +const ( + containerStatusRunning containerState = "running" + containerStatusExited containerState = "exited" + containerStatusCreated containerState = "created" + containerStatusPaused containerState = "paused" + containerStatusRestarting containerState = "restarting" + containerStatusDead containerState = "dead" + containerStatusRemoving containerState = "removing" +) + +var containerStates = map[string]containerState{ + "running": containerStatusRunning, + "exited": containerStatusExited, + "created": containerStatusCreated, + "paused": containerStatusPaused, + "restarting": containerStatusRestarting, + "dead": containerStatusDead, + "removing": containerStatusRemoving, +} + +func toContainerState(s string) containerState { + if state, ok := containerStates[strings.ToLower(s)]; ok { + return state + } + return containerState(s) +} -const containerStatusRunning = "running" +const ( + containerRestartAttempts = 3 -// snapshotImageLabel marks a committed image as a devsy workspace snapshot, -// so it's identifiable via `docker inspect`/`docker images --filter` by -// anyone who pulls or lists it outside `devsy snapshot` tooling — the -// snapshot manifest (pkg/snapshot) already carries richer sh.devsy.snapshot.* -// metadata, but that lives in a separate OCI artifact a raw image pull won't -// see. -const snapshotImageLabel = "sh.devsy.snapshot=true" + // snapshotImageLabel marks a committed image as a devsy workspace snapshot, + // so it's identifiable via `docker inspect`/`docker images --filter`. + snapshotImageLabel = "sh.devsy.snapshot=true" +) func (d *dockerDriver) CommandDevContainer( ctx context.Context, @@ -58,75 +83,101 @@ func (d *dockerDriver) CommandDevContainer( return nil } -// 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. +// ensureContainerRunning checks the container's state and starts it if necessary. func (d *dockerDriver) ensureContainerRunning( ctx context.Context, container *config.ContainerDetails, ) error { - status := strings.ToLower(container.State.Status) - if status == "dead" || status == "removing" { + status := toContainerState(container.State.Status) + switch status { + case containerStatusRunning: + return nil + case containerStatusDead, containerStatusRemoving: return fmt.Errorf( "%w: container %s is %q", docker.ErrContainerTerminal, container.ID, status, ) + case containerStatusPaused: + return d.unpauseAndWait(ctx, container) + case containerStatusExited, containerStatusCreated, + containerStatusRestarting: + return d.restartAndWait(ctx, container, status) + default: + return fmt.Errorf( + "%w: container %s is in unknown state %q", + docker.ErrContainerTerminal, + container.ID, + status, + ) } - if status == containerStatusRunning { - return nil - } +} +// restartAndWait starts the container and waits for it to be running, +// retrying up to containerRestartAttempts times. It aborts immediately when +// the container enters a terminal state. +func (d *dockerDriver) restartAndWait( + ctx context.Context, + container *config.ContainerDetails, + status containerState, +) error { 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)", + "restarting container %s (status=%s, 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) + if err := d.Docker.StartContainer(ctx, container.ID); err != nil { + lastErr = fmt.Errorf("start container: %w", err) + } else if err := d.Docker.WaitContainerRunning(ctx, container.ID); err != nil { + lastErr = fmt.Errorf("wait for container to be running: %w", err) + } else { + log.Infof("container %s is running", container.ID) return nil } - if errors.Is(err, docker.ErrContainerTerminal) { - return err + if errors.Is(lastErr, docker.ErrContainerTerminal) || + errors.Is(lastErr, context.Canceled) || + errors.Is(lastErr, context.DeadlineExceeded) { + return lastErr } - lastErr = err - log.Debugf("container %s restart attempt %d failed: %v", container.ID, attempt, err) + log.Debugf("container %s restart attempt %d failed: %v", container.ID, attempt, lastErr) } return fmt.Errorf( - "%w: container %s did not stay running after %d restart attempts: %v", + "%w: container %s did not stay running after %d attempts: %w", docker.ErrContainerTerminal, container.ID, containerRestartAttempts, lastErr, ) } -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) +// unpauseAndWait unpauses a paused container and waits for it to be running. +func (d *dockerDriver) unpauseAndWait( + ctx context.Context, + container *config.ContainerDetails, +) error { + log.Infof("unpausing container %s", container.ID) + if err := d.Docker.UnpauseContainer(ctx, container.ID); err != nil { + return fmt.Errorf("unpause container: %w", err) } - if err := d.Docker.WaitContainerRunning(ctx, containerID); err != nil { + if err := d.Docker.WaitContainerRunning(ctx, container.ID); err != nil { return fmt.Errorf("wait for container to be running: %w", err) } + log.Infof("container %s is running", container.ID) return nil } func (d *dockerDriver) PushDevContainer(ctx context.Context, image string) error { - // push image writer := log.Writer(log.LevelInfo) defer func() { _ = writer.Close() }() - // build args args := []string{ "push", image, } - // run command log.Debugf( "running docker push command: command=%s, args=%s", d.Docker.DockerCommand, @@ -141,18 +192,15 @@ func (d *dockerDriver) PushDevContainer(ctx context.Context, image string) error } func (d *dockerDriver) TagDevContainer(ctx context.Context, image, tag string) error { - // Tag image writer := log.Writer(log.LevelInfo) defer func() { _ = writer.Close() }() - // build args args := []string{ "tag", image, tag, } - // run command log.Debugf( "running docker tag command: command=%s, args=%s", d.Docker.DockerCommand, @@ -201,7 +249,7 @@ func (d *dockerDriver) DeleteDevContainer(ctx context.Context, workspaceId strin return nil } - if strings.ToLower(container.State.Status) == containerStatusRunning { + if status := toContainerState(container.State.Status); status == containerStatusRunning { if err := d.Docker.Stop(ctx, container.ID); err != nil { log.Warnf("stop before delete failed for %s: %v", container.ID, err) } diff --git a/pkg/driver/docker/lifecycle_test.go b/pkg/driver/docker/lifecycle_test.go index ecacf8599..d37b9a490 100644 --- a/pkg/driver/docker/lifecycle_test.go +++ b/pkg/driver/docker/lifecycle_test.go @@ -203,7 +203,7 @@ func TestEnsureContainerRunning_AlreadyRunning(t *testing.T) { d := &dockerDriver{Docker: &docker.DockerHelper{DockerCommand: testDockerCmd}} container := &config.ContainerDetails{ ID: "c1", - State: config.ContainerDetailsState{Status: containerStatusRunning}, + State: config.ContainerDetailsState{Status: string(containerStatusRunning)}, } require.NoError(t, d.ensureContainerRunning(context.Background(), container)) @@ -358,3 +358,54 @@ esac assert.Contains(t, string(logged), "stop\n") assert.Contains(t, string(logged), "rm\n") } + +func TestEnsureContainerRunning_PausedUnpausesNotRestarts(t *testing.T) { + dir := t.TempDir() + calls := filepath.Join(dir, "calls") + script := `#!/bin/sh +echo "$1" >> "` + calls + `" +case "$1" in + inspect) + echo '[{"ID":"c1","State":{"Status":"running"}}]' + ;; + unpause) + ;; + start) + echo "start should not be called on paused container" >&2 + exit 1 + ;; +esac +` + bin := filepath.Join(dir, "docker-fake") + require.NoError(t, os.WriteFile(bin, []byte(script), 0o755)) //nolint:gosec + + d := &dockerDriver{Docker: &docker.DockerHelper{DockerCommand: bin}} + container := &config.ContainerDetails{ + ID: "c1", + State: config.ContainerDetailsState{Status: "paused"}, + } + + require.NoError(t, d.ensureContainerRunning(context.Background(), container)) + + logged, readErr := os.ReadFile(calls) //nolint:gosec // test-controlled path + require.NoError(t, readErr) + assert.Contains(t, string(logged), "unpause\n") + assert.NotContains( + t, + string(logged), + "start\n", + "paused containers should be unpaused, not started", + ) +} + +func TestEnsureContainerRunning_UnknownStateIsTerminal(t *testing.T) { + d := &dockerDriver{Docker: &docker.DockerHelper{DockerCommand: testDockerCmd}} + container := &config.ContainerDetails{ + ID: "c1", + State: config.ContainerDetailsState{Status: "unknown"}, + } + + err := d.ensureContainerRunning(context.Background(), container) + require.Error(t, err) + assert.ErrorIs(t, err, docker.ErrContainerTerminal) +} From 8f6de78e7a0830ca274789e65e5d7c4badad0361 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 25 Aug 2026 04:18:46 +0000 Subject: [PATCH 2/5] refactor: unify container status handling Type ContainerDetailsState.Status as config.ContainerStatus and normalize it in UnmarshalJSON, so every runtime's inspect output (docker, podman, apple, microsandbox, kubernetes) yields canonical lowercase values at the boundary. Replace all per-package status vocabularies and defensive ToLower/EqualFold wrappers with direct comparisons against the shared constants; fixes a latent case-sensitive "removing" check in compose helper. --- pkg/apple/helper.go | 10 ++--- pkg/apple/integration_test.go | 4 +- pkg/apple/types.go | 13 +++--- pkg/apple/types_test.go | 17 +++++--- pkg/compose/helper.go | 6 +-- pkg/devcontainer/compose.go | 3 +- pkg/devcontainer/config/container_details.go | 46 ++++++++++++++++++-- pkg/devcontainer/delete.go | 5 +-- pkg/devcontainer/single.go | 2 +- pkg/docker/boot_state_test.go | 36 ++++++--------- pkg/docker/helper.go | 34 +++++++-------- pkg/driver/apple/driver.go | 1 - pkg/driver/apple/lifecycle.go | 5 +-- pkg/driver/apple/lifecycle_test.go | 2 +- pkg/driver/docker/lifecycle.go | 45 ++++--------------- pkg/driver/docker/lifecycle_test.go | 2 +- pkg/driver/kubernetes/find.go | 4 +- pkg/driver/microsandbox/microsandbox.go | 4 +- pkg/workspace/exec.go | 5 +-- pkg/workspace/exec_apple.go | 3 +- 20 files changed, 123 insertions(+), 124 deletions(-) diff --git a/pkg/apple/helper.go b/pkg/apple/helper.go index 55cebd0f6..01e434a4e 100644 --- a/pkg/apple/helper.go +++ b/pkg/apple/helper.go @@ -167,10 +167,10 @@ func (h *AppleHelper) WaitContainerRunning(ctx context.Context, id string) error if len(details) == 0 { return false, nil } - switch strings.ToLower(details[0].State.Status) { - case stateRunning: + switch details[0].State.Status { + case config.ContainerStatusRunning: return true, nil - case stateExited: + case config.ContainerStatusExited: return false, fmt.Errorf("container %s exited before reaching running state", id) default: return false, nil @@ -249,7 +249,7 @@ func (h *AppleHelper) FindContainerByID( return nil, err } for i := range details { - if strings.ToLower(details[i].State.Status) != "removing" { + if details[i].State.Status != config.ContainerStatusRemoving { return &details[i], nil } } @@ -357,7 +357,7 @@ func (h *AppleHelper) SystemRunning(ctx context.Context) bool { if err != nil { return false } - return strings.Contains(strings.ToLower(string(out)), stateRunning) + return strings.Contains(strings.ToLower(string(out)), string(config.ContainerStatusRunning)) } func (h *AppleHelper) buildCmd(ctx context.Context, args ...string) *exec.Cmd { diff --git a/pkg/apple/integration_test.go b/pkg/apple/integration_test.go index c8c6ba77b..058daf487 100644 --- a/pkg/apple/integration_test.go +++ b/pkg/apple/integration_test.go @@ -83,8 +83,8 @@ func (p *probe) findRunning(label string) *config.ContainerDetails { if found == nil { p.t.Fatal("FindDevContainer returned nil for a running labelled container") } - if found.State.Status != stateRunning { - p.t.Errorf("found.State.Status = %q, want %q", found.State.Status, stateRunning) + if found.State.Status != config.ContainerStatusRunning { + p.t.Errorf("found.State.Status = %q, want %q", found.State.Status, config.ContainerStatusRunning) } if found.Config.Labels["devsy.e2e"] != "1" { p.t.Errorf("label devsy.e2e = %q, want 1", found.Config.Labels["devsy.e2e"]) diff --git a/pkg/apple/types.go b/pkg/apple/types.go index 367eeff5f..3d0e0d6d2 100644 --- a/pkg/apple/types.go +++ b/pkg/apple/types.go @@ -9,8 +9,6 @@ import ( ) const ( - stateRunning = "running" - stateExited = "exited" mountTypeBind = "bind" archUnknown = "unknown" // placeholder arch in Apple's multi-arch image index ) @@ -130,12 +128,13 @@ func (c containerInspect) toContainerDetails() config.ContainerDetails { } } -func normalizeState(state string) string { - s := strings.ToLower(strings.TrimSpace(state)) +// normalizeState maps the runner's status vocabulary onto the shared Docker +// vocabulary: "stopped" means the container ran and exited, matching what +// terminal-state checks expect. +func normalizeState(state string) config.ContainerStatus { + s := config.ToContainerStatus(strings.TrimSpace(state)) if s == "stopped" { - // Docker uses "exited" for a container that ran and stopped; the - // runner's terminal-state checks key off that vocabulary. - return stateExited + return config.ContainerStatusExited } return s } diff --git a/pkg/apple/types_test.go b/pkg/apple/types_test.go index 57ac6ed99..8f273d9b2 100644 --- a/pkg/apple/types_test.go +++ b/pkg/apple/types_test.go @@ -3,6 +3,8 @@ package apple import ( "encoding/json" "testing" + + "github.com/devsy-org/devsy/pkg/devcontainer/config" ) // containerInspectFixture is verbatim output from `container inspect` on @@ -46,7 +48,7 @@ func TestContainerInspectMapping(t *testing.T) { name, got, want string }{ {"ID", got.ID, "devsy-probe"}, - {"State.Status", got.State.Status, stateRunning}, + {"State.Status", string(got.State.Status), string(config.ContainerStatusRunning)}, {"StartedAt", got.State.StartedAt, "2026-07-25T01:57:25Z"}, {"WorkingDir", got.Config.WorkingDir, "/"}, {"User", got.Config.User, "0"}, @@ -69,11 +71,14 @@ func TestContainerInspectMapping(t *testing.T) { } func TestNormalizeState(t *testing.T) { - cases := []struct{ in, want string }{ - {"running", stateRunning}, - {"Running", stateRunning}, - {"stopped", stateExited}, - {"Stopped", stateExited}, + cases := []struct { + in string + want config.ContainerStatus + }{ + {"running", config.ContainerStatusRunning}, + {"Running", config.ContainerStatusRunning}, + {"stopped", config.ContainerStatusExited}, + {"Stopped", config.ContainerStatusExited}, } for _, c := range cases { if got := normalizeState(c.in); got != c.want { diff --git a/pkg/compose/helper.go b/pkg/compose/helper.go index aed737561..ff97305ec 100644 --- a/pkg/compose/helper.go +++ b/pkg/compose/helper.go @@ -217,9 +217,9 @@ func (h *ComposeHelper) FindDevContainer( return nil, err } - for _, details := range containerDetails { - if details.State.Status != "removing" { - return &details, nil + for i := range containerDetails { + if containerDetails[i].State.Status != config.ContainerStatusRemoving { + return &containerDetails[i], nil } } diff --git a/pkg/devcontainer/compose.go b/pkg/devcontainer/compose.go index f19179803..700f49e0a 100644 --- a/pkg/devcontainer/compose.go +++ b/pkg/devcontainer/compose.go @@ -24,7 +24,6 @@ const ( FeaturesBuildOverrideFilePrefix = "docker-compose.devcontainer.build" FeaturesStartOverrideFilePrefix = "docker-compose.devcontainer.containerFeatures" - containerStatusRunning = "running" composeProjectNameFlag = "--project-name" ) @@ -328,7 +327,7 @@ func (r *runner) ensureComposeContainer( } // container already exists and is running, nothing to do - if containerDetails != nil && containerDetails.State.Status == containerStatusRunning && + if containerDetails != nil && containerDetails.State.Status == config.ContainerStatusRunning && !options.Recreate { return containerDetails, nil } diff --git a/pkg/devcontainer/config/container_details.go b/pkg/devcontainer/config/container_details.go index 8f018a8d1..a032236ad 100644 --- a/pkg/devcontainer/config/container_details.go +++ b/pkg/devcontainer/config/container_details.go @@ -1,5 +1,10 @@ package config +import ( + "encoding/json" + "strings" +) + type ImageDetails struct { ID string Config ImageDetailsConfig @@ -43,8 +48,41 @@ type ContainerDetailsConfig struct { } type ContainerDetailsState struct { - Status string `json:"Status,omitempty"` - StartedAt string `json:"StartedAt,omitempty"` - ExitCode int `json:"ExitCode,omitempty"` - Error string `json:"Error,omitempty"` + Status ContainerStatus `json:"Status,omitempty"` + StartedAt string `json:"StartedAt,omitempty"` + ExitCode int `json:"ExitCode,omitempty"` + Error string `json:"Error,omitempty"` +} + +// UnmarshalJSON decodes inspect output and normalizes Status, so the field is +// always canonical regardless of the runtime's casing. +func (s *ContainerDetailsState) UnmarshalJSON(data []byte) error { + type alias ContainerDetailsState + var raw alias + if err := json.Unmarshal(data, &raw); err != nil { + return err + } + raw.Status = ToContainerStatus(string(raw.Status)) + *s = ContainerDetailsState(raw) + return nil +} + +// ContainerStatus is a normalized container state string (`State.Status` from +// `docker inspect` and friends), comparable against the ContainerStatus* +// constants. +type ContainerStatus string + +const ( + ContainerStatusRunning ContainerStatus = "running" + ContainerStatusExited ContainerStatus = "exited" + ContainerStatusCreated ContainerStatus = "created" + ContainerStatusPaused ContainerStatus = "paused" + ContainerStatusRestarting ContainerStatus = "restarting" + ContainerStatusDead ContainerStatus = "dead" + ContainerStatusRemoving ContainerStatus = "removing" +) + +// ToContainerStatus normalizes a raw status string for comparison. +func ToContainerStatus(s string) ContainerStatus { + return ContainerStatus(strings.ToLower(s)) } diff --git a/pkg/devcontainer/delete.go b/pkg/devcontainer/delete.go index 8c5ab9993..a5954579e 100644 --- a/pkg/devcontainer/delete.go +++ b/pkg/devcontainer/delete.go @@ -3,7 +3,6 @@ package devcontainer import ( "context" "fmt" - "strings" "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/log" @@ -32,7 +31,7 @@ func (r *runner) Delete(ctx context.Context, options DeleteOptions) error { func (r *runner) stopAndDeleteContainer( ctx context.Context, containerDetails *config.ContainerDetails, ) error { - if strings.ToLower(containerDetails.State.Status) == "running" { + if containerDetails.State.Status == config.ContainerStatusRunning { if err := r.driver.StopDevContainer(ctx, r.id); err != nil { return err } @@ -65,7 +64,7 @@ func (r *runner) Stop(ctx context.Context) error { return nil } - if strings.ToLower(containerDetails.State.Status) != "running" { + if containerDetails.State.Status != config.ContainerStatusRunning { return nil } diff --git a/pkg/devcontainer/single.go b/pkg/devcontainer/single.go index d03a81354..b4a79e50f 100644 --- a/pkg/devcontainer/single.go +++ b/pkg/devcontainer/single.go @@ -216,7 +216,7 @@ func (r *runner) ensureRunning( ctx context.Context, containerDetails *config.ContainerDetails, ) (*config.ContainerDetails, error) { - if strings.ToLower(containerDetails.State.Status) == containerStatusRunning { + if containerDetails.State.Status == config.ContainerStatusRunning { return containerDetails, nil } diff --git a/pkg/docker/boot_state_test.go b/pkg/docker/boot_state_test.go index 587f6377d..b68a1d662 100644 --- a/pkg/docker/boot_state_test.go +++ b/pkg/docker/boot_state_test.go @@ -3,48 +3,40 @@ package docker import ( "testing" + "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/stretchr/testify/assert" ) -const ( - statusDead = "dead" - statusRemoving = "removing" - statusExited = "exited" - statusCreated = "created" - statusPaused = "paused" - statusRestarting = "restarting" -) - func TestFailedBootSentinel(t *testing.T) { tests := []struct { name string - status string + status config.ContainerStatus graceElapsed bool wantSentinel error wantNil bool }{ - {"dead is terminal regardless of grace", statusDead, false, ErrContainerTerminal, false}, - {"dead is terminal even after grace", statusDead, true, ErrContainerTerminal, false}, + {"dead is terminal regardless of grace", config.ContainerStatusDead, false, ErrContainerTerminal, false}, + {"dead is terminal even after grace", config.ContainerStatusDead, true, ErrContainerTerminal, false}, { "removing is terminal regardless of grace", - statusRemoving, + config.ContainerStatusRemoving, false, ErrContainerTerminal, false, }, { "removing is terminal even after grace", - statusRemoving, + config.ContainerStatusRemoving, true, ErrContainerTerminal, false, }, - {"exited before grace is still booting", statusExited, false, nil, true}, - {"exited after grace failed", statusExited, true, ErrContainerExited, false}, - {"created before grace is still booting", statusCreated, false, nil, true}, - {"created after grace failed", statusCreated, true, ErrContainerExited, false}, - {"paused is not a terminal boot state", statusPaused, true, nil, true}, - {"restarting is not a terminal boot state", statusRestarting, false, nil, true}, + {"exited before grace is still booting", config.ContainerStatusExited, false, nil, true}, + {"exited after grace failed", config.ContainerStatusExited, true, ErrContainerExited, false}, + {"created before grace is still booting", config.ContainerStatusCreated, false, nil, true}, + {"created after grace failed", config.ContainerStatusCreated, true, ErrContainerExited, false}, + {"paused is not a terminal boot state", config.ContainerStatusPaused, true, nil, true}, + {"restarting is not a terminal boot state", config.ContainerStatusRestarting, false, nil, true}, {"empty status is not a terminal boot state", "", true, nil, true}, } @@ -69,8 +61,8 @@ func TestFailedBootSentinel(t *testing.T) { } func TestFailedBootSentinel_TerminalNotConfusedWithExited(t *testing.T) { - terminal := failedBootSentinel(statusRemoving, false) - exited := failedBootSentinel(statusExited, true) + terminal := failedBootSentinel(config.ContainerStatusRemoving, false) + exited := failedBootSentinel(config.ContainerStatusExited, true) assert.ErrorIs(t, terminal, ErrContainerTerminal) assert.NotErrorIs(t, terminal, ErrContainerExited, diff --git a/pkg/docker/helper.go b/pkg/docker/helper.go index e6e451d1d..6d0cfe7ae 100644 --- a/pkg/docker/helper.go +++ b/pkg/docker/helper.go @@ -36,12 +36,6 @@ var ( 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") - ErrImageNotFound = errors.New("image not found") // podmanMachineStartTimeout is the maximum time to wait for a Podman machine to start. podmanMachineStartTimeout = 90 * time.Second @@ -50,6 +44,13 @@ var ( pingTimeout = 30 * time.Second ) +// Sentinels for container lifecycle failures; match with errors.Is. +var ( + ErrContainerTerminal = errors.New("container in terminal state") + ErrContainerExited = errors.New("container exited after start") + ErrImageNotFound = errors.New("image not found") +) + var imageNotFoundMarkers = []string{ "no such image", "no such object", @@ -280,11 +281,10 @@ func (r *DockerHelper) FindContainerByID( return nil, err } - // find matching container - for _, details := range containerDetails { - if strings.ToLower(details.State.Status) != "removing" { - details.State.Status = strings.ToLower(details.State.Status) - return &details, nil + // find matching container, skipping containers already being removed + for i := range containerDetails { + if containerDetails[i].State.Status != config.ContainerStatusRemoving { + return &containerDetails[i], nil } } @@ -687,7 +687,7 @@ func (r *DockerHelper) containerStateError( "%w: container %s is %q (%s)", sentinel, containerID, - strings.ToLower(state.Status), + state.Status, detail, ) } @@ -722,8 +722,8 @@ func (r *DockerHelper) evaluateContainerState( ) } state := details[0].State - status := strings.ToLower(state.Status) - if status == "running" { + status := state.Status + if status == config.ContainerStatusRunning { return true, nil } if sentinel := failedBootSentinel(status, elapsed > containerExitGrace); sentinel != nil { @@ -735,11 +735,11 @@ func (r *DockerHelper) evaluateContainerState( // 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 { +func failedBootSentinel(status config.ContainerStatus, graceElapsed bool) error { switch status { - case "dead", "removing": + case config.ContainerStatusDead, config.ContainerStatusRemoving: return ErrContainerTerminal - case "exited", "created": + case config.ContainerStatusExited, config.ContainerStatusCreated: if graceElapsed { return ErrContainerExited } diff --git a/pkg/driver/apple/driver.go b/pkg/driver/apple/driver.go index 426b7e1a2..caa07a7f4 100644 --- a/pkg/driver/apple/driver.go +++ b/pkg/driver/apple/driver.go @@ -22,7 +22,6 @@ import ( const ( appleExec = "exec" defaultCommand = "container" - statusRunning = "running" ) type appleDriver struct { diff --git a/pkg/driver/apple/lifecycle.go b/pkg/driver/apple/lifecycle.go index 216f764da..459bcd590 100644 --- a/pkg/driver/apple/lifecycle.go +++ b/pkg/driver/apple/lifecycle.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "io" - "strings" "github.com/devsy-org/devsy/pkg/apple" "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -44,7 +43,7 @@ func (d *appleDriver) ensureContainerRunning( ctx context.Context, container *config.ContainerDetails, ) error { - if strings.ToLower(container.State.Status) == statusRunning { + if container.State.Status == config.ContainerStatusRunning { return nil } if err := d.Apple.StartContainer(ctx, container.ID); err != nil { @@ -83,7 +82,7 @@ func (d *appleDriver) DeleteDevContainer(ctx context.Context, workspaceID string // Apple's `delete` requires a stopped container. `container stop` is synchronous; // a stop failure is logged but delete is still attempted (it fails loudly if the // container is genuinely still running). - if strings.ToLower(container.State.Status) == statusRunning { + if container.State.Status == config.ContainerStatusRunning { if err := d.Apple.Stop(ctx, container.ID); err != nil { log.Warnf("stop before delete failed for %s: %v", container.ID, err) } diff --git a/pkg/driver/apple/lifecycle_test.go b/pkg/driver/apple/lifecycle_test.go index d70e6614f..27d25ac15 100644 --- a/pkg/driver/apple/lifecycle_test.go +++ b/pkg/driver/apple/lifecycle_test.go @@ -103,7 +103,7 @@ func (m *mockClient) GetContainerLogs(context.Context, string, io.Writer, io.Wri func running(id string) *config.ContainerDetails { return &config.ContainerDetails{ ID: id, - State: config.ContainerDetailsState{Status: statusRunning}, + State: config.ContainerDetailsState{Status: config.ContainerStatusRunning}, } } diff --git a/pkg/driver/docker/lifecycle.go b/pkg/driver/docker/lifecycle.go index 64dcca585..77e8fdda6 100644 --- a/pkg/driver/docker/lifecycle.go +++ b/pkg/driver/docker/lifecycle.go @@ -15,35 +15,6 @@ import ( "k8s.io/apimachinery/pkg/util/wait" ) -type containerState string - -const ( - containerStatusRunning containerState = "running" - containerStatusExited containerState = "exited" - containerStatusCreated containerState = "created" - containerStatusPaused containerState = "paused" - containerStatusRestarting containerState = "restarting" - containerStatusDead containerState = "dead" - containerStatusRemoving containerState = "removing" -) - -var containerStates = map[string]containerState{ - "running": containerStatusRunning, - "exited": containerStatusExited, - "created": containerStatusCreated, - "paused": containerStatusPaused, - "restarting": containerStatusRestarting, - "dead": containerStatusDead, - "removing": containerStatusRemoving, -} - -func toContainerState(s string) containerState { - if state, ok := containerStates[strings.ToLower(s)]; ok { - return state - } - return containerState(s) -} - const ( containerRestartAttempts = 3 @@ -88,21 +59,21 @@ func (d *dockerDriver) ensureContainerRunning( ctx context.Context, container *config.ContainerDetails, ) error { - status := toContainerState(container.State.Status) + status := container.State.Status switch status { - case containerStatusRunning: + case config.ContainerStatusRunning: return nil - case containerStatusDead, containerStatusRemoving: + case config.ContainerStatusDead, config.ContainerStatusRemoving: return fmt.Errorf( "%w: container %s is %q", docker.ErrContainerTerminal, container.ID, status, ) - case containerStatusPaused: + case config.ContainerStatusPaused: return d.unpauseAndWait(ctx, container) - case containerStatusExited, containerStatusCreated, - containerStatusRestarting: + case config.ContainerStatusExited, config.ContainerStatusCreated, + config.ContainerStatusRestarting: return d.restartAndWait(ctx, container, status) default: return fmt.Errorf( @@ -120,7 +91,7 @@ func (d *dockerDriver) ensureContainerRunning( func (d *dockerDriver) restartAndWait( ctx context.Context, container *config.ContainerDetails, - status containerState, + status config.ContainerStatus, ) error { var lastErr error for attempt := 1; attempt <= containerRestartAttempts; attempt++ { @@ -249,7 +220,7 @@ func (d *dockerDriver) DeleteDevContainer(ctx context.Context, workspaceId strin return nil } - if status := toContainerState(container.State.Status); status == containerStatusRunning { + if container.State.Status == config.ContainerStatusRunning { if err := d.Docker.Stop(ctx, container.ID); err != nil { log.Warnf("stop before delete failed for %s: %v", container.ID, err) } diff --git a/pkg/driver/docker/lifecycle_test.go b/pkg/driver/docker/lifecycle_test.go index d37b9a490..05fe6d71c 100644 --- a/pkg/driver/docker/lifecycle_test.go +++ b/pkg/driver/docker/lifecycle_test.go @@ -203,7 +203,7 @@ func TestEnsureContainerRunning_AlreadyRunning(t *testing.T) { d := &dockerDriver{Docker: &docker.DockerHelper{DockerCommand: testDockerCmd}} container := &config.ContainerDetails{ ID: "c1", - State: config.ContainerDetailsState{Status: string(containerStatusRunning)}, + State: config.ContainerDetailsState{Status: config.ContainerStatusRunning}, } require.NoError(t, d.ensureContainerRunning(context.Background(), container)) diff --git a/pkg/driver/kubernetes/find.go b/pkg/driver/kubernetes/find.go index 23aeea2e2..b91b36789 100644 --- a/pkg/driver/kubernetes/find.go +++ b/pkg/driver/kubernetes/find.go @@ -36,9 +36,9 @@ func (k *KubernetesDriver) FindDevContainer( } // determine status - status := "exited" + status := config.ContainerStatusExited if pod != nil && isPodRunning(pod) { - status = "running" + status = config.ContainerStatusRunning } // check started diff --git a/pkg/driver/microsandbox/microsandbox.go b/pkg/driver/microsandbox/microsandbox.go index 5c5d47b04..100cef53d 100644 --- a/pkg/driver/microsandbox/microsandbox.go +++ b/pkg/driver/microsandbox/microsandbox.go @@ -465,9 +465,9 @@ func hasUserNSMapping(options *driver.RunOptions) bool { } func toContainerDetails(info *sandboxInfo) *config.ContainerDetails { - status := "exited" + status := config.ContainerStatusExited if info.Running { - status = "running" + status = config.ContainerStatusRunning } return &config.ContainerDetails{ ID: info.Name, diff --git a/pkg/workspace/exec.go b/pkg/workspace/exec.go index 2c07ef6ab..18fb5e0ad 100644 --- a/pkg/workspace/exec.go +++ b/pkg/workspace/exec.go @@ -23,8 +23,7 @@ import ( ) const ( - DefaultDockerCommand = "docker" - ContainerStatusRunning = "running" + DefaultDockerCommand = "docker" ) // defaultExecTimeoutSeconds bounds an exec when no caller or configured default @@ -283,7 +282,7 @@ func (r *DockerRuntime) FindRunning( ) } - if !strings.EqualFold(container.State.Status, ContainerStatusRunning) { + if container.State.Status != devcconfig.ContainerStatusRunning { return nil, fmt.Errorf( "container %s is not running (status: %s)", container.ID, diff --git a/pkg/workspace/exec_apple.go b/pkg/workspace/exec_apple.go index 676c4e9bc..8f6619e37 100644 --- a/pkg/workspace/exec_apple.go +++ b/pkg/workspace/exec_apple.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "io" - "strings" "github.com/devsy-org/devsy/pkg/apple" devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -116,7 +115,7 @@ func (r *AppleRuntime) FindRunning( if container == nil { return nil, fmt.Errorf("no running container found for workspace %q", workspaceID) } - if !strings.EqualFold(container.State.Status, ContainerStatusRunning) { + if container.State.Status != devcconfig.ContainerStatusRunning { return nil, fmt.Errorf( "container %s is not running (status: %s)", container.ID, container.State.Status, From 148a0177f1d1d4c379b7c002f2a5184623998abd Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 25 Aug 2026 04:32:30 +0000 Subject: [PATCH 3/5] fix: migrate cmd status checks to ContainerStatus The typed State.Status field surfaced remaining raw comparisons in cmd/: agentworkspace status, pro start (including its local inspect types), container_tunnel, runusercommands, workspace exec, and config apply now compare directly against the shared config.ContainerStatus constants. Drops the deleted workspace.ContainerStatusRunning references and pro's duplicate string status. --- cmd/config/apply.go | 2 +- cmd/internal/agentworkspace/status.go | 7 ++++--- cmd/internal/container_tunnel.go | 5 ++--- cmd/internal/container_tunnel_test.go | 4 ++-- cmd/internal/runusercommands.go | 2 +- cmd/pro/start.go | 11 ++++++----- cmd/workspace/exec.go | 2 +- 7 files changed, 17 insertions(+), 16 deletions(-) diff --git a/cmd/config/apply.go b/cmd/config/apply.go index 5db02fc0e..43b3fa8e0 100644 --- a/cmd/config/apply.go +++ b/cmd/config/apply.go @@ -159,7 +159,7 @@ func (cmd *ApplyCmd) inspectRunningContainer( } containerDetails := &details[0] - if !strings.EqualFold(containerDetails.State.Status, pkgworkspace.ContainerStatusRunning) { + if containerDetails.State.Status != devcconfig.ContainerStatusRunning { return nil, fmt.Errorf( "container %s is not running (status: %s)", cmd.Container, diff --git a/cmd/internal/agentworkspace/status.go b/cmd/internal/agentworkspace/status.go index 48e6450a6..99584b458 100644 --- a/cmd/internal/agentworkspace/status.go +++ b/cmd/internal/agentworkspace/status.go @@ -3,11 +3,11 @@ package agentworkspace import ( "context" "fmt" - "strings" "github.com/devsy-org/devsy/cmd/flags" "github.com/devsy-org/devsy/pkg/agent" "github.com/devsy-org/devsy/pkg/client" + config2 "github.com/devsy-org/devsy/pkg/devcontainer/config" cliflags "github.com/devsy-org/devsy/pkg/flags" "github.com/devsy-org/devsy/pkg/flags/names" "github.com/spf13/cobra" @@ -66,10 +66,11 @@ func (cmd *StatusCmd) Run(ctx context.Context) error { } // is running? - if strings.ToLower(containerDetails.State.Status) == "running" { + switch containerDetails.State.Status { + case config2.ContainerStatusRunning: fmt.Print(client.StatusRunning) return nil - } else if strings.ToLower(containerDetails.State.Status) == "exited" { + case config2.ContainerStatusExited: fmt.Print(client.StatusStopped) return nil } diff --git a/cmd/internal/container_tunnel.go b/cmd/internal/container_tunnel.go index 075d6d618..6e63d8dce 100644 --- a/cmd/internal/container_tunnel.go +++ b/cmd/internal/container_tunnel.go @@ -15,6 +15,7 @@ import ( "github.com/devsy-org/devsy/pkg/agent" pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer" + "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/encoding" cliflags "github.com/devsy-org/devsy/pkg/flags" "github.com/devsy-org/devsy/pkg/flags/names" @@ -24,8 +25,6 @@ import ( "github.com/spf13/cobra" ) -const containerStatusRunning = "running" - // containerRootUser is the user to use when running commands inside the container that // require root privileges. const containerRootUser = "root" @@ -166,7 +165,7 @@ func startDevContainer( return fmt.Errorf("find devcontainer: %w", err) } - if containerDetails == nil || containerDetails.State.Status != containerStatusRunning { + if containerDetails == nil || containerDetails.State.Status != config.ContainerStatusRunning { if err := startContainer(ctx, runner, workspaceConfig); err != nil { return fmt.Errorf("start container: %w", err) } diff --git a/cmd/internal/container_tunnel_test.go b/cmd/internal/container_tunnel_test.go index 3fa9259ae..60b5641f9 100644 --- a/cmd/internal/container_tunnel_test.go +++ b/cmd/internal/container_tunnel_test.go @@ -124,7 +124,7 @@ func TestStartDevContainer_RunningContainerWithoutResultRestarts(t *testing.T) { upErr := errors.New("boom") runner := stubRunner{ findResult: &config.ContainerDetails{ - State: config.ContainerDetailsState{Status: containerStatusRunning}, + State: config.ContainerDetailsState{Status: config.ContainerStatusRunning}, }, commandErr: errors.New("cat: no such file"), // hasDevContainerResult -> false upErr: upErr, @@ -147,7 +147,7 @@ func TestStartDevContainer_RunningContainerWithoutResultRestarts(t *testing.T) { func TestStartDevContainer_RunningContainerWithResultIsNoOp(t *testing.T) { runner := stubRunner{ findResult: &config.ContainerDetails{ - State: config.ContainerDetailsState{Status: containerStatusRunning}, + State: config.ContainerDetailsState{Status: config.ContainerStatusRunning}, }, commandErr: nil, // hasDevContainerResult -> true upErr: errors.New("Up must not be called on the happy path"), diff --git a/cmd/internal/runusercommands.go b/cmd/internal/runusercommands.go index 5113a206e..373208140 100644 --- a/cmd/internal/runusercommands.go +++ b/cmd/internal/runusercommands.go @@ -299,7 +299,7 @@ func (cmd *RunUserCommandsCmd) inspectRunningContainer( } containerDetails := &details[0] - if !strings.EqualFold(containerDetails.State.Status, workspace2.ContainerStatusRunning) { + if containerDetails.State.Status != devcconfig.ContainerStatusRunning { errMsg := fmt.Sprintf( "container %s is not running (status: %s)", cmd.ContainerID, diff --git a/cmd/pro/start.go b/cmd/pro/start.go index c874b2d1b..c794fbbd8 100644 --- a/cmd/pro/start.go +++ b/cmd/pro/start.go @@ -24,6 +24,7 @@ import ( loftclientset "github.com/devsy-org/api/pkg/clientset/versioned" proflags "github.com/devsy-org/devsy/cmd/pro/flags" "github.com/devsy-org/devsy/pkg/config" + devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/hash" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/machineid" @@ -665,8 +666,8 @@ func (cmd *StartCmd) successDocker(ctx context.Context, containerID string) erro containerDetails, err := cmd.inspectContainer(ctx, containerID) if err != nil { return false, fmt.Errorf("inspect loft container: %w", err) - } else if strings.ToLower(containerDetails.State.Status) == "exited" || - strings.ToLower(containerDetails.State.Status) == "dead" { + } else if containerDetails.State.Status == devcconfig.ContainerStatusExited || + containerDetails.State.Status == devcconfig.ContainerStatusDead { logs, _ := cmd.logsContainer(ctx, containerID) return false, fmt.Errorf( "container failed (status: %s):\n %s", @@ -919,7 +920,7 @@ func (cmd *StartCmd) resolveRunningContainer( switch { case err != nil: return "", err - case onlyRunning && strings.ToLower(containerState.State.Status) != "running": + case onlyRunning && containerState.State.Status != devcconfig.ContainerStatusRunning: err = cmd.removeContainer(ctx, containerID) if err != nil { return "", err @@ -2144,8 +2145,8 @@ type ContainerDetailsConfig struct { } type ContainerDetailsState struct { - Status string `json:"Status,omitempty"` - StartedAt string `json:"StartedAt,omitempty"` + Status devcconfig.ContainerStatus `json:"Status,omitempty"` + StartedAt string `json:"StartedAt,omitempty"` } func WrapCommandError(stdout []byte, err error) error { diff --git a/cmd/workspace/exec.go b/cmd/workspace/exec.go index a26084eb3..383b61d8e 100644 --- a/cmd/workspace/exec.go +++ b/cmd/workspace/exec.go @@ -245,7 +245,7 @@ func (cmd *ExecCmd) runWithContainerID(ctx context.Context, args []string) error } containerDetails := &details[0] - if !strings.EqualFold(containerDetails.State.Status, workspace2.ContainerStatusRunning) { + if containerDetails.State.Status != devcconfig.ContainerStatusRunning { return fmt.Errorf( "container %s is not running (status: %s)", cmd.ContainerID, From 13dd7134c3a6f5cc8c12ea5fe3e4a2a3b6d35900 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 25 Aug 2026 05:13:38 +0000 Subject: [PATCH 4/5] style: apply golines to test tables golines (enforced via golangci-lint-fmt) requires the longer ContainerStatus table rows in boot_state_test.go and the integration_test error message to be split across lines. --- pkg/apple/integration_test.go | 6 +++++- pkg/docker/boot_state_test.go | 40 ++++++++++++++++++++++++++++++----- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/pkg/apple/integration_test.go b/pkg/apple/integration_test.go index 058daf487..e8795f075 100644 --- a/pkg/apple/integration_test.go +++ b/pkg/apple/integration_test.go @@ -84,7 +84,11 @@ func (p *probe) findRunning(label string) *config.ContainerDetails { p.t.Fatal("FindDevContainer returned nil for a running labelled container") } if found.State.Status != config.ContainerStatusRunning { - p.t.Errorf("found.State.Status = %q, want %q", found.State.Status, config.ContainerStatusRunning) + p.t.Errorf( + "found.State.Status = %q, want %q", + found.State.Status, + config.ContainerStatusRunning, + ) } if found.Config.Labels["devsy.e2e"] != "1" { p.t.Errorf("label devsy.e2e = %q, want 1", found.Config.Labels["devsy.e2e"]) diff --git a/pkg/docker/boot_state_test.go b/pkg/docker/boot_state_test.go index b68a1d662..cc965e81f 100644 --- a/pkg/docker/boot_state_test.go +++ b/pkg/docker/boot_state_test.go @@ -15,8 +15,20 @@ func TestFailedBootSentinel(t *testing.T) { wantSentinel error wantNil bool }{ - {"dead is terminal regardless of grace", config.ContainerStatusDead, false, ErrContainerTerminal, false}, - {"dead is terminal even after grace", config.ContainerStatusDead, true, ErrContainerTerminal, false}, + { + "dead is terminal regardless of grace", + config.ContainerStatusDead, + false, + ErrContainerTerminal, + false, + }, + { + "dead is terminal even after grace", + config.ContainerStatusDead, + true, + ErrContainerTerminal, + false, + }, { "removing is terminal regardless of grace", config.ContainerStatusRemoving, @@ -32,11 +44,29 @@ func TestFailedBootSentinel(t *testing.T) { false, }, {"exited before grace is still booting", config.ContainerStatusExited, false, nil, true}, - {"exited after grace failed", config.ContainerStatusExited, true, ErrContainerExited, false}, + { + "exited after grace failed", + config.ContainerStatusExited, + true, + ErrContainerExited, + false, + }, {"created before grace is still booting", config.ContainerStatusCreated, false, nil, true}, - {"created after grace failed", config.ContainerStatusCreated, true, ErrContainerExited, false}, + { + "created after grace failed", + config.ContainerStatusCreated, + true, + ErrContainerExited, + false, + }, {"paused is not a terminal boot state", config.ContainerStatusPaused, true, nil, true}, - {"restarting is not a terminal boot state", config.ContainerStatusRestarting, false, nil, true}, + { + "restarting is not a terminal boot state", + config.ContainerStatusRestarting, + false, + nil, + true, + }, {"empty status is not a terminal boot state", "", true, nil, true}, } From 32c71b90fdf4773b72cbc59760ffbd7403dca9fc Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 25 Aug 2026 15:53:48 +0000 Subject: [PATCH 5/5] fix: wait out restarting containers before start Docker rejects 'docker start' on a container still in the restarting state under a restart policy, burning all retry attempts in restartAndWait. Re-inspect until the container settles: if it reaches running, done; once it lands in exited, take the explicit-start path. Add a regression test asserting restart is never invoked with start. --- pkg/driver/docker/lifecycle.go | 26 +++++++++++++++-- pkg/driver/docker/lifecycle_test.go | 44 +++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/pkg/driver/docker/lifecycle.go b/pkg/driver/docker/lifecycle.go index 77e8fdda6..069c945de 100644 --- a/pkg/driver/docker/lifecycle.go +++ b/pkg/driver/docker/lifecycle.go @@ -72,8 +72,9 @@ func (d *dockerDriver) ensureContainerRunning( ) case config.ContainerStatusPaused: return d.unpauseAndWait(ctx, container) - case config.ContainerStatusExited, config.ContainerStatusCreated, - config.ContainerStatusRestarting: + case config.ContainerStatusRestarting: + return d.waitForRestart(ctx, container) + case config.ContainerStatusExited, config.ContainerStatusCreated: return d.restartAndWait(ctx, container, status) default: return fmt.Errorf( @@ -140,6 +141,27 @@ func (d *dockerDriver) unpauseAndWait( return nil } +// waitForRestart lets the daemon finish an in-flight restart before acting: +// DockerHelper.StartContainer rejects containers still in the "restarting" +// state. Once the container settles, either it is already running or it has +// stopped again (ErrContainerExited) and needs an explicit start. +func (d *dockerDriver) waitForRestart( + ctx context.Context, + container *config.ContainerDetails, +) error { + log.Infof("container %s is restarting, waiting for a stable state", container.ID) + err := d.Docker.WaitContainerRunning(ctx, container.ID) + switch { + case err == nil: + log.Infof("container %s is running", container.ID) + return nil + case errors.Is(err, docker.ErrContainerExited): + return d.restartAndWait(ctx, container, config.ContainerStatusExited) + default: + return err + } +} + func (d *dockerDriver) PushDevContainer(ctx context.Context, image string) error { writer := log.Writer(log.LevelInfo) defer func() { _ = writer.Close() }() diff --git a/pkg/driver/docker/lifecycle_test.go b/pkg/driver/docker/lifecycle_test.go index 05fe6d71c..c8aa4ed60 100644 --- a/pkg/driver/docker/lifecycle_test.go +++ b/pkg/driver/docker/lifecycle_test.go @@ -398,6 +398,50 @@ esac ) } +func TestEnsureContainerRunning_RestartingWaitsNotStarts(t *testing.T) { + dir := t.TempDir() + calls := filepath.Join(dir, "calls") + inspects := filepath.Join(dir, "inspects") + script := `#!/bin/sh +echo "$1" >> "` + calls + `" +case "$1" in + inspect) + n=$(cat "` + inspects + `" 2>/dev/null || echo 0) + n=$((n + 1)) + echo "$n" > "` + inspects + `" + if [ "$n" -ge 2 ]; then + echo '[{"ID":"c1","State":{"Status":"running"}}]' + else + echo '[{"ID":"c1","State":{"Status":"restarting"}}]' + fi + ;; + start) + echo "start should not be called on a restarting container" >&2 + exit 1 + ;; +esac +` + bin := filepath.Join(dir, "docker-fake") + require.NoError(t, os.WriteFile(bin, []byte(script), 0o755)) //nolint:gosec + + d := &dockerDriver{Docker: &docker.DockerHelper{DockerCommand: bin}} + container := &config.ContainerDetails{ + ID: "c1", + State: config.ContainerDetailsState{Status: config.ContainerStatusRestarting}, + } + + require.NoError(t, d.ensureContainerRunning(context.Background(), container)) + + logged, readErr := os.ReadFile(calls) //nolint:gosec // test-controlled path + require.NoError(t, readErr) + assert.NotContains( + t, + string(logged), + "start\n", + "restarting containers should be waited out, not started", + ) +} + func TestEnsureContainerRunning_UnknownStateIsTerminal(t *testing.T) { d := &dockerDriver{Docker: &docker.DockerHelper{DockerCommand: testDockerCmd}} container := &config.ContainerDetails{