fix(supervisor): SIGTERM an app's process group on stop, SIGKILL after a 3 s grace

plugin/appstore/reap.go

@@ -96,6 +96,49 @@ func killAppGroup(p *os.Process) error {
 	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
 
@@ -115,6 +158,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/supervisor.go

@@ -1040,9 +1040,11 @@ func (s *supervisor) spawn(ctx context.Context, a *installedApp) int {
 	// 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. Kill the
-	// whole process group the app leads instead (killAppGroup).
-	cmd.Cancel = func() error { return killAppGroup(cmd.Process) }
+	// 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
@@ -1079,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/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)
+	}
+}