Hygiene round: context discipline on cleanup paths, comment budget, wg.Go - #42
Merged
Conversation
ensureNetnsLoopback and launchCmd already document the netns loopback and `ip netns exec` wrapping on their own godoc; repeating it at the launch call site is the same fact twice.
Every detached cleanup manufactured a fresh root context, so it dropped what the caller's context carried (core/log reads the tracing id and the gRPC peer off ctx when it reports an error) and hid which call sites are deliberately uncancelable. context.WithoutCancel keeps the detachment and the values. Reachable failures closed: - cleanupFailedVM now takes the caller ctx. create/run/clone rollback runs on a bounded WithoutCancel copy, so a Ctrl-C mid-create still reaps QEMU, the VNC proxy and the netns instead of losing the caller's log correlation. - nbdConnection.disconnect, cleanupNBDMount and connectFreeNBD's two failure paths do the same for the qemu-nbd lease: a lost disconnect leaves a server pinning the OpenCore qcow2 and the next inject finds no free /dev/nbd. - vm rm reaps stray helpers under the command ctx, matching the sibling branch that already terminates QEMU and tears down networking under it. The 30 s detached window made Ctrl-C ignored on exactly one of the two rm paths. - withVMLock and withNBDLease release under WithoutCancel(ctx), not a raw Background and not the live ctx. Passing the live ctx would strand the in-process token once the caller cancels, blocking every later vm command on that dir; TestWithVMLockUnlocksAfterCallerCancel deadlocks the second acquire when release honors a cancelled caller ctx. flock.Unlock ignores its ctx today, so these two rows pin the contract rather than a live failure. - scaffoldVM resolves the command ctx before deriving its reset budget.
Go 1.25 wg.Go replaces the hand-rolled Add/Done + go func pairs in the VM lock and NBD lease concurrency tests.
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.
Adjudicated hygiene findings from the cocoon-macos style walkthrough. No new mechanisms, no restructuring beyond the three rows.
Commits
review: drop call-site comments duplicated from the callee godoc—cmd/vm/lifecycle.go+2 −2, comments 13 → 11.ensureNetnsLoopbackandlaunchCmdalready document the netns loopback and theip netns execwrapping on their own godoc. The launch call site repeated both facts.fix: thread the caller context through cleanup paths— 5 files, +51 −29, comments unchanged (one reworded inqemu/inject.go, 9 → 9).Detached cleanup paths built a fresh root context instead of deriving one from the caller, dropping whatever the caller's context carried and hiding which call sites are deliberately uncancelable. Every one of them now uses
context.WithoutCancel(ctx), with the timeout kept where there was one.cleanupFailedVMtakes the caller ctx;create/run/clonerollback runs on a bounded detached copy.nbdConnection.disconnectandcleanupNBDMountgain a ctx parameter;connectFreeNBD's two failure paths derive their cleanup ctx from the caller. A lostqemu-nbd --disconnectleaves a server pinning the OpenCore qcow2 and the next inject finds no free/dev/nbd.vm rmreaps stray helpers under the command ctx, matching the sibling branch that already terminates QEMU and tears down networking under it. The 30 s detached window made Ctrl-C ignored on exactly one of the two rm paths.withVMLockandwithNBDLeaserelease underWithoutCancel(ctx)— not a rawBackground, and not the live ctx, which would strand the in-process token once the caller cancels.scaffoldVMresolves the command ctx before deriving its reset budget.TestWithVMLockUnlocksAfterCallerCancelpins the release contract: it deadlocks the second acquire when release honors a cancelled caller ctx (verified by mutation).flock.Unlockignores its ctx parameter today, so the two lock rows are a contract fix rather than a live failure.review: use wg.Go for the test goroutine pairs— 2 files, +6 −12, comments unchanged (0 in both).Go 1.25
wg.Goreplaces the hand-rolledAdd/Done+go funcpairs in the VM lock and NBD lease concurrency tests.Comment delta
Net −2 lines across the branch; no file's comment count rose.
Gates
All
GOWORK=offfrom the worktree root.