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