feat: vm stop/delete --all for bulk cleanup#12
Conversation
ember vm stop --all # stop all running VMs ember vm stop --all --force # SIGKILL all running VMs ember vm delete --all --force # stop + delete every VM Useful for cleanup and for ending all VMs (including non-pool control agent VMs that pool destroy doesn't touch). Co-Authored-By: Claude <noreply@anthropic.com>
aljoscha
left a comment
aljoscha
left a comment
There was a problem hiding this comment.
Nice, clean feature — reuses existing logic well and the CLI ergonomics are solid. A few things to address before merge, mostly around project conventions and edge-case UX.
Also: the PR mentions 26 unit tests pass, but there's no test coverage for the new --all paths. Please add an integration test that exercises the bulk stop/delete flow end-to-end (create multiple VMs, stop --all, verify states, delete --all, verify cleanup).
| return stop_all(args.force, state_dir); | ||
| } | ||
|
|
||
| let name = args.name.as_deref().unwrap(); |
There was a problem hiding this comment.
Project convention (CLAUDE.md): "Prefer explicit error handling. Use ? for propagation, not .unwrap()." Clap's required_unless_present prevents this from panicking in practice, but an explicit error is more defensive and consistent:
let name = args.name.as_deref()
.context("VM name is required when --all is not used")?;Same applies to the identical .unwrap() in delete() below.
|
|
||
| println!("Stopping {} VMs...", targets.len()); | ||
| for metadata in &targets { | ||
| let stop_args = StopArgs { |
There was a problem hiding this comment.
Constructing a StopArgs just to call back into stop() is a bit roundabout. Consider extracting the single-VM logic into a helper like stop_one(name: &str, force: bool, state_dir: &Path) that both stop() and stop_all() can call directly. Same pattern applies to delete_all below.
Not blocking, but it would simplify both call sites and avoid synthesizing args structs.
| } | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
No summary line after the loop completes. When some VMs fail (warnings printed to stderr), the user has no way to tell how many succeeded at a glance. Consider tracking success/failure counts and printing a final line:
println!("Stopped {ok} of {} VMs.", targets.len());Same applies to delete_all.
|
|
||
| println!("Deleting {} VMs...", vms.len()); | ||
| for metadata in &vms { | ||
| let delete_args = DeleteArgs { |
There was a problem hiding this comment.
Ordering issue with fork hierarchies: vm::list() returns VMs in arbitrary order. If a parent VM is visited before its fork child, delete() with --force will cascade-delete the child via force_delete_vm. When the loop later reaches that already-deleted child, vm::load fails and the user sees a spurious "warning: failed to delete 'child'" message.
This works correctly (no data loss), but the warnings are confusing. Two options:
- Sort VMs children-first (leaves before parents) so children are deleted before their parents.
- Collect successfully-deleted names in a
HashSetand skip VMs that were already cascade-deleted.
2 participants
Summary
Add
--allflag toember vm stopandember vm deletefor bulk operations.ember vm stop --all— stops all running VMsember vm delete --all— deletes all VMs (requires--forceif any are running)namebecomes optional when--allis used (required_unless_present)--forceis usedFiles changed
src/cli/vm.rs--allflag onStopArgs/DeleteArgs,stop_all()/delete_all()functionsTest plan
cargo buildclean,cargo clippycleanember vm stop --allstops all running VMsember vm delete --all --forcecleans up everything🤖 Generated with Claude Code