diff --git a/server/cmd/wrapper/main.go b/server/cmd/wrapper/main.go index 37b70e3a..7da559f0 100644 --- a/server/cmd/wrapper/main.go +++ b/server/cmd/wrapper/main.go @@ -76,6 +76,9 @@ func main() { startupCtx, cancelStartup := context.WithCancel(context.Background()) defer cancelStartup() + // Nothing else reaps the orphans we adopt as pid 1; start before we fork. + startReaper() + // /dev/shm: only mount when not running under Docker (Docker manages it). if os.Getenv("WITHDOCKER") == "" { _ = os.MkdirAll("/dev/shm", 0o1777) @@ -137,12 +140,12 @@ func main() { // supervisord — start in nodaemon mode so we own its lifecycle. // Without -n it forks and the parent exits with code 0, which would - // drop us out of supCmd.Wait() and the container would stop. + // drop us out of waitOwned(supCmd) and the container would stop. logf("starting supervisord") supCmd := exec.Command("supervisord", "-n", "-c", supervisorConf) supCmd.Stdout = os.Stdout supCmd.Stderr = os.Stderr - if err := supCmd.Start(); err != nil { + if err := startOwned(supCmd); err != nil { fatalf("supervisord start: %v", err) } // Install the shutdown goroutine now so it can clean up if a signal @@ -196,7 +199,7 @@ func main() { if err := prepareSnapshotStartPage(startupCtx, os.Getenv("INTERNAL_PORT")); err != nil { if errors.Is(err, context.Canceled) { logf("snapshot start page preparation canceled") - if err := supCmd.Wait(); err != nil { + if err := waitOwned(supCmd); err != nil { logf("supervisord exited: %v", err) } return @@ -217,7 +220,7 @@ func main() { forkIdentity, identityDeadline, ok := waitForForkIdentityIfEnabled(startupCtx, forkIdentityWait) if !ok { - if err := supCmd.Wait(); err != nil { + if err := waitOwned(supCmd); err != nil { logf("supervisord exited: %v", err) } return @@ -264,7 +267,7 @@ func main() { } // Block on supervisord; container exits when it does. - if err := supCmd.Wait(); err != nil { + if err := waitOwned(supCmd); err != nil { logf("supervisord exited: %v", err) } } diff --git a/server/cmd/wrapper/reap.go b/server/cmd/wrapper/reap.go new file mode 100644 index 00000000..f50487f8 --- /dev/null +++ b/server/cmd/wrapper/reap.go @@ -0,0 +1,152 @@ +package main + +import ( + "bytes" + "os" + "os/exec" + "os/signal" + "strconv" + "strings" + "sync" + "syscall" + "time" +) + +// The wrapper is pid 1 in both the container (ENTRYPOINT) and the unikernel +// (Kraftfile cmd), so any process whose parent dies first is reparented onto +// it — chromium's crashpad handlers are the common case, one per relaunch. +// Nothing waits on those, so each stays a zombie holding a pid slot until the +// instance dies, and every /proc walk on the host slows down with them. +// +// Reaping can't just be wait4(-1) on SIGCHLD: os/exec waits on a specific pid, +// and if the reaper collects one of our children first that Cmd.Wait fails +// with ECHILD and loses its exit status. So we track the commands we start and +// only collect pids we don't own. Ownership is keyed by command identity +// because Cmd.Wait frees the pid before releaseOwned runs, and a late release +// must not evict whatever took that pid next. +// +// Only commands whose exit status we act on need this — supervisord, whose +// exit ends the instance, and runStream, which runStreamFatal turns into a +// boot failure. The rest already throw their status away with `_ =`, so +// there's nothing for the reaper to take from them. +var owned = struct { + sync.Mutex + cmds map[int]*exec.Cmd +}{cmds: map[int]*exec.Cmd{}} + +// startOwned starts cmd and records it as ours to wait on. The lock is held +// across Start so a concurrent reap can't observe the child in the window +// between fork and registration. +func startOwned(cmd *exec.Cmd) error { + owned.Lock() + defer owned.Unlock() + if err := cmd.Start(); err != nil { + return err + } + owned.cmds[cmd.Process.Pid] = cmd + return nil +} + +// waitOwned waits on a command started by startOwned and releases its pid. +func waitOwned(cmd *exec.Cmd) error { + err := cmd.Wait() + releaseOwned(cmd) + return err +} + +// releaseOwned drops cmd's pid, but only while that pid still belongs to cmd. +func releaseOwned(cmd *exec.Cmd) { + pid := cmd.Process.Pid + owned.Lock() + if owned.cmds[pid] == cmd { + delete(owned.cmds, pid) + } + owned.Unlock() +} + +// runOwned is startOwned followed by waitOwned: the replacement for Cmd.Run. +func runOwned(cmd *exec.Cmd) error { + if err := startOwned(cmd); err != nil { + return err + } + return waitOwned(cmd) +} + +// startReaper drains adopted zombies for the life of the process. It does +// nothing when we aren't pid 1, since then orphans reparent elsewhere. +func startReaper() { + if os.Getpid() != 1 { + return + } + ch := make(chan os.Signal, 1) + signal.Notify(ch, syscall.SIGCHLD) + go func() { + // SIGCHLD coalesces, so a burst arriving mid-drain can leave + // stragglers. The ticker bounds how long one sits around. + tick := time.NewTicker(30 * time.Second) + defer tick.Stop() + for { + select { + case <-ch: + case <-tick.C: + } + reapOrphans() + } + }() +} + +func reapOrphans() { + for _, pid := range zombieChildren() { + owned.Lock() + if owned.cmds[pid] == nil { + var ws syscall.WaitStatus + // A pid that slipped away between the scan and here just comes + // back around on the next wakeup. + _, _ = syscall.Wait4(pid, &ws, syscall.WNOHANG, nil) + } + owned.Unlock() + } +} + +// zombieChildren returns the pids of our children currently in Z state. +func zombieChildren() []int { + entries, err := os.ReadDir("/proc") + if err != nil { + return nil + } + self := os.Getpid() + var zombies []int + for _, e := range entries { + pid, err := strconv.Atoi(e.Name()) + if err != nil { + continue + } + if state, ppid, ok := procStat(pid); ok && state == 'Z' && ppid == self { + zombies = append(zombies, pid) + } + } + return zombies +} + +// procStat reads a process's state and parent pid from /proc//stat. +// comm is the second field, parenthesized, and may itself contain spaces and +// parens, so the fixed-width fields are read from after its closing paren. +func procStat(pid int) (state byte, ppid int, ok bool) { + b, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/stat") + if err != nil { + return 0, 0, false + } + commEnd := bytes.LastIndexByte(b, ')') + if commEnd < 0 || commEnd+2 >= len(b) { + return 0, 0, false + } + fields := strings.Fields(string(b[commEnd+2:])) + if len(fields) < 2 { + return 0, 0, false + } + parent, err := strconv.Atoi(fields[1]) + if err != nil { + return 0, 0, false + } + return fields[0][0], parent, true +} diff --git a/server/cmd/wrapper/reap_test.go b/server/cmd/wrapper/reap_test.go new file mode 100644 index 00000000..51facffb --- /dev/null +++ b/server/cmd/wrapper/reap_test.go @@ -0,0 +1,100 @@ +package main + +import ( + "os" + "os/exec" + "testing" + "time" +) + +func waitForZombie(t *testing.T, pid int) { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + if state, ppid, ok := procStat(pid); ok && state == 'Z' && ppid == os.Getpid() { + return + } + time.Sleep(5 * time.Millisecond) + } + t.Fatalf("pid %d never became a zombie child", pid) +} + +func isZombieChild(pid int) bool { + state, ppid, ok := procStat(pid) + return ok && state == 'Z' && ppid == os.Getpid() +} + +func TestReapOrphansCollectsUnownedZombie(t *testing.T) { + cmd := exec.Command("true") + if err := cmd.Start(); err != nil { + t.Fatalf("start: %v", err) + } + pid := cmd.Process.Pid + waitForZombie(t, pid) + + reapOrphans() + + if isZombieChild(pid) { + t.Fatalf("pid %d still a zombie after reapOrphans", pid) + } +} + +func TestReapOrphansLeavesOwnedZombie(t *testing.T) { + cmd := exec.Command("true") + if err := startOwned(cmd); err != nil { + t.Fatalf("startOwned: %v", err) + } + pid := cmd.Process.Pid + waitForZombie(t, pid) + + reapOrphans() + + if !isZombieChild(pid) { + t.Fatalf("reapOrphans collected owned pid %d", pid) + } + if err := waitOwned(cmd); err != nil { + t.Fatalf("waitOwned after reapOrphans: %v", err) + } + if owned.cmds[pid] != nil { + t.Fatalf("pid %d still tracked after waitOwned", pid) + } +} + +func TestReleaseOwnedKeepsLaterHolderOfSamePid(t *testing.T) { + first := exec.Command("true") + if err := startOwned(first); err != nil { + t.Fatalf("startOwned: %v", err) + } + pid := first.Process.Pid + if err := waitOwned(first); err != nil { + t.Fatalf("waitOwned: %v", err) + } + + // Stand in for the kernel handing this pid to a later command. + second := exec.Command("true") + owned.Lock() + owned.cmds[pid] = second + owned.Unlock() + + releaseOwned(first) + + owned.Lock() + defer owned.Unlock() + if owned.cmds[pid] != second { + t.Fatal("a stale release evicted the current holder of the pid") + } + delete(owned.cmds, pid) +} + +func TestProcStatReadsSelf(t *testing.T) { + state, ppid, ok := procStat(os.Getpid()) + if !ok { + t.Fatal("procStat failed on self") + } + if state == 'Z' { + t.Fatalf("self reported as zombie") + } + if ppid != os.Getppid() { + t.Fatalf("ppid = %d, want %d", ppid, os.Getppid()) + } +} diff --git a/server/cmd/wrapper/supervisord.go b/server/cmd/wrapper/supervisord.go index 4672db47..a907b5bb 100644 --- a/server/cmd/wrapper/supervisord.go +++ b/server/cmd/wrapper/supervisord.go @@ -88,7 +88,7 @@ func tailFile(path string) { return } cmd.Stderr = nil - if err := cmd.Start(); err != nil { + if err := startOwned(cmd); err != nil { return } label := filepath.Base(path) @@ -97,13 +97,24 @@ func tailFile(path string) { for scanner.Scan() { fmt.Printf("[%s] %s\n", label, scanner.Text()) } + // A clean scan ends when tail closes its stdout, i.e. when it has exited. + // A scan that ends on an error (a log line past the 1MB cap, say) leaves + // tail running, so kill it rather than block here forever. + // + // Either way we have to collect it: the wrapper is pid 1 in both the + // container and the unikernel, so a tail nobody waits on stays a zombie + // for the life of the instance. + if scanner.Err() != nil { + _ = cmd.Process.Kill() + } + _ = waitOwned(cmd) } func runStream(label, name string, args ...string) error { cmd := exec.Command(name, args...) cmd.Stdout = prefixWriter{label: label, w: os.Stdout} cmd.Stderr = prefixWriter{label: label, w: os.Stderr} - return cmd.Run() + return runOwned(cmd) } // runStreamFatal is runStream + fatalf on non-zero exit. Use for scripts the