fix(docker): handling for exited containers that fail to start

[skevetter]

Signed-off-by: Samuel K skevetter@pm.me

Summary by CodeRabbit

• New Features

  • Added clearer handling for container execution results, including exit codes and error details.
  • Improved container recovery by retrying restarts when a container is not yet running.

• Bug Fixes

  • Containers that exit or fail now surface errors more quickly with more useful diagnostic information.
  • Terminal container states are detected reliably and stop further restart attempts.

• Tests

  • Added coverage for running, exited, dead, and retry-recovery scenarios.

[netlify]

✅ Deploy Preview for images-devsy-sh canceled.

Name	Link
🔨 Latest commit	17f3a55
🔍 Latest deploy log	https://app.netlify.com/projects/images-devsy-sh/deploys/6a47ec6d16ea760008a28907

[coderabbitai]

📝 Walkthrough

Walkthrough

Container state model gains ExitCode and Error fields. WaitContainerRunning is refactored to distinguish terminal versus exited containers using new sentinel errors and a grace period, with centralized error/log formatting. Container restart logic adds a bounded retry loop. Corresponding unit tests are added.

Changes

Container wait and restart resilience

Layer / File(s)	Summary
ContainerDetailsState exit fields pkg/devcontainer/config/container_details.go	Adds ExitCode and Error JSON fields to ContainerDetailsState.
WaitContainerRunning sentinel errors and grace period pkg/docker/helper.go, pkg/docker/helper_test.go	Introduces ErrContainerExited, grace/timing constants, refactors polling to use evaluateContainerState, containerStateError, tailContainerLogs, and failedBootSentinel; adds tests for running, exited, and dead scenarios.
ensureContainerRunning bounded retry loop pkg/driver/docker/lifecycle.go, pkg/driver/docker/lifecycle_test.go, pkg/driver/docker/docker_test.go	Adds containerRestartAttempts and restartAndWait, replaces single restart attempt with a retry loop that returns immediately on terminal errors and wraps the last error after exhausting attempts; adds tests covering already-running, terminal, recovery, and never-starts scenarios, plus a new test constant.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Driver
  participant ensureContainerRunning
  participant restartAndWait
  participant DockerHelper

  Driver->>ensureContainerRunning: ensureContainerRunning(ctx)
  loop up to containerRestartAttempts
    ensureContainerRunning->>ensureContainerRunning: check ctx.Err()
    ensureContainerRunning->>restartAndWait: restartAndWait()
    restartAndWait->>DockerHelper: StartContainer / WaitContainerRunning
    DockerHelper-->>restartAndWait: success or error
    restartAndWait-->>ensureContainerRunning: result
    alt terminal error
      ensureContainerRunning-->>Driver: ErrContainerTerminal
    else success
      ensureContainerRunning-->>Driver: nil
    end
  end
  ensureContainerRunning-->>Driver: wrapped ErrContainerTerminal after attempts exhausted

Loading

sequenceDiagram
  participant Caller
  participant WaitContainerRunning
  participant InspectContainers
  participant evaluateContainerState

  Caller->>WaitContainerRunning: WaitContainerRunning(ctx)
  loop poll until timeout
    WaitContainerRunning->>InspectContainers: InspectContainers()
    InspectContainers-->>WaitContainerRunning: container state
    WaitContainerRunning->>evaluateContainerState: evaluateContainerState(state, elapsed)
    evaluateContainerState-->>WaitContainerRunning: ErrContainerTerminal / ErrContainerExited / nil
  end
  WaitContainerRunning-->>Caller: success or containerStateError

Loading

Suggested labels: size/xl

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: improved Docker handling for exited containers that fail to start.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[netlify]

✅ Deploy Preview for devsydev canceled.

Name	Link
🔨 Latest commit	17f3a55
🔍 Latest deploy log	https://app.netlify.com/projects/devsydev/deploys/6a47ec6de674b60008be170e

[coderabbitai]

🧹 Nitpick comments (2)

pkg/docker/helper.go (2)

33-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Hardcoded wait/grace timing hurts test speed and determinism.

containerRunningPollInterval, containerRunningTimeout, and containerExitGrace are unexported package constants with no way to override them in tests. As a result, TestWaitContainerRunning_ExitedFailsFast, and especially the 3-attempt retry tests in lifecycle_test.go (RecoversOnRetry, NeverStartsFailsFast), each burn multiple real wall-clock seconds (containerExitGrace × attempts) just waiting out the grace period, rather than being deterministic and fast.

Consider exposing these as (overridable) fields on DockerHelper (with these constants as defaults) so tests can inject shorter values.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/docker/helper.go` around lines 33 - 37, The hardcoded timing constants in
DockerHelper make wait/retry tests slow and non-deterministic. Update
DockerHelper to carry overridable timing fields for
containerRunningPollInterval, containerRunningTimeout, and containerExitGrace,
and keep the current constants as defaults. Then adjust the wait/retry paths in
the DockerHelper methods used by WaitContainerRunning and the lifecycle retry
flow so tests can inject shorter values instead of burning real wall-clock time.

---

264-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Timeout error loses container-state context when there's no inspect error.

lastErr is reset to nil on every successful inspect (line 260). If the container gets stuck in a non-terminal, non-running status (e.g. created/restarting) for the full 30s without any inspect error, pollErr will be the generic wait-timeout error with no indication of the last observed container status/exit info, making failures harder to diagnose than the terminal/exited paths.

Consider tracking the last observed state/status alongside lastErr and including it in the final error when pollErr is a plain timeout.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/docker/helper.go` around lines 264 - 267, The timeout path in the polling
loop loses container-state context when `lastErr` is nil, so update the
`helper.go` logic around the inspect polling to also remember the last observed
container `state`/`status` alongside `lastErr`. Then, in the final `pollErr`
handling, detect the plain wait-timeout case and wrap it with the saved
state/status details so failures from stuck containers (for example `created` or
`restarting`) still report the last known container condition. Keep the existing
`pollErr`/`lastErr` behavior for inspect failures, but make the final returned
error from the polling function include the captured container-state context
when there was no inspect error.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@pkg/docker/helper.go`:
- Around line 33-37: The hardcoded timing constants in DockerHelper make
wait/retry tests slow and non-deterministic. Update DockerHelper to carry
overridable timing fields for containerRunningPollInterval,
containerRunningTimeout, and containerExitGrace, and keep the current constants
as defaults. Then adjust the wait/retry paths in the DockerHelper methods used
by WaitContainerRunning and the lifecycle retry flow so tests can inject shorter
values instead of burning real wall-clock time.
- Around line 264-267: The timeout path in the polling loop loses
container-state context when `lastErr` is nil, so update the `helper.go` logic
around the inspect polling to also remember the last observed container
`state`/`status` alongside `lastErr`. Then, in the final `pollErr` handling,
detect the plain wait-timeout case and wrap it with the saved state/status
details so failures from stuck containers (for example `created` or
`restarting`) still report the last known container condition. Keep the existing
`pollErr`/`lastErr` behavior for inspect failures, but make the final returned
error from the polling function include the captured container-state context
when there was no inspect error.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 076fc7cc-5bec-417b-ae0b-c8a99b45b2ae

📥 Commits

Reviewing files that changed from the base of the PR and between 585c718 and 17f3a55.

📒 Files selected for processing (6)

• pkg/devcontainer/config/container_details.go
• pkg/docker/helper.go
• pkg/docker/helper_test.go
• pkg/driver/docker/docker_test.go
• pkg/driver/docker/lifecycle.go
• pkg/driver/docker/lifecycle_test.go