fix(cli): pad server stop keeps a live legacy PID file instead of deleting it, and clears a dead one (BUG-2970) (#1485)

* fix(cli): pad server stop keeps a legacy bare-pid file instead of deleting a live server's record (BUG-2970)

On Unix, pidFileOwner read a free lock as proof that a record was stale, and
removed it. That holds for records this build writes, because their server
holds the lock for its whole life. It does not hold for a bare-pid file written
before BUG-2969: no build that wrote that form took a lock, so a free lock says
nothing about its server, which may be live. The day-19 refresh deleted exactly
such a record.

A record without a fingerprint (zero StartedAt, which is what a bare pid
parses to) is now unprovable and left in place, the answer Windows already
gave for the same shape. Fingerprinted records are unchanged. Three existing
fixtures modelled fingerprinted records without a StartedAt, which no build
writes; they now carry one.

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

* fix(cli): a legacy PID file naming a dead process is stale and removed; start is never blocked by a kept one (BUG-2970)

The kept, unprovable state now applies only while the legacy record's pid
exists. A pid with no process behind it cannot be anyone's server, so that
record is stale by the same proof as an unheld fingerprinted one, and is
removed under the lock. Stop then says "server not running" instead of
refusing forever.

Tests pin both halves of the way out. Start is never blocked: EnsureServer
probes the port, and the new server's ClaimPIDFile takes over a kept legacy
file and writes a fingerprinted record stop can address. And the kept
refusal names its manual remedy (remove <path>).

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

internal/cli/pid_file_test.go

@@ -4,10 +4,14 @@ package cli
 
 import (
 	"os"
+	"os/exec"
 	"path/filepath"
 	"strconv"
+	"strings"
 	"testing"
 	"time"
+
+	"github.com/PerpetualSoftware/pad/internal/config"
 )
 
 // BUG-2965 and BUG-2969 together: the PID file must let `pad server stop`
@@ -65,7 +69,7 @@ func TestClaimPIDFile_HoldsTheLockWhileRunning(t *testing.T) {
 // liveness check would call it ours and signal it.
 func TestPIDFileOwner_UnheldFileIsStale(t *testing.T) {
 	path := filepath.Join(t.TempDir(), "pad.pid")
-	rec := pidRecord{PID: os.Getpid()}
+	rec := pidRecord{PID: os.Getpid(), StartedAt: time.Now().UTC()}
 	if err := writePIDRecord(path, rec); err != nil {
 		t.Fatalf("seed: %v", err)
 	}
@@ -158,7 +162,7 @@ func TestPIDFileOwner_ReportsTheRecordItVerified(t *testing.T) {
 	path := filepath.Join(t.TempDir(), "pad.pid")
 
 	// Predecessor's record, unheld.
-	if err := writePIDRecord(path, pidRecord{PID: 111}); err != nil {
+	if err := writePIDRecord(path, pidRecord{PID: 111, StartedAt: time.Now().UTC()}); err != nil {
 		t.Fatalf("seed: %v", err)
 	}
 
@@ -201,7 +205,7 @@ func TestPIDFileOwner_HeldButUnreadableIsUnprovable(t *testing.T) {
 // returns is how that placement stays put.
 func TestPIDFileOwner_RemovesTheStaleFileItself(t *testing.T) {
 	path := filepath.Join(t.TempDir(), "pad.pid")
-	if err := writePIDRecord(path, pidRecord{PID: os.Getpid()}); err != nil {
+	if err := writePIDRecord(path, pidRecord{PID: os.Getpid(), StartedAt: time.Now().UTC()}); err != nil {
 		t.Fatalf("seed: %v", err)
 	}
 
@@ -214,6 +218,40 @@ func TestPIDFileOwner_RemovesTheStaleFileItself(t *testing.T) {
 	}
 }
 
+// TestPIDFileOwner_LegacyBarePIDIsUnprovableAndKept is BUG-2970. A bare-pid
+// file was written by a build that predates the lock (BUG-2969), so the free
+// lock on it proves nothing: its server never took one, and may be answering
+// right now. That is what happened on the day-19 refresh, where the probe
+// deleted the record of a live pre-fix server on its way to refusing.
+//
+// Two assertions, because each catches a different wrong answer: the verdict
+// (stale would permit the cleanup path) and the FILE (a fix that returned
+// unprovable after removing it would pass the first and still lose the
+// record). The pid is this live test process, the strongest form.
+func TestPIDFileOwner_LegacyBarePIDIsUnprovableAndKept(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "pad.pid")
+	legacy := []byte(strconv.Itoa(os.Getpid()) + "\n")
+	if err := os.WriteFile(path, legacy, 0o644); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+
+	if _, owner := pidFileOwner(path); owner != pidFileUnprovable {
+		t.Errorf("owner = %v for an unheld legacy bare-pid file, want unprovable — no build that wrote this "+
+			"form took the lock, so a free lock says nothing about its server", owner)
+	}
+	got, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatalf("the legacy PID file was removed by the ownership check (%v) — that deletes the only record "+
+			"of a server that may be live", err)
+	}
+	if string(got) != string(legacy) {
+		t.Errorf("legacy PID file rewritten: %q, want %q", got, legacy)
+	}
+	// Not left locked: a successor starting now must be able to claim it.
+	release := ClaimPIDFile(path)
+	release()
+}
+
 // TestClaimPIDFile_RetriesPastABriefProbe covers the interaction the round-2
 // fix created: `stop`'s ownership probe holds the lock for an instant, and a
 // server claiming in that instant must not lose its claim for the rest of its
@@ -241,3 +279,90 @@ func TestClaimPIDFile_RetriesPastABriefProbe(t *testing.T) {
 		t.Errorf("claim did not survive a brief foreign hold (record %+v, ok=%v)", rec, ok)
 	}
 }
+
+// deadPID returns the pid of a process that has already exited and been
+// reaped. The number can in principle be reused before the caller looks, which
+// would turn a "dead" leg into a live one; the tests using it assert the
+// dead-pid behaviour only after checking processIsGone themselves.
+func deadPID(t *testing.T) int {
+	t.Helper()
+	cmd := exec.Command("true")
+	if err := cmd.Run(); err != nil {
+		t.Fatalf("run a short-lived process: %v", err)
+	}
+	pid := cmd.ProcessState.Pid()
+	if !processIsGone(pid) {
+		t.Skipf("pid %d was reused before the test could look; nothing to assert", pid)
+	}
+	return pid
+}
+
+// TestPIDFileOwner_LegacyBarePIDNamingADeadProcessIsStale is the way out of
+// BUG-2970's kept state (lead review, day 79): a legacy record whose pid has no
+// process behind it cannot be anyone's server, so it is stale by the same
+// proof as an unheld fingerprinted record, and is removed under the lock.
+// Without this a dead legacy file would be kept forever and every stop would
+// refuse over it.
+func TestPIDFileOwner_LegacyBarePIDNamingADeadProcessIsStale(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "pad.pid")
+	pid := deadPID(t)
+	if err := os.WriteFile(path, []byte(strconv.Itoa(pid)+"\n"), 0o644); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+
+	if _, owner := pidFileOwner(path); owner != pidFileStale {
+		t.Errorf("owner = %v for a legacy file naming a DEAD pid, want stale", owner)
+	}
+	if _, err := os.Stat(path); !os.IsNotExist(err) {
+		t.Errorf("a legacy file naming a dead pid survived (stat err = %v); it names nothing and must not linger", err)
+	}
+}
+
+// TestStopServer_LegacyPIDFileNamingADeadProcessSaysNotRunning is the same
+// through stop: the user sees "not running", not the unprovable refusal.
+func TestStopServer_LegacyPIDFileNamingADeadProcessSaysNotRunning(t *testing.T) {
+	dir := t.TempDir()
+	cfg := &config.Config{Host: "127.0.0.1", Port: unusedPort(t), DataDir: dir}
+	pid := deadPID(t)
+	if err := os.WriteFile(cfg.PIDFile(), []byte(strconv.Itoa(pid)+"\n"), 0o644); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+
+	err := StopServer(cfg)
+	if err == nil || !strings.Contains(err.Error(), "server not running") {
+		t.Fatalf("StopServer = %v, want a \"server not running\" answer for a legacy record naming a dead pid", err)
+	}
+	if strings.Contains(err.Error(), "cannot confirm") {
+		t.Errorf("a dead legacy pid got the unprovable refusal %q; it is provably stale", err.Error())
+	}
+	if _, statErr := os.Stat(cfg.PIDFile()); !os.IsNotExist(statErr) {
+		t.Errorf("the dead legacy PID file survived stop (stat err = %v)", statErr)
+	}
+}
+
+// TestClaimPIDFile_OverwritesAKeptLegacyRecord is the START half of the lead's
+// question: a legacy file kept as unprovable must not block a new server.
+// `pad server start` never reads the file (EnsureServer probes the port), and
+// the new server's claim takes it over: no lock was ever held on a legacy file,
+// so the claim succeeds and writes this build's fingerprinted record.
+func TestClaimPIDFile_OverwritesAKeptLegacyRecord(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "pad.pid")
+	// Live pid, so the file is in the KEPT state before the claim.
+	if err := os.WriteFile(path, []byte(strconv.Itoa(os.Getpid())+"\n"), 0o644); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+	if _, owner := pidFileOwner(path); owner != pidFileUnprovable {
+		t.Fatalf("PRECONDITION: owner = %v, want the kept (unprovable) state", owner)
+	}
+
+	release := ClaimPIDFile(path)
+	defer release()
+
+	rec, ok := readPIDRecord(path)
+	if !ok || rec.PID != os.Getpid() || rec.StartedAt.IsZero() {
+		t.Fatalf("after the claim the file holds %+v (ok=%v), want this build's fingerprinted record", rec, ok)
+	}
+	if _, owner := pidFileOwner(path); owner != pidFileOurs {
+		t.Errorf("owner = %v after the claim, want ours: the new server must be addressable by stop", owner)
+	}
+}

internal/cli/pidfile_unix.go

@@ -92,6 +92,24 @@ func pidFileOwner(path string) (pidRecord, pidFileOwnership) {
 		}
 		return rec, pidFileUnprovable
 	}
+	if ok && rec.StartedAt.IsZero() && !processIsGone(rec.PID) {
+		// A LEGACY record (BUG-2970): a bare pid, written by a build that
+		// predates the lock (BUG-2969), so a free lock proves nothing about it
+		// — its writer never took one, and may be a live server right now.
+		// Unprovable, as Windows already answers for the same shape, and the
+		// file is LEFT: deleting the record of a live server is the failure
+		// BUG-2969 exists to prevent. Every record this build writes carries a
+		// StartedAt, so only a pre-lock file reaches this branch.
+		//
+		// Only while its pid EXISTS. A pid with no process behind it cannot be
+		// anyone's server, so a legacy record naming one is stale by the same
+		// proof and falls through to the removal below, under this lock. The
+		// kept case is therefore never a trap: `pad server start` probes the
+		// port, not this file, and its claim overwrites it; and stop's
+		// unprovable refusal names the manual remedy (remove the file).
+		_ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN)
+		return rec, pidFileUnprovable
+	}
 	// Stale. Remove the file WHILE STILL HOLDING the lock (codex round 2):
 	// releasing first leaves a window in which a replacement server claims the
 	// path, and the remove then deletes ITS live record — the same

