diff --git a/internal/cli/pid_file_test.go b/internal/cli/pid_file_test.go index c2607839d..137d10c47 100644 --- a/internal/cli/pid_file_test.go +++ b/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) + } +} diff --git a/internal/cli/pidfile_unix.go b/internal/cli/pidfile_unix.go index abc4880f8..d38eef96c 100644 --- a/internal/cli/pidfile_unix.go +++ b/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 diff --git a/internal/cli/server_stop_honesty_test.go b/internal/cli/server_stop_honesty_test.go index 563a5c195..9f12718d4 100644 --- a/internal/cli/server_stop_honesty_test.go +++ b/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.