fix: correctness fixes and verified cleanups from full-codebase audit - #3
Merged
Conversation
Correctness fixes (all confirmed by independent verification):
- vm console printed wrong endpoints ("590%d": 59010 for display 10, "590-1"/
"ssh -p 0" for the disabled defaults); derive from the same vncCol/sshCol
helpers vm list uses
- vm create/run/clone with an existing --name silently truncated the live
disk overlay and orphaned its qemu; scaffoldVM now refuses an existing record
- vm rm raced concurrent lifecycle ops (the one handler not under the per-VM
flock); wrap terminate/teardown/RemoveAll in withVMLock, tolerating a
missing dir
- image pull ignored --force for OCI refs and re-downloaded multi-GiB
artifacts already in the store; mirror the URL path's idempotency check
Reuse/altitude/consistency:
- setVNCPassword: thread ctx and replace the hand-rolled dial-retry loop with
utils.WaitFor; InjectConfig forward-path subprocesses use CommandContext
(cleanup stays plain so it survives cancellation)
- isFileHeld via utils.FindVMMByCmdline (one /proc walk) instead of scanning
every process's fd table 10x/s during nbd disconnect
- teardownNet and disconnectNBD warn on best-effort failures instead of
swallowing them (leaked TAP/netns and held-qcow2 now leave a trail)
- --state-dir registered once on the root command (was duplicated per-tree
with drifted help); qemu.IsQcow2NVRAM is the single source of the NVRAM
format rule (was duplicated in launch args + snapshot selection)
- extract scaffoldVM (create/clone shared block); hoist the OS-independent
prepareNet head over per-OS provisionNet; drop applyNet's derivable param;
utils.EnsureDirs; findFreeNBD/isFileHeld renames; cmp.Or for netCol
CMGS
added a commit
that referenced
this pull request
Jul 3, 2026
…CLI (#3) Correctness fixes (all confirmed by independent verification): - vm console printed wrong endpoints ("590%d": 59010 for display 10, "590-1"/ "ssh -p 0" for the disabled defaults); derive from the same vncCol/sshCol helpers vm list uses - vm create/run/clone with an existing --name silently truncated the live disk overlay and orphaned its qemu; scaffoldVM now refuses an existing record - vm rm raced concurrent lifecycle ops (the one handler not under the per-VM flock); wrap terminate/teardown/RemoveAll in withVMLock, tolerating a missing dir - image pull ignored --force for OCI refs and re-downloaded multi-GiB artifacts already in the store; mirror the URL path's idempotency check Reuse/altitude/consistency: - setVNCPassword: thread ctx and replace the hand-rolled dial-retry loop with utils.WaitFor; InjectConfig forward-path subprocesses use CommandContext (cleanup stays plain so it survives cancellation) - isFileHeld via utils.FindVMMByCmdline (one /proc walk) instead of scanning every process's fd table 10x/s during nbd disconnect - teardownNet and disconnectNBD warn on best-effort failures instead of swallowing them (leaked TAP/netns and held-qcow2 now leave a trail) - --state-dir registered once on the root command (was duplicated per-tree with drifted help); qemu.IsQcow2NVRAM is the single source of the NVRAM format rule (was duplicated in launch args + snapshot selection) - extract scaffoldVM (create/clone shared block); hoist the OS-independent prepareNet head over per-OS provisionNet; drop applyNet's derivable param; utils.EnsureDirs; findFreeNBD/isFileHeld renames; cmp.Or for netCol (cherry picked from commit 8bed8e5)
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
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.
Summary
Bug fixes (user-visible)
vm consoleprinted wrong endpoints. The VNC line used590%d(so display 10 rendered as59010, not5910) and the disabled defaults produced590-1/ssh -p 0. It now derives the VNC endpoint and SSH command from the samevncCol/sshColhelpersvm listuses, printing-when either is disabled.vm create/run/clonewith an existing--namesilently clobbered a live VM. A second call under the same name truncated the running VM's disk overlay and orphaned its qemu.scaffoldVMnow refuses when a record already exists (vm "<name>" already exists; rm it first or pick another --name).vm rmraced concurrent lifecycle ops. It was the one handler not taking the per-VM flock, so a concurrentstartcould relaunch qemu betweenterminateandRemoveAll.rmnow runs terminate/teardown/RemoveAllunderwithVMLock(tolerating a missing dir).image pullignored--forcefor OCI/ghcr refs. It re-downloaded multi-GiB artifacts already in the store. It now mirrors the URL path's idempotency check and skips theorasdownload when the ref is present (unless--force).Hardening
teardownNetanddisconnectNBDnow warn on best-effort failures instead of swallowing them, so a leaked TAP/netns or a still-held qcow2 leaves a diagnostic trail.setVNCPasswordand theInjectConfigforward path takectx(dial-retry viautils.WaitFor; subprocesses viaCommandContext). NBD cleanup deliberately stays on plainexec.Commandso it still runs after cancellation.Refactors (no behavior change)
isFileHeldusesutils.FindVMMByCmdline(one/procwalk) instead of scanning every process's fd table 10x/s during NBD disconnect.--state-dirregistered once on the root command (was duplicated per-subtree with drifted help text);qemu.IsQcow2NVRAMis the single source of the NVRAM-format rule (was duplicated across launch args and snapshot selection).scaffoldVM(shared create/clone block); hoisted the OS-independentprepareNethead over the per-OSprovisionNet; droppedapplyNet's derivable param;utils.EnsureDirs;findFreeNBD/isFileHeldrenames;cmp.Orfor the net column.Validation
gofumpt -l .— cleango build ./...andGOOS=linux go build ./...— okGOOS=darwin golangci-lint run ./...andGOOS=linux golangci-lint run ./...— 0 issues eachgo test -race -count=1 ./...— pass--state-dir:vm consolenow printsVNC 127.0.0.1:5910 SSH: ssh -p 2222 cocoon@localhost(and-/-when disabled); the duplicate-name guard refusescreate --name <existing>and leaves the record dir untouched (no truncated overlay);vm rmprints and exits 0 for a missing name and terminates + removes the dir under the flock for an existing one.🤖 Generated with Claude Code