[codex] fix: make macOS VM creation transactional - #34
Conversation
CMGS
left a comment
CMGS
left a comment
There was a problem hiding this comment.
the transactional design is good: lock moved to <parent>/.locks/<name>.lock via flock.NewTransient so rm can't unlink the inode mid-hold and split mutual exclusion; cleanupFailedVM reaps helpers/net/qemu on every create/run failure with an uncanceled bounded context; launch now verifies the pid before committing; start adopts a qemu that daemonized before its pid was saved. combined-branch gates: lint (linux+darwin) 0, go test, gofmt all green.
stacked-draft caveat: this carries the #30 and #31 commits, so 3 of the 4 asl findings (qemu/inject.go:29, qemu/inject_test.go:116, cmd/vm/handler_test.go:73) and the inject comment nits belong to those PRs — already flagged there, they'll clear when you rebase onto master after #30/#31 merge. the items below are the ones new to c4d89af.
| // cleanupFailedVM makes create/run transactional. It uses an uncanceled, | ||
| // bounded context so SIGTERM-driven command cancellation still reaps helpers, | ||
| // networking and any QEMU process started before the record was committed. | ||
| func cleanupFailedVM(cmd *cobra.Command, dir string, r *record) error { |
There was a problem hiding this comment.
asl (both GOOS): cleanupFailedVM is a standalone func sitting between the Handler method set (create ... launch). move it below the Handler methods with the other package funcs.
| return nil | ||
| } | ||
|
|
||
| func cleanupQEMUForPath(ctx context.Context, path string) error { |
There was a problem hiding this comment.
| } | ||
|
|
||
| // withVMLock serializes concurrent lifecycle ops on one VM (vm.json is read-modify-write). | ||
| // withVMLock serializes concurrent lifecycle ops on one VM. The lock lives |
There was a problem hiding this comment.
comment budget: 3 lines → 1. the inode-split WHY is the keeper:
// lock lives outside the VM dir so rm can't unlink the inode a waiter still holds and split mutual exclusion.
| return dir, overlay, ovmfVars, digest, nil | ||
| } | ||
|
|
||
| // resetIncompleteVMDir removes state left before vm.json was committed. It |
There was a problem hiding this comment.
comment budget: 3 lines → 1, e.g. // remove pre-commit VM state; refuses a dir a live qemu still references.
| return utils.VerifyProcessCmdline(r.PID, qemuBinary, r.Disk) | ||
| } | ||
|
|
||
| // adoptRunningQEMU repairs a record whose launch was interrupted after QEMU |
There was a problem hiding this comment.
comment budget: 3 lines → 1, e.g. // adopt a qemu that daemonized before its pid was saved; >1 match is corruption, not a guess.
2 participants
Summary
startadopt a QEMU process when the CLI was interrupted before saving the PIDRoot cause
VM creation spans filesystem scaffolding, OpenCore/NBD injection, networking, QEMU daemonization, and record persistence. Cancellation or failure between those steps could leave partial state. The old lock file also lived inside the directory removed by
rm, allowing concurrent operations to lock different inodes.Impact
Failed or interrupted VM creation becomes retryable without manual cleanup, concurrent lifecycle commands remain serialized, and an already-running guest is not duplicated when its PID record was not committed.
Dependency
This is a stacked draft based on the changes proposed in #30 and #31. Those commits are included temporarily so the combined behavior can be tested. After #30 and #31 merge, this branch will be rebased onto
master, leaving only the transactional-create commit for review.Validation
GOWORK=off go test ./...make fmt-check vet lint