From 851ae4274eeab3f30a43f4d097dd10d3fe0bd04e Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 13:09:07 +0300 Subject: [PATCH] fix(scaffold): a timed-out cli call gets SIGTERM before SIGKILL The cli Runner used exec.CommandContext's default cancel, an immediate SIGKILL to the CLI's pid. The CLI never got to stop what it had started itself, so its children ran on, reparented to init. Measured on the published io.pilot.aegis 0.1.3 bundles: `aegis.exec install-models` with a slow curl hit the 60 s method timeout, and the curl was still running with ppid=1 afterwards (darwin/arm64 pid 21800, linux/arm64 pid 30, linux/amd64 pid 39). The call also took 65 s, because the orphan held the stderr pipe until WaitDelay ran out. cmd.Cancel now sends SIGTERM. exec's existing WaitDelay (killGrace, 5 s) still escalates to SIGKILL, so a CLI that ignores SIGTERM costs at most 5 s more than before and can't hang a call. TestCLICancelSendsSIGTERMFirstE2E builds a real cli adapter over a script with a 1 s method timeout: - graceful: the script traps SIGTERM and kills its own child. On main the trap never runs and the child outlives the call; now both happen. - stubborn: the script ignores SIGTERM. It is still SIGKILLed, about 6 s after the call started (1 s timeout + 5 s killGrace). Both subtests fail on main and pass here (darwin/arm64, 3 runs). Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/CLI-ADAPTER.md | 3 +- .../scaffold/templates/client_cli.go.tmpl | 13 +- .../zz_cli_cancel_sigterm_e2e_test.go | 214 ++++++++++++++++++ 3 files changed, 227 insertions(+), 3 deletions(-) create mode 100644 internal/scaffold/zz_cli_cancel_sigterm_e2e_test.go diff --git a/docs/CLI-ADAPTER.md b/docs/CLI-ADAPTER.md index fee63bc..4ada4bf 100644 --- a/docs/CLI-ADAPTER.md +++ b/docs/CLI-ADAPTER.md @@ -50,7 +50,8 @@ The generated `exec.go` is defensive by default: `{"stdout","stderr","exit","truncated"}` rather than an opaque error, so the caller sees everything the CLI produced. Only spawn failures (binary missing) and timeouts surface as IPC errors; the per-method `timeout`/`duration` bounds - the run and the child is killed on cancel. + the run. On cancel the child gets SIGTERM, so it can stop what it started + itself, and SIGKILL 5 s later if it is still running. ## Why HTTP works today and CLI doesn't diff --git a/internal/scaffold/templates/client_cli.go.tmpl b/internal/scaffold/templates/client_cli.go.tmpl index 9edcb5f..09e1c8b 100644 --- a/internal/scaffold/templates/client_cli.go.tmpl +++ b/internal/scaffold/templates/client_cli.go.tmpl @@ -41,6 +41,7 @@ import ( "regexp" "sort" "strings" + "syscall" "time" ) @@ -48,8 +49,9 @@ import ( // the adapter's memory. Output past the cap is dropped and flagged truncated. const maxOutputBytes = 4 << 20 // 4 MiB -// killGrace is how long the child has to exit after its context is cancelled -// before Wait abandons it; pairs with the SIGKILL exec sends on cancel. +// killGrace is how long the child has, after the SIGTERM sent when its call is +// cancelled, to stop before exec escalates to SIGKILL and stops waiting on its +// output pipes. const killGrace = 5 * time.Second // baseEnvKeys is the minimal environment every fronted CLI may see. Anything an @@ -114,6 +116,13 @@ func (r *Runner) Run(ctx context.Context, spec Spec, call Call) (json.RawMessage cmd := exec.CommandContext(ctx, r.base[0], args...) cmd.Env = r.childEnv() + // When the call's context ends first (the method timeout, for one), ask the + // CLI to stop with SIGTERM; exec sends SIGKILL only if it is still running + // killGrace later. exec's default, an immediate SIGKILL, gives the CLI no + // chance to stop what it started itself: every timed-out + // `aegis.exec install-models` left its curl download running, reparented + // to init. + cmd.Cancel = func() error { return cmd.Process.Signal(syscall.SIGTERM) } cmd.WaitDelay = killGrace if call.Stdin != "" { cmd.Stdin = strings.NewReader(call.Stdin) diff --git a/internal/scaffold/zz_cli_cancel_sigterm_e2e_test.go b/internal/scaffold/zz_cli_cancel_sigterm_e2e_test.go new file mode 100644 index 0000000..a0add89 --- /dev/null +++ b/internal/scaffold/zz_cli_cancel_sigterm_e2e_test.go @@ -0,0 +1,214 @@ +//go:build !windows + +package scaffold + +import ( + "encoding/json" + "net" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/pilot-protocol/app-store/pkg/ipc" +) + +// TestCLICancelSendsSIGTERMFirstE2E checks what a cli adapter does to its CLI +// when a call's deadline passes. It used to SIGKILL the CLI outright (exec's +// default), so a CLI never got to stop what it had started itself: with +// io.pilot.aegis, every timed-out `aegis.exec install-models` left its curl +// download running, reparented to init. Now the CLI gets SIGTERM first and +// SIGKILL only killGrace (5 s) later. +// +// - graceful: a CLI that traps SIGTERM and stops its own child. After the +// 1 s timeout it must have seen SIGTERM, and its child must be gone. +// - stubborn: a CLI that ignores SIGTERM. It must still be SIGKILLed about +// killGrace after the timeout, and the call must return. +func TestCLICancelSendsSIGTERMFirstE2E(t *testing.T) { + if testing.Short() { + t.Skip("builds and runs a real adapter binary; skipped under -short") + } + if _, err := exec.LookPath("go"); err != nil { + t.Skip("go toolchain not available") + } + + root := t.TempDir() + tool := filepath.Join(root, "slowtool") + // $1 = directory for pid/marker files, $2 = mode. + script := `#!/bin/sh +dir=$1 +case "$2" in +graceful) + sleep 600 >/dev/null 2>&1 & + kid=$! + echo "$kid" > "$dir/grandchild.pid" + trap 'echo term > "$dir/got-term"; kill "$kid"; exit 143' TERM + wait + ;; +stubborn) + trap '' TERM + echo $$ > "$dir/cli.pid" + sleep 600 >/dev/null 2>&1 & + echo $! > "$dir/grandchild.pid" + while :; do sleep 1; done + ;; +esac +` + if err := os.WriteFile(tool, []byte(script), 0o755); err != nil { + t.Fatal(err) + } + + spec := ` +id: io.pilot.slowtool +app_version: 0.1.0 +description: "Fronts slowtool." +namespace: slowtool +backend: + type: cli + command: ["` + tool + `"] +methods: + - name: slowtool.run + summary: "Passthrough with a short deadline." + timeout: 1s + cli: {passthrough: true} +` + cfg := parseSpec(t, spec) + proj := filepath.Join(root, "proj") + if _, err := Generate(cfg, proj); err != nil { + t.Fatalf("generate: %v", err) + } + if sum, err := os.ReadFile(filepath.Join("..", "..", "go.sum")); err == nil { + _ = os.WriteFile(filepath.Join(proj, "go.sum"), sum, 0o644) + } + bin := filepath.Join(root, "adapter") + build := exec.Command("go", "build", "-o", bin, "./cmd/"+cfg.BinaryName) + build.Dir = proj + build.Env = append(os.Environ(), "GOFLAGS=-mod=mod") + if out, err := build.CombinedOutput(); err != nil { + t.Fatalf("build adapter: %v\n%s", err, out) + } + + // A short socket path: t.TempDir() can exceed sun_path (104 bytes on macOS). + sockDir, err := os.MkdirTemp("/tmp", "sigterm-e2e-") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.RemoveAll(sockDir) }) + sock := filepath.Join(sockDir, "app.sock") + adapter := exec.Command(bin, "--socket", sock, "--manifest", filepath.Join(proj, "manifest.json")) + adapter.Stderr = os.Stderr + if err := adapter.Start(); err != nil { + t.Fatalf("start adapter: %v", err) + } + t.Cleanup(func() { _ = adapter.Process.Kill(); _, _ = adapter.Process.Wait() }) + deadline := time.Now().Add(10 * time.Second) + for { + if c, err := net.Dial("unix", sock); err == nil { + c.Close() + break + } + if time.Now().After(deadline) { + t.Fatal("adapter never listened") + } + time.Sleep(20 * time.Millisecond) + } + + // call runs one passthrough call and returns how long it took. The reply + // is only logged: what matters is what happened to the processes. + call := func(dir, mode string) time.Duration { + t.Helper() + conn, err := net.DialTimeout("unix", sock, 3*time.Second) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer conn.Close() + _ = conn.SetDeadline(time.Now().Add(30 * time.Second)) + args, _ := json.Marshal(map[string][]string{"args": {dir, mode}}) + start := time.Now() + var out json.RawMessage + err = ipc.Call(conn, "slowtool.run", json.RawMessage(args), &out) + took := time.Since(start) + t.Logf("%s: reply %s, err %v, after %v", mode, out, err, took) + return took + } + + t.Run("graceful", func(t *testing.T) { + dir := t.TempDir() + took := call(dir, "graceful") + kid := sigtermTestPID(t, filepath.Join(dir, "grandchild.pid")) + t.Cleanup(func() { _ = syscall.Kill(kid, syscall.SIGKILL) }) + + if _, err := os.Stat(filepath.Join(dir, "got-term")); err != nil { + t.Errorf("the CLI never got SIGTERM when its 1s deadline passed (call took %v)", took) + } + if !sigtermTestGone(kid, 3*time.Second) { + t.Errorf("the CLI's own child (pid %d) outlived the timed-out call", kid) + } + if took > 4*time.Second { + t.Errorf("call took %v; a CLI that stops on SIGTERM should end right after the 1s deadline", took) + } + }) + + t.Run("stubborn", func(t *testing.T) { + dir := t.TempDir() + took := call(dir, "stubborn") + cli := sigtermTestPID(t, filepath.Join(dir, "cli.pid")) + kid := sigtermTestPID(t, filepath.Join(dir, "grandchild.pid")) + t.Cleanup(func() { + _ = syscall.Kill(cli, syscall.SIGKILL) + _ = syscall.Kill(kid, syscall.SIGKILL) + }) + + if !sigtermTestGone(cli, 3*time.Second) { + t.Errorf("a CLI that ignores SIGTERM (pid %d) was never SIGKILLed", cli) + } + if took < 5*time.Second || took > 10*time.Second { + t.Errorf("call took %v; want the 1s deadline plus the 5s killGrace", took) + } + }) +} + +func sigtermTestPID(t *testing.T, path string) int { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for { + if b, err := os.ReadFile(path); err == nil { + if pid, err := strconv.Atoi(strings.TrimSpace(string(b))); err == nil { + return pid + } + } + if time.Now().After(deadline) { + t.Fatalf("no pid in %s", path) + } + time.Sleep(20 * time.Millisecond) + } +} + +// sigtermTestGone reports whether pid exits within d. A zombie counts as gone: +// it has exited and only waits to be reaped by its new parent (which, in a +// container without an init, may never happen). +func sigtermTestGone(pid int, d time.Duration) bool { + deadline := time.Now().Add(d) + for { + if syscall.Kill(pid, 0) != nil { + return true + } + if stat, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/stat"); err == nil { + // Linux: the state letter follows the ")" that closes the command name. + if i := strings.LastIndexByte(string(stat), ')'); i >= 0 && i+2 < len(stat) && (stat[i+2] == 'Z' || stat[i+2] == 'X') { + return true + } + } else if out, err := exec.Command("ps", "-o", "stat=", "-p", strconv.Itoa(pid)).Output(); err == nil && + strings.HasPrefix(strings.TrimSpace(string(out)), "Z") { + return true + } + if time.Now().After(deadline) { + return false + } + time.Sleep(20 * time.Millisecond) + } +}