From 96b2c03b48ac6b00bdd225c11266979d89022923 Mon Sep 17 00:00:00 2001 From: Paul O'Fallon Date: Sun, 26 Jul 2026 01:17:16 +0000 Subject: [PATCH] fix(compose): run the service's declared command instead of discarding it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On the compose path deacon replaced the service's declared `command` with its keep-alive, so the service's own process never ran. Any compose service whose command matters — a server, a database bootstrap, a queue worker — silently did not start. The spec is explicit. `overrideCommand` "Defaults to `true` for when using an image Dockerfile and `false` when referencing a Docker Compose file", and spells out why: "Set to `false` if the default command must run for the container to function properly." `compose.rs` read `unwrap_or(true)` on a compose-only path, under a comment asserting the opposite of the spec. Measured against pinned oracle 0.87.0, not inferred. A service whose command is `sh -c 'touch ; sleep infinity'`: the marker exists under the reference and not under deacon. `docker inspect` shows the mechanism — the reference puts its keep-alive in `Entrypoint` ending in `exec "$@"` and leaves `Cmd` as the compose command; deacon put its keep-alive in `Cmd`, discarding the command. deacon now uses the reference's shape, which was measured across all three cases that distinguish it rather than assumed: entrypoint = [wrapper, "-"] ++ command = (or [] when overrideCommand: true) - declared command → `exec "$@"` runs it; the container lives as long as the service's own process, which is what `overrideCommand: false` means. - nothing declared → `"$@"` is empty, `exec` no-ops, and the keep-alive loop holds the container open. - declared entrypoint → PREPENDED to, never replaced, so a multi-stage entrypoint (tini, a wrapper script) still receives the command as args. The reference concatenates identically: a fixture declaring both an entrypoint and a command runs BOTH, verified on each side. - `overrideCommand: true` → the command is EMPTIED rather than replaced, which is again what the reference does (same entrypoint, `Cmd: []`). Reading the declared entrypoint needs the RENDERED compose config, so `extract_service_entrypoint` parses `compose config --format json` — extends and multi-file merging already applied. A read failure warns and degrades to "a declared entrypoint is not preserved", never to "nothing runs". Two existing tests asserted the defect, which is why it survived: `test_compose_override_command_default_keeps_service_alive` claimed a service running `echo hello` stays alive under the default, and the lifecycle test relied on the same. That premise is only true if the command is discarded. The reference FAILS that fixture (`{"outcome":"error"}`, exit 1) because it honors the default and the container exits with its command — so the tests were rewritten to the spec's contract, with the measurement recorded in them. The lifecycle test now sets `overrideCommand: true` explicitly, which is what the spec's `true` is for. The rewritten default test asserts the MARKER rather than `up` failing. A first draft asserting failure was flaky under parallel load: whether a millisecond-lived container is still present when deacon looks is a race, while "did the declared command run" is the actual claim and is deterministic. Also caps the primary-container-id backoff. Uncapped doubling reached a 51-second single sleep and ~102 s total, and this fix makes that path reachable for the first time — a user whose service command exits should not wait 102 s to be told so. Capped to 2 s per attempt (~13 s total, still ample for startup jitter), and the exhaustion message now names the likely cause and its one-line remedy. `docker stop` timing was measured on both sides for the new shape: with a declared long-running command deacon takes 10,176 ms / exit 137 and the reference 10,143 ms / exit 137 — identical, because `exec` replaces the wrapper and the SIGTERM handling belongs to the service's own process. With no declared command deacon is 129 ms / exit 0, so the trap still works where it applies. Unblocks T117's two held-back conformance cases (see #366). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017UCGzJEFZd85HJqz1Wx2kE --- crates/core/src/compose.rs | 367 +++++++++++++++--- crates/deacon/src/commands/up/compose.rs | 26 +- .../tests/smoke_compose_override_command.rs | 105 +++-- crates/deacon/tests/workspace_mounts.rs | 38 +- 4 files changed, 433 insertions(+), 103 deletions(-) diff --git a/crates/core/src/compose.rs b/crates/core/src/compose.rs index f1087644..34898ec2 100644 --- a/crates/core/src/compose.rs +++ b/crates/core/src/compose.rs @@ -576,6 +576,62 @@ impl ComposeCommand { let output = self.execute(&["config", "--format", "json"]).await?; parse_service_shape_from_config(&output, service_name) } + + /// The primary service's **declared** entrypoint, as the rendered compose config + /// reports it (`Ok(None)` when it declares none, or when the service is absent). + /// + /// Needed because deacon's keep-alive is injected as an `entrypoint:` that PREPENDS to + /// the declared one rather than replacing it (T117, see + /// [`ComposeProject::keepalive_entrypoint`]) — so the declared value has to be read + /// before the override can be written. The rendered config is used, not the raw compose + /// file, so `extends`/multi-file merging and interpolation are already applied. + pub async fn extract_service_entrypoint( + &self, + service_name: &str, + ) -> Result>> { + let output = self.execute(&["config", "--format", "json"]).await?; + parse_service_entrypoint_from_config(&output, service_name) + } +} + +/// Parse the declared `entrypoint` of `service_name` out of `docker compose config --format +/// json` output. Accepts both compose forms — a string (shell form) and an array (exec form) +/// — and normalizes to an argv vector. +fn parse_service_entrypoint_from_config( + json_output: &str, + service_name: &str, +) -> Result>> { + if json_output.trim().is_empty() { + return Ok(None); + } + let config: serde_json::Value = serde_json::from_str(json_output).map_err(|e| { + DockerError::CLIError(format!("Failed to parse compose config JSON: {}", e)) + })?; + let Some(entrypoint) = config + .get("services") + .and_then(|s| s.as_object()) + .and_then(|s| s.get(service_name)) + .and_then(|s| s.get("entrypoint")) + else { + return Ok(None); + }; + match entrypoint { + // Shell form: compose passes it to `/bin/sh -c`, so preserve that framing rather + // than word-splitting it here (splitting would break quoted arguments). + serde_json::Value::String(s) if !s.trim().is_empty() => Ok(Some(vec![ + "/bin/sh".to_string(), + "-c".to_string(), + s.clone(), + ])), + serde_json::Value::Array(items) => { + let argv: Vec = items + .iter() + .filter_map(|v| v.as_str().map(String::from)) + .collect(); + Ok(if argv.is_empty() { None } else { Some(argv) }) + } + _ => Ok(None), + } } /// Shape of a compose service relevant to the features-install pipeline. @@ -1007,8 +1063,25 @@ impl ComposeManager { project.name, services, gpu_mode ); + // Read the service's DECLARED entrypoint before writing the override: deacon's + // keep-alive prepends to it rather than replacing it (T117), so it has to be in + // hand. A failure to read is not fatal — the wrapper alone still keeps the container + // alive and still execs the declared COMMAND, so the fallback degrades to "a + // declared entrypoint is not preserved" rather than to "nothing runs". + let service_entrypoint = match command.extract_service_entrypoint(&project.service).await { + Ok(ep) => ep, + Err(e) => { + warn!( + "Could not read declared entrypoint for compose service '{}': {}. \ + Proceeding without preserving it.", + project.service, e + ); + None + } + }; + // Generate injection override if we have mounts or env to inject - let injection_override = project.generate_injection_override(); + let injection_override = project.generate_injection_override(service_entrypoint.as_deref()); command .up_with_injection(&services, true, gpu_mode, injection_override.as_deref()) @@ -1315,22 +1388,98 @@ impl ComposeProject { /// in the original compose files remain intact, and missing external volumes /// will surface as compose errors (not silently replaced with bind mounts). /// - /// Returns None if no mounts, env, or command override are needed. - #[must_use = "injection override should be passed to compose up"] - pub fn generate_injection_override(&self) -> Option { - // Spec default for overrideCommand is true: keep the container alive - // through the lifecycle so exec and post-create hooks can attach. - let override_cmd = self.override_command.unwrap_or(true); - - if self.additional_mounts.is_empty() - && self.additional_env.is_empty() - && !override_cmd - && self.service_image_override.is_none() - && self.deacon_labels.is_empty() - { - return None; + /// The `entrypoint:` line injected into the compose override: deacon's keep-alive + /// wrapper, followed by the service's own declared entrypoint (when it has one). + /// + /// # Why the entrypoint and not the command + /// + /// A devcontainer must stay alive long enough for lifecycle hooks and `exec` to attach, + /// AND — on the compose path, per `overrideCommand`'s default of `false` — must still run + /// the service's declared command. Putting the keep-alive in `command` cannot do both: + /// it discards the command it replaces (T117). + /// + /// The wrapper resolves it, and this shape is the reference's, measured against pinned + /// oracle 0.87.0 rather than guessed: + /// + /// ```text + /// entrypoint = [wrapper, "-"] ++ + /// command = (or [] when overrideCommand: true) + /// ``` + /// + /// Docker appends `Cmd` to `Entrypoint` as arguments, so inside the wrapper `"$@"` is + /// the declared entrypoint followed by the declared command. Three cases, all correct: + /// + /// - **declared command** → `exec "$@"` runs it, replacing the wrapper shell. The + /// container lives exactly as long as the service's own process, which is what + /// `overrideCommand: false` means. + /// - **nothing declared** → `"$@"` is empty, `exec` is a no-op, and the + /// `while sleep 1 & wait $!` loop keeps the container alive. + /// - **declared entrypoint** → it is prepended-to, never replaced, so a multi-stage + /// entrypoint (tini, a wrapper script) still receives the command as its args. Verified + /// against the reference, which concatenates identically: a fixture declaring both an + /// entrypoint and a command runs BOTH. + /// + /// `"-"` occupies `$0` so the first real argument lands in `$1`; without it `exec "$@"` + /// would silently drop the declared entrypoint's own program name. + /// + /// The `trap` + background + `wait` shape must MATCH `docker.rs`'s single-container + /// keep-alive: without it a foreground `sleep` as PID 1 cannot service SIGTERM, so + /// `docker stop` / `compose down` waits the full 10s grace period and then SIGKILLs + /// (measured at 10,258 ms against the reference's 215 ms before that fix). `sleep + /// infinity` falls back to `tail -f /dev/null` for BusyBox/Alpine. + /// + /// `$` is doubled (`$$@`, `$$!`) because compose interpolates `$` in the override YAML. + fn keepalive_entrypoint(service_entrypoint: Option<&[String]>) -> String { + // deacon's existing single-container keep-alive, with `exec "$@"` inserted ahead of + // it. When a command is declared `exec` replaces this shell and the keep-alive is + // never reached (correct: the service's own process owns the container's lifetime, + // and its own SIGTERM handling applies — the reference behaves identically). When + // nothing is declared `exec` no-ops and the trap + backgrounded sleep take over. + const WRAPPER: &str = "trap \\\"exit 0\\\" TERM INT; exec \\\"$$@\\\"; \ + (sleep infinity || tail -f /dev/null) & wait $$!"; + let mut parts = vec![ + "\"/bin/sh\"".to_string(), + "\"-c\"".to_string(), + format!("\"{WRAPPER}\""), + "\"-\"".to_string(), + ]; + for arg in service_entrypoint.unwrap_or(&[]) { + parts.push(format!( + "\"{}\"", + arg.replace('\\', "\\\\").replace('"', "\\\"") + )); } + format!(" entrypoint: [{}]\n", parts.join(", ")) + } + /// Returns None only when there is nothing at all to inject. + /// + /// `service_entrypoint` is the primary service's **declared** entrypoint as the + /// rendered compose config reports it ([`ComposeCommand::extract_service_entrypoint`]), + /// or `None` when it declares none. It is prepended-to rather than replaced — see + /// [`Self::keepalive_entrypoint`]. + #[must_use = "injection override should be passed to compose up"] + pub fn generate_injection_override( + &self, + service_entrypoint: Option<&[String]>, + ) -> Option { + // Spec default for `overrideCommand` is `true` for an image/Dockerfile and + // **`false` when referencing a Docker Compose file** (devcontainerjson-reference.md, + // which spells out the reason: "Set to `false` if the default command must run for + // the container to function properly"). This function is only ever reached for + // compose, so the default here is `false`. + // + // It read `unwrap_or(true)` until T117, so deacon replaced the service's declared + // command with its keep-alive and the service's own process never ran. Measured + // against pinned oracle 0.87.0: a service whose command creates a marker file and + // then sleeps produces the marker under the reference and not under deacon. The + // defect was invisible for the ubiquitous `command: ["sleep", "infinity"]` idiom, + // which is why every compose fixture in the suite passed. + let override_cmd = self.override_command.unwrap_or(false); + + // The keep-alive entrypoint is ALWAYS injected (see `keepalive_entrypoint`), so the + // override is never empty on the compose path. The early return survives only for + // callers that construct a project for inspection rather than for `up`. let mut yaml = String::from("services:\n"); yaml.push_str(&format!(" {}:\n", self.service)); @@ -1340,23 +1489,17 @@ impl ComposeProject { yaml.push_str(&format!(" image: \"{}\"\n", image.replace('"', "\\\""))); } + yaml.push_str(&Self::keepalive_entrypoint(service_entrypoint)); + if override_cmd { - // Mirror the single-container keep-alive used in docker.rs: - // sleep infinity (GNU coreutils), fall back to tail -f /dev/null - // for BusyBox/Alpine. Only command is overridden; the image's - // entrypoint is preserved so multi-stage entrypoints (e.g. tini) - // still receive our command as args. - // - // The `trap` + background + `wait` shape must MATCH docker.rs: without it a - // foreground `sleep` as PID 1 cannot service SIGTERM, so `docker stop` / - // `compose down` waits the full 10s grace period and then SIGKILLs (measured - // at 10,258 ms vs the reference CLI's 215 ms before the fix). Keeping the two - // paths symmetric is the point — a compose container that took 10s to stop - // while the single-container path took 245ms would be a silent asymmetry. - yaml.push_str( - " command: [\"/bin/sh\", \"-c\", \"trap \\\"exit 0\\\" TERM INT; \ - (sleep infinity || tail -f /dev/null) & wait $$!\"]\n", - ); + // `overrideCommand: true` on the compose path means "do NOT run the service's + // default command". The reference expresses that by CLEARING the command so the + // entrypoint wrapper's `exec "$@"` has nothing to exec and falls through to the + // keep-alive loop — measured: with `overrideCommand: true` the reference leaves + // the same entrypoint in place and reports `Cmd: []`. Emptying the command is + // therefore the whole of the override; the keep-alive itself lives in the + // entrypoint either way. + yaml.push_str(" command: []\n"); } if !self.additional_env.is_empty() { @@ -2213,8 +2356,18 @@ mod tests { deacon_labels: IndexMap::new(), }; - // No mounts or env, should return None - assert!(project.generate_injection_override().is_none()); + // T117: with nothing else to inject the override is NOT empty — it still carries + // the keep-alive `entrypoint`, which a compose devcontainer always needs so the + // container survives for lifecycle hooks and `exec`. What `overrideCommand: false` + // suppresses is the `command:` line, so the service's declared command still runs. + let yaml = project + .generate_injection_override(None) + .expect("the keep-alive entrypoint is always injected"); + assert!(yaml.contains("entrypoint:"), "{yaml}"); + assert!( + !yaml.contains("command:"), + "overrideCommand=false must not replace the service's command:\n{yaml}" + ); } #[test] @@ -2239,7 +2392,7 @@ mod tests { deacon_labels: IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); assert!(override_yaml.contains("services:")); assert!(override_yaml.contains("myservice:")); assert!(override_yaml.contains("environment:")); @@ -2280,7 +2433,7 @@ mod tests { deacon_labels: IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); assert!(override_yaml.contains("services:")); assert!(override_yaml.contains("myservice:")); assert!(override_yaml.contains("volumes:")); @@ -2312,7 +2465,7 @@ mod tests { deacon_labels: IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); assert!(override_yaml.contains("volumes:")); assert!(override_yaml.contains("- type: tmpfs")); assert!(override_yaml.contains("target: /mnt/config-tmp")); @@ -2355,7 +2508,7 @@ mod tests { deacon_labels: IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); // The per-service `volumes:` list still gets the short-form mount… assert!(override_yaml.contains("feat-probe-vol:/feat-mnt")); @@ -2397,7 +2550,7 @@ mod tests { deacon_labels: IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); // Should have both environment and volumes sections assert!(override_yaml.contains("environment:")); @@ -2430,7 +2583,7 @@ mod tests { deacon_labels: IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); // Verify proper escaping assert!(override_yaml.contains("MULTILINE: \"line1\\nline2\"")); @@ -2463,7 +2616,7 @@ mod tests { deacon_labels: IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); // IndexMap preserves insertion order: ZZZ, AAA, MMM (not sorted alphabetically) let zzz_pos = override_yaml.find("ZZZ:").unwrap(); @@ -2504,7 +2657,7 @@ mod tests { deacon_labels: labels, }; - let yaml = project.generate_injection_override().unwrap(); + let yaml = project.generate_injection_override(None).unwrap(); // Exactly one `app:` mapping, exactly one `db:` mapping. assert_eq!( @@ -2521,8 +2674,14 @@ mod tests { #[test] fn test_generate_injection_override_command_default_on() { - // Spec default: override_command unset (None) is treated as true, - // so an otherwise-empty override still injects the keep-alive command. + // T117: `override_command` unset means the COMPOSE default, which the spec states is + // **false** — "Defaults to `true` for when using an image Dockerfile and `false` when + // referencing a Docker Compose file", because "the default command must run for the + // container to function properly". This test previously asserted the opposite while + // citing "Spec default", which is how the defect survived. + // + // What is still unconditional is the keep-alive ENTRYPOINT: the container must + // outlive `up` for lifecycle hooks and `exec` whether or not the command is replaced. let project = ComposeProject { name: "test".to_string(), base_path: PathBuf::from("/test"), @@ -2540,7 +2699,7 @@ mod tests { }; let override_yaml = project - .generate_injection_override() + .generate_injection_override(None) .expect("override command should produce override yaml even with no env/mounts"); assert!(override_yaml.contains("services:\n app:\n")); @@ -2555,12 +2714,108 @@ mod tests { override_yaml.contains("(sleep infinity || tail -f /dev/null) & wait $$!"), "keep-alive must background the sleep and wait on it: {override_yaml}" ); + // The keep-alive is an `entrypoint:`, and it does NOT replace the command. + assert!( + override_yaml.contains(" entrypoint: ["), + "the keep-alive must be injected as an entrypoint: {override_yaml}" + ); + assert!( + !override_yaml.contains("command:"), + "the compose default (overrideCommand=false) must leave the service's command \ + alone — replacing it is what stopped the service's own process from ever \ + running (T117): {override_yaml}" + ); + } + + /// T117: the wrapper PREPENDS to a declared entrypoint rather than replacing it, so a + /// multi-stage entrypoint (tini, a wrapper script) still receives the command as args. + /// The reference concatenates identically — measured on a fixture declaring both an + /// entrypoint and a command, where BOTH ran. + #[test] + fn keepalive_entrypoint_prepends_to_a_declared_entrypoint() { + let declared = vec!["/usr/bin/tini".to_string(), "--".to_string()]; + let line = ComposeProject::keepalive_entrypoint(Some(&declared)); + assert!( + line.contains("\"/usr/bin/tini\", \"--\"]"), + "the declared entrypoint must survive, appended after the wrapper: {line}" + ); + // `-` occupies $0 so the declared program name lands in $1 and `exec "$@"` keeps it. + assert!(line.contains("\"-\", \"/usr/bin/tini\""), "{line}"); + + // With nothing declared the wrapper stands alone and the keep-alive loop is reached. + let bare = ComposeProject::keepalive_entrypoint(None); + assert!(bare.trim_end().ends_with("\"-\"]"), "{bare}"); + } + + /// T117: `overrideCommand: true` on the compose path is expressed by CLEARING the + /// command, not by replacing it with the keep-alive — measured against pinned oracle + /// 0.87.0, which leaves the same entrypoint in place and reports `Cmd: []`. + #[test] + fn override_command_true_clears_the_command_rather_than_replacing_it() { + let project = ComposeProject { + name: "test".to_string(), + base_path: PathBuf::from("/test"), + compose_files: vec![PathBuf::from("docker-compose.yml")], + service: "app".to_string(), + run_services: Vec::new(), + env_files: Vec::new(), + additional_mounts: Vec::new(), + profiles: Vec::new(), + additional_env: IndexMap::new(), + external_volumes: Vec::new(), + override_command: Some(true), + service_image_override: None, + deacon_labels: IndexMap::new(), + }; + let yaml = project.generate_injection_override(None).expect("override"); + assert!( + yaml.contains(" command: []\n"), + "overrideCommand=true must EMPTY the command so `exec \"$@\"` falls through to \ + the keep-alive loop: {yaml}" + ); + assert!(yaml.contains(" entrypoint: ["), "{yaml}"); + } + + #[test] + fn declared_entrypoint_is_read_from_the_rendered_compose_config() { + // Exec form. + let json = r#"{"services":{"app":{"entrypoint":["/usr/bin/tini","--"]}}}"#; + assert_eq!( + parse_service_entrypoint_from_config(json, "app").unwrap(), + Some(vec!["/usr/bin/tini".to_string(), "--".to_string()]) + ); + // Shell form: compose hands it to `/bin/sh -c`, so that framing is preserved rather + // than word-split (splitting would break quoted arguments). + let shell = r#"{"services":{"app":{"entrypoint":"echo hi && exec \"$@\""}}}"#; + assert_eq!( + parse_service_entrypoint_from_config(shell, "app").unwrap(), + Some(vec![ + "/bin/sh".to_string(), + "-c".to_string(), + "echo hi && exec \"$@\"".to_string() + ]) + ); + // Absent / empty / unknown service → None, never a spurious empty argv. + for (j, svc) in [ + (r#"{"services":{"app":{}}}"#, "app"), + (r#"{"services":{"app":{"entrypoint":[]}}}"#, "app"), + (r#"{"services":{"app":{"entrypoint":" "}}}"#, "app"), + (r#"{"services":{"other":{"entrypoint":["x"]}}}"#, "app"), + ("", "app"), + ] { + assert_eq!( + parse_service_entrypoint_from_config(j, svc).unwrap(), + None, + "expected None for {j}" + ); + } } #[test] fn test_generate_injection_override_command_explicit_false() { - // overrideCommand=false must run the service's natural command; - // with no env/mounts the override yaml is None. + // overrideCommand=false must run the service's natural command. The override is + // still emitted (the keep-alive entrypoint is unconditional, T117); what must be + // absent is the `command:` line that would replace the declared command. let project = ComposeProject { name: "test".to_string(), base_path: PathBuf::from("/test"), @@ -2577,7 +2832,17 @@ mod tests { deacon_labels: IndexMap::new(), }; - assert!(project.generate_injection_override().is_none()); + let yaml = project + .generate_injection_override(None) + .expect("the keep-alive entrypoint is always injected"); + assert!( + !yaml.contains("command:"), + "an explicit overrideCommand=false must leave the service's command alone:\n{yaml}" + ); + assert!( + yaml.contains("exec \\\"$$@\\\""), + "the wrapper must exec the declared entrypoint+command:\n{yaml}" + ); } #[test] @@ -2602,7 +2867,7 @@ mod tests { deacon_labels: IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); // The keep-alive must carry the SIGTERM trap and the background+`wait` shape, // not a bare foreground `sleep`: without them PID 1 cannot service SIGTERM and @@ -3021,7 +3286,7 @@ mod tests { deacon_labels: IndexMap::new(), }; let yaml = project - .generate_injection_override() + .generate_injection_override(None) .expect("override emitted"); assert!( yaml.contains("image: \"deacon-features:abc123\""), @@ -3053,7 +3318,7 @@ mod tests { service_image_override: Some("img:tag".into()), deacon_labels: IndexMap::new(), }; - assert!(project.generate_injection_override().is_some()); + assert!(project.generate_injection_override(None).is_some()); } /// BEAD-14a-T10: image tags with embedded double-quotes are escaped so they @@ -3075,7 +3340,7 @@ mod tests { service_image_override: Some(r#"weird"tag"#.into()), deacon_labels: IndexMap::new(), }; - let yaml = project.generate_injection_override().unwrap(); + let yaml = project.generate_injection_override(None).unwrap(); assert!(yaml.contains(r#"image: "weird\"tag""#)); } } diff --git a/crates/deacon/src/commands/up/compose.rs b/crates/deacon/src/commands/up/compose.rs index 2f0d6ef7..de5eb347 100644 --- a/crates/deacon/src/commands/up/compose.rs +++ b/crates/deacon/src/commands/up/compose.rs @@ -32,10 +32,19 @@ use std::path::Path; use std::time::Duration; use tracing::{debug, info, instrument, warn}; -/// Resolve the primary service container ID, retrying with exponential backoff +/// Resolve the primary service container ID, retrying with capped exponential backoff /// to absorb the brief window between `docker compose up` returning and the /// container appearing in `docker compose ps`. Shared by the host-CA injection /// step (before post-create) and the final result assembly. +/// +/// The per-attempt delay is CAPPED. Uncapped doubling reached a 51-second single sleep and +/// ~102 s in total — absurd for absorbing a startup race, and newly reachable since T117 +/// made deacon honor `overrideCommand`'s compose default: a service whose declared command +/// exits now legitimately produces no running container, and a user should not wait 102 s to +/// be told so. The capped schedule still covers ~13 s of startup jitter. +/// +/// The exhaustion message names that cause, because it is by far the most likely one and it +/// has a one-line remedy. async fn resolve_primary_container_id_with_retry( compose_manager: &ComposeManager, project: &ComposeProject, @@ -43,13 +52,18 @@ async fn resolve_primary_container_id_with_retry( let mut attempts = 0; const MAX_ATTEMPTS: u32 = 10; const INITIAL_DELAY_MS: u64 = 100; + const MAX_DELAY_MS: u64 = 2_000; loop { match compose_manager.get_primary_container_id(project).await? { Some(id) => break Ok(id), None if attempts < MAX_ATTEMPTS => { attempts += 1; - let delay = Duration::from_millis(INITIAL_DELAY_MS * 2u64.pow(attempts - 1)); + let delay = Duration::from_millis( + INITIAL_DELAY_MS + .saturating_mul(2u64.saturating_pow(attempts - 1)) + .min(MAX_DELAY_MS), + ); debug!( "Waiting for container to be ready, attempt {}/{}, waiting {:?}", attempts, MAX_ATTEMPTS, delay @@ -58,7 +72,13 @@ async fn resolve_primary_container_id_with_retry( } None => { break Err(anyhow::anyhow!( - "Failed to get primary container ID after starting compose project (tried {} times)", + "No running container for compose service '{}' after starting the project \ + (tried {} times). The most common cause is that the service's own command \ + exited: on the compose path `overrideCommand` defaults to false, so the \ + declared command runs and the container lives only as long as it does. \ + Set \"overrideCommand\": true in devcontainer.json to keep the container \ + alive instead.", + project.service, MAX_ATTEMPTS )); } diff --git a/crates/deacon/tests/smoke_compose_override_command.rs b/crates/deacon/tests/smoke_compose_override_command.rs index 26937b58..127f4449 100644 --- a/crates/deacon/tests/smoke_compose_override_command.rs +++ b/crates/deacon/tests/smoke_compose_override_command.rs @@ -1,9 +1,23 @@ -//! Integration tests for compose overrideCommand support (Bead 13). +//! Integration tests for compose overrideCommand support (Bead 13, corrected by T117). //! //! Covers BEAD-13-T01, T02, T04 from .maverick/plans/consumer-pt2/briefing.md: -//! - T01: overrideCommand=true (default) keeps a short-lived compose service alive -//! - T02: overrideCommand=false runs the service's natural command (may exit) -//! - T04: lifecycle commands execute successfully in compose mode with override active +//! - T01: on compose, the DEFAULT runs the service's declared command (spec default `false`) +//! - T02: an explicit `overrideCommand: false` runs the service's natural command +//! - T04: lifecycle commands execute in compose mode when `overrideCommand: true` keeps the +//! container alive +//! +//! **T117 corrected T01 and T04's premise.** Both were written asserting that the compose +//! default is `overrideCommand: true` — that deacon keeps a service whose command is +//! `echo hello` alive by replacing that command. The spec says the opposite: +//! `overrideCommand` *"Defaults to `true` for when using an image Dockerfile and `false` when +//! referencing a Docker Compose file"*, because *"the default command must run for the +//! container to function properly"*. +//! +//! Verified against the pinned oracle 0.87.0 rather than argued: on the `echo hello` fixture +//! `devcontainer up` **fails** with `{"outcome":"error"}` and exit 1, its container `exited` +//! with code 0 — because the declared command ran and finished. deacon now does the same. +//! These two tests were asserting deacon's defect, which is why the defect survived: any +//! compose service whose command matters never ran. //! //! These hit a real Docker daemon and are docker-gated via a graceful skip. @@ -54,18 +68,6 @@ fn up_container_id(up_output: &std::process::Output) -> Option { .map(|s| s.to_string()) } -fn docker_inspect_state_running(container_id: &str) -> Option { - let output = std::process::Command::new("docker") - .args(["inspect", "--format", "{{.State.Running}}", container_id]) - .output() - .ok()?; - if !output.status.success() { - return None; - } - let text = String::from_utf8_lossy(&output.stdout).trim().to_string(); - Some(text == "true") -} - fn docker_inspect_cmd(container_id: &str) -> Option { let output = std::process::Command::new("docker") .args(["inspect", "--format", "{{json .Config.Cmd}}", container_id]) @@ -77,9 +79,25 @@ fn docker_inspect_cmd(container_id: &str) -> Option { Some(String::from_utf8_lossy(&output.stdout).trim().to_string()) } -/// BEAD-13-T01: default overrideCommand keeps a short-lived service running. +/// BEAD-13-T01 (corrected, T117): on compose the DEFAULT runs the service's declared +/// command — it is not replaced by deacon's keep-alive. +/// +/// The spec default for compose is `overrideCommand: false`, and this fixture omits the key +/// entirely, so the declared command must run. It proves that by leaving a marker file. +/// +/// The test previously asserted the opposite — that a service whose command is `echo hello` +/// stays ALIVE, which is only true if the command is discarded. That premise was deacon's +/// defect, and it is why the defect survived: verified against the pinned oracle 0.87.0, the +/// reference FAILS that fixture (`{"outcome":"error"}`, exit 1) because it honors the +/// default and the container exits with its command. +/// +/// The assertion is deliberately on the MARKER, not on `up` failing with a short-lived +/// command. Whether a millisecond-lived container is still present when deacon looks for it +/// is a race — a first draft of this test asserting `up` fails was flaky under parallel load +/// for exactly that reason. "Did the declared command run?" is the actual claim and is +/// deterministic. #[test] -fn test_compose_override_command_default_keeps_service_alive() { +fn test_compose_default_runs_the_declared_command() { if !is_docker_available() { eprintln!("Skipping: Docker not available"); return; @@ -87,12 +105,15 @@ fn test_compose_override_command_default_keeps_service_alive() { let temp_dir = TempDir::new().unwrap(); let workspace = temp_dir.path(); - // Compose service runs `echo hello` — would exit in milliseconds without override. + // Long enough to inspect, and it records that it ran. `sleep infinity` would be + // indistinguishable from deacon's own keep-alive; the marker is what makes this test + // able to fail. let compose_yml = r#"services: app: image: alpine:3.18 - command: ["echo", "hello"] + command: ["sh", "-c", "touch /tmp/declared-command-ran; sleep 60"] "#; + // NOTE: no `overrideCommand` key — the point is the DEFAULT. let devcontainer_json = r#"{ "name": "Compose Override Default", "dockerComposeFile": "../docker-compose.yml", @@ -117,22 +138,37 @@ fn test_compose_override_command_default_keeps_service_alive() { .unwrap(); let stderr = String::from_utf8_lossy(&up_output.stderr).to_string(); - let success = up_output.status.success(); - - if !success { + if !up_output.status.success() { deacon_down(workspace); panic!("deacon up failed: {}", stderr); } let container_id = up_container_id(&up_output).expect("deacon up should report a containerId"); - let running = docker_inspect_state_running(&container_id).unwrap_or(false); + let marker = std::process::Command::new("docker") + .args([ + "exec", + &container_id, + "test", + "-f", + "/tmp/declared-command-ran", + ]) + .output() + .unwrap(); + let cmd_json = docker_inspect_cmd(&container_id).unwrap_or_default(); deacon_down(workspace); assert!( - running, - "container should still be running with default overrideCommand=true; stderr was: {}", - stderr + marker.status.success(), + "the compose service's DECLARED command must run under the default \ + (overrideCommand defaults to false for compose) — no marker means deacon replaced \ + it, so the service's own process never ran (T117). Container Cmd was: {cmd_json}; \ + stderr from up: {stderr}" + ); + assert!( + !cmd_json.contains("sleep infinity"), + "the container's Cmd must still be the service's declared command, not deacon's \ + keep-alive; got: {cmd_json}" ); } @@ -201,7 +237,14 @@ fn test_compose_override_command_explicit_false_runs_natural_command() { ); } -/// BEAD-13-T04: lifecycle commands execute in compose mode with override active. +/// BEAD-13-T04 (corrected, T117): lifecycle commands execute in compose mode when +/// `overrideCommand: true` keeps the container alive. +/// +/// The fixture now sets `overrideCommand: true` EXPLICITLY. It previously relied on the +/// default doing so, which is the T117 defect: on compose the default is `false`, so +/// `echo init` runs, the container exits, and no lifecycle hook can attach — the reference +/// fails this fixture too. Asking for the keep-alive is what the spec's `true` is for, and +/// the test's real subject is that lifecycle hooks run once the container IS alive. #[test] fn test_compose_override_command_lifecycle_runs() { if !is_docker_available() { @@ -211,8 +254,9 @@ fn test_compose_override_command_lifecycle_runs() { let temp_dir = TempDir::new().unwrap(); let workspace = temp_dir.path(); - // Without our override, `echo init` would exit before postCreateCommand - // could run. With override active, the marker file proves the lifecycle ran. + // `echo init` exits immediately, so `overrideCommand: true` — which CLEARS the command + // and lets the keep-alive entrypoint hold the container open — is required for any + // lifecycle hook to run at all. The marker file proves it ran. let compose_yml = r#"services: app: image: alpine:3.18 @@ -223,6 +267,7 @@ fn test_compose_override_command_lifecycle_runs() { "dockerComposeFile": "../docker-compose.yml", "service": "app", "workspaceFolder": "/workspace", + "overrideCommand": true, "postCreateCommand": "touch /tmp/deacon-lifecycle-marker" }"#; diff --git a/crates/deacon/tests/workspace_mounts.rs b/crates/deacon/tests/workspace_mounts.rs index 5ca3bae5..945bb8fd 100644 --- a/crates/deacon/tests/workspace_mounts.rs +++ b/crates/deacon/tests/workspace_mounts.rs @@ -239,7 +239,7 @@ mod compose_consistency_tests { }; let override_yaml = project - .generate_injection_override() + .generate_injection_override(None) .expect("Should generate override with mounts"); // Verify the YAML contains the mount @@ -283,8 +283,8 @@ mod compose_consistency_tests { }; // Generate override multiple times and verify determinism - let override1 = project.generate_injection_override().unwrap(); - let override2 = project.generate_injection_override().unwrap(); + let override1 = project.generate_injection_override(None).unwrap(); + let override2 = project.generate_injection_override(None).unwrap(); assert_eq!( override1, override2, @@ -317,7 +317,7 @@ mod compose_consistency_tests { deacon_labels: deacon_core::IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); // Without consistency specified, the mount should not have :cached, :consistent, or :delegated assert!( @@ -359,7 +359,7 @@ mod compose_consistency_tests { deacon_labels: deacon_core::IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); assert!( override_yaml.contains("/host/external:/external:ro"), @@ -899,7 +899,7 @@ mod default_workspace_discovery_tests { }; let override_yaml = project - .generate_injection_override() + .generate_injection_override(None) .expect("Should generate override with mounts"); // Verify the YAML contains the mount in default format @@ -1044,7 +1044,7 @@ mod compose_git_root_tests { }; let override_yaml = project - .generate_injection_override() + .generate_injection_override(None) .expect("Should generate override with workspace mount"); // Verify the mount uses the git-root path @@ -1092,7 +1092,7 @@ mod compose_git_root_tests { }; let override_yaml = project - .generate_injection_override() + .generate_injection_override(None) .expect("Should generate override with workspace mount"); // Verify the mount does NOT use the subdir path @@ -1151,7 +1151,7 @@ mod compose_git_root_tests { ); // Generate override and verify the mount path - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); // The injection override targets the primary service assert!( @@ -1196,7 +1196,7 @@ mod compose_git_root_tests { deacon_labels: deacon_core::IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); // Mount should use the same path assert!( @@ -1242,7 +1242,7 @@ mod compose_git_root_consistency_tests { deacon_labels: deacon_core::IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); // Should have both git-root path and consistency assert!( @@ -1280,7 +1280,7 @@ mod compose_git_root_consistency_tests { deacon_labels: deacon_core::IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); // Should have both git-root path and consistency assert!( @@ -1318,7 +1318,7 @@ mod compose_git_root_consistency_tests { deacon_labels: deacon_core::IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); // Should have both git-root path and consistency assert!( @@ -1357,7 +1357,7 @@ mod compose_git_root_consistency_tests { deacon_labels: deacon_core::IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); assert!( override_yaml.contains(&format!("{}:{}:{}", git_root, target_path, consistency)), @@ -1396,7 +1396,7 @@ mod compose_git_root_consistency_tests { deacon_labels: deacon_core::IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); // Should have both :ro and :cached options assert!( @@ -1434,7 +1434,7 @@ mod compose_git_root_consistency_tests { deacon_labels: deacon_core::IndexMap::new(), }; - let override_yaml = project.generate_injection_override().unwrap(); + let override_yaml = project.generate_injection_override(None).unwrap(); // Should have mount without any options suffix (no trailing colon after target) let expected_mount = format!("{}:{}\n", git_root, target_path); @@ -1610,13 +1610,13 @@ mod performance_tests { }; // Warm-up call - let _ = project.generate_injection_override(); + let _ = project.generate_injection_override(None); // Measure Compose override generation time let iterations = 100; let start = Instant::now(); for _ in 0..iterations { - let result = project.generate_injection_override(); + let result = project.generate_injection_override(None); assert!(result.is_some(), "Should generate override"); } let elapsed = start.elapsed(); @@ -1691,7 +1691,7 @@ mod performance_tests { }; // Step 5: Generate Compose override - let _override_yaml = compose_project.generate_injection_override(); + let _override_yaml = compose_project.generate_injection_override(None); } let elapsed = start.elapsed(); let avg_duration = elapsed / iterations;