From 0c473f9e96e25d45ae1ddb383b6cff19f5bef1fa Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 11:43:52 +0300 Subject: [PATCH] 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) + } +}