Keep devlaunch's own copy of the volume names, and reclaim from it - #516
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideThe PR adds a cache-backed, provenance-preserving copy of devpod’s substituted volume names at completed-up boundaries, then extends prune to safely reclaim those volumes only after devpod forgets the workspace, with liveness revalidation, retryable refusal handling, delete integration, structural guards, tests, and documentation. Sequence diagram for preserving volume names during upsequenceDiagram
participant User
participant Launch as launch::up_under_stage
participant Devpod
participant KeptCopies as flows::kept_copies
participant Provision
User->>Launch: workspace_up()
Launch->>Devpod: devpod up
Devpod-->>Launch: completed up
Launch->>KeptCopies: keep(workspace_id, devpod_home)
KeptCopies->>Devpod: read workspace_result.json
KeptCopies-->>Launch: copy written atomically
Launch->>Provision: provision_tools()
Provision-->>Launch: result
Sequence diagram for reclaiming forgotten workspace volumessequenceDiagram
participant User
participant Prune as dl --prune
participant Devpod
participant KeptCopies as flows::kept_copies
participant Docker
User->>Prune: prune_plan()
Prune->>KeptCopies: copied()
KeptCopies-->>Prune: workspace ids and volume names
Prune->>Devpod: devpod list
Devpod-->>Prune: live workspace ids
Prune->>Prune: exclude listed workspaces
User->>Prune: confirm and run
Prune->>Devpod: devpod list
Devpod-->>Prune: live workspace ids
Prune->>Docker: volume rm --force names
alt Removed
Docker-->>Prune: success
Prune->>KeptCopies: forget(workspace_id)
else Refused
Docker-->>Prune: volume is in use
Prune-->>User: report refusal, keep copy for retry
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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:
|
Owner
Author
|
APPROVE (fresh-context review by the orchestrating session, which did not write this; GitHub refuses a formal approval from the author's own account, so it lands as a comment). Four hazards hunted, none survived:
On the one unilateral call, widening Non-blocking, recorded rather than asked for: |
A workspace deleted by a bare `devpod delete` outside `dl` leaves its two named volumes standing and takes the only record of their names with it. devlaunch#325 reads those names out of devpod's `workspace_result.json` at delete time, which closed the leak for every delete that goes through `dl` and did nothing for the ones that do not. So `dl` now takes a second read of the same document at the tail of every completed `up`, before the provisioning pass, and keeps it as a per workspace file under its own cache beside the tool verdict markers. `dl --prune` reclaims from those copies: the domain is the set of copies rather than a clone walk, and the one precondition per copy is that no workspace `devpod list` returns carries that id. Nothing is synthesized. Every name that reaches `docker volume rm` still came out of a substitution devpod performed and wrote down, at whichever of the two reads, and `tests/volume_names.rs` holds the crate to that by construction: one module builds a name, one call site removes one. A copy can be wrong in exactly two ways and neither is caught by trust. An already gone volume is a `--force` no-op; a held one is a docker refusal, reported, with the copy kept so the retry survives. A copy is dropped once, on proof: a removal that came back removed for a workspace devpod does not list. The 39 orphaned volumes measured on the reference host stay. No route reaches them that is not the pattern devlaunch#451 refuses. Closes #456.
`docs/cleanup.md`'s "the `--prune` half of the pair still frees no volume at all" is rewritten rather than deleted: prune still deletes no workspace, and because it deletes none, the volumes it reclaims belong to workspaces something else already deleted. The new section beside it says where the copy lives, what makes it safe to be wrong, and that the 39 measured orphans stay. `docs/development.md` gains the sentence the scratch-cache convention needs: a scratch run that creates a workspace writes its copy into the cache it then throws away. `public-api.rest.txt` is hand-edited. This container has neither prerequisite `scripts/public-api-snapshots.sh` needs, which docs/development.md names as exactly this case: provisional until CI's public-api job agrees. Nothing promised moved, so `public-api.api.txt` is untouched.
Three things this container could not check for itself, all of them named in docs/development.md as exactly that case. `SweepOccasion` sorts *after* `StopOutcome`, not before: ordering is not a partition property, so nothing local catches a block written in the wrong position and only CI's regeneration can say. Taken from its diff verbatim. `public-api.api.txt` moves too, and it is a change to the **promised** surface rather than a routine one: `api::workspace_delete` gains the copy store as its sixth parameter, because a delete that sweeps a workspace's volumes is also the moment its copy is provably pointless. One re-exported declaration, and its only caller in this repository is `dl`. And codespell prefers "unparsable".
blooop
force-pushed
the
wayfinder/devlaunch-456
branch
from
August 29, 2026 19:35
1be803a to
4f4c759
Compare
This was referenced Aug 29, 2026
blooop
added a commit
that referenced
this pull request
Aug 29, 2026
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 <ws> 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.
blooop
added a commit
that referenced
this pull request
Aug 29, 2026
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 <ws> 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.
blooop
added a commit
that referenced
this pull request
Aug 29, 2026
`workspace_remove` now takes the volume-copy store #516 added to the delete and passes it straight through, so the fold keeps the reclamation rather than dropping it: a removal that came back removed still forgets the copy that named the volumes docker has just taken away. `api` re-exports `KeptCopies` with it, because a parameter type outside the promise is a promised call nobody outside can make. Also: the repo_manager import list is the union of both sides, `main`'s new `FetchRepoError` beside this branch's `TreeSweep`; one unit test from #516 follows `DeleteOutcome` to its new name; and the CHANGELOG entry sits under the current `[Unreleased]`, not inside the 0.25.0 section the release cut renamed underneath it.
blooop
added a commit
that referenced
this pull request
Aug 29, 2026
No nightly toolchain on this host, so the generator could not be run here: CI's public-api job regenerates and diffs, and these are the two rows it named. `api::LifecycleNotice::VolumesNotRemoved::occasion` is a merge casualty rather than a stale file. #516 added the field while this branch was moving the whole notice vocabulary from the rest file into the promise, so the row was added to a block that no longer existed and lost on the way across. The second is the re-export's own cost: promising `KeptCopies` makes the generator render its inherent and derived impls a second time, at the canonical path, beside the other promised types' impls.
blooop
added a commit
that referenced
this pull request
Aug 29, 2026
As pushed, `public-api.api.txt` named `agent_worktrees::Standing` in `RemovalRefused` and did not promise it: the type has no struct or impl rows in that file at all. A consumer holding only `api` got a struct with a field whose type it could not name, which is devlaunch#531's gap and would have been its third instance after #427 and #516. Promoting it honestly was the other option and it is not small. `Standing` reaches `StandingSite`, `Reason`, `Place`, `Blank`, `Subject` and `NonEmpty<Loss>`, and `agent_worktrees` has over three hundred rows in the binary-surface snapshot. That is most of a module's internal vocabulary arriving in the one tier whose worth is being small and stable. So the promoted shape was either incomplete or far too wide, and rendering at the seam is the only option that is both complete and narrow. It is the move the `--ls --json` payload already makes for the wire, at the same boundary and for the same reason. `RemovalRefused` now carries a `RemovalGrounds`, which is made of `String`: `WouldLose`, `CouldNotTell`, or `BothAtOnce`. Three arms rather than two options, because a standing is non-empty and every reason in it answers one of the two, so "neither" cannot happen -- and both render sites carried a fourth arm apologising for being unreachable, which this deletes rather than comments. `BothAtOnce` is what keeps #446 true across the seam: a refusal still never picks one of two true things to say. Nothing inside `flows` changed. `Standing` is exactly as it was, the domain type still carries the whole standing, and the conversion is a private free function at the boundary rather than a method -- a public constructor taking a `Standing` would put it straight back into the promised tier's signatures. Also removes `Standing::any_unproved`, which this branch added and nothing ever called. `Standing` is in the residual, so an uncalled reader there is rows a consumer can bind to for nothing.
This was referenced Aug 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds #456, the decision #451 settled: keep devlaunch's own copy of the two
substituted volume names at the end of a completed
up, and reclaim from thatcopy the volumes of a workspace devpod has forgotten.
The existing 39 orphaned volumes are out of scope by decision and stay. No
route reaches them that is not the pattern #451 refuses.
The five seams, as built
1. The copy store is a new module,
flows::kept_copies, with a per workspacemarker under
<cache>/workspace-copies/<id>.jsonbeside the tool verdict markers.Three verbs, and the module doc argues each:
keep(write on a completedup,and it does the read itself, so there is one parse of one document),
volumes(read one,
Option<NonEmpty<String>>, no empty-list state) withcopiedoverthe directory for the enumeration, and
forget(drop on proof). Modelled onprovision::verdict_cachedown to the atomic write, and the one place it divergesfrom that module's "nothing ever deletes a marker" carries its reason in the doc.
The parse moved here too:
Substitutions,parse_substitutionsandvolume_namesused to live inflows::lifecycle. Now the live read at delete timeand the kept copy's read share one parser and agree by construction rather than by
two implementations being written alike.
The marker carries the image reference alongside the names, from the same read
at the same moment (
ContainerDetails.Config.ImageandMergedConfig.imagekeptapart), per #450's breadcrumb, so #458 has one document and no second migration.
Nothing here removes an image.
2. The write site is the tail of a successful
up, before the provisioningpass, on both arms of
launch::up_under_stage: afterdevpod upreturns, and onthe arm where a sibling's
upwon the race (#452, since that sibling may not havebeen
dl). Before rather than after because provisioning can fail and take thelaunch with it while the container and its volumes stand.
3. The plan is a second enumeration in
prune_plan, over the set of copiesrather than the clone walk (#445's lesson: a copy whose clone was deleted by hand
names volumes no clone-shaped walk reaches). Names ride on each
ReclaimableVolumesrecord, never as a plan-wide list. Precondition per copy: noworkspace
devpod listreturns carries that id.4. The act re-asks that precondition under the second
devpod listthe actingpass already pays for, then goes through
sweep_volumes, the same removal thedelete path uses.
Removeddrops the copy;Refusedkeeps it so the retrysurvives; no docker is silent.
dl <ws> rmdrops the copy on the same proof, so adelete that already swept does not leave the next prune reporting a reclaim.
5. The binary boundary:
a_prune_reclaims_the_volumes_of_a_workspace_devpod_forgot_and_drops_the_copyis the whole regression in one run, with docker's argv as the assertion.
Provenance is on the occasion, not the name
SweepOccasionis two arms,DevpodResultandKeptCopy, and there is no third,so adding a pattern arm later is a compile error at every match. There is no
constructor for an inferred name to reach, which is what #451 asked for over a
Provenancefield with aPatternarm.devlaunch-core/tests/volume_names.rsis the by-construction guard #451 asked for,in
tests/devpod_layout.rs's shape: the two name templates are spelled in onemodule and nowhere else, and
docker::remove_volumesis reachable from one callsite. Both halves are two-sided, so the guard cannot pass by the thing it guards
having vanished.
Tests
New: 15 in
flows::kept_copies, 4 inflows::launch, 9 inflows::lifecycle,3 in
tests/volume_names.rs. Each was red before its seam existed.Two things to know when reviewing
The promised surface moves by one declaration, deliberately.
api::workspace_deletegains the copy store as its sixth parameter, because adelete that sweeps a workspace's volumes is also the moment that workspace's
copy is provably pointless, and dropping it there is what keeps the next
--prunefrom reporting a reclaim of volumes that already went. It is the onlyrow in
public-api.api.txtthat changed, and its only caller in this repositoryis
dl(Decide: wf links devlaunch-core vs keeps the wire seam — prototype before cutover #266:wflinks none of this;dl --ls --jsonis its contract).Both snapshots were hand-edited, since this container has neither prerequisite
scripts/public-api-snapshots.shneeds, and CI's first run caught the one thingno local check can: ordering is not a partition property, and
SweepOccasionsorts after
StopOutcomerather than before. That is taken from the job's ownregenerated diff, verbatim.
A volume refusal does not change
--prune's exit code. It is reported andthe run is otherwise finished, which is the delete path's precedent: the
workspace was already gone, and the copy is kept so the retry stays possible.
Docs:
docs/cleanup.mdgains the mechanism section and its "the--prunehalf ofthe pair still frees no volume at all" paragraph is rewritten rather than deleted,
as the ticket asks.
docs/development.mdgains the scratch-cache sentence.README's two prune rows and the CHANGELOG
[Unreleased]follow.Closes #456. Unblocks #458.
🤖 Generated with Claude Code
Summary by Sourcery
Preserve devpod’s recorded volume names and use them to safely reclaim volumes from forgotten workspaces during
dl --prune.New Features:
dl --pruneto reclaim recorded volumes for workspaces no longer returned by devpod, while reporting reclaimed and retained volumes.Bug Fixes:
Enhancements:
Documentation:
dl --prune.Tests:
Chores: