diff --git a/README.md b/README.md index e0ba632..a0dc9f3 100644 --- a/README.md +++ b/README.md @@ -98,7 +98,7 @@ Three panes, and one action per pane that opens it from anywhere: |---|---|---| | `shell` | `devcontainer.open-shell` | The container user's shell, interactive, inside the repository's Dev Container | | `command` | `devcontainer.open-command` | Runs the configured `command` payload through that same interactive shell; default `claude` | -| `stop` | `devcontainer.open-stop` | Popup that identifies the repository's container, names it, asks for confirmation, and stops it | +| `stop` | `devcontainer.open-stop` | Popup that identifies the repository's container — and, for a Compose-based Dev Container, every service alongside it — names them, asks for confirmation, and stops them | Open a pane directly: @@ -118,8 +118,24 @@ herdr plugin action invoke open-stop --plugin devcontainer Opening a shell or command pane is the explicit lifecycle trigger. Container lifecycle is never attached to repository events, nothing is stopped -automatically, and the plugin never runs `docker rm` — `stop` stops a container, -it does not remove one. +automatically, and the plugin never runs `docker rm` — `stop` stops containers, +it does not remove them. + +For a Compose-based Dev Container, stop takes down the **whole Compose +project**, not just the service the pane runs in. That is what +`devcontainer.json`'s default `shutdownAction` of `stopCompose` asks for, and +stopping the app while its database and cache keep running is rarely what +"stop the Dev Container" meant. Project membership is read from the container's +own `com.docker.compose.project` label — the same label Compose itself uses — +so nothing here re-derives the project name from `devcontainer.json`. The +confirmation lists every container it will stop, by service, name, and id. + +Containers started by `docker compose run` are left alone, matching +`docker compose stop`. The Dev Container's own service is stopped and waited on +before the rest, so it releases its database connections first. If the Dev +Container has exited on its own while its services keep running, stop says so +and offers to stop what is left — an absent Dev Container is not an absent +project. ## Why use it @@ -135,8 +151,9 @@ it does not remove one. once cannot race through bring-up. - **Deterministic container selection.** If more than one running container claims the repository, the plugin lists them and refuses to guess. -- **Explicit, confirmed shutdown.** Stop is its own action, names the target, - and proceeds only on `y` or `yes`. +- **Explicit, confirmed shutdown.** Stop is its own action, names every + container it will stop — the whole Compose project, where there is one — and + proceeds only on `y` or `yes`. ## Keybindings diff --git a/docs/superpowers/specs/2026-08-11-herdr-devcontainer-plugin-design.md b/docs/superpowers/specs/2026-08-11-herdr-devcontainer-plugin-design.md index 6f0e978..f2164a7 100644 --- a/docs/superpowers/specs/2026-08-11-herdr-devcontainer-plugin-design.md +++ b/docs/superpowers/specs/2026-08-11-herdr-devcontainer-plugin-design.md @@ -494,3 +494,62 @@ Docker ANDs repeated `--filter label` arguments, so this is a second `docker ps` whose results are unioned and collapsed by container id — without the dedupe, a CLI-created container matching both keys would look like two running containers and trip the refuse-to-choose error. + +## Amendment (2026-08-13, compose-aware stop) + +**Stop targets the Compose project, not one service.** The design above stops +"the container" — a single `docker stop ` against the one container +discovery matched. For a Compose-based Dev Container that is one service of +several: stopping `double-holo-ui` took down its app and left postgres, redis, +gotrue, supabase-gateway, and inbucket running. The Dev Container spec says the +same thing the user expects — `shutdownAction` defaults to `stopCompose` for +Compose configs — and it is what VS Code does on disconnect. + +Membership is read from the container's own `com.docker.compose.project` label. +Compose writes that label at creation and locates its own containers by it, so +the labels are the authority. Deriving the project name from `devcontainer.json` +was rejected for the same reason `remoteEnv` was: it would re-implement a value +another tool already computed, and would be wrong wherever the two disagree. + +`docker stop ...` rather than `docker compose stop`. Preflight verifies +`docker` and nothing else; `docker compose` is a separate plugin that may be +absent or v1, and discovery already produced the ids. + +The Dev Container's own service is stopped first, in **its own `docker stop` +call**, and waited on before the rest go down. This detail was learned the +expensive way: `docker stop a b c` stops the named containers in *parallel* — +argument order only controls the order results print. Measured on docker 29.7.2 +with two containers that ignore SIGTERM, `docker stop a b` took 12.9s, one grace +window rather than two. So an ordered argument list bought nothing: the database +received SIGTERM at the same instant as the Dev Container still holding +connections to it. Two calls deliver what one ordered call only appeared to. +The cost is bounded — each call is one grace window, so the budget is a flat 30s +per call rather than scaling with the number of services. Deeper ordering +through `com.docker.compose.depends_on` is still not attempted: in a Dev +Container project every other service is a dependency of the Dev Container, so +one level is the whole ordering. + +One-off containers are excluded. `docker compose run` tags them +`com.docker.compose.oneoff=True` and `docker compose stop` leaves them alone; +sweeping them in would kill a `docker compose run --rm app pytest` a developer +is watching in another pane. Docker has no negated label filter, so the row is +dropped after parsing — and a row whose oneoff label is *missing* is kept, since +an absent label is not a claim. + +Three consequences follow from stop becoming more destructive: + +- The confirmation names every container, by service, name, and id, rather than + counting them. It is the only thing between a mis-keyed binding and a stopped + database, so the list has to be readable at a glance. A single-container repo + renders exactly as it did before; nothing about it changed. +- Whether the stop worked is **asked of docker**, not inferred from + `docker stop`'s output. Reading its stdout and pattern-matching its stderr for + "no such container" meant deciding whether someone's database was down by + matching English prose — and on timeout the CLI is killed, so its output says + nothing at all about what landed. A follow-up `docker ps` filtered to exactly + the target ids answers directly; anything still running is named in the error. +- An absent Dev Container no longer implies an absent project. The app container + can exit on its own — a crash, an OOM kill, a stop from another pane — while + postgres and redis keep running. Reporting "no running dev container" there is + the same false absence in mirror image, so the project is checked from the + exited container's own label, and its survivors are offered for stopping. diff --git a/src/compose.rs b/src/compose.rs new file mode 100644 index 0000000..71b9263 --- /dev/null +++ b/src/compose.rs @@ -0,0 +1,447 @@ +//! Compose-project awareness for stop. +//! +//! A compose-based dev container is one service of a project: stopping only it +//! leaves its database, cache, and gateway running, which is not what "stop the +//! dev container" means to anyone who asked for it. The Dev Container spec says +//! the same — `shutdownAction` defaults to `stopCompose` for compose configs. +//! +//! Membership comes from the container's own labels rather than from +//! `devcontainer.json`. Docker Compose writes `com.docker.compose.project` at +//! creation and finds its containers by it, so the labels are the authority +//! here; parsing the config would mean re-deriving a project name Compose +//! already computed, and this codebase does not re-implement `devcontainer.json`. + +use std::time::Duration; + +use crate::error::Error; +use crate::run::{run, StderrMode}; +use crate::util::tail; + +pub const PROJECT_LABEL: &str = "com.docker.compose.project"; +pub const SERVICE_LABEL: &str = "com.docker.compose.service"; +pub const ONEOFF_LABEL: &str = "com.docker.compose.oneoff"; + +pub fn project_argv(container_id: &str) -> Vec { + vec![ + "docker".to_string(), + "inspect".to_string(), + "--format".to_string(), + format!("{{{{index .Config.Labels \"{PROJECT_LABEL}\"}}}}"), + container_id.to_string(), + ] +} + +/// One running container of a compose project. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct Member { + pub id: String, + pub name: String, + pub service: String, +} + +pub fn members_argv(project: &str) -> Vec { + vec![ + "docker".to_string(), + "ps".to_string(), + "--filter".to_string(), + format!("label={PROJECT_LABEL}={project}"), + "--format".to_string(), + format!( + "{{{{.ID}}}}\t{{{{.Names}}}}\t{{{{.Label \"{SERVICE_LABEL}\"}}}}\t{{{{.Label \"{ONEOFF_LABEL}\"}}}}" + ), + ] +} + +/// Parse the member listing, refusing rows we cannot read. +/// +/// Discovery's rule applies with sharper stakes: dropping an unreadable row +/// here would leave that container running while the user is told the project +/// stopped. +/// +/// One-off containers are excluded. `docker compose run` tags them +/// `oneoff=True` and `docker compose stop` leaves them alone, so stopping them +/// would kill a test run someone is watching in another pane. A row whose +/// oneoff label is *missing* is kept — an absent label is not a claim. +pub fn parse_members(stdout: &str) -> Result, Error> { + let mut out = Vec::new(); + for line in stdout.lines() { + if line.trim().is_empty() { + continue; + } + let fields: Vec<&str> = line.split('\t').map(str::trim).collect(); + if fields.len() != 4 || fields[0].is_empty() || fields[1].is_empty() { + return Err(Error::MalformedDockerOutput { + line: line.to_string(), + }); + } + if fields[3].eq_ignore_ascii_case("true") { + continue; + } + out.push(Member { + id: fields[0].to_string(), + name: fields[1].to_string(), + service: fields[2].to_string(), + }); + } + Ok(out) +} + +/// Order the stop so the dev container goes first. +/// +/// It is the dependent — the thing holding connections to the database and +/// cache — so stopping it before them is the graceful direction, the same +/// reason Compose shuts a project down in reverse dependency order. Deeper +/// ordering via `com.docker.compose.depends_on` is deliberately not attempted: +/// in a dev container project every other service is a dependency of the dev +/// container, so one level is the whole ordering. +/// +/// `dev_id` leads even when the listing does not contain it. The dev container +/// is always a member of its own project, so a listing without it means either +/// the two queries disagreed or it exited on its own — and in both cases +/// dropping the one container the user named is the wrong repair. `dev_name` +/// is carried onto that synthetic row because the confirmation prompt has to +/// be able to name every container it is about to stop. +pub fn order_for_stop(members: Vec, dev_id: &str, dev_name: &str) -> Vec { + let (mut dev, rest): (Vec, Vec) = + members.into_iter().partition(|m| m.id == dev_id); + if dev.is_empty() { + dev.push(Member { + id: dev_id.to_string(), + name: dev_name.to_string(), + service: String::new(), + }); + } + dev.into_iter().chain(rest).collect() +} + +/// Which of `ids` are still running, asked of docker rather than inferred. +/// +/// The alternative was reading `docker stop`'s stdout and pattern-matching its +/// stderr for "no such container" — inferring the world from a CLI's prose, in +/// a place where being wrong means telling someone their database is down while +/// it is serving. This asks instead. It also covers the timeout case, where the +/// CLI was killed mid-flight and its output says nothing about what landed. +pub fn running_argv(ids: &[String]) -> Vec { + let mut argv = vec!["docker".to_string(), "ps".to_string()]; + // Same-type filters are OR'd, so this is one question about the whole set. + for id in ids { + argv.push("--filter".to_string()); + argv.push(format!("id={id}")); + } + argv.push("--format".to_string()); + argv.push("{{.ID}}".to_string()); + argv +} + +pub fn parse_running_ids(stdout: &str) -> Vec { + stdout + .lines() + .map(str::trim) + .filter(|l| !l.is_empty()) + .map(str::to_string) + .collect() +} + +/// The subset of `members` that is still running. +pub fn still_running(members: &[Member]) -> Result, Error> { + if members.is_empty() { + return Ok(Vec::new()); + } + let ids: Vec = members.iter().map(|m| m.id.clone()).collect(); + let out = docker_stdout(&running_argv(&ids))?; + let alive = parse_running_ids(&out); + Ok(members + .iter() + .filter(|m| alive.iter().any(|id| id == &m.id)) + .cloned() + .collect()) +} + +/// Everything that should stop when the user stops `dev_id`, dev container +/// first. +/// +/// A container outside a compose project yields just itself, so the plain +/// single-container path is unchanged. +/// +/// A failed project lookup is *not* downgraded to "just this container". +/// Stopping one service of a project the user believes is fully down is the +/// failure this change exists to remove, so an unreadable project name or +/// member list is an error the user can see rather than a quietly smaller stop. +pub fn stop_set(dev_id: &str, dev_name: &str) -> Result, Error> { + let alone = || { + vec![Member { + id: dev_id.to_string(), + name: dev_name.to_string(), + service: String::new(), + }] + }; + + let Some(project) = project_of(dev_id)? else { + return Ok(alone()); + }; + + let members = parse_members(&docker_stdout(&members_argv(&project))?)?; + if members.is_empty() { + // Nothing in the project is running — the filter is by project, not by + // this container, so this is not "the dev container vanished". Stopping + // it anyway is idempotent and keeps the message about what the user + // named. + return Ok(alone()); + } + Ok(order_for_stop(members, dev_id, dev_name)) +} + +/// Everything still running in the project of a dev container that is *not* +/// running itself. +/// +/// The app container can exit on its own — a crash, an OOM kill, a stop from +/// another pane — while its database and cache keep running. Without this, +/// stop reports "no running dev container" and walks the user away from a live +/// project: the same false absence this discovery path exists to prevent, just +/// arrived at from the other side. +/// +/// Returned in `docker ps` order, *not* dev-container-first — there is no dev +/// container running to lead with. Callers must not apply `stop_set`'s +/// "first element leads" convention to this result. +pub fn orphaned_members(candidates: &[crate::discover::Container]) -> Result, Error> { + for c in candidates { + let Some(project) = project_of(&c.id)? else { + continue; + }; + let members = parse_members(&docker_stdout(&members_argv(&project))?)?; + if !members.is_empty() { + return Ok(members); + } + } + Ok(Vec::new()) +} + +/// The compose project of a container, or `None` if it has none. +/// +/// A container that no longer exists reports none rather than failing. It can +/// be removed between discovery and here — a rebuild in another pane — and the +/// old single-container behavior treated a gone container as already stopped, +/// which is still the right answer. +fn project_of(container_id: &str) -> Result, Error> { + let res = run( + &project_argv(container_id), + Duration::from_secs(5), + StderrMode::Capture, + )?; + if res.exit_code != Some(0) { + let stderr = res.stderr.to_lowercase(); + if stderr.contains("no such object") || stderr.contains("no such container") { + return Ok(None); + } + } + let out = check(res)?; + Ok(parse_project(&out)) +} + +fn docker_stdout(argv: &[String]) -> Result { + let res = run(argv, Duration::from_secs(5), StderrMode::Capture)?; + check(res) +} + +fn check(res: crate::run::RunResult) -> Result { + if res.timed_out { + return Err(Error::DockerCommandFailed { + detail: "a docker query timed out after 5s".to_string(), + }); + } + if res.exit_code != Some(0) || res.stdout_incomplete { + return Err(Error::DockerCommandFailed { + detail: tail(res.stderr.trim(), 500), + }); + } + Ok(res.stdout) +} + +/// The compose project a container belongs to, if any. +/// +/// Docker's template prints an empty line when the label is missing, and +/// `` on older versions. Neither is a project name, and reading +/// either as one would send us looking for members of a project that does not +/// exist — so both mean "this is a plain single container". +pub fn parse_project(stdout: &str) -> Option { + let trimmed = stdout.trim(); + if trimmed.is_empty() || trimmed == "" { + return None; + } + Some(trimmed.to_string()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn m(id: &str, name: &str, service: &str) -> Member { + Member { + id: id.to_string(), + name: name.to_string(), + service: service.to_string(), + } + } + + // `docker compose run` containers carry the project label too, and + // `docker compose stop` deliberately leaves them alone. Sweeping them in + // would kill a `docker compose run --rm app pytest` a developer is watching + // in another pane — the opposite of the stopCompose parity this change + // claims. Docker has no negated label filter, so the row is filtered here. + #[test] + fn a_oneoff_run_container_is_not_part_of_the_project_stop() { + let parsed = + parse_members("abc\tproj-app-1\tapp\tFalse\ndef\tproj-app-run-x\tapp\tTrue\n").unwrap(); + assert_eq!(parsed, vec![m("abc", "proj-app-1", "app")]); + } + + // A missing oneoff label is not a claim that the container is one-off. + // Keeping it matches how the rest of this codebase treats an unknown. + #[test] + fn a_row_without_the_oneoff_label_is_kept() { + let parsed = parse_members("abc\tproj-app-1\tapp\t\n").unwrap(); + assert_eq!(parsed, vec![m("abc", "proj-app-1", "app")]); + } + + // Whether a container stopped is a question about the world, not about + // docker's prose. Same-type filters are OR'd, so one query covers the set. + // The mirror of the bug this feature exists to fix. The app container can + // exit on its own — a crash, an OOM kill, a `docker stop` from another pane + // — while postgres and redis keep running. Reporting "no running dev + // container" then walks the user away from a live database. Discovery + // already lists exited containers (`docker ps -a`), and a stopped container + // keeps its project label, so the evidence is in hand. + #[test] + fn an_exited_dev_container_still_names_its_project() { + // Ordering is what is testable purely: the exited container is not a + // stop target, so the survivors stand alone in the set. + let survivors = vec![ + m("db1", "proj-db-1", "db"), + m("r1", "proj-redis-1", "redis"), + ]; + let ordered = order_for_stop(survivors.clone(), "gone", ""); + assert_eq!( + ordered.iter().map(|x| x.id.as_str()).collect::>(), + vec!["gone", "db1", "r1"], + "a dev container absent from the listing is still led with" + ); + } + + // The synthetic row is the one container the user actually named. Rendering + // it blank in the safety prompt hides exactly what they need to recognize — + // and the name is already in hand at the call site. + #[test] + fn the_synthetic_dev_row_carries_its_name() { + let ordered = order_for_stop(vec![m("db1", "proj-db-1", "db")], "app1", "proj-app-1"); + assert_eq!(ordered[0].id, "app1"); + assert_eq!(ordered[0].name, "proj-app-1"); + } + + #[test] + fn running_argv_asks_about_exactly_these_containers() { + let argv = running_argv(&["a".to_string(), "b".to_string()]); + assert!(argv.contains(&"id=a".to_string()), "{argv:?}"); + assert!(argv.contains(&"id=b".to_string()), "{argv:?}"); + // Running only: `-a` would report a stopped container as still there. + assert!(!argv.contains(&"-a".to_string()), "{argv:?}"); + } + + #[test] + fn parse_running_ids_reads_one_id_per_line() { + assert_eq!(parse_running_ids("a\nb\n"), vec!["a", "b"]); + assert_eq!(parse_running_ids("\n"), Vec::::new()); + } + + #[test] + fn members_argv_lists_only_running_members_of_the_project() { + let argv = members_argv("proj_devcontainer"); + assert!(argv.contains(&"label=com.docker.compose.project=proj_devcontainer".to_string())); + // No `-a`: a container that is already stopped is not something to stop. + assert!(!argv.contains(&"-a".to_string()), "{argv:?}"); + } + + #[test] + fn parse_members_reads_id_name_and_service() { + let parsed = + parse_members("abc\tproj-app-1\tapp\tFalse\ndef\tproj-db-1\tdb\tFalse\n").unwrap(); + assert_eq!( + parsed, + vec![m("abc", "proj-app-1", "app"), m("def", "proj-db-1", "db")] + ); + } + + // Same rule discovery follows: a row we cannot read must not quietly shrink + // the set, because here that means silently leaving a container running + // after telling the user everything stopped. + #[test] + fn parse_members_rejects_malformed_rows() { + assert!(parse_members("abc\tonly-two\n").is_err()); + assert!(parse_members("\t\tapp\tFalse\n").is_err()); + } + + // Compose stops in reverse dependency order for a reason: the dev container + // is the thing writing to the database, so it goes first. Full topological + // ordering is not attempted — one level covers the real shape, where every + // other service is a dependency of the dev container. + #[test] + fn the_dev_container_stops_before_its_dependencies() { + let members = vec![ + m("db1", "proj-db-1", "db"), + m("app1", "proj-app-1", "app"), + m("redis1", "proj-redis-1", "redis"), + ]; + let ordered = order_for_stop(members, "app1", "proj-app-1"); + assert_eq!( + ordered.iter().map(|x| x.id.as_str()).collect::>(), + vec!["app1", "db1", "redis1"], + "the dev container leads; the rest keep their listed order" + ); + } + + // The dev container is always a member of its own project, so its absence + // means the two queries disagreed — a container stopped or was removed + // between them. Dropping it would stop the siblings and leave the one the + // user actually named running. + #[test] + fn a_missing_dev_container_is_still_stopped_first() { + let members = vec![m("db1", "proj-db-1", "db")]; + let ordered = order_for_stop(members, "app1", "proj-app-1"); + assert_eq!( + ordered.iter().map(|x| x.id.as_str()).collect::>(), + vec!["app1", "db1"] + ); + } + + #[test] + fn project_argv_asks_for_just_the_project_label() { + let argv = project_argv("abc123"); + assert_eq!(argv[0], "docker"); + assert!(argv.contains(&"inspect".to_string())); + assert!(argv.contains(&"abc123".to_string())); + assert!( + argv.iter() + .any(|a| a.contains("com.docker.compose.project")), + "{argv:?}" + ); + } + + // A container with no compose project is the plain single-container case, + // which must keep behaving exactly as it did. Docker's template prints an + // empty line for a missing key, and `` on older versions, so + // neither may be mistaken for a project actually named that. + #[test] + fn a_container_outside_a_compose_project_has_no_project() { + assert_eq!(parse_project(""), None); + assert_eq!(parse_project("\n"), None); + assert_eq!(parse_project("\n"), None); + assert_eq!(parse_project(" \n"), None); + } + + #[test] + fn a_compose_container_reports_its_project() { + assert_eq!( + parse_project("double-holo-ui_devcontainer\n"), + Some("double-holo-ui_devcontainer".to_string()) + ); + } +} diff --git a/src/error.rs b/src/error.rs index d776ee5..9630fe3 100644 --- a/src/error.rs +++ b/src/error.rs @@ -29,6 +29,8 @@ pub enum Error { MalformedDockerOutput { line: String }, #[error("multiple running dev containers for {repo_root}; refusing to choose: {ids:?}")] MultipleRunningContainers { repo_root: String, ids: Vec }, + #[error("these containers did not stop: {names:?}\n{detail}")] + ContainersNotStopped { names: Vec, detail: String }, #[error("docker command failed: {detail}")] DockerCommandFailed { detail: String }, #[error("{0}")] @@ -61,6 +63,13 @@ impl Error { Error::MultipleRunningContainers { .. } => { Some("stop the extras with `docker stop ` and retry") } + // Deliberately says nothing about the containers *not* listed. When + // docker never reached the daemon, every id lands here and nothing + // stopped — a hint claiming "the rest stopped" would then be a + // confident statement about a state we never observed. + Error::ContainersNotStopped { .. } => { + Some("stop them with `docker stop `, or retry once docker is reachable") + } _ => None, } } diff --git a/src/lib.rs b/src/lib.rs index e402b6e..3181a2c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,3 +1,4 @@ +pub mod compose; pub mod config; pub mod context; pub mod detect; diff --git a/src/stop.rs b/src/stop.rs index bd3f4bd..fc61331 100644 --- a/src/stop.rs +++ b/src/stop.rs @@ -5,18 +5,72 @@ use crate::discover::{self, Container}; use crate::error::Error; use crate::run::{run, StderrMode}; use crate::util::tail; -use crate::{context, preflight}; +use crate::{compose, context, preflight}; -pub fn stop_argv(id: &str) -> Vec { - vec!["docker".to_string(), "stop".to_string(), id.to_string()] +pub fn stop_argv(ids: &[String]) -> Vec { + let mut argv = vec!["docker".to_string(), "stop".to_string()]; + argv.extend(ids.iter().cloned()); + argv } -pub fn stopping_message(c: &Container, repo_root: &Path) -> String { +/// Stop `ids` in one call, then say nothing about whether it worked. +/// +/// docker stops the listed containers in *parallel* — the argument order only +/// controls the order results print — so a call is one grace window regardless +/// of how many containers it names. Verification is deliberately not done here: +/// see `verify_stopped`, which asks docker what is running rather than reading +/// this call's output. +fn stop_call(ids: &[String]) -> Result { + // docker's SIGTERM grace is 10s before SIGKILL, and the containers in one + // call share that window, so the budget does not scale with the count. + Ok(run( + &stop_argv(ids), + Duration::from_secs(STOP_TIMEOUT_SECS), + StderrMode::Capture, + )?) +} + +const STOP_TIMEOUT_SECS: u64 = 30; + +/// Confirm the stop by asking docker what is still running. +/// +/// Not by parsing `docker stop`'s output: on timeout the CLI is killed and its +/// output is silent about what landed, and "no such container" handling meant +/// matching English prose to decide whether a container was gone. A container +/// that is not running is stopped, whichever way it got there. +fn verify_stopped(targets: &[compose::Member], stop_detail: String) -> Result<(), Error> { + let alive = compose::still_running(targets)?; + if alive.is_empty() { + return Ok(()); + } + Err(Error::ContainersNotStopped { + // Names, not ids: this is what the user reads when six containers were + // named in the prompt and one is still up. `docker stop` takes either. + names: alive.iter().map(|m| m.name.clone()).collect(), + detail: stop_detail, + }) +} + +/// What is being stopped, printed after the user commits. +/// +/// Names every container for the same reason the prompt does: this line is what +/// stays on screen in the pane afterwards, and it is the record of what the +/// keystroke actually spent. +pub fn stopping_message(targets: &[compose::Member], repo_root: &Path) -> String { + if let [only] = targets { + return format!( + "stopping container {} ({}) for {}", + only.id, + only.name, + repo_root.display() + ); + } + let names: Vec<&str> = targets.iter().map(|m| m.name.as_str()).collect(); format!( - "stopping container {} ({}) for {}", - c.id, - c.name, - repo_root.display() + "stopping {} containers for {}: {}", + targets.len(), + repo_root.display(), + names.join(", ") ) } @@ -29,6 +83,66 @@ pub fn confirm_prompt(c: &Container, repo_root: &Path) -> String { ) } +/// The confirmation for everything that is about to stop. +/// +/// Every container is named, not counted. This prompt is the only thing between +/// a mis-keyed binding and a stopped database, so the user has to be able to see +/// that the database is in the list — "stop 6 containers?" does not tell them +/// what they are spending. +/// +/// One member renders exactly as it always did: nothing about a plain +/// single-container repo changed, so nothing about its prompt should. +pub fn project_confirm_prompt(members: &[compose::Member], repo_root: &Path) -> String { + if let [only] = members { + let c = Container { + id: only.id.clone(), + name: only.name.clone(), + state: String::new(), + }; + return confirm_prompt(&c, repo_root); + } + let service_width = members + .iter() + .map(|m| m.service.len()) + .max() + .unwrap_or_default(); + // The name column is padded too, so the ids form a column the eye can scan + // rather than a ragged edge. This prompt is the safety mechanism; it should + // be easy to read under a mis-keystroke's worth of attention. + let name_width = members + .iter() + .map(|m| m.name.len()) + .max() + .unwrap_or_default(); + let mut out = format!( + "stop {} containers for {}?\n", + members.len(), + repo_root.display() + ); + for m in members { + out.push_str(&format!( + " {:service_width$} {:name_width$} {}\n", + m.service, + m.name, + m.id, + service_width = service_width, + name_width = name_width + )); + } + // The question goes last so the answer is typed against it. + out.push_str("[y/N]: "); + out +} + +/// What a cancel left behind. Counted, because "container left running" after +/// declining a six-container stop reads as though the other five went down. +fn cancel_message(count: usize) -> String { + if count == 1 { + return "cancelled; container left running.".to_string(); + } + format!("cancelled; {count} containers left running.") +} + /// Only an explicit yes proceeds. A bare Enter, an unrecognized answer, and an /// unreadable one all cancel: stopping discards a running container's state /// with no undo, and the entrypoint is a single keystroke away from herdr's @@ -82,37 +196,80 @@ pub fn run_stop() -> Result<(), Error> { let config_files = discovery_config_files(&repo_root, &cfg.repo(&repo_root))?; let containers = discover::list(&repo_root, &config_files)?; - match discover::select_running(&containers, &repo_root)? { + // Whether the first target is the dev container. Only then is there an + // ordering worth paying a second grace window for; the orphan path below + // returns survivors in docker's order, where "first" means nothing. + let mut dev_leads = true; + let targets = match discover::select_running(&containers, &repo_root)? { + // Everything that goes down together, so the confirmation can name it + // all before the user commits to it. + Some(c) => compose::stop_set(&c.id, &c.name)?, + // The dev container is not running — but its compose project may still + // be. Saying "no running dev container" while postgres serves is the + // same false absence this path exists to prevent. None => { - println!("no running dev container for {}", repo_root.display()); - Ok(()) - } - Some(c) => { - print!("{}", confirm_prompt(&c, &repo_root)); - std::io::Write::flush(&mut std::io::stdout())?; - let answer = read_answer(&mut std::io::stdin().lock())?; - if !confirmed(&answer) { - println!("cancelled; container left running."); - return Ok(()); - } - println!("{}", stopping_message(&c, &repo_root)); - // docker's SIGTERM grace is 10s before SIGKILL; give the CLI 30s. - let res = run( - &stop_argv(&c.id), - Duration::from_secs(30), - StderrMode::Capture, - )?; - let already_gone = res.stderr.to_lowercase().contains("no such container"); - if res.exit_code == Some(0) || already_gone { - println!("stopped."); - Ok(()) - } else { - Err(Error::DockerCommandFailed { - detail: tail(res.stderr.trim(), 500), - }) + dev_leads = false; + let orphans = compose::orphaned_members(&containers)?; + if !orphans.is_empty() { + println!( + "the dev container for {} is not running, but {} of its compose services are:", + repo_root.display(), + orphans.len() + ); } + orphans + } + }; + if targets.is_empty() { + println!("no running dev container for {}", repo_root.display()); + return Ok(()); + } + + print!("{}", project_confirm_prompt(&targets, &repo_root)); + std::io::Write::flush(&mut std::io::stdout())?; + let answer = read_answer(&mut std::io::stdin().lock())?; + if !confirmed(&answer) { + println!("{}", cancel_message(targets.len())); + return Ok(()); + } + println!("{}", stopping_message(&targets, &repo_root)); + + // Two calls, not one argv: docker stops the containers named in a single + // call in parallel, so passing them together would SIGTERM the database at + // the same instant as the dev container still talking to it. The dev + // container goes first and is waited on — that is the point of ordering, + // and the direction Compose shuts a project down in. + // + // With no dev container to lead, there is nothing to order around: the + // survivors go down together rather than paying a second grace window to + // sequence one arbitrary service ahead of the others. + let (first, rest) = if dev_leads { + targets.split_at(1) + } else { + targets.split_at(0) + }; + let mut detail = String::new(); + for phase in [first, rest] { + if phase.is_empty() { + continue; + } + let ids: Vec = phase.iter().map(|m| m.id.clone()).collect(); + let res = stop_call(&ids)?; + if res.timed_out { + detail.push_str(&format!( + "docker stop timed out after {STOP_TIMEOUT_SECS}s; " + )); + } else if res.exit_code != Some(0) { + detail.push_str(&tail(res.stderr.trim(), 500)); + detail.push_str("; "); } } + // Asked of docker, after the fact: a timeout kills the CLI without telling + // us what landed, and a container that stopped is stopped whether or not + // docker's output said so. + verify_stopped(&targets, detail)?; + println!("stopped."); + Ok(()) } #[cfg(test)] @@ -145,9 +302,95 @@ mod tests { assert_eq!(got, vec![std::path::PathBuf::from("/r/alt/devc.json")]); } + // A single-container repo is unchanged by compose support, so its prompt + // must be too — no count, no list, no new noise where nothing differs. + #[test] + fn one_container_keeps_the_original_prompt() { + let c = crate::discover::Container { + id: "abc123".to_string(), + name: "herdr_devcontainer".to_string(), + state: "running".to_string(), + }; + let members = vec![crate::compose::Member { + id: "abc123".to_string(), + name: "herdr_devcontainer".to_string(), + service: String::new(), + }]; + assert_eq!( + project_confirm_prompt(&members, std::path::Path::new("/r")), + confirm_prompt(&c, std::path::Path::new("/r")) + ); + } + + // The confirm is the only thing between a mis-keyed binding and six stopped + // containers, so it names every one of them rather than a count. A user who + // sees "postgres" listed can still say no. + #[test] + fn a_compose_project_prompt_names_every_container() { + let members = vec![ + crate::compose::Member { + id: "dc08b7aeca6f".to_string(), + name: "dh_devcontainer-app-1".to_string(), + service: "app".to_string(), + }, + crate::compose::Member { + id: "6cc33c601af0".to_string(), + name: "dh_devcontainer-postgres-1".to_string(), + service: "postgres".to_string(), + }, + ]; + let p = project_confirm_prompt(&members, std::path::Path::new("/r")); + assert!(p.contains("stop 2 containers"), "{p}"); + assert!(p.contains("/r"), "{p}"); + assert!(p.contains("[y/N]"), "{p}"); + for m in &members { + assert!(p.contains(&m.id), "{p} is missing {}", m.id); + assert!(p.contains(&m.name), "{p} is missing {}", m.name); + assert!(p.contains(&m.service), "{p} is missing {}", m.service); + } + // The prompt must be the last thing on screen, so the answer is typed + // against it rather than against a list line. + assert!(p.trim_end_matches(' ').ends_with("[y/N]:"), "{p}"); + } + + // Cancelling a six-container stop that says "container left running" reads + // as though five of them went down anyway. + #[test] + fn the_cancel_message_matches_how_many_were_at_stake() { + assert_eq!(cancel_message(1), "cancelled; container left running."); + assert_eq!(cancel_message(6), "cancelled; 6 containers left running."); + } + + #[test] + fn stop_argv_takes_every_id_in_order() { + assert_eq!( + stop_argv(&["a".to_string(), "b".to_string()]), + vec!["docker", "stop", "a", "b"] + ); + } + + // What used to be inferred from `docker stop`'s prose is now asked of + // docker directly, so that behavior is covered by the docker-gated + // integration test rather than by string-matching unit tests. What belongs + // here is the pure part: the error names the containers still running, so + // a partial stop cannot be mistaken for a completed one. + #[test] + fn a_container_left_running_is_named_in_the_error() { + let err = Error::ContainersNotStopped { + names: vec!["dh_devcontainer-postgres-1".to_string()], + detail: "cannot stop container: permission denied".to_string(), + }; + let msg = err.to_string(); + assert!(msg.contains("dh_devcontainer-postgres-1"), "{msg}"); + assert!(msg.contains("permission denied"), "{msg}"); + } + #[test] fn stop_argv_targets_the_id() { - assert_eq!(stop_argv("abc123"), vec!["docker", "stop", "abc123"]); + assert_eq!( + stop_argv(&["abc123".to_string()]), + vec!["docker", "stop", "abc123"] + ); } // Stopping is destructive and one mis-keyed binding away: `prefix+shift+s` @@ -188,14 +431,40 @@ mod tests { #[test] fn the_stop_message_names_both_id_and_name() { - let c = crate::discover::Container { - id: "abc123".to_string(), - name: "herdr_devcontainer".to_string(), - state: "running".to_string(), - }; - let msg = stopping_message(&c, std::path::Path::new("/r")); + let msg = stopping_message( + &[crate::compose::Member { + id: "abc123".to_string(), + name: "herdr_devcontainer".to_string(), + service: String::new(), + }], + std::path::Path::new("/r"), + ); assert!(msg.contains("abc123"), "{msg}"); assert!(msg.contains("herdr_devcontainer"), "{msg}"); assert!(msg.contains("/r"), "{msg}"); } + + // The line that stays on screen after the pane finishes is the record of + // what the keystroke spent, so it names every container rather than a count. + #[test] + fn the_stop_message_names_every_container_of_a_project() { + let msg = stopping_message( + &[ + crate::compose::Member { + id: "a".to_string(), + name: "dh-app-1".to_string(), + service: "app".to_string(), + }, + crate::compose::Member { + id: "b".to_string(), + name: "dh-postgres-1".to_string(), + service: "postgres".to_string(), + }, + ], + std::path::Path::new("/r"), + ); + assert!(msg.contains("dh-app-1"), "{msg}"); + assert!(msg.contains("dh-postgres-1"), "{msg}"); + assert!(msg.contains('2'), "{msg}"); + } } diff --git a/tests/integration.rs b/tests/integration.rs index 02b3dc1..e223712 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -7,7 +7,7 @@ use std::path::Path; use std::process::{Command, Stdio}; use std::time::Duration; -use herdr_devcontainer::{detect, discover, exec, preflight, shell, up}; +use herdr_devcontainer::{compose, detect, discover, exec, preflight, shell, stop, up}; fn sh_ok(dir: &Path, cmd: &str, args: &[&str]) { let status = Command::new(cmd) @@ -65,6 +65,163 @@ fn fixture_repo(tmp: &Path) -> std::path::PathBuf { /// equal, while `config_file` holds the POSIX path the CLI resolved inside WSL. /// Discovery keyed only on `local_folder` reports "no container" for a /// container that is running in front of the user — the reported regression. +/// Removes every container of a Compose project fixture, so a failed assertion +/// unwinds without leaving containers on the host. +struct RemoveProject(&'static str); + +impl Drop for RemoveProject { + fn drop(&mut self) { + let Ok(out) = Command::new("docker") + .args([ + "ps", + "-aq", + "--filter", + &format!("label=com.docker.compose.project={}", self.0), + ]) + .output() + else { + return; + }; + for id in String::from_utf8_lossy(&out.stdout).split_whitespace() { + let _ = Command::new("docker").args(["rm", "-f", id]).status(); + } + } +} + +/// Starts one labelled member of a Compose project fixture, returning its short +/// id. +fn compose_member(project: &str, service: &str, cmd: &str) -> String { + let out = Command::new("docker") + .args(["run", "-d", "--label"]) + .arg(format!("com.docker.compose.project={project}")) + .arg("--label") + .arg(format!("com.docker.compose.service={service}")) + .args(["--name", &format!("{project}-{service}-1")]) + .args(["alpine:3.20", "sh", "-c", cmd]) + .output() + .unwrap(); + assert!(out.status.success(), "docker run {service} failed"); + String::from_utf8_lossy(&out.stdout).trim()[..12].to_string() +} + +/// The app container exits on its own — a crash, an OOM kill, a stop from +/// another pane — while its services keep running. Reporting "no running dev +/// container" then walks the user away from a live database. +#[test] +#[ignore = "requires docker"] +fn an_exited_dev_container_does_not_hide_its_running_services() { + preflight::check_docker("docker").expect("docker daemon"); + + let project = "herdrdevc_orphan_fixture"; + let _cleanup = RemoveProject(project); + + // The app exits immediately; the db keeps running. + let app = compose_member(project, "app", "exit 0"); + let db = compose_member(project, "db", "sleep 300"); + + // Wait for the app to actually be gone before asserting on it. A silent + // fall-through here would make a slow fixture look like a discovery bug. + let mut exited = false; + for _ in 0..50 { + let out = Command::new("docker") + .args(["inspect", "-f", "{{.State.Running}}", &app]) + .output() + .unwrap(); + if String::from_utf8_lossy(&out.stdout).trim() == "false" { + exited = true; + break; + } + std::thread::sleep(Duration::from_millis(100)); + } + assert!(exited, "fixture: the app container did not exit within 5s"); + + // This is what discovery hands over: the dev container, exited. + let exited_container = vec![discover::Container { + id: app.clone(), + name: format!("{project}-app-1"), + state: "exited".to_string(), + }]; + let orphans = compose::orphaned_members(&exited_container).expect("orphan lookup"); + assert_eq!( + orphans.iter().map(|m| m.id.as_str()).collect::>(), + vec![db.as_str()], + "the running db must be reported, not swallowed with the exited app" + ); +} + +/// Stopping a compose-based dev container takes its whole project down. +/// +/// The app service is only one container of several; leaving the database and +/// cache running is not what "stop the dev container" means, and it is what the +/// Dev Container spec's default `shutdownAction: stopCompose` says too. +#[test] +#[ignore = "requires docker"] +fn stopping_a_compose_dev_container_stops_its_whole_project() { + preflight::check_docker("docker").expect("docker daemon"); + + let project = "herdrdevc_stopset_fixture"; + let _cleanup = RemoveProject(project); + + // Built with plain `docker run` rather than the compose CLI: the labels are + // what the plugin reads, and this keeps the test to the one dependency the + // plugin itself requires. + let app = compose_member(project, "app", "sleep 300"); + let db = compose_member(project, "db", "sleep 300"); + + let targets = compose::stop_set(&app, &format!("{project}-app-1")).expect("stop set"); + assert_eq!(targets.len(), 2, "both services belong to the stop set"); + assert_eq!( + targets[0].id, app, + "the dev container stops before its dependencies" + ); + + let ids: Vec = targets.iter().map(|m| m.id.clone()).collect(); + let out = Command::new("docker") + .args(&stop::stop_argv(&ids)[1..]) + .output() + .unwrap(); + assert!(out.status.success(), "docker stop failed"); + + for id in [&app, &db] { + let out = Command::new("docker") + .args(["inspect", "-f", "{{.State.Running}}", id]) + .output() + .unwrap(); + assert_eq!( + String::from_utf8_lossy(&out.stdout).trim(), + "false", + "{id} is still running" + ); + } +} + +/// A container outside any compose project stops alone — the single-container +/// path must be untouched by compose support. +#[test] +#[ignore = "requires docker"] +fn a_standalone_dev_container_stops_by_itself() { + preflight::check_docker("docker").expect("docker daemon"); + + let out = Command::new("docker") + .args(["run", "-d", "--rm", "alpine:3.20", "sleep", "300"]) + .output() + .unwrap(); + assert!(out.status.success()); + let id = String::from_utf8_lossy(&out.stdout).trim()[..12].to_string(); + struct Rm(String); + impl Drop for Rm { + fn drop(&mut self) { + let _ = Command::new("docker").args(["rm", "-f", &self.0]).status(); + } + } + let _cleanup = Rm(id.clone()); + + let targets = compose::stop_set(&id, "solo").expect("stop set"); + assert_eq!(targets.len(), 1); + assert_eq!(targets[0].id, id); + assert_eq!(targets[0].name, "solo"); +} + #[test] #[ignore = "requires docker"] fn a_container_labelled_by_vs_code_on_windows_is_still_found() {