fix(supervisor): stopping an app stops the processes it started - #39
Merged
Merged
Conversation
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) <noreply@anthropic.com>
startFakeApp ran `/bin/sh -c 'while :; do sleep 1; done' <argv>`. 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) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
… 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) <noreply@anthropic.com>
…r 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) <noreply@anthropic.com>
This was referenced Sep 24, 2026
fix(supervisor): SIGTERM an app's process group on stop, SIGKILL after a 3 s grace
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Every way an app is stopped goes through one path: cancelling the context of its
exec.CommandContext. That covers daemon shutdown (StopPlugins),pilotctl appstore uninstall(the rescan cancels the app), and a rescan that replaces an app. By default, exec's Cancel SIGKILLs only the app's own pid. Any process the app had started got reparented to launchd/init and kept running, and nothing would ever stop it.reapStalecan't catch these because it matches the app's binary and socket in argv, and a subprocess doesn't carry them.Reproduced on
origin/main(79e9944) with a test app whose script runssleep 300 &, then stopping it:Fix
Apps already lead their own process group (
Setpgid), and the other kill paths (reapStale,watchSocket) already signal the group.spawnnow setscmd.CanceltokillAppGroup, which sends SIGKILL to the whole group. It falls back to the pid alone (today's behaviour) when the group can't be signalled.Waitreaps the app. The app's pid, which is also the group id, is still held (by the process or its zombie), so it can't have been reused.setsid) is not touched.Tests
TestStopKillsAppSubprocessesstarts an app that backgroundssleep 300, stops it, and asserts the sleep is gone. With the fix removed it fails on macOS and on Linux (subprocess pid=... outlived the app it belongs to). With the fix it passes. A zombie counts as gone, because a container's pid 1 may never reap it.startFakeApploopedsleepin/bin/sh. Each forkedsleepcarries the shell's argv until its exec, soreapStalecould count a third instance (seen in a Linux container:reapStale = [2110 2111 2113], want both pids 2110 and 2111). macOS's/bin/shshim also re-execs bash, and a scan during that re-exec can miss an instance. The fake is now the test binary blocked on a pipe, which never forks or re-execs.Runs:
go test -race ./...passes; fullplugin/appstore3/3 runs; reap, watchSocket, hard-death and stop tests 10x-count.--init):go test -race ./...5/5 plus 1.Not covered here (remaining gap)
When the daemon dies hard on Linux,
Pdeathsigkills the app itself but not the processes the app started.reapStalecan't find those later, because the leader is gone and their argv doesn't match. Probe onorigin/main:reapStale reaped=[]; subprocess pid=2236 alive: true. On macOS the same scenario is cleaned up, becausereapStalekills the orphaned leader's whole group. A pid-reuse-safe fix needs a design choice (for example a marker the subprocesses inherit), so it's left out of this PR.web4 picks this up with an app-store release and a go.mod bump (currently
v1.0.3).🤖 Generated with Claude Code