diff --git a/CHANGELOG.md b/CHANGELOG.md index 547dfcf0..410092ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **`rm`, `kill` and a launch remove the containers a forgotten workspace left running.** + `devpod delete` finds a container only by the uid of the record it deletes, so a forced + delete whose `compose down` failed, or a record made again under a new uid, left the old + compose project up with nothing that owned it. Its fixed `container_name`s then failed every + later `up` of that workspace with `Conflict. The container name ... is already in use`. + Once devpod holds no record of the workspace, dl now removes every container whose compose + labels name devpod's directory for that id or the clone dl made, and names each one. A + workspace devpod still holds (or whose record dl cannot read), a local folder's own compose + projects, and any container that names another workspace's devpod directory or a folder + devpod holds as another workspace's source, are left alone. + ## [0.62.0] - 2026-10-16 ### Added diff --git a/docs/cleanup.md b/docs/cleanup.md index 8c394ddc..b067ca1f 100644 --- a/docs/cleanup.md +++ b/docs/cleanup.md @@ -97,6 +97,47 @@ records died before any of this existed, no route reaches them that is not the pattern above, and they stay. See [the disk neither command frees](#the-disk-neither-command-frees). +### Containers a forgotten workspace left running + +`devpod delete` finds a workspace's container by one label, +`dev.containers.id=`, and runs `compose down` only when it finds it. Two +ordinary events leave a compose project running after devpod has dropped its +record: + +- A forced delete (`devpod delete --force`, which `dl kill` runs) whose + `compose down` failed. A compose file that no longer interpolates is enough, for + example a required variable from a `.env` that the `initializeCommand` wrote and + that is gone from the clone. devpod logs the error and drops the record anyway. +- A record made again under a new uid. A `devpod up` over the leftovers makes a + new record, then fails because a compose file that pins `container_name:` cannot + make a second container with that name. The next `devpod delete` looks for the + new uid, finds no container, and reports success. The leftovers stay up. + +So `dl rm` (and `kill`), once devpod holds no record of the workspace in any +context, removes every container whose compose labels +(`com.docker.compose.project.working_dir`, `com.docker.compose.project.config_files`) +name a path inside devpod's own directory for that workspace id +(`/agent/contexts//workspaces/`, where devpod writes its +compose override) or inside the clone dl made for it. A launch does the same +before its `devpod up` for a workspace devpod has no record of, so the new compose +project does not trip over the old names. Each removal is named on stderr: + +``` +Removed containers left behind: , +``` + +A workspace devpod still holds is left alone (a record dl cannot read counts as +held), and so is a container that names +only a local folder opened with `dl ./folder`: that folder is yours, and a compose +project you started in it by hand is not a leftover. Names are never matched. + +When any doubt remains, the container stays. A container is left when one of +its labels names devpod's directory for a *different* workspace id, or a path +inside a folder that devpod holds as another workspace's source and that sits +in the clone (a local folder opened with `dl ./folder` can sit inside a clone). +When devpod's records cannot be read, a container that names only the clone +stays too. + ### What a delete says while it does it `dl rm` names the workspace going in and again once it has gone, both on diff --git a/docs/development.md b/docs/development.md index 28f332ba..bdf953d4 100644 --- a/docs/development.md +++ b/docs/development.md @@ -69,7 +69,7 @@ once under the `api` section and once under the module that owns them. **What it still does not reach, and it is not one type.** A type `api` never re-exports but a promised signature hands back is reachable from outside and classified as binary surface. Counted -on the checked-in files rather than guessed at, that is **41 types owning close to six hundred rows** in +on the checked-in files rather than guessed at, that is **42 types owning close to six hundred rows** in `public-api.rest.txt`, and the command that lists them needs no toolchain: **A falling count is not automatically a win, and it is worth knowing which kind you are looking diff --git a/rust/devlaunch-core/public-api.api.txt b/rust/devlaunch-core/public-api.api.txt index 6c6a3991..5fcce1a1 100644 --- a/rust/devlaunch-core/public-api.api.txt +++ b/rust/devlaunch-core/public-api.api.txt @@ -93,6 +93,16 @@ pub devlaunch_core::api::LaunchNotice::HerdrTab(devlaunch_core::flows::launch::H pub devlaunch_core::api::LaunchNotice::LaunchLockUnavailable pub devlaunch_core::api::LaunchNotice::LaunchLockUnavailable::reason: alloc::string::String pub devlaunch_core::api::LaunchNotice::LaunchLockUnavailable::workspace_id: alloc::string::String +pub devlaunch_core::api::LaunchNotice::LeftoverContainersNotListed +pub devlaunch_core::api::LaunchNotice::LeftoverContainersNotListed::reason: alloc::string::String +pub devlaunch_core::api::LaunchNotice::LeftoverContainersNotListed::workspace_id: alloc::string::String +pub devlaunch_core::api::LaunchNotice::LeftoverContainersNotRemoved +pub devlaunch_core::api::LaunchNotice::LeftoverContainersNotRemoved::names: devlaunch_core::domain::workspace_state::NonEmpty +pub devlaunch_core::api::LaunchNotice::LeftoverContainersNotRemoved::reason: alloc::string::String +pub devlaunch_core::api::LaunchNotice::LeftoverContainersNotRemoved::workspace_id: alloc::string::String +pub devlaunch_core::api::LaunchNotice::LeftoverContainersRemoved +pub devlaunch_core::api::LaunchNotice::LeftoverContainersRemoved::names: devlaunch_core::domain::workspace_state::NonEmpty +pub devlaunch_core::api::LaunchNotice::LeftoverContainersRemoved::workspace_id: alloc::string::String pub devlaunch_core::api::LaunchNotice::Lifecycle(devlaunch_core::flows::lifecycle::LifecycleNotice) pub devlaunch_core::api::LaunchNotice::MountSourceEmpty pub devlaunch_core::api::LaunchNotice::MountSourceMissing @@ -177,6 +187,16 @@ pub devlaunch_core::api::LifecycleNotice::CloneNotRemoved::refusal: devlaunch_co pub devlaunch_core::api::LifecycleNotice::CloneNotRemoved::workspace_id: alloc::string::String pub devlaunch_core::api::LifecycleNotice::CloneRemoved pub devlaunch_core::api::LifecycleNotice::CloneRemoved::workspace_id: alloc::string::String +pub devlaunch_core::api::LifecycleNotice::LeftoverContainersNotListed +pub devlaunch_core::api::LifecycleNotice::LeftoverContainersNotListed::reason: alloc::string::String +pub devlaunch_core::api::LifecycleNotice::LeftoverContainersNotListed::workspace_id: alloc::string::String +pub devlaunch_core::api::LifecycleNotice::LeftoverContainersNotRemoved +pub devlaunch_core::api::LifecycleNotice::LeftoverContainersNotRemoved::names: devlaunch_core::domain::workspace_state::NonEmpty +pub devlaunch_core::api::LifecycleNotice::LeftoverContainersNotRemoved::reason: alloc::string::String +pub devlaunch_core::api::LifecycleNotice::LeftoverContainersNotRemoved::workspace_id: alloc::string::String +pub devlaunch_core::api::LifecycleNotice::LeftoverContainersRemoved +pub devlaunch_core::api::LifecycleNotice::LeftoverContainersRemoved::names: devlaunch_core::domain::workspace_state::NonEmpty +pub devlaunch_core::api::LifecycleNotice::LeftoverContainersRemoved::workspace_id: alloc::string::String pub devlaunch_core::api::LifecycleNotice::RecordNotDropped pub devlaunch_core::api::LifecycleNotice::RecordNotDropped::path: std::path::PathBuf pub devlaunch_core::api::LifecycleNotice::RecordNotDropped::refusal: devlaunch_core::domain::metadata::MetadataError @@ -677,6 +697,16 @@ pub devlaunch_core::flows::launch::LaunchNotice::HerdrTab(devlaunch_core::flows: pub devlaunch_core::flows::launch::LaunchNotice::LaunchLockUnavailable pub devlaunch_core::flows::launch::LaunchNotice::LaunchLockUnavailable::reason: alloc::string::String pub devlaunch_core::flows::launch::LaunchNotice::LaunchLockUnavailable::workspace_id: alloc::string::String +pub devlaunch_core::flows::launch::LaunchNotice::LeftoverContainersNotListed +pub devlaunch_core::flows::launch::LaunchNotice::LeftoverContainersNotListed::reason: alloc::string::String +pub devlaunch_core::flows::launch::LaunchNotice::LeftoverContainersNotListed::workspace_id: alloc::string::String +pub devlaunch_core::flows::launch::LaunchNotice::LeftoverContainersNotRemoved +pub devlaunch_core::flows::launch::LaunchNotice::LeftoverContainersNotRemoved::names: devlaunch_core::domain::workspace_state::NonEmpty +pub devlaunch_core::flows::launch::LaunchNotice::LeftoverContainersNotRemoved::reason: alloc::string::String +pub devlaunch_core::flows::launch::LaunchNotice::LeftoverContainersNotRemoved::workspace_id: alloc::string::String +pub devlaunch_core::flows::launch::LaunchNotice::LeftoverContainersRemoved +pub devlaunch_core::flows::launch::LaunchNotice::LeftoverContainersRemoved::names: devlaunch_core::domain::workspace_state::NonEmpty +pub devlaunch_core::flows::launch::LaunchNotice::LeftoverContainersRemoved::workspace_id: alloc::string::String pub devlaunch_core::flows::launch::LaunchNotice::Lifecycle(devlaunch_core::flows::lifecycle::LifecycleNotice) pub devlaunch_core::flows::launch::LaunchNotice::MountSourceEmpty pub devlaunch_core::flows::launch::LaunchNotice::MountSourceMissing @@ -839,6 +869,16 @@ pub devlaunch_core::flows::lifecycle::LifecycleNotice::CloneNotRemoved::refusal: pub devlaunch_core::flows::lifecycle::LifecycleNotice::CloneNotRemoved::workspace_id: alloc::string::String pub devlaunch_core::flows::lifecycle::LifecycleNotice::CloneRemoved pub devlaunch_core::flows::lifecycle::LifecycleNotice::CloneRemoved::workspace_id: alloc::string::String +pub devlaunch_core::flows::lifecycle::LifecycleNotice::LeftoverContainersNotListed +pub devlaunch_core::flows::lifecycle::LifecycleNotice::LeftoverContainersNotListed::reason: alloc::string::String +pub devlaunch_core::flows::lifecycle::LifecycleNotice::LeftoverContainersNotListed::workspace_id: alloc::string::String +pub devlaunch_core::flows::lifecycle::LifecycleNotice::LeftoverContainersNotRemoved +pub devlaunch_core::flows::lifecycle::LifecycleNotice::LeftoverContainersNotRemoved::names: devlaunch_core::domain::workspace_state::NonEmpty +pub devlaunch_core::flows::lifecycle::LifecycleNotice::LeftoverContainersNotRemoved::reason: alloc::string::String +pub devlaunch_core::flows::lifecycle::LifecycleNotice::LeftoverContainersNotRemoved::workspace_id: alloc::string::String +pub devlaunch_core::flows::lifecycle::LifecycleNotice::LeftoverContainersRemoved +pub devlaunch_core::flows::lifecycle::LifecycleNotice::LeftoverContainersRemoved::names: devlaunch_core::domain::workspace_state::NonEmpty +pub devlaunch_core::flows::lifecycle::LifecycleNotice::LeftoverContainersRemoved::workspace_id: alloc::string::String pub devlaunch_core::flows::lifecycle::LifecycleNotice::RecordNotDropped pub devlaunch_core::flows::lifecycle::LifecycleNotice::RecordNotDropped::path: std::path::PathBuf pub devlaunch_core::flows::lifecycle::LifecycleNotice::RecordNotDropped::refusal: devlaunch_core::domain::metadata::MetadataError diff --git a/rust/devlaunch-core/src/clients/devpod_home.rs b/rust/devlaunch-core/src/clients/devpod_home.rs index e3fb5a69..247d7cf4 100644 --- a/rust/devlaunch-core/src/clients/devpod_home.rs +++ b/rust/devlaunch-core/src/clients/devpod_home.rs @@ -134,6 +134,116 @@ impl DevpodHome { .join("workspace.lock") } + /// devpod's agent-side directories, one per context, each holding one + /// directory per workspace. + fn agent_contexts(&self) -> PathBuf { + self.root.join("agent").join("contexts") + } + + /// Whether any context still holds a record for this workspace. + /// + /// Every context rather than [`Self::sole_context_holding`]'s single answer, + /// because the caller asks the opposite question: whether devpod has *no* + /// workspace by this id anywhere, so that nothing on the machine can still + /// claim the containers that name it. Two contexts holding it is + /// [`RecordPresence::Held`]. + /// + /// Only "not found", for the `contexts` directory or for every record path + /// under it, is [`RecordPresence::Absent`]. Anything else that would not + /// read (a directory dl may not search, an entry the listing could not + /// return) is [`RecordPresence::Unknown`]: a record may be there. A context + /// name that is not UTF-8 is read like any other, by its bytes. + pub(crate) fn record_presence(&self, workspace_id: &str) -> RecordPresence { + let contexts = match std::fs::read_dir(self.contexts()) { + Ok(contexts) => contexts, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + return RecordPresence::Absent; + } + Err(_) => return RecordPresence::Unknown, + }; + let mut unknown = false; + for context in contexts { + let Ok(context) = context else { + unknown = true; + continue; + }; + let record = context + .path() + .join("workspaces") + .join(workspace_id) + .join("workspace.json"); + match std::fs::metadata(&record) { + Ok(_) => return RecordPresence::Held, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(_) => unknown = true, + } + } + if unknown { + RecordPresence::Unknown + } else { + RecordPresence::Absent + } + } + + /// The workspace id whose devpod agent-side directory `path` is inside, + /// under any context, or nothing when it is inside none. + /// + /// devpod writes its compose override for a workspace there + /// (`.docker-compose/docker-compose.devcontainer.containerFeatures-*.yml`), and + /// compose records that file in the `config_files` label of every container + /// the project makes. So the label names the workspace *by id*, whatever uid + /// devpod gave the project, which is the one handle a container keeps after + /// devpod has forgotten it. + pub(crate) fn agent_dir_owner<'p>(&self, path: &'p Path) -> Option<&'p std::ffi::OsStr> { + let rest = path.strip_prefix(self.agent_contexts()).ok()?; + let mut components = rest.components().skip(1); + if components.next()?.as_os_str() != "workspaces" { + return None; + } + match components.next()? { + std::path::Component::Normal(id) => Some(id), + _ => None, + } + } + + /// The local folders of every workspace devpod holds a record for, or + /// nothing when any record, or the walk to it, would not read. + /// + /// A record whose source is not a local folder (a git source devpod cloned + /// itself) names no folder on this machine and is skipped. A record that is + /// there and would not read is not: it could name any folder, so the answer + /// is "unknown" rather than a list missing one. + pub(crate) fn held_local_folders(&self) -> Option> { + let mut folders = Vec::new(); + let Some(contexts) = read_dir_if_any(&self.contexts())? else { + return Some(folders); + }; + for context in contexts { + let Some(workspaces) = read_dir_if_any(&context.ok()?.path().join("workspaces"))? + else { + continue; + }; + for workspace in workspaces { + let record = workspace.ok()?.path().join("workspace.json"); + let text = match std::fs::read_to_string(&record) { + Ok(text) => text, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue, + Err(_) => return None, + }; + let record: serde_json::Value = serde_json::from_str(&text).ok()?; + if let Some(folder) = record + .get("source") + .and_then(|source| source.get("localFolder")) + .and_then(serde_json::Value::as_str) + .filter(|folder| !folder.is_empty()) + { + folders.push(PathBuf::from(folder)); + } + } + } + Some(folders) + } + /// devpod's record of a *completed* create for one workspace. /// /// devpod writes this beside [`Self::record`] on its way out of a successful @@ -247,6 +357,30 @@ impl DevpodHome { } } +/// Whether devpod holds a record for one workspace id, in any context. +/// +/// Three arms, because "could not tell" must not read as "no": the one caller +/// removes containers on [`Self::Absent`] alone. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum RecordPresence { + /// A record is there, in at least one context. + Held, + /// No context holds one, and every place one could be said so. + Absent, + /// Some place a record could be would not read. + Unknown, +} + +/// A directory's entries; `Some(None)` when it is not there, and nothing when +/// it is there and would not read. +fn read_dir_if_any(dir: &Path) -> Option> { + match std::fs::read_dir(dir) { + Ok(entries) => Some(Some(entries)), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Some(None), + Err(_) => None, + } +} + /// Whether devpod finished creating a workspace, as far as its own records say. /// /// Three arms rather than a bool, for the reason [`crate::domain::workspace_state`] @@ -428,6 +562,36 @@ pub(crate) fn devpod_home_with(entries: &[(&str, &str, Option<()>)]) -> ScratchH ScratchHome { _dir: dir, home } } +/// An empty record for one workspace under a context named by raw bytes, for +/// the test that needs a context name that is not UTF-8. +#[cfg(test)] +pub(crate) fn record_under_context( + home: &DevpodHome, + context: &std::ffi::OsStr, + workspace_id: &str, +) { + let dir = home + .contexts() + .join(context) + .join("workspaces") + .join(workspace_id); + std::fs::create_dir_all(&dir).expect("a record directory"); + std::fs::write(dir.join("workspace.json"), "{}").expect("a record"); +} + +/// A file in devpod's agent-side directory for one workspace, where devpod +/// writes its compose override. Not created: a container label names it whether +/// or not it is still on disk. +#[cfg(test)] +pub(crate) fn agent_compose_file(home: &DevpodHome, context: &str, workspace_id: &str) -> PathBuf { + home.agent_contexts() + .join(context) + .join("workspaces") + .join(workspace_id) + .join(".docker-compose") + .join("docker-compose.devcontainer.containerFeatures-0.yml") +} + /// The `flock` devpod blocks on, created empty, for the one test that has to /// assert it is still standing afterwards. /// diff --git a/rust/devlaunch-core/src/clients/docker.rs b/rust/devlaunch-core/src/clients/docker.rs index ea69e833..2b03ac9a 100644 --- a/rust/devlaunch-core/src/clients/docker.rs +++ b/rust/devlaunch-core/src/clients/docker.rs @@ -1,11 +1,12 @@ //! Everything devlaunch asks `docker`, and everything it reads back. //! -//! Three calls, and between them they cover two commands. `dl rm` removes -//! the named volumes a workspace's devcontainer created, once the workspace -//! itself is gone (devlaunch#325). `dl kill` asks which containers a wedged -//! workspace's compose project still has up, and kills them (devlaunch#484) — -//! the one reading in the module, and the reason [`Running`] exists beside -//! [`Answer`]. +//! `dl rm` removes the named volumes a workspace's devcontainer created, +//! once the workspace itself is gone (devlaunch#325). `dl kill` asks which +//! containers a wedged workspace's compose project still has up, and kills them +//! (devlaunch#484) — the reason [`Running`] exists beside [`Answer`]. `rm` and the +//! launch both list compose containers by their labels and remove the ones a +//! forgotten workspace left behind ([`compose_containers`], [`remove_containers`]; +//! see [`crate::flows::orphan_containers`]). //! //! Nothing here removes an *image* — images are shared, they are expensive to //! rebuild, and which workspace owns one is genuinely ambiguous, so they stay @@ -21,6 +22,7 @@ //! apart, and these are the only spawns that can produce either. use std::io::ErrorKind; +use std::path::{Path, PathBuf}; use std::time::Duration; use crate::domain::workspace_state::NonEmpty; @@ -174,6 +176,165 @@ pub(crate) fn kill_containers(runner: &dyn Runner, ids: &NonEmpty) -> An answer(runner.capture(&spec)) } +/// What listing every compose container on the machine may cost. +const LIST_COMPOSE_CONTAINERS: Duration = Duration::from_secs(10); + +/// What force-removing a handful of containers may cost. A `kill` and an unlink +/// per container, done by the daemon; bounded because the launch path calls it +/// in front of a `devpod up`, and a daemon that never answers must not turn into +/// a launch that never starts. +const REMOVE_THEM: Duration = Duration::from_secs(60); + +/// One container docker made for a compose project, and the paths its compose +/// labels name. +/// +/// The paths are what devpod's own delete does *not* look at: devpod finds a +/// workspace's container by `dev.containers.id=`, and a workspace recreated +/// under a new uid no longer matches the containers the old one left. The +/// compose labels still point at the clone and at devpod's per-workspace +/// directory, whatever uid made them. +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct ComposeContainer { + /// The full container id. + pub(crate) id: String, + /// The container's name, for the line that says it went. + pub(crate) name: String, + /// `com.docker.compose.project.working_dir`, when it is set. + pub(crate) working_dir: Option, + /// `com.docker.compose.project.config_files`, split on its commas. + pub(crate) config_files: Vec, +} + +impl ComposeContainer { + /// Every path the compose labels name. + pub(crate) fn paths(&self) -> impl Iterator { + self.working_dir + .as_deref() + .into_iter() + .chain(self.config_files.iter().map(PathBuf::as_path)) + } +} + +/// Why docker could not list its compose containers. +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) enum Unlisted { + /// No docker on PATH: a machine with no docker made no containers. + NotInstalled, + /// docker was there and did not answer, in these words. + Failed(String), +} + +/// The `--format` template [`compose_containers`] reads back: one tab-separated +/// line per container. +const COMPOSE_FORMAT: &str = "{{.ID}}\t{{.Names}}\t\ +{{.Label \"com.docker.compose.project.working_dir\"}}\t\ +{{.Label \"com.docker.compose.project.config_files\"}}"; + +/// Every container on this machine that a compose project made, running or not. +pub(crate) fn compose_containers(runner: &dyn Runner) -> Result, Unlisted> { + let stdout = listing( + runner, + [ + "ps", + "--all", + "--no-trunc", + "--filter", + "label=com.docker.compose.project", + "--format", + COMPOSE_FORMAT, + ] + .map(str::to_owned), + )?; + Ok(stdout.lines().filter_map(compose_container).collect()) +} + +/// Which of these containers docker still has, running or not, by full id. +/// +/// For after a removal that failed: `docker rm` exits non-zero when any one of +/// several is already gone, and still removes the rest. +pub(crate) fn present_containers( + runner: &dyn Runner, + ids: &NonEmpty, +) -> Result, Unlisted> { + let mut args = ["ps", "--quiet", "--all", "--no-trunc"] + .map(str::to_owned) + .to_vec(); + for id in ids.iter() { + args.push("--filter".to_owned()); + args.push(format!("id={id}")); + } + let stdout = listing(runner, args)?; + Ok(stdout + .lines() + .filter_map(|line| line.split_whitespace().next()) + .map(str::to_owned) + .collect()) +} + +/// One `docker ps`, its stdout, or why docker would not give it. +fn listing( + runner: &dyn Runner, + args: impl IntoIterator, +) -> Result { + let spec = SpawnSpec::from(Invocation::new(PROGRAM).with_args(args)) + .with_timeout(LIST_COMPOSE_CONTAINERS); + match ran(runner.capture(&spec)) { + Ok((exit, CapturedText { stdout, .. })) if exit.is_success() => Ok(stdout), + Ok((exit, CapturedText { stderr, .. })) => Err(Unlisted::Failed(format!( + "docker ps failed ({exit:?}): {}", + stderr.trim() + ))), + Err(NoAnswer::NotInstalled) => Err(Unlisted::NotInstalled), + Err(NoAnswer::NotStarted(failure)) if failure.kind == ErrorKind::TimedOut => Err( + Unlisted::Failed("docker ps did not answer in time".to_owned()), + ), + Err(NoAnswer::NotStarted(failure)) => Err(Unlisted::Failed(format!( + "docker ps could not start ({:?})", + failure.kind + ))), + } +} + +/// One line of [`COMPOSE_FORMAT`], or nothing for a line that is not one. +fn compose_container(line: &str) -> Option { + let mut fields = line.split('\t'); + let id = fields.next()?.trim(); + let name = fields.next()?.trim(); + if id.is_empty() { + return None; + } + let working_dir = fields + .next() + .map(str::trim) + .filter(|dir| !dir.is_empty()) + .map(PathBuf::from); + let config_files = fields + .next() + .unwrap_or_default() + .split(',') + .map(str::trim) + .filter(|file| !file.is_empty()) + .map(PathBuf::from) + .collect(); + Some(ComposeContainer { + id: id.to_owned(), + name: name.to_owned(), + working_dir, + config_files, + }) +} + +/// Remove these containers, running or not, in one call. +/// +/// `--force` because a leftover is usually still running: it is the container a +/// delete did not stop. +pub(crate) fn remove_containers(runner: &dyn Runner, ids: &NonEmpty) -> Answer { + let mut args = vec!["rm".to_owned(), "--force".to_owned()]; + args.extend(ids.iter().cloned()); + let spec = SpawnSpec::from(Invocation::new(PROGRAM).with_args(args)).with_timeout(REMOVE_THEM); + answer(runner.capture(&spec)) +} + /// What asking docker for every container may cost before it is abandoned. const LIST_EVERY_CONTAINER: Duration = Duration::from_secs(5); diff --git a/rust/devlaunch-core/src/domain/workspace_state.rs b/rust/devlaunch-core/src/domain/workspace_state.rs index 5f873d1a..7ea8acd9 100644 --- a/rust/devlaunch-core/src/domain/workspace_state.rs +++ b/rust/devlaunch-core/src/domain/workspace_state.rs @@ -396,6 +396,13 @@ impl NonEmpty { pub fn iter(&self) -> impl Iterator { std::iter::once(&self.first).chain(self.rest.iter()) } + + pub(crate) fn map(&self, mut f: impl FnMut(&T) -> U) -> NonEmpty { + NonEmpty { + first: f(&self.first), + rest: self.rest.iter().map(f).collect(), + } + } } /// What one workspace clone holds, as far as git can tell. diff --git a/rust/devlaunch-core/src/domain/xdg.rs b/rust/devlaunch-core/src/domain/xdg.rs index cec1ed9a..08f459c7 100644 --- a/rust/devlaunch-core/src/domain/xdg.rs +++ b/rust/devlaunch-core/src/domain/xdg.rs @@ -141,6 +141,24 @@ pub fn clone_root_in(cache: &Path) -> PathBuf { cache.join("repos") } +/// Whether `path` is the clone dl makes for `workspace_id` under `clone_root`: +/// `///`, exactly. +/// +/// Not merely a path under the root: the repository's directory above a clone +/// holds every clone of that repository, so a test by prefix would read the +/// repository's directory as one workspace's clone and its clones as that +/// workspace's. +pub(crate) fn is_clone_of(clone_root: &Path, path: &Path, workspace_id: &str) -> bool { + use std::path::Component::Normal; + path.strip_prefix(clone_root).is_ok_and(|under| { + let parts: Vec<_> = under.components().collect(); + matches!( + parts.as_slice(), + [Normal(_), Normal(_), Normal(leaf)] if *leaf == std::ffi::OsStr::new(workspace_id) + ) + }) +} + /// The whole XDG rule, as a function of its two inputs. /// /// An empty value counts as unset, which is what the XDG basedir spec says and @@ -163,6 +181,20 @@ mod tests { use std::ffi::OsString; use std::path::{Path, PathBuf}; + /// A clone dl makes is `///`, and only that: + /// the repository's directory above it holds every clone of the repository, + /// and a path elsewhere is somebody's own directory. + #[test] + fn only_the_owner_repo_id_directory_is_a_workspaces_clone() { + let root = Path::new("/cache/devlaunch/repos"); + + assert!(is_clone_of(root, &root.join("o/r/ws"), "ws")); + assert!(!is_clone_of(root, &root.join("o/r"), "r")); + assert!(!is_clone_of(root, &root.join("o/r/ws"), "other")); + assert!(!is_clone_of(root, &root.join("o/r/ws/sub"), "sub")); + assert!(!is_clone_of(root, Path::new("/home/me/o/r/ws"), "ws")); + } + fn home() -> PathBuf { PathBuf::from("/home/someone") } diff --git a/rust/devlaunch-core/src/flows/launch.rs b/rust/devlaunch-core/src/flows/launch.rs index 33713064..2ecdf300 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -90,6 +90,7 @@ use crate::flows::lifecycle::{ self, KnownWorkspace, LifecycleNotice, Refresh, RefreshReason, StopOutcome, }; use crate::flows::listing::CommandContext; +use crate::flows::orphan_containers::OrphanSweep; use crate::flows::provision::verdict_cache::VerdictCache; use crate::flows::provision::{ self, ClaudeConfig, DevpodMissing, HostLayout, PassOccasion, ProvisionEvent, Provisioning, @@ -528,6 +529,27 @@ pub enum LaunchNotice { /// binds and this host does not set. docker names no variable, so neither does /// this -- it says where to look instead. MountSourceEmpty, + /// Containers an earlier incarnation of this workspace left behind, removed + /// before the `up` so it does not trip over their fixed `container_name`. + /// devpod held no record of the workspace, so nothing still owned them; see + /// `flows::orphan_containers`. + LeftoverContainersRemoved { + workspace_id: String, + names: NonEmpty, + }, + /// docker would not list containers, in its own words, so leftovers were not + /// looked for. The `up` goes ahead, and will name the container it trips over. + LeftoverContainersNotListed { + workspace_id: String, + reason: String, + }, + /// Leftover containers docker would not remove, in its own words. The `up` + /// goes ahead, and will name the container it trips over. + LeftoverContainersNotRemoved { + workspace_id: String, + names: NonEmpty, + reason: String, + }, // --- the dotfiles this `up` asked devpod for (devlaunch#560) // @@ -1256,6 +1278,222 @@ impl<'a> Naming<'a> { } } +/// Remove the containers a forgotten earlier incarnation of `workspace_id` +/// left, and say so. +/// +/// `source` counts as the workspace's clone only when it is the directory +/// devlaunch clones `workspace_id` into, `///`: a +/// local folder is the user's, and a compose project they started there by hand +/// is not a leftover; a repository's directory above the clones holds other +/// workspaces' clones, whose containers are not this one's. +fn sweep_leftovers( + runner: &dyn Runner, + host: &Host, + workspace_id: &str, + source: &str, + notices: &mut dyn Notices, +) { + let source = Path::new(source); + let clone_root = crate::domain::xdg::clone_root_in(&host.cache_dir); + let clone = + crate::domain::xdg::is_clone_of(&clone_root, source, workspace_id).then_some(source); + match crate::flows::orphan_containers::sweep( + runner, + host.devpod_home.as_ref(), + workspace_id, + clone, + ) { + OrphanSweep::Removed(names) => notices.say(LaunchNotice::LeftoverContainersRemoved { + workspace_id: workspace_id.to_owned(), + names, + }), + OrphanSweep::NotListed { reason } => { + notices.say(LaunchNotice::LeftoverContainersNotListed { + workspace_id: workspace_id.to_owned(), + reason, + }) + } + OrphanSweep::NotRemoved { names, reason } => { + notices.say(LaunchNotice::LeftoverContainersNotRemoved { + workspace_id: workspace_id.to_owned(), + names, + reason, + }); + } + OrphanSweep::Nothing | OrphanSweep::NoDocker => {} + } +} + +#[cfg(test)] +mod leftover_tests { + use devlaunch_test_support::{FakeRunner, Response}; + + use super::*; + use crate::clients::devpod_home::{agent_compose_file, devpod_home_with}; + + fn host_with(home: &DevpodHome, cache: &Path) -> Host { + let mut host = Host::from_process(cache); + host.devpod_home = Some(home.clone()); + host + } + + /// A workspace devpod has no record of, about to be created over containers + /// an older one left: they go before the `up`, and the launch names them. A + /// container that only names the clone counts, because the clone is dl's. + #[test] + fn leftovers_of_a_forgotten_workspace_are_removed_before_the_up() { + let home = devpod_home_with(&[]); + let cache = Path::new("/cache/devlaunch"); + let clone = crate::domain::xdg::clone_root_in(cache).join("o/r/ws"); + let fake = FakeRunner::new(); + fake.script( + ["docker", "ps"], + Response::stdout(format!( + "aaa\tdev_ws\t{dir}\t{dir}/a.yml,{agent}\nbbb\tside_ws\t{dir}\t{dir}/b.yml\n", + dir = clone.join(".devcontainer").display(), + agent = agent_compose_file(&home, "default", "ws").display(), + )), + ); + let mut notices: Vec = Vec::new(); + + sweep_leftovers( + &fake, + &host_with(&home, cache), + "ws", + &clone.display().to_string(), + &mut notices, + ); + + assert_eq!( + fake.args_to("docker").last().expect("a removal"), + &["rm", "--force", "aaa", "bbb"] + ); + assert_eq!( + notices, + [LaunchNotice::LeftoverContainersRemoved { + workspace_id: "ws".to_owned(), + names: NonEmpty::of(["dev_ws".to_owned(), "side_ws".to_owned()]) + .expect("two names"), + }] + ); + } + + /// A docker that will not list its containers is said, in its own words, + /// and nothing is removed. + #[test] + fn a_listing_docker_refuses_is_said_in_dockers_words() { + let home = devpod_home_with(&[]); + let fake = FakeRunner::new(); + fake.script(["docker", "ps"], Response::failed(1, "daemon is down\n")); + let mut notices: Vec = Vec::new(); + + sweep_leftovers( + &fake, + &host_with(&home, Path::new("/cache/devlaunch")), + "ws", + "/home/me/ws", + &mut notices, + ); + + assert_eq!( + notices, + [LaunchNotice::LeftoverContainersNotListed { + workspace_id: "ws".to_owned(), + reason: "docker ps failed (Code(1)): daemon is down".to_owned(), + }] + ); + assert_eq!(fake.args_to("docker").len(), 1, "a listing and no removal"); + } + + /// A docker that will not remove the leftovers is said, with their names + /// and docker's words. + #[test] + fn a_removal_docker_refuses_is_said_in_dockers_words() { + let home = devpod_home_with(&[]); + let fake = FakeRunner::new(); + fake.script( + ["docker", "ps"], + Response::stdout(format!( + "aaa\tdev_ws\t/x\t{}\n", + agent_compose_file(&home, "default", "ws").display() + )), + ); + fake.script(["docker", "rm"], Response::failed(1, "daemon said no\n")); + let mut notices: Vec = Vec::new(); + + sweep_leftovers( + &fake, + &host_with(&home, Path::new("/cache/devlaunch")), + "ws", + "/home/me/ws", + &mut notices, + ); + + assert_eq!( + notices, + [LaunchNotice::LeftoverContainersNotRemoved { + workspace_id: "ws".to_owned(), + names: NonEmpty::of(["dev_ws".to_owned()]).expect("one name"), + reason: "docker rm failed (Code(1)): daemon said no".to_owned(), + }] + ); + } + + /// A local folder is the user's directory: a container that names only the + /// folder, and not devpod's directory for the workspace, is not a leftover. + #[test] + fn a_local_folder_is_not_treated_as_a_clone() { + let home = devpod_home_with(&[]); + let fake = FakeRunner::new(); + fake.script( + ["docker", "ps"], + Response::stdout("aaa\tmy_db\t/home/me/proj\t/home/me/proj/compose.yml\n"), + ); + let mut notices: Vec = Vec::new(); + + sweep_leftovers( + &fake, + &host_with(&home, Path::new("/cache/devlaunch")), + "proj", + "/home/me/proj", + &mut notices, + ); + + assert_eq!(fake.args_to("docker").len(), 1, "{:?}", fake.argvs()); + assert!(notices.is_empty()); + } + + /// A repository's directory under the clone root holds every clone of that + /// repository: launched as a folder, it is not the clone of a workspace named + /// after its leaf, and the live workspaces' containers under it stay. + #[test] + fn a_launch_at_a_clones_parent_leaves_the_clones_containers() { + let home = devpod_home_with(&[]); + let cache = Path::new("/cache/devlaunch"); + let repo = crate::domain::xdg::clone_root_in(cache).join("o/r"); + let fake = FakeRunner::new(); + fake.script( + ["docker", "ps"], + Response::stdout(format!( + "aaa\tdev_ws\t{dir}\t{dir}/a.yml\n", + dir = repo.join("ws/.devcontainer").display(), + )), + ); + let mut notices: Vec = Vec::new(); + + sweep_leftovers( + &fake, + &host_with(&home, cache), + "r", + &repo.display().to_string(), + &mut notices, + ); + + assert_eq!(fake.args_to("docker").len(), 1, "{:?}", fake.argvs()); + assert!(notices.is_empty()); + } +} + /// One `devpod up`, as data. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub(crate) struct UpRequest<'a> { @@ -1865,6 +2103,14 @@ fn up_under_stage( // nothing at all on the fast-attach arm, which never reaches this function. ensure_a_provider(context.runner(), notices); + // Inside the launch lock and in front of the `up`: a workspace devpod holds + // no record of gets a new compose project, and a container an older one left + // under a fixed `container_name` makes that project's create fail. The sweep + // does nothing for a workspace devpod still holds. + if let Some(identity) = request.naming.identity() { + sweep_leftovers(context.runner(), host, identity, request.source, notices); + } + // Give every workspace the host's gh login, whatever its devcontainer.json // does or does not set up for itself. The file is removed when `staged` drops, // which is the whole of Python's context manager — including on the path where diff --git a/rust/devlaunch-core/src/flows/lifecycle/delete.rs b/rust/devlaunch-core/src/flows/lifecycle/delete.rs index 2663e750..c9b1031b 100644 --- a/rust/devlaunch-core/src/flows/lifecycle/delete.rs +++ b/rust/devlaunch-core/src/flows/lifecycle/delete.rs @@ -1,6 +1,6 @@ //! `dl rm`: a workspace, its clone, and the volumes its devcontainer made. -use std::path::Path; +use std::path::{Path, PathBuf}; use std::time::Duration; use super::delete_guard::{ @@ -17,6 +17,7 @@ use crate::domain::workspace_state::NonEmpty; use crate::flows::kept_copies::{self, KeptCopies}; use crate::flows::kill; use crate::flows::listing::CommandContext; +use crate::flows::orphan_containers::{self, OrphanSweep}; use crate::flows::workspace_clone::{RemoveWorkspaceError, Removed, WorkspaceCloneManager}; use crate::notices::Notices; use crate::runner::{Exit, OsFailure, Runner}; @@ -274,6 +275,9 @@ pub(crate) fn workspace_delete( // names live. Named afterwards, this would find nothing every time and look // like a working cleanup. let named = devcontainer_volumes(devpod_home, workspace_id); + // Named before the delete for the same reason, and before the clone goes + // with it: the leftover sweep below matches containers by the clone's path. + let clone_path = managed_clone(clones, storage, workspace_id); // The lock line is answered the way a launch answers it (devlaunch#602): said, // and then *cleared*, by the same sweep `up` runs. It used to be said and // nothing more, with advice to run `kill` in another terminal, which left an @@ -311,6 +315,13 @@ pub(crate) fn workspace_delete( // `NotRun` here used to mean devpod was missing or would not exec. Err(NotRun::TimedOut) => { context.forget_workspaces(); + sweep_leftovers( + context.runner(), + devpod_home, + workspace_id, + clone_path.as_deref(), + notices, + ); refresh.ask(context.runner(), RefreshReason::Forced); return Err(NotRun::TimedOut); } @@ -319,6 +330,18 @@ pub(crate) fn workspace_delete( // Unconditionally: a delete that reports failure may still have got far enough // to change what devpod lists. context.forget_workspaces(); + // On every outcome where devpod ran, and before the clone goes. The sweep + // acts only once devpod holds no record of this workspace in any context, so + // a delete devpod refused, which leaves the record, keeps its containers; a + // `--force` delete that gave up on `compose down` and dropped the record + // anyway does not leave them running. + sweep_leftovers( + context.runner(), + devpod_home, + workspace_id, + clone_path.as_deref(), + notices, + ); if !exit.is_success() { refresh.ask(context.runner(), RefreshReason::Forced); return Ok(RemoveOutcome::DevpodRefused { exit }); @@ -376,6 +399,56 @@ pub(crate) fn workspace_delete( Ok(RemoveOutcome::Deleted { clone, volumes }) } +/// This workspace's clone, when it is one devlaunch made, for the leftover +/// sweep to match container labels against. +/// +/// Only a clone in the shape devlaunch makes, `///`: +/// a recorded path anywhere else is somebody's directory, or a repository's +/// directory holding other workspaces' clones, and a compose project there is +/// not a leftover of this workspace. +fn managed_clone( + clones: &WorkspaceCloneManager<'_>, + storage: &MetadataStorage, + workspace_id: &str, +) -> Option { + let recorded = storage.get_worktree_by_workspace_id(workspace_id)?; + // The removal below says whatever naming the clone has to say; this read is + // silent so it is not said twice. + let mut unsaid = Vec::new(); + let path = clones.resolve_clone_path(recorded, &mut unsaid)?; + crate::domain::xdg::is_clone_of(clones.repos_dir(), &path, workspace_id).then_some(path) +} + +/// Remove the containers a deleted workspace left behind, and say so. +fn sweep_leftovers( + runner: &dyn Runner, + devpod_home: Option<&DevpodHome>, + workspace_id: &str, + clone: Option<&Path>, + notices: &mut dyn Notices, +) { + match orphan_containers::sweep(runner, devpod_home, workspace_id, clone) { + OrphanSweep::Removed(names) => notices.say(LifecycleNotice::LeftoverContainersRemoved { + workspace_id: workspace_id.to_owned(), + names, + }), + OrphanSweep::NotListed { reason } => { + notices.say(LifecycleNotice::LeftoverContainersNotListed { + workspace_id: workspace_id.to_owned(), + reason, + }) + } + OrphanSweep::NotRemoved { names, reason } => { + notices.say(LifecycleNotice::LeftoverContainersNotRemoved { + workspace_id: workspace_id.to_owned(), + names, + reason, + }); + } + OrphanSweep::Nothing | OrphanSweep::NoDocker => {} + } +} + /// Remove the volumes `named`, and say what became of them. /// /// The one place a [`docker::Answer`] becomes a [`VolumeSweep`], so every removal diff --git a/rust/devlaunch-core/src/flows/lifecycle/notices.rs b/rust/devlaunch-core/src/flows/lifecycle/notices.rs index 17cbbaa6..e11520a1 100644 --- a/rust/devlaunch-core/src/flows/lifecycle/notices.rs +++ b/rust/devlaunch-core/src/flows/lifecycle/notices.rs @@ -5,6 +5,7 @@ use std::path::PathBuf; use super::delete::{SweepOccasion, VolumeRefusal}; use super::delete_guard::RemovalRefused; use crate::domain::metadata::{self}; +use crate::domain::workspace_state::NonEmpty; use crate::flows::repo_manager::CacheNotice; use crate::flows::workspace_clone::RemoveWorkspaceError; use crate::notices::{Notices, Wrapped}; @@ -44,6 +45,27 @@ pub enum LifecycleNotice { occasion: SweepOccasion, refusal: VolumeRefusal, }, + /// Containers this workspace left behind were removed, named. devpod's own + /// delete finds a container by the uid of the record it is deleting, so a + /// compose project from an older record, or one a forced delete gave up on, + /// outlives it; see `flows::orphan_containers`. + LeftoverContainersRemoved { + workspace_id: String, + names: NonEmpty, + }, + /// docker would not list containers, so whether this workspace left any + /// behind is not known; docker said why. + LeftoverContainersNotListed { + workspace_id: String, + reason: String, + }, + /// Containers this workspace left behind are still on this machine, and + /// docker said why. + LeftoverContainersNotRemoved { + workspace_id: String, + names: NonEmpty, + reason: String, + }, /// A clone directory went and its `metadata.json` record could not be /// dropped. Named by the path, which is what the record described. /// diff --git a/rust/devlaunch-core/src/flows/lifecycle/tests.rs b/rust/devlaunch-core/src/flows/lifecycle/tests.rs index 27d99631..22d0af47 100644 --- a/rust/devlaunch-core/src/flows/lifecycle/tests.rs +++ b/rust/devlaunch-core/src/flows/lifecycle/tests.rs @@ -50,7 +50,9 @@ use super::*; use crate::clients::devpod::{ ContainerState, ListingUnreadable, NotRun, Patience, Workspace, WorkspaceSource, }; -use crate::clients::devpod_home::{DevpodHome, RepointFailure, ScratchHome, devpod_home_with}; +use crate::clients::devpod_home::{ + DevpodHome, RepointFailure, ScratchHome, agent_compose_file, devpod_home_with, +}; use crate::clients::git::Git; use crate::clients::{devpod, docker}; use crate::domain::locks; @@ -2348,6 +2350,60 @@ fn a_delete_killed_at_its_deadline_still_invalidates_the_listing() { ); } +/// A delete killed at its deadline may still have dropped devpod's record, and +/// the containers it was composing down with it. With no record left, the +/// leftovers are swept on this arm too, not only when devpod answered. +#[test] +fn a_delete_killed_at_its_deadline_still_removes_the_containers_left_behind() { + let world = a_stopping_world(); + world + .devpod + .fake + .script(["devpod", "delete"], Response::TimedOut); + let home = devpod_home_with(&[]); + let override_file = agent_compose_file(&home, "default", "myws"); + world.devpod.fake.script( + ["docker", "ps"], + Response::stdout(format!( + "aaa\tmyws_dev\t/elsewhere\t{}\n", + override_file.display() + )), + ); + let mut world_cache = World::empty(); + let clones = clones_for(&world_cache.repos_dir, &world_cache.devpod); + let mut context = CommandContext::new(&world.devpod); + let mut refresh = Refresh::new(&world.updater, &world.cache_path); + let mut notices = Vec::new(); + + let copies = world_cache.copies(); + let outcome = workspace_delete( + &mut context, + &mut refresh, + &clones, + &mut world_cache.storage, + Some(&home), + &copies, + "myws", + Insistence::Insisted, + Persistence::Wedged, + &mut unblocked(), + &mut notices, + ); + + assert_eq!(outcome, Err(NotRun::TimedOut)); + assert_eq!( + world.devpod.docker_argvs().last().expect("a removal"), + &["rm", "--force", "aaa"] + ); + assert!( + notices.contains(&LifecycleNotice::LeftoverContainersRemoved { + workspace_id: "myws".to_owned(), + names: NonEmpty::one("myws_dev".to_owned()), + }), + "{notices:?}" + ); +} + /// And a devpod that never started leaves the listing alone, which is the /// distinction the arm above turns on: nothing ran, so nothing changed, and a /// forced refresh would be a background `devpod list` bought for nothing. diff --git a/rust/devlaunch-core/src/flows/mod.rs b/rust/devlaunch-core/src/flows/mod.rs index b5e2739a..9cd5feb1 100644 --- a/rust/devlaunch-core/src/flows/mod.rs +++ b/rust/devlaunch-core/src/flows/mod.rs @@ -27,6 +27,8 @@ pub mod lifecycle; pub mod listing; // binary surface — not part of the frozen wf API (#251 §7) pub mod migration; +// Crate-private: what a sweep found reaches the binary as a notice. +pub(crate) mod orphan_containers; // binary surface — not part of the frozen wf API (#251 §7) pub mod provision; // binary surface — not part of the frozen wf API (#251 §7) diff --git a/rust/devlaunch-core/src/flows/orphan_containers.rs b/rust/devlaunch-core/src/flows/orphan_containers.rs new file mode 100644 index 00000000..fe7d9bd1 --- /dev/null +++ b/rust/devlaunch-core/src/flows/orphan_containers.rs @@ -0,0 +1,657 @@ +//! Containers a workspace left behind after devpod forgot it. +//! +//! `devpod delete` finds a workspace's container by one label, +//! `dev.containers.id=`, and composes the project down only if it finds that +//! container. Two ordinary events break the match and leave the compose project +//! running with nothing on the machine that still owns it: +//! +//! - **A forced delete.** `devpod delete --force` (what `dl kill` runs) +//! logs a failed `compose down` and removes the record anyway. A compose file +//! that no longer interpolates is enough: a required variable from a `.env` +//! the `initializeCommand` wrote, gone from the clone. +//! - **A record recreated under a new uid.** A `devpod up` over leftover +//! containers makes a new record with a new uid, then fails on the leftovers' +//! fixed `container_name`. The next `devpod delete` looks for the *new* uid, +//! finds no container, and reports success. The leftovers stay up. +//! +//! The leftovers then block every later `up` of the same workspace, because a +//! compose file that pins `container_name:` cannot make a second container under +//! the same name in a new project. +//! +//! # Which containers count +//! +//! Only containers of a workspace devpod holds **no record for in any context**. +//! With a record somewhere, devpod still owns the workspace and dl stays out of +//! its containers. A record dl cannot read counts as one: only a "not found" +//! everywhere a record could be is "no record". Without one, a container is this workspace's leftover when one +//! of its compose labels (`working_dir`, `config_files`) names a path inside: +//! +//! - devpod's agent-side directory for this workspace id, under any context. +//! devpod writes its compose override there for every compose devcontainer, +//! so this one handle names the workspace by id whatever uid made the project; +//! - or the workspace's clone, when it is one devlaunch made. A local folder +//! opened with `dl ./folder` is the user's directory, and a compose project +//! the user started there by hand is not dl's to remove, so a local folder is +//! matched by the devpod directory alone. +//! +//! And a container is left whenever anything else may own it: a label that +//! names the devpod directory of a different id, or a path in a folder devpod +//! holds as another workspace's source inside the clone. When devpod's records +//! would not read, the clone alone matches nothing. +//! +//! Nothing is matched on a container *name*: names are the devcontainer's to +//! choose, and a pattern over them would reach other people's containers. + +use std::path::Path; + +use crate::clients::devpod_home::{DevpodHome, RecordPresence}; +use crate::clients::docker::{self, ComposeContainer, Unlisted}; +use crate::domain::workspace_state::NonEmpty; + +/// What a sweep for one workspace's leftover containers came to. +/// +/// Two silent arms and three that are worth a line, on the line every other +/// docker cleanup in dl draws: nothing to do and no docker to do it with are +/// silent; a removal, or a docker that would not deliver, is said. +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) enum OrphanSweep { + /// Nothing to remove: devpod still holds the workspace, no container names + /// it, or there is no devpod home to check against. + Nothing, + /// No docker on this machine, so nothing was made to be left. + NoDocker, + /// These containers were removed, by name. + Removed(NonEmpty), + /// docker would not list the containers, in its own words. + NotListed { reason: String }, + /// docker would not remove these containers, in its own words. + NotRemoved { + names: NonEmpty, + reason: String, + }, +} + +/// Remove the containers this workspace left behind, if devpod has forgotten it. +/// +/// `clone` is the workspace's clone directory when devlaunch made it, and `None` +/// for anything else (see the module docs for why a local folder is not passed). +/// +/// One `docker ps` when devpod holds no record for the id, and no docker call at +/// all when it does: the common case, a workspace devpod still knows, costs a +/// directory listing. +pub(crate) fn sweep( + runner: &dyn crate::runner::Runner, + devpod_home: Option<&DevpodHome>, + workspace_id: &str, + clone: Option<&Path>, +) -> OrphanSweep { + let Some(home) = devpod_home else { + return OrphanSweep::Nothing; + }; + if workspace_id.is_empty() || home.record_presence(workspace_id) != RecordPresence::Absent { + return OrphanSweep::Nothing; + } + let containers = match docker::compose_containers(runner) { + Ok(containers) => containers, + Err(Unlisted::NotInstalled) => return OrphanSweep::NoDocker, + Err(Unlisted::Failed(reason)) => return OrphanSweep::NotListed { reason }, + }; + let clone = clone.map(|path| CloneDir::of(path, home)); + let Some(left) = NonEmpty::of( + containers + .iter() + .filter(|container| left_by(container, home, workspace_id, clone.as_ref())), + ) else { + return OrphanSweep::Nothing; + }; + let names = left.map(|c| c.name.clone()); + let ids = left.map(|c| c.id.clone()); + match docker::remove_containers(runner, &ids) { + docker::Answer::Ran { exit, .. } if exit.is_success() => OrphanSweep::Removed(names), + docker::Answer::Ran { exit, stderr } => { + let reason = format!("docker rm failed ({exit:?}): {}", stderr.trim()); + still_there(runner, &left, names, reason) + } + docker::Answer::NotInstalled => OrphanSweep::NoDocker, + docker::Answer::NotStarted(failure) => OrphanSweep::NotRemoved { + names, + reason: format!("docker rm did not finish ({:?})", failure.kind), + }, + } +} + +/// What a removal that docker said failed came to, asked again. +/// +/// `docker rm` exits non-zero when any one of several containers is already +/// gone, and still removes the rest. So the sweep lists them again and names only +/// those still there; when none is, every one of them is gone. When the second +/// listing fails too, all of them are named, as the first answer said. +fn still_there( + runner: &dyn crate::runner::Runner, + left: &NonEmpty<&ComposeContainer>, + names: NonEmpty, + reason: String, +) -> OrphanSweep { + let ids = left.map(|c| c.id.clone()); + let Ok(present) = docker::present_containers(runner, &ids) else { + return OrphanSweep::NotRemoved { names, reason }; + }; + match NonEmpty::of( + left.iter() + .filter(|c| present.contains(&c.id)) + .map(|c| c.name.clone()), + ) { + Some(names) => OrphanSweep::NotRemoved { names, reason }, + None => OrphanSweep::Removed(names), + } +} + +/// Whether one container's compose labels name this workspace, and nothing +/// else that could own it. +/// +/// A container is left whenever any doubt remains: +/// +/// - a label that names the devpod directory of a *different* workspace id +/// makes the container that workspace's, whatever else it names; +/// - a label inside the source folder of a workspace devpod holds, where that +/// folder is the clone or inside it, makes the container that workspace's: a +/// local folder opened as its own workspace can sit inside a clone; +/// - when devpod's records would not read, the clone alone proves nothing, +/// and only this id's devpod directory counts. +fn left_by( + container: &ComposeContainer, + home: &DevpodHome, + workspace_id: &str, + clone: Option<&CloneDir<'_>>, +) -> bool { + let mut by_id = false; + let mut in_clone = false; + for path in container.paths() { + match home.agent_dir_owner(path) { + Some(owner) if owner == workspace_id => by_id = true, + Some(_) => return false, + None => {} + } + if let Some(clone) = clone { + if clone + .held_inside + .as_ref() + .is_some_and(|held| held.iter().any(|folder| path.starts_with(folder))) + { + return false; + } + in_clone |= path.starts_with(clone.path); + } + } + by_id || (in_clone && clone.is_some_and(|clone| clone.held_inside.is_some())) +} + +/// The workspace's clone, and the folders inside it that devpod holds as other +/// workspaces' sources (nothing when devpod's records would not say). +struct CloneDir<'a> { + path: &'a Path, + held_inside: Option>, +} + +impl<'a> CloneDir<'a> { + fn of(path: &'a Path, home: &DevpodHome) -> Self { + let held_inside = home.held_local_folders().map(|folders| { + folders + .into_iter() + .filter(|folder| folder.starts_with(path)) + .collect() + }); + Self { path, held_inside } + } +} + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use devlaunch_test_support::{FakeRunner, Response}; + + use super::*; + use crate::clients::devpod_home::{agent_compose_file, devpod_home_with, record_under_context}; + + const WS: &str = "kinisi-ros-ci-sim-shard-rebalance-cd1e"; + + fn names(names: [String; N]) -> NonEmpty { + NonEmpty::of(names).expect("at least one name") + } + + /// One `docker ps` row in the template `compose_containers` asks for. + fn row(id: &str, name: &str, working_dir: &Path, config_files: &[&Path]) -> String { + let files: Vec = config_files + .iter() + .map(|f| f.display().to_string()) + .collect(); + format!( + "{id}\t{name}\t{}\t{}\n", + working_dir.display(), + files.join(",") + ) + } + + fn clone_dir() -> PathBuf { + PathBuf::from("/cache/devlaunch/repos/org/repo").join(WS) + } + + /// The case from the field: devpod has no record, and the old project's two + /// containers, the dev container and a sidecar from a second compose file, + /// are both still there. Both go, in one call, and the sweep names them. + #[test] + fn a_forgotten_workspaces_containers_are_removed_by_their_compose_labels() { + let home = devpod_home_with(&[]); + let override_file = agent_compose_file(&home, "default", WS); + let clone = clone_dir(); + let devcontainer = clone.join(".devcontainer"); + let compose = devcontainer.join("docker-compose.sim.x64.yml"); + let fake = FakeRunner::new(); + fake.script( + ["docker", "ps"], + Response::stdout( + row( + "aaa", + &format!("kinisi_jazzy_{WS}"), + &devcontainer, + &[&compose, &override_file], + ) + &row( + "bbb", + &format!("kinisi_unreal_runtime_1.6.1_{WS}"), + &devcontainer, + &[&compose, &override_file], + ) + &row( + "ccc", + "someone_elses", + Path::new("/home/other/.devcontainer"), + &[Path::new("/home/other/.devcontainer/compose.yml")], + ), + ), + ); + + let swept = sweep(&fake, Some(&home), WS, Some(&clone)); + + assert_eq!( + swept, + OrphanSweep::Removed(names([ + format!("kinisi_jazzy_{WS}"), + format!("kinisi_unreal_runtime_1.6.1_{WS}"), + ])) + ); + assert_eq!( + fake.args_to("docker").last().expect("a removal"), + &["rm", "--force", "aaa", "bbb"] + ); + } + + /// devpod still holds the workspace, so its containers are devpod's: dl does + /// not even ask docker. + #[test] + fn a_workspace_devpod_still_holds_is_left_alone() { + let home = devpod_home_with(&[("default", WS, Some(()))]); + let fake = FakeRunner::new(); + + assert_eq!( + sweep(&fake, Some(&home), WS, Some(&clone_dir())), + OrphanSweep::Nothing + ); + assert!(fake.args_to("docker").is_empty(), "{:?}", fake.argvs()); + } + + /// A record dl cannot read may still be there, so it holds the workspace + /// as surely as one dl can read: root-owned directories from container + /// mounts are ordinary on real hosts. + #[test] + fn a_record_dl_cannot_read_still_holds_it() { + // SAFETY: `geteuid` takes nothing, returns a uid and cannot fail. + if unsafe { libc::geteuid() } == 0 { + eprintln!("skipped: root reads a directory of mode 000"); + return; + } + use std::os::unix::fs::PermissionsExt; + let home = devpod_home_with(&[("default", WS, None)]); + let dir = home + .record("default", WS) + .parent() + .expect("a record directory") + .to_path_buf(); + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o000)) + .expect("a locked record directory"); + let fake = FakeRunner::new(); + + let swept = sweep(&fake, Some(&home), WS, Some(&clone_dir())); + + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)) + .expect("the record directory unlocked again"); + assert_eq!(swept, OrphanSweep::Nothing); + assert!(fake.args_to("docker").is_empty(), "{:?}", fake.argvs()); + } + + /// A context whose name is not UTF-8 is still a directory devpod may have + /// written a record into. + #[test] + fn a_record_under_a_context_name_that_is_not_utf8_holds_it() { + use std::os::unix::ffi::OsStrExt; + let home = devpod_home_with(&[]); + record_under_context(&home, std::ffi::OsStr::from_bytes(b"ctx\xff"), WS); + let fake = FakeRunner::new(); + + assert_eq!(sweep(&fake, Some(&home), WS, None), OrphanSweep::Nothing); + assert!(fake.args_to("docker").is_empty(), "{:?}", fake.argvs()); + } + + /// A record in any context counts, not only the one dl would pick. + #[test] + fn a_record_in_another_context_also_holds_it() { + let home = devpod_home_with(&[("other", WS, None)]); + let fake = FakeRunner::new(); + + assert_eq!(sweep(&fake, Some(&home), WS, None), OrphanSweep::Nothing); + assert!(fake.args_to("docker").is_empty()); + } + + /// The devpod directory is matched by whole path components: `ws` is not + /// `ws-2`, and neither is a clone whose name only starts the same way. + #[test] + fn a_workspace_whose_id_only_starts_the_same_is_not_matched() { + let home = devpod_home_with(&[]); + let longer = format!("{WS}-2"); + let theirs = agent_compose_file(&home, "default", &longer); + let their_clone = PathBuf::from(format!("{}-2", clone_dir().display())); + let fake = FakeRunner::new(); + fake.script( + ["docker", "ps"], + Response::stdout(row( + "ddd", + &format!("kinisi_jazzy_{longer}"), + &their_clone.join(".devcontainer"), + &[&theirs], + )), + ); + + assert_eq!( + sweep(&fake, Some(&home), WS, Some(&clone_dir())), + OrphanSweep::Nothing + ); + assert_eq!(fake.args_to("docker").len(), 1, "a listing and no removal"); + } + + /// Without a clone to match, a container is matched by devpod's directory + /// alone: a compose project somebody started by hand in their own folder + /// names no devpod directory and stays. + #[test] + fn a_local_folder_is_matched_by_the_devpod_directory_only() { + let home = devpod_home_with(&[]); + let folder = PathBuf::from("/home/me/project"); + let override_file = agent_compose_file(&home, "default", "project"); + let fake = FakeRunner::new(); + fake.script( + ["docker", "ps"], + Response::stdout( + row( + "eee", + "project_dev", + &folder.join(".devcontainer"), + &[&folder.join(".devcontainer/compose.yml"), &override_file], + ) + &row("fff", "my_own_db", &folder, &[&folder.join("compose.yml")]), + ), + ); + + assert_eq!( + sweep(&fake, Some(&home), "project", None), + OrphanSweep::Removed(names(["project_dev".to_owned()])) + ); + } + + /// A project whose compose files all sit elsewhere still names the + /// workspace when compose ran it from devpod's directory for the id. + #[test] + fn a_working_dir_in_the_devpod_directory_alone_is_enough() { + let home = devpod_home_with(&[]); + let agent_dir = agent_compose_file(&home, "default", WS) + .parent() + .expect("the compose override's directory") + .to_path_buf(); + let fake = FakeRunner::new(); + fake.script( + ["docker", "ps"], + Response::stdout(row( + "aaa", + "dev", + &agent_dir, + &[Path::new("/srv/elsewhere/compose.yml")], + )), + ); + + assert_eq!( + sweep(&fake, Some(&home), WS, None), + OrphanSweep::Removed(names(["dev".to_owned()])) + ); + assert_eq!( + fake.args_to("docker").last().expect("a removal"), + &["rm", "--force", "aaa"] + ); + } + + /// A local folder opened as its own workspace, `sub`, can sit inside another + /// workspace's clone, `ws`. devpod still holds `sub`; it holds nothing for + /// `ws`. Sweeping `ws` must not take `sub`'s live container just because its + /// compose files are inside `ws`'s clone: its labels name `sub`'s devpod + /// directory, so it is `sub`'s. + #[test] + fn a_held_workspace_nested_in_the_clone_keeps_its_containers() { + let home = devpod_home_with(&[("default", "sub", None)]); + let clone = PathBuf::from("/cache/devlaunch/repos/o/r/ws"); + let sub = clone.join("sub/.devcontainer"); + let fake = FakeRunner::new(); + fake.script( + ["docker", "ps"], + Response::stdout(row( + "aaa", + "sub_dev", + &sub, + &[ + &sub.join("compose.yml"), + &agent_compose_file(&home, "default", "sub"), + ], + )), + ); + + assert_eq!( + sweep(&fake, Some(&home), "ws", Some(&clone)), + OrphanSweep::Nothing + ); + assert_eq!(fake.args_to("docker").len(), 1, "a listing and no removal"); + } + + /// A container that names another workspace's devpod directory is that + /// workspace's, even beside this one's: two owners is one too many to remove. + #[test] + fn a_container_naming_another_workspaces_devpod_directory_stays() { + let home = devpod_home_with(&[]); + let fake = FakeRunner::new(); + fake.script( + ["docker", "ps"], + Response::stdout(row( + "aaa", + "dev", + Path::new("/x"), + &[ + &agent_compose_file(&home, "default", WS), + &agent_compose_file(&home, "default", "other"), + ], + )), + ); + + assert_eq!( + sweep(&fake, Some(&home), WS, Some(&clone_dir())), + OrphanSweep::Nothing + ); + assert_eq!(fake.args_to("docker").len(), 1, "a listing and no removal"); + } + + /// A compose project inside a folder devpod holds as a workspace's source is + /// that workspace's business, even when the folder is inside this clone and + /// the project names no devpod directory at all. + #[test] + fn a_project_in_a_held_workspaces_source_inside_the_clone_stays() { + let home = devpod_home_with(&[("default", "sub", None)]); + let clone = clone_dir(); + let sub = clone.join("sub"); + std::fs::write( + home.record("default", "sub"), + format!(r#"{{"source":{{"localFolder":"{}"}}}}"#, sub.display()), + ) + .expect("a record with a source"); + let fake = FakeRunner::new(); + fake.script( + ["docker", "ps"], + Response::stdout(row("aaa", "their_db", &sub, &[&sub.join("compose.yml")])), + ); + + assert_eq!( + sweep(&fake, Some(&home), WS, Some(&clone)), + OrphanSweep::Nothing + ); + assert_eq!(fake.args_to("docker").len(), 1, "a listing and no removal"); + } + + /// A held record whose source dl cannot read could name a folder inside the + /// clone, so the clone alone proves nothing: only devpod's directory for the + /// id still counts. + #[test] + fn an_unreadable_held_record_leaves_clone_only_matches() { + let home = devpod_home_with(&[("default", "sub", None)]); + std::fs::write(home.record("default", "sub"), "not json").expect("a broken record"); + let clone = clone_dir(); + let dir = clone.join(".devcontainer"); + let fake = FakeRunner::new(); + fake.script( + ["docker", "ps"], + Response::stdout( + row( + "aaa", + "ours", + &dir, + &[&agent_compose_file(&home, "default", WS)], + ) + &row("bbb", "maybe_theirs", &dir, &[&dir.join("b.yml")]), + ), + ); + + assert_eq!( + sweep(&fake, Some(&home), WS, Some(&clone)), + OrphanSweep::Removed(names(["ours".to_owned()])) + ); + } + + #[test] + fn a_listing_docker_refuses_removes_nothing_and_keeps_docker_words() { + let home = devpod_home_with(&[]); + let fake = FakeRunner::new(); + fake.script(["docker", "ps"], Response::failed(1, "daemon is down\n")); + + let swept = sweep(&fake, Some(&home), WS, None); + + assert!( + matches!( + &swept, + OrphanSweep::NotListed { reason } if reason.contains("daemon is down") + ), + "{swept:?}" + ); + assert_eq!(fake.args_to("docker").len(), 1, "a listing and no removal"); + } + + #[test] + fn no_devpod_home_means_nothing_to_check_against() { + let fake = FakeRunner::new(); + + assert_eq!(sweep(&fake, None, WS, None), OrphanSweep::Nothing); + assert!(fake.argvs().is_empty()); + } + + #[test] + fn a_machine_with_no_docker_is_silent() { + let home = devpod_home_with(&[]); + let fake = FakeRunner::new(); + fake.script_missing("docker"); + + assert_eq!(sweep(&fake, Some(&home), WS, None), OrphanSweep::NoDocker); + } + + /// docker exits non-zero when one container of several is already gone, + /// and still removes the rest. The sweep asks again which are still there + /// and names only those as not removed. + #[test] + fn a_partly_failed_removal_names_only_what_is_still_there() { + let home = devpod_home_with(&[]); + let override_file = agent_compose_file(&home, "default", WS); + let fake = FakeRunner::new(); + fake.script(["docker", "ps", "--quiet"], Response::stdout("bbb\n")); + fake.script( + ["docker", "ps"], + Response::stdout( + row("aaa", "dev", Path::new("/x"), &[&override_file]) + + &row("bbb", "side", Path::new("/x"), &[&override_file]), + ), + ); + fake.script( + ["docker", "rm"], + Response::failed(1, "Error response from daemon: No such container: aaa\n"), + ); + + assert_eq!( + sweep(&fake, Some(&home), WS, None), + OrphanSweep::NotRemoved { + names: names(["side".to_owned()]), + reason: "docker rm failed (Code(1)): \ + Error response from daemon: No such container: aaa" + .to_owned(), + } + ); + } + + /// A removal that failed only on containers already gone left nothing + /// behind: every one of them is gone, which is what the sweep says. + #[test] + fn a_failed_removal_that_left_nothing_is_a_removal() { + let home = devpod_home_with(&[]); + let override_file = agent_compose_file(&home, "default", WS); + let fake = FakeRunner::new(); + fake.script(["docker", "ps", "--quiet"], Response::ok()); + fake.script( + ["docker", "ps"], + Response::stdout(row("aaa", "dev", Path::new("/x"), &[&override_file])), + ); + fake.script( + ["docker", "rm"], + Response::failed(1, "Error response from daemon: No such container: aaa\n"), + ); + + assert_eq!( + sweep(&fake, Some(&home), WS, None), + OrphanSweep::Removed(names(["dev".to_owned()])) + ); + } + + #[test] + fn a_removal_docker_refuses_keeps_the_names_and_docker_words() { + let home = devpod_home_with(&[]); + let override_file = agent_compose_file(&home, "default", WS); + let fake = FakeRunner::new(); + fake.script( + ["docker", "ps"], + Response::stdout(row("aaa", "dev", Path::new("/x"), &[&override_file])), + ); + fake.script(["docker", "rm"], Response::failed(1, "daemon said no\n")); + + assert_eq!( + sweep(&fake, Some(&home), WS, None), + OrphanSweep::NotRemoved { + names: names(["dev".to_owned()]), + reason: "docker rm failed (Code(1)): daemon said no".to_owned(), + } + ); + } +} diff --git a/rust/devlaunch-core/src/lib.rs b/rust/devlaunch-core/src/lib.rs index d381e9ae..015fd1d4 100644 --- a/rust/devlaunch-core/src/lib.rs +++ b/rust/devlaunch-core/src/lib.rs @@ -66,7 +66,7 @@ //! **What it still does not reach, and it is not one type.** A type [`api`] //! never re-exports but a promised signature names is reachable from outside //! and classified as binary surface, so a break in it diffs -//! `public-api.rest.txt` alone. Counted rather than guessed at: **41 such types +//! `public-api.rest.txt` alone. Counted rather than guessed at: **42 such types //! own close to six hundred rows over there**, and `scripts/public-api-snapshots.sh //! --print-residual` lists them. //! @@ -82,7 +82,7 @@ //! //! So a `public-api.rest.txt` diff is routine for a row whose subject nothing //! promised names, and a contract change for a row whose subject is one of the -//! 41. `--print-residual` is how you tell which you are looking at. +//! 42. `--print-residual` is how you tell which you are looking at. //! //! `scripts/public-api-snapshots.sh` regenerates both; see "The public-API //! snapshots" in docs/development.md. diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index 54ed32c4..162d1ce8 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -1248,6 +1248,19 @@ fn lifecycle_notice(notice: &LifecycleNotice) -> Option { occasion, refusal, } => volumes_not_removed(workspace_id, *occasion, refusal), + LifecycleNotice::LeftoverContainersRemoved { + workspace_id, + names, + } => leftovers_removed(workspace_id, names), + LifecycleNotice::LeftoverContainersNotListed { + workspace_id, + reason, + } => leftovers_not_listed(workspace_id, reason), + LifecycleNotice::LeftoverContainersNotRemoved { + workspace_id, + names, + reason, + } => leftovers_not_removed(workspace_id, names, reason), LifecycleNotice::RecordNotDropped { path, refusal } => { format!( "Could not drop the record for {}: {}", @@ -1274,6 +1287,39 @@ fn lifecycle_notice(notice: &LifecycleNotice) -> Option { }) } +/// The line for containers a forgotten workspace left behind, removed. Shared by +/// `rm` and the launch, which find the same leftovers at two different times. +fn leftovers_removed(workspace_id: &str, names: &NonEmpty) -> String { + format!( + "Removed containers {workspace_id} left behind: {}", + joined(names, ", ") + ) +} + +/// The line for a docker that would not list containers, so leftovers were not +/// looked for. +fn leftovers_not_listed(workspace_id: &str, reason: &str) -> String { + format!("Could not check for containers {workspace_id} left behind: {reason}") +} + +/// The line for leftovers docker would not remove, with the command that does +/// it by hand. +fn leftovers_not_removed(workspace_id: &str, names: &NonEmpty, reason: &str) -> String { + format!( + "Containers {workspace_id} left behind are still here ({reason}). Remove them with: \ + docker rm -f {}", + joined(names, " ") + ) +} + +fn joined(names: &NonEmpty, separator: &str) -> String { + names + .iter() + .map(String::as_str) + .collect::>() + .join(separator) +} + /// One storage-flow notice, in the words the module that logged it used. /// /// `worktree/repo_manager.py` and `worktree/workspace_clone.py` write these @@ -3259,6 +3305,19 @@ pub(crate) fn launch_notice(notice: &LaunchNotice) -> Option { source and the container cannot be created. Look for `${localEnv:` in its \ .devcontainer/devcontainer.json and set the variable it names." .to_owned(), + LaunchNotice::LeftoverContainersRemoved { + workspace_id, + names, + } => leftovers_removed(workspace_id, names), + LaunchNotice::LeftoverContainersNotListed { + workspace_id, + reason, + } => leftovers_not_listed(workspace_id, reason), + LaunchNotice::LeftoverContainersNotRemoved { + workspace_id, + names, + reason, + } => leftovers_not_removed(workspace_id, names, reason), // --- the dotfiles (info; devlaunch#560, no Python line) // @@ -6108,6 +6167,38 @@ mod tests { assert!(empty.contains("${localEnv:"), "{empty}"); } + /// Leftover containers are named, and a removal docker refused hands over + /// the command that does it by hand. + #[test] + fn leftover_containers_are_named_and_a_refusal_gives_the_command() { + let removed = launch_notice(&LaunchNotice::LeftoverContainersRemoved { + workspace_id: "ws".to_owned(), + names: NonEmpty::of(["dev_ws".to_owned(), "side_ws".to_owned()]).expect("two names"), + }); + assert_eq!( + removed.as_deref(), + Some("Removed containers ws left behind: dev_ws, side_ws") + ); + + let refused = lifecycle_notice(&LifecycleNotice::LeftoverContainersNotRemoved { + workspace_id: "ws".to_owned(), + names: NonEmpty::of(["dev_ws".to_owned()]).expect("a name"), + reason: "docker rm failed".to_owned(), + }) + .expect("a line"); + assert!(refused.ends_with("docker rm -f dev_ws"), "{refused}"); + + let unlisted = lifecycle_notice(&LifecycleNotice::LeftoverContainersNotListed { + workspace_id: "ws".to_owned(), + reason: "docker ps did not answer in time".to_owned(), + }) + .expect("a line"); + assert_eq!( + unlisted, + "Could not check for containers ws left behind: docker ps did not answer in time" + ); + } + #[test] fn a_shared_pixi_cache_that_could_not_be_made_is_a_warning_not_a_failure() { // The launch survives either way, which is exactly why these are warnings: diff --git a/rust/dl/tests/launch.rs b/rust/dl/tests/launch.rs index 72688919..6af84e64 100644 --- a/rust/dl/tests/launch.rs +++ b/rust/dl/tests/launch.rs @@ -865,6 +865,79 @@ fn a_cold_triple_prepares_a_clone_creates_the_workspace_and_attaches() { ); } +/// Every docker call the launch made, each with the number of devpod calls +/// logged before it. +fn docker_calls(world: &World) -> Vec<(usize, String)> { + std::fs::read_to_string(world.path("docker-log")) + .unwrap_or_default() + .lines() + .map(|line| { + let (before, argv) = line.split_once(' ').expect("a count and an argv"); + (before.parse().expect("a count"), argv.to_owned()) + }) + .collect() +} + +/// A cold launch of a workspace devpod holds no record of, over containers an +/// older one left under the same id: they go before the `devpod up`, which +/// would otherwise trip over their fixed names, and the launch says so. +#[test] +fn a_cold_launch_removes_the_leftovers_of_a_forgotten_workspace_before_the_up() { + let world = World::with(&[]); + let root = world.root.display().to_string(); + let clone = format!("{root}/cache/devlaunch/repos/blooop/devlaunch/{COLD}"); + let agent = format!( + "{root}/devpod/agent/contexts/default/workspaces/{COLD}/.docker-compose/\ + docker-compose.devcontainer.containerFeatures-0.yml" + ); + std::fs::write( + world.path("docker-ps"), + format!( + "aaa\tdev_old\t{clone}/.devcontainer\t{clone}/.devcontainer/a.yml,{agent}\n\ + ccc\tsomeone_else\t/srv/other\t/srv/other/compose.yml\n" + ), + ) + .expect("a docker listing"); + + let run = world.dl(&["blooop/devlaunch@cold"]); + + run.exited(0); + let calls = docker_calls(&world); + let removal = calls + .iter() + .find(|(_, argv)| argv.starts_with("rm ")) + .unwrap_or_else(|| panic!("no removal in {calls:?}")); + assert_eq!(removal.1, "rm --force aaa"); + let up = world + .raw_calls() + .iter() + .position(|argv| argv.first().is_some_and(|word| word == "up")) + .expect("a devpod up"); + assert!(removal.0 <= up, "the removal ran after the up: {calls:?}"); + assert!( + run.err + .contains(&format!("Removed containers {COLD} left behind: dev_old\n")), + "{}", + run.err + ); +} + +/// A workspace devpod holds a record of is devpod's to manage: the launch asks +/// docker nothing, so an ordinary `up` costs no docker call at all. +#[test] +fn a_launch_of_a_workspace_devpod_holds_asks_docker_nothing() { + let world = World::with(&[]); + let record = world.path(&format!( + "devpod/contexts/default/workspaces/{COLD}/workspace.json" + )); + std::fs::create_dir_all(record.parent().expect("a record directory")).expect("a record dir"); + std::fs::write(&record, "{}").expect("devpod's record"); + + world.dl(&["blooop/devlaunch@cold"]).exited(0); + + assert_eq!(docker_calls(&world), []); +} + #[test] fn a_bare_owner_repo_names_the_default_branch_before_it_derives_an_id() { // No `@branch`, so the default branch has to be named first — from the record diff --git a/rust/dl/tests/launch_scenario.py b/rust/dl/tests/launch_scenario.py index 9ce9c1d0..1cf7ad1b 100755 --- a/rust/dl/tests/launch_scenario.py +++ b/rust/dl/tests/launch_scenario.py @@ -153,6 +153,20 @@ def build(root: pathlib.Path, shim: pathlib.Path, wanted: set) -> None: f'#!/bin/sh\nexec "{sys.executable}" "{shim}" "$@"\n', ) + # A fake docker in every world: `bin` comes first on the test's PATH, so + # without it a launch's leftover-container sweep would ask whatever docker + # the machine running the suite has. Each call is logged to `docker-log` as + # the number of devpod calls logged before it, then its argv, so a test can + # tell which side of `devpod up` it ran on. A `ps` prints `docker-ps` when a + # test has written one; otherwise nothing. + _executable( + root / "bin" / "docker", + "#!/bin/sh\n" + f'n=$(cat "{root / "shim-log.jsonl"}" 2>/dev/null | wc -l | tr -d " ")\n' + f'echo "$n $*" >> "{root / "docker-log"}"\n' + f'if [ "$1" = ps ] && [ -f "{root / "docker-ps"}" ]; then cat "{root / "docker-ps"}"; fi\n', + ) + if "gh" in wanted: # Only `auth token` answers; anything else exits non-zero, as a gh that # does not know a subcommand does. diff --git a/rust/dl/tests/lifecycle.rs b/rust/dl/tests/lifecycle.rs index 1afbb509..95b61484 100644 --- a/rust/dl/tests/lifecycle.rs +++ b/rust/dl/tests/lifecycle.rs @@ -1317,17 +1317,183 @@ fn a_workspace_with_no_clone_is_still_named_going_in_and_coming_out() { } /// A workspace devpod never finished creating has no create result, so there is -/// nothing to name and docker is not run at all — not run with a guessed name in -/// it, which would be somebody else's disk. +/// nothing to name and docker removes nothing — not a volume with a guessed name +/// in it, which would be somebody else's disk. The one docker call is the +/// leftover-container listing, a read, which finds nothing here. #[test] -fn a_delete_with_nothing_recorded_to_name_runs_no_docker() { +fn a_delete_with_nothing_recorded_to_name_removes_nothing() { let world = World::base(); world.dl(&["devlaunch-main-legacy", "rm"]).exited(0); + assert_eq!(world.docker_calls(), [LEFTOVER_LISTING]); +} + +/// The leftover-container listing, as the fake docker logs its argv. +const LEFTOVER_LISTING: &str = "ps --all --no-trunc --filter label=com.docker.compose.project \ + --format {{.ID}}\t{{.Names}}\t{{.Label \"com.docker.compose.project.working_dir\"}}\t\ + {{.Label \"com.docker.compose.project.config_files\"}}"; + +/// devpod's delete finds a workspace's containers by the uid of the record it is +/// deleting, so containers an older record left (or a forced delete gave up on) +/// outlive it, and their fixed `container_name`s then block every later `up` of +/// the workspace. `rm` removes them once devpod has let go: every container whose +/// compose labels name the clone or devpod's directory for this id, and nothing +/// else on the machine. +#[test] +fn rm_removes_the_containers_devpod_left_behind() { + let world = World::base(); + leave_containers_behind(&world); + + let run = world.dl(&["devlaunch-main-legacy", "rm"]); + + run.exited(0); + assert_eq!( + world.docker_calls(), + [LEFTOVER_LISTING, "rm --force aaa bbb"] + ); + assert!( + run.err.contains( + "Removed containers devlaunch-main-legacy left behind: \ + dev_devlaunch-main-legacy, sidecar_devlaunch-main-legacy\n" + ), + "{}", + run.err + ); +} + +/// The ordinary shape: devpod held a record for the workspace until its own +/// delete dropped it, so the sweep runs after the delete and not instead of it. +/// The volume removal after it shows the record was there to name them from. +#[test] +fn rm_removes_the_leftovers_once_devpods_delete_drops_its_record() { + let world = World::with(&["--devcontainer-volumes", "--record-dropping-delete"]); + leave_containers_behind(&world); + assert!( + world.exists("devpod/contexts/default/workspaces/devlaunch-main-legacy/workspace.json") + ); + + let run = world.dl(&["devlaunch-main-legacy", "rm"]); + + run.exited(0); + assert!(!world.exists("devpod/contexts/default/workspaces/devlaunch-main-legacy")); + assert_eq!( + world.docker_calls(), + [ + LEFTOVER_LISTING, + "rm --force aaa bbb", + "volume rm --force devlaunch-main-legacy-pixi dind-var-lib-docker-0f4b2c1d", + ] + ); + assert!( + run.err.contains( + "Removed containers devlaunch-main-legacy left behind: \ + dev_devlaunch-main-legacy, sidecar_devlaunch-main-legacy\n" + ), + "{}", + run.err + ); +} + +/// `kill` ends in the same delete, so it sweeps the same leftovers. The calls +/// before the sweep are `kill`'s own hunt for what holds the workspace. +#[test] +fn kill_removes_the_leftovers_once_devpods_delete_drops_its_record() { + let world = World::with(&["--devcontainer-volumes", "--record-dropping-delete"]); + leave_containers_behind(&world); + + let run = world.dl(&["devlaunch-main-legacy", "kill"]); + + run.exited(0); + let calls = world.docker_calls(); + assert!( + calls.ends_with(&[ + LEFTOVER_LISTING.to_owned(), + "rm --force aaa bbb".to_owned(), + "volume rm --force devlaunch-main-legacy-pixi dind-var-lib-docker-0f4b2c1d".to_owned(), + ]), + "{calls:?}" + ); + assert!( + run.err.contains( + "Removed containers devlaunch-main-legacy left behind: \ + dev_devlaunch-main-legacy, sidecar_devlaunch-main-legacy\n" + ), + "{}", + run.err + ); +} + +/// A delete devpod refuses leaves devpod's record standing, so the workspace is +/// still devpod's and so are its containers: the sweep asks docker nothing and +/// removes nothing, though leftovers that name the workspace are on the machine. +#[test] +fn a_refused_delete_that_keeps_the_record_leaves_the_containers() { + let world = World::with(&["--devcontainer-volumes"]); + leave_containers_behind(&world); + world.devpod_answers(&["delete"], 3, "devpod: cannot read devcontainer.json\n"); + + world.dl(&["devlaunch-main-legacy", "rm"]).exited(3); + + assert!( + world.exists("devpod/contexts/default/workspaces/devlaunch-main-legacy/workspace.json") + ); assert_eq!(world.docker_calls(), Vec::::new()); } +/// A local folder opened as its own workspace can sit inside another +/// workspace's clone. devpod still holds the inner one, `sub`, and its live +/// container names `sub`'s devpod directory; removing the outer workspace must +/// leave that container, though its compose files are inside the outer clone. +#[test] +fn rm_leaves_the_containers_of_a_held_workspace_inside_the_clone() { + let world = World::base(); + let root = world.root.display().to_string(); + let sub = format!("{root}/cache/devlaunch/repos/blooop/devlaunch/devlaunch-main-legacy/sub"); + let record = world.path("devpod/contexts/default/workspaces/sub/workspace.json"); + std::fs::create_dir_all(record.parent().expect("a record directory")).expect("a record dir"); + std::fs::write( + &record, + format!(r#"{{"id":"sub","source":{{"localFolder":"{sub}"}}}}"#), + ) + .expect("devpod's record of sub"); + let agent = format!( + "{root}/devpod/agent/contexts/default/workspaces/sub/.docker-compose/\ + docker-compose.devcontainer.containerFeatures-0.yml" + ); + std::fs::write( + world.path("docker-ps"), + format!("sss\tsub_dev\t{sub}/.devcontainer\t{sub}/.devcontainer/compose.yml,{agent}\n"), + ) + .expect("a docker listing"); + + let run = world.dl(&["devlaunch-main-legacy", "rm"]); + + run.exited(0); + assert_eq!(world.docker_calls(), [LEFTOVER_LISTING]); + assert!(!run.err.contains("sub_dev"), "{}", run.err); +} + +/// Put three compose containers on the world's docker: two left by +/// `devlaunch-main-legacy` (`aaa`, `bbb`), and one of somebody else's. +fn leave_containers_behind(world: &World) { + let root = world.root.display().to_string(); + let clone = format!("{root}/cache/devlaunch/repos/blooop/devlaunch/devlaunch-main-legacy"); + let agent = format!( + "{root}/devpod/agent/contexts/default/workspaces/devlaunch-main-legacy/.docker-compose/\ + docker-compose.devcontainer.containerFeatures-0.yml" + ); + std::fs::write( + world.path("docker-ps"), + format!( + "aaa\tdev_devlaunch-main-legacy\t{clone}/.devcontainer\t{clone}/.devcontainer/a.yml,{agent}\n\ + bbb\tsidecar_devlaunch-main-legacy\t{clone}/.devcontainer\t{clone}/.devcontainer/b.yml\n\ + ccc\tsomeone_else\t/srv/other\t/srv/other/compose.yml\n" + ), + ) + .expect("a docker listing"); +} + #[test] fn a_clone_holding_work_that_is_nowhere_else_is_refused() { let world = World::base(); diff --git a/rust/dl/tests/lifecycle_scenario.py b/rust/dl/tests/lifecycle_scenario.py index 094ea804..91c1b572 100755 --- a/rust/dl/tests/lifecycle_scenario.py +++ b/rust/dl/tests/lifecycle_scenario.py @@ -197,17 +197,33 @@ def build(root: pathlib.Path, shim: pathlib.Path, wanted: set) -> None: path.mkdir(parents=True, exist_ok=True) devpod = root / "bin" / "devpod" - devpod.write_text(f'#!/bin/sh\nexec "{sys.executable}" "{shim}" "$@"\n', encoding="utf-8") + if "record-dropping-delete" in wanted: + # A delete that also drops devpod's own record of the workspace, the way + # real devpod's does, so that what a test sees afterwards is a devpod + # holding no record for the id. The shim keeps its state in its own file + # and never touches `contexts/`. + devpod.write_text( + f'#!/bin/sh\n"{sys.executable}" "{shim}" "$@"\nstatus=$?\n' + 'if [ "$1" = delete ] && [ "$status" = 0 ]; then\n' + ' rm -rf "$DEVPOD_HOME/contexts/default/workspaces/$2"\n' + "fi\n" + 'exit "$status"\n', + encoding="utf-8", + ) + else: + devpod.write_text(f'#!/bin/sh\nexec "{sys.executable}" "{shim}" "$@"\n', encoding="utf-8") devpod.chmod(0o755) # A fake docker, in *every* world and not only the one that asserts about it: # `bin` comes first on the test's PATH, so without this a delete would reach # whatever docker the machine running the suite has and remove volumes on it. # It logs one line per call and exits 0, which is what a removal that worked - # looks like. + # looks like. A `ps` prints `docker-ps` when a test has written one, which is + # how a test puts leftover containers on the machine; otherwise nothing. docker = root / "bin" / "docker" docker.write_text( - f'#!/bin/sh\necho "$@" >> "{root / "docker-log"}"\n', + f'#!/bin/sh\necho "$@" >> "{root / "docker-log"}"\n' + f'if [ "$1" = ps ] && [ -f "{root / "docker-ps"}" ]; then cat "{root / "docker-ps"}"; fi\n', encoding="utf-8", ) docker.chmod(0o755) @@ -578,7 +594,7 @@ def build(root: pathlib.Path, shim: pathlib.Path, wanted: set) -> None: "[--stranded-clone] " "[--tagged-release] [--fork] " "[--sealed-cache] [--symlinked-cache] [--v1-cache] " - "[--devcontainer-volumes]" + "[--devcontainer-volumes] [--record-dropping-delete]" ) flags = {argument.lstrip("-") for argument in sys.argv[3:]} unknown = flags - { @@ -599,6 +615,7 @@ def build(root: pathlib.Path, shim: pathlib.Path, wanted: set) -> None: "symlinked-cache", "v1-cache", "devcontainer-volumes", + "record-dropping-delete", } if unknown: raise SystemExit(f"lifecycle_scenario.py: unknown fixture(s): {sorted(unknown)}") diff --git a/scripts/public-api-snapshots.sh b/scripts/public-api-snapshots.sh index a15846d7..764f64b2 100755 --- a/scripts/public-api-snapshots.sh +++ b/scripts/public-api-snapshots.sh @@ -37,7 +37,7 @@ # The limit that is left, and it is not one type. A type `api` never re-exports # but a promised signature names is reachable from outside and is classified as # binary surface, so a break in it diffs public-api.rest.txt alone. Measured on -# the checked-in files rather than guessed at: 41 such types own close to six +# the checked-in files rather than guessed at: 42 such types own close to six # hundred rows over there. `--print-residual` lists them, needs no toolchain, # and prints the exact row count; the type count above is the figure # `test/test_public_api_snapshots_doc.py` diffs against it, because that is the @@ -55,7 +55,7 @@ # # What that means for reading a rest-file diff: it is routine for a row whose # subject is nothing a promised signature names, and a contract change for a row -# whose subject is one of the 41. `--print-residual` is how you tell. +# whose subject is one of the 42. `--print-residual` is how you tell. # # Whether the tool does this already, since the obvious first question is why # any of it is hand-rolled (#352 asked it explicitly). It does not, in the pin