diff --git a/internal/buildtools/bug4066_harness_guard_test.go b/internal/buildtools/bug4066_harness_guard_test.go index eac43456f..9d548ae6a 100644 --- a/internal/buildtools/bug4066_harness_guard_test.go +++ b/internal/buildtools/bug4066_harness_guard_test.go @@ -60,14 +60,18 @@ func testTempBase(t *testing.T) string { } // tempScope is where THIS test's temp dirs live: the shared base and the -// test's top-level name. t.TempDir() names its dirs "", and a -// subtest's "_", so the top-level name is a prefix of -// both and of no other test's (BUG-4066 r2). +// test's top-level name. t.TempDir() names its dir for the FULL test name cut +// to 64 bytes, symbols such as "/" dropped, then random digits (Go's +// testing.go makeTempDir). So a subtest's is "" and digits, and +// a top-level name of 64 bytes or more appears only as its first 64. top is +// cut the same way, or a long test name is refused by its own temp dir +// (BUG-4068, found when its tests first ran under this guard). type tempScope struct{ base, top string } func testTempScope(t *testing.T) tempScope { t.Helper() - return tempScope{testTempBase(t), strings.SplitN(t.Name(), "/", 2)[0]} + top := strings.SplitN(t.Name(), "/", 2)[0] + return tempScope{testTempBase(t), top[:min(len(top), 64)]} } // insideTestTempDir reports whether p is inside this test's temp dir: under diff --git a/internal/buildtools/bug4068_pidfile_test.go b/internal/buildtools/bug4068_pidfile_test.go new file mode 100644 index 000000000..b2290362d --- /dev/null +++ b/internal/buildtools/bug4068_pidfile_test.go @@ -0,0 +1,262 @@ +package buildtools + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "testing" +) + +// BUG-4068: section 4 of install-refresh.sh removes /pad.pid only +// when it names the pid this refresh STOPPED or a pid that is provably DEAD. +// It keeps the file when it names any live process (a sibling server sharing +// the data dir, or a reused pid), when liveness is unknown (EPERM), and when +// it does not parse. The testdata stub never writes pad.pid itself, so after a +// refresh the file is either the fixture's bytes or absent. + +type pidFileCase struct { + // body builds the pad.pid content from the stopped stub's pid. + body func(stopped int) string + // server starts a stub on the port first, so the refresh stops it. + server bool +} + +func jsonRecord(pid int, exe string) string { + return fmt.Sprintf("{\n \"pid\": %d,\n \"started_at\": \"2026-10-25T00:00:00Z\",\n \"exe\": %q\n}\n", pid, exe) +} + +// liveSibling starts a long-lived process this test owns, unrelated to the +// stub, and returns its pid. +func liveSibling(t *testing.T) int { + t.Helper() + cmd := exec.Command("sleep", "600") + if err := cmd.Start(); err != nil { + t.Fatalf("start sibling: %v", err) + } + t.Cleanup(func() { _ = cmd.Process.Kill(); _ = cmd.Wait() }) + return cmd.Process.Pid +} + +// runPidFileRefresh runs the real script against a stub on the test's own +// port and returns the pad.pid path and the fixture bytes written to it. +func runPidFileRefresh(t *testing.T, c pidFileCase) (pidFile, written string) { + t.Helper() + requireScriptDeps(t) + home, dir := t.TempDir(), t.TempDir() + name := uniqueName(t) + defer killStub(t, name) + port := freePort(t) + const commit = "abc1234" + + built := installStub(t, dir, name) + installed := filepath.Join(home, "bin", name) + e := stubEnv{version: "pad version dev (" + commit + " x)", healthy: "1", + argvLog: filepath.Join(home, "argv.log"), port: port, home: home, + dataDir: filepath.Join(t.TempDir(), "data")} + if err := os.MkdirAll(e.dataDir, 0o755); err != nil { + t.Fatal(err) + } + stopped := 0 + if c.server { + pre := exec.Command(built, "server", "start", "--host", "127.0.0.1") + pre.Env = e.env() + startPreServer(t, pre, "127.0.0.1", port) + stopped = pre.Process.Pid + } + pidFile = filepath.Join(e.dataDir, "pad.pid") + written = c.body(stopped) + if err := os.WriteFile(pidFile, []byte(written), 0o644); err != nil { + t.Fatal(err) + } + res := runScript(t, e, built, installed, commit) + if res.err != nil { + t.Fatalf("refresh failed: %v\nstdout=%s\nstderr=%s", res.err, res.stdout, res.stderr) + } + return pidFile, written +} + +func wantRemoved(t *testing.T, pidFile string) { + t.Helper() + if b, err := os.ReadFile(pidFile); err == nil { + t.Errorf("pad.pid survived the refresh with content %q", b) + } else if !os.IsNotExist(err) { + t.Errorf("stat pad.pid: %v", err) + } +} + +func wantKept(t *testing.T, pidFile, written string) { + t.Helper() + b, err := os.ReadFile(pidFile) + if err != nil { + t.Fatalf("pad.pid was removed (%v); it must be kept byte-identical", err) + } + if string(b) != written { + t.Errorf("pad.pid changed: got %q, want %q", b, written) + } +} + +func TestInstallRefresh_PIDFile_RemovesAJSONRecordNamingTheStoppedServer(t *testing.T) { + requireScriptDeps(t) + f, _ := runPidFileRefresh(t, pidFileCase{server: true, body: func(p int) string { return jsonRecord(p, "/x/pad") }}) + wantRemoved(t, f) +} + +func TestInstallRefresh_PIDFile_KeepsARecordNamingALiveSiblingWhileTheServerIsStopped(t *testing.T) { + requireScriptDeps(t) + sib := liveSibling(t) + f, w := runPidFileRefresh(t, pidFileCase{server: true, body: func(int) string { return jsonRecord(sib, "/x/pad") }}) + wantKept(t, f, w) +} + +func TestInstallRefresh_PIDFile_RemovesALegacyBareIntegerNamingTheStoppedServer(t *testing.T) { + requireScriptDeps(t) + f, _ := runPidFileRefresh(t, pidFileCase{server: true, body: func(p int) string { return strconv.Itoa(p) + "\n" }}) + wantRemoved(t, f) +} + +func TestInstallRefresh_PIDFile_RemovesADeadPidWhenNothingWasStopped(t *testing.T) { + requireScriptDeps(t) + dead := reapedPid(t) + f, _ := runPidFileRefresh(t, pidFileCase{body: func(int) string { return jsonRecord(dead, "/x/pad") }}) + wantRemoved(t, f) +} + +func TestInstallRefresh_PIDFile_KeepsALiveSiblingWhenNothingWasStopped(t *testing.T) { + requireScriptDeps(t) + sib := liveSibling(t) + f, w := runPidFileRefresh(t, pidFileCase{body: func(int) string { return jsonRecord(sib, "/x/pad") }}) + wantKept(t, f, w) +} + +func TestInstallRefresh_PIDFile_KeepsAFileThatDoesNotParse(t *testing.T) { + requireScriptDeps(t) + f, w := runPidFileRefresh(t, pidFileCase{body: func(int) string { return "garbage\n" }}) + wantKept(t, f, w) +} + +func TestInstallRefresh_PIDFile_RemovesTheStoppedPidEvenWhenTheExeDiffersBecauseTheStopProvedTheProcess(t *testing.T) { + requireScriptDeps(t) + f, _ := runPidFileRefresh(t, pidFileCase{server: true, body: func(p int) string { return jsonRecord(p, "/somewhere/else/pad") }}) + wantRemoved(t, f) +} + +// scriptFns runs prog after the script's own numeric, pid_file_pid, +// pid_liveness and pid_file_removable, extracted with sed: the rest of the +// script never runs. pre is bash placed between the functions and prog, used +// to stand in a fake kill for the builtin. +func scriptFns(t *testing.T, pre, prog string) string { + t.Helper() + requireScriptDeps(t) + script := scriptPath(t) + var fns strings.Builder + for _, expr := range []string{`/^numeric() /p`, `/^pid_file_pid() {/,/^}/p`, `/^pid_liveness() {/,/^}/p`, `/^pid_file_removable() {/,/^}/p`} { + out, err := exec.Command("sed", "-n", expr, script).Output() + if err != nil || len(out) == 0 { + t.Fatalf("extract %q from the script: %v", expr, err) + } + fns.Write(out) + } + cmd := exec.Command("bash", "-c", fns.String()+"\n"+pre+"\n"+prog) + cmd.Env = os.Environ() + out, err := cmd.Output() + if err != nil { + t.Fatalf("run extracted functions: %v", err) + } + return strings.TrimSpace(string(out)) +} + +// reapedPid is a pid that existed and is gone now. +func reapedPid(t *testing.T) int { + t.Helper() + cmd := exec.Command("true") + if err := cmd.Run(); err != nil { + t.Fatal(err) + } + return cmd.Process.Pid +} + +// The reader checks lines against the SHAPE internal/cli/pidfile.go writes +// (BUG-4068 r1 items 1 and 3, r2 items 1 and 2): braces on their own lines, +// each inner line STARTING like a two-space-indent quoted key (the rest of +// the line is unchecked), one pid, at most 4194304; or a bare integer. It is a +// line-shape check, not full JSON validation. The REFUSED rows do not parse, +// and the file is kept. +func TestInstallRefresh_PIDFile_ReadsOnlyWhatPadWrites(t *testing.T) { + for _, tc := range []struct{ name, body, want string }{ + {"MarshalIndent", jsonRecord(4242, "/x/pad"), "4242"}, + {"CRLF", strings.ReplaceAll(jsonRecord(4242, "/x/pad"), "\n", "\r\n"), "4242"}, + {"bare", " 777 \n", "777"}, + {"bare leading zeros", "007\n", "7"}, + {"duplicate pid keys", "{\n \"pid\": 5,\n \"pid\": 6\n}\n", "REFUSED"}, + {"nested pid", "{\n \"x\": {\n \"pid\": 5\n }\n}\n", "REFUSED"}, + {"compact one-line JSON is not what pad writes", "{\"pid\": 4242}\n", "REFUSED"}, + {"inner whitespace", "1 2\n", "REFUSED"}, + {"more than 10 digits", "12345678901\n", "REFUSED"}, + {"zero", "0\n", "REFUSED"}, + {"negative", "-5\n", "REFUSED"}, + {"garbage", "garbage\n", "REFUSED"}, + {"missing closing brace", "{\n \"pid\": 5,\n garbage\n", "REFUSED"}, + {"garbage after the pid line", "{\n \"pid\": 5,\ngarbage\n}\n", "REFUSED"}, + {"a CR inside a bare pid", "1\r2\n", "REFUSED"}, + {"Linux's largest pid_max", "4194304\n", "4194304"}, + {"above any pid_max", "4194305\n", "REFUSED"}, + {"would wrap as a pid_t", "2147483648\n", "REFUSED"}, + } { + t.Run(tc.name, func(t *testing.T) { + f := filepath.Join(t.TempDir(), "pad.pid") + if err := os.WriteFile(f, []byte(tc.body), 0o644); err != nil { + t.Fatal(err) + } + got := scriptFns(t, "", fmt.Sprintf("pid_file_pid %q || printf REFUSED", f)) + if got != tc.want { + t.Errorf("pid_file_pid(%q) = %q, want %q", tc.body, got, tc.want) + } + }) + } +} + +// DEAD is ESRCH only (BUG-4068 r1 item 2): a fake kill prints each message +// the builtin can, so EPERM and anything unrecognised are not dead. +func TestInstallRefresh_PIDFile_OnlyESRCHIsDead(t *testing.T) { + for _, tc := range []struct{ name, msg, want string }{ + {"EPERM", "kill: (5) - Operation not permitted", "unknown"}, + {"ESRCH", "kill: (5) - No such process", "dead"}, + {"unrecognised", "kill: something else", "unknown"}, + } { + t.Run(tc.name, func(t *testing.T) { + fake := fmt.Sprintf("kill() { echo %q >&2; return 1; }", tc.msg) + if got := scriptFns(t, fake, "pid_liveness 5"); got != tc.want { + t.Errorf("pid_liveness with %q = %q, want %q", tc.msg, got, tc.want) + } + }) + } + if got := scriptFns(t, "", fmt.Sprintf("pid_liveness %d", reapedPid(t))); got != "dead" { + t.Errorf("the real kill on a reaped pid: %q, want dead", got) + } + if got := scriptFns(t, "", fmt.Sprintf("pid_liveness %d", os.Getpid())); got != "alive" { + t.Errorf("the real kill on this test's own pid: %q, want alive", got) + } +} + +// The stopped-pid leg on its own (BUG-4068 r1 item 5). The e2e stopped-server +// rows reach it too: their pre-server is this test's own child, a zombie +// after the TERM until cleanup reaps it, and kill -0 on a zombie succeeds, so +// the dead leg cannot answer for them. This row does not depend on reaping: a +// LIVE pid named as the stopped pid is removed, and with no stopped pid the +// same file is kept. +func TestInstallRefresh_PIDFile_TheStoppedPidLegRemovesEvenALivePid(t *testing.T) { + sib := liveSibling(t) + f := filepath.Join(t.TempDir(), "pad.pid") + if err := os.WriteFile(f, []byte(jsonRecord(sib, "/x/pad")), 0o644); err != nil { + t.Fatal(err) + } + if got := scriptFns(t, "", fmt.Sprintf("pid_file_removable %q %d && echo REMOVE", f, sib)); got != "REMOVE" { + t.Errorf("a file naming the stopped pid %d was not removable: %q", sib, got) + } + if got := scriptFns(t, "", fmt.Sprintf("pid_file_removable %q '' && echo REMOVE; true", f)); !strings.Contains(got, "is alive") { + t.Errorf("with no stopped pid, a live pid's file was not kept as alive: %q", got) + } +} diff --git a/scripts/install-refresh.sh b/scripts/install-refresh.sh index 6223ede22..25ad3e373 100755 --- a/scripts/install-refresh.sh +++ b/scripts/install-refresh.sh @@ -397,6 +397,7 @@ listener_pids() { } TARGET_PID="" +STOPPED_PID="" LISTENERS=() HIDDEN="" while read -r lp; do @@ -710,7 +711,13 @@ if [ -n "$TARGET_PID" ]; then die "pid $TARGET_PID no longer runs the captured server. Nothing was stopped or installed; re-run." fi - kill -TERM "$TARGET_PID" 2>/dev/null || true + # The pid this refresh signalled: the only pid whose pad.pid record + # section 4 may treat as its own (BUG-4068). Recorded only when the + # signal was delivered, so a pid that exited and was reused after + # still_target is never claimed. + if kill -TERM "$TARGET_PID" 2>/dev/null; then + STOPPED_PID="$TARGET_PID" + fi for _ in 1 2 3 4 5; do kill -0 "$TARGET_PID" 2>/dev/null || break sleep 1 @@ -727,7 +734,104 @@ STAGED="" # cfg.PIDFile() is /pad.pid (config.go), claimed by `pad server # start` (cmd/pad/cmd_server.go); not $HOME/.pad under PAD_DATA_DIR (BUG-4064). # DATA_DIR is section 2a's, anchored like the log. -rm -f "$DATA_DIR/pad.pid" +# +# The file is removed only when it is provably not someone else's (BUG-4068): +# it names the pid this refresh stopped, or a pid that is provably DEAD. It is +# kept when it names any live process, a sibling server sharing the data dir +# or a reused pid, and when its liveness is unknown (EPERM) or it does not +# parse. A kept stale file is the safe failure: `pad server stop` refuses a +# record it cannot prove (BUG-2969). +pid_file_pid() { + # The pid in pad.pid, read by checking each line against the SHAPE that + # internal/cli/pidfile.go writes (BUG-4068 r1, r2). This is a line-shape + # check, not full JSON validation: + # - the MarshalIndent record: first non-blank line exactly "{", last + # exactly "}", and each line between must START like a two-space-indent + # quoted key (' "' and a key character); the rest of such a line is + # unchecked. Exactly one of them is " \"pid\": N" (with or without a + # trailing comma), and "pid" appears nowhere else in the file; + # - or the legacy bare integer: one non-blank line, surrounding space only. + # A "\r" is dropped only at a line end. The pid is at most 10 digits, + # checked before any arithmetic, and at most 4194304 (Linux's largest + # pid_max; macOS's is 99999), so it never wraps as a pid_t. Anything else + # does not parse, and the file is kept. + local raw line pid="" n=0 lines=() + raw="$(cat "$1" 2>/dev/null)" || return 1 + while IFS= read -r line || [ -n "$line" ]; do + line="${line%$'\r'}" + case "$line" in *[![:space:]]*) lines+=("$line") ;; esac + done <<<"$raw" + [ ${#lines[@]} -gt 0 ] || return 1 + case "$raw" in *'"pid"'*'"pid"'*) return 1 ;; esac + if [ "${lines[0]}" = "{" ]; then + [ ${#lines[@]} -ge 3 ] && [ "${lines[${#lines[@]} - 1]}" = "}" ] || return 1 + local i + for ((i = 1; i < ${#lines[@]} - 1; i++)); do + line="${lines[i]}" + case "$line" in + ' "pid": '*) + pid="${line#' "pid": '}" + pid="${pid%,}" + n=$((n + 1)) + ;; + ' "'[!\ \"]*) ;; + *) return 1 ;; + esac + done + [ "$n" -eq 1 ] || return 1 + else + [ ${#lines[@]} -eq 1 ] || return 1 + pid="${lines[0]}" + while case "$pid" in [[:space:]]*) true ;; *) false ;; esac; do pid="${pid#?}"; done + while case "$pid" in *[[:space:]]) true ;; *) false ;; esac; do pid="${pid%?}"; done + fi + numeric "$pid" && [ ${#pid} -le 10 ] || return 1 + pid=$((10#$pid)) + [ "$pid" -gt 0 ] && [ "$pid" -le 4194304 ] || return 1 + printf '%s' "$pid" +} +pid_liveness() { + # alive, dead or unknown. DEAD is ESRCH only: kill -0 also fails with EPERM + # for a live process this user may not signal (BUG-4068 r1), so the + # builtin's own message is read under LC_ALL=C. ps is not asked: a missing, + # busybox or hidepid-restricted ps would read a live process as gone. + # ESRCH is per pid namespace: a sibling in another container that shares + # the data dir reads as dead here (ruled inherent, f8009dad). + local out + if out="$(LC_ALL=C kill -0 "$1" 2>&1)"; then + echo alive + return + fi + case "$out" in + *'No such process'*) echo dead ;; + *) echo unknown ;; + esac +} +pid_file_removable() { + # pid_file_removable : succeeds when the + # file is provably not a live sibling's, the pid this refresh stopped or a + # dead one. Otherwise it prints why the file is kept, and fails. + local file_pid + if ! file_pid="$(pid_file_pid "$1")"; then + echo "it does not parse as a pid record" + return 1 + fi + [ -n "$2" ] && [ "$file_pid" = "$2" ] && return 0 + case "$(pid_liveness "$file_pid")" in + dead) return 0 ;; + alive) echo "it names pid $file_pid, which is alive and is not the server this refresh stopped" ;; + *) echo "it names pid $file_pid, whose liveness is unknown" ;; + esac + return 1 +} +pid_file="$DATA_DIR/pad.pid" +if [ -e "$pid_file" ]; then + if kept_why="$(pid_file_removable "$pid_file" "$STOPPED_PID")"; then + rm -f "$pid_file" + else + note "kept $pid_file: $kept_why" + fi +fi # The check TASK-2787 asks for, and it is deliberately on the installed file # rather than on any step: read what is now at the install path and require it