From ea671fdf4979f3696b9477a32c556f3f3cf1b89a Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 10:11:41 +0300 Subject: [PATCH 1/4] fix(supervisor): stopping an app stops the processes it started Daemon shutdown (StopPlugins), `pilotctl appstore uninstall` (rescan cancels the app) and a rescan that replaces an app all stop it by cancelling the context of its exec.CommandContext. exec's default Cancel SIGKILLs only the app's own pid, so every process the app had started was reparented to launchd/init and kept running with nothing left that would ever stop it (reapStale matches an app's binary and socket in argv, which a subprocess does not carry). Apps already lead their own process group (Setpgid), and the other kill paths (reapStale, watchSocket) already signal the group. The app's exec.Cmd.Cancel now does the same: SIGKILL to the whole group, falling back to the pid alone, as before, when the group cannot be signalled. Cancel runs before Wait reaps the app, so the pid, which is the group id, is still held and cannot have been reused. A subprocess that moved itself to its own group or session (setsid) is not touched. TestStopKillsAppSubprocesses starts an app whose script backgrounds a `sleep 300`, stops it, and asserts the sleep is gone. It fails on origin/main on macOS and Linux (verified) and passes with this change. Co-Authored-By: Claude Opus 5.5 (1M context) --- plugin/appstore/reap.go | 14 +++++ plugin/appstore/supervisor.go | 6 +++ plugin/appstore/zz6_stop_group_test.go | 75 ++++++++++++++++++++++++++ 3 files changed, 95 insertions(+) create mode 100644 plugin/appstore/zz6_stop_group_test.go diff --git a/plugin/appstore/reap.go b/plugin/appstore/reap.go index 73ea4aa..0ff27df 100644 --- a/plugin/appstore/reap.go +++ b/plugin/appstore/reap.go @@ -82,6 +82,20 @@ 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() +} + // socketCheckInterval is how often a running app's socket is checked. const socketCheckInterval = 5 * time.Second diff --git a/plugin/appstore/supervisor.go b/plugin/appstore/supervisor.go index e5c6733..c0c0fa3 100644 --- a/plugin/appstore/supervisor.go +++ b/plugin/appstore/supervisor.go @@ -1037,6 +1037,12 @@ 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. Kill the + // whole process group the app leads instead (killAppGroup). + cmd.Cancel = func() error { return killAppGroup(cmd.Process) } if a.Sideloaded { // Signal sideload status to cap-aware children. Apps that // honour their declared grants (e.g. the wallet) can read diff --git a/plugin/appstore/zz6_stop_group_test.go b/plugin/appstore/zz6_stop_group_test.go new file mode 100644 index 0000000..5187eee --- /dev/null +++ b/plugin/appstore/zz6_stop_group_test.go @@ -0,0 +1,75 @@ +//go:build linux || darwin + +package appstore + +import ( + "context" + "os" + "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) + } +} + +// 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' +} From 82c884f98b315e6f299962f2a933ae5519cfda22 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 10:11:41 +0300 Subject: [PATCH 2/4] test(reap): fake orphans never fork or re-exec startFakeApp ran `/bin/sh -c 'while :; do sleep 1; done' `. Each `sleep` the loop forks carries the shell's argv until its exec, so reapStale could count a third instance (seen in a Linux container: "reapStale = [2110 2111 2113], want both pids 2110 and 2111"), and macOS's /bin/sh shim re-execs bash, during which a scan can miss one. The fake is now this test binary (APPSTORE_TEST_FAKE_ORPHAN), blocked reading a pipe the test holds open: its argv is final once Start returns, and it never forks. Test-only. Co-Authored-By: Claude Opus 5.5 (1M context) --- plugin/appstore/reap_test.go | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) 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 } From 1ad004237fa929547255b534503bb79db12d12df Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 10:46:32 +0300 Subject: [PATCH 3/4] test(supervisor): killAppGroup falls back to the pid when there is no group Covers the fallback branch: a process that does not lead a process group is still killed, as exec's default Cancel would. Co-Authored-By: Claude Opus 5.5 (1M context) --- plugin/appstore/zz6_stop_group_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/plugin/appstore/zz6_stop_group_test.go b/plugin/appstore/zz6_stop_group_test.go index 5187eee..8119735 100644 --- a/plugin/appstore/zz6_stop_group_test.go +++ b/plugin/appstore/zz6_stop_group_test.go @@ -5,6 +5,7 @@ package appstore import ( "context" "os" + "os/exec" "path/filepath" "strconv" "strings" @@ -56,6 +57,23 @@ func TestStopKillsAppSubprocesses(t *testing.T) { } } +// 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 From 0c473f9e96e25d45ae1ddb383b6cff19f5bef1fa Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 11:43:52 +0300 Subject: [PATCH 4/4] fix(supervisor): SIGTERM an app's process group on stop, SIGKILL after a 3 s grace Builds on killAppGroup. Every stop (daemon shutdown, uninstall, a rescan that replaces the app) still SIGKILLed the app at once, so the app never ran its shutdown path. For io.pilot.wallet that means in-flight calls are cut off between the ledger write and the spend-cap log append, the sqlite ledger is never closed, and app.sock is left on disk. cmd.Cancel is now stopAppGroup. It SIGTERMs the group the app leads (falling back to the pid), waits up to appStopGrace (3 s) for the app to exit, then SIGKILLs the group through killAppGroup. The group is only signalled while the app is unreaped, so its pid (the group id) is still held. The daemon's plugin stop budget is 5 s and apps stop in parallel. Also: - A stop now reports the app's real exit status instead of "wait: context canceled" / -1. - watchSocket ignores a socket that disappears after the stop began. The app removing it on SIGTERM is a normal shutdown, not a "socket-lost" event. Tests (zz7_stop_graceful_test.go). With the Cancel line reverted to killAppGroup: - TestStopLetsAppShutDown fails: "app never received SIGTERM", "socket ... left behind", "spawn returned -1". - TestStopKillsAppThatIgnoresSIGTERM fails: "returned after 1.558ms; expected the 3s grace". Both pass with this change. TestStopKillsAppSubprocesses still passes. Co-Authored-By: Claude Opus 5.5 (1M context) --- plugin/appstore/reap.go | 48 ++++++++ plugin/appstore/supervisor.go | 14 ++- plugin/appstore/zz7_stop_graceful_test.go | 143 ++++++++++++++++++++++ 3 files changed, 202 insertions(+), 3 deletions(-) create mode 100644 plugin/appstore/zz7_stop_graceful_test.go diff --git a/plugin/appstore/reap.go b/plugin/appstore/reap.go index 0ff27df..a3fc058 100644 --- a/plugin/appstore/reap.go +++ b/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: diff --git a/plugin/appstore/supervisor.go b/plugin/appstore/supervisor.go index c0c0fa3..dedd160 100644 --- a/plugin/appstore/supervisor.go +++ b/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() } 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) + } +}