Files
pad/internal/cli/server.go
xarmian cba880ca17 fix(config): warn on a relative PAD_DATA_DIR / PAD_DB_PATH; anchor install-refresh's marker and config reads (BUG-4066 phase 1) (#2308)
* fix(config): warn once per process on a relative PAD_DATA_DIR / PAD_DB_PATH; install-refresh anchors its marker and config reads (BUG-4066 phase 1)

config.Load takes PAD_DATA_DIR verbatim, or PAD_DB_PATH's dirname, with no
filepath.Abs, so every process resolves a relative value against its own cwd:
a CLI and a server started in different directories silently use different
data (database, attachments, key). Phase 1 (lead ruling 4b4cb4ff) leaves the
resolution unchanged and makes it loud:

- One helper, envDataDir(), is the only reader of the two variables (Load and
  userConfigPath both use it). A relative value warns ONCE per process,
  naming the absolute path this process resolved, that a CLI and a server in
  different directories will use different data, and that a future release
  refuses it (TASK-4072). A literal '~' gets a hint. A CLI prints it to
  stderr; `pad server start` logs it through slog; `pad watch --stream
  --for-session` (the plugin monitor) suppresses it.
- install-refresh anchors the refresh marker and config_value the way 2a
  anchors the log (${SERVER_CWD:-$PWD}), so the marker is written and
  cleaned up in one place (it used to leak under the caller's cwd), and warns
  before anything is read or stopped.
- docs/deployment.md: both variables must be absolute; relative is
  deprecated and warns.

Tests: the warning rule (and Windows, GOOS-guarded), the marker and the
warning-before-stop end to end, config_value at function level, and
child-process tests for the monitor's silence and the server's slog line.

Harness guard (after this unit's draft test stopped the box's shared :7777
server twice): every install-refresh exec in internal/buildtools now passes
requireSafeScriptEnv first, which refuses an environment whose resolved port
is missing (the script would fall back to 7777) or is 7777, whose HOME or
data dir is the real user's or outside a test temp dir, or whose port answers
on a LAN address without this test's random stub token.

Claude-Session: https://claude.ai/code/session_01Hk4vxsaKDm3E1LvsoHtHeV

* fix(config,mcp,scripts): the relative data-dir warning names the server's path and stays out of agent-facing errors; harden the test guard (BUG-4066 r1)

r1 rulings (c0ef54e8):
- install-refresh's warning now runs once SERVER_CWD is captured (still
  before the marker and the stop) and names the anchored path
  (${SERVER_CWD:-$PWD}/<dir>, a leading ./ stripped); when the caller's cwd
  differs it also names the caller's reading. It used to name $PWD/<dir>,
  which the server does not use.
- `pad mcp serve` checks the data dir once on its own stderr; the stdio
  dispatcher's `pad` children get PAD_INTERNAL_NO_DATA_DIR_WARNING=1, so the
  warning is not folded into a failed tool call's agent-facing error. The
  variable is internal (code-documented only), so users cannot silence the
  deprecation. Shell completion no longer prints the warning mid-Tab.
