Repository navigation
fix: remove the compose containers a forgotten workspace left running - #682
Conversation
devpod delete finds a workspace's container only by dev.containers.id=<uid of the record it deletes>, 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 <ws> 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.
…iners sweep_leftovers took any source under the clone root as the workspace's clone, so `dl <cache>/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.
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.
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.
…oined it The leftover-container notices name NonEmpty<String>, which put domain::workspace_state::NonEmpty in the residual: 41 types now, and the doc guard that recomputes the count from the snapshots failed.
Reviewer's GuideThis PR adds a label- and path-based orphan compose-container sweep, guarded by the absence of any devpod workspace record, and invokes it during rm/kill cleanup and before up while preserving local-folder and unrelated-workspace containers. It also adds bounded Docker operations, explicit diagnostics, extensive regression coverage, and documentation/API snapshot updates. Sequence diagram for orphan container cleanup during workspace lifecycle commandssequenceDiagram
actor User
participant DL as dl rm kill up
participant Devpod as devpod
participant Sweep as orphan_containers::sweep
participant Docker as Docker
User->>DL: Run lifecycle command
alt rm or kill
DL->>Devpod: delete workspace
Devpod-->>DL: Record removed, failed, or timed out
end
DL->>Sweep: sweep workspace_id and managed clone
Sweep->>Sweep: Check holds_any_record(workspace_id)
alt devpod still holds a record
Sweep-->>DL: Nothing
else no record exists
Sweep->>Docker: compose_containers()
Docker-->>Sweep: Compose labels and paths
Sweep->>Sweep: Match working_dir or config_files
alt matching containers found
Sweep->>Docker: remove_containers(ids)
Docker-->>Sweep: Removal result
Sweep-->>DL: Removed names or failure details
else no matches
Sweep-->>DL: Nothing
end
end
alt up
DL->>Devpod: up workspace
else rm or kill
DL->>DL: Delete managed clone after sweep
end
Flow diagram for label- and path-based orphan container matchingflowchart TB
start["Workspace lifecycle command"] --> record{"Does devpod hold any record for workspace id?"}
record -->|Yes| keep["Leave containers untouched"]
record -->|No| list["docker ps --all --filter label=com.docker.compose.project"]
list --> paths{"working_dir or config_files is inside a trusted path?"}
paths -->|No| unrelated["Keep unrelated containers"]
paths -->|Yes| matched["Collect container ids and names"]
matched --> remove["docker rm --force"]
remove --> report["Report removed container names"]
list -->|Docker unavailable or fails| diagnose["Report that leftovers could not be checked"]
subgraph trusted["Trusted paths"]
agent["devpod agent directory for workspace id"]
clone["Exact dl-managed clone for workspace id"]
end
agent --> paths
clone --> paths
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…-containers # Conflicts: # CHANGELOG.md
Codecov Report❌ Patch coverage is Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…from main main's #673 stack and this branch's NonEmpty each added one residual type, so after the merge the count is 42.
…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].
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
This PR removes the compose containers that devpod lost track of, on rm, kill and every up. One proven defect blocks the merge: the clone-path match can remove the live containers of another workspace that devpod still holds (inline on orphan_containers.rs:125). Removing the wrong containers is the one outcome this change must never cause.
Ran: Defects, Types, Tests and Mutant (1264 changed lines, so the five-axis split; Spec skipped, because no spec is available: the PR closes no issue). Defects reproduced findings 1 and 2 with probe tests. Mutant: the old prefix rule restored in sweep_leftovers makes a_launch_at_a_clones_parent_leaves_the_clones_containers fail, so the mutant is killed.
Not inline:
- The refused-delete skip has no test. A binary test with a kept record (
--devcontainer-volumes), a failed delete andleave_containers_behindshould assert nodocker psand norm --force. The current kept-record tests write nodocker-psfile, so the skip passes trivially. managed_clone(delete.rs:418) checks onlystarts_with(repos_dir). The launch checks the<owner>/<repo>/<id>shape. Only a hand-editedlocal_pathreaches the gap today.
| ) -> bool { | ||
| container.paths().any(|path| { | ||
| home.is_in_agent_dir_of(path, workspace_id) | ||
| || clone.is_some_and(|clone| path.starts_with(clone)) |
There was a problem hiding this comment.
Removes another workspace's live containers (reproduced). A local-folder workspace sub at <repos>/o/r/ws/sub has a devpod record. ws has none. dl ws rm / kill / a relaunch of ws sweeps with clone <repos>/o/r/ws. The container of sub has working_dir=<clone>/sub/.devcontainer, so path.starts_with(clone) matches, and the sweep returns Removed(["sub_dev"]) and runs docker rm --force on it. That container's own config_files label names the agent dir of sub, which devpod holds. This breaks the invariant at lines 23-24 and in docs/cleanup.md. Fix: skip a container when any of its labels names the agent dir of a different id that devpod holds. Add the test with exactly this shape.
| context | ||
| .file_name() | ||
| .to_str() | ||
| .is_some_and(|name| self.record(name, workspace_id).is_file()) |
There was a problem hiding this comment.
An unreadable record counts as "no record" (reproduced; Defects and Types both found it). is_file() returns false on EACCES, .flatten() drops entry errors, and a non-UTF-8 context name fails to_str(). Root-owned dirs from container mounts happen on real hosts. With contexts/default/workspaces/ws mode 000, holds_any_record("ws") is false, and the sweep removes ws's containers through the agent-dir match. The doc at 148-150 promises the opposite. Shape: fn record_presence(..) -> Held | Absent | Unknown(io::Error) from fs::metadata, with only NotFound as Absent, and sweep only on Absent.
| // 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); |
There was a problem hiding this comment.
No test runs this call. leftover_tests calls sweep_leftovers directly, and no launch-flow test or rust/dl/tests/launch.rs test feeds a docker ps row, so deleting this line leaves the suite green. This is the path that runs on every up. Cases to add in rust/dl/tests/launch.rs: (1) a world with no record and one leftover in docker-ps: docker_calls() holds rm --force <id> before devpod up, and stderr names it; (2) a world with a record: docker_calls() is empty, so a normal up costs no docker call.
| workspace_id: workspace_id.to_owned(), | ||
| names, | ||
| }), | ||
| OrphanSweep::NotListed { reason } => { |
There was a problem hiding this comment.
The two failure arms (NotListed here, NotRemoved below) have no test. Add cases in leftover_tests: a docker ps that fails gives LeftoverContainersNotListed with docker's words, and a docker rm that fails gives LeftoverContainersNotRemoved with docker's words.
| 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 { |
There was a problem hiding this comment.
A partly failed rm reports the wrong names (argued). If container a goes away between docker ps and docker rm --force a b, docker exits 1 but still removes b. NotRemoved { names: [a, b] } then tells the user that neither was removed. Nothing is removed in error, but the message is wrong. A docker ps after the failure could name what is really left.
… 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.
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.
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.
…ords The NotListed and NotRemoved arms of the launch's sweep had no test, so either could go silent unnoticed.
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.
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.
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 <owner>/<repo>/<id> shape check as the launch, from one helper.
The review fixes narrowed the rule: a record dl cannot read counts as held, and a container that names another workspace stays.
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.
1 participant
After this change,
dl rm,dl killanddl upremove the compose containers that devpod lost track of, so a workspace with a fixedcontainer_name:can come up again at the same path.Evidence
xychart-beta title "Leftover containers still Up: bars = dl 0.60.0, line = this branch" x-axis ["kill, compose down fails", "up over leftovers", "rm, record has new uid"] y-axis "containers Up" 0 --> 3 bar [2, 2, 2] line [0, 0, 0]The chart shows the old compose containers that are still Up after each command: 2 of 2 with dl 0.60.0, 0 with this branch. The source is a throwaway local folder
dlleakprobe, a devcontainer with two compose files and two services that setcontainer_name:. ItsinitializeCommandwrites the.envthat one compose file needs, as kinisi_ros does. The scriptsrepro.sh <dl>andrepro_up_rm.sh <released dl> <fixed dl>ran on 2026-10-16 against the release build of this branch (it printsdl 0.60.1, the version onmain). Each scenario:dl killaftercompose downfails.dl up, delete.devcontainer/.env, thendl kill.error deleting container: error=exit status 1, thendeleted workspace dlleakprobe.dl killexits 0, anddlleakprobe_dev_fixedanddlleakprobe_side_fixedstayUp.Removed containers dlleakprobe left behind: dlleakprobe_dev_fixed, dlleakprobe_side_fixed, anddocker ps -a --filter name=dlleakprobeis empty.dl upover the leftovers.Conflict. The container name "/dlleakprobe_dev_fixed" is already in use. The old containers stayUp.Removed containers dlleakprobe left behind: dlleakprobe_side_fixed, dlleakprobe_dev_fixed, and then compose starts both services again in a new project (default-dl-3b171→default-dl-6f164).dl rmafter a failedupmade a new record with a new uid (default-dl-899e1).dl rmprintsRemoved workspace dlleakprobe., exits 0, and the two old containers stayUp.Removed containers dlleakprobe left behind: dlleakprobe_dev_fixed, dlleakprobe_side_fixed, thenRemoved workspace dlleakprobe., and no container is left.dl upexits 1 in every run, also after the fix. The cause is in the probe: its image has nogit, so the dotfiles step fails (exec: "git": executable file not found in $PATH) after the containers are up.What this changes
rust/devlaunch-core/src/flows/orphan_containers.rs. It runs only when devpod has no record of the workspace id in any context. It lists the compose containers by label and removes each container whosecom.docker.compose.project.working_dirorconfig_filesis inside devpod's directory for that id, or inside dl's clone for that id under<cache>/repos. It never matches by container name.dl rmanddl killrun the flow afterdevpod delete, when the delete succeeds, fails or times out, and before dl deletes the clone.dl upruns the flow inside the launch lock, beforedevpod up. For the clone, it uses only the exact clone directory of this workspace id, sodlon a parent directory of clones does not touch other workspaces.Removed containers <id> left behind: <names>. If docker refuses, dl prints adocker rm -f <names>hint. Ifdocker psfails, dl says that it could not check, with docker's words. The outcome types useNonEmpty<String>, so "removed" and "not removed" always carry at least one name.docs/cleanup.mdandCHANGELOG.mddescribe the new behavior. The public-API snapshot and the documented residual count (40 → 41, becauseNonEmptyis now part of it) match.Where this fits
devpod v0.26.3 finds a dev container only by the label
dev.containers.id=<uid of the record>, and compose names the project after that uid. When no container carries that uid, the delete succeeds and touches nothing. With--force, devpod drops the record even whencompose downfailed. The new flow (blue) runs after that point:--- config: flowchart: curve: linear nodeSpacing: 24 rankSpacing: 36 wrappingWidth: 340 --- flowchart TB classDef default fill:transparent classDef changed fill:transparent,stroke:#2563eb,stroke-width:2px classDef outside fill:transparent,stroke-dasharray:5 4 rm["<b>dl rm / dl kill</b>"] --> del["<b>devpod delete</b><br/>finds containers only by<br/>dev.containers.id=#lt;uid#gt;"] up["<b>dl up</b><br/>inside the launch lock"] --> sweep del -->|ok, failed or timed out| sweep["<b>orphan_containers::sweep</b><br/>skip if devpod holds a record of the id<br/>list compose containers by label<br/>keep those whose working_dir or<br/>config_files is in devpod's dir for the id<br/>or in dl's clone of the id"] sweep -->|docker rm -f| out["<b>stderr</b><br/>Removed containers #lt;id#gt; left behind: ..."] sweep --> next["<b>devpod up</b> / delete the clone"] class sweep,out changed class del,next outsideBlue boxes are new code. Dashed boxes are code that this PR does not change.
How I checked it
The e2e test
rm_removes_the_leftovers_once_devpods_delete_drops_its_recordfails without the fix:A deliberate mutant (the "devpod has no record" check inverted) makes
rm_removes_the_containers_devpod_left_behindfail. Each new test passes on this branch and fails when its production code is reverted.Other new tests cover:
dl killwith a dropped record, a delete killed at its deadline, a match byworking_diralone, adocker psthat fails, a workspace id that only starts the same (foo/foo-bar), a local folder, a record that devpod still holds, a launch at a parent directory of clones, and the output text.Commands, run in the pixi env from
rust/:cargo fmt --checkandcargo clippy --locked --all-targets -- -D warnings: clean.cargo test --workspace --no-fail-fast: all pass except 5 tests that also fail onorigin/mainon this host. These are 4 core tests aboutpushed_by_url, a remote that cannot be reached and prune on fetch, and the e2ea_commit_pushed_by_url_does_not_stop_rm.pixi run pytest: 839 passed, 7 skipped.prek run --from-ref origin/main --to-ref HEAD: all hooks pass.I did not run
scripts/public-api-snapshots.shon the last commits, because it needs the pinned nightly that this host does not have. I editedpublic-api.api.txtby hand, and thepublic_api_snapshotscargo tests pass. CI runs the full check.Merge danger
Door: two-way. The change adds a step and new output lines. It changes no file format and no stored data. A revert removes it.
Blast radius: medium. The flow runs
docker rm --force. If a match is wrong, dl removes a container that belongs to some other work. Three guards limit this: dl acts only when devpod has no record of the id in any context, it matches only on compose labels that point inside devpod's directory or dl's own clone for that id, and it names each container it removes.Review notes
dl killlooks for running containers bylabel=com.docker.compose.project=<workspace id>. devpod 0.26 names the project by the uid, sokillalways printsNothing on this host is holding workspace <id>.This PR does not fix that.<project>_defaultnetworks of the old projects are not removed.dl upstill fails with the name conflict, because devpod holds a record of the id. Adl rmremoves the containers.dl --purgedoes not run the flow. The next launch at that path removes the leftovers.🤖 Generated with Claude Code
Summary by Sourcery
Clean up compose containers left behind by forgotten workspaces before subsequent lifecycle operations or launches.
Bug Fixes:
rm,kill, or launch, preventing fixed container names from blocking future launches.Enhancements:
Documentation:
Tests:
Chores:
Review fixes
Fixes for the review, one commit per finding:
record_presenceanswers Held / Absent / Unknown; only a not-found is Absent, and the sweep runs on Absent alone.docker rmnamed every container as not removed: 84373a2. The sweep lists them again and names only those still there.managed_clonechecked only a prefix: fdcce24. Delete and launch share one<owner>/<repo>/<id>shape check.CHANGELOG entry updated: f7b97a2. The layout guard fix for a finding-2 test: eca5810. No public-api change (all new items are
pub(crate)).