Skip to content

Pass -r to xargs so empty container lists don't log as failed #1909

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ya-luotao wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Pass -r to xargs so empty container lists don't log as failed #1909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/kamal/commands/base.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ def substitute(*commands)
end

def xargs(command)
[ :xargs, command ].flatten
[ :xargs, "-r", command ].flatten

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve Docker list failures with xargs -r

When the producer in these pipelines is docker container ls, -r makes xargs return success on empty stdin, and these commands are emitted as ordinary shell pipelines with no pipefail setup in the repo. If the Docker query itself fails before printing IDs (for example, daemon unavailable or permission denied), commands such as remove_container or app stop --version now succeed as no-ops instead of surfacing the Docker error; GNU xargs --help confirms -r means it will not run the command when there are no arguments. Please preserve failures from the left-hand side while making truly empty container lists no-op.

Useful? React with 👍 / 👎.

@ya-luotao ya-luotao Jul 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point on the tradeoff, but I think the impact is narrower than it reads:

  • The deploy-flow call sites (app.stop, status, container_health_log) already run with raise_on_non_zero_exit: false or just capture output, so a failing docker container ls was never surfaced through the exit code there — nothing is lost.
  • The call sites where the exit code did matter are the remove_container/remove_containers paths. There the pre--r "surfacing" was accidental: the daemon error came through as a confusing docker rm usage error (exit 123) rather than as the real failure. The daemon error itself still lands in the log via stderr either way; only the exit code changes.
  • Preserving left-hand failures would need pipefail (not portable across sh implementations) or wrapping every pipeline in a sh -c emptiness guard — out of proportion for the "daemon dies between two docker commands" window, where the next docker command in the flow fails loudly anyway.

Happy to scope -r down to just the best-effort stop pipelines if the maintainers prefer to keep the remove/status paths as they are.

end

def shell(command)
Expand Down
30 changes: 15 additions & 15 deletions test/cli/app_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class CliAppTest < CliTestCase
run_command("boot").tap do |output|
assert_match "docker tag dhh/app:latest dhh/app:latest", output
assert_match /docker run --detach --restart unless-stopped --name app-web-latest --network kamal --hostname 1.1.1.1-[0-9a-f]{12} /, output
assert_match "docker container ls --all --filter 'name=^app-web-123$' --quiet | xargs docker stop", output
assert_match "docker container ls --all --filter 'name=^app-web-123$' --quiet | xargs -r docker stop", output
end
end

Expand All @@ -30,7 +30,7 @@ class CliAppTest < CliTestCase
assert_match /Renaming container .* to .* as already deployed on 1.1.1.1/, output # Rename
assert_match /docker rename app-web-latest app-web-latest_replaced_[0-9a-f]{16}/, output
assert_match /docker run --detach --restart unless-stopped --name app-web-latest --network kamal --hostname 1.1.1.1-[0-9a-f]{12} /, output
assert_match "docker container ls --all --filter 'name=^app-web-123$' --quiet | xargs docker stop", output
assert_match "docker container ls --all --filter 'name=^app-web-123$' --quiet | xargs -r docker stop", output
end
ensure
Thread.report_on_exception = true
Expand Down Expand Up @@ -100,7 +100,7 @@ class CliAppTest < CliTestCase
assert_match "/usr/bin/env mkdir -p .kamal/apps/app/assets/volumes/web-latest ; cp -rnT .kamal/apps/app/assets/extracted/web-latest .kamal/apps/app/assets/volumes/web-latest ; cp -rnT .kamal/apps/app/assets/extracted/web-latest .kamal/apps/app/assets/volumes/web-123 || true ; cp -rnT .kamal/apps/app/assets/extracted/web-123 .kamal/apps/app/assets/volumes/web-latest || true", output
assert_match "/usr/bin/env mkdir -p .kamal/apps/app/assets/extracted/web-latest && docker container rm app-web-assets 2> /dev/null || true && docker container create --name app-web-assets dhh/app:latest && docker container cp -L app-web-assets:/public/assets/. .kamal/apps/app/assets/extracted/web-latest && docker container rm app-web-assets", output
assert_match /docker run --detach --restart unless-stopped --name app-web-latest --network kamal --hostname 1.1.1.1-[0-9a-f]{12} /, output
assert_match "docker container ls --all --filter 'name=^app-web-123$' --quiet | xargs docker stop", output
assert_match "docker container ls --all --filter 'name=^app-web-123$' --quiet | xargs -r docker stop", output
assert_match "/usr/bin/env find .kamal/apps/app/assets/extracted -maxdepth 1 -name 'web-*' ! -name web-latest -exec rm -rf \"{}\" + ; find .kamal/apps/app/assets/volumes -maxdepth 1 -name 'web-*' ! -name web-latest -exec rm -rf \"{}\" +", output
end
end
Expand All @@ -123,7 +123,7 @@ class CliAppTest < CliTestCase
run_command("boot", config: :with_env_tags).tap do |output|
assert_match "docker tag dhh/app:latest dhh/app:latest", output
assert_match %r{docker run --detach --restart unless-stopped --name app-web-latest --network kamal --hostname 1.1.1.1-[0-9a-f]{12} --env KAMAL_CONTAINER_NAME="app-web-latest" --env KAMAL_VERSION="latest" --env KAMAL_HOST="1.1.1.1" --env TEST="root" --env EXPERIMENT="disabled" --env SITE="site1"}, output
assert_match "docker container ls --all --filter 'name=^app-web-123$' --quiet | xargs docker stop", output
assert_match "docker container ls --all --filter 'name=^app-web-123$' --quiet | xargs -r docker stop", output
end
end

