fix(supervisor): stopping an app stops the processes it started

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:

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
 }
 

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()
 		}

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'
+}

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)
+	}
+}