mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 05:49:50 +00:00
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
This commit is contained in:
xarmian
+145
-48
@@ -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-<commit>, 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 </dev/null &
|
||||
else
|
||||
|
||||
Reference in New Issue
Block a user