Expand All @@ -133,7 +133,7 @@ class CliAppTest < CliTestCase
SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info).returns("123") # old version

SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
.with(:docker, :container, :ls, "--all", "--filter", "'name=^app-workers-latest$'", "--quiet", "|", :xargs, :docker, :inspect, "--format", "'{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}'")
.with(:docker, :container, :ls, "--all", "--filter", "'name=^app-workers-latest$'", "--quiet", "|", :xargs, "-r", :docker, :inspect, "--format", "'{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}'")
.returns("running").at_least_once # workers health check

run_command("boot", config: :with_roles, host: nil).tap do |output|
Expand All @@ -153,7 +153,7 @@ class CliAppTest < CliTestCase

SSHKit::Backend::Abstract.any_instance.stubs(:execute).returns("")
SSHKit::Backend::Abstract.any_instance.stubs(:execute)
.with(:docker, :container, :ls, "--all", "--filter", "'name=^app-web-latest$'", "--quiet", "|", :xargs, :docker, :stop, raise_on_non_zero_exit: false)
.with(:docker, :container, :ls, "--all", "--filter", "'name=^app-web-latest$'", "--quiet", "|", :xargs, "-r", :docker, :stop, raise_on_non_zero_exit: false)
SSHKit::Backend::Abstract.any_instance.expects(:execute)
.with(:docker, :exec, "kamal-proxy", "kamal-proxy", :deploy, "app-web", "--target=\"123:80\"", "--deploy-timeout=\"1s\"", "--drain-timeout=\"30s\"", "--buffer-requests", "--buffer-responses", "--log-request-header=\"Cache-Control\"", "--log-request-header=\"Last-Modified\"", "--log-request-header=\"User-Agent\"").raises(SSHKit::Command::Failed.new("Failed to deploy"))

Expand All @@ -177,7 +177,7 @@ class CliAppTest < CliTestCase
SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info).returns("123") # old version

SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
.with(:docker, :container, :ls, "--all", "--filter", "'name=^app-workers-latest$'", "--quiet", "|", :xargs, :docker, :inspect, "--format", "'{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}'")
.with(:docker, :container, :ls, "--all", "--filter", "'name=^app-workers-latest$'", "--quiet", "|", :xargs, "-r", :docker, :inspect, "--format", "'{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}'")
.returns("unhealthy").at_least_once # workers health check

run_command("boot", config: :with_roles, host: nil, allow_execute_error: true).tap do |output|
Expand All @@ -200,7 +200,7 @@ class CliAppTest < CliTestCase
SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info).returns("123") # old version

SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
.with(:docker, :container, :ls, "--all", "--filter", "'name=^app-workers-latest$'", "--quiet", "|", :xargs, :docker, :inspect, "--format", "'{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}'")
.with(:docker, :container, :ls, "--all", "--filter", "'name=^app-workers-latest$'", "--quiet", "|", :xargs, "-r", :docker, :inspect, "--format", "'{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}'")
.returns("running", "stopped").at_least_once # workers health check

run_command("boot", config: :with_roles, host: "1.1.1.3", allow_execute_error: true).tap do |output|
Expand All @@ -216,7 +216,7 @@ class CliAppTest < CliTestCase
SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info).returns("123") # old version

SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
.with(:docker, :container, :ls, "--all", "--filter", "'name=^app-workers-latest$'", "--quiet", "|", :xargs, :docker, :inspect, "--format", "'{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}'")
.with(:docker, :container, :ls, "--all", "--filter", "'name=^app-workers-latest$'", "--quiet", "|", :xargs, "-r", :docker, :inspect, "--format", "'{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}'")
.returns("running").at_least_once # workers health check

run_command("boot", config: :with_only_workers, host: nil).tap do |output|
Expand All @@ -232,7 +232,7 @@ class CliAppTest < CliTestCase
assert_match /Uploading .*kamal-error-pages.*\/latest to \.kamal\/proxy\/apps-config\/app\/error_pages/, output
assert_match "docker tag dhh/app:latest dhh/app:latest", output
assert_match /docker run --detach --restart unless-stopped --name app-web-latest --network kamal --hostname 1.1.1.1-[0-9a-f]{12} /, output
assert_match "docker container ls --all --filter 'name=^app-web-123$' --quiet | xargs docker stop", output
assert_match "docker container ls --all --filter 'name=^app-web-123$' --quiet | xargs -r docker stop", output
assert_match "Running /usr/bin/env find .kamal/proxy/apps-config/app/error_pages -mindepth 1 -maxdepth 1 ! -name latest -exec rm -rf {} + on 1.1.1.1", output
end
end
Expand Down Expand Up @@ -264,7 +264,7 @@ class CliAppTest < CliTestCase

test "stop" do
run_command("stop").tap do |output|
assert_match "sh -c 'docker ps --latest --quiet --filter label=service=app --filter label=destination= --filter label=role=web --filter status=running --filter status=restarting --filter ancestor=$(docker image ls --filter reference=dhh/app:latest --format '\\''{{.ID}}'\\'') ; docker ps --latest --quiet --filter label=service=app --filter label=destination= --filter label=role=web --filter status=running --filter status=restarting' | head -1 | xargs docker stop", output
assert_match "sh -c 'docker ps --latest --quiet --filter label=service=app --filter label=destination= --filter label=role=web --filter status=running --filter status=restarting --filter ancestor=$(docker image ls --filter reference=dhh/app:latest --format '\\''{{.ID}}'\\'') ; docker ps --latest --quiet --filter label=service=app --filter label=destination= --filter label=role=web --filter status=running --filter status=restarting' | head -1 | xargs -r docker stop", output
end
end

Expand Down Expand Up @@ -293,7 +293,7 @@ class CliAppTest < CliTestCase

run_command("stale_containers", "--stop").tap do |output|
assert_match /Stopping stale container for role web with version 87654321/, output
assert_match /#{Regexp.escape("docker container ls --all --filter 'name=^app-web-87654321$' --quiet | xargs docker stop")}/, output
assert_match /#{Regexp.escape("docker container ls --all --filter 'name=^app-web-87654321$' --quiet | xargs -r docker stop")}/, output
end
end

Expand All @@ -305,7 +305,7 @@ class CliAppTest < CliTestCase

test "remove" do
run_command("remove").tap do |output|
assert_match "sh -c 'docker ps --latest --quiet --filter label=service=app --filter label=destination= --filter label=role=web --filter status=running --filter status=restarting --filter ancestor=$(docker image ls --filter reference=dhh/app:latest --format '\\''{{.ID}}'\\'') ; docker ps --latest --quiet --filter label=service=app --filter label=destination= --filter label=role=web --filter status=running --filter status=restarting' | head -1 | xargs docker stop", output
assert_match "sh -c 'docker ps --latest --quiet --filter label=service=app --filter label=destination= --filter label=role=web --filter status=running --filter status=restarting --filter ancestor=$(docker image ls --filter reference=dhh/app:latest --format '\\''{{.ID}}'\\'') ; docker ps --latest --quiet --filter label=service=app --filter label=destination= --filter label=role=web --filter status=running --filter status=restarting' | head -1 | xargs -r docker stop", output
assert_match "docker container prune --force --filter label=service=app", output
assert_match "docker image prune --all --force --filter label=service=app", output
assert_match "rm -r .kamal/apps/app on 1.1.1.1", output
Expand Down Expand Up @@ -337,7 +337,7 @@ class CliAppTest < CliTestCase

test "remove_container" do
run_command("remove_container", "1234567").tap do |output|
assert_match "docker container ls --all --filter 'name=^app-web-1234567$' --quiet | xargs docker container rm", output
assert_match "docker container ls --all --filter 'name=^app-web-1234567$' --quiet | xargs -r docker container rm", output
end
end

Expand Down Expand Up @@ -548,7 +548,7 @@ class CliAppTest < CliTestCase
assert_match /docker rename app-web-latest app-web-latest_replaced_[0-9a-f]{16}/, output
assert_match /docker run --detach --restart unless-stopped --name app-web-latest --network kamal --hostname 1.1.1.1-[0-9a-f]{12} --env KAMAL_CONTAINER_NAME="app-web-latest" --env KAMAL_VERSION="latest" --env KAMAL_HOST="1.1.1.1" --env-file .kamal\/apps\/app\/env\/roles\/web.env --log-opt max-size="10m" --label service="app" --label role="web" --label destination dhh\/app:latest/, output
assert_match /docker exec kamal-proxy kamal-proxy deploy app-web --target="123:80"/, output
assert_match "docker container ls --all --filter 'name=^app-web-123$' --quiet | xargs docker stop", output
assert_match "docker container ls --all --filter 'name=^app-web-123$' --quiet | xargs -r docker stop", output
end
end

Expand Down
6 changes: 3 additions & 3 deletions test/cli/main_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ class CliMainTest < CliTestCase
end

SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
.with(:docker, :container, :ls, "--all", "--filter", "'name=^app-workers-123$'", "--quiet", "|", :xargs, :docker, :inspect, "--format", "'{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}'")
.with(:docker, :container, :ls, "--all", "--filter", "'name=^app-workers-123$'", "--quiet", "|", :xargs, "-r", :docker, :inspect, "--format", "'{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}'")
.returns("running").at_least_once # health check

Kamal::Commands::Hook.any_instance.stubs(:hook_exists?).returns(true)
Expand All @@ -465,7 +465,7 @@ class CliMainTest < CliTestCase
assert_hook_ran "pre-deploy", output
assert_match "docker tag dhh/app:123 dhh/app:latest", output
assert_match "docker run --detach --restart unless-stopped --name app-web-123", output
assert_match "docker container ls --all --filter 'name=^app-web-version-to-rollback$' --quiet | xargs docker stop", output, "Should stop the container that was previously running"
assert_match "docker container ls --all --filter 'name=^app-web-version-to-rollback$' --quiet | xargs -r docker stop", output, "Should stop the container that was previously running"
assert_hook_ran "post-deploy", output
end
end
Expand Down Expand Up @@ -690,7 +690,7 @@ class CliMainTest < CliTestCase
assert_match /docker container prune --force --filter label=org.opencontainers.image.title=kamal-proxy/, output
assert_match /docker image prune --all --force --filter label=org.opencontainers.image.title=kamal-proxy/, output

assert_match /docker ps --quiet --filter label=service=app | xargs docker stop/, output
assert_match /docker ps --quiet --filter label=service=app | xargs -r docker stop/, output
assert_match /docker container prune --force --filter label=service=app/, output
assert_match /docker image prune --all --force --filter label=service=app/, output
assert_match "/usr/bin/env rm -r .kamal/apps/app", output
Expand Down
Loading