From 125ec09ff9ef184c4530d1a171031db7bb3d5386 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 16 Oct 2026 15:51:10 +0100 Subject: [PATCH 01/20] fix: remove the containers a forgotten workspace left running devpod delete finds a workspace's container only by dev.containers.id=, and runs compose down only when it finds it. Two ordinary events leave a compose project up with nothing that owns it: - devpod delete --force (what `dl kill` runs) logs a failed compose down and drops 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 devpod up over the leftovers makes a new record with a new uid and fails on their fixed container_name. The next devpod delete looks for the new uid, finds no container, and reports success. The leftovers then fail every later up of the workspace with "Conflict. The container name ... is already in use". Once devpod holds no record of the workspace in any context, `rm`, `kill` and the launch (before its devpod up) now list compose containers and remove the ones whose working_dir or config_files label names devpod's agent directory for that id, or the clone dl made. Each removal is named on stderr. A workspace devpod still holds is left alone; a local folder is matched by devpod's directory only, so a compose project the user started there by hand stays. --- CHANGELOG.md | 11 + docs/cleanup.md | 33 ++ rust/devlaunch-core/public-api.api.txt | 28 ++ .../devlaunch-core/src/clients/devpod_home.rs | 61 ++++ rust/devlaunch-core/src/clients/docker.rs | 139 +++++++- rust/devlaunch-core/src/flows/launch.rs | 140 ++++++++ .../src/flows/lifecycle/delete.rs | 68 +++- .../src/flows/lifecycle/notices.rs | 15 + rust/devlaunch-core/src/flows/mod.rs | 2 + .../src/flows/orphan_containers.rs | 318 ++++++++++++++++++ rust/dl/src/render.rs | 74 ++++ rust/dl/tests/lifecycle.rs | 56 ++- rust/dl/tests/lifecycle_scenario.py | 6 +- 13 files changed, 938 insertions(+), 13 deletions(-) create mode 100644 rust/devlaunch-core/src/flows/orphan_containers.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 03e2b7d4..68e30096 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ 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, and a local folder's own compose projects, are left alone. + ## [0.60.1] - 2026-10-16 ### Changed diff --git a/docs/cleanup.md b/docs/cleanup.md index 8c394ddc..a3130c77 100644 --- a/docs/cleanup.md +++ b/docs/cleanup.md @@ -97,6 +97,39 @@ 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, 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. + ### 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/rust/devlaunch-core/public-api.api.txt b/rust/devlaunch-core/public-api.api.txt index 988b7e30..601de06b 100644 --- a/rust/devlaunch-core/public-api.api.txt +++ b/rust/devlaunch-core/public-api.api.txt @@ -67,6 +67,13 @@ 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::LeftoverContainersNotRemoved +pub devlaunch_core::api::LaunchNotice::LeftoverContainersNotRemoved::names: alloc::vec::Vec +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: alloc::vec::Vec +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 @@ -150,6 +157,13 @@ 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::LeftoverContainersNotRemoved +pub devlaunch_core::api::LifecycleNotice::LeftoverContainersNotRemoved::names: alloc::vec::Vec +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: alloc::vec::Vec +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 @@ -624,6 +638,13 @@ 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::LeftoverContainersNotRemoved +pub devlaunch_core::flows::launch::LaunchNotice::LeftoverContainersNotRemoved::names: alloc::vec::Vec +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: alloc::vec::Vec +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 @@ -785,6 +806,13 @@ 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::LeftoverContainersNotRemoved +pub devlaunch_core::flows::lifecycle::LifecycleNotice::LeftoverContainersNotRemoved::names: alloc::vec::Vec +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: alloc::vec::Vec +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..38a8fde2 100644 --- a/rust/devlaunch-core/src/clients/devpod_home.rs +++ b/rust/devlaunch-core/src/clients/devpod_home.rs @@ -134,6 +134,54 @@ 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 a yes. A + /// `contexts` directory that would not read is also a yes, so that an + /// unreadable home counts as "a record may be there". + pub(crate) fn holds_any_record(&self, workspace_id: &str) -> bool { + let Ok(contexts) = std::fs::read_dir(self.contexts()) else { + return self.contexts().exists(); + }; + contexts.flatten().any(|context| { + context + .file_name() + .to_str() + .is_some_and(|name| self.record(name, workspace_id).is_file()) + }) + } + + /// Whether `path` is inside devpod's agent-side directory for this workspace, + /// under any context. + /// + /// 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 is_in_agent_dir_of(&self, path: &Path, workspace_id: &str) -> bool { + let Ok(rest) = path.strip_prefix(self.agent_contexts()) else { + return false; + }; + let mut components = rest.components().skip(1); + components + .next() + .is_some_and(|c| c.as_os_str() == "workspaces") + && components + .next() + .is_some_and(|c| c.as_os_str() == workspace_id) + } + /// devpod's record of a *completed* create for one workspace. /// /// devpod writes this beside [`Self::record`] on its way out of a successful @@ -428,6 +476,19 @@ pub(crate) fn devpod_home_with(entries: &[(&str, &str, Option<()>)]) -> ScratchH ScratchHome { _dir: dir, home } } +/// 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 5b3d48c6..5fef7640 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; @@ -162,6 +164,131 @@ 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 spec = SpawnSpec::from(Invocation::new(PROGRAM).with_args([ + "ps", + "--all", + "--no-trunc", + "--filter", + "label=com.docker.compose.project", + "--format", + COMPOSE_FORMAT, + ])) + .with_timeout(LIST_COMPOSE_CONTAINERS); + match ran(runner.capture(&spec)) { + Ok((exit, CapturedText { stdout, .. })) if exit.is_success() => { + Ok(stdout.lines().filter_map(compose_container).collect()) + } + 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/flows/launch.rs b/rust/devlaunch-core/src/flows/launch.rs index e2251e7e..cc35c5f4 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -88,6 +88,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, @@ -509,6 +510,21 @@ 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: Vec, + }, + /// Leftover containers docker would not list or remove, in its own words. + /// The `up` goes ahead, and will name the container it trips over. + LeftoverContainersNotRemoved { + workspace_id: String, + names: Vec, + reason: String, + }, // --- the dotfiles this `up` asked devpod for (devlaunch#560) // @@ -1194,6 +1210,122 @@ 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 a directory under +/// devlaunch's clone root: a local folder is the user's, and a compose project +/// they started there by hand is not a leftover. +fn sweep_leftovers( + runner: &dyn Runner, + host: &Host, + workspace_id: &str, + source: &str, + notices: &mut dyn Notices, +) { + let source = Path::new(source); + let clone = (source.is_absolute() + && source.starts_with(crate::domain::xdg::clone_root_in(&host.cache_dir))) + .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::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: vec!["dev_ws".to_owned(), "side_ws".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()); + } +} + /// One `devpod up`, as data. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub(crate) struct UpRequest<'a> { @@ -1803,6 +1935,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..eaec4f43 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,49 @@ 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 under devlaunch's own clone root: a recorded path anywhere else +/// is somebody's directory, and a compose project they started there by hand 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)?; + path.starts_with(clones.repos_dir()).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::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..e3575f6b 100644 --- a/rust/devlaunch-core/src/flows/lifecycle/notices.rs +++ b/rust/devlaunch-core/src/flows/lifecycle/notices.rs @@ -44,6 +44,21 @@ 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: Vec, + }, + /// Containers this workspace left behind are still on this machine, and + /// docker said why. `names` is empty when docker would not list them at all. + LeftoverContainersNotRemoved { + workspace_id: String, + names: Vec, + 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/mod.rs b/rust/devlaunch-core/src/flows/mod.rs index 6f49cd58..669d9887 100644 --- a/rust/devlaunch-core/src/flows/mod.rs +++ b/rust/devlaunch-core/src/flows/mod.rs @@ -26,6 +26,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..6f803075 --- /dev/null +++ b/rust/devlaunch-core/src/flows/orphan_containers.rs @@ -0,0 +1,318 @@ +//! 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. 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. +//! +//! 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; +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 two 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(Vec), + /// docker would not list or remove them. `names` is what was found, and is + /// empty when the listing itself failed; `reason` is docker's own words. + NotRemoved { names: Vec, 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.holds_any_record(workspace_id) { + 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::NotRemoved { + names: Vec::new(), + reason, + }; + } + }; + let left: Vec<&ComposeContainer> = containers + .iter() + .filter(|container| left_by(container, home, workspace_id, clone)) + .collect(); + let names: Vec = left.iter().map(|c| c.name.clone()).collect(); + let Some(ids) = NonEmpty::of(left.iter().map(|c| c.id.clone())) else { + return OrphanSweep::Nothing; + }; + match docker::remove_containers(runner, &ids) { + docker::Answer::Ran { exit, .. } if exit.is_success() => OrphanSweep::Removed(names), + docker::Answer::Ran { exit, stderr } => OrphanSweep::NotRemoved { + names, + reason: format!("docker rm failed ({exit:?}): {}", stderr.trim()), + }, + docker::Answer::NotInstalled => OrphanSweep::NoDocker, + docker::Answer::NotStarted(failure) => OrphanSweep::NotRemoved { + names, + reason: format!("docker rm did not finish ({:?})", failure.kind), + }, + } +} + +/// Whether one container's compose labels name this workspace. +fn left_by( + container: &ComposeContainer, + home: &DevpodHome, + workspace_id: &str, + clone: Option<&Path>, +) -> bool { + container.paths().any(|path| { + home.is_in_agent_dir_of(path, workspace_id) + || clone.is_some_and(|clone| path.starts_with(clone)) + }) +} + +#[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}; + + const WS: &str = "kinisi-ros-ci-sim-shard-rebalance-cd1e"; + + /// 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(vec![ + 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 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(vec!["project_dev".to_owned()]) + ); + } + + #[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); + } + + #[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: vec!["dev".to_owned()], + reason: "docker rm failed (Code(1)): daemon said no".to_owned(), + } + ); + } +} diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index 1c1030b9..110f89fd 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -1136,6 +1136,15 @@ 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::LeftoverContainersNotRemoved { + workspace_id, + names, + reason, + } => leftovers_not_removed(workspace_id, names, reason), LifecycleNotice::RecordNotDropped { path, refusal } => { format!( "Could not drop the record for {}: {}", @@ -1162,6 +1171,29 @@ 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: &[String]) -> String { + format!( + "Removed containers {workspace_id} left behind: {}", + names.join(", ") + ) +} + +/// The line for leftovers docker would not remove, with the command that does +/// it by hand when there are names to give it. +fn leftovers_not_removed(workspace_id: &str, names: &[String], reason: &str) -> String { + if names.is_empty() { + format!("Could not check for containers {workspace_id} left behind: {reason}") + } else { + format!( + "Containers {workspace_id} left behind are still here ({reason}). Remove them with: \ + docker rm -f {}", + names.join(" ") + ) + } +} + /// One storage-flow notice, in the words the module that logged it used. /// /// `worktree/repo_manager.py` and `worktree/workspace_clone.py` write these @@ -3100,6 +3132,15 @@ 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::LeftoverContainersNotRemoved { + workspace_id, + names, + reason, + } => leftovers_not_removed(workspace_id, names, reason), // --- the dotfiles (info; devlaunch#560, no Python line) // @@ -5689,6 +5730,39 @@ 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: vec!["dev_ws".to_owned(), "side_ws".to_owned()], + }); + 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: vec!["dev_ws".to_owned()], + reason: "docker rm failed".to_owned(), + }) + .expect("a line"); + assert!(refused.ends_with("docker rm -f dev_ws"), "{refused}"); + + let unlisted = lifecycle_notice(&LifecycleNotice::LeftoverContainersNotRemoved { + workspace_id: "ws".to_owned(), + names: vec![], + 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/lifecycle.rs b/rust/dl/tests/lifecycle.rs index b71ace98..0803910d 100644 --- a/rust/dl/tests/lifecycle.rs +++ b/rust/dl/tests/lifecycle.rs @@ -1012,15 +1012,63 @@ 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(), Vec::::new()); + 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(); + 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"); + + 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 + ); } #[test] diff --git a/rust/dl/tests/lifecycle_scenario.py b/rust/dl/tests/lifecycle_scenario.py index 59e76c74..78b37d1a 100755 --- a/rust/dl/tests/lifecycle_scenario.py +++ b/rust/dl/tests/lifecycle_scenario.py @@ -199,10 +199,12 @@ def build(root: pathlib.Path, shim: pathlib.Path, wanted: set) -> None: # `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) From b60f4506d578edb38d24a5b547ec2ad96c5937ca Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 16 Oct 2026 15:57:12 +0100 Subject: [PATCH 02/20] fix: a launch at a clone's parent dir removed other workspaces' containers sweep_leftovers took any source under the clone root as the workspace's clone, so `dl /repos/org/repo` matched every live clone of org/repo and force-removed their containers. Only the exact clone dir for this workspace id now counts. --- rust/devlaunch-core/src/flows/launch.rs | 56 ++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 6 deletions(-) diff --git a/rust/devlaunch-core/src/flows/launch.rs b/rust/devlaunch-core/src/flows/launch.rs index cc35c5f4..a5e1d213 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -1213,9 +1213,11 @@ 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 a directory under -/// devlaunch's clone root: a local folder is the user's, and a compose project -/// they started there by hand is not a leftover. +/// `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, @@ -1224,9 +1226,21 @@ fn sweep_leftovers( notices: &mut dyn Notices, ) { let source = Path::new(source); - let clone = (source.is_absolute() - && source.starts_with(crate::domain::xdg::clone_root_in(&host.cache_dir))) - .then_some(source); + let clone_root = crate::domain::xdg::clone_root_in(&host.cache_dir); + let clone = source + .strip_prefix(&clone_root) + .is_ok_and(|under| { + let parts: Vec<_> = under.components().collect(); + matches!( + parts.as_slice(), + [ + std::path::Component::Normal(_), + std::path::Component::Normal(_), + std::path::Component::Normal(leaf), + ] if *leaf == std::ffi::OsStr::new(workspace_id) + ) + }) + .then_some(source); match crate::flows::orphan_containers::sweep( runner, host.devpod_home.as_ref(), @@ -1324,6 +1338,36 @@ mod leftover_tests { 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. From f7e687599ed0a51de11a2293d1e852c4ccfb6611 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 16 Oct 2026 15:59:52 +0100 Subject: [PATCH 03/20] fix: a failed docker rm with no names rendered as a failed listing OrphanSweep::NotRemoved used empty names to mean "docker ps failed", and the renderer branched on that, so a NotRemoved with no names printed the listing-failure line. A failed listing is now its own arm, NotListed, and the removed and not-removed names are NonEmpty, so neither sentinel can be built. The two notice enums follow. --- rust/devlaunch-core/public-api.api.txt | 28 +++++++--- .../src/domain/workspace_state.rs | 7 +++ rust/devlaunch-core/src/flows/launch.rs | 23 +++++++-- .../src/flows/lifecycle/delete.rs | 6 +++ .../src/flows/lifecycle/notices.rs | 13 +++-- .../src/flows/orphan_containers.rs | 50 +++++++++--------- rust/dl/src/render.rs | 51 ++++++++++++------- 7 files changed, 122 insertions(+), 56 deletions(-) diff --git a/rust/devlaunch-core/public-api.api.txt b/rust/devlaunch-core/public-api.api.txt index 601de06b..11e66050 100644 --- a/rust/devlaunch-core/public-api.api.txt +++ b/rust/devlaunch-core/public-api.api.txt @@ -67,12 +67,15 @@ 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: alloc::vec::Vec +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: alloc::vec::Vec +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 @@ -157,12 +160,15 @@ 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: alloc::vec::Vec +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: alloc::vec::Vec +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 @@ -638,12 +644,15 @@ 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: alloc::vec::Vec +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: alloc::vec::Vec +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 @@ -806,12 +815,15 @@ 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: alloc::vec::Vec +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: alloc::vec::Vec +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 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/flows/launch.rs b/rust/devlaunch-core/src/flows/launch.rs index a5e1d213..97110e6b 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -516,13 +516,19 @@ pub enum LaunchNotice { /// `flows::orphan_containers`. LeftoverContainersRemoved { workspace_id: String, - names: Vec, + names: NonEmpty, }, - /// Leftover containers docker would not list or remove, in its own words. - /// The `up` goes ahead, and will name the container it trips over. + /// 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: Vec, + names: NonEmpty, reason: String, }, @@ -1251,6 +1257,12 @@ fn sweep_leftovers( 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(), @@ -1310,7 +1322,8 @@ mod leftover_tests { notices, [LaunchNotice::LeftoverContainersRemoved { workspace_id: "ws".to_owned(), - names: vec!["dev_ws".to_owned(), "side_ws".to_owned()], + names: NonEmpty::of(["dev_ws".to_owned(), "side_ws".to_owned()]) + .expect("two names"), }] ); } diff --git a/rust/devlaunch-core/src/flows/lifecycle/delete.rs b/rust/devlaunch-core/src/flows/lifecycle/delete.rs index eaec4f43..5f7a341b 100644 --- a/rust/devlaunch-core/src/flows/lifecycle/delete.rs +++ b/rust/devlaunch-core/src/flows/lifecycle/delete.rs @@ -431,6 +431,12 @@ fn sweep_leftovers( 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(), diff --git a/rust/devlaunch-core/src/flows/lifecycle/notices.rs b/rust/devlaunch-core/src/flows/lifecycle/notices.rs index e3575f6b..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}; @@ -50,13 +51,19 @@ pub enum LifecycleNotice { /// outlives it; see `flows::orphan_containers`. LeftoverContainersRemoved { workspace_id: String, - names: Vec, + 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. `names` is empty when docker would not list them at all. + /// docker said why. LeftoverContainersNotRemoved { workspace_id: String, - names: Vec, + names: NonEmpty, reason: String, }, /// A clone directory went and its `metadata.json` record could not be diff --git a/rust/devlaunch-core/src/flows/orphan_containers.rs b/rust/devlaunch-core/src/flows/orphan_containers.rs index 6f803075..c24a0f9f 100644 --- a/rust/devlaunch-core/src/flows/orphan_containers.rs +++ b/rust/devlaunch-core/src/flows/orphan_containers.rs @@ -44,9 +44,9 @@ use crate::domain::workspace_state::NonEmpty; /// What a sweep for one workspace's leftover containers came to. /// -/// Two silent arms and two 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. +/// 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 @@ -55,10 +55,14 @@ pub(crate) enum OrphanSweep { /// No docker on this machine, so nothing was made to be left. NoDocker, /// These containers were removed, by name. - Removed(Vec), - /// docker would not list or remove them. `names` is what was found, and is - /// empty when the listing itself failed; `reason` is docker's own words. - NotRemoved { names: Vec, reason: String }, + 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. @@ -84,21 +88,17 @@ pub(crate) fn sweep( let containers = match docker::compose_containers(runner) { Ok(containers) => containers, Err(Unlisted::NotInstalled) => return OrphanSweep::NoDocker, - Err(Unlisted::Failed(reason)) => { - return OrphanSweep::NotRemoved { - names: Vec::new(), - reason, - }; - } + Err(Unlisted::Failed(reason)) => return OrphanSweep::NotListed { reason }, }; - let left: Vec<&ComposeContainer> = containers - .iter() - .filter(|container| left_by(container, home, workspace_id, clone)) - .collect(); - let names: Vec = left.iter().map(|c| c.name.clone()).collect(); - let Some(ids) = NonEmpty::of(left.iter().map(|c| c.id.clone())) else { + let Some(left) = NonEmpty::of( + containers + .iter() + .filter(|container| left_by(container, home, workspace_id, clone)), + ) 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 } => OrphanSweep::NotRemoved { @@ -137,6 +137,10 @@ mod tests { 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 @@ -191,10 +195,10 @@ mod tests { assert_eq!( swept, - OrphanSweep::Removed(vec![ + 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"), @@ -275,7 +279,7 @@ mod tests { assert_eq!( sweep(&fake, Some(&home), "project", None), - OrphanSweep::Removed(vec!["project_dev".to_owned()]) + OrphanSweep::Removed(names(["project_dev".to_owned()])) ); } @@ -310,7 +314,7 @@ mod tests { assert_eq!( sweep(&fake, Some(&home), WS, None), OrphanSweep::NotRemoved { - names: vec!["dev".to_owned()], + names: names(["dev".to_owned()]), reason: "docker rm failed (Code(1)): daemon said no".to_owned(), } ); diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index 110f89fd..85d31505 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -1140,6 +1140,10 @@ fn lifecycle_notice(notice: &LifecycleNotice) -> Option { workspace_id, names, } => leftovers_removed(workspace_id, names), + LifecycleNotice::LeftoverContainersNotListed { + workspace_id, + reason, + } => leftovers_not_listed(workspace_id, reason), LifecycleNotice::LeftoverContainersNotRemoved { workspace_id, names, @@ -1173,25 +1177,35 @@ 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: &[String]) -> String { +fn leftovers_removed(workspace_id: &str, names: &NonEmpty) -> String { format!( "Removed containers {workspace_id} left behind: {}", - names.join(", ") + 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 when there are names to give it. -fn leftovers_not_removed(workspace_id: &str, names: &[String], reason: &str) -> String { - if names.is_empty() { - format!("Could not check for containers {workspace_id} left behind: {reason}") - } else { - format!( - "Containers {workspace_id} left behind are still here ({reason}). Remove them with: \ - docker rm -f {}", - names.join(" ") - ) - } +/// 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. @@ -3136,6 +3150,10 @@ pub(crate) fn launch_notice(notice: &LaunchNotice) -> Option { workspace_id, names, } => leftovers_removed(workspace_id, names), + LaunchNotice::LeftoverContainersNotListed { + workspace_id, + reason, + } => leftovers_not_listed(workspace_id, reason), LaunchNotice::LeftoverContainersNotRemoved { workspace_id, names, @@ -5736,7 +5754,7 @@ mod tests { fn leftover_containers_are_named_and_a_refusal_gives_the_command() { let removed = launch_notice(&LaunchNotice::LeftoverContainersRemoved { workspace_id: "ws".to_owned(), - names: vec!["dev_ws".to_owned(), "side_ws".to_owned()], + names: NonEmpty::of(["dev_ws".to_owned(), "side_ws".to_owned()]).expect("two names"), }); assert_eq!( removed.as_deref(), @@ -5745,15 +5763,14 @@ mod tests { let refused = lifecycle_notice(&LifecycleNotice::LeftoverContainersNotRemoved { workspace_id: "ws".to_owned(), - names: vec!["dev_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::LeftoverContainersNotRemoved { + let unlisted = lifecycle_notice(&LifecycleNotice::LeftoverContainersNotListed { workspace_id: "ws".to_owned(), - names: vec![], reason: "docker ps did not answer in time".to_owned(), }) .expect("a line"); From b81a4dc068ada34315d05296ba0a371e673511f3 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 16 Oct 2026 15:58:48 +0100 Subject: [PATCH 04/20] test: rm and kill sweep leftovers once devpod's delete drops its record --- rust/dl/tests/lifecycle.rs | 96 ++++++++++++++++++++++++----- rust/dl/tests/lifecycle_scenario.py | 19 +++++- 2 files changed, 99 insertions(+), 16 deletions(-) diff --git a/rust/dl/tests/lifecycle.rs b/rust/dl/tests/lifecycle.rs index 0803910d..daa17915 100644 --- a/rust/dl/tests/lifecycle.rs +++ b/rust/dl/tests/lifecycle.rs @@ -1038,28 +1038,47 @@ const LEFTOVER_LISTING: &str = "ps --all --no-trunc --filter label=com.docker.co #[test] fn rm_removes_the_containers_devpod_left_behind() { let world = World::base(); - 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" + 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"] ); - 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" + assert!( + run.err.contains( + "Removed containers devlaunch-main-legacy left behind: \ + dev_devlaunch-main-legacy, sidecar_devlaunch-main-legacy\n" ), - ) - .expect("a docker listing"); + "{}", + 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"] + [ + LEFTOVER_LISTING, + "rm --force aaa bbb", + "volume rm --force devlaunch-main-legacy-pixi dind-var-lib-docker-0f4b2c1d", + ] ); assert!( run.err.contains( @@ -1071,6 +1090,55 @@ fn rm_removes_the_containers_devpod_left_behind() { ); } +/// `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 + ); +} + +/// 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 78b37d1a..b1702b08 100755 --- a/rust/dl/tests/lifecycle_scenario.py +++ b/rust/dl/tests/lifecycle_scenario.py @@ -192,7 +192,21 @@ 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: @@ -570,7 +584,7 @@ def build(root: pathlib.Path, shim: pathlib.Path, wanted: set) -> None: "[--stranded-clone] " "[--tagged-release] " "[--sealed-cache] [--symlinked-cache] [--v1-cache] " - "[--devcontainer-volumes]" + "[--devcontainer-volumes] [--record-dropping-delete]" ) flags = {argument.lstrip("-") for argument in sys.argv[3:]} unknown = flags - { @@ -590,6 +604,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)}") From 901feb65849bd578a7614365480f2de3c2360d85 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 16 Oct 2026 15:59:49 +0100 Subject: [PATCH 05/20] test: a delete killed at its deadline still removes the containers left behind --- .../src/flows/lifecycle/tests.rs | 58 ++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/rust/devlaunch-core/src/flows/lifecycle/tests.rs b/rust/devlaunch-core/src/flows/lifecycle/tests.rs index 5311436e..46b9f87d 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: vec!["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. From eb90f5cb70867ed32b9b3eb7d3c865c83324a7e5 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 16 Oct 2026 16:00:34 +0100 Subject: [PATCH 06/20] test: a working_dir in the devpod directory alone marks a leftover --- .../src/flows/orphan_containers.rs | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/rust/devlaunch-core/src/flows/orphan_containers.rs b/rust/devlaunch-core/src/flows/orphan_containers.rs index c24a0f9f..1158058c 100644 --- a/rust/devlaunch-core/src/flows/orphan_containers.rs +++ b/rust/devlaunch-core/src/flows/orphan_containers.rs @@ -283,6 +283,36 @@ mod tests { ); } + /// 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(vec!["dev".to_owned()]) + ); + assert_eq!( + fake.args_to("docker").last().expect("a removal"), + &["rm", "--force", "aaa"] + ); + } + #[test] fn no_devpod_home_means_nothing_to_check_against() { let fake = FakeRunner::new(); From b26f2b62edff26bf590ebac52477e2a95bb6f5ec Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 16 Oct 2026 16:00:34 +0100 Subject: [PATCH 07/20] test: a docker ps that fails removes nothing and keeps docker's words --- .../src/flows/orphan_containers.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/rust/devlaunch-core/src/flows/orphan_containers.rs b/rust/devlaunch-core/src/flows/orphan_containers.rs index 1158058c..1053ba37 100644 --- a/rust/devlaunch-core/src/flows/orphan_containers.rs +++ b/rust/devlaunch-core/src/flows/orphan_containers.rs @@ -313,6 +313,25 @@ mod tests { ); } + #[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::NotRemoved { names, reason } + if names.is_empty() && 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(); From 1be4387f11b6e2dfe32241e7146e4be40aa33fad Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 16 Oct 2026 16:05:04 +0100 Subject: [PATCH 08/20] fix: tests written against the old OrphanSweep shape did not compile The orphan-sweep tests from the cherry-picked branches built Removed and LeftoverContainersRemoved from a Vec, and matched a failed listing as a NotRemoved with no names. Build NonEmpty names, and match the failed listing as NotListed, which is the shape that answer now has. --- rust/devlaunch-core/src/flows/lifecycle/tests.rs | 2 +- rust/devlaunch-core/src/flows/orphan_containers.rs | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/rust/devlaunch-core/src/flows/lifecycle/tests.rs b/rust/devlaunch-core/src/flows/lifecycle/tests.rs index 46b9f87d..911eac20 100644 --- a/rust/devlaunch-core/src/flows/lifecycle/tests.rs +++ b/rust/devlaunch-core/src/flows/lifecycle/tests.rs @@ -2398,7 +2398,7 @@ fn a_delete_killed_at_its_deadline_still_removes_the_containers_left_behind() { assert!( notices.contains(&LifecycleNotice::LeftoverContainersRemoved { workspace_id: "myws".to_owned(), - names: vec!["myws_dev".to_owned()], + names: NonEmpty::one("myws_dev".to_owned()), }), "{notices:?}" ); diff --git a/rust/devlaunch-core/src/flows/orphan_containers.rs b/rust/devlaunch-core/src/flows/orphan_containers.rs index 1053ba37..c4a8369c 100644 --- a/rust/devlaunch-core/src/flows/orphan_containers.rs +++ b/rust/devlaunch-core/src/flows/orphan_containers.rs @@ -305,7 +305,7 @@ mod tests { assert_eq!( sweep(&fake, Some(&home), WS, None), - OrphanSweep::Removed(vec!["dev".to_owned()]) + OrphanSweep::Removed(names(["dev".to_owned()])) ); assert_eq!( fake.args_to("docker").last().expect("a removal"), @@ -324,8 +324,7 @@ mod tests { assert!( matches!( &swept, - OrphanSweep::NotRemoved { names, reason } - if names.is_empty() && reason.contains("daemon is down") + OrphanSweep::NotListed { reason } if reason.contains("daemon is down") ), "{swept:?}" ); From 861f5902323cb14ee199686654ede986f68c3f74 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Fri, 16 Oct 2026 16:06:06 +0100 Subject: [PATCH 09/20] fix: the documented public-api residual count was 40 after NonEmpty joined it The leftover-container notices name NonEmpty, which put domain::workspace_state::NonEmpty in the residual: 41 types now, and the doc guard that recomputes the count from the snapshots failed. --- docs/development.md | 2 +- rust/devlaunch-core/src/lib.rs | 4 ++-- scripts/public-api-snapshots.sh | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/development.md b/docs/development.md index c9c2c1c6..28f332ba 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 **40 types owning close to six hundred rows** in +on the checked-in files rather than guessed at, that is **41 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/src/lib.rs b/rust/devlaunch-core/src/lib.rs index aefb3692..d381e9ae 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: **40 such types +//! `public-api.rest.txt` alone. Counted rather than guessed at: **41 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 -//! 40. `--print-residual` is how you tell which you are looking at. +//! 41. `--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/scripts/public-api-snapshots.sh b/scripts/public-api-snapshots.sh index f9e0afd4..a15846d7 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: 40 such types own close to six +# the checked-in files rather than guessed at: 41 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 40. `--print-residual` is how you tell. +# whose subject is one of the 41. `--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 From 895bf92e665929e475d673953492487c86a51bcd Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 19 Oct 2026 10:09:05 +0100 Subject: [PATCH 10/20] fix: the documented public-api residual count was 41 after the merge from main main's #673 stack and this branch's NonEmpty each added one residual type, so after the merge the count is 42. --- docs/development.md | 2 +- rust/devlaunch-core/src/lib.rs | 4 ++-- scripts/public-api-snapshots.sh | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) 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/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/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 From 3766f210184a195fdf7ffd6b407fb497b62b540e Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 19 Oct 2026 10:18:09 +0100 Subject: [PATCH 11/20] fix: the merge from main filed this branch's CHANGELOG entry under 0.61.0 The entry sat under [Unreleased], which main cut into 0.61.0, so the merge kept it in a released section. It goes back under [Unreleased]. --- CHANGELOG.md | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index be405a94..eeb4e5e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ 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, and a local folder's own compose projects, are left alone. + ## [0.62.0] - 2026-10-16 ### Added @@ -50,17 +61,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 See docs/cli.md, "`--refresh-stale`: recreate the stale workspaces that are idle". -### 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, and a local folder's own compose projects, are left alone. - ## [0.60.1] - 2026-10-16 ### Changed From ac8696c7af44c59edb8c2fd6118556ce73405d67 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 19 Oct 2026 11:26:39 +0100 Subject: [PATCH 12/20] fix: a sweep removed the live containers of a workspace nested in the clone A local folder opened as its own workspace can sit inside another workspace's clone. Sweeping the outer workspace matched the inner one's containers by the clone path alone and removed them, though their labels named the inner workspace's devpod directory and devpod still held it. A container now stays when a label names another id's devpod directory, or a folder devpod holds as another workspace's source inside the clone. When devpod's records do not read, the clone alone matches nothing. --- docs/cleanup.md | 7 + .../devlaunch-core/src/clients/devpod_home.rs | 71 +++++-- .../src/flows/orphan_containers.rs | 177 +++++++++++++++++- rust/dl/tests/lifecycle.rs | 33 ++++ 4 files changed, 269 insertions(+), 19 deletions(-) diff --git a/docs/cleanup.md b/docs/cleanup.md index a3130c77..6b89fe00 100644 --- a/docs/cleanup.md +++ b/docs/cleanup.md @@ -130,6 +130,13 @@ A workspace devpod still holds is left alone, 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/rust/devlaunch-core/src/clients/devpod_home.rs b/rust/devlaunch-core/src/clients/devpod_home.rs index 38a8fde2..e9cad6dc 100644 --- a/rust/devlaunch-core/src/clients/devpod_home.rs +++ b/rust/devlaunch-core/src/clients/devpod_home.rs @@ -160,8 +160,8 @@ impl DevpodHome { }) } - /// Whether `path` is inside devpod's agent-side directory for this workspace, - /// under any context. + /// 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 @@ -169,17 +169,54 @@ impl DevpodHome { /// 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 is_in_agent_dir_of(&self, path: &Path, workspace_id: &str) -> bool { - let Ok(rest) = path.strip_prefix(self.agent_contexts()) else { - return false; - }; + 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); - components - .next() - .is_some_and(|c| c.as_os_str() == "workspaces") - && components - .next() - .is_some_and(|c| c.as_os_str() == workspace_id) + 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. @@ -295,6 +332,16 @@ impl DevpodHome { } } +/// 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`] diff --git a/rust/devlaunch-core/src/flows/orphan_containers.rs b/rust/devlaunch-core/src/flows/orphan_containers.rs index c4a8369c..aba01887 100644 --- a/rust/devlaunch-core/src/flows/orphan_containers.rs +++ b/rust/devlaunch-core/src/flows/orphan_containers.rs @@ -33,6 +33,11 @@ //! 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. @@ -90,10 +95,11 @@ pub(crate) fn sweep( 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)), + .filter(|container| left_by(container, home, workspace_id, clone.as_ref())), ) else { return OrphanSweep::Nothing; }; @@ -113,17 +119,63 @@ pub(crate) fn sweep( } } -/// Whether one container's compose labels name this workspace. +/// 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<&Path>, + clone: Option<&CloneDir<'_>>, ) -> bool { - container.paths().any(|path| { - home.is_in_agent_dir_of(path, workspace_id) - || clone.is_some_and(|clone| path.starts_with(clone)) - }) + 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)] @@ -313,6 +365,117 @@ mod tests { ); } + /// 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(&[]); diff --git a/rust/dl/tests/lifecycle.rs b/rust/dl/tests/lifecycle.rs index f914df49..01f87f83 100644 --- a/rust/dl/tests/lifecycle.rs +++ b/rust/dl/tests/lifecycle.rs @@ -1424,6 +1424,39 @@ fn kill_removes_the_leftovers_once_devpods_delete_drops_its_record() { ); } +/// 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) { From d878940dfa71dac5de837f3df5d5e74f609c4b7d Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 19 Oct 2026 11:27:59 +0100 Subject: [PATCH 13/20] fix: an unreadable devpod record counted as no record holds_any_record answered false when a record could not be read: is_file() is false on EACCES, entry errors were dropped, and a context name that is not UTF-8 was skipped. The sweep then removed the containers of a workspace devpod may still hold. record_presence now answers Held, Absent or Unknown, only a not-found is Absent, and the sweep runs on Absent alone. --- docs/cleanup.md | 3 +- .../devlaunch-core/src/clients/devpod_home.rs | 63 +++++++++++++++---- .../src/flows/orphan_containers.rs | 58 ++++++++++++++++- 3 files changed, 108 insertions(+), 16 deletions(-) diff --git a/docs/cleanup.md b/docs/cleanup.md index 6b89fe00..b067ca1f 100644 --- a/docs/cleanup.md +++ b/docs/cleanup.md @@ -126,7 +126,8 @@ 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, and so is a container that names +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. diff --git a/rust/devlaunch-core/src/clients/devpod_home.rs b/rust/devlaunch-core/src/clients/devpod_home.rs index e9cad6dc..b18e75a0 100644 --- a/rust/devlaunch-core/src/clients/devpod_home.rs +++ b/rust/devlaunch-core/src/clients/devpod_home.rs @@ -145,19 +145,44 @@ impl DevpodHome { /// 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 a yes. A - /// `contexts` directory that would not read is also a yes, so that an - /// unreadable home counts as "a record may be there". - pub(crate) fn holds_any_record(&self, workspace_id: &str) -> bool { - let Ok(contexts) = std::fs::read_dir(self.contexts()) else { - return self.contexts().exists(); + /// 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, }; - contexts.flatten().any(|context| { - context - .file_name() - .to_str() - .is_some_and(|name| self.record(name, workspace_id).is_file()) - }) + 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, @@ -332,6 +357,20 @@ 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> { diff --git a/rust/devlaunch-core/src/flows/orphan_containers.rs b/rust/devlaunch-core/src/flows/orphan_containers.rs index aba01887..1a132739 100644 --- a/rust/devlaunch-core/src/flows/orphan_containers.rs +++ b/rust/devlaunch-core/src/flows/orphan_containers.rs @@ -22,7 +22,8 @@ //! //! 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. Without one, a container is this workspace's leftover when one +//! 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. @@ -43,7 +44,7 @@ use std::path::Path; -use crate::clients::devpod_home::DevpodHome; +use crate::clients::devpod_home::{DevpodHome, RecordPresence}; use crate::clients::docker::{self, ComposeContainer, Unlisted}; use crate::domain::workspace_state::NonEmpty; @@ -87,7 +88,7 @@ pub(crate) fn sweep( let Some(home) = devpod_home else { return OrphanSweep::Nothing; }; - if workspace_id.is_empty() || home.holds_any_record(workspace_id) { + if workspace_id.is_empty() || home.record_presence(workspace_id) != RecordPresence::Absent { return OrphanSweep::Nothing; } let containers = match docker::compose_containers(runner) { @@ -272,6 +273,57 @@ mod tests { 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(&[]); + let record = home + .path() + .join("contexts") + .join(std::ffi::OsStr::from_bytes(b"ctx\xff")) + .join("workspaces") + .join(WS) + .join("workspace.json"); + std::fs::create_dir_all(record.parent().expect("a record directory")) + .expect("a record directory"); + std::fs::write(&record, "{}").expect("a record"); + 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() { From 4d33d05e725c98355dee87835deb0c0bbeb139fc Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 19 Oct 2026 11:29:45 +0100 Subject: [PATCH 14/20] test: every up sweeps a forgotten workspace's leftovers before devpod up No test ran the sweep on the launch path, so deleting it left the suite green. The launch world now has a fake docker that logs where each call fell among devpod's, so one test sees the removal land before the up and another sees a workspace devpod holds cost no docker call. --- rust/dl/tests/launch.rs | 73 ++++++++++++++++++++++++++++++++ rust/dl/tests/launch_scenario.py | 14 ++++++ 2 files changed, 87 insertions(+) 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. From e15f8ae3eeb9f89092182dcaca32ab320678328b Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 19 Oct 2026 11:30:25 +0100 Subject: [PATCH 15/20] test: a launch says a refused docker listing or removal in docker's words The NotListed and NotRemoved arms of the launch's sweep had no test, so either could go silent unnoticed. --- rust/devlaunch-core/src/flows/launch.rs | 61 +++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/rust/devlaunch-core/src/flows/launch.rs b/rust/devlaunch-core/src/flows/launch.rs index fc6f4a91..9a9d478f 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -1390,6 +1390,67 @@ mod leftover_tests { ); } + /// 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] From 4ef0fb581cd61268dfb29dfead9081f592e40d63 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 19 Oct 2026 11:31:04 +0100 Subject: [PATCH 16/20] test: a refused delete that keeps the record asks docker nothing The kept-record tests wrote no docker listing, so the skip passed trivially. This one puts leftovers on the machine, has devpod refuse the delete, and sees no docker ps and no docker rm. --- rust/dl/tests/lifecycle.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/rust/dl/tests/lifecycle.rs b/rust/dl/tests/lifecycle.rs index 01f87f83..95b61484 100644 --- a/rust/dl/tests/lifecycle.rs +++ b/rust/dl/tests/lifecycle.rs @@ -1424,6 +1424,23 @@ fn kill_removes_the_leftovers_once_devpods_delete_drops_its_record() { ); } +/// 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 From 84373a252a188d986e80656a9ddfa6b717511ccb Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 19 Oct 2026 11:32:11 +0100 Subject: [PATCH 17/20] fix: a partly failed docker rm named every container as not removed docker rm exits non-zero when one of several containers is already gone, and still removes the rest. The sweep then said none was removed. It now lists them again and names only those still there; when none is, it says they were removed. --- rust/devlaunch-core/src/clients/docker.rs | 60 ++++++++++--- .../src/flows/orphan_containers.rs | 89 ++++++++++++++++++- 2 files changed, 132 insertions(+), 17 deletions(-) diff --git a/rust/devlaunch-core/src/clients/docker.rs b/rust/devlaunch-core/src/clients/docker.rs index 04d42c49..2b03ac9a 100644 --- a/rust/devlaunch-core/src/clients/docker.rs +++ b/rust/devlaunch-core/src/clients/docker.rs @@ -232,20 +232,54 @@ const COMPOSE_FORMAT: &str = "{{.ID}}\t{{.Names}}\t\ /// Every container on this machine that a compose project made, running or not. pub(crate) fn compose_containers(runner: &dyn Runner) -> Result, Unlisted> { - let spec = SpawnSpec::from(Invocation::new(PROGRAM).with_args([ - "ps", - "--all", - "--no-trunc", - "--filter", - "label=com.docker.compose.project", - "--format", - COMPOSE_FORMAT, - ])) - .with_timeout(LIST_COMPOSE_CONTAINERS); + 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.lines().filter_map(compose_container).collect()) - } + Ok((exit, CapturedText { stdout, .. })) if exit.is_success() => Ok(stdout), Ok((exit, CapturedText { stderr, .. })) => Err(Unlisted::Failed(format!( "docker ps failed ({exit:?}): {}", stderr.trim() diff --git a/rust/devlaunch-core/src/flows/orphan_containers.rs b/rust/devlaunch-core/src/flows/orphan_containers.rs index 1a132739..bf21e46b 100644 --- a/rust/devlaunch-core/src/flows/orphan_containers.rs +++ b/rust/devlaunch-core/src/flows/orphan_containers.rs @@ -108,10 +108,10 @@ pub(crate) fn sweep( 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 } => OrphanSweep::NotRemoved { - names, - reason: format!("docker rm failed ({exit:?}): {}", stderr.trim()), - }, + 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, @@ -120,6 +120,32 @@ pub(crate) fn sweep( } } +/// 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. /// @@ -563,6 +589,61 @@ mod tests { 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(&[]); From fdcce241cade6789b7bb4efa4fd56039737e0837 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 19 Oct 2026 11:33:37 +0100 Subject: [PATCH 18/20] fix: rm matched leftovers against any recorded path under the clone root managed_clone took any recorded local_path under the clone root as the workspace's clone, so a hand-edited path naming a repository's directory would match the containers of every clone in it. Delete now uses the same // shape check as the launch, from one helper. --- rust/devlaunch-core/src/domain/xdg.rs | 32 +++++++++++++++++++ rust/devlaunch-core/src/flows/launch.rs | 16 ++-------- .../src/flows/lifecycle/delete.rs | 7 ++-- 3 files changed, 38 insertions(+), 17 deletions(-) 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 9a9d478f..2ecdf300 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -1295,20 +1295,8 @@ fn sweep_leftovers( ) { let source = Path::new(source); let clone_root = crate::domain::xdg::clone_root_in(&host.cache_dir); - let clone = source - .strip_prefix(&clone_root) - .is_ok_and(|under| { - let parts: Vec<_> = under.components().collect(); - matches!( - parts.as_slice(), - [ - std::path::Component::Normal(_), - std::path::Component::Normal(_), - std::path::Component::Normal(leaf), - ] if *leaf == std::ffi::OsStr::new(workspace_id) - ) - }) - .then_some(source); + 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(), diff --git a/rust/devlaunch-core/src/flows/lifecycle/delete.rs b/rust/devlaunch-core/src/flows/lifecycle/delete.rs index 5f7a341b..c9b1031b 100644 --- a/rust/devlaunch-core/src/flows/lifecycle/delete.rs +++ b/rust/devlaunch-core/src/flows/lifecycle/delete.rs @@ -402,8 +402,9 @@ pub(crate) fn workspace_delete( /// This workspace's clone, when it is one devlaunch made, for the leftover /// sweep to match container labels against. /// -/// Only a clone under devlaunch's own clone root: a recorded path anywhere else -/// is somebody's directory, and a compose project they started there by hand is +/// 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<'_>, @@ -415,7 +416,7 @@ fn managed_clone( // silent so it is not said twice. let mut unsaid = Vec::new(); let path = clones.resolve_clone_path(recorded, &mut unsaid)?; - path.starts_with(clones.repos_dir()).then_some(path) + 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. From f7b97a24999ffc05b364103dc89b9dbdf15331f5 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 19 Oct 2026 11:39:40 +0100 Subject: [PATCH 19/20] fix: the CHANGELOG entry did not say which containers the sweep leaves The review fixes narrowed the rule: a record dl cannot read counts as held, and a container that names another workspace stays. --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eeb4e5e3..410092ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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, and a local folder's own compose projects, are left alone. + 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 From eca5810f563d104cd3275380fd00e474b5eb98bf Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 19 Oct 2026 11:47:41 +0100 Subject: [PATCH 20/20] fix: a sweep test spelled devpod's layout outside its adapter The devpod_layout guard allows devpod's on-disk layout only in devpod_home.rs. The non-UTF-8 context test now writes its record through a test helper there. --- rust/devlaunch-core/src/clients/devpod_home.rs | 17 +++++++++++++++++ .../src/flows/orphan_containers.rs | 13 ++----------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/rust/devlaunch-core/src/clients/devpod_home.rs b/rust/devlaunch-core/src/clients/devpod_home.rs index b18e75a0..247d7cf4 100644 --- a/rust/devlaunch-core/src/clients/devpod_home.rs +++ b/rust/devlaunch-core/src/clients/devpod_home.rs @@ -562,6 +562,23 @@ 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. diff --git a/rust/devlaunch-core/src/flows/orphan_containers.rs b/rust/devlaunch-core/src/flows/orphan_containers.rs index bf21e46b..fe7d9bd1 100644 --- a/rust/devlaunch-core/src/flows/orphan_containers.rs +++ b/rust/devlaunch-core/src/flows/orphan_containers.rs @@ -212,7 +212,7 @@ mod tests { use devlaunch_test_support::{FakeRunner, Response}; use super::*; - use crate::clients::devpod_home::{agent_compose_file, devpod_home_with}; + use crate::clients::devpod_home::{agent_compose_file, devpod_home_with, record_under_context}; const WS: &str = "kinisi-ros-ci-sim-shard-rebalance-cd1e"; @@ -334,16 +334,7 @@ mod tests { fn a_record_under_a_context_name_that_is_not_utf8_holds_it() { use std::os::unix::ffi::OsStrExt; let home = devpod_home_with(&[]); - let record = home - .path() - .join("contexts") - .join(std::ffi::OsStr::from_bytes(b"ctx\xff")) - .join("workspaces") - .join(WS) - .join("workspace.json"); - std::fs::create_dir_all(record.parent().expect("a record directory")) - .expect("a record directory"); - std::fs::write(&record, "{}").expect("a record"); + 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);