Skip to content

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

Open
ya-luotao wants to merge 1 commit into
basecamp:mainfrom
ya-luotao:xargs-no-run-if-empty
Open

Pass -r to xargs so empty container lists don't log as failed#1909
ya-luotao wants to merge 1 commit into
basecamp:mainfrom
ya-luotao:xargs-no-run-if-empty

Conversation

@ya-luotao

@ya-luotao ya-luotao commented Jul 8, 2026

Copy link
Copy Markdown

When the container list feeding one of Kamal's xargs docker ... pipelines is empty, GNU xargs still runs the command once with no arguments. docker stop then fails with a usage error, and the deploy log reports a scary-looking failure for what is actually a no-op:

INFO [491f2b0b] Running docker container ls --all --filter 'name=^app-web-abc123$' --quiet | xargs docker stop -t 30 on 1.2.3.4
INFO [491f2b0b] Finished in 0.449 seconds with exit status 123 (failed).

This happens whenever the container is already gone — kamal app stop on an already-stopped app, or a deploy where the old version's container was already pruned or removed. Kamal already treats the deploy-flow stops as best-effort (raise_on_non_zero_exit: false in cli/app/boot.rb and cli/app.rb), so the failure is harmless — but the (failed) line is noise that reads like a real problem to anyone scanning deploy output.

This PR passes -r (--no-run-if-empty) through the shared xargs helper, so an empty list becomes a clean no-op that exits 0.

Besides the log noise, this also improves the other call sites of the helper:

  • app remove_container / remove_containers run through plain execute, so today they raise when the container is already gone; with -r they're idempotent no-ops.
  • app.status (healthcheck poller) and container_health_log: if the container disappears mid-poll, the poller today aborts with a raw SSHKit exit-123 error; with -r the empty status falls into the normal retry loop and fails with the proper "container not ready" message at the deploy timeout.
  • The proxy boot pipe (boot_config | xargs docker run) is unaffected: boot_config is an echo that always produces output.

The hardcoded xargs docker logs strings in commands/app/logging.rb are deliberately left untouched to keep this focused.

Portability of -r:

  • GNU findutils: supported (this is the case that matters — these commands run on the Docker deploy hosts).
  • BSD/macOS xargs: doesn't run the utility on empty input to begin with, and explicitly accepts -r as a GNU-compatibility no-op (documented in the man page).
  • busybox xargs: supports -r.

When the container list feeding an xargs docker pipeline is empty,
GNU xargs still runs docker once with no arguments, which fails with
a usage error. Deploy logs then show 'exit status 123 (failed)' for
what is actually a harmless no-op, and app remove_container raises
when the container is already gone.

xargs -r (--no-run-if-empty) skips the command on empty input. BSD
xargs already behaves this way and accepts -r as a compatibility
no-op; busybox supports it too.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015A3uSJQMxBnPxxHRA665cy
Copilot AI review requested due to automatic review settings July 8, 2026 16:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR makes Kamal’s xargs docker ... pipelines behave as clean no-ops when the input list is empty by passing -r (--no-run-if-empty) through the shared xargs command helper. This avoids misleading “(failed)” deploy log lines (and in some cases exceptions) when a container is already gone.

Changes:

  • Update Kamal::Commands::Base#xargs to emit xargs -r ... for all helper-based pipelines.
  • Adjust command and CLI tests to expect xargs -r in generated pipelines.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
lib/kamal/commands/base.rb Adds -r to the shared xargs helper so empty inputs don’t run the downstream command.
test/commands/proxy_test.rb Updates expected proxy boot pipeline to include xargs -r.
test/commands/app_test.rb Updates expected app command pipelines (stop/remove container) to include xargs -r.
test/cli/proxy_test.rb Updates CLI output expectations where proxy-related pipelines include xargs -r.
test/cli/main_test.rb Updates CLI output expectations for stop pipeline to include xargs -r.
test/cli/app_test.rb Updates CLI output expectations for app stop/remove-container pipelines to include xargs -r.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f19558347b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


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

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 👍 / 👎.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants