From ec4ee5a3a48ccd31cfca766bcf1dc41b45de0f8b Mon Sep 17 00:00:00 2001 From: Aronnax <87559395+aronnaxlin@users.noreply.github.com> Date: Fri, 2 Oct 2026 00:42:48 +0800 Subject: [PATCH] fix(proc): end what a CLI probe started, on its timeout and when magpie exits (#414) * fix(proc): end what a CLI probe started, on its timeout and when magpie exits The probes that ask an agent's CLI something (claude auth status, cursor-agent about/models/status, the --version checks) ran through proc.CommandContext, which on its timeout kills only the command itself. A CLI that is a script (a version manager's wrapper, a shim) left what it had started running, adopted by init; and a probe still asking when magpie exited (a one-shot command, an update, a restart) was left whole, with nothing to end it. With Omarchy's mise wrappers stuck in their exec loop (omacom/omarchy#7234) that was five processes holding about half a core each for up to six hours. proc.ProbeContext gives such a probe a session of its own on Unix, as StartTree does, and its context ending kills the whole group. Probes still running are listed, and proc.EndProbes kills them when magpie exits (after run in main) or puts a new version in its place (update.Reexec). On Windows a probe is killed alone, as before, a job being joined only once the command has started. TestProbeTimeoutEndsWhatItStarted and TestEndProbes fail without the change. Co-Authored-By: Claude Opus 5.5 * fix(proc): end probes on a terminal's signals and on a Mac's quit Review of the first change: a probe leading its own session no longer gets the Ctrl+C, hang-up or SIGTERM sent to magpie's terminal or service, and magpie ending on the signal never reached EndProbes, so the probe was left to init where before it died with magpie. main now catches SIGINT, SIGTERM and SIGHUP, ends the probes and exits as the signal would have (128+n). A signal magpie was started ignoring (nohup, a background job) stays ignored. The account sign-in and plugin commands, which end themselves on Ctrl+C, go through interruptContext and are left to do so. On a Mac the desktop app's quit ([NSApp terminate:], also the restart to update) exits without returning to main, so OnShutdown ends the probes. TestProbeTimeoutEndsWhatItStarted ended its probe on a 500ms timeout, which on a Mac could come before the script had written its child's pid (6 of 10 runs failed in review). It now ends the context once that pid is read, the same Cancel path. Co-Authored-By: Claude Opus 5.5 * fix: let the app and the TUI quit on a signal their own way Review of the signal handling: endProbesOnSignal exited ahead of Wails and bubbletea, which quit on SIGINT and SIGTERM themselves. `magpie tray` exited 143 without OnShutdown (no downloaded update installed, and sessions.Saved skipped in main), and `magpie tui` left the terminal on its alternate screen with the cursor hidden. The app's and the TUI's entry points now hand the signals back (ownSignals): the app's OnShutdown ends the probes, and the TUI comes back to main, which ends them too. A one-shot command keeps the handler. TestOwnSignalsHandsThemBack fails without the change (magpie exited 143 before the command could quit its own way); TestSignalExits keeps the one-shot behavior. childPid waited 3s for a script just written to start its child, which a busy Mac can exceed (2 of 3 full runs failed in review). It waits 30s now and, should the probe end first, says the probe's error rather than "the script started nothing". Co-Authored-By: Claude Opus 5.5 * fix: keep SIGHUP for the app and the TUI, and die of the signal ownSignals handed SIGHUP back along with SIGINT and SIGTERM, but neither Wails nor bubbletea handles it: the terminal closing under the TUI or an app started from a shell ended magpie on the spot, and its probes, in a session of their own, out of the hang-up's reach, kept running (before this PR they were in the terminal's group and ended with it). SIGHUP now stays with endProbesOnSignal. A tray under a pty whose terminal closes left 2 processes before, none now. The handler asked whether a command was ending itself on Ctrl+C only after EndProbes, which can take up to a second; a sign-in done in the meantime got magpie ended in the middle of its own cleanup. It is asked first. It exited 128+n, which a shell takes for a child that handled the signal: a loop running magpie went on to the next run on Ctrl+C. It now dies of the signal (handling reset, the signal sent again); Windows, which can't send one, still exits with the code. agentProbe and agentCommand share withProxy; the Windows probeGroup comment no longer says EndProbes ends what a shim started. TestOwnSignalsKeepsHangUp and TestInterruptWhileEndingProbes fail without the change. Co-Authored-By: Claude Opus 5.5 * fix: hand signals to the TUI and the app only once they listen Review: runTUI handed SIGINT and SIGTERM back before tui.Run, whose newModel asks agents' CLIs (claude auth status, up to 5s) before bubbletea listens for signals. Ctrl+C, SIGTERM or SIGINT while it loaded ended magpie by Go's default and left the probe and its child running. tui.Run now takes ready, called once the model is made, and runTUI passes ownSignals. The app had the same window: runGUI handed them back before gui.Run, which starts the backend (probes included) before Wails' Run starts listening. ownSignals is now gui.Started, called on ApplicationStarted, which Wails emits after its signal handler is set up. A tray sent SIGTERM 0.05s after starting, a probe under way, left 2 processes before and none now; one sent it once started still quits through OnShutdown (exit 0). TestTUIKeepsSignalsWhileLoading fails without the change. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- accounts_cli.go | 3 +- gui_on.go | 2 + internal/agent/cliupdate.go | 2 +- internal/gui/app.go | 16 +++- internal/library/rtk.go | 2 +- internal/proc/probe.go | 72 ++++++++++++++ internal/proc/probe_other.go | 17 ++++ internal/proc/probe_test.go | 127 +++++++++++++++++++++++++ internal/proc/probe_windows.go | 14 +++ internal/provider/account.go | 2 +- internal/provider/claude_signin.go | 2 +- internal/provider/codex_models.go | 2 +- internal/provider/cursor.go | 6 +- internal/provider/grok.go | 12 ++- internal/tui/tui.go | 10 +- internal/update/proc_unix.go | 3 + main.go | 16 +++- plugins_cli.go | 4 +- signals.go | 101 ++++++++++++++++++++ signals_test.go | 148 +++++++++++++++++++++++++++++ signals_unix.go | 23 +++++ signals_windows.go | 7 ++ 22 files changed, 571 insertions(+), 20 deletions(-) create mode 100644 internal/proc/probe.go create mode 100644 internal/proc/probe_other.go create mode 100644 internal/proc/probe_test.go create mode 100644 internal/proc/probe_windows.go create mode 100644 signals.go create mode 100644 signals_test.go create mode 100644 signals_unix.go create mode 100644 signals_windows.go diff --git a/accounts_cli.go b/accounts_cli.go index db60a3dc..66a1087c 100644 --- a/accounts_cli.go +++ b/accounts_cli.go @@ -7,7 +7,6 @@ import ( "fmt" "os" "os/exec" - "os/signal" "runtime" "strings" "sync" @@ -304,7 +303,7 @@ func addAccount(agentID string) error { fmt.Println("and confirm the code", st.Code) } openInBrowser(st.URL) - ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt) + ctx, stop := interruptContext() defer stop() if st.PasteCallback || st.PasteCode { if st.PasteCode { diff --git a/gui_on.go b/gui_on.go index 5785a666..b3713ae7 100644 --- a/gui_on.go +++ b/gui_on.go @@ -13,6 +13,7 @@ const hasGUI = true // PATH a terminal has func runGUI(showMain bool, link string) error { proc.UserPath() + gui.Started = ownSignals // once Wails handles them: it quits through OnShutdown return gui.Run(version, showMain, link) } @@ -28,5 +29,6 @@ func runWindow(view string) error { func runPanel() error { proc.UserPath() gui.OpenPanel = true + gui.Started = ownSignals // once Wails handles them: it quits through OnShutdown return gui.Run(version, false, "") } diff --git a/internal/agent/cliupdate.go b/internal/agent/cliupdate.go index 2472da73..e69f8fb1 100644 --- a/internal/agent/cliupdate.go +++ b/internal/agent/cliupdate.go @@ -401,7 +401,7 @@ func installedVersion(bin string) string { var runVersion = func(bin string) string { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - cmd := proc.CommandContext(ctx, bin, "--version") + cmd := proc.ProbeContext(ctx, bin, "--version") cmd.Stdin = nil // /dev/null: one that would ask something gets nothing cmd.Env = append(os.Environ(), "NO_COLOR=1") out, _ := cmd.CombinedOutput() diff --git a/internal/gui/app.go b/internal/gui/app.go index 6ba5d9f4..7bbbc6f2 100644 --- a/internal/gui/app.go +++ b/internal/gui/app.go @@ -23,6 +23,7 @@ import ( "github.com/yetone/magpie/internal/autostart" "github.com/yetone/magpie/internal/library" "github.com/yetone/magpie/internal/omarchy" + "github.com/yetone/magpie/internal/proc" "github.com/yetone/magpie/internal/settings" "github.com/yetone/magpie/internal/shortcut" "github.com/yetone/magpie/internal/stats" @@ -280,10 +281,18 @@ func Run(version string, showMain bool, link string) error { Windows: application.WindowsOptions{DisableQuitOnLastWindowClosed: true}, // A version downloaded but not restarted into is installed on the // way out, so the next launch is the new one. - OnShutdown: func() { updates.install(false) }, + // Quitting doesn't come back to main on a Mac (NSApp terminate: + // exits), so the CLIs still being asked something end here. + OnShutdown: func() { + proc.EndProbes() + updates.install(false) + }, // Wails exits on some webview errors; say why before it does. ErrorHandler: func(err error) { log.Println("magpie:", err) }, }) + if Started != nil { + h.app.Event.OnApplicationEvent(events.Common.ApplicationStarted, func(*application.ApplicationEvent) { Started() }) + } onDock = func(s settings.Settings) { h.dock(s, h.main.IsVisible()) } // The Dock icon opens the window. Wails would show every hidden window @@ -591,6 +600,11 @@ var OpenPanel bool // a restart to update comes back where it was asked for. var OpenView string +// Started is called once the app has started, by when Wails handles SIGINT +// and SIGTERM itself (it starts listening as it runs, before the app is +// said to have started); until then a signal is magpie's to handle. +var Started func() + // togglePanel opens the quick panel by the tray icon, or closes it. func (h *host) togglePanel() { if omarchy.Hyprland() { diff --git a/internal/library/rtk.go b/internal/library/rtk.go index d7e610d9..6d20fafe 100644 --- a/internal/library/rtk.go +++ b/internal/library/rtk.go @@ -262,7 +262,7 @@ var openCodeVersion = func() string { } ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - cmd := proc.CommandContext(ctx, bin, "--version") + cmd := proc.ProbeContext(ctx, bin, "--version") cmd.Stdin = nil out, err := cmd.Output() if err != nil { diff --git a/internal/proc/probe.go b/internal/proc/probe.go new file mode 100644 index 00000000..c757fed9 --- /dev/null +++ b/internal/proc/probe.go @@ -0,0 +1,72 @@ +package proc + +import ( + "context" + "os/exec" + "sync" + "time" +) + +// ProbeContext is CommandContext for a command magpie only asks something +// of and waits on for the answer: who a CLI says is signed in, its version. +// The CLI is often a script (a version manager's wrapper, a shim), and +// killing it on the timeout left what it had started running; a probe still +// asking when magpie quit or updated itself was left whole, adopted by init +// with nothing to end it, and one stuck in a loop held a core for hours. A +// probe leads a group of its own on Unix, the context ending kills the whole +// group, and EndProbes does the same for those still asking when magpie +// exits. ctx must end (a timeout): the probe is forgotten when it does. +func ProbeContext(ctx context.Context, name string, args ...string) *exec.Cmd { + ctx, cancel := context.WithCancel(ctx) + p := &probe{cancel: cancel, ended: make(chan struct{})} + cmd := CommandContext(ctx, name, args...) + probeGroup(cmd, p) + probes.Lock() + probes.m[p] = struct{}{} + probes.Unlock() + context.AfterFunc(ctx, func() { + probes.Lock() + delete(probes.m, p) + probes.Unlock() + }) + return cmd +} + +type probe struct { + cancel context.CancelFunc + once sync.Once + ended chan struct{} // closed once it was killed +} + +func (p *probe) end() { p.once.Do(func() { close(p.ended) }) } + +var probes = struct { + sync.Mutex + m map[*probe]struct{} +}{m: map[*probe]struct{}{}} + +// endWait is how long EndProbes waits for the probes to be killed. +var endWait = time.Second + +// EndProbes kills every probe still running, with what it started, and +// waits a moment for that: magpie is exiting or putting a new version in its +// place, and nothing would end them after. +func EndProbes() { + probes.Lock() + ps := make([]*probe, 0, len(probes.m)) + for p := range probes.m { + ps = append(ps, p) + } + probes.Unlock() + for _, p := range ps { + p.cancel() + } + deadline := time.After(endWait) + for _, p := range ps { + select { + case <-p.ended: + case <-deadline: + return // one that had answered already, or never started + } + } +} diff --git a/internal/proc/probe_other.go b/internal/proc/probe_other.go new file mode 100644 index 00000000..5bfd5c98 --- /dev/null +++ b/internal/proc/probe_other.go @@ -0,0 +1,17 @@ +//go:build !windows + +package proc + +import "os/exec" + +// probeGroup has the probe lead a session, and its context ending kill the +// session's group: the script at its head and what that started. +func probeGroup(cmd *exec.Cmd, p *probe) { + ownGroup(cmd) + cmd.Cancel = func() error { + defer p.end() + var g group + g.kill(cmd) + return cmd.Process.Kill() + } +} diff --git a/internal/proc/probe_test.go b/internal/proc/probe_test.go new file mode 100644 index 00000000..8c085b48 --- /dev/null +++ b/internal/proc/probe_test.go @@ -0,0 +1,127 @@ +//go:build !windows + +package proc + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" +) + +// probeScript is a CLI that is a script starting another program and +// waiting on it, as a version manager's wrapper does; it writes down the +// pid of what it started. +func probeScript(t *testing.T) (sh, pidFile string) { + dir := t.TempDir() + sh, pidFile = filepath.Join(dir, "cli"), filepath.Join(dir, "child.pid") + os.WriteFile(sh, []byte("#!/bin/sh\nsleep 30 &\necho $! > "+pidFile+"\nwait\n"), 0o755) + old := waitDelay + waitDelay = 200 * time.Millisecond + t.Cleanup(func() { waitDelay = old }) + return sh, pidFile +} + +// runProbe runs the probe in the background; the channel gives what its +// run ended with. +func runProbe(cmd *exec.Cmd) chan error { + done := make(chan error, 1) + go func() { + _, err := cmd.Output() + done <- err + }() + return done +} + +// childPid waits for the script to have started its child. A script just +// written can take seconds to start on a busy Mac, so the wait is long; a +// probe that ended before is said with its error. +func childPid(t *testing.T, pidFile string, done chan error) int { + t.Helper() + tick := time.NewTicker(20 * time.Millisecond) + defer tick.Stop() + end := time.After(30 * time.Second) + for { + if b, err := os.ReadFile(pidFile); err == nil { + if pid, err := strconv.Atoi(strings.TrimSpace(string(b))); err == nil { + t.Cleanup(func() { _ = syscall.Kill(pid, syscall.SIGKILL) }) + return pid + } + } + select { + case err := <-done: + t.Fatalf("the probe ended (%v) before the script started anything", err) + case <-end: + t.Fatal("the script started nothing in 30s") + case <-tick.C: + } + } +} + +// a probe whose context ends (its timeout) ends with what it started: +// killing the script alone left its child running. The context is ended +// once the script has started its child, not on a timer that a slow start +// could beat. +func TestProbeTimeoutEndsWhatItStarted(t *testing.T) { + sh, pidFile := probeScript(t) + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + done := runProbe(ProbeContext(ctx, sh)) + pid := childPid(t, pidFile, done) + cancel() + select { + case <-done: + case <-time.After(3 * time.Second): + t.Fatal("the probe went on after its context ended") + } + if !gone(pid) { + t.Fatalf("what the probe started (pid %d) outlived its context", pid) + } +} + +// a probe still asking when magpie exits is ended with it, rather than +// left to init +func TestEndProbes(t *testing.T) { + sh, pidFile := probeScript(t) + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + done := runProbe(ProbeContext(ctx, sh)) + pid := childPid(t, pidFile, done) + start := time.Now() + EndProbes() + select { + case <-done: + case <-time.After(3 * time.Second): + t.Fatal("the probe went on after EndProbes") + } + if !gone(pid) { + t.Fatalf("what the probe started (pid %d) outlived EndProbes", pid) + } + if d := time.Since(start); d > endWait+time.Second { + t.Fatalf("EndProbes took %v", d) + } +} + +// a probe that answered is forgotten once its context ends, so EndProbes +// doesn't wait on it +func TestProbeForgotten(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + if _, err := ProbeContext(ctx, "true").Output(); err != nil { + t.Fatal(err) + } + cancel() + for end := time.Now().Add(time.Second); time.Now().Before(end); time.Sleep(10 * time.Millisecond) { + probes.Lock() + n := len(probes.m) + probes.Unlock() + if n == 0 { + return + } + } + t.Fatal("a probe that answered is still listed") +} diff --git a/internal/proc/probe_windows.go b/internal/proc/probe_windows.go new file mode 100644 index 00000000..878f4c08 --- /dev/null +++ b/internal/proc/probe_windows.go @@ -0,0 +1,14 @@ +package proc + +import "os/exec" + +// probeGroup kills the probe alone, as CommandContext does: a job, as +// StartTree gives a command, is joined only once it has started, and Output +// starts it out of reach. EndProbes ends it the same way when magpie exits, +// so what a shim started (npm's .cmd running node) is still left running. +func probeGroup(cmd *exec.Cmd, p *probe) { + cmd.Cancel = func() error { + defer p.end() + return cmd.Process.Kill() + } +} diff --git a/internal/provider/account.go b/internal/provider/account.go index 221dcd95..6d8f5066 100644 --- a/internal/provider/account.go +++ b/internal/provider/account.go @@ -536,7 +536,7 @@ func askClaudeStatus() (user, plan string, signedOut, ok bool) { // settings.json it applies itself (auth status takes no // --setting-sources), and then answers with no email; claudeSignedInUser // names the account from ~/.claude.json instead (#177). - cmd := proc.CommandContext(ctx, path, "auth", "status", "--json") + cmd := proc.ProbeContext(ctx, path, "auth", "status", "--json") cmd.Env = withoutClaudeWiring(os.Environ()) out, _ := cmd.Output() var status struct { diff --git a/internal/provider/claude_signin.go b/internal/provider/claude_signin.go index 8d6637d6..b7383b02 100644 --- a/internal/provider/claude_signin.go +++ b/internal/provider/claude_signin.go @@ -228,7 +228,7 @@ func claudeSignedIn(path, dir string) (savedLogin, error) { } ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() - cmd := proc.CommandContext(ctx, path, "auth", "status", "--json") + cmd := proc.ProbeContext(ctx, path, "auth", "status", "--json") cmd.Dir = dir cmd.Env = claudeSignInEnv(os.Environ(), dir) b, _ := cmd.Output() diff --git a/internal/provider/codex_models.go b/internal/provider/codex_models.go index 2c5714f3..d093360c 100644 --- a/internal/provider/codex_models.go +++ b/internal/provider/codex_models.go @@ -90,7 +90,7 @@ func codexVersion() string { } if exe := codexExecutable(); exe != "" { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - if out, err := proc.CommandContext(ctx, exe, "--version").Output(); err == nil { + if out, err := proc.ProbeContext(ctx, exe, "--version").Output(); err == nil { newer(string(out)) // "codex-cli 0.155.1" } cancel() diff --git a/internal/provider/cursor.go b/internal/provider/cursor.go index 862f4380..2291c01d 100644 --- a/internal/provider/cursor.go +++ b/internal/provider/cursor.go @@ -72,7 +72,7 @@ func askCursorStatus() (user, plan string, ok bool, err error) { } ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - out, err := agentCommand(ctx, path, "about", "--format", "json").Output() + out, err := agentProbe(ctx, path, "about", "--format", "json").Output() user, plan, said := parseCursorAbout(out) switch { case user != "": @@ -138,7 +138,7 @@ func cursorModels(ctx context.Context) ([]catalog.Model, error) { } ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() - out, err := agentCommand(ctx, path, "models").Output() + out, err := agentProbe(ctx, path, "models").Output() if err != nil { return nil, errorf("cursor-agent models: %v", err) } @@ -351,7 +351,7 @@ func CursorToken() (string, error) { tok = t // renewed while this waited } else { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - _ = agentCommand(ctx, path, "status").Run() + _ = agentProbe(ctx, path, "status").Run() cancel() tok = readCursorToken() } diff --git a/internal/provider/grok.go b/internal/provider/grok.go index 2a86e827..f8a14120 100644 --- a/internal/provider/grok.go +++ b/internal/provider/grok.go @@ -379,7 +379,7 @@ func grokVersion() string { v := grokClientVersion if exe := GrokExecutable(); exe != "" { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - if out, err := proc.CommandContext(ctx, exe, "--version").Output(); err == nil { // "grok 1.0.41 (4220f3b224a6)" + if out, err := proc.ProbeContext(ctx, exe, "--version").Output(); err == nil { // "grok 1.0.41 (4220f3b224a6)" if c := grokSemver.FindString(string(out)); c != "" && compareClaudeVersion(c, v) > 0 { v = c } @@ -533,7 +533,15 @@ func startGrokSignIn(s *signInFlow) error { // agentCommand runs an agent's CLI with magpie's proxy. func agentCommand(ctx context.Context, path string, args ...string) *exec.Cmd { - cmd := proc.CommandContext(ctx, path, args...) + return withProxy(proc.CommandContext(ctx, path, args...)) +} + +// agentProbe is agentCommand for asking the CLI something (proc.ProbeContext). +func agentProbe(ctx context.Context, path string, args ...string) *exec.Cmd { + return withProxy(proc.ProbeContext(ctx, path, args...)) +} + +func withProxy(cmd *exec.Cmd) *exec.Cmd { cmd.Env = netproxy.Env(nil) return cmd } diff --git a/internal/tui/tui.go b/internal/tui/tui.go index 1debbcb6..94261fdb 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -138,9 +138,15 @@ type flashMsg struct { type syncedMsg struct{ err error } -// Run starts magpie in the terminal. -func Run() error { +// Run starts magpie in the terminal. ready is called once the model is +// made: making it asks agents' CLIs (claude auth status, up to seconds), +// and a signal then is the caller's to handle, as bubbletea starts +// listening only as the program runs. +func Run(ready func()) error { m := newModel() + if ready != nil { + ready() + } if len(m.agents) == 0 { return fmt.Errorf("no supported agents found on this machine") } diff --git a/internal/update/proc_unix.go b/internal/update/proc_unix.go index 0068bdfb..4c275e92 100644 --- a/internal/update/proc_unix.go +++ b/internal/update/proc_unix.go @@ -6,6 +6,8 @@ import ( "os" "os/exec" "syscall" + + "github.com/yetone/magpie/internal/proc" ) // alive reports whether pid is still running. A relaunched magpie is that @@ -31,5 +33,6 @@ func detach(cmd *exec.Cmd) { // process, in the same terminal or under the same service manager, now // running the new version (magpie web's restart to update). func Reexec(exe string, args, env []string) error { + proc.EndProbes() // the new version doesn't know them, nor waits on them return syscall.Exec(exe, append([]string{exe}, args...), env) } diff --git a/main.go b/main.go index 6cb18dbc..1fe9d018 100644 --- a/main.go +++ b/main.go @@ -17,6 +17,7 @@ import ( "github.com/yetone/magpie/internal/gateway" "github.com/yetone/magpie/internal/imagemcp" "github.com/yetone/magpie/internal/netproxy" + "github.com/yetone/magpie/internal/proc" "github.com/yetone/magpie/internal/profile" "github.com/yetone/magpie/internal/provider" "github.com/yetone/magpie/internal/sessions" @@ -111,10 +112,12 @@ func main() { // Claude Code, signing in for magpie, handed over the page to open return } + endProbesOnSignal() gateway.Version = version netproxy.Install() update.GUI = hasGUI err := run(os.Args[1:]) + proc.EndProbes() // a CLI still being asked something isn't left to init sessions.Saved() // the session index kept, for the next run if err != nil { fmt.Fprintln(os.Stderr, "magpie:", err) @@ -122,6 +125,15 @@ func main() { } } +// runTUI runs the TUI, which quits on Ctrl+C and SIGTERM itself once it +// has started; it asks CLIs first, and a signal then ends those. +func runTUI() error { + return tuiRun(ownSignals) +} + +// tuiRun is tui.Run; a var so tests can stand in for it. +var tuiRun = tui.Run + func run(args []string) error { if len(args) > 0 && args[0] == "healthcheck" { return healthcheck() // every few seconds in a container: nothing else @@ -151,7 +163,7 @@ func run(args []string) error { if hasGUI { return runGUI(true, "") } - return tui.Run() + return runTUI() } // a magpie:// link the system handed over (Windows, Linux): the app // opens it for the user to confirm @@ -163,7 +175,7 @@ func run(args []string) error { } switch args[0] { case "tui": - return tui.Run() + return runTUI() case "web": return webCmd(args[1:]) case "app", "gui": diff --git a/plugins_cli.go b/plugins_cli.go index ba44344e..d9604023 100644 --- a/plugins_cli.go +++ b/plugins_cli.go @@ -5,8 +5,6 @@ import ( "encoding/json" "errors" "fmt" - "os" - "os/signal" "strconv" "strings" "time" @@ -33,7 +31,7 @@ func pluginCmd(args []string) error { sub = args[1] } rest := args[min(len(args), 2):] - ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt) + ctx, stop := interruptContext() defer stop() switch sub { case "list", "ls", "--json": diff --git a/signals.go b/signals.go new file mode 100644 index 00000000..1ce2ffb0 --- /dev/null +++ b/signals.go @@ -0,0 +1,101 @@ +package main + +import ( + "context" + "os" + "os/signal" + "sync" + "sync/atomic" + "syscall" + + "github.com/yetone/magpie/internal/proc" +) + +// endProbesOnSignal: the probes magpie asks a CLI with lead a session of +// their own (proc.ProbeContext), so Ctrl+C in the terminal, the terminal +// closing or a service manager's SIGTERM reach magpie and not them, and +// magpie ending on the signal left them to init. They are ended first, and +// magpie then dies of the signal. A command that waits on Ctrl+C itself +// (interruptContext) is left to finish its own way, its probes ended all the +// same; the app and the TUI, which quit on SIGINT and SIGTERM their own way, +// take those back (ownSignals). A signal magpie was started ignoring (nohup, +// a background job) stays ignored: asking for it would undo that. +func endProbesOnSignal() { + caught = nil + for _, s := range []os.Signal{os.Interrupt, syscall.SIGTERM, syscall.SIGHUP} { + if !signal.Ignored(s) { + caught = append(caught, s) + } + } + if len(caught) == 0 { + return + } + onSignal = make(chan os.Signal, 1) + signal.Notify(onSignal, caught...) + go func(sigs chan os.Signal) { + for s := range sigs { + // asked before the probes are ended, which can take a moment + // the command may spend finishing, and stop asking + handled := s == os.Interrupt && interrupts.Load() > 0 + proc.EndProbes() + if !handled { + die(s) + } + } + }(onSignal) +} + +// caught are the signals endProbesOnSignal asked for; onSignal is where +// they come. +var ( + caught []os.Signal + onSignal chan os.Signal +) + +// die ends magpie of s, as s would have had there been no handler: the +// shell that ran it sees a process a signal ended (a loop running magpie +// stops on Ctrl+C), not one that exited. Where a signal can't be sent to +// oneself (Windows), it exits as a shell reports such a process. A var so +// tests can see what a signal would do. +var die = func(s os.Signal) { + raise(s) + code := 1 + if n, ok := s.(syscall.Signal); ok { + code = 128 + int(n) + } + os.Exit(code) +} + +// ownSignals hands SIGINT and SIGTERM back, for a command that quits on +// them its own way: Wails ends the app through OnShutdown (which ends the +// probes, and installs a downloaded update), bubbletea leaves the TUI's +// screen as it found it, and each comes back to main, where the probes are +// ended too. Exiting first skipped all of that. SIGHUP stays: neither +// handles it, and the terminal closing would end magpie there and then, +// with its probes, out of the terminal's reach, left running. +func ownSignals() { + if onSignal == nil { + return + } + signal.Stop(onSignal) + for _, s := range caught { + if s == syscall.SIGHUP { + signal.Notify(onSignal, s) + } + } +} + +// interrupts counts the commands ending themselves on Ctrl+C. +var interrupts atomic.Int32 + +// interruptContext is signal.NotifyContext(…, os.Interrupt), for a command +// that ends itself on Ctrl+C rather than being ended by it. +func interruptContext() (context.Context, context.CancelFunc) { + interrupts.Add(1) + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt) + var once sync.Once + return ctx, func() { + stop() + once.Do(func() { interrupts.Add(-1) }) + } +} diff --git a/signals_test.go b/signals_test.go new file mode 100644 index 00000000..cc4aac9c --- /dev/null +++ b/signals_test.go @@ -0,0 +1,148 @@ +//go:build !windows + +package main + +import ( + "context" + "os" + "os/signal" + "syscall" + "testing" + "time" + + "github.com/yetone/magpie/internal/proc" +) + +// deaths records what the signal handler would die of, and takes the +// handler down after the test. +func deaths(t *testing.T, sigs ...os.Signal) chan os.Signal { + t.Helper() + for _, s := range sigs { + if signal.Ignored(s) { + t.Skipf("%v is ignored here", s) + } + } + died := make(chan os.Signal, 4) + old := die + die = func(s os.Signal) { died <- s } + t.Cleanup(func() { + if onSignal != nil { + signal.Stop(onSignal) + onSignal = nil + } + die = old + }) + return died +} + +func send(t *testing.T, s syscall.Signal) { + t.Helper() + if err := syscall.Kill(os.Getpid(), s); err != nil { + t.Fatal(err) + } +} + +func diesOf(t *testing.T, died chan os.Signal, want os.Signal) { + t.Helper() + select { + case s := <-died: + if s != want { + t.Fatalf("died of %v, want %v", s, want) + } + case <-time.After(3 * time.Second): + t.Fatalf("%v didn't end magpie", want) + } +} + +func livesOn(t *testing.T, died chan os.Signal, why string) { + t.Helper() + select { + case s := <-died: + t.Fatalf("magpie died of %v %s", s, why) + case <-time.After(300 * time.Millisecond): + } +} + +// a command that doesn't quit on a signal itself ends its probes and dies +// of the signal +func TestSignalEndsMagpie(t *testing.T) { + died := deaths(t, syscall.SIGTERM) + endProbesOnSignal() + send(t, syscall.SIGTERM) + diesOf(t, died, syscall.SIGTERM) +} + +// the app and the TUI take SIGINT and SIGTERM back: the signal reaches their +// own handling (Wails's, bubbletea's) and magpie doesn't end first, which +// skipped the app's OnShutdown and left the terminal on the TUI's screen +func TestOwnSignalsHandsThemBack(t *testing.T) { + died := deaths(t, syscall.SIGTERM) + endProbesOnSignal() + ownSignals() + theirs := make(chan os.Signal, 1) // the app's or the TUI's own + signal.Notify(theirs, syscall.SIGTERM) + defer signal.Stop(theirs) + send(t, syscall.SIGTERM) + select { + case <-theirs: + case <-time.After(3 * time.Second): + t.Fatal("the command's own handling didn't get SIGTERM") + } + livesOn(t, died, "before the command could quit its own way") +} + +// but not SIGHUP, which neither Wails nor bubbletea handles: the terminal +// closing under the app or the TUI still ends the probes before magpie +func TestOwnSignalsKeepsHangUp(t *testing.T) { + died := deaths(t, syscall.SIGHUP) + endProbesOnSignal() + ownSignals() + send(t, syscall.SIGHUP) + diesOf(t, died, syscall.SIGHUP) +} + +// a command ending itself on Ctrl+C is left to, even when it is done by the +// time the probes are ended (which can take a moment): that it was there is +// asked first +func TestInterruptWhileEndingProbes(t *testing.T) { + died := deaths(t, os.Interrupt) + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + proc.ProbeContext(ctx, "true") // never run: EndProbes waits a while on it + endProbesOnSignal() + _, stop := interruptContext() + send(t, syscall.SIGINT) + time.Sleep(300 * time.Millisecond) // the handler is ending the probes + stop() // and the command is done + select { + case s := <-died: + t.Fatalf("magpie died of %v in a command ending itself on Ctrl+C", s) + case <-time.After(2 * time.Second): + } +} + +// the TUI asks CLIs while it loads, before bubbletea listens for signals: +// one then is still magpie's, which ends those probes, and the TUI takes the +// signals once it is ready +func TestTUIKeepsSignalsWhileLoading(t *testing.T) { + died := deaths(t, syscall.SIGTERM) + theirs := make(chan os.Signal, 1) // bubbletea's, once it runs + signal.Notify(theirs, syscall.SIGTERM) + defer signal.Stop(theirs) + old := tuiRun + tuiRun = func(ready func()) error { + send(t, syscall.SIGTERM) // while loading + diesOf(t, died, syscall.SIGTERM) + <-theirs + ready() + send(t, syscall.SIGTERM) // once running + <-theirs + livesOn(t, died, "while the TUI quits on it its own way") + return nil + } + defer func() { tuiRun = old }() + endProbesOnSignal() + if err := runTUI(); err != nil { + t.Fatal(err) + } +} diff --git a/signals_unix.go b/signals_unix.go new file mode 100644 index 00000000..b63b252f --- /dev/null +++ b/signals_unix.go @@ -0,0 +1,23 @@ +//go:build !windows + +package main + +import ( + "os" + "os/signal" + "syscall" + "time" +) + +// raise sends s to magpie again with its handling put back to the default, +// which ends the process. +func raise(s os.Signal) { + n, ok := s.(syscall.Signal) + if !ok { + return + } + signal.Reset(s) + if syscall.Kill(os.Getpid(), n) == nil { + time.Sleep(time.Second) // it is on its way + } +} diff --git a/signals_windows.go b/signals_windows.go new file mode 100644 index 00000000..469337bc --- /dev/null +++ b/signals_windows.go @@ -0,0 +1,7 @@ +package main + +import "os" + +// raise has nothing to do: Windows can't send a process a signal, and die +// exits instead. +func raise(os.Signal) {}