From 27508ec0b5136e36dde08aea5dff8d05b772c57c Mon Sep 17 00:00:00 2001 From: Vasili Pascal Date: Mon, 21 Sep 2026 19:01:58 +0300 Subject: [PATCH] fix(compose): stop double-wrapping healthcheck test commands Every healthcheck written the way the reference documents came out broken: /bin/sh: 1: CMD-SHELL: not found Docker wraps a plain string `test` in `CMD-SHELL` itself, so a string that already spells the prefix out gets wrapped twice and the container tries to execute a program named `CMD-SHELL`. The prefix only means anything in the list form. The generator was emitting the author's string verbatim. Observed on a real deploy: both stackpilot services that declare a healthcheck came up unhealthy while the application itself was fine. Harmless on its own, but the state is frozen into a baked snapshot, and a stack using `depends_on: condition: service_healthy` would never start. The decision now lives in one place. `compose_service_sync` already had `healthcheck_test_value` doing this correctly for the server-side path; the generator delegates to it and only renders the result as inline YAML. That also picks up a subtlety a second implementation would have missed: `CMD` executes argv directly, so a command containing `&&`, `|`, `$` or redirection is emitted as `CMD-SHELL` instead. An explicit list written by the author now passes through untouched. The reference said `test: "CMD pg_isready -U postgres"` and left it there. It now documents all three accepted forms, what each one runs, and why the prefix works here but not in a plain compose file. Co-Authored-By: Claude Opus 5 --- docs/STACKER_YML_REFERENCE.md | 20 +++++++- src/cli/compose_service_sync.rs | 12 ++++- src/cli/generator/compose.rs | 88 ++++++++++++++++++++++++++++++++- 3 files changed, 117 insertions(+), 3 deletions(-) diff --git a/docs/STACKER_YML_REFERENCE.md b/docs/STACKER_YML_REFERENCE.md index 9830eeff..788f4e30 100644 --- a/docs/STACKER_YML_REFERENCE.md +++ b/docs/STACKER_YML_REFERENCE.md @@ -431,11 +431,29 @@ Docker health check configuration. Mapped directly to the compose `healthcheck:` | Field | Type | Default | Description | |-------|------|---------|-------------| -| `test` | `string` | — | Health check command (e.g. `"CMD pg_isready -U postgres"`) | +| `test` | `string` | — | Health check command. See the forms below. | | `interval` | `string` | `30s` | Time between checks | | `timeout` | `string` | `30s` | Maximum time per check | | `retries` | `string \| integer` | `3` | Number of failures before unhealthy | +**Forms of `test`.** Three spellings, all accepted: + +| What you write | What runs | +|----------------|-----------| +| `pg_isready -U postgres` | the command, through a shell | +| `CMD-SHELL pg_isready -U postgres` | the same — the prefix is stripped and the list form emitted | +| `CMD pg_isready -U postgres` | the command directly, without a shell | + +`CMD` executes the arguments as-is, so it cannot use `&&`, `|`, `$` or +redirection; a `CMD` command containing any of those is emitted as `CMD-SHELL` +instead, which is what the author meant. + +Write the prefix or leave it out, whichever reads better — stacker converts to +the list form Docker expects either way. Writing the prefix inside a plain +compose file would not work: Docker wraps a bare string in `CMD-SHELL` itself, +so the prefix would be wrapped a second time and the container would try to run +a program named `CMD-SHELL`. + ```yaml services: - name: postgres diff --git a/src/cli/compose_service_sync.rs b/src/cli/compose_service_sync.rs index 1b843db2..99ccbcc0 100644 --- a/src/cli/compose_service_sync.rs +++ b/src/cli/compose_service_sync.rs @@ -276,13 +276,23 @@ fn upsert_compose_service( /// shell syntax cannot run in exec form, so it degrades to `CMD-SHELL` rather /// than being split into nonsense argv. Unprefixed strings are already valid /// and are left alone. -fn healthcheck_test_value(test: &str) -> serde_yaml::Value { +pub(crate) fn healthcheck_test_value(test: &str) -> serde_yaml::Value { fn list(parts: impl IntoIterator) -> serde_yaml::Value { serde_yaml::Value::Sequence(parts.into_iter().map(serde_yaml::Value::String).collect()) } let trimmed = test.trim(); + // An author who already wrote the list form knows what they are doing; + // re-quoting it as a string would break the very thing they got right. + if trimmed.starts_with('[') { + if let Ok(serde_yaml::Value::Sequence(parts)) = + serde_yaml::from_str::(trimmed) + { + return serde_yaml::Value::Sequence(parts); + } + } + if let Some(rest) = trimmed.strip_prefix("CMD-SHELL ") { return list(["CMD-SHELL".to_string(), rest.trim().to_string()]); } diff --git a/src/cli/generator/compose.rs b/src/cli/generator/compose.rs index 203eb68d..34c6c9d1 100644 --- a/src/cli/generator/compose.rs +++ b/src/cli/generator/compose.rs @@ -703,7 +703,10 @@ impl ComposeDefinition { if let Some(ref hc) = svc.healthcheck { out.push_str(" healthcheck:\n"); - out.push_str(&format!(" test: {}\n", yaml_quote(&hc.test))); + out.push_str(&format!( + " test: {}\n", + render_healthcheck_test(&hc.test) + )); out.push_str(&format!(" interval: {}\n", hc.interval)); out.push_str(&format!(" timeout: {}\n", hc.timeout)); out.push_str(&format!(" retries: {}\n", hc.retries)); @@ -1193,6 +1196,39 @@ fn closing_brace_on_line(rest: &str) -> Option { rest[..line_end].find('}') } +/// Render a healthcheck `test` in the form Docker actually expects. +/// +/// Docker wraps a plain *string* in `CMD-SHELL` on its own, so a string that +/// already spells the prefix out is wrapped twice and the container ends up +/// trying to run a program named `CMD-SHELL`: +/// +/// ```text +/// /bin/sh: 1: CMD-SHELL: not found +/// ``` +/// +/// The prefix only means anything in the list form — and the prefix form is +/// what the stacker.yml reference tells authors to write, so it is converted +/// rather than rejected. +/// +/// The decision itself lives in [`crate::cli::compose_service_sync::healthcheck_test_value`], +/// which the server-side sync path already uses; this only renders its result +/// as inline YAML. One rule, two call sites. +fn render_healthcheck_test(test: &str) -> String { + match crate::cli::compose_service_sync::healthcheck_test_value(test) { + serde_yaml::Value::Sequence(parts) => { + let rendered = parts + .iter() + .filter_map(serde_yaml::Value::as_str) + .map(yaml_quote) + .collect::>() + .join(", "); + format!("[{rendered}]") + } + serde_yaml::Value::String(plain) => yaml_quote(&plain), + other => yaml_quote(other.as_str().unwrap_or_default()), + } +} + /// Returns `true` when `s` looks like a POSIX env-variable name. fn is_env_identifier(s: &str) -> bool { !s.is_empty() @@ -2646,6 +2682,56 @@ services: assert_eq!(parameterize_compose_env_vars(compose, &keys), compose); } + // ── healthcheck test form ────────────────────────────────────────────── + + /// Docker wraps a *string* `test` in `CMD-SHELL` itself, so a string that + /// already carries the prefix is wrapped twice and the container tries to + /// execute a program literally named `CMD-SHELL`: + /// + /// /bin/sh: 1: CMD-SHELL: not found + /// + /// The prefix is only meaningful in the list form, and the prefix form is + /// what `docs/STACKER_YML_REFERENCE.md` tells authors to write. + #[test] + fn healthcheck_cmd_shell_prefix_becomes_a_list() { + let rendered = render_healthcheck_test("CMD-SHELL pg_isready -d app -U app"); + assert_eq!(rendered, r#"["CMD-SHELL", "pg_isready -d app -U app"]"#); + } + + /// `CMD` runs the argv directly, so each word is its own element. + #[test] + fn healthcheck_cmd_prefix_becomes_an_argv_list() { + let rendered = render_healthcheck_test("CMD redis-cli ping"); + assert_eq!(rendered, r#"["CMD", "redis-cli", "ping"]"#); + } + + /// Without a prefix the string form is correct — Docker supplies the shell. + #[test] + fn healthcheck_without_a_prefix_stays_a_string() { + let rendered = render_healthcheck_test("curl -f http://localhost/health"); + assert_eq!(rendered, r#""curl -f http://localhost/health""#); + } + + /// The list form an author wrote by hand must survive untouched. + #[test] + fn healthcheck_already_a_list_is_left_alone() { + let rendered = render_healthcheck_test(r#"["CMD", "true"]"#); + assert_eq!(rendered, r#"["CMD", "true"]"#); + } + + /// A value that merely starts with the letters CMD is not a prefix. + #[test] + fn healthcheck_cmdline_is_not_mistaken_for_a_prefix() { + let rendered = render_healthcheck_test("cmdline-check --fast"); + assert_eq!(rendered, r#""cmdline-check --fast""#); + } + + #[test] + fn healthcheck_quotes_are_escaped_inside_the_list() { + let rendered = render_healthcheck_test(r#"CMD-SHELL test "$(id -u)" = 0"#); + assert_eq!(rendered, r#"["CMD-SHELL", "test \"$(id -u)\" = 0"]"#); + } + #[test] fn parameterize_no_keys_returns_original() { let compose = "services:\n app:\n environment:\n FOO: bar\n";