Conversation
gtsiolis
requested review from
carole-lavillonniere and
silv-io
as code owners
internal/container/stop.go
Outdated
| msg := e.Err.Error() | ||
|
|
||
| if strings.Contains(msg, "No such container") || strings.Contains(msg, "not found") { | ||
| return fmt.Sprintf("%s is not running", e.Name) |
There was a problem hiding this comment.
suggestion: It seems like the docker sdk is defining functions that can be used to check the error instead of doing string comparison: https://pkg.go.dev/github.com/containerd/errdefs#IsNotFound
There was a problem hiding this comment.
With this change I don't think there is a need to define a custom error StopError anymore
📝 WalkthroughWalkthroughAdds a new CLI subcommand "stop" and the runtime/container support to stop and remove configured containers, including Docker runtime implementations and integration tests. (47 words) Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI Command\n(cmd/stop.go)
participant Orch as Container Orchestration\n(internal/container/stop.go)
participant Runtime as Docker Runtime\n(internal/runtime/docker.go)
participant Docker as Docker Daemon
User->>CLI: run `stop` command
CLI->>CLI: create DockerRuntime
CLI->>Orch: Stop(ctx, rt, onProgress)
Orch->>Orch: load config, iterate containers
loop per container
Orch->>Orch: onProgress("Stopping <name>")
Orch->>Runtime: Stop(ctx, <name>)
Runtime->>Docker: ContainerStop(<name>)
Docker-->>Runtime: stopped
Runtime->>Docker: ContainerRemove(<name>)
Docker-->>Runtime: removed
Orch->>Orch: onProgress("stopped <name>")
end
Orch-->>CLI: return result
CLI-->>User: print progress/status
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Some of the checks also fail in @carole-lavillonniere, could you take another look when you get a chance? 😁 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/integration/stop_test.go`:
- Around line 14-22: The test fails when the image is missing because
startTestContainer calls dockerClient.ContainerCreate without pulling the image;
update startTestContainer to explicitly pull "alpine:latest" before calling
dockerClient.ContainerCreate by invoking dockerClient.ImagePull(ctx,
"alpine:latest", types.ImagePullOptions{}), reading and closing the returned
io.ReadCloser to completion, checking for errors, then proceed with
dockerClient.ContainerCreate and dockerClient.ContainerStart (use the same
containerName/resp.ID as before).
We are aware of the integration tests failing on windows/main, just created an issue: DRG-505 However there are more tests failing in this PR, namely: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/container/start.go`:
- Around line 53-57: The error check in the container removal loop uses
containerd's errdefs, causing Docker-specific errors to be misclassified; update
the import and check to use Docker's errdefs: replace usages of
containerd/errdefs.IsNotFound(err) with
github.com/docker/docker/errdefs.IsNotFound(err) (the removal loop calling
rt.Remove and the DockerRuntime.Remove implementation will then correctly allow
not-found errors to be ignored).
In `@internal/runtime/docker.go`:
- Around line 99-107: The Stop implementation currently calls ContainerRemove
with empty RemoveOptions which leaves volumes behind; update DockerRuntime.Stop
to call ContainerRemove(ctx, containerName,
container.RemoveOptions{RemoveVolumes: true}) so volumes are removed when
stopping a container (and consider making the same change for
DockerRuntime.Remove if removal should always include volumes), or alternatively
add a comment/docstring on DockerRuntime.Stop and DockerRuntime.Remove
clarifying that volumes are intentionally retained.
🧹 Nitpick comments (1)
cmd/stop.go (1)
12-31: UseRunEto return errors instead of callingos.Exitdirectly.This is the recommended Cobra pattern for commands that can fail. It improves testability and lets Cobra handle error output and exit codes consistently. Errors returned from
RunEare processed at the top-levelExecute()call, which handles the exit code once instead of scattered throughout the command logic.♻️ Proposed refactor
var stopCmd = &cobra.Command{ Use: "stop", Short: "Stop LocalStack", Long: "Stop the LocalStack emulator.", - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { rt, err := runtime.NewDockerRuntime() if err != nil { - fmt.Fprintf(os.Stderr, "Error: %v\n", err) - os.Exit(1) + return err } onProgress := func(msg string) { - fmt.Println(msg) + fmt.Fprintln(cmd.OutOrStdout(), msg) } if err := container.Stop(cmd.Context(), rt, onProgress); err != nil { - fmt.Fprintln(os.Stderr, err) - os.Exit(1) + return err } + return nil }, }
| for _, config := range containers { | ||
| // Remove any existing stopped container with the same name | ||
| if err := rt.Remove(ctx, config.Name); err != nil && !errdefs.IsNotFound(err) { | ||
| return fmt.Errorf("failed to remove existing container %s: %w", config.Name, err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the file structure and locate the relevant code
head -60 internal/container/start.go | cat -nRepository: localstack/lstk
Length of output: 1912
🏁 Script executed:
# Check imports in the start.go file to understand what errdefs is being used
head -20 internal/container/start.go | cat -nRepository: localstack/lstk
Length of output: 630
🏁 Script executed:
# Search for the rt.Remove definition and understand what rt is
rg -B 10 "rt.Remove" internal/container/start.goRepository: localstack/lstk
Length of output: 364
🏁 Script executed:
# Look for the runtime type definition in the codebase
rg "type.*Runtime" internal/container/ --max-count 5Repository: localstack/lstk
Length of output: 41
🏁 Script executed:
# Check for Docker/containerd imports across the codebase
rg "containerd|docker" internal/container/start.goRepository: localstack/lstk
Length of output: 91
🏁 Script executed:
# Find the runtime package and interface definition
find internal/runtime -type f -name "*.go" | head -10Repository: localstack/lstk
Length of output: 113
🏁 Script executed:
# Look for the Runtime interface definition
rg "type Runtime interface" internal/runtime/Repository: localstack/lstk
Length of output: 111
🏁 Script executed:
# Search for Remove method implementation
rg -A 10 "func.*Remove" internal/runtime/ --max-count 5Repository: localstack/lstk
Length of output: 749
🏁 Script executed:
# Check what Docker client is being used in the runtime implementation
rg "docker|Docker" internal/runtime/ | head -20Repository: localstack/lstk
Length of output: 1448
🏁 Script executed:
# Let's check the complete Remove method and error handling in docker.go
cat -n internal/runtime/docker.go | sed -n '/func (d \*DockerRuntime) Remove/,/^[0-9]*\tFunc/p' | head -20Repository: localstack/lstk
Length of output: 840
🏁 Script executed:
# Check if there's any error handling or conversion in the docker.go file
cat internal/runtime/docker.go | head -50Repository: localstack/lstk
Length of output: 1236
🏁 Script executed:
# Look for any other error handling in docker.go to see patterns
rg "errdefs\|error" internal/runtime/docker.goRepository: localstack/lstk
Length of output: 41
🏁 Script executed:
# Check the complete docker.go file size and content
wc -l internal/runtime/docker.go && echo "---" && cat internal/runtime/docker.goRepository: localstack/lstk
Length of output: 3568
Replace containerd/errdefs.IsNotFound() with docker/errdefs.IsNotFound() for Docker error compatibility.
The Remove() method in DockerRuntime returns errors directly from the Docker client's ContainerRemove(), which returns Docker-specific error types from the docker/errdefs package. However, the error check at line 55 uses containerd/errdefs.IsNotFound(), which cannot recognize Docker errors. This causes the code to fail when a container doesn't exist instead of gracefully skipping the removal, breaking fresh starts.
Import github.com/docker/docker/errdefs and use errdefs.IsNotFound(err) from the Docker package instead of the containerd version.
🤖 Prompt for AI Agents
In `@internal/container/start.go` around lines 53 - 57, The error check in the
container removal loop uses containerd's errdefs, causing Docker-specific errors
to be misclassified; update the import and check to use Docker's errdefs:
replace usages of containerd/errdefs.IsNotFound(err) with
github.com/docker/docker/errdefs.IsNotFound(err) (the removal loop calling
rt.Remove and the DockerRuntime.Remove implementation will then correctly allow
not-found errors to be ignored).
| func (d *DockerRuntime) Stop(ctx context.Context, containerName string) error { | ||
| if err := d.client.ContainerStop(ctx, containerName, container.StopOptions{}); err != nil { | ||
| return err | ||
| } | ||
| return d.client.ContainerRemove(ctx, containerName, container.RemoveOptions{}) | ||
| } | ||
|
|
||
| func (d *DockerRuntime) Remove(ctx context.Context, containerName string) error { | ||
| return d.client.ContainerRemove(ctx, containerName, container.RemoveOptions{}) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
github.com/docker/docker v28 ContainerStopOptions defaults timeout
💡 Result:
In github.com/docker/docker v28, ContainerStopOptions (API type container.StopOptions) has:
Timeout *int(seconds). If you don’t set it, it staysnil(so it’s omitted). [1]
What happens when Timeout == nil:
- The client won’t send the
t/timeout query parameter at all. [2] - The daemon then uses the container’s configured
StopTimeout(if set); otherwise it falls back to the engine default. [3]
Engine default stop timeout (when neither API timeout nor per-container StopTimeout is set):
- non-Windows:
10seconds (defaultStopTimeout = 10) [4] - Windows:
30seconds (defaultStopTimeout = 30) [5]
Sources
[1] (pkg.go.dev)
[2] (fuchsia.googlesource.com)
[3] (pkg.go.dev)
[4] (fossies.org)
[5] (fossies.org)
Citations:
- 1: https://pkg.go.dev/github.com/docker/docker/api/types/container?utm_source=openai
- 2: https://fuchsia.googlesource.com/third_party/github.com/moby/moby/%2B/refs/tags/v28.3.0/client/container_stop.go?utm_source=openai
- 3: https://pkg.go.dev/github.com/docker/docker/daemon?utm_source=openai
- 4: https://fossies.org/linux/moby/container/container_unix.go?utm_source=openai
- 5: https://fossies.org/linux/moby/container/container_windows.go?utm_source=openai
🌐 Web query:
github.com/docker/docker v28 ContainerRemoveOptions defaults volume removal
💡 Result:
In github.com/docker/docker v28, ContainerRemoveOptions.RemoveVolumes is a bool with no explicit default set in code, so its zero-value default is false (i.e., volumes are not removed unless you set it). [1]
This matches the Docker Engine API: DELETE /containers/{id} has query param v (“remove volumes”) with default false. [2]
So, to remove volumes you must opt in, e.g.:
- Go:
ContainerRemoveOptions{RemoveVolumes: true} - API:
DELETE .../containers/{id}?v=1 - CLI:
docker rm -v .../--volumes[3]
Sources: [1] [2] [3]
Clarify whether volumes should be removed in the Stop operation.
The Stop() method uses empty StopOptions{}, which defers to the Docker daemon's default timeout (10 seconds on non-Windows, 30 seconds on Windows)—a reasonable default. However, ContainerRemove() with empty RemoveOptions{} leaves volumes behind by default (equivalent to docker rm without -v). If lstk stop is meant to fully clean up the container and its associated resources, set RemoveVolumes: true in the RemoveOptions:
container.RemoveOptions{RemoveVolumes: true}Otherwise, document the volume retention behavior if it is intentional.
🤖 Prompt for AI Agents
In `@internal/runtime/docker.go` around lines 99 - 107, The Stop implementation
currently calls ContainerRemove with empty RemoveOptions which leaves volumes
behind; update DockerRuntime.Stop to call ContainerRemove(ctx, containerName,
container.RemoveOptions{RemoveVolumes: true}) so volumes are removed when
stopping a container (and consider making the same change for
DockerRuntime.Remove if removal should always include volumes), or alternatively
add a comment/docstring on DockerRuntime.Stop and DockerRuntime.Remove
clarifying that volumes are intentionally retained.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@internal/container/stop.go`:
- Around line 12-20: The Stop function accepts onProgress func(string) but calls
it unguarded, which can panic if callers pass nil; at the start of Stop
(function Stop(ctx context.Context, rt runtime.Runtime, onProgress
func(string))) set a default no-op when onProgress == nil (e.g., onProgress =
func(string) {}), then continue using onProgress throughout the function
(including the places around the "Stopping %s..." and the later progress calls)
so calls are safe even when nil was provided.
- Around line 18-26: The loop over cfg.Containers currently returns on the first
stop error which leaves later containers running; change it to attempt stopping
all containers and only return an error after the loop: introduce a variable
(e.g., firstErr error) before the loop, call rt.Stop(ctx, name) for each
container, if err != nil handle IsNotFound by setting firstErr if unset and
continue, otherwise set firstErr = fmt.Errorf("failed to stop %s: %w", name,
err) if unset and continue; after the loop, if firstErr != nil return firstErr,
otherwise proceed normally (keep onProgress calls and existing name := c.Name()
usage).
- Around line 21-24: The check using errdefs.IsNotFound(err) in the caller
(rt.Stop) is unreachable for Docker runtime errors because
internal/runtime/docker.go currently returns raw Docker (moby) errors; update
the Docker runtime implementation in internal/runtime/docker.go (the Docker
Stop() implementation) to translate Docker "not found" errors into
containerd-style errors by detecting Docker's not-found condition (e.g., via
moby client helpers or checking the error string/type returned by the Docker
client) and returning an errdefs.NotFound error (or wrapping the docker error
into errdefs.NotFound) so callers can reliably use errdefs.IsNotFound(err) on
rt.Stop results.
| func Stop(ctx context.Context, rt runtime.Runtime, onProgress func(string)) error { | ||
| cfg, err := config.Get() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get config: %w", err) | ||
| } | ||
|
|
||
| for _, c := range cfg.Containers { | ||
| name := c.Name() | ||
| onProgress(fmt.Sprintf("Stopping %s...", name)) |
There was a problem hiding this comment.
Guard against nil onProgress to avoid a panic.
If callers pass nil, the current implementation will panic at Line 20/Line 27. Consider defaulting to a no-op.
🔧 Suggested fix
func Stop(ctx context.Context, rt runtime.Runtime, onProgress func(string)) error {
+ if onProgress == nil {
+ onProgress = func(string) {}
+ }
cfg, err := config.Get()Also applies to: 27-30
🤖 Prompt for AI Agents
In `@internal/container/stop.go` around lines 12 - 20, The Stop function accepts
onProgress func(string) but calls it unguarded, which can panic if callers pass
nil; at the start of Stop (function Stop(ctx context.Context, rt
runtime.Runtime, onProgress func(string))) set a default no-op when onProgress
== nil (e.g., onProgress = func(string) {}), then continue using onProgress
throughout the function (including the places around the "Stopping %s..." and
the later progress calls) so calls are safe even when nil was provided.
| for _, c := range cfg.Containers { | ||
| name := c.Name() | ||
| onProgress(fmt.Sprintf("Stopping %s...", name)) | ||
| if err := rt.Stop(ctx, name); err != nil { | ||
| if errdefs.IsNotFound(err) { | ||
| return fmt.Errorf("%s is not running", name) | ||
| } | ||
| return fmt.Errorf("failed to stop %s: %w", name, err) | ||
| } |
There was a problem hiding this comment.
Stopping halts on first error, leaving later containers running.
Because you return on the first failure, any remaining containers will never be stopped. Consider continuing the loop and returning an aggregated/first error after attempting all stops.
♻️ Suggested fix (continue + return first error)
- for _, c := range cfg.Containers {
+ var firstErr error
+ for _, c := range cfg.Containers {
name := c.Name()
onProgress(fmt.Sprintf("Stopping %s...", name))
if err := rt.Stop(ctx, name); err != nil {
if errdefs.IsNotFound(err) {
- return fmt.Errorf("%s is not running", name)
+ if firstErr == nil {
+ firstErr = fmt.Errorf("%s is not running", name)
+ }
+ continue
}
- return fmt.Errorf("failed to stop %s: %w", name, err)
+ if firstErr == nil {
+ firstErr = fmt.Errorf("failed to stop %s: %w", name, err)
+ }
+ continue
}
onProgress(fmt.Sprintf("%s stopped", name))
}
- return nil
+ return firstErr🤖 Prompt for AI Agents
In `@internal/container/stop.go` around lines 18 - 26, The loop over
cfg.Containers currently returns on the first stop error which leaves later
containers running; change it to attempt stopping all containers and only return
an error after the loop: introduce a variable (e.g., firstErr error) before the
loop, call rt.Stop(ctx, name) for each container, if err != nil handle
IsNotFound by setting firstErr if unset and continue, otherwise set firstErr =
fmt.Errorf("failed to stop %s: %w", name, err) if unset and continue; after the
loop, if firstErr != nil return firstErr, otherwise proceed normally (keep
onProgress calls and existing name := c.Name() usage).
| if err := rt.Stop(ctx, name); err != nil { | ||
| if errdefs.IsNotFound(err) { | ||
| return fmt.Errorf("%s is not running", name) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect Docker runtime error mapping for Stop.
echo "=== Searching for Stop method implementation ==="
rg -n -C3 'func\s+\(.*\)\s+Stop\s*\(' internal/runtime -g '*.go'
echo ""
echo "=== Searching for errdefs usage patterns ==="
rg -n -C3 'errdefs\.|IsNotFound|NotFound|No such container' internal/runtime -g '*.go'Repository: localstack/lstk
Length of output: 579
🏁 Script executed:
#!/bin/bash
echo "=== Checking if errdefs is used anywhere in the codebase ==="
rg -n 'errdefs\.' --type go | head -20
echo ""
echo "=== Checking imports in container/stop.go ==="
head -30 internal/container/stop.go
echo ""
echo "=== Looking for error wrapping/normalization patterns ==="
rg -n 'fmt\.Errorf|errors\.Is|errors\.As|errdefs' internal/container/stop.goRepository: localstack/lstk
Length of output: 1357
🏁 Script executed:
#!/bin/bash
echo "=== Checking docker.go for client initialization ==="
head -50 internal/runtime/docker.go | grep -A5 -B5 'client'
echo ""
echo "=== Full relevant sections of docker.go ==="
sed -n '1,100p' internal/runtime/docker.go | tail -60
echo ""
echo "=== Checking go.mod for Docker and errdefs versions ==="
rg 'containerd|moby|docker' go.modRepository: localstack/lstk
Length of output: 2889
🏁 Script executed:
#!/bin/bash
echo "=== Check for Docker errdefs or error handling patterns ==="
rg -n 'docker.*errdefs|errdefs.*docker' --type go
echo ""
echo "=== Search for ContainerStop error handling ==="
rg -n -B5 -A5 'ContainerStop' internal/runtime/docker.go
echo ""
echo "=== Check for error type assertions or conversions ==="
rg -n 'errors\.Is|errors\.As|type.*error' internal/runtime/docker.go | head -15
echo ""
echo "=== Look for any Docker error normalization ==="
rg -n 'IsNotFound|NotFound|errs' internal/runtime/docker.goRepository: localstack/lstk
Length of output: 608
🏁 Script executed:
#!/bin/bash
echo "=== Check Runtime interface definition ==="
rg -n -A10 'type Runtime interface' internal/runtime
echo ""
echo "=== Check for other Runtime implementations ==="
fd -e go internal/runtime | xargs grep -l 'func.*Runtime.*Start\|func.*Runtime.*Stop'
echo ""
echo "=== Check test files to understand expected error handling ==="
fd -e go -path '*/test*' -o -path '*_test.go' internal/runtime | head -5
echo ""
echo "=== Verify if containerd runtime exists ==="
ls -la internal/runtime/Repository: localstack/lstk
Length of output: 1721
🏁 Script executed:
#!/bin/bash
echo "=== Check Docker client import to understand error types ==="
rg -n 'import' internal/runtime/docker.go | head -20
echo ""
echo "=== Search for any error handling comments or TODOs ==="
rg -n 'TODO|FIXME|BUG|XXX' internal/runtime/
echo ""
echo "=== Check how errors from Docker API are typically handled in Go ==="
rg -n -B2 -A2 'client\.Container' internal/runtime/docker.go | grep -A2 -B2 'err'Repository: localstack/lstk
Length of output: 1398
errdefs.IsNotFound will not match Docker runtime errors—the condition is unreachable.
internal/runtime/docker.go returns Docker client errors directly without wrapping. Since Docker (moby) and containerd have incompatible error types, errdefs.IsNotFound() from containerd will never recognize Docker "not found" errors. The Stop() method must normalize Docker errors to errdefs format, or use Docker-specific error checking instead.
🤖 Prompt for AI Agents
In `@internal/container/stop.go` around lines 21 - 24, The check using
errdefs.IsNotFound(err) in the caller (rt.Stop) is unreachable for Docker
runtime errors because internal/runtime/docker.go currently returns raw Docker
(moby) errors; update the Docker runtime implementation in
internal/runtime/docker.go (the Docker Stop() implementation) to translate
Docker "not found" errors into containerd-style errors by detecting Docker's
not-found condition (e.g., via moby client helpers or checking the error
string/type returned by the Docker client) and returning an errdefs.NotFound
error (or wrapping the docker error into errdefs.NotFound) so callers can
reliably use errdefs.IsNotFound(err) on rt.Stop results.
2 participants
Fix DES-108