diff --git a/plugin/appstore/reap.go b/plugin/appstore/reap.go index 75af70b..cb9ad2a 100644 --- a/plugin/appstore/reap.go +++ b/plugin/appstore/reap.go @@ -107,6 +107,63 @@ func hasArg(raw []byte, want string) bool { return false } +// killAppGroup SIGKILLs the process group a running app leads: the app and +// any process it started that did not move itself to a group of its own +// (setsid/setpgid). It is the app's exec.Cmd.Cancel, so it runs before +// Wait has reaped the app: the app's pid, which is also the group id, is +// still held (by the app, or its zombie) and cannot have been reused. When +// the group cannot be signalled it falls back to the app alone, which is +// what exec's default Cancel does. +func killAppGroup(p *os.Process) error { + if err := syscall.Kill(-p.Pid, syscall.SIGKILL); err == nil { + return nil + } + return p.Kill() +} + +// appStopGrace is how long a stopped app gets to run its own shutdown +// after SIGTERM before its process group is SIGKILLed. It fits inside the +// daemon's 5s plugin stop budget (pilotprotocol cmd/daemon +// pluginStopTimeout); apps are stopped in parallel. +const appStopGrace = 3 * time.Second + +// stopPollInterval is how often stopAppGroup checks whether the app has +// exited during its grace period. +const stopPollInterval = 20 * time.Millisecond + +// stopAppGroup is the app's exec.Cmd.Cancel. It asks the process group +// the app leads to terminate (SIGTERM), so the app runs its shutdown path: +// finish in-flight calls, close its database, remove its socket. If the +// app is still running after grace, the whole group is SIGKILLed +// (killAppGroup), so nothing the app started outlives it. +// +// It returns once the app has exited. Exec's Wait reaps the app +// concurrently; once it has, Signal reports os.ErrProcessDone. Until then +// the app's pid, which is also the group id, is held by the app or its +// zombie, so the group signals cannot reach a recycled pid. +func stopAppGroup(p *os.Process, grace time.Duration) error { + // exec can call Cancel after Wait has already reaped the app (ctx + // cancelled just as it exited). Then there is nothing to stop, and its + // pid must not be signalled as a group. + if err := p.Signal(syscall.Signal(0)); err != nil { + return err + } + if syscall.Kill(-p.Pid, syscall.SIGTERM) != nil { + // Not a group leader (or already gone): the app alone. + if err := p.Signal(syscall.SIGTERM); err != nil { + return err // os.ErrProcessDone: it already exited + } + } + deadline := time.Now().Add(grace) + for time.Now().Before(deadline) { + if p.Signal(syscall.Signal(0)) != nil { + return nil // exited and reaped + } + time.Sleep(stopPollInterval) + } + return killAppGroup(p) +} + // socketCheckInterval is how often a running app's socket is checked. const socketCheckInterval = 5 * time.Second @@ -126,6 +183,11 @@ func (s *supervisor) watchSocket(ctx context.Context, a *installedApp, pid int, return case <-t.C: } + if ctx.Err() != nil { + // Being stopped: the app removing its socket on SIGTERM is + // its normal shutdown, not a lost socket. + return + } _, err := os.Stat(a.SocketPath) switch { case err == nil: diff --git a/plugin/appstore/reap_test.go b/plugin/appstore/reap_test.go index d8517d3..1023aff 100644 --- a/plugin/appstore/reap_test.go +++ b/plugin/appstore/reap_test.go @@ -22,11 +22,19 @@ import ( // a real supervisor process. const fakeDaemonEnv = "APPSTORE_TEST_FAKE_DAEMON_DIR" +// fakeOrphanEnv switches the test binary into a stand-in for an orphaned +// app (startFakeApp) that runs until its stdin closes or it is killed. +const fakeOrphanEnv = "APPSTORE_TEST_FAKE_ORPHAN" + func TestMain(m *testing.M) { if dir := os.Getenv(fakeDaemonEnv); dir != "" { runFakeDaemon(dir) return } + if os.Getenv(fakeOrphanEnv) != "" { + _, _ = io.Copy(io.Discard, os.Stdin) + os.Exit(0) + } os.Exit(m.Run()) } @@ -71,15 +79,30 @@ func quietSupervisor() *supervisor { } // startFakeApp starts a long-lived process in its own process group whose -// argv carries the given strings, mimicking an orphaned app child. +// argv carries the given strings, mimicking an orphaned app child. It is +// this test binary (fakeOrphanEnv), blocked reading a pipe the test holds +// open: its argv is final once Start returns and it never forks. A shell +// looping over `sleep` was neither — each forked child carries the +// shell's argv until its exec, and macOS's /bin/sh re-execs bash — so +// reapStale could see a third instance, or miss one mid-exec. func startFakeApp(t *testing.T, argv ...string) *exec.Cmd { t.Helper() - cmd := exec.Command("/bin/sh", append([]string{"-c", "while :; do sleep 1; done"}, argv...)...) + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + cmd := exec.Command(os.Args[0], append([]string{"-test.run=^$"}, argv...)...) + cmd.Env = append(os.Environ(), fakeOrphanEnv+"=1") + cmd.Stdin = r cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} if err := cmd.Start(); err != nil { t.Fatalf("start: %v", err) } - t.Cleanup(func() { _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) }) + _ = r.Close() + t.Cleanup(func() { + _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + _ = w.Close() + }) return cmd } diff --git a/plugin/appstore/supervisor.go b/plugin/appstore/supervisor.go index e5c6733..dedd160 100644 --- a/plugin/appstore/supervisor.go +++ b/plugin/appstore/supervisor.go @@ -1037,6 +1037,14 @@ func (s *supervisor) spawn(ctx context.Context, a *installedApp) int { cmd.Stdout = os.Stderr cmd.Stderr = os.Stderr cmd.SysProcAttr = childSysProcAttr() // own process group → clean SIGTERM + // Stopping the app (daemon shutdown, uninstall, a rescan that replaces + // it) cancels ctx. exec's default Cancel SIGKILLs only the app's own pid, + // so every process the app had started kept running, reparented to + // launchd/init, with nothing left that would ever stop it, and the app + // itself never ran its shutdown path. SIGTERM the whole process group + // the app leads, give it appStopGrace to exit, then SIGKILL the group + // (stopAppGroup). + cmd.Cancel = func() error { return stopAppGroup(cmd.Process, appStopGrace) } if a.Sideloaded { // Signal sideload status to cap-aware children. Apps that // honour their declared grants (e.g. the wallet) can read @@ -1073,6 +1081,12 @@ func (s *supervisor) spawn(ctx context.Context, a *installedApp) int { go s.watchSocket(watchCtx, a, cmd.Process.Pid, socketCheckInterval) if err := cmd.Wait(); err != nil { + if ctx.Err() != nil && cmd.ProcessState != nil { + // Stopped by us (cmd.Cancel). Wait reports ctx's error even + // when the app shut down cleanly; report how it exited. + s.logger.Printf("app=%s stopped: %s", a.Manifest.ID, cmd.ProcessState) + return cmd.ProcessState.ExitCode() + } if exitErr, ok := err.(*exec.ExitError); ok { return exitErr.ExitCode() } diff --git a/plugin/appstore/zz6_stop_group_test.go b/plugin/appstore/zz6_stop_group_test.go new file mode 100644 index 0000000..8119735 --- /dev/null +++ b/plugin/appstore/zz6_stop_group_test.go @@ -0,0 +1,93 @@ +//go:build linux || darwin + +package appstore + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" +) + +// TestStopKillsAppSubprocesses: stopping an app (the ctx cancel that daemon +// shutdown, uninstall and a replacing rescan all use) also stops the +// processes the app started. exec's default cancel killed only the app's +// own pid, so they were reparented to launchd/init and ran on. +func TestStopKillsAppSubprocesses(t *testing.T) { + dir := t.TempDir() + pidFile := filepath.Join(dir, "child.pid") + body := "#!/bin/sh\nsleep 300 &\necho $! > " + pidFile + "\nwhile :; do sleep 1; done\n" + bin := filepath.Join(dir, "group-app") + if err := os.WriteFile(bin, []byte(body), 0o755); err != nil { + t.Fatal(err) + } + a := fakeApp(dir, bin, sha256Hex([]byte(body))) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + exited := make(chan int, 1) + go func() { exited <- quietSupervisor().spawn(ctx, a) }() + + var child int + if !waitFor(10*time.Second, func() bool { + raw, err := os.ReadFile(pidFile) + if err != nil { + return false + } + child, _ = strconv.Atoi(strings.TrimSpace(string(raw))) + return child > 0 + }) { + t.Fatal("app never started its subprocess") + } + t.Cleanup(func() { _ = syscall.Kill(child, syscall.SIGKILL) }) + + cancel() + select { + case <-exited: + case <-time.After(10 * time.Second): + t.Fatal("spawn did not return after the app was stopped") + } + if !waitFor(5*time.Second, func() bool { return processGone(child) }) { + t.Fatalf("subprocess pid=%d outlived the app it belongs to", child) + } +} + +// TestKillAppGroupFallsBackToThePid: a process that does not lead a group +// (so there is no group to signal) is still killed, as exec's default +// Cancel would. +func TestKillAppGroupFallsBackToThePid(t *testing.T) { + cmd := exec.Command("/bin/sh", "-c", "while :; do sleep 1; done") // no Setpgid: not a leader + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = cmd.Process.Kill() }) + if err := killAppGroup(cmd.Process); err != nil { + t.Fatalf("killAppGroup: %v", err) + } + if !waitExited(cmd, 5*time.Second) { + t.Fatal("process survived killAppGroup") + } +} + +// processGone reports whether pid has exited. Killed and reparented, it +// stays a zombie until pid 1 reaps it, which a container's pid 1 (often +// the test binary's parent) may never do, so on Linux a zombie counts as +// gone. +func processGone(pid int) bool { + if syscall.Kill(pid, 0) != nil { + return true + } + stat, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/stat") + if err != nil { + return false + } + // pid (comm) state ...: comm may contain spaces, so read after the last ')'. + s := string(stat) + i := strings.LastIndexByte(s, ')') + return i >= 0 && i+2 < len(s) && s[i+2] == 'Z' +} diff --git a/plugin/appstore/zz7_stop_graceful_test.go b/plugin/appstore/zz7_stop_graceful_test.go new file mode 100644 index 0000000..f70bfb4 --- /dev/null +++ b/plugin/appstore/zz7_stop_graceful_test.go @@ -0,0 +1,143 @@ +//go:build linux || darwin + +package appstore + +import ( + "context" + "os" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" +) + +// gracefulApp writes an app stand-in that creates the socket the +// supervisor passes with --socket, starts a subprocess, and then idles. +// With ignoreTerm=false it traps SIGTERM like a real app's shutdown path: +// it records the signal, removes its socket and exits 0. With +// ignoreTerm=true it (and the subprocess, which inherits the ignored +// disposition) ignores SIGTERM, to exercise the SIGKILL fallback. +func gracefulApp(t *testing.T, ignoreTerm bool) (a *installedApp, termMarker, childPID string) { + t.Helper() + dir := t.TempDir() + termMarker = filepath.Join(dir, "got-sigterm") + childPID = filepath.Join(dir, "child.pid") + trap := `trap 'echo term > "` + termMarker + `"; rm -f "$SOCK"; exit 0' TERM` + if ignoreTerm { + trap = `trap '' TERM` + } + body := "#!/bin/sh\n" + + "while [ $# -gt 0 ]; do [ \"$1\" = --socket ] && SOCK=$2; shift; done\n" + + trap + "\n" + + ": > \"$SOCK\"\n" + + "sleep 300 &\n" + + "echo $! > " + childPID + "\n" + + "while :; do sleep 0.05; done\n" + bin := filepath.Join(dir, "graceful-app") + if err := os.WriteFile(bin, []byte(body), 0o755); err != nil { + t.Fatal(err) + } + return fakeApp(dir, bin, sha256Hex([]byte(body))), termMarker, childPID +} + +func readPID(t *testing.T, path string) int { + t.Helper() + var pid int + if !waitFor(10*time.Second, func() bool { + raw, err := os.ReadFile(path) + if err != nil { + return false + } + pid, _ = strconv.Atoi(strings.TrimSpace(string(raw))) + return pid > 0 + }) { + t.Fatalf("%s never written", path) + } + t.Cleanup(func() { _ = syscall.Kill(pid, syscall.SIGKILL) }) + return pid +} + +// TestStopLetsAppShutDown: stopping an app (daemon shutdown, uninstall, a +// replacing rescan) delivers SIGTERM first, so the app runs its own +// shutdown and removes its socket, and the stop reports the app's real +// exit status. A straight SIGKILL skipped all of that: the wallet never +// closed its ledger and left app.sock behind. +func TestStopLetsAppShutDown(t *testing.T) { + a, termMarker, childPIDFile := gracefulApp(t, false) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + exited := make(chan int, 1) + go func() { exited <- quietSupervisor().spawn(ctx, a) }() + + child := readPID(t, childPIDFile) + if !waitFor(10*time.Second, func() bool { _, err := os.Stat(a.SocketPath); return err == nil }) { + t.Fatal("app never created its socket") + } + + start := time.Now() + cancel() + var code int + select { + case code = <-exited: + case <-time.After(appStopGrace + 10*time.Second): + t.Fatal("spawn did not return after the app was stopped") + } + if took := time.Since(start); took >= appStopGrace { + t.Errorf("stop took %s; an app that exits on SIGTERM should not wait out the %s grace", took, appStopGrace) + } + if b, err := os.ReadFile(termMarker); err != nil || !strings.Contains(string(b), "term") { + t.Errorf("app never received SIGTERM (marker %q, err %v)", b, err) + } + if _, err := os.Stat(a.SocketPath); !os.IsNotExist(err) { + t.Errorf("socket %s left behind after a graceful stop (stat err=%v)", a.SocketPath, err) + } + if code != 0 { + t.Errorf("spawn returned %d, want the app's own exit status 0", code) + } + if !waitFor(5*time.Second, func() bool { return processGone(child) }) { + t.Errorf("subprocess pid=%d outlived the app it belongs to", child) + } +} + +// TestStopKillsAppThatIgnoresSIGTERM: an app (and its subprocess) that +// ignore SIGTERM are SIGKILLed as a group once appStopGrace runs out. +func TestStopKillsAppThatIgnoresSIGTERM(t *testing.T) { + a, _, childPIDFile := gracefulApp(t, true) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + exited := make(chan int, 1) + go func() { exited <- quietSupervisor().spawn(ctx, a) }() + + child := readPID(t, childPIDFile) + + start := time.Now() + cancel() + select { + case <-exited: + case <-time.After(appStopGrace + 10*time.Second): + t.Fatal("an app that ignores SIGTERM was never killed") + } + if took := time.Since(start); took < appStopGrace-100*time.Millisecond { + t.Errorf("returned after %s; expected the %s grace before SIGKILL", took, appStopGrace) + } + if !waitFor(5*time.Second, func() bool { return processGone(child) }) { + t.Fatalf("subprocess pid=%d (ignoring SIGTERM) survived the group SIGKILL", child) + } +} + +// TestStopAppGroupAlreadyExited: cancelling after the app has exited on +// its own is not an error exec treats as a failed cancel. +func TestStopAppGroupAlreadyExited(t *testing.T) { + p, err := os.StartProcess("/bin/sh", []string{"sh", "-c", "exit 0"}, &os.ProcAttr{}) + if err != nil { + t.Fatal(err) + } + if _, err := p.Wait(); err != nil { + t.Fatal(err) + } + if err := stopAppGroup(p, time.Second); err != nil && err != os.ErrProcessDone { + t.Fatalf("stopAppGroup on an exited process: %v", err) + } +}