From 3903350ef74bb3b5c02217c378257d579665a310 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 17 Oct 2026 11:02:15 +0100 Subject: [PATCH 1/2] fix(rm): remove volumes a devcontainer named after the workspace dl rm swept only two hard-coded volume names (-pixi and dind-var-lib-docker-). A devcontainer that names its volume any other way, such as kinisi-ansible-pixi-${localWorkspaceFolderBasename}, leaked one volume per workspace. Read the volume mounts from the merged config devpod records in workspace_result.json. A recorded source that holds the workspace's substituted basename or devcontainer id is scoped to that workspace and is removed with it. A fixed-name volume is shared between workspaces and is left alone. The names stay devpod's record, never a pattern over docker volume ls. --- docs/cleanup.md | 11 +- rust/devlaunch-core/src/flows/kept_copies.rs | 155 +++++++++++++++++- .../src/flows/lifecycle/delete.rs | 11 +- rust/dl/tests/lifecycle.rs | 12 +- rust/dl/tests/lifecycle_scenario.py | 11 +- 5 files changed, 182 insertions(+), 18 deletions(-) diff --git a/docs/cleanup.md b/docs/cleanup.md index 8c394ddc..8df8f1df 100644 --- a/docs/cleanup.md +++ b/docs/cleanup.md @@ -24,13 +24,20 @@ workspaces](#cleaning-up-workspaces)), and **the named Docker volumes that workspace's devcontainer created**. Every path that removes a workspace does all three: `dl rm`, `dl rme`, `dl --rm`, and `--purge`. -Two volumes per workspace, both named from what devpod recorded substituting into -the devcontainer: +The volumes are named from what devpod recorded substituting into the +devcontainer: | volume | declared by | | --- | --- | | `-pixi` | a `mounts` entry in the devcontainer, for the `.pixi` cache. This repo's own devcontainer has one | | `dind-var-lib-docker-` | the [`docker-in-docker`](https://github.com/devcontainers/features/tree/main/src/docker-in-docker) feature, for the nested daemon's `/var/lib/docker` | +| any other `type=volume` mount whose recorded source holds the workspace's basename or devcontainer id | a `mounts` entry of any other name, such as `myrepo-pixi-${localWorkspaceFolderBasename}` | + +The third row is read from the merged config devpod recorded, where each mount's +source is already substituted. A volume whose name does not vary per workspace +(`myrepo-pip-cache`, say) is **not** removed: every workspace of that repository +mounts the same one, and a cache shared between workspaces is usually shared on +purpose. They used to be left behind, and that was measured rather than assumed: on one development machine, **39 orphaned volumes holding 37.28 GB**, not one of them diff --git a/rust/devlaunch-core/src/flows/kept_copies.rs b/rust/devlaunch-core/src/flows/kept_copies.rs index e2a86a04..fc8c240e 100644 --- a/rust/devlaunch-core/src/flows/kept_copies.rs +++ b/rust/devlaunch-core/src/flows/kept_copies.rs @@ -246,9 +246,10 @@ fn parse_result(bytes: &[u8]) -> Copy { /// What devpod recorded substituting into one workspace's devcontainer. /// -/// Only the two fields the volume names are built from. Both optional because -/// devpod omits an empty one, and a record written by a devpod that never learned -/// about `${devcontainerId}` has neither. +/// The two substituted values the volume names are built from, and the volume +/// sources devpod recorded in the merged config's `mounts`. Both values optional +/// because devpod omits an empty one, and a record written by a devpod that never +/// learned about `${devcontainerId}` has neither. /// /// There is deliberately **no provenance field**, here or on the names. A /// `from_pattern: bool`, or a `Provenance` enum with a `Pattern` arm, would make an @@ -268,6 +269,12 @@ pub(crate) struct Substitutions { /// `SubstitutionContext.DevContainerID` — what `${devcontainerId}` expanded /// to. devcontainer_id: Option, + /// The `source` of every `type=volume` entry in `MergedConfig.mounts`, as + /// devpod recorded it — already substituted, so a devcontainer that declared + /// `myrepo-pixi-${localWorkspaceFolderBasename}` is recorded with the + /// workspace's basename in it. Unfiltered here; [`Self::volume_names`] keeps + /// only the ones scoped to this workspace. + recorded_volumes: Vec, } impl Substitutions { @@ -278,22 +285,47 @@ impl Substitutions { /// `docker-in-docker` feature's, which is devlaunch#325's scope; the two /// *values* in them are devpod's own. This is the only place either template is /// spelled, and `devlaunch-core/tests/volume_names.rs` holds the crate to that. + /// + /// After the two templates come the volume sources devpod recorded in the + /// merged config whose name holds the substituted basename or devcontainer id: + /// a volume any devcontainer named after *this* workspace, which no other + /// workspace can be using. Nothing is synthesized there either — the name is + /// devpod's record verbatim, and the two values only decide which recorded + /// names belong to the workspace. A fixed-name volume is left alone, because + /// a name that does not vary per workspace is a volume every workspace of the + /// repository shares, often on purpose. pub(crate) fn volume_names(&self) -> Vec { let basename = self .local_workspace_folder .as_deref() .map(Path::new) .and_then(Path::file_name) - .map(|name| name.to_string_lossy().into_owned()); - [ - basename.map(|basename| format!("{basename}-pixi")), + .map(|name| name.to_string_lossy().into_owned()) + .filter(|name| !name.is_empty()); + let scopes: Vec<&str> = [basename.as_deref(), self.devcontainer_id.as_deref()] + .into_iter() + .flatten() + .collect(); + let templated = [ + basename.as_ref().map(|basename| format!("{basename}-pixi")), self.devcontainer_id .as_deref() .map(|id| format!("dind-var-lib-docker-{id}")), ] .into_iter() - .flatten() - .collect() + .flatten(); + let scoped = self + .recorded_volumes + .iter() + .filter(|source| scopes.iter().any(|scope| source.contains(scope))) + .cloned(); + let mut names: Vec = Vec::new(); + for name in templated.chain(scoped) { + if !names.contains(&name) { + names.push(name); + } + } + names } } @@ -303,9 +335,53 @@ pub(crate) fn substitutions_in(document: &serde_json::Value) -> Substitutions { Substitutions { local_workspace_folder: non_empty_string(&context["LocalWorkspaceFolder"]), devcontainer_id: non_empty_string(&context["DevContainerID"]), + recorded_volumes: recorded_volume_sources(&document["MergedConfig"]["mounts"]), } } +/// The `source` of each volume mount in a recorded `mounts` array, in order. +/// +/// A devcontainer.json mount is either an object (`{"type": "volume", "source": +/// …}`) or a `--mount`-style string (`"source=…,target=…,type=volume"`), and devpod +/// records whichever the config used. Anything else in the array — a bind, an +/// entry of neither shape, a volume with no source — names nothing. +fn recorded_volume_sources(mounts: &serde_json::Value) -> Vec { + let Some(mounts) = mounts.as_array() else { + return Vec::new(); + }; + mounts + .iter() + .filter_map(|mount| match mount { + serde_json::Value::Object(fields) => (fields.get("type")?.as_str()? == "volume") + .then(|| non_empty_string(fields.get("source")?)) + .flatten(), + serde_json::Value::String(spec) => volume_source_of_spec(spec), + _ => None, + }) + .collect() +} + +/// The source of a `--mount`-style spec, where its `type` is `volume`. +/// +/// `src` is docker's other spelling of `source`. A spec with no `type` is a +/// volume to docker, but devcontainer.json specs always carry one, and guessing +/// costs a removal, so an untyped spec names nothing. +fn volume_source_of_spec(spec: &str) -> Option { + let mut kind = None; + let mut source = None; + for field in spec.split(',') { + match field.split_once('=') { + Some(("type", value)) => kind = Some(value.trim()), + Some(("source" | "src", value)) => source = Some(value.trim()), + _ => {} + } + } + (kind? == "volume") + .then_some(source?) + .filter(|source| !source.is_empty()) + .map(str::to_owned) +} + /// Read the two substituted values off devpod's create result. /// /// Total for [`parse_result`]'s reason, and the same function underneath: the live @@ -606,6 +682,7 @@ mod tests { let both = Substitutions { local_workspace_folder: Some("/repos/o/r/repo-branch-abcd".to_owned()), devcontainer_id: Some("deadbeef".to_owned()), + recorded_volumes: Vec::new(), }; assert_eq!( both.volume_names(), @@ -615,6 +692,7 @@ mod tests { let no_id = Substitutions { local_workspace_folder: Some("/repos/o/r/repo-branch-abcd".to_owned()), devcontainer_id: None, + recorded_volumes: Vec::new(), }; assert_eq!(no_id.volume_names(), ["repo-branch-abcd-pixi"]); @@ -633,6 +711,67 @@ mod tests { assert_eq!(blank.volume_names(), Vec::::new()); } + /// A devcontainer that names its own volume after the workspace has that + /// volume swept with it: the source devpod recorded in the merged config's + /// `mounts` carries the substituted basename (or devcontainer id), so it is a + /// name devpod wrote down *and* one scoped to this workspace. A fixed-name + /// volume beside it is a cache somebody shares between workspaces on purpose, + /// and a bind mount is not a volume at all; neither is named. + /// + /// Both spellings of a mount are devcontainer.json's: the object, and the + /// `--mount`-style string. + #[test] + fn a_recorded_volume_mount_scoped_to_the_workspace_is_named() { + let recorded = parse_substitutions( + serde_json::json!({ + "MergedConfig": { "mounts": [ + { "type": "volume", "source": "repo-main-ab12-pixi", "target": "/w/.pixi" }, + { "type": "volume", "source": "dind-var-lib-docker-deadbeef", "target": "/var/lib/docker" }, + "source=kinisi-ansible-pixi-repo-main-ab12,target=/w/.pixi,type=volume", + { "type": "volume", "source": "cache-for-deadbeef", "target": "/c" }, + { "type": "volume", "source": "shared-pip-cache", "target": "/p" }, + "type=volume,source=shared-apt-cache,target=/a", + { "type": "bind", "source": "/home/me/repo-main-ab12", "target": "/b" }, + "source=/home/me/repo-main-ab12-logs,target=/l,type=bind", + ]}, + "SubstitutionContext": { + "LocalWorkspaceFolder": "/repos/o/r/repo-main-ab12", + "DevContainerID": "deadbeef", + }, + }) + .to_string() + .as_bytes(), + ); + + assert_eq!( + recorded.volume_names(), + [ + "repo-main-ab12-pixi", + "dind-var-lib-docker-deadbeef", + "kinisi-ansible-pixi-repo-main-ab12", + "cache-for-deadbeef", + ] + ); + } + + /// With no substitution recorded there is nothing to scope a mount by, so a + /// recorded volume names nothing — not even one whose source happens to look + /// workspace-shaped. + #[test] + fn a_recorded_volume_mount_with_no_substitution_names_nothing() { + let recorded = parse_substitutions( + serde_json::json!({ + "MergedConfig": { "mounts": [ + { "type": "volume", "source": "repo-main-ab12-pixi", "target": "/w/.pixi" }, + ]}, + }) + .to_string() + .as_bytes(), + ); + + assert_eq!(recorded.volume_names(), Vec::::new()); + } + /// Bytes that are not JSON, and JSON of another shape, name nothing rather /// than failing: a result devlaunch cannot read is a result it removes no /// volumes from. diff --git a/rust/devlaunch-core/src/flows/lifecycle/delete.rs b/rust/devlaunch-core/src/flows/lifecycle/delete.rs index 2663e750..5431d22e 100644 --- a/rust/devlaunch-core/src/flows/lifecycle/delete.rs +++ b/rust/devlaunch-core/src/flows/lifecycle/delete.rs @@ -415,11 +415,12 @@ pub(super) fn sweep_volumes(runner: &dyn Runner, named: Option> /// Deriving the basename from the clone directory devlaunch chose instead would be /// a second answer to a question devpod has already answered, and the two can /// disagree — a workspace opened before a rename, say. Neither *value* is guessed: -/// a substitution that is not in the record names no volume. The two name -/// **templates** are still this repository's own devcontainer and the -/// `docker-in-docker` feature's, so a `mounts` entry naming some third volume is -/// not swept — devlaunch#325's scope, and the follow-up that would end it is -/// reading the mount sources out of the recorded merged config instead. +/// a substitution that is not in the record names no volume. Beyond the two +/// templates, any `type=volume` mount the recorded merged config names is swept +/// when its (already substituted) source holds the workspace's basename or +/// devcontainer id — a volume a devcontainer named after this workspace. A +/// fixed-name volume is a cache the repository's workspaces share and is left +/// alone; `kept_copies::Substitutions::volume_names` is where that is decided. /// /// [`sole_workspace_result`] is what finds the file, rather than a contexts walk of /// its own: an id under two contexts must answer nothing, and `devpod delete` diff --git a/rust/dl/tests/lifecycle.rs b/rust/dl/tests/lifecycle.rs index 1afbb509..51041ac5 100644 --- a/rust/dl/tests/lifecycle.rs +++ b/rust/dl/tests/lifecycle.rs @@ -1264,6 +1264,10 @@ fn rmt_force_is_the_way_past_the_guard() { /// and it is the *binary* that has to hand core the devpod home they are named /// from — a `None` there would leave every core test passing and every volume on /// disk. +/// +/// The third name is a volume the devcontainer's `mounts` named after the +/// workspace, read from devpod's recorded merged config; the fixed-name cache +/// beside it in the same record is shared between workspaces and stays. #[test] fn a_removed_workspaces_devcontainer_volumes_go_with_it() { let world = World::with(&["--devcontainer-volumes"]); @@ -1272,7 +1276,9 @@ fn a_removed_workspaces_devcontainer_volumes_go_with_it() { assert_eq!( world.docker_calls(), - ["volume rm --force devlaunch-main-legacy-pixi dind-var-lib-docker-0f4b2c1d"] + [ + "volume rm --force devlaunch-main-legacy-pixi dind-var-lib-docker-0f4b2c1d devlaunch-cache-devlaunch-main-legacy" + ] ); } @@ -1290,7 +1296,9 @@ fn a_purge_takes_the_devcontainer_volumes_of_the_workspaces_it_deleted() { // never deleted. assert_eq!( world.docker_calls(), - ["volume rm --force devlaunch-main-legacy-pixi dind-var-lib-docker-0f4b2c1d"] + [ + "volume rm --force devlaunch-main-legacy-pixi dind-var-lib-docker-0f4b2c1d devlaunch-cache-devlaunch-main-legacy" + ] ); } diff --git a/rust/dl/tests/lifecycle_scenario.py b/rust/dl/tests/lifecycle_scenario.py index 094ea804..a0010ee0 100755 --- a/rust/dl/tests/lifecycle_scenario.py +++ b/rust/dl/tests/lifecycle_scenario.py @@ -539,7 +539,16 @@ def build(root: pathlib.Path, shim: pathlib.Path, wanted: set) -> None: json.dumps( { "ContainerDetails": {"Id": "abc123def456"}, - "MergedConfig": {}, + # A third volume the devcontainer named after the workspace, + # in the `--mount` string form, and a fixed-name cache every + # workspace of the repository shares. devpod records both + # already substituted. + "MergedConfig": { + "mounts": [ + f"source=devlaunch-cache-{CLEAN_LEAF},target=/c,type=volume", + {"type": "volume", "source": "devlaunch-shared-cache", "target": "/s"}, + ] + }, "SubstitutionContext": { "LocalWorkspaceFolder": str(clean), "ContainerWorkspaceFolder": "/workspaces/devlaunch", From 3d9c45997e17d73e4f30b75ce77e529d1f0846e6 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 17 Oct 2026 11:07:13 +0100 Subject: [PATCH 2/2] fix(rm): match the workspace scope as a whole token of a volume name A plain substring test let a short basename such as kr match a shared volume such as kros-cache. Require the basename or devcontainer id to be bounded by the ends of the name or by a non-alphanumeric character. --- rust/devlaunch-core/src/flows/kept_copies.rs | 51 +++++++++++++++++++- 1 file changed, 49 insertions(+), 2 deletions(-) diff --git a/rust/devlaunch-core/src/flows/kept_copies.rs b/rust/devlaunch-core/src/flows/kept_copies.rs index fc8c240e..833224dd 100644 --- a/rust/devlaunch-core/src/flows/kept_copies.rs +++ b/rust/devlaunch-core/src/flows/kept_copies.rs @@ -291,7 +291,7 @@ impl Substitutions { /// a volume any devcontainer named after *this* workspace, which no other /// workspace can be using. Nothing is synthesized there either — the name is /// devpod's record verbatim, and the two values only decide which recorded - /// names belong to the workspace. A fixed-name volume is left alone, because + /// names belong to the workspace, each as a whole token ([`holds_token`]). A fixed-name volume is left alone, because /// a name that does not vary per workspace is a volume every workspace of the /// repository shares, often on purpose. pub(crate) fn volume_names(&self) -> Vec { @@ -317,7 +317,7 @@ impl Substitutions { let scoped = self .recorded_volumes .iter() - .filter(|source| scopes.iter().any(|scope| source.contains(scope))) + .filter(|source| scopes.iter().any(|scope| holds_token(source, scope))) .cloned(); let mut names: Vec = Vec::new(); for name in templated.chain(scoped) { @@ -329,6 +329,23 @@ impl Substitutions { } } +/// Whether *token* occurs in *name* as a whole token: each end of the match is an +/// end of *name* or a character that is not alphanumeric. +/// +/// So the basename `kr` is found in `kr-cache` and `data.kr`, and not in +/// `kros-cache`: a short basename or id is a substring of many names it has no +/// part in, and each false match here is somebody's shared volume removed. +fn holds_token(name: &str, token: &str) -> bool { + if token.is_empty() { + return false; + } + let bounded = |c: Option| c.is_none_or(|c| !c.is_alphanumeric()); + name.match_indices(token).any(|(start, _)| { + bounded(name[..start].chars().next_back()) + && bounded(name[start + token.len()..].chars().next()) + }) +} + /// The two substituted values devpod's create result records. pub(crate) fn substitutions_in(document: &serde_json::Value) -> Substitutions { let context = &document["SubstitutionContext"]; @@ -754,6 +771,36 @@ mod tests { ); } + /// A short basename is a substring of plenty of names it has nothing to do + /// with: a workspace opened from a folder called `kr` must not take a shared + /// `kros-cache` with it. The basename or id has to stand as a whole token of + /// the volume name, bounded by its ends or by a non-alphanumeric character. + #[test] + fn a_scope_inside_a_longer_word_names_nothing() { + let recorded = parse_substitutions( + serde_json::json!({ + "MergedConfig": { "mounts": [ + { "type": "volume", "source": "kros-cache", "target": "/a" }, + { "type": "volume", "source": "shared-kr2-cache", "target": "/b" }, + { "type": "volume", "source": "beefcake-abc1", "target": "/c" }, + { "type": "volume", "source": "kr_data", "target": "/d" }, + { "type": "volume", "source": "data.kr", "target": "/e" }, + ]}, + "SubstitutionContext": { + "LocalWorkspaceFolder": "/repos/kr", + "DevContainerID": "abc", + }, + }) + .to_string() + .as_bytes(), + ); + + assert_eq!( + recorded.volume_names(), + ["kr-pixi", "dind-var-lib-docker-abc", "kr_data", "data.kr"] + ); + } + /// With no substitution recorded there is nothing to scope a mount by, so a /// recorded volume names nothing — not even one whose source happens to look /// workspace-shaped.