From d869230a7b13965d868b306c86bcf90b96511066 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 29 Aug 2026 18:42:11 +0100 Subject: [PATCH 1/3] Name the sources in --purge's leaving list The list of workspaces a purge leaves standing printed ids and nothing else, and an id is the one thing a user cannot decide on: devpod addresses a workspace by it and it carries nothing about where the workspace came from, so `pythontemplate` reads the same whether it is a `dl ` of yours, a `dl ./project` whose checkout you care about, or something another tool made. Each line now carries the source beside the id, from the same `describe_source` reading that fills `dl --ls`'s SOURCE column, so the two surfaces cannot describe one workspace differently. A source dl cannot read is printed as devpod's own object and said to be one, rather than sitting after a colon looking like a path. The block also says what removing the cache costs the survivors. They keep working, but a clone an older dl placed outside the cache under the retired worktree.repos_dir key is named only by a record inside the cache that is about to go: afterwards `dl rm` deletes the workspace and leaves that directory standing with nothing on the machine pointing at it. Removing such a workspace first is what takes its clone with it. now reports a config.toml that still sets that key. It reads the config directly rather than opening dl's records, because opening them runs the cache migration, which would write records into the tree this command is about to remove and into one an aborted purge was asked to leave alone. Where a workspace still opens such a clone the leaving list names the path; where none does, that notice is the only mention the tree gets. Deliberately not a claim about volumes: the two named volumes a devcontainer made are read from devpod's own record under DEVPOD_HOME, which a purge does not touch. --- CHANGELOG.md | 18 +++++ README.md | 2 +- docs/cleanup.md | 32 ++++++++- rust/dl/src/commands.rs | 28 ++++++++ rust/dl/src/render.rs | 86 +++++++++++++++++++++-- rust/dl/tests/lifecycle.rs | 102 +++++++++++++++++++++++++++- rust/dl/tests/lifecycle_scenario.py | 34 ++++++++++ test/e2e/test_full_workflow.py | 10 ++- 8 files changed, 298 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 15f5e219..9c50abba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 path, and a `metadata.json` written by an older `dl` reads exactly as it did. [docs/cleanup.md](docs/cleanup.md) has the detail. +### Changed + +- **`dl --purge` names where each surviving workspace came from.** The list of + workspaces a purge is leaving standing printed ids and nothing else, and an id + is the one thing you cannot decide on: `pythontemplate` reads exactly the same + whether it is a `dl ` of yours, a `dl ./project` whose checkout you + care about, or something another tool made. Each line now carries the source + beside the id, the same string `dl --ls` shows in its `SOURCE` column. + + The block also says what removing the cache costs the workspaces that stay. + They keep working, but a clone an older `dl` placed outside the cache, under the + retired `worktree.repos_dir` key, is named only by a record inside the cache + that is about to go: after the purge, `dl rm` deletes the workspace + and leaves that directory standing with nothing on the machine pointing at it. + Removing such a workspace first is what takes its clone with it. `--purge` also + reports a `config.toml` that still sets that key now, which is the only mention + a stranded tree gets when no workspace opens it any more. + ### Fixed - **Sixty-six citations that pointed at nothing now point at something, and a diff --git a/README.md b/README.md index 6b6faca7..a9639bfe 100644 --- a/README.md +++ b/README.md @@ -369,7 +369,7 @@ different jobs: | Command | Takes | Leaves | |---|---|---| | `dl --prune` | Clone directories no workspace opens, and the volumes of workspaces devpod no longer lists | Every workspace, container and image | -| `dl --purge` | The workspaces devlaunch created, and its caches | Workspaces it did not create, named before it asks | +| `dl --purge` | The workspaces devlaunch created, and its caches | Workspaces it did not create, each named with its source before it asks | | `dl --reconcile` | Nothing | Repairs records that stopped matching the disk | Two promises worth knowing. **Nothing deletes work that exists nowhere else:** a clone with diff --git a/docs/cleanup.md b/docs/cleanup.md index 2f4ebd33..1a257ec5 100644 --- a/docs/cleanup.md +++ b/docs/cleanup.md @@ -166,7 +166,8 @@ created, meaning the clones it made under its own cache directory (`$XDG_CACHE_H directory the purge is about to remove anyway. Everything else keeps working afterwards, because nothing a purge touches backs it. -Anything it is leaving is named before it asks: +Anything it is leaving is named before it asks, by where it came from and not +only by its id: ``` $ dl --purge @@ -175,12 +176,23 @@ This will remove all devlaunch data: - /home/you/.cache/devlaunch/ (workspace clones, repo caches, the shared pixi cache, completions) Leaving 2 workspace(s) devlaunch did not create: - - pythontemplate - - my-hand-made-workspace + - pythontemplate: https://github.com/blooop/pythontemplate + - my-hand-made-workspace: /home/you/projects/thing + +Removing the cache also drops what dl recorded about them. They keep working, and `dl rm` still removes one. +A clone an older dl placed outside the cache is named only by a record in there, though, so remove such a workspace now if the clone should go with it. Are you sure? [y/N] ``` +The source is the half you can decide on. An id is what devpod addresses a +workspace by and says nothing about where it came from, so `pythontemplate` reads +the same whether it is a `dl ` of yours, a `dl ./project`, or something +somebody else's tool made. It is the same string `dl --ls` shows in its `SOURCE` +column, read the same way, so the two never describe one workspace differently. +A source `dl` cannot read at all, which is devpod's own object rather than a path +or a URL, is printed as that object and said to be one. + Three things `dl` does create can land in that second list rather than the first. `dl ./some/path` and `dl ` open a source `dl` did not clone, so it cannot tell them from a workspace you made by hand. The third is older: a @@ -192,6 +204,20 @@ Erring this way is deliberate: a purge that skips one of your own workspaces costs you a command, and the other kind of mistake costs you work you cannot get back. +That third one is what the two sentences under the list are about. The workspace +stays, and the record naming its clone was in the cache that has just gone, so a +`dl rm` afterwards deletes the workspace and leaves the directory +standing with nothing on the machine pointing at it. Removing it before the purge +is what takes the clone too. If `config.toml` still sets `worktree.repos_dir`, +`--purge` now says so as well, before the plan: where no workspace opens such a +clone any more there is no line in any list for it, and that notice is the only +mention that tree will get. + +Volumes are not part of that loss. The two named volumes a workspace's +devcontainer made are read from devpod's own record under `DEVPOD_HOME`, which a +purge does not touch, so a survivor's volumes still go with it whenever it is +removed. + #### When part of the cache will not go A container writes into its clone as its own user, `vscode` at uid 1000 in the diff --git a/rust/dl/src/commands.rs b/rust/dl/src/commands.rs index 01bcfadc..653b7620 100644 --- a/rust/dl/src/commands.rs +++ b/rust/dl/src/commands.rs @@ -1129,7 +1129,35 @@ fn render_purge(context: &mut CommandContext<'_>, cache: &Path, yes: bool) -> En purge_devlaunch_data(context, cache, yes).with_the_boundary() } +/// `worktree.repos_dir`'s notice on a path that opens no records (devlaunch#461). +/// +/// [`report`] says this for every command that opens dl's records, which is where +/// it belongs and is not here: a purge reads `metadata.json` for nothing, and +/// opening it would run the cache migration, writing records into the tree this +/// command is about to remove and into one an aborted purge was asked to leave +/// alone. So the config is read on its own, which creates nothing and touches +/// nothing. +/// +/// It is worth saying *here* in particular, and #467 left the decision to this +/// ticket. A clone under that retired root is not devlaunch's by the only test +/// `--purge` has, so the purge leaves it and removes the record that was the last +/// thing pointing at it. Where a workspace still opens such a clone the leaving +/// list now names the path; where none does, this line is the only mention that +/// tree will ever get. +/// +/// A `config.toml` that cannot be read says nothing rather than refusing: a purge +/// does not otherwise need the file, and the next command that opens dl's records +/// is where a broken config is somebody's problem. +fn say_retired_keys() { + if let Ok((_, retired)) = session::worktree_config() { + for line in render::retired_keys(&retired) { + eprintln!("{line}"); + } + } +} + fn purge_devlaunch_data(context: &mut CommandContext<'_>, cache: &Path, yes: bool) -> Cleanup { + say_retired_keys(); let plan = match lifecycle::purge_plan(context, cache) { Err(refused) => return Cleanup::Raised(refuse_listing(&refused)), Ok(plan) => plan, diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index bda90964..10a7ab84 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -9,7 +9,7 @@ use std::fmt::Write as _; use std::io; use std::path::Path; -use devlaunch_core::clients::devpod::{ListingUnreadable, NotAListing, NotRun}; +use devlaunch_core::clients::devpod::{ListingUnreadable, NotAListing, NotRun, Workspace}; use devlaunch_core::clients::devpod_home::RepointFailure; use devlaunch_core::clients::gh::{GhEvent, GhUnavailable}; use devlaunch_core::clients::git::Failure as GitFailure; @@ -37,7 +37,7 @@ use devlaunch_core::flows::lifecycle::{ Unlocatable, VolumeRefusal, VolumesKeptBecause, }; use devlaunch_core::flows::listing::{ - LastUsed, SizeCell, Sizes, SweptRepoNote, TableRow, WorkspaceTable, + self, LastUsed, SizeCell, Sizes, SourceKind, SweptRepoNote, TableRow, WorkspaceTable, }; use devlaunch_core::flows::migration::{Listing, MigrationReport}; use devlaunch_core::flows::provision::{BundleFailed, FailureLevel, ProvisionEvent}; @@ -1769,11 +1769,47 @@ pub(crate) const DOCKER_BOUNDARY: &str = concat!( // --purge // --------------------------------------------------------------------------- +/// What removing the cache costs the workspaces the purge is *not* deleting. +/// +/// Said in the block that lists them, because it is a reason to answer `n` and +/// remove one of them properly first (devlaunch#461). Two things are true of a +/// survivor and only the first is obvious: it keeps working, and dl stops knowing +/// anything about it. +/// +/// The second line is where that costs something rather than merely being tidy. A +/// clone a pre-#467 dl placed under `worktree.repos_dir` is outside the cache, so +/// the workspace opening it is foreign here and stays; the record naming that +/// directory is *inside* the cache and goes. After this, `dl rm` answers +/// `NothingRecorded` and leaves the tree standing, and nothing else on the machine +/// mentions it. Removing that workspace before the purge is what takes its clone +/// with it. +/// +/// Deliberately not a claim about volumes. The names of the two volumes a +/// workspace's devcontainer made are read from devpod's own `workspace_result.json` +/// under `DEVPOD_HOME`, which a purge does not touch, so a survivor's volumes still +/// go with it whenever it is removed. That changes when devlaunch keeps its own +/// copy under the cache (devlaunch#456), and this sentence is where it will be +/// said. +const SURVIVORS_LOSE_THE_RECORDS: [&str; 2] = [ + "Removing the cache also drops what dl recorded about them. They keep working, and \ + `dl rm` still removes one.", + "A clone an older dl placed outside the cache is named only by a record in there, though, \ + so remove such a workspace now if the clone should go with it.", +]; + /// What a purge would take, printed before the question is asked. /// /// The workspaces devlaunch did not create are *named* rather than merely left out /// of the count: a user who asked for a clean slate and gets survivors should /// learn it while saying no is still an option. +/// +/// **Named by their source and not only by their id** (devlaunch#461). An id is +/// what devpod addresses a workspace by and carries nothing about where it came +/// from, so `someones-project` reads the same whether it is a `dl ./project` of +/// yours, a `dl `, or a workspace somebody made with `devpod up` -- and +/// this is the one screen where that difference is being decided on. The source is +/// the same string `dl --ls` puts in its `SOURCE` column, from the same reading of +/// it, so the two surfaces cannot describe one workspace differently. pub(crate) fn purge_plan_lines(plan: &PurgePlan) -> Vec { let ownership = plan.ownership(); let mut lines = vec![ @@ -1791,16 +1827,32 @@ pub(crate) fn purge_plan_lines(plan: &PurgePlan) -> Vec { ownership.foreign.len() )); lines.extend( - ownership - .foreign - .iter() - .map(|workspace| format!(" - {}", workspace.id)), + ownership.foreign.iter().map(|workspace| { + format!(" - {}: {}", workspace.id, left_standing_source(workspace)) + }), ); + lines.push(String::new()); + lines.extend(SURVIVORS_LOSE_THE_RECORDS.map(str::to_owned)); } lines.push(String::new()); lines } +/// Where one surviving workspace came from, as the leaving list names it. +/// +/// [`describe_source`](listing::describe_source) is what `dl --ls` reads, and the +/// detail alone carries the answer for the two arms that have one: a path is a +/// path and a URL is a URL, and neither needs the `TYPE` column's word repeated +/// beside it. The third arm does, because devpod's own object is not a source in +/// any readable sense and would otherwise sit after a colon looking like one. +fn left_standing_source(workspace: &Workspace) -> String { + let described = listing::describe_source(workspace.source()); + match described.kind { + SourceKind::Local | SourceKind::Git => described.detail, + SourceKind::Unknown => format!("a source dl cannot read, {}", described.detail), + } +} + /// One rendered line, and which stream it belongs on. /// /// Every other renderer here answers with lines for one stream, because its caller @@ -2930,6 +2982,28 @@ mod tests { assert!(retired_keys(&[]).is_empty()); } + // ------------------------------------------------------------- --purge + + #[test] + fn the_cleanup_page_quotes_the_sentences_a_purge_really_prints() { + // `docs/cleanup.md` reproduces the block `--purge` prints above its + // question, which makes it a second hand-maintained copy of these two + // sentences -- and a sample output that has drifted from the command is + // worse than no sample. This is the diff test the standing rule asks for + // beside such a copy. If that section moves to another page, this path + // moves with it, in the same change. + let page = std::fs::read_to_string( + Path::new(env!("CARGO_MANIFEST_DIR")).join("../../docs/cleanup.md"), + ) + .expect("docs/cleanup.md"); + for said in SURVIVORS_LOSE_THE_RECORDS { + assert!( + page.contains(said), + "docs/cleanup.md no longer quotes what the purge says: {said}" + ); + } + } + // ---------------------------------------------- the refusal advice line /// The one line a person is meant to paste, with paths a shell would diff --git a/rust/dl/tests/lifecycle.rs b/rust/dl/tests/lifecycle.rs index 14e3237b..03c906b6 100644 --- a/rust/dl/tests/lifecycle.rs +++ b/rust/dl/tests/lifecycle.rs @@ -1714,14 +1714,25 @@ fn a_fresh_cache_stops_the_child_before_it_sweeps() { // --purge // =========================================================================== -/// Python's `dl --purge` plan for the base world, verbatim. +/// Python's `dl --purge` plan for the base world, with devlaunch#461's two +/// additions to the block that names the survivors. +/// +/// **The second deliberate divergence from Python in this file**, beside +/// [`DOCKER_BOUNDARY`] below. Python printed a bare workspace id per survivor, and +/// an id is exactly what a user cannot decide on: `someones-project` could be a +/// `dl ./project` of theirs, a `dl `, or a workspace somebody made with +/// `devpod up`. So each one is named by its source, and the sentence under the +/// list says what removing the cache costs the workspaces that are staying. const PURGE_PLAN: &str = "\ This will remove all devlaunch data: - 2 DevPod workspace(s) - {ROOT}/cache/devlaunch/ (workspace clones, repo caches, the shared pixi cache, completions) Leaving 1 workspace(s) devlaunch did not create: - - someones-project + - someones-project: {ROOT}/foreign/proj + +Removing the cache also drops what dl recorded about them. They keep working, and `dl rm` still removes one. +A clone an older dl placed outside the cache is named only by a record in there, though, so remove such a workspace now if the clone should go with it. "; @@ -1738,6 +1749,93 @@ const DOCKER_BOUNDARY: &str = "devlaunch does not manage Docker images: the imag workspaces built may still hold disk, and `docker system df` \ shows what Docker is holding.\n"; +#[test] +fn the_leaving_list_names_each_survivors_source_beside_its_id() { + // devlaunch#461. The id on its own is not something a user can decide on: a + // `dl ./project` of theirs, a `dl ` and a workspace they made with + // `devpod up` all read the same, and this is the one screen where somebody is + // deciding. The source is what tells them apart. + let world = World::base(); + let run = world.answering("n\n", &["--purge"]); + run.exited(0); + assert!( + run.out + .contains(" - someones-project: {ROOT}/foreign/proj\n"), + "the leaving list named an id and no source:\n{}", + run.out + ); + assert!( + run.out + .contains("Removing the cache also drops what dl recorded about them."), + "the block did not say what the purge costs the survivors:\n{}", + run.out + ); +} + +#[test] +fn a_survivor_whose_source_dl_cannot_read_is_said_to_be_one() { + // The third arm of the source, and the reason the leaving list does not simply + // print the detail: devpod's own object after a colon reads like a source, and + // this is the one row where dl has nothing truer to say than the object. + let world = World::with(&["--unplaceable"]); + let run = world.answering("n\n", &["--purge"]); + run.exited(0); + assert!( + run.out.contains( + " - a-source-nobody-can-read: a source dl cannot read, {\"localFolder\": 42}\n" + ), + "{}", + run.out + ); +} + +#[test] +fn a_purge_names_the_clone_a_retired_repos_dir_left_outside_the_cache() { + // devlaunch#461, the case #467's review reproduced. A pre-#467 `dl` put this + // clone under `worktree.repos_dir`, so the workspace opening it is foreign + // here: the purge leaves it standing and removes the record that is the last + // thing on the machine pointing at the tree. It used to print `devlaunch-main-3j1t` + // and nothing else, which names neither the clone nor the fact that it is one. + let world = World::with(&["--stranded-clone"]); + let run = world.answering("n\n", &["--purge"]); + run.exited(0); + assert!( + run.out.contains( + "Leaving 2 workspace(s) devlaunch did not create:\n \ + - someones-project: {ROOT}/foreign/proj\n \ + - devlaunch-main-3j1t: {ROOT}/old-repos/blooop/devlaunch/devlaunch-main-3j1t\n" + ), + "the stranded clone's path is not in the plan:\n{}", + run.out + ); + // And the retired key earns its notice on this path too, which is the half the + // list cannot cover: a clone under that root with no workspace left opening it + // has no line in any plan, and this run is what removes its record. + assert!( + run.err.contains("worktree.repos_dir = '{ROOT}/old-repos'"), + "the purge said nothing about the key that put a tree there:\n{}", + run.err + ); + + // What the sentence under the list is warning about, on disk: the tree stays + // and the record naming it does not. + let purged = world.dl(&["--purge", "-y"]); + purged.exited(0); + assert!( + world.exists("old-repos/blooop/devlaunch/devlaunch-main-3j1t"), + "the purge removed a clone outside its own cache" + ); + assert!( + !world.exists("cache/devlaunch/metadata.json"), + "the record survived, so the sentence about losing it is wrong" + ); + assert_eq!( + world.devpod_calls().last().map(String::as_str), + Some("devpod delete devlaunch-dirty-fqta --force"), + "the stranded workspace was deleted, or the ownership scope moved" + ); +} + #[test] fn a_purge_answered_no_removes_nothing_and_still_names_the_disk_it_does_not_free() { let world = World::with(&["--prunable"]); diff --git a/rust/dl/tests/lifecycle_scenario.py b/rust/dl/tests/lifecycle_scenario.py index e13c5e5d..6260fb25 100755 --- a/rust/dl/tests/lifecycle_scenario.py +++ b/rust/dl/tests/lifecycle_scenario.py @@ -112,6 +112,15 @@ NOT_A_CLONE_LEAF = "devlaunch-opaque-nogit" NOT_A_CLONE_WS = "devlaunch-opaque-nogit" +# --stranded-clone: a clone a pre-#467 dl placed under `worktree.repos_dir`, a key +# this build no longer reads, and the `config.toml` that still names it. The clone +# sits outside the cache, so the workspace opening it is foreign to `--purge` and +# the tree is out of `--prune`'s reach; the record naming it is inside the cache +# the purge is about to remove. +STRANDED_LEAF = "devlaunch-main-3j1t" +STRANDED_WS = "devlaunch-main-3j1t" +STRANDED_ROOT = "old-repos" + def git(cwd, *args): """Run git in *cwd*, insisting it worked.""" @@ -346,6 +355,29 @@ def build(root: pathlib.Path, shim: pathlib.Path, wanted: set) -> None: NOT_A_CLONE_WS, {"localFolder": str(opaque)}, OLDER, "Stopped" ) + if "stranded-clone" in wanted: + # devlaunch#461, reproduced from #467's review. Everything a machine that + # once set `worktree.repos_dir` still has: the clone at the path that key + # named, a devpod workspace sourcing it, the record in dl's cache that + # points at it, and the key itself, still in `config.toml`. + stranded = _clone( + root, + origin, + root / STRANDED_ROOT / "blooop" / "devlaunch" / STRANDED_LEAF, + "main", + ) + worktrees["blooop/devlaunch/stranded"] = _record( + "blooop", "devlaunch", "stranded", stranded, STRANDED_WS + ) + workspaces[STRANDED_WS] = _workspace( + STRANDED_WS, {"localFolder": str(stranded)}, OLDER, "Stopped" + ) + config = root / "config" / "devlaunch" + config.mkdir(parents=True, exist_ok=True) + (config / "config.toml").write_text( + f'[worktree]\nrepos_dir = "{root / STRANDED_ROOT}"\n', encoding="utf-8" + ) + if "unplaceable" in wanted: workspaces[UNPLACEABLE_WS] = _workspace( UNPLACEABLE_WS, {"localFolder": 42}, OLDER, "Stopped" @@ -501,6 +533,7 @@ def build(root: pathlib.Path, shim: pathlib.Path, wanted: set) -> None: "usage: lifecycle_scenario.py [--prunable] " "[--stale-record] [--orphan] [--unplaceable] [--unwritable] " "[--no-cache] [--no-workspaces] [--not-a-clone] [--unpushed] " + "[--stranded-clone] " "[--tagged-release] " "[--sealed-cache] [--symlinked-cache] [--v1-cache] " "[--devcontainer-volumes]" @@ -515,6 +548,7 @@ def build(root: pathlib.Path, shim: pathlib.Path, wanted: set) -> None: "no-cache", "no-workspaces", "not-a-clone", + "stranded-clone", "unpushed", "tagged-release", "sealed-cache", diff --git a/test/e2e/test_full_workflow.py b/test/e2e/test_full_workflow.py index 1e21de16..eac470c1 100644 --- a/test/e2e/test_full_workflow.py +++ b/test/e2e/test_full_workflow.py @@ -331,9 +331,15 @@ def test_purge_deletes_devlaunchs_workspaces_and_leaves_everyone_elses( assert [line for line in printed if line.startswith("Deleting DevPod workspace:")] == [ f"Deleting DevPod workspace: {mine}" ], report - # Named rather than silently passed over. + # Named rather than silently passed over, and named by its *source*: + # an id on its own cannot be told from a `dl ./project` of yours + # (devlaunch#461). The source is asserted as "there is one" rather than + # against a literal, because what devpod echoes back for a git source is + # devpod's normalisation of the URL and not this test's business. assert "Leaving 1 workspace(s) devlaunch did not create:" in printed, report - assert f" - {theirs}" in printed, report + left = [line for line in printed if line.startswith(f" - {theirs}: ")] + assert len(left) == 1, report + assert left[0].split(": ", 1)[1].strip(), report listed_after = workspace_ids() assert mine not in listed_after, report From dd7389028c9a3db9747246af1e075e942f7f0e5e Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 29 Aug 2026 20:33:22 +0100 Subject: [PATCH 2/3] Say the stranded-clone sentence in the tense the run has earned Four findings from #515's fresh-eyes review. The advice under the leaving list ("remove such a workspace now if the clone should go with it") was printed above the question and under -y alike, and a -y run deletes the records that make it possible three lines later. The plan renderer is now told which of the two it is (render::Confirmation), and the sentence has two spellings: the same subject clause, with the action for a reader who can still take it and the consequence for a run that has already committed. Only that one line moves, so PURGE_PLAN and PURGE_PLAN_YES differ by it and nothing else. The reviewer's positive test goes in as written: the printed advice is true only because resolve_clone_path prefers the record's absolute local_path over the cache-derived one, every unit test around that function uses a path under the clone root, and the lifecycle test pinned only the negative half. Now `dl rm` really taking a tree outside the cache is a checked fact. The docs diff test covers the sample's survivor lines as well as the two sentences, which is what the PR body claimed of it: the line format is what this change is about, and `- pythontemplate` on its own would have gone on reading correctly with nothing failing. It also asserts the page does *not* quote the -y spelling, since that one is described in prose instead and a second copy of it would drift unguarded. And the e2e assertion refuses the degraded arm. "Something after the colon" passed just as happily on `a source dl cannot read, {...}`, which is exactly what a broken reading prints. --- docs/cleanup.md | 6 ++ rust/dl/src/commands.rs | 10 ++- rust/dl/src/render.rs | 115 +++++++++++++++++++++++++-------- rust/dl/tests/lifecycle.rs | 52 +++++++++++++-- test/e2e/test_full_workflow.py | 4 ++ 5 files changed, 155 insertions(+), 32 deletions(-) diff --git a/docs/cleanup.md b/docs/cleanup.md index 1a257ec5..7f9fe6d2 100644 --- a/docs/cleanup.md +++ b/docs/cleanup.md @@ -213,6 +213,12 @@ is what takes the clone too. If `config.toml` still sets `worktree.repos_dir`, clone any more there is no line in any list for it, and that notice is the only mention that tree will get. +Under `-y` the second sentence is the same fact in the tense that run has earned. +"Remove such a workspace now" is an action only somebody who still has the +question in front of them can take, and printing it into a run that deletes the +records three lines later would be advice arriving after the door shut, so what +`-y` says instead is what will be true of `dl rm` from then on. + Volumes are not part of that loss. The two named volumes a workspace's devcontainer made are read from devpod's own record under `DEVPOD_HOME`, which a purge does not touch, so a survivor's volumes still go with it whenever it is diff --git a/rust/dl/src/commands.rs b/rust/dl/src/commands.rs index 653b7620..a8420b12 100644 --- a/rust/dl/src/commands.rs +++ b/rust/dl/src/commands.rs @@ -1162,7 +1162,15 @@ fn purge_devlaunch_data(context: &mut CommandContext<'_>, cache: &Path, yes: boo Err(refused) => return Cleanup::Raised(refuse_listing(&refused)), Ok(plan) => plan, }; - print(&render::purge_plan_lines(&plan)); + // The plan is told which of the two it is. Everything in it is preventable + // while the question is still coming, and one sentence of it offers an action + // that only an interactive reader can still take. + let confirmation = if yes { + render::Confirmation::AnsweredOnTheLine + } else { + render::Confirmation::WillBeAsked + }; + print(&render::purge_plan_lines(&plan, confirmation)); if !yes && !confirmed("Are you sure? [y/N] ") { println!("Aborted."); return Cleanup::Ended(Ending::Done); diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index 10a7ab84..31ce802c 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -1769,6 +1769,21 @@ pub(crate) const DOCKER_BOUNDARY: &str = concat!( // --purge // --------------------------------------------------------------------------- +/// Whether the plan above the leaving list is still a decision. +/// +/// `dl --purge` prints the block and *then* asks, so every line of it is something +/// the reader can still prevent. `dl --purge -y` answered on the command line, and +/// the same lines are a record of what is about to happen instead. One sentence in +/// the block turns on that difference, which is why the renderer is told rather +/// than left to print advice into a run that has stopped taking any. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum Confirmation { + /// `Are you sure? [y/N]` is coming. + WillBeAsked, + /// `-y` answered it before the plan was printed. + AnsweredOnTheLine, +} + /// What removing the cache costs the workspaces the purge is *not* deleting. /// /// Said in the block that lists them, because it is a reason to answer `n` and @@ -1776,26 +1791,52 @@ pub(crate) const DOCKER_BOUNDARY: &str = concat!( /// survivor and only the first is obvious: it keeps working, and dl stops knowing /// anything about it. /// -/// The second line is where that costs something rather than merely being tidy. A -/// clone a pre-#467 dl placed under `worktree.repos_dir` is outside the cache, so -/// the workspace opening it is foreign here and stays; the record naming that -/// directory is *inside* the cache and goes. After this, `dl rm` answers -/// `NothingRecorded` and leaves the tree standing, and nothing else on the machine -/// mentions it. Removing that workspace before the purge is what takes its clone -/// with it. -/// /// Deliberately not a claim about volumes. The names of the two volumes a /// workspace's devcontainer made are read from devpod's own `workspace_result.json` /// under `DEVPOD_HOME`, which a purge does not touch, so a survivor's volumes still /// go with it whenever it is removed. That changes when devlaunch keeps its own /// copy under the cache (devlaunch#456), and this sentence is where it will be /// said. -const SURVIVORS_LOSE_THE_RECORDS: [&str; 2] = [ - "Removing the cache also drops what dl recorded about them. They keep working, and \ - `dl rm` still removes one.", - "A clone an older dl placed outside the cache is named only by a record in there, though, \ - so remove such a workspace now if the clone should go with it.", -]; +const SURVIVORS_KEEP_WORKING: &str = "Removing the cache also drops what dl recorded about them. They keep working, and \ + `dl rm` still removes one."; + +/// The sentence under that one, in the tense the run has earned. +/// +/// Where the loss is a loss rather than untidiness. A clone a pre-#467 dl placed +/// under `worktree.repos_dir` is outside the cache, so the workspace opening it is +/// foreign here and stays; the record naming that directory is *inside* the cache +/// and goes. Afterwards `dl rm` answers `NothingRecorded` and leaves the tree +/// standing, and nothing else on the machine mentions it. +/// +/// **Two spellings of one fact, and what separates them is whether it is still +/// actionable.** Printed above the question, "remove such a workspace now" is the +/// action the whole block exists to offer. Printed under `-y` it would be asking +/// for something the same run makes impossible three lines later, which is advice +/// arriving after the door shut. The subject clause is the same either way; only +/// what follows from it moves. +fn stranded_clones(confirmation: Confirmation) -> &'static str { + match confirmation { + Confirmation::WillBeAsked => { + "A clone an older dl placed outside the cache is named only by a record in there, \ + though, so remove such a workspace now if the clone should go with it." + } + Confirmation::AnsweredOnTheLine => { + "A clone an older dl placed outside the cache is named only by a record in there, \ + though, so from here on `dl rm` takes such a workspace and leaves its \ + clone standing." + } + } +} + +/// One survivor's line in the leaving list. +/// +/// A function rather than a `format!` in the loop so that the sample output in +/// `docs/cleanup.md` can be diffed against the real shape of the line. The page is +/// a second hand-maintained copy of it, and `the_cleanup_page_quotes_what_a_purge_ +/// really_prints` is the test the standing rule asks for beside such a copy. +fn leaving_line(id: &str, source: &str) -> String { + format!(" - {id}: {source}") +} /// What a purge would take, printed before the question is asked. /// @@ -1810,7 +1851,7 @@ const SURVIVORS_LOSE_THE_RECORDS: [&str; 2] = [ /// this is the one screen where that difference is being decided on. The source is /// the same string `dl --ls` puts in its `SOURCE` column, from the same reading of /// it, so the two surfaces cannot describe one workspace differently. -pub(crate) fn purge_plan_lines(plan: &PurgePlan) -> Vec { +pub(crate) fn purge_plan_lines(plan: &PurgePlan, confirmation: Confirmation) -> Vec { let ownership = plan.ownership(); let mut lines = vec![ "This will remove all devlaunch data:".to_owned(), @@ -1827,12 +1868,14 @@ pub(crate) fn purge_plan_lines(plan: &PurgePlan) -> Vec { ownership.foreign.len() )); lines.extend( - ownership.foreign.iter().map(|workspace| { - format!(" - {}: {}", workspace.id, left_standing_source(workspace)) - }), + ownership + .foreign + .iter() + .map(|workspace| leaving_line(&workspace.id, &left_standing_source(workspace))), ); lines.push(String::new()); - lines.extend(SURVIVORS_LOSE_THE_RECORDS.map(str::to_owned)); + lines.push(SURVIVORS_KEEP_WORKING.to_owned()); + lines.push(stranded_clones(confirmation).to_owned()); } lines.push(String::new()); lines @@ -2985,23 +3028,41 @@ mod tests { // ------------------------------------------------------------- --purge #[test] - fn the_cleanup_page_quotes_the_sentences_a_purge_really_prints() { + fn the_cleanup_page_quotes_what_a_purge_really_prints() { // `docs/cleanup.md` reproduces the block `--purge` prints above its - // question, which makes it a second hand-maintained copy of these two - // sentences -- and a sample output that has drifted from the command is - // worse than no sample. This is the diff test the standing rule asks for - // beside such a copy. If that section moves to another page, this path - // moves with it, in the same change. + // question, which makes the page a second hand-maintained copy of it -- + // and a sample output that has drifted from the command is worse than no + // sample. This is the diff test the standing rule asks for beside such a + // copy. If that section moves to another page, this path moves with it, + // in the same change. + // + // The survivor line is in here as well as the two sentences, because the + // line is what this change is about: the sample would go on reading + // `- pythontemplate` on its own if the renderer's format ever went back to + // an id, and nothing else would notice. let page = std::fs::read_to_string( Path::new(env!("CARGO_MANIFEST_DIR")).join("../../docs/cleanup.md"), ) .expect("docs/cleanup.md"); - for said in SURVIVORS_LOSE_THE_RECORDS { + let quoted = [ + SURVIVORS_KEEP_WORKING.to_owned(), + stranded_clones(Confirmation::WillBeAsked).to_owned(), + leaving_line("pythontemplate", "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/blooop/pythontemplate"), + leaving_line("my-hand-made-workspace", "/home/you/projects/thing"), + ]; + for said in quoted { assert!( - page.contains(said), + page.contains(&said), "docs/cleanup.md no longer quotes what the purge says: {said}" ); } + // And the `-y` spelling is deliberately *not* quoted there: the page + // describes it in prose instead, so there is no second copy of it to + // drift. + assert!( + !page.contains(stranded_clones(Confirmation::AnsweredOnTheLine)), + "the page grew a copy of the -y sentence; guard it here or take it out" + ); } // ---------------------------------------------- the refusal advice line diff --git a/rust/dl/tests/lifecycle.rs b/rust/dl/tests/lifecycle.rs index 03c906b6..4ebcfc9e 100644 --- a/rust/dl/tests/lifecycle.rs +++ b/rust/dl/tests/lifecycle.rs @@ -1723,6 +1723,11 @@ fn a_fresh_cache_stops_the_child_before_it_sweeps() { /// `dl ./project` of theirs, a `dl `, or a workspace somebody made with /// `devpod up`. So each one is named by its source, and the sentence under the /// list says what removing the cache costs the workspaces that are staying. +/// +/// This is the plan a run that is **about to ask** prints. `-y` prints +/// [`PURGE_PLAN_YES`], which differs in the one line that offers an action, and +/// the two are spelled out separately rather than derived from each other so that +/// a change to either is read as the output change it is. const PURGE_PLAN: &str = "\ This will remove all devlaunch data: - 2 DevPod workspace(s) @@ -1736,6 +1741,26 @@ A clone an older dl placed outside the cache is named only by a record in there, "; +/// The same plan under `-y`, where the last sentence is in the tense that run has +/// earned. +/// +/// "Remove such a workspace now" is an action only a reader with the question +/// still in front of them can take; the same run deletes the records that make it +/// possible three lines later. So `-y` gets the same fact as what will be true +/// from then on, and every `-y` golden below is built from this one. +const PURGE_PLAN_YES: &str = "\ +This will remove all devlaunch data: + - 2 DevPod workspace(s) + - {ROOT}/cache/devlaunch/ (workspace clones, repo caches, the shared pixi cache, completions) + +Leaving 1 workspace(s) devlaunch did not create: + - someones-project: {ROOT}/foreign/proj + +Removing the cache also drops what dl recorded about them. They keep working, and `dl rm` still removes one. +A clone an older dl placed outside the cache is named only by a record in there, though, so from here on `dl rm` takes such a workspace and leaves its clone standing. + +"; + /// The sentence both cleanups end on. /// /// **A deliberate divergence from Python, and the one line in this file that is @@ -1836,6 +1861,25 @@ fn a_purge_names_the_clone_a_retired_repos_dir_left_outside_the_cache() { ); } +#[test] +fn removing_a_stranded_workspace_before_the_purge_takes_its_clone() { + // What the plan's last sentence advises, checked rather than assumed. It is + // true because `resolve_clone_path` prefers the record's absolute `local_path` + // over the path derived from the cache root, and every unit test around that + // function uses a path *under* the clone root -- so a later "only remove trees + // under the cache" hardening would turn a printed sentence into bad advice + // with nothing failing. Found in review of devlaunch#461. + let world = World::with(&["--stranded-clone"]); + let run = world.dl(&["devlaunch-main-3j1t", "rm"]); + assert!( + !world.exists("old-repos/blooop/devlaunch/devlaunch-main-3j1t"), + "the clone stayed: exit {:?}\nout:{}\nerr:{}", + run.code, + run.out, + run.err + ); +} + #[test] fn a_purge_answered_no_removes_nothing_and_still_names_the_disk_it_does_not_free() { let world = World::with(&["--prunable"]); @@ -1899,7 +1943,7 @@ fn a_purge_deletes_the_workspaces_devlaunch_made_and_its_cache() { assert_eq!( run.out, format!( - "{PURGE_PLAN}Deleting DevPod workspace: devlaunch-main-legacy\n\ + "{PURGE_PLAN_YES}Deleting DevPod workspace: devlaunch-main-legacy\n\ Deleting DevPod workspace: devlaunch-dirty-fqta\n\ Removed: {{ROOT}}/cache/devlaunch\n{DOCKER_BOUNDARY}" ) @@ -1945,7 +1989,7 @@ fn a_purge_that_could_not_remove_everything_says_which_paths_refused() { assert_eq!( run.out, format!( - "{PURGE_PLAN}Deleting DevPod workspace: devlaunch-main-legacy\n\ + "{PURGE_PLAN_YES}Deleting DevPod workspace: devlaunch-main-legacy\n\ Deleting DevPod workspace: devlaunch-dirty-fqta\n\ Removed what was permitted under {{ROOT}}/cache/devlaunch. These refused:\n \ - {{ROOT}}/cache/devlaunch/repos/blooop/devlaunch/devlaunch-gone-locked/held: \ @@ -1972,7 +2016,7 @@ fn a_purge_that_removed_not_one_path_says_that_rather_than_the_other_sentence() assert_eq!( run.out, format!( - "{PURGE_PLAN}Deleting DevPod workspace: devlaunch-main-legacy\n\ + "{PURGE_PLAN_YES}Deleting DevPod workspace: devlaunch-main-legacy\n\ Deleting DevPod workspace: devlaunch-dirty-fqta\n\ Removed nothing under {{ROOT}}/cache/devlaunch. These refused:\n \ - {{ROOT}}/cache/devlaunch: is a symbolic link to {{ROOT}}/elsewhere/devlaunch, \ @@ -2016,7 +2060,7 @@ fn a_purge_that_deleted_workspaces_and_found_no_cache_says_nothing_about_the_cac assert_eq!( run.out, format!( - "{PURGE_PLAN}Deleting DevPod workspace: devlaunch-main-legacy\n\ + "{PURGE_PLAN_YES}Deleting DevPod workspace: devlaunch-main-legacy\n\ Deleting DevPod workspace: devlaunch-dirty-fqta\n{DOCKER_BOUNDARY}" ) ); diff --git a/test/e2e/test_full_workflow.py b/test/e2e/test_full_workflow.py index eac470c1..1e66ffd5 100644 --- a/test/e2e/test_full_workflow.py +++ b/test/e2e/test_full_workflow.py @@ -340,6 +340,10 @@ def test_purge_deletes_devlaunchs_workspaces_and_leaves_everyone_elses( left = [line for line in printed if line.startswith(f" - {theirs}: ")] assert len(left) == 1, report assert left[0].split(": ", 1)[1].strip(), report + # And a source, not the fallback for one dl could not read: that arm is + # what a broken reading degrades to, and "something after the colon" + # accepts it just as happily as a real URL. + assert "a source dl cannot read" not in left[0], report listed_after = workspace_ids() assert mine not in listed_after, report From 9d9386c49d3c7a3d796b65ec804a5b3d783b20ad Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 29 Aug 2026 20:48:38 +0100 Subject: [PATCH 3/3] Say what a purge costs the kept copies, now that #456 has landed This branch said "volumes are not part of that loss" and gave the reason: the two names are read from devpod's own workspace_result.json under DEVPOD_HOME, which a purge does not touch. That was true when it was written and stopped being true underneath it when #516 merged, which puts dl's own copy of those names under the cache the purge removes. It is the case #452 predicted the sentence would have to cover: --purge never deletes the foreign workspaces, so it destroys the copies naming their volumes while leaving the volumes standing. What is unchanged is the ordinary route, and the sentence now separates the two: `dl rm` still takes a survivor's volumes with it for as long as devpod lists the workspace, because that read is devpod's record and not the copy. The reach a purge costs is --prune's, which reclaims from the copy after devpod has forgotten a workspace, so a survivor deleted with a bare devpod delete afterwards leaves volumes nothing can name. The doc comment, the docs page and the CHANGELOG entry say the same thing, and the two goldens move with the line. --- CHANGELOG.md | 9 ++++++--- docs/cleanup.md | 16 +++++++++++----- rust/dl/src/render.rs | 21 +++++++++++++-------- rust/dl/tests/lifecycle.rs | 10 ++++++---- 4 files changed, 36 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c50abba..6a516673 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,9 +82,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 retired `worktree.repos_dir` key, is named only by a record inside the cache that is about to go: after the purge, `dl rm` deletes the workspace and leaves that directory standing with nothing on the machine pointing at it. - Removing such a workspace first is what takes its clone with it. `--purge` also - reports a `config.toml` that still sets that key now, which is the only mention - a stranded tree gets when no workspace opens it any more. + Removing such a workspace first is what takes its clone with it. The copy of + their volume names goes the same way, so a survivor deleted with a bare `devpod + delete` after a purge leaves volumes nothing can reclaim, where `dl --prune` + would have. `--purge` also reports a `config.toml` that still sets that key now, + which is the only mention a stranded tree gets when no workspace opens it any + more. ### Fixed diff --git a/docs/cleanup.md b/docs/cleanup.md index 7f9fe6d2..59483f82 100644 --- a/docs/cleanup.md +++ b/docs/cleanup.md @@ -179,7 +179,7 @@ Leaving 2 workspace(s) devlaunch did not create: - pythontemplate: https://github.com/blooop/pythontemplate - my-hand-made-workspace: /home/you/projects/thing -Removing the cache also drops what dl recorded about them. They keep working, and `dl rm` still removes one. +Removing the cache also drops what dl recorded about them, the copy of their volume names included. They keep working, and `dl rm` still removes one and its volumes while devpod still lists it. A clone an older dl placed outside the cache is named only by a record in there, though, so remove such a workspace now if the clone should go with it. Are you sure? [y/N] @@ -219,10 +219,16 @@ question in front of them can take, and printing it into a run that deletes the records three lines later would be advice arriving after the door shut, so what `-y` says instead is what will be true of `dl rm` from then on. -Volumes are not part of that loss. The two named volumes a workspace's -devcontainer made are read from devpod's own record under `DEVPOD_HOME`, which a -purge does not touch, so a survivor's volumes still go with it whenever it is -removed. +The volume names are part of that loss, and this is the one place it shows. +Deleting a survivor with `dl rm` still takes its volumes: that read +happens at delete time, out of devpod's own record under `DEVPOD_HOME`, which a +purge does not touch. What goes is [the copy `dl` +keeps](#the-volumes-of-a-workspace-devpod-has-already-forgotten), which is what +`--prune` reclaims from once devpod has forgotten a workspace. So a survivor +deleted with a bare `devpod delete` after a purge leaves both its volumes with +nothing on the machine naming them, where before the purge `dl --prune` would +have reclaimed them. Deleting such a workspace through `dl` is what avoids that, +and it is the same advice the sentence above gives for its clone. #### When part of the cache will not go diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index 31ce802c..9f0ce55d 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -1791,14 +1791,19 @@ pub(crate) enum Confirmation { /// survivor and only the first is obvious: it keeps working, and dl stops knowing /// anything about it. /// -/// Deliberately not a claim about volumes. The names of the two volumes a -/// workspace's devcontainer made are read from devpod's own `workspace_result.json` -/// under `DEVPOD_HOME`, which a purge does not touch, so a survivor's volumes still -/// go with it whenever it is removed. That changes when devlaunch keeps its own -/// copy under the cache (devlaunch#456), and this sentence is where it will be -/// said. -const SURVIVORS_KEEP_WORKING: &str = "Removing the cache also drops what dl recorded about them. They keep working, and \ - `dl rm` still removes one."; +/// **The volume names are named because they are now in the cache** +/// (devlaunch#456, merged while this was open). dl keeps its own copy of the two +/// volumes a workspace's devcontainer made, under the cache, so a purge that +/// leaves a foreign workspace standing destroys the copy of *its* names while +/// leaving its volumes -- which is the exact case #452 predicted this sentence +/// would have to cover. What is not lost is the ordinary route: `dl rm` +/// reads devpod's own `workspace_result.json` under `DEVPOD_HOME`, which a purge +/// does not touch, so a survivor deleted through dl still takes its volumes with +/// it. The copy is what `--prune` reclaims from *after* devpod has forgotten a +/// workspace, and that is the reach a purge costs it. +const SURVIVORS_KEEP_WORKING: &str = "Removing the cache also drops what dl recorded about them, the copy of their volume \ + names included. They keep working, and `dl rm` still removes one and its \ + volumes while devpod still lists it."; /// The sentence under that one, in the tense the run has earned. /// diff --git a/rust/dl/tests/lifecycle.rs b/rust/dl/tests/lifecycle.rs index 4ebcfc9e..135b2904 100644 --- a/rust/dl/tests/lifecycle.rs +++ b/rust/dl/tests/lifecycle.rs @@ -1736,7 +1736,7 @@ This will remove all devlaunch data: Leaving 1 workspace(s) devlaunch did not create: - someones-project: {ROOT}/foreign/proj -Removing the cache also drops what dl recorded about them. They keep working, and `dl rm` still removes one. +Removing the cache also drops what dl recorded about them, the copy of their volume names included. They keep working, and `dl rm` still removes one and its volumes while devpod still lists it. A clone an older dl placed outside the cache is named only by a record in there, though, so remove such a workspace now if the clone should go with it. "; @@ -1756,7 +1756,7 @@ This will remove all devlaunch data: Leaving 1 workspace(s) devlaunch did not create: - someones-project: {ROOT}/foreign/proj -Removing the cache also drops what dl recorded about them. They keep working, and `dl rm` still removes one. +Removing the cache also drops what dl recorded about them, the copy of their volume names included. They keep working, and `dl rm` still removes one and its volumes while devpod still lists it. A clone an older dl placed outside the cache is named only by a record in there, though, so from here on `dl rm` takes such a workspace and leaves its clone standing. "; @@ -1790,8 +1790,10 @@ fn the_leaving_list_names_each_survivors_source_beside_its_id() { run.out ); assert!( - run.out - .contains("Removing the cache also drops what dl recorded about them."), + run.out.contains( + "Removing the cache also drops what dl recorded about them, the copy of \ + their volume names included." + ), "the block did not say what the purge costs the survivors:\n{}", run.out );