internal/cli/server_stop_honesty_test.go

@@ -9,6 +9,7 @@ import (
 	"strconv"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/PerpetualSoftware/pad/internal/config"
 )
@@ -124,7 +125,7 @@ func TestStopServer_StalePIDFileNamingALiveStrangerIsNotSignalled(t *testing.T)
 	// The record names THIS process — alive, and emphatically not a pad server.
 	// Nothing holds the file, so ownership is stale and the pid must not be
 	// signalled. If it were, this test process would receive SIGTERM.
-	if err := writePIDRecord(cfg.PIDFile(), pidRecord{PID: os.Getpid()}); err != nil {
+	if err := writePIDRecord(cfg.PIDFile(), pidRecord{PID: os.Getpid(), StartedAt: time.Now().UTC()}); err != nil {
 		t.Fatalf("seed: %v", err)
 	}
 
@@ -140,6 +141,39 @@ func TestStopServer_StalePIDFileNamingALiveStrangerIsNotSignalled(t *testing.T)
 	}
 }
 
+// TestStopServer_LegacyPIDFileIsRefusedAndKept is BUG-2970 through stop
+// itself: the day-19 refresh, where the new binary's stop met a bare-pid file
+// written by a running pre-lock server. Refusing was right; deleting the file
+// on the way was not, since it was the only record of a live server. Unix
+// only in effect: Windows already answered unprovable for this shape.
+func TestStopServer_LegacyPIDFileIsRefusedAndKept(t *testing.T) {
+	dir := t.TempDir()
+	cfg := &config.Config{Host: "127.0.0.1", Port: unusedPort(t), DataDir: dir}
+
+	// This live process stands in for the pre-lock server. If stop signalled
+	// it, the test process would receive SIGTERM.
+	legacy := []byte(strconv.Itoa(os.Getpid()) + "\n")
+	if err := os.WriteFile(cfg.PIDFile(), legacy, 0o644); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+
+	err := StopServer(cfg)
+	if err == nil {
+		t.Fatal("StopServer reported success for a legacy record it cannot prove is ours")
+	}
+	if !strings.Contains(err.Error(), "cannot confirm") {
+		t.Errorf("message = %q, want the unprovable refusal (cannot confirm)", err.Error())
+	}
+	// The way out must be NAMED (lead review, day 79): the kept state is
+	// only acceptable if the refusal tells the user how to leave it.
+	if !strings.Contains(err.Error(), "remove "+cfg.PIDFile()) {
+		t.Errorf("message = %q, want it to name the manual remedy (remove %s)", err.Error(), cfg.PIDFile())
+	}
+	if got, readErr := os.ReadFile(cfg.PIDFile()); readErr != nil || string(got) != string(legacy) {
+		t.Errorf("legacy PID file not kept intact: content %q, read err %v", got, readErr)
+	}
+}
+
 // TestStopServer_HeldPIDFileIsAddressable is the counterfactual: without it,
 // "never signal" would be satisfiable by never signalling anything, which would
 // break stop entirely. A held claim must read as ours.