From b2bee3242fbdef8b8dfdaee41b3a0cb320dfb1c5 Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 24 Sep 2026 10:38:16 -0400 Subject: [PATCH] fix(scripts): install-refresh restores the server's own cwd and matches the running server by identity (BUG-3196) (#1512) * fix(scripts): install-refresh restores the server's own cwd with its argv, refuses a deleted one, and matches the running server by identity rather than the built binary's name (BUG-3196) Claude-Session: https://claude.ai/code/session_01RYiqbqXmxQMoKHsGm8B77N * fix(scripts): one existence check refuses a deleted server cwd, naming the path without the kernel's suffix (BUG-3196) Claude-Session: https://claude.ai/code/session_01RYiqbqXmxQMoKHsGm8B77N * test(scripts): the no-/proc refresh test declares lsof, which now reads the server's cwd there (BUG-3196) Claude-Session: https://claude.ai/code/session_01RYiqbqXmxQMoKHsGm8B77N * test(scripts): the lsof dependency sits on the requireScriptDeps line the guard reads (BUG-3196) Claude-Session: https://claude.ai/code/session_01RYiqbqXmxQMoKHsGm8B77N * docs(make): the install target's guarantees include the restored cwd (BUG-3196) Claude-Session: https://claude.ai/code/session_01RYiqbqXmxQMoKHsGm8B77N * fix(scripts): the server's cwd is re-checked immediately before the stop, and the capture's limits are stated (BUG-3196) Claude-Session: https://claude.ai/code/session_01RYiqbqXmxQMoKHsGm8B77N * fix(scripts): a failed restart cd says what state it leaves, not that no server runs (BUG-3196) Claude-Session: https://claude.ai/code/session_01RYiqbqXmxQMoKHsGm8B77N * fix(scripts): an unreadable server cwd is refused naming what was read and its error, and the case is tested (BUG-3196) Claude-Session: https://claude.ai/code/session_01RYiqbqXmxQMoKHsGm8B77N --- Makefile | 5 +- internal/buildtools/install_refresh_test.go | 256 +++++++++++++++++++- internal/buildtools/testdata/stub/main.go | 10 + scripts/install-refresh.sh | 193 +++++++++++---- 4 files changed, 412 insertions(+), 52 deletions(-) diff --git a/Makefile b/Makefile index 5e1c90f14..0ecb0317d 100644 --- a/Makefile +++ b/Makefile @@ -38,7 +38,10 @@ install: build @# sibling's build sitting at ./pad cannot be installed silently @# (TASK-2787); @# - the server comes back with the argv of the process it killed, - @# rather than whatever an auto-start defaults to (BUG-2897); + @# rather than whatever an auto-start defaults to (BUG-2897), and in + @# that process's working directory rather than the caller's; a server + @# whose directory is gone when checked, just before the stop, is + @# refused with nothing stopped (BUG-3196); @# - nothing is printed about a restart until the server answers on @# BOTH 127.0.0.1 and the configured host. @# diff --git a/internal/buildtools/install_refresh_test.go b/internal/buildtools/install_refresh_test.go index 0af6dbd72..edbcf54c5 100644 --- a/internal/buildtools/install_refresh_test.go +++ b/internal/buildtools/install_refresh_test.go @@ -78,7 +78,7 @@ func TestMain(m *testing.M) { // running these tests without anyone remembering to update a skip list. func requireScriptDeps(t *testing.T, extra ...string) { t.Helper() - for _, tool := range append([]string{"bash", "pgrep", "pkill", "curl", "ps"}, extra...) { + for _, tool := range append([]string{"bash", "pgrep", "pkill", "curl", "ps", "readlink"}, extra...) { if _, err := exec.LookPath(tool); err != nil { t.Skipf("%s not available; this environment cannot run install-refresh.sh", tool) } @@ -929,7 +929,7 @@ func TestInstallRefresh_ConfigValuesIgnoreInlineComments(t *testing.T) { // as the setsid case, and for the same reason: the branch carries the risk, // the detection is one test. func TestInstallRefresh_CapturesArgvWithoutProc(t *testing.T) { - requireScriptDeps(t) + requireScriptDeps(t, "lsof") // without /proc the server's cwd is read with lsof (BUG-3196) home, dir := t.TempDir(), t.TempDir() name := uniqueName(t) defer killStub(t, name) @@ -1046,7 +1046,7 @@ func TestInstallRefresh_FailsClosedWhenTheLANAddressIsUnknown(t *testing.T) { // hostname/ipconfig. Built by symlinking, so the rest of the script // still works and only address discovery is impossible. stubPath := t.TempDir() - for _, tool := range []string{"bash", "sed", "awk", "curl", "cp", "mv", "mkdir", "rm", "chmod", "pkill", "pgrep", "ps", "sleep", "mktemp", "dirname", "basename", "git", "head", "setsid", "nohup", "printf", "cat"} { + for _, tool := range []string{"bash", "sed", "awk", "curl", "cp", "mv", "mkdir", "rm", "chmod", "pkill", "pgrep", "ps", "sleep", "mktemp", "dirname", "basename", "git", "head", "setsid", "nohup", "printf", "cat", "readlink"} { full, err := exec.LookPath(tool) if err != nil { continue @@ -1557,3 +1557,253 @@ func TestEveryScriptTestDeclaresItsDependencies(t *testing.T) { t.Errorf("these tests drive install-refresh.sh without declaring their dependencies, so they FAIL rather than SKIP where the tools are absent: %v", missing) } } + +// runScriptIn is runScript with the SCRIPT's own working directory set, which +// is what a refresh run from a worktree looks like (BUG-3196). +func runScriptIn(t *testing.T, e stubEnv, dir, built, installed, commit string, extraEnv ...string) runResult { + t.Helper() + cmd := exec.Command("bash", scriptPath(t), built, installed, commit) + cmd.Env = append(e.env(), extraEnv...) + cmd.Dir = dir + var out, errb strings.Builder + cmd.Stdout, cmd.Stderr = &out, &errb + err := cmd.Run() + return runResult{stdout: out.String(), stderr: errb.String(), err: err} +} + +func realDir(t *testing.T, d string) string { + t.Helper() + r, err := filepath.EvalSymlinks(d) + if err != nil { + t.Fatalf("eval %s: %v", d, err) + } + return r +} + +// padName is uniqueName for a binary whose name starts `pad-`, the shape a +// listener found only by port must have to be taken for a pad server. +func padName(t *testing.T) string { + t.Helper() + return "pad-" + strings.TrimPrefix(uniqueName(t), "padstb") +} + +// BUG-3196: the restart runs in the directory the SERVER was running in, not +// in the directory the refresh was run from. Before the fix a refresh run from +// a worktree left :7777 in that worktree, and then in a deleted directory once +// the worktree was removed. +func TestInstallRefresh_RestartsInTheServersOwnCwd(t *testing.T) { + requireScriptDeps(t) + home, dir := t.TempDir(), t.TempDir() + serverDir, callerDir := realDir(t, t.TempDir()), realDir(t, 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) + if err := os.MkdirAll(filepath.Dir(installed), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + cwdLog := filepath.Join(home, "cwd.log") + e := stubEnv{version: "pad version dev (" + commit + " x)", healthy: "1", + argvLog: filepath.Join(home, "argv.log"), port: port, home: home} + + pre := exec.Command(built, "server", "start", "--host", "127.0.0.1") + pre.Env = append(e.env(), "STUB_CWD_LOG="+cwdLog) + pre.Dir = serverDir + startPreServer(t, pre, "127.0.0.1", port) + + res := runScriptIn(t, e, callerDir, built, installed, commit, "STUB_CWD_LOG="+cwdLog) + if res.err != nil { + t.Fatalf("script failed: %v\nstdout=%s\nstderr=%s", res.err, res.stdout, res.stderr) + } + logged, err := os.ReadFile(cwdLog) + if err != nil { + t.Fatalf("read cwd log: %v", err) + } + lines := strings.Split(strings.TrimSpace(string(logged)), "\n") + if len(lines) < 2 { + t.Fatalf("expected the pre-server and the restart in the cwd log, got %q", lines) + } + if got := lines[len(lines)-1]; got != serverDir { + t.Errorf("the restart ran in %q, want the server's own %q (the caller was in %q)", got, serverDir, callerDir) + } + if !strings.Contains(res.stdout, "captured server cwd: "+serverDir) { + t.Errorf("the script did not report the captured cwd; stdout=%s", res.stdout) + } +} + +// A server whose directory has been deleted cannot be put back where it was, +// so the refresh refuses, naming the path, before anything is stopped. +func TestInstallRefresh_RefusesAServerWhoseCwdWasDeleted(t *testing.T) { + requireScriptDeps(t, "ss") + home, dir := t.TempDir(), t.TempDir() + gone := filepath.Join(realDir(t, t.TempDir()), "worktree") + if err := os.Mkdir(gone, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + 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} + pre := exec.Command(built, "server", "start", "--host", "127.0.0.1") + pre.Env = e.env() + pre.Dir = gone + startPreServer(t, pre, "127.0.0.1", port) + if err := os.Remove(gone); err != nil { + t.Fatalf("remove the server's directory: %v", err) + } + + res := runScript(t, e, built, installed, commit) + if res.err == nil { + t.Fatalf("script proceeded with the server in a deleted directory; stdout=%s", res.stdout) + } + // The path, not the kernel's " (deleted)" spelling of it. + if !strings.Contains(res.stderr, "no longer exists: "+gone+"\n") { + t.Errorf("refusal did not name the missing directory %s; stderr=%s", gone, res.stderr) + } + if _, err := os.Stat(installed); err == nil { + t.Errorf("installed despite refusing: %s", installed) + } + if got, err := pidAnswersAt(port); err != nil || got != strconv.Itoa(pre.Process.Pid) { + t.Errorf("the server was touched by a refused refresh: %q %v", got, err) + } +} + +// The staged binary's NAME does not decide the match (BUG-3196). The running +// server is the installed binary; the build being installed has another name. +// Before the fix the candidates came from the built name only, found nothing, +// and the listener read as a foreign process. +func TestInstallRefresh_MatchesTheRunningServerNotTheBuiltName(t *testing.T) { + requireScriptDeps(t, "ss") + home, dir := t.TempDir(), t.TempDir() + runName, builtName := uniqueName(t), uniqueName(t) + defer killStub(t, runName) + defer killStub(t, builtName) + port := freePort(t) + const commit = "abc1234" + + built := installStub(t, dir, builtName) + installed := installStub(t, filepath.Join(home), runName) + e := stubEnv{version: "pad version dev (" + commit + " x)", healthy: "1", + argvLog: filepath.Join(home, "argv.log"), port: port, home: home} + pre := exec.Command(installed, "server", "start", "--host", "127.0.0.1") + pre.Env = e.env() + startPreServer(t, pre, "127.0.0.1", port) + + res := runScript(t, e, built, installed, commit) + if res.err != nil { + t.Fatalf("script refused a server running as the installed binary: %v\nstdout=%s\nstderr=%s", res.err, res.stdout, res.stderr) + } + if got, err := pidAnswersAt(port); err != nil || got == strconv.Itoa(pre.Process.Pid) { + t.Errorf("the server was not replaced: %q %v", got, err) + } +} + +// A listener on the port found by NEITHER candidate name is taken for the +// server only when its own argv[0] names a pad binary. +func TestInstallRefresh_JudgesAPortOnlyListenerByItsOwnName(t *testing.T) { + requireScriptDeps(t, "ss") + const commit = "abc1234" + for _, c := range []struct { + name string + runName func(*testing.T) string + replaced bool + }{ + {"a pad-named server is replaced", padName, true}, + {"a non-pad process reading `server start` is refused", func(t *testing.T) string { + return "xsrv" + strings.TrimPrefix(uniqueName(t), "padstb") + }, false}, + } { + t.Run(c.name, func(t *testing.T) { + home, dir := t.TempDir(), t.TempDir() + runName, builtName := c.runName(t), uniqueName(t) + defer killStub(t, runName) + defer killStub(t, builtName) + port := freePort(t) + built := installStub(t, dir, builtName) + running := installStub(t, t.TempDir(), runName) + installed := filepath.Join(home, "bin", builtName) + e := stubEnv{version: "pad version dev (" + commit + " x)", healthy: "1", + argvLog: filepath.Join(home, "argv.log"), port: port, home: home} + pre := exec.Command(running, "server", "start", "--host", "127.0.0.1") + pre.Env = e.env() + startPreServer(t, pre, "127.0.0.1", port) + + res := runScript(t, e, built, installed, commit) + got, perr := pidAnswersAt(port) + if c.replaced { + if res.err != nil { + t.Fatalf("refused a pad server on the port: %v\nstderr=%s", res.err, res.stderr) + } + if perr != nil || got == strconv.Itoa(pre.Process.Pid) { + t.Errorf("not replaced: %q %v", got, perr) + } + return + } + if res.err == nil || !strings.Contains(res.stderr, "is held by") { + t.Fatalf("a non-pad listener was not refused as foreign: err=%v stderr=%s", res.err, res.stderr) + } + if perr != nil || got != strconv.Itoa(pre.Process.Pid) { + t.Errorf("the foreign listener was touched: %q %v", got, perr) + } + }) + } +} + +// An UNREADABLE server cwd is refused like a missing one, naming what was +// read and why it failed (lead ruling on BUG-3196). Reached deterministically: +// PAD_NO_PROC forces the lsof path, and a PATH without lsof leaves nothing to +// read the cwd with. +func TestInstallRefresh_RefusesAnUnreadableCwdNamingWhatWasRead(t *testing.T) { + requireScriptDeps(t, "ss") + 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} + pre := exec.Command(built, "server", "start", "--host", "127.0.0.1") + pre.Env = e.env() + startPreServer(t, pre, "127.0.0.1", port) + + stubPath := t.TempDir() + for _, tool := range []string{"bash", "sed", "awk", "curl", "cp", "mv", "mkdir", "rm", "chmod", "pkill", "pgrep", "ps", "sleep", "mktemp", "dirname", "basename", "git", "head", "setsid", "nohup", "printf", "cat", "readlink", "ss", "grep", "cut", "sort", "tail"} { + if full, err := exec.LookPath(tool); err == nil { + _ = os.Symlink(full, filepath.Join(stubPath, tool)) + } + } + env := []string{} + for _, kv := range e.env() { + if !strings.HasPrefix(kv, "PATH=") { + env = append(env, kv) + } + } + env = append(env, "PATH="+stubPath, "PAD_NO_PROC=1") + cmd := exec.Command("bash", scriptPath(t), built, installed, commit) + cmd.Env = env + var out, errb strings.Builder + cmd.Stdout, cmd.Stderr = &out, &errb + if err := cmd.Run(); err == nil { + t.Fatalf("script proceeded without being able to read the server's cwd; stdout=%s", out.String()) + } + if !strings.Contains(errb.String(), "cannot be read: (no /proc and no lsof)") { + t.Errorf("refusal did not name what was read; stderr=%s", errb.String()) + } + if _, err := os.Stat(installed); err == nil { + t.Errorf("installed despite refusing: %s", installed) + } + if got, err := pidAnswersAt(port); err != nil || got != strconv.Itoa(pre.Process.Pid) { + t.Errorf("the server was touched by a refused refresh: %q %v", got, err) + } +} diff --git a/internal/buildtools/testdata/stub/main.go b/internal/buildtools/testdata/stub/main.go index 5fc9b789c..1ba1107d2 100644 --- a/internal/buildtools/testdata/stub/main.go +++ b/internal/buildtools/testdata/stub/main.go @@ -53,6 +53,16 @@ func main() { if !(len(os.Args) > 2 && os.Args[1] == "server" && os.Args[2] == "start") { return } + // STUB_CWD_LOG records the directory each server start runs in, so a test + // can see where a restart put it (BUG-3196). + if logPath := os.Getenv("STUB_CWD_LOG"); logPath != "" { + if wd, err := os.Getwd(); err == nil { + if f, err := os.OpenFile(logPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644); err == nil { + fmt.Fprintln(f, wd) + f.Close() + } + } + } // STUB_HEALTHY=0 exits immediately: what a restart that silently did // not happen looks like from outside. if os.Getenv("STUB_HEALTHY") != "1" { diff --git a/scripts/install-refresh.sh b/scripts/install-refresh.sh index e546eba85..80297c3eb 100755 --- a/scripts/install-refresh.sh +++ b/scripts/install-refresh.sh @@ -193,50 +193,69 @@ join_us() { local a for a in "$@"; do printf '%s%s' "$a" "$US"; done } -if command -v pgrep >/dev/null 2>&1; then - while read -r pid; do - [ -n "$pid" ] || continue - # /proc where it exists, `ps` where it does not. - # - # Codex round 5 (P1): macOS has no /proc, so this loop found - # nothing there, SERVER_ARGV stayed empty, and the restart fell - # back to defaults — the exact defect BUG-2897 is about, silently - # unfixed on the platform the setsid fallback had just been added - # for. A fix that is Linux-only while advertising portability is - # worse than one that admits its platform. - # - # /proc is preferred because it gives the argv NUL-separated and - # therefore exactly; `ps -o args=` returns one space-joined string, - # so an argument containing a space is split. Named rather than - # hidden: no `pad server start` flag takes such a value today. - # No `local` here: this loop runs at TOP LEVEL, and bash answers - # `local` outside a function with an error on stderr for every - # matching process (codex round 6). It kept working — the arrays - # were still assigned — while printing a spurious error on every - # normal refresh, which is precisely the class of noise this unit - # exists to remove. - argv=() - # PAD_NO_PROC forces the `ps` path. Same reasoning as PAD_NO_SETSID: - # the branch is unreachable on any machine with /proc, and an - # untestable branch is exactly how the macOS half of this fix would - # have shipped broken. - if [ -z "${PAD_NO_PROC:-}" ] && [ -r "/proc/$pid/cmdline" ]; then - mapfile -d '' -t argv < "/proc/$pid/cmdline" - else - line="$(ps -o args= -p "$pid" 2>/dev/null || true)" - [ -n "$line" ] || continue - read -r -a argv <<<"$line" +# capture_pid records $1 as a server if its argv carries the `server start` +# verb pair. Keyed by identity (the argv), not by the binary's file name +# (BUG-3196): the name of the binary being INSTALLED says nothing about the +# name of the one running. +# +# With a second argument `require-pad`, argv[0] must also name a pad binary +# (pad, pad-*, pad.*), checked before anything is recorded. The name-matched +# candidates do not need it; a listener found only by port does. +capture_pid() { + local pid="$1" require="${2:-}" + [ -n "$pid" ] || return 0 + [ -n "${SERVER_ARGV_OF[$pid]+x}" ] && return 0 + # /proc where it exists, `ps` where it does not. + # + # Codex round 5 (P1): macOS has no /proc, so this loop found + # nothing there, SERVER_ARGV stayed empty, and the restart fell + # back to defaults — the exact defect BUG-2897 is about, silently + # unfixed on the platform the setsid fallback had just been added + # for. A fix that is Linux-only while advertising portability is + # worse than one that admits its platform. + # + # /proc is preferred because it gives the argv NUL-separated and + # therefore exactly; `ps -o args=` returns one space-joined string, + # so an argument containing a space is split. Named rather than + # hidden: no `pad server start` flag takes such a value today. + # A function now (BUG-3196), so `local` is legal here; codex round 6 of + # BUG-3194 had removed it when this body ran at top level. + local argv=() line i + # PAD_NO_PROC forces the `ps` path. Same reasoning as PAD_NO_SETSID: + # the branch is unreachable on any machine with /proc, and an + # untestable branch is exactly how the macOS half of this fix would + # have shipped broken. + if [ -z "${PAD_NO_PROC:-}" ] && [ -r "/proc/$pid/cmdline" ]; then + mapfile -d '' -t argv < "/proc/$pid/cmdline" + else + line="$(ps -o args= -p "$pid" 2>/dev/null || true)" + [ -n "$line" ] || return 0 + read -r -a argv <<<"$line" + fi + [ ${#argv[@]} -gt 0 ] || return 0 + if [ "$require" = require-pad ]; then + case "$(basename "${argv[0]}")" in + pad | pad-* | pad.*) ;; + *) return 0 ;; + esac + fi + # argv[0] is the binary; look for the `server start` verb pair. + for ((i = 1; i < ${#argv[@]}; i++)); do + if [ "${argv[i]}" = "server" ] && [ "${argv[i + 1]:-}" = "start" ]; then + SERVER_PIDS+=("$pid") + SERVER_ARGV_OF[$pid]="$(join_us "${argv[@]}")" + break fi - [ ${#argv[@]} -gt 0 ] || continue - # argv[0] is the binary; look for the `server start` verb pair. - for ((i = 1; i < ${#argv[@]}; i++)); do - if [ "${argv[i]}" = "server" ] && [ "${argv[i + 1]:-}" = "start" ]; then - SERVER_PIDS+=("$pid") - SERVER_ARGV_OF[$pid]="$(join_us "${argv[@]}")" - break - fi - done - done < <(pgrep -x "$(basename "$BUILT")" 2>/dev/null || true) + done +} +if command -v pgrep >/dev/null 2>&1; then + # Candidates by name: the running binary is normally the installed one, + # and the built one's name is kept for a server started from a build. + while read -r pid; do + capture_pid "$pid" + # A pid found under both names is captured once: capture_pid skips a pid + # it has already recorded. + done < <({ pgrep -x "$(basename "$INSTALLED")"; pgrep -x "$(basename "$BUILT")"; } 2>/dev/null || true) fi # --- 1b. Choose the TARGET: the server holding the port this refresh is for ---- @@ -304,6 +323,16 @@ while read -r lp; do esac done < <(listener_pids "$TARGET_PORT") +# A listener is judged by what it IS rather than by the name of the binary +# being installed (BUG-3196): a pad `server start` holding the port is a server +# even when neither candidate name matched it (a binary started as +# pad-, say). Its OWN argv[0] must still be a pad binary: without that +# any tool on this port whose argv happens to read `server start` would be +# stopped and replaced. +for lp in "${LISTENERS[@]}"; do + capture_pid "$lp" require-pad +done + matched=() foreign=() for lp in "${LISTENERS[@]}"; do @@ -322,12 +351,12 @@ if [ -n "$HIDDEN" ]; then Nothing was stopped or installed; this script will not guess which server that is." fi if [ "${#foreign[@]}" -gt 0 ]; then - die "port $TARGET_PORT is held by pid(s) ${foreign[*]}, which is not a running \`$(basename "$BUILT") server start\`. + die "port $TARGET_PORT is held by pid(s) ${foreign[*]}, which is not a running \`server start\`. Nothing was stopped or installed; find out what holds the port first." fi if [ "${#matched[@]}" -gt 1 ]; then - die "more than one \`$(basename "$BUILT") server start\` is listening on port $TARGET_PORT (pids: ${matched[*]}). + die "more than one \`server start\` is listening on port $TARGET_PORT (pids: ${matched[*]}). Nothing was stopped or installed. Use the manual sibling-safe refresh." elif [ "${#matched[@]}" -eq 1 ]; then TARGET_PID="${matched[0]}" @@ -343,11 +372,11 @@ elif [ "${#SERVER_PIDS[@]}" -eq 1 ]; then if flag_value --port "${only[@]}" >/dev/null; then TARGET_PID="${SERVER_PIDS[0]}" else - die "nothing is listening on port $TARGET_PORT, and the one \`$(basename "$BUILT") server start\` running (pid ${SERVER_PIDS[0]}) was not started with --port, so it is not this box's server on another port. + die "nothing is listening on port $TARGET_PORT, and the one \`server start\` running (pid ${SERVER_PIDS[0]}) was not started with --port, so it is not this box's server on another port. Nothing was stopped or installed. Start the server, or use the manual refresh." fi elif [ "${#SERVER_PIDS[@]}" -gt 1 ]; then - die "more than one \`$(basename "$BUILT") server start\` process is running (pids: ${SERVER_PIDS[*]}) and none is listening on port $TARGET_PORT. + die "more than one \`server start\` process is running (pids: ${SERVER_PIDS[*]}) and none is listening on port $TARGET_PORT. This script cannot tell which one to replace. Nothing was stopped or installed. Use the manual sibling-safe refresh." fi @@ -359,7 +388,7 @@ if [ -n "$TARGET_PID" ]; then [ "$sp" != "$TARGET_PID" ] && OTHERS+=("$sp") done if [ "${#OTHERS[@]}" -gt 0 ]; then - note "other \`$(basename "$BUILT") server start\` processes are left running: ${OTHERS[*]}" + note "other \`server start\` processes are left running: ${OTHERS[*]}" fi fi @@ -369,6 +398,54 @@ else note "no running server found; will start with defaults after install" fi +# --- 1c. Capture the TARGET's working directory, with its argv ---------------- +# +# BUG-3196. The restart used to inherit the INVOKING shell's cwd, so a refresh +# run from a worktree left the server in that worktree, and removing the +# worktree left it in a deleted directory. The server's cwd is part of how it +# was started, like its argv, so it is captured and restored the same way. +# +# Refused BEFORE anything is stopped: +# - the captured cwd no longer exists: the server cannot be put back where +# it was, and the refusal names the path so the operator can choose; +# - the cwd cannot be read at all: restoring both argv and cwd is then +# impossible, and falling back to the caller's cwd is this bug. +# With no server running there is nothing to restore; the caller's cwd is used, +# as before. +SERVER_CWD="" +if [ -n "$TARGET_PID" ]; then + # What was read and why it failed, for the refusal: the path asked and + # the tool's own error text, which carries the errno (e.g. "Permission + # denied"). Lead ruling on BUG-3196. + cwd_source="" cwd_err="" + if [ -z "${PAD_NO_PROC:-}" ] && [ -d /proc ]; then + cwd_source="/proc/$TARGET_PID/cwd" + cwd_err="$(readlink "$cwd_source" 2>&1 >/dev/null)" || true + SERVER_CWD="$(readlink "$cwd_source" 2>/dev/null || true)" + elif command -v lsof >/dev/null 2>&1; then + cwd_source="lsof -a -p $TARGET_PID -d cwd" + cwd_err="$(lsof -a -p "$TARGET_PID" -d cwd -Fn 2>&1 >/dev/null)" || true + SERVER_CWD="$(lsof -a -p "$TARGET_PID" -d cwd -Fn 2>/dev/null | sed -n 's/^n//p' | head -1)" + else + cwd_source="(no /proc and no lsof)" + fi + if [ -z "$SERVER_CWD" ]; then + die "the working directory of the running server (pid $TARGET_PID) cannot be read: $cwd_source: ${cwd_err:-no output} + Nothing was stopped or installed; the restart could not put it back where it was." + fi + # Linux reads a removed directory back with a " (deleted)" suffix. Strip + # it so the refusal names the real path; the one existence check below + # then refuses it, since that path is gone. + SERVER_CWD="${SERVER_CWD% (deleted)}" + if [ ! -d "$SERVER_CWD" ]; then + die "the running server (pid $TARGET_PID) is in a directory that no longer exists: $SERVER_CWD + Nothing was stopped or installed. Restart it from the directory it should run in, then refresh." + fi + # Stated limit: command substitution strips trailing newlines, so a + # directory whose NAME ends in one is not captured exactly. + note "captured server cwd: $SERVER_CWD" +fi + # --- 2. SNAPSHOT the artifact, then check the snapshot ------------------------ # # The copy happens BEFORE the stop, into a temporary file beside the final @@ -431,6 +508,16 @@ if [ -n "$TARGET_PID" ]; then [ ${#now[@]} -gt 0 ] && [ "$(join_us "${now[@]}")" = "${SERVER_ARGV_OF[$TARGET_PID]}" ] } if kill -0 "$TARGET_PID" 2>/dev/null; then + # The cwd is checked again at the last moment before the stop + # (BUG-3196, codex round 1): it was checked before the snapshot, and + # a directory removed since then would leave the server stopped with + # nowhere to restart it. RESIDUAL, stated: one removed between this + # check and the restart's `cd` still can, and the restart then fails + # loudly (naming the port to check) rather than starting elsewhere. + if [ -n "$SERVER_CWD" ] && [ ! -d "$SERVER_CWD" ]; then + die "the running server's directory no longer exists: $SERVER_CWD + Nothing was stopped or installed. Restart it from the directory it should run in, then refresh." + fi if ! still_target; then die "pid $TARGET_PID no longer runs the captured server. Nothing was stopped or installed; re-run." @@ -522,6 +609,16 @@ if [ -n "$EXPECT_COMMIT" ]; then fi fi +# In the captured cwd (BUG-3196). The binary path is made absolute first, +# since the install path may have been given relative to the caller's cwd. +case "${restart[0]}" in +/*) ;; +*) restart[0]="$(cd "$(dirname "${restart[0]}")" && pwd)/$(basename "${restart[0]}")" ;; +esac +if [ -n "$SERVER_CWD" ]; then + cd "$SERVER_CWD" || die "could not enter $SERVER_CWD to restart the server there. + The old server was signalled to stop and the new binary is installed; nothing was restarted. Check what is running on port $TARGET_PORT." +fi if [ -z "${PAD_NO_SETSID:-}" ] && command -v setsid >/dev/null 2>&1; then setsid nohup "${restart[@]}" >>"$HOME/.pad/server.log" 2>&1