diff --git a/free/ci/go.mod b/free/ci/go.mod index 08760f692..b009cedd0 100644 --- a/free/ci/go.mod +++ b/free/ci/go.mod @@ -20,7 +20,7 @@ require ( golang.org/x/mod v0.41.0 // indirect golang.org/x/net v0.60.0 // indirect golang.org/x/sync v0.23.0 // indirect - golang.org/x/sys v0.48.0 // indirect + golang.org/x/sys v0.48.0 golang.org/x/tools v0.50.0 // indirect modernc.org/libc v1.77.1 // indirect modernc.org/mathutil v1.7.1 // indirect diff --git a/free/ci/internal/exec/hygiene_unix.go b/free/ci/internal/exec/hygiene_unix.go index 98a5d6712..e48c1d10b 100644 --- a/free/ci/internal/exec/hygiene_unix.go +++ b/free/ci/internal/exec/hygiene_unix.go @@ -47,16 +47,16 @@ func KillAttemptGroup(home, attempt string) error { return nil } if groupAlive(marker.PID) { - identity, idErr := processIdentity(marker.PID) + same, known := sameProcess(marker.PID, marker.Identity) leaderGone := errors.Is(syscall.Kill(marker.PID, 0), syscall.ESRCH) switch { case leaderGone: // A live group whose leader has exited is the attempt's own: the kernel does not reuse a // PID that still names a process group. - case idErr != nil || identity == "": + case !known: // the leader exists but its start time is unreadable: it cannot be told from a reused PID return coded("E610", "attempt process identity unavailable") - case identity != marker.Identity: + case !same: // the leader PID now belongs to another process; its group is not ours to kill removeProcessMarker(home, attempt) return nil diff --git a/free/ci/internal/exec/process_identity_test.go b/free/ci/internal/exec/process_identity_test.go new file mode 100644 index 000000000..29bebae67 --- /dev/null +++ b/free/ci/internal/exec/process_identity_test.go @@ -0,0 +1,158 @@ +//go:build !windows + +package exec + +import ( + "context" + "os" + osexec "os/exec" + "strings" + "testing" + "time" +) + +// startGroup runs a sleeping process group and returns its leader. +func startGroup(t *testing.T) *osexec.Cmd { + t.Helper() + cmd := osexec.Command("sh", "-c", "sleep 30") + if err := prepareProcess(cmd); err != nil { + t.Fatal(err) + } + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + _ = killGroup(cmd.Process.Pid) + _ = cmd.Wait() + }) + return cmd +} + +func waitGroupGone(pid int) bool { + deadline := time.Now().Add(2 * time.Second) + for groupAlive(pid) && time.Now().Before(deadline) { + time.Sleep(20 * time.Millisecond) + } + return !groupAlive(pid) +} + +func TestNativeStartIDIsStableAndPerProcess(t *testing.T) { + self, ok := nativeStartID(os.Getpid()) + if !ok || !strings.HasPrefix(self, "st:") { + t.Fatalf("self id = %q ok=%v", self, ok) + } + if again, _ := nativeStartID(os.Getpid()); again != self { + t.Fatalf("id changed between reads: %q vs %q", self, again) + } + cmd := startGroup(t) + other, ok := nativeStartID(cmd.Process.Pid) + if !ok || other == self { + t.Fatalf("child id %q must differ from self %q", other, self) + } + if _, ok := nativeStartID(1 << 30); ok { + t.Fatal("a pid that cannot exist reported a start id") + } +} + +// A marker written by an older build holds ps lstart text; it must still match +// the live leader, so a legacy marker's group is killed and not leaked. +func TestKillAttemptGroupAcceptsLegacyMarker(t *testing.T) { + home := t.TempDir() + cmd := startGroup(t) + pid := cmd.Process.Pid + legacy, err := psIdentity(pid) + if err != nil || legacy == "" { + t.Fatalf("ps identity: %q %v", legacy, err) + } + writeMarkerWith(t, home, "legacy", "old", pid, legacy) + if err := KillAttemptGroup(home, "legacy"); err != nil { + t.Fatal(err) + } + _ = cmd.Wait() + if !waitGroupGone(pid) { + t.Fatal("group with a legacy marker survived KillAttemptGroup") + } +} + +// A marker whose identity differs from the live leader means the PID was +// reused: the group belongs to someone else and must not be killed. +func TestKillAttemptGroupSparesReusedPID(t *testing.T) { + for name, identity := range map[string]string{"native": "st:1", "legacy": "Thu Jan 1 00:00:00 1970"} { + t.Run(name, func(t *testing.T) { + home := t.TempDir() + cmd := startGroup(t) + pid := cmd.Process.Pid + writeMarkerWith(t, home, "reused", "old", pid, identity) + if err := KillAttemptGroup(home, "reused"); err != nil { + t.Fatal(err) + } + if !groupAlive(pid) { + t.Fatal("KillAttemptGroup killed a group whose leader PID was reused") + } + if _, err := os.Stat(markerPath(home, "reused")); !os.IsNotExist(err) { + t.Fatalf("stale marker retained: %v", err) + } + }) + } +} + +func TestSweepLegacyMarkerKillsAndReusedPIDRefuses(t *testing.T) { + home := t.TempDir() + legacyCmd := startGroup(t) + reusedCmd := startGroup(t) + legacy, err := psIdentity(legacyCmd.Process.Pid) + if err != nil { + t.Fatal(err) + } + writeMarkerWith(t, home, "legacy", "old", legacyCmd.Process.Pid, legacy) + writeMarkerWith(t, home, "reused", "old", reusedCmd.Process.Pid, "st:1") + e := &Executor{Stale: staleChecker{"old": true}} + if err := e.sweepProcessMarker(context.Background(), home, "current", "legacy"); err != nil { + t.Fatal(err) + } + _ = legacyCmd.Wait() + if !waitGroupGone(legacyCmd.Process.Pid) { + t.Fatal("legacy-marker group survived sweep") + } + if err := e.sweepProcessMarker(context.Background(), home, "current", "reused"); err == nil { + t.Fatal("sweep of a reused PID must refuse") + } + if !groupAlive(reusedCmd.Process.Pid) { + t.Fatal("sweep killed a group whose leader PID was reused") + } +} + +// The tracker must follow nested descendants, not only direct children. +func TestTrackerFollowsNestedDescendants(t *testing.T) { + cmd := osexec.Command("sh", "-c", "sh -c 'sleep 30 & sleep 30 & wait' & wait") + if err := prepareProcess(cmd); err != nil { + t.Fatal(err) + } + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + tr := trackDescendants(cmd.Process.Pid, "") + deadline := time.Now().Add(2 * time.Second) + for len(tr.pids()) < 3 && time.Now().Before(deadline) { + time.Sleep(20 * time.Millisecond) + } + got := len(tr.pids()) + tr.signalKill() + _ = killGroup(cmd.Process.Pid) + _ = cmd.Wait() + tr.stop() + if got < 3 { + t.Fatalf("tracker saw %d descendants, want at least 3", got) + } +} + +func writeMarkerWith(t *testing.T, home, attempt, coordinator string, pid int, identity string) { + t.Helper() + if err := os.MkdirAll(home+"/processes", 0o700); err != nil { + t.Fatal(err) + } + data := `{"pid":` + itoa(pid) + `,"coordinator":"` + coordinator + `","identity":"` + identity + `"}` + if err := os.WriteFile(markerPath(home, attempt), []byte(data), 0o600); err != nil { + t.Fatal(err) + } +} diff --git a/free/ci/internal/exec/process_marker.go b/free/ci/internal/exec/process_marker.go index 666dd2b44..bf84b8326 100644 --- a/free/ci/internal/exec/process_marker.go +++ b/free/ci/internal/exec/process_marker.go @@ -21,7 +21,39 @@ func markerPath(home, attempt string) string { return filepath.Join(home, "processes", attempt+".json") } +// processIdentity names a process by its start instant. The native form +// ("st:" prefix) needs no fork; markers written by older builds hold the ps +// lstart text, so psIdentity stays for comparing against those. func processIdentity(pid int) (string, error) { + if id, ok := nativeStartID(pid); ok { + return id, nil + } + return psIdentity(pid) +} + +// sameProcess compares the live start instant of pid with a marker's recorded +// identity, in the form the marker was written in: "st:" markers use the native +// reader, older markers hold ps lstart text. known is false when the live +// identity cannot be read in that form, which is not the same as "different". +func sameProcess(pid int, recorded string) (same, known bool) { + var now string + if strings.HasPrefix(recorded, "st:") { + id, ok := nativeStartID(pid) + if !ok { + return false, false + } + now = id + } else { + id, err := psIdentity(pid) + if err != nil || id == "" { + return false, false + } + now = id + } + return now == recorded, true +} + +func psIdentity(pid int) (string, error) { out, err := osexec.Command("ps", "-o", "lstart=", "-p", strconv.Itoa(pid)).Output() if err != nil { return "", err @@ -99,9 +131,8 @@ func (e *Executor) sweepProcessMarker(ctx context.Context, home, current, attemp if !stale { return nil } - identity, err := processIdentity(marker.PID) if groupAlive(marker.PID) { - if err == nil && identity != "" && identity != marker.Identity { + if same, known := sameProcess(marker.PID, marker.Identity); known && !same { // A different process has reused the leader PID. Do not kill its group. return coded("E610", "stale group identity changed") } diff --git a/free/ci/internal/exec/process_subreaper_linux.go b/free/ci/internal/exec/process_subreaper_linux.go index a6fb45890..12e305090 100644 --- a/free/ci/internal/exec/process_subreaper_linux.go +++ b/free/ci/internal/exec/process_subreaper_linux.go @@ -5,10 +5,8 @@ package exec import ( "bytes" "os" - osexec "os/exec" "path/filepath" "strconv" - "strings" "sync" "syscall" "time" @@ -49,23 +47,8 @@ func processExists(pid int) bool { return syscall.Kill(pid, 0) == nil || os.IsPermission(syscall.Kill(pid, 0)) } -func groupAlive(pgid int) bool { - out, err := osexec.Command("ps", "-A", "-o", "pgid=", "-o", "stat=").Output() - if err != nil { - return syscall.Kill(-pgid, 0) == nil - } - for _, line := range strings.Split(string(out), "\n") { - fields := strings.Fields(line) - if len(fields) != 2 { - continue - } - group, parseErr := strconv.Atoi(fields[0]) - if parseErr == nil && group == pgid && fields[1][0] != 'Z' { - return true - } - } - return false -} +// processGroupProbe is the signal-based fallback when /proc cannot be read. +func processGroupProbe(pgid int) bool { return syscall.Kill(-pgid, 0) == nil } func adoptedDescendants(parents map[int]int, attempt string) []int { if attempt == "" { diff --git a/free/ci/internal/exec/process_table_darwin.go b/free/ci/internal/exec/process_table_darwin.go new file mode 100644 index 000000000..53113d45c --- /dev/null +++ b/free/ci/internal/exec/process_table_darwin.go @@ -0,0 +1,33 @@ +//go:build darwin + +package exec + +import ( + "strconv" + + "golang.org/x/sys/unix" +) + +// processTable returns pid -> parent pid for every process, read with one +// sysctl (no fork, no cgo). +func processTable() (map[int]int, error) { + procs, err := unix.SysctlKinfoProcSlice("kern.proc.all") + if err != nil { + return nil, err + } + parents := make(map[int]int, len(procs)) + for i := range procs { + parents[int(procs[i].Proc.P_pid)] = int(procs[i].Eproc.Ppid) + } + return parents, nil +} + +// nativeStartID names the start instant of pid: it changes when the pid is reused. +func nativeStartID(pid int) (string, bool) { + p, err := unix.SysctlKinfoProc("kern.proc.pid", pid) + if err != nil || int(p.Proc.P_pid) != pid { + return "", false + } + tv := p.Proc.P_starttime + return "st:" + strconv.FormatInt(int64(tv.Sec), 10) + "." + strconv.FormatInt(int64(tv.Usec), 10), true +} diff --git a/free/ci/internal/exec/process_table_linux.go b/free/ci/internal/exec/process_table_linux.go new file mode 100644 index 000000000..ee7827262 --- /dev/null +++ b/free/ci/internal/exec/process_table_linux.go @@ -0,0 +1,116 @@ +//go:build linux + +package exec + +import ( + "os" + "strconv" + "strings" + "sync" +) + +// procStat is the part of /proc//stat the executor needs. +type procStat struct { + state byte + ppid, pgrp int + startTicks string +} + +// readProcStat parses /proc//stat. The command name may hold spaces and +// parentheses, so fields are counted from the last ')'. +func readProcStat(pid int) (procStat, bool) { + data, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/stat") + if err != nil { + return procStat{}, false + } + s := string(data) + i := strings.LastIndexByte(s, ')') + if i < 0 { + return procStat{}, false + } + f := strings.Fields(s[i+1:]) + if len(f) < 20 || f[0] == "" { + return procStat{}, false + } + ppid, e1 := strconv.Atoi(f[1]) + pgrp, e2 := strconv.Atoi(f[2]) + if e1 != nil || e2 != nil { + return procStat{}, false + } + return procStat{state: f[0][0], ppid: ppid, pgrp: pgrp, startTicks: f[19]}, true +} + +// procPIDs lists the numeric entries of /proc. +func procPIDs() ([]int, error) { + dir, err := os.Open("/proc") + if err != nil { + return nil, err + } + defer func() { _ = dir.Close() }() + names, err := dir.Readdirnames(-1) + if err != nil { + return nil, err + } + pids := make([]int, 0, len(names)) + for _, n := range names { + if pid, err := strconv.Atoi(n); err == nil && pid > 0 { + pids = append(pids, pid) + } + } + return pids, nil +} + +// processTable returns pid -> parent pid for every process, read from /proc +// without forking. +func processTable() (map[int]int, error) { + pids, err := procPIDs() + if err != nil { + return nil, err + } + parents := make(map[int]int, len(pids)) + for _, pid := range pids { + if st, ok := readProcStat(pid); ok { + parents[pid] = st.ppid + } + } + return parents, nil +} + +// nativeStartID names the start instant of pid: it changes when the pid is reused. +func nativeStartID(pid int) (string, bool) { + st, ok := readProcStat(pid) + if !ok { + return "", false + } + return "st:" + st.startTicks + bootSuffix(), true +} + +var ( + bootOnce sync.Once + bootID string +) + +// bootSuffix ties start ticks (counted from boot) to one boot, so a marker kept +// across a reboot cannot match a new process that has the same pid and ticks. +func bootSuffix() string { + bootOnce.Do(func() { + if b, err := os.ReadFile("/proc/sys/kernel/random/boot_id"); err == nil { + bootID = "@" + strings.TrimSpace(string(b)) + } + }) + return bootID +} + +// groupAlive reports whether any non-zombie process is in group pgid. +func groupAlive(pgid int) bool { + pids, err := procPIDs() + if err != nil { + return processGroupProbe(pgid) + } + for _, pid := range pids { + if st, ok := readProcStat(pid); ok && st.pgrp == pgid && st.state != 'Z' { + return true + } + } + return false +} diff --git a/free/ci/internal/exec/process_tree_nops_test.go b/free/ci/internal/exec/process_tree_nops_test.go new file mode 100644 index 000000000..b1687e3c6 --- /dev/null +++ b/free/ci/internal/exec/process_tree_nops_test.go @@ -0,0 +1,45 @@ +//go:build !windows + +package exec + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// D-1008: the descendant tracker must not fork ps. Each fork cost about 10 ms, +// ran three times per snapshot and dominated the V15 tiny-repo run time. +func TestTrackerDoesNotForkPS(t *testing.T) { + shim := t.TempDir() + log := filepath.Join(shim, "ps.calls") + script := "#!/bin/sh\necho \"$@\" >> " + log + "\nexit 0\n" + if err := os.WriteFile(filepath.Join(shim, "ps"), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", shim+string(os.PathListSeparator)+os.Getenv("PATH")) + res := (&Executor{Grace: 100 * time.Millisecond}).Run(context.Background(), JobSpec{ + AttemptID: "no-ps", CoordinatorID: "coord", Home: t.TempDir(), + Command: []string{"sh", "-c", "sleep 0.3 & sleep 0.2; wait"}, + }) + if res.Err != nil || res.ExitCode != 0 { + t.Fatalf("run: %+v", res) + } + if b, _ := os.ReadFile(log); strings.TrimSpace(string(b)) != "" { + t.Fatalf("tracker forked ps %d time(s): %q", strings.Count(string(b), "\n"), b) + } +} + +// The table must list this process, its parent, and a live child. +func TestProcessTableSeesChildren(t *testing.T) { + parents, err := processTable() + if err != nil { + t.Fatal(err) + } + if got := parents[os.Getpid()]; got != os.Getppid() { + t.Fatalf("ppid of self = %d, want %d", got, os.Getppid()) + } +} diff --git a/free/ci/internal/exec/process_tree_unix.go b/free/ci/internal/exec/process_tree_unix.go index 15acf9954..eeb2a1c52 100644 --- a/free/ci/internal/exec/process_tree_unix.go +++ b/free/ci/internal/exec/process_tree_unix.go @@ -47,59 +47,82 @@ func (t *processTree) stop() { reapDescendants(t.pids()) } +// snapshot reads the process table and adds every descendant of the root (and +// every adopted orphan) to the seen set. A pass that found new processes is +// repeated (at most three passes) so forks made while the table was being read +// are caught. The table comes from /proc or sysctl, which cost no fork; ps is +// only the fallback when those fail. func (t *processTree) snapshot() { - // ps is available on supported Unix hosts. Repeated walks capture forks that - // occur while a prior snapshot is being read. for i := 0; i < 3; i++ { - out, err := osexec.Command("ps", "-A", "-o", "pid=", "-o", "ppid=").Output() + parents, err := processTable() if err != nil { + if parents, err = psProcessTable(); err != nil { + return + } + } + if !t.absorb(parents) { return } - parents := make(map[int]int) - for _, line := range strings.Split(string(out), "\n") { - fields := strings.Fields(line) - if len(fields) != 2 { + } +} + +// absorb adds the descendants found in parents to the seen set and reports +// whether it added any. +func (t *processTree) absorb(parents map[int]int) bool { + t.mu.Lock() + defer t.mu.Unlock() + known := map[int]struct{}{t.root: {}} + for pid := range t.seen { + known[pid] = struct{}{} + } + changed := false + for _, pid := range adoptedDescendants(parents, t.attempt) { + known[pid] = struct{}{} + if _, ok := t.seen[pid]; !ok { + t.seen[pid] = struct{}{} + changed = true + } + } + for { + added := false + for pid, ppid := range parents { + if _, ok := known[ppid]; !ok { continue } - pid, e1 := strconv.Atoi(fields[0]) - ppid, e2 := strconv.Atoi(fields[1]) - if e1 == nil && e2 == nil { - parents[pid] = ppid + if _, ok := known[pid]; ok { + continue } - } - t.mu.Lock() - known := map[int]struct{}{t.root: {}} - for pid := range t.seen { - known[pid] = struct{}{} - } - for _, pid := range adoptedDescendants(parents, t.attempt) { known[pid] = struct{}{} t.seen[pid] = struct{}{} + added = true + changed = true } - changed := false - for { - added := false - for pid, ppid := range parents { - if _, ok := known[ppid]; !ok { - continue - } - if _, ok := known[pid]; ok { - continue - } - known[pid] = struct{}{} - t.seen[pid] = struct{}{} - added = true - changed = true - } - if !added { - break - } + if !added { + return changed } - t.mu.Unlock() - if !changed { - return + } +} + +// psProcessTable is the fork-based fallback for hosts where the native table +// is unreadable. +func psProcessTable() (map[int]int, error) { + out, err := osexec.Command("ps", "-A", "-o", "pid=", "-o", "ppid=").Output() + if err != nil { + return nil, err + } + parents := make(map[int]int) + for _, line := range strings.Split(string(out), "\n") { + fields := strings.Fields(line) + if len(fields) != 2 { + continue + } + pid, e1 := strconv.Atoi(fields[0]) + ppid, e2 := strconv.Atoi(fields[1]) + if e1 == nil && e2 == nil { + parents[pid] = ppid } } + return parents, nil } func (t *processTree) pids() []int { diff --git a/free/ci/internal/exec/process_tree_windows.go b/free/ci/internal/exec/process_tree_windows.go index 3b7f46e85..6a066dc9d 100644 --- a/free/ci/internal/exec/process_tree_windows.go +++ b/free/ci/internal/exec/process_tree_windows.go @@ -10,3 +10,4 @@ func (*processTree) signalTerminate() {} func (*processTree) signalKill() {} func (*processTree) anyAlive() bool { return false } func (*processTree) stop() {} +func nativeStartID(int) (string, bool) { return "", false }