- Test guard: refuses a relative data dir with a config-sourced port (the
  script reads that port from the server's cwd, which the guard cannot see);
  compares symlink-resolved temp paths (macOS); asks for the stub token on
  the LAN address that answered; compares 7777 numerically; and a source-scan
  test fails if any install-refresh exec in the package is unguarded.

Claude-Session: https://claude.ai/code/session_01Hk4vxsaKDm3E1LvsoHtHeV

* fix(config,mcp,scripts): the relative-dir warning names the data dir; completion and MCP fetchers stay quiet; harden the test guard (BUG-4066 r2)

- PAD_DB_PATH warning names the resolved data dir (Dir of the value), in Go and in install-refresh.sh; the script normalises . and .. in pure shell.
- Shell completion (__complete, __completeNoDesc) sets the silent sink in main.
- MCP resource and bootstrap fetchers exec with dispatchChildEnv; a population scan pins every pad exec in internal/mcp.
- Harness guard: numeric 7777 refusal before any dial (asserted by message), PAD_HOST must be empty or loopback, temp-dir check scoped to this test, hardened and self-tested source scan, marker-presence probe in the restarted stub, child timeout, sink reset.

Claude-Session: https://claude.ai/code/session_01Hk4vxsaKDm3E1LvsoHtHeV

* fixup! fix(config,mcp,scripts): the relative-dir warning names the data dir; completion and MCP fetchers stay quiet; harden the test guard (BUG-4066 r2)

test(buildtools): the config-file 7777 row asserts the numeric refusal by message too (BUG-4066 r2 item 12, same class)

* test(buildtools,cmd): close the m15, m24 and m28 survivors (BUG-4066 r2)

m24: a scanner self-test case whose variable never runs in its function,
with a guard in a later function, so the function boundary is what flags it.
m28: insideTestTempDir refuses another test's directory, and admits a
subtest's, which t.TempDir names <Parent><sub...> with no separator.
m15: the --workspace completion func no longer sets its own sink. main()
silences every completion request, which made that line dead code.

Claude-Session: https://claude.ai/code/session_01Hk4vxsaKDm3E1LvsoHtHeV

* fix(cli,buildtools): an auto-started server does not inherit the warning silencer; the guard reads the config port like the script (BUG-4066 r3)

Item 2: an MCP tool-call child carries PAD_INTERNAL_NO_DATA_DIR_WARNING=1
and is often the process that auto-starts the server, so that long-lived
server never logged the relative data-dir warning. serverStartCommand drops
that one variable from the spawn env, and keeps every other one. It is the
only `server start` spawn in internal/cli; a source test pins both facts.

Item 7: the harness guard took the leading digits of config.toml's port, so
"8080abc" read as 8080 where install-refresh rejects it and falls back to
7777. It now reads the first port line the way config_value does, and a
non-numeric value is refused as unreadable.

Claude-Session: https://claude.ai/code/session_01Hk4vxsaKDm3E1LvsoHtHeV

* fix(scripts): install-refresh's relative-dir warning is one line; the completion tests run main() (BUG-4066 r4)

Item 1: when the server's cwd differs from the caller's, the warning printed
a second "Warning:" line naming the shell's-directory reading, and a third
for a leading ~. Phase 1's contract is one warning per process, as
config.Load prints it, so both now ride in the one line. The test asserts
both paths sit in that line and that it prints once.

Item 6: the completion child cases called silenceWarningsForCompletion
themselves, so deleting main()'s call passed every test. They now set
os.Args and run main().

Claude-Session: https://claude.ai/code/session_01Hk4vxsaKDm3E1LvsoHtHeV
2026-10-25 01:13:51 -04:00

342 lines
13 KiB
Go

package cli
import (
"fmt"
"log/slog"
"net"
"net/http"
"os"
"os/exec"
"strconv"
"strings"
"time"
"github.com/PerpetualSoftware/pad/internal/config"
)
// EnsureServer checks if the pad server is running; if not, starts it in the background.
func EnsureServer(cfg *config.Config) error {
// Only an explicitly configured local client should auto-manage a local
// background process. Unconfigured or external clients should connect only
// to their configured target.
if !cfg.ManagesLocalServer() {
return nil
}
if serverResponds(cfg.Host, cfg.Port) {
return nil
}
// A refresh may have stopped the server on purpose and be about to
// restart it (BUG-3215). Its window is its own: wait for it rather than
// spawn a server with this caller's binary, flags and cwd over it.
if err := awaitRefresh(cfg); err != nil {
return err
}
if serverResponds(cfg.Host, cfg.Port) {
return nil
}
// Something holds the port but sent no response: a server still starting
// (it binds before it migrates, TASK-3620). Spawning over it only makes a
// second start that loses the bind. Wait for it to serve instead, so the
// caller's request does not sit in the backlog past its own timeout.
if portHeld(cfg.Host, cfg.Port) {
return awaitServing(cfg)
}
return spawnServer(cfg)
}
// startupWait bounds awaitServing. Generous, because a start that runs
// migrations holds the port for as long as they take; a variable so a test can
// shorten it.
var startupWait = 2 * time.Minute
// awaitServing waits for the server holding the port to answer HTTP.
func awaitServing(cfg *config.Config) error {
deadline := time.Now().Add(startupWait)
for time.Now().Before(deadline) {
if serverResponds(cfg.Host, cfg.Port) {
return nil
}
if !portHeld(cfg.Host, cfg.Port) {
// The holder went away without serving. Leave the port to the
// next call's ordinary auto-start rather than race for it here.
return fmt.Errorf("the process holding %s exited before it served; retry", cfg.Addr())
}
time.Sleep(refreshPoll)
}
return fmt.Errorf("a process holds %s but has not answered HTTP within %s (a server still migrating?); "+
"not starting another over it. Check %s", cfg.Addr(), startupWait, cfg.LogFile())
}
// spawnServer starts `<this binary> server start` in the background and waits
// for it to answer. A variable so the tests can count spawns: the defect
// BUG-3215 guards against is a spawn, and only a counter observes one that
// did not happen.
var spawnServer = startBackgroundServer
// serverStartCommand is the auto-started server's command. Its env is this
// process's, minus config.InternalNoDataDirWarningEnv: an MCP tool-call child
// carries that variable so its own warning stays out of an agent-facing error,
// and it is often the process that auto-starts the server, which is
// long-lived and must still log the warning (BUG-4066 r3 item 2).
func serverStartCommand(exePath string) *exec.Cmd {
cmd := exec.Command(exePath, "server", "start")
prefix := config.InternalNoDataDirWarningEnv + "="
for _, kv := range os.Environ() {
if !strings.HasPrefix(kv, prefix) {
cmd.Env = append(cmd.Env, kv)
}
}
return cmd
}
func startBackgroundServer(cfg *config.Config) error {
// Start server as background process
exePath, err := os.Executable()
if err != nil {
return fmt.Errorf("find executable: %w", err)
}
cmd := serverStartCommand(exePath)
setSysProcAttr(cmd)
// Redirect stdout/stderr to log file
logFile, err := os.OpenFile(cfg.LogFile(), os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644)
if err != nil {
return fmt.Errorf("open log file: %w", err)
}
cmd.Stdout = logFile
cmd.Stderr = logFile
if err := cmd.Start(); err != nil {
logFile.Close()
return fmt.Errorf("start server: %w", err)
}
// The PID file is deliberately NOT written here (BUG-2965, codex round 3).
// The child claims it AFTER it binds the port, so the file always names the
// process that actually owns the address. Writing it from the parent, at
// spawn time, records a process that may never bind — and when the port is
// held by an existing server that is briefly unhealthy, that write
// overwrites the running server's own entry with a pid that is about to
// exit, leaving a healthy server unaddressable by `pad server stop`. The
// health wait below is what tells us the child got there.
// Release the process so it doesn't become a zombie
cmd.Process.Release()
logFile.Close()
// Wait for server to become healthy
for i := 0; i < 30; i++ {
time.Sleep(100 * time.Millisecond)
if serverResponds(cfg.Host, cfg.Port) {
return nil
}
}
// The child binds before it migrates (TASK-3620), so a held port after
// three seconds is a server still starting: wait for it as EnsureServer
// does, rather than fail a call whose server is about to serve.
if portHeld(cfg.Host, cfg.Port) {
return awaitServing(cfg)
}
return fmt.Errorf("server failed to start within 3 seconds. Check %s for errors", cfg.LogFile())
}
// StopServer stops the server this CLI's config points at — or explains why it
// will not, which is the harder half.
//
// The PID file is a HANDLE, not evidence (BUG-2965), and the pid inside it is
// not proof of identity (BUG-2969). Measured before this changed: a `sleep 600`
// whose pid had been written into the file was SIGTERMed, and stop printed
// "Server stopped." — os.FindProcess succeeds for any pid on Unix, nothing
// asked whether the pid was ours, and the confirmation loop polled the PORT,
// which is unhealthy from the first poll when nothing was ever serving. The
// success check was satisfied by the failure case.
//
// So the order is: read the record, ask the PLATFORM whether a live pad server
// owns it (an advisory lock on Unix, the process creation time on Windows),
// and signal only on a yes. Stale and unprovable both mean signal nothing.
func StopServer(cfg *config.Config) error {
path := cfg.PIDFile()
// One read, under the ownership probe: pidFileOwner returns the record it
// verified, so the pid signalled below is the pid whose ownership was
// established. An earlier separate read left a window for a successor to
// claim the file between the two (codex round 1).
rec, owner := pidFileOwner(path)
if owner == pidFileStale && rec.PID == 0 {
// No file at all. A server started outside this CLI still holds the
// port, and saying "not running" about it is the BUG-2965 failure.
if portHeld(cfg.Host, cfg.Port) {
return fmt.Errorf(
"a server is answering on %s but this CLI did not start it (no PID file at %s) — "+
"stop it where it was started, or signal the process listening on that address",
cfg.Addr(), path)
}
return fmt.Errorf("server not running (no PID file)")
}
switch owner {
case pidFileStale:
// The writer is gone. The pid may since have been reused by something
// unrelated, so it does not get signalled on the strength of a file
// the dead process left behind.
// The file is already gone: pidFileOwner removed it while holding the
// lock, which is the only point at which no replacement can have
// claimed the path (codex round 2).
if portHeld(cfg.Host, cfg.Port) {
// Deliberately NOT "the process is gone": the recorded pid may
// well be alive — that is the dangerous case, since it is a
// stranger that inherited the number — and a reader who checks
// will find it running. What is true is that it does not own this
// file, so it is not the server answering on that address.
//
// The message does not claim the file was removed: Unix drops it
// under the ownership lock, Windows leaves it for the next start
// to overwrite (see pidfile_windows.go), and a message that
// asserted a cleanup the platform did not do would be the same
// class of lie this whole item is about.
return fmt.Errorf(
"a server is answering on %s, but the PID file does not belong to it (%s) — "+
"nothing was signalled; stop that server where it was started",
cfg.Addr(), describePIDRecord(rec))
}
return fmt.Errorf("server not running (stale PID file: it named %s, which does not own it)", describePIDRecord(rec))
case pidFileUnprovable:
return fmt.Errorf(
"cannot confirm that %s belongs to this pad server, so it was not signalled — "+
"stop it where it was started, or remove %s if you know the process is gone",
describePIDRecord(rec), path)
case pidFileOurs:
// Fall through to the stop.
default:
return fmt.Errorf("unrecognised PID file ownership state %q", owner)
}
process, err := os.FindProcess(rec.PID)
if err != nil {
return fmt.Errorf("process not found")
}
if err := stopProcess(process); err != nil {
return fmt.Errorf("failed to stop server: %w", err)
}
// Wait for OUR PROCESS to exit — not for the port to go quiet. A port that
// nothing was serving is quiet immediately, which is how the old loop
// confirmed a kill that never touched a pad server.
for i := 0; i < 60; i++ {
if processIsGone(rec.PID) {
// Deliberately NOT removing the file here (codex round 1): the
// server removes its own on the way down, and by the time we
// observe its exit a successor may already have bound the port and
// claimed the path. Deleting then would unaddress the live server.
// A file left behind by a crash is handled where it belongs — the
// next stop reads it as stale and removes it there.
return nil
}
time.Sleep(100 * time.Millisecond)
}
// Still there after six seconds. The file stays: the process it names is
// alive, so the record is still true and still the handle for a retry.
return fmt.Errorf("signalled %s but it has not exited after 6s — it may be draining connections", describePIDRecord(rec))
}
// IsServerRunning reports whether a server answers on the configured address.
func IsServerRunning(cfg *config.Config) bool {
return portHeld(cfg.Host, cfg.Port)
}
// Two questions, two probes (TASK-3620). The old single probe was an HTTP GET
// of /health that required a 200, and it was wrong both ways:
//
// - The probe shares one per-address rate-limit bucket across every CLI on
// the machine, so a parallel fan-out drew 429s that read as "not running":
// 206 `pad server start` spawns in two days, each opening the live
// database before losing the bind.
// - `pad server start` binds before it migrates, so a starting server holds
// the port and answers nothing until startup is done.
//
// portHeld answers "may I start a server here, or touch the database under
// it?": whether a TCP connect succeeds. serverResponds answers "is it serving
// yet?": whether an HTTP request gets any response, whatever the status, since
// /health itself only answers 200 and anything else is middleware in front of
// a running server.
// portHeld reports whether something holds host:port.
func portHeld(host string, port int) bool {
conn, err := net.DialTimeout("tcp", net.JoinHostPort(host, strconv.Itoa(port)), time.Second)
if err != nil {
return false
}
_ = conn.Close()
return true
}
// serverResponds reports whether an HTTP request to host:port gets a response.
func serverResponds(host string, port int) bool {
client := &http.Client{Timeout: 1 * time.Second}
resp, err := client.Get(fmt.Sprintf("http://%s:%d/api/v1/health", host, port))
if err != nil {
return false
}
_ = resp.Body.Close()
return true
}
// ClaimPIDFile records this process in the PID file and holds the platform's
// ownership marker for as long as it runs. The returned release drops that
// marker and removes the file.
//
// CALL IT ONLY AFTER BINDING THE PORT. That ordering is the whole ownership
// story and it took two codex rounds to arrive at (BUG-2965): whoever holds the
// address is the process `stop` must signal, so the claim follows the bind
// rather than racing it. A write before the bind lets a start that LOSES the
// race record itself and then, on its way out, remove the file the winner
// relies on.
//
// On Unix the marker is an advisory lock held on the file itself, which is what
// lets `stop` distinguish this server from a stranger that inherited the pid
// (BUG-2969). On Windows there is no such lock, and the recorded creation time
// is the discriminator instead.
//
// Failure is NOT fatal: the server is what the caller asked for, and a missing
// or unlocked PID file degrades `stop` to its port-probe message rather than
// breaking anything. The returned release is always safe to call, and safe to
// call twice.
func ClaimPIDFile(path string) func() {
release, err := holdPIDFile(path)
if err != nil {
slog.Warn("could not claim the PID file; `pad server stop` will not be able to address this process",
"path", path, "error", err)
return func() {}
}
rec := currentPIDRecord()
if err := writePIDRecord(path, rec); err != nil {
slog.Warn("could not write PID file; `pad server stop` will not be able to address this process",
"path", path, "error", err)
release()
return func() {}
}
return func() {
// Remove BEFORE dropping the lock: while we still hold it, no other
// server can have claimed this path, so there is nothing of anyone
// else's to delete. Dropping first would reopen the read-then-remove
// race that codex round 4 found in the previous shape.
if current, ok := readPIDRecord(path); ok && current.PID == rec.PID {
if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
slog.Warn("could not remove PID file on shutdown", "path", path, "error", err)
}
}
release()
}
}