Skip to content

Commit a75442f

Browse files
committed
fix: address Copilot round-1 findings on orphan VMM PR
- utils/process_linux.go: slices.Sort the returned pids so callers get a deterministic smallest-pid choice (Copilot caught the /proc lexicographic ordering trap, e.g. "100" < "11"). - hypervisor/state.go: fail-closed when /proc scan errors after pidfile-based check fails; previously returned ErrNotRunning on inconclusive state, which could let start/delete proceed against a still-running VM. - hypervisor/stop.go: fail-closed in DeleteAll second-pass when /proc scan errors; previously dropped scanErr and risked re-introducing the orphan leak the PR is trying to fix. - utils/process_test.go: replace flaky "sleep marker 60" (sleep rejects non-numeric arg and exits immediately) with "sh -c 'sleep 60 && :' marker" (compound prevents sh tail-exec into sleep). Gate on runtime.GOOS == "linux".
1 parent f9d7a14 commit a75442f

4 files changed

Lines changed: 21 additions & 16 deletions

File tree

hypervisor/state.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,10 @@ func (b *Backend) WithRunningVM(ctx context.Context, rec *VMRecord, fn func(pid
2828
if utils.VerifyProcessCmdline(pid, b.Conf.BinaryName(), sockPath) {
2929
return fn(pid)
3030
}
31-
// Covers pidfile/socket cleaned up before VMM exited.
31+
// Covers pidfile/socket cleaned up before VMM exited. Fail-closed if scan errors so callers don't treat inconclusive state as ErrNotRunning.
3232
scanned, scanErr := utils.FindVMMByCmdline(b.Conf.BinaryName(), sockPath)
3333
if scanErr != nil {
34-
logger.Warnf(ctx, "scan /proc for VM %s: %v", rec.ID, scanErr)
34+
return fmt.Errorf("VM %s: pidfile-based check failed and /proc scan errored: %w", rec.ID, scanErr)
3535
}
3636
if len(scanned) == 0 {
3737
return ErrNotRunning

hypervisor/stop.go

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -75,14 +75,16 @@ func (b *Backend) DeleteAll(ctx context.Context, refs []string, force bool, stop
7575
}
7676
return fmt.Errorf("refuse delete: api socket %s still responsive (suspected orphan vmm; kill the vmm process then retry)", sockPath)
7777
}
78-
// Catches workers/siblings the pidfile-based stop didn't see.
79-
if scanned, _ := utils.FindVMMByCmdline(b.Conf.BinaryName(), sockPath); len(scanned) > 0 {
80-
for _, pid := range scanned {
81-
if termErr := utils.TerminateProcess(ctx, pid, b.Conf.BinaryName(), sockPath, b.Conf.TerminateGracePeriod()); termErr != nil {
82-
return fmt.Errorf("terminate orphan VMM pid=%d for VM %s: %w", pid, id, termErr)
83-
}
84-
log.WithFunc(b.Typ+".Delete").Warnf(ctx, "killed orphan VMM pid=%d for VM %s", pid, id)
78+
// Catches workers/siblings the pidfile-based stop didn't see; fail-closed on scan error so we never wipe rundir while VMM state is unknown.
79+
scanned, scanErr := utils.FindVMMByCmdline(b.Conf.BinaryName(), sockPath)
80+
if scanErr != nil {
81+
return fmt.Errorf("refuse delete: VM %s /proc scan errored: %w (resolve host issue and retry)", id, scanErr)
82+
}
83+
for _, pid := range scanned {
84+
if termErr := utils.TerminateProcess(ctx, pid, b.Conf.BinaryName(), sockPath, b.Conf.TerminateGracePeriod()); termErr != nil {
85+
return fmt.Errorf("terminate orphan VMM pid=%d for VM %s: %w", pid, id, termErr)
8586
}
87+
log.WithFunc(b.Typ+".Delete").Warnf(ctx, "killed orphan VMM pid=%d for VM %s", pid, id)
8688
}
8789
if rmErr := RemoveVMDirs(rec.RunDir, rec.LogDir); rmErr != nil {
8890
return fmt.Errorf("cleanup VM dirs: %w", rmErr)

utils/process_linux.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,12 @@ import (
66
"fmt"
77
"os"
88
"path/filepath"
9+
"slices"
910
"strconv"
1011
"strings"
1112
)
1213

13-
// FindVMMByCmdline returns pids whose argv[0] basename matches binaryName and args contain expectArg.
14+
// FindVMMByCmdline returns pids whose argv[0] basename matches binaryName and args contain expectArg, sorted numerically.
1415
func FindVMMByCmdline(binaryName, expectArg string) ([]int, error) {
1516
entries, err := os.ReadDir("/proc")
1617
if err != nil {
@@ -26,6 +27,7 @@ func FindVMMByCmdline(binaryName, expectArg string) ([]int, error) {
2627
pids = append(pids, pid)
2728
}
2829
}
30+
slices.Sort(pids)
2931
return pids, nil
3032
}
3133

utils/process_test.go

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"os"
66
"os/exec"
77
"path/filepath"
8+
"runtime"
89
"strconv"
910
"testing"
1011
"time"
@@ -246,12 +247,12 @@ func TestTerminateProcess_SIGTERMIgnored_FallsBackToKill(t *testing.T) {
246247
}
247248

248249
func TestFindVMMByCmdline(t *testing.T) {
249-
if _, err := os.Stat("/proc/self/cmdline"); err != nil {
250-
t.Skip("/proc not available")
250+
if runtime.GOOS != "linux" {
251+
t.Skip("FindVMMByCmdline scans /proc — linux only")
251252
}
252253
marker := "cocoon-find-marker-" + strconv.Itoa(os.Getpid())
253-
cmd := exec.Command("sleep", "60")
254-
cmd.Args = []string{"sleep", marker, "60"}
254+
// "sleep 60 && :" is a compound command so sh can't tail-exec into sleep and lose the marker arg.
255+
cmd := exec.Command("sh", "-c", "sleep 60 && :", marker)
255256
if err := cmd.Start(); err != nil {
256257
t.Fatalf("start: %v", err)
257258
}
@@ -263,7 +264,7 @@ func TestFindVMMByCmdline(t *testing.T) {
263264
// Poll briefly: cmdline is written by execve, so the parent may scan before /proc/<pid>/cmdline reflects argv.
264265
var pids []int
265266
for range 50 {
266-
got, err := FindVMMByCmdline("sleep", marker)
267+
got, err := FindVMMByCmdline("sh", marker)
267268
if err != nil {
268269
t.Fatalf("FindVMMByCmdline: %v", err)
269270
}
@@ -280,7 +281,7 @@ func TestFindVMMByCmdline(t *testing.T) {
280281
if got, _ := FindVMMByCmdline("definitely-no-such-binary", marker); len(got) != 0 {
281282
t.Errorf("wrong-binary scan matched: %v", got)
282283
}
283-
if got, _ := FindVMMByCmdline("sleep", "no-such-marker"); len(got) != 0 {
284+
if got, _ := FindVMMByCmdline("sh", "no-such-marker-"+marker); len(got) != 0 {
284285
t.Errorf("wrong-marker scan matched: %v", got)
285286
}
286287
}

0 commit comments

Comments
 (0)