Files
pad/internal/cli/pidfile_windows.go
xarmian 4618876e3e fix(cli): pad server stop signals only a process it can prove is ours (BUG-2969) (#1299)
fix(cli): `pad server stop` signals only a process it can prove is ours (BUG-2969)

Measured on the merged binary before this change: a `sleep 600` whose pid had
been written into the PID file was SIGTERMed, and stop printed "Server stopped."
No pad server was running anywhere near that config.

Three things had to be true at once for that. os.FindProcess succeeds for ANY
pid on Unix. Nothing asked whether the pid belonged to a pad server. And the
confirmation loop polled the PORT — which is unhealthy from the first poll when
nothing was ever serving, so the success check was satisfied by the failure
case.

Liveness is the wrong question, and this is the trap the obvious fix falls into:
the stranger WAS alive. The question is whether the pid is OUR server.

## The discriminator

Unix takes an advisory flock on the PID file, held for the server's lifetime.
`stop` probes it non-blockingly: acquiring it proves nobody holds the file, so
the record is stale whatever the pid now names; failing to acquire proves a live
pad server holds THIS file. One implementation for Linux and macOS, no new
dependency, and the same primitive session_lock_unix.go has used since
TASK-2767.

Windows has no flock in that pattern, so it compares the process creation time
from GetProcessTimes against the one recorded at start — the attribute that
survives pid reuse, since a reused pid belongs to a process that started later.

The lead first ruled start-time comparison on every platform; I objected with
the cost (three implementations — /proc, a macOS sysctl promoting x/sys to a
direct dependency, and GetProcessTimes) and the ruling changed to this hybrid.
The cost table is on the item so the next reader sees why the shape moved.

The PID file gains a fingerprint on both platforms — pid, start time, executable
path — as JSON, with the legacy bare-integer form still parsed. A legacy record
carries no proof, which reads as UNPROVABLE, and unprovable means nothing is
signalled.

## Three races, each found by codex and each the same shape

1. Reading the record and checking ownership were separate steps, so a successor
   could claim the file between them: the lock then reported "held" — truthfully,
   about the successor — while the pid handed back was the predecessor's.
   pidFileOwner now returns the record it read from the descriptor it probed.
2. Removing the PID file after a successful stop could delete a fast successor's
   live record. It no longer removes at all there: the server removes its own on
   the way down, and a file left by a crash is handled by the next stop.
3. Removing a STALE file after the probe released the lock had the same window.
   The removal now happens inside the ownership check, while the lock is held —
   the only moment at which no replacement can have claimed the path. A claim
   arriving during that instant retries for half a second rather than losing its
   claim for the life of the process.

Windows deliberately does NOT delete a stale file: with no atomic primitive, a
check-then-remove would race a successor, and a stale file that the next start
overwrites is recoverable where a wrongly deleted record is not.

## Verified

Negative control, and it is the literal one: with the ownership check bypassed,
`go test` reports `signal: terminated` — the test binary is SIGTERMed by the
code under test, because the stale record names the test process itself.

Live, in throwaway HOMEs: a stale record naming a live `sleep` is refused and the
sleep survives (it was killed before this change); a stale record with a HEALTHY
port answering is still refused, nothing signalled, and both the stranger and the
real server survive; a server stopped through its own held record stops, and its
file is gone.

The CI smoke on windows-latest now stops the server with `pad server stop`
instead of Stop-Process, because that is the only place the Windows ownership
check runs — a smoke that killed the process directly would leave the
GetProcessTimes path unexercised on every platform.

make lint, make test green; codex CLEAN in round 4.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR
2026-09-08 19:33:15 -04:00

97 lines
3.3 KiB
Go

//go:build windows
package cli
import (
"time"
"golang.org/x/sys/windows"
)
// holdPIDFile is a no-op on Windows: there is no flock in the pattern this
// package uses elsewhere (session_lock_other.go documents the same boundary).
// Ownership here is decided by the recorded creation time instead — see
// pidFileOwner — so the PID file needs no lock held across the process's life.
func holdPIDFile(path string) (release func(), err error) {
return func() {}, nil
}
// pidFileOwner reports whether the pid in rec is still the process that wrote
// the record, by comparing the OS-reported creation time against the one
// recorded at start.
//
// Creation time is the discriminator that survives pid reuse: a reused pid
// belongs to a process that started later, so the timestamps differ. Compared
// exactly — both sides come from GetProcessTimes, so there is no clock skew to
// tolerate and a tolerance window would only admit a collision.
func pidFileOwner(path string) (pidRecord, pidFileOwnership) {
rec, ok := readPIDRecord(path)
if !ok {
return pidRecord{}, pidFileStale
}
if rec.StartedAt.IsZero() {
// A legacy bare-pid file, or one written before the fingerprint
// existed. Unprovable — and unprovable means we do not signal.
return rec, pidFileUnprovable
}
started := processStartTime(rec.PID)
if started.IsZero() {
// No such process (or no rights to ask). Nothing holds this record.
return rec, pidFileStale
}
if started.Equal(rec.StartedAt) {
return rec, pidFileOurs
}
// The pid is live but was created at a different moment: it is a REUSE,
// not our server.
return rec, pidFileStale
}
// Windows deliberately does NOT delete a stale PID file, where Unix removes it
// under the lock it already holds (codex round 3).
//
// There is no lock here to make check-then-remove atomic, so a successor that
// claims the path between the two would have its live record deleted and be
// unaddressable for the rest of its life. Weigh the two outcomes: a stale file
// left behind is overwritten by the next `pad server start`, which claims the
// path unconditionally, and until then `stop` reports it as stale and signals
// nothing. A wrongly deleted record has no such recovery. So the platform
// without an atomic primitive does the harmless half and leaves cleanup to the
// next writer.
// processStartTime returns pid's creation time, or the zero time when the
// process does not exist or cannot be opened.
func processStartTime(pid int) time.Time {
h, err := windows.OpenProcess(windows.PROCESS_QUERY_LIMITED_INFORMATION, false, uint32(pid))
if err != nil {
return time.Time{}
}
defer windows.CloseHandle(h)
var creation, exit, kernel, user windows.Filetime
if err := windows.GetProcessTimes(h, &creation, &exit, &kernel, &user); err != nil {
return time.Time{}
}
return time.Unix(0, creation.Nanoseconds()).UTC()
}
// processIsGone reports whether pid has exited.
func processIsGone(pid int) bool {
if pid <= 0 {
return true
}
h, err := windows.OpenProcess(windows.PROCESS_QUERY_LIMITED_INFORMATION, false, uint32(pid))
if err != nil {
return true
}
defer windows.CloseHandle(h)
var code uint32
if err := windows.GetExitCodeProcess(h, &code); err != nil {
return true
}
const stillActive = 259 // STILL_ACTIVE
return code != stillActive
}