From f683bed28d78f31911d08b64c121a839e35b4d94 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 8 Sep 2026 15:52:37 -0400 Subject: [PATCH 1/3] feat(stack): read the process identity token via sysctl on darwin (RIG-3548) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit compass-stack up records a start-time identity token for every child it spawns, to close the pid-recycling window. The only reader was /proc//stat, so up refused on macOS: pgidfile.go is //go:build unix, which darwin satisfies, meaning darwin compiled the Linux reader and failed at runtime rather than at build time. Add a darwin reader at BOTH identity sites, split behind the existing seams into _linux.go/_darwin.go files. The token is read at spawn by the core and independently at teardown by the group-signal adapter, and GroupSignaller.Alive compares the two for uint64 equality — so both darwin readers share one packing rule (sec*1e6 + usec over the KinfoProc start timeval, via sysctl kern.proc.pid), pinned by mirrored tests in both packages. A drift between them would report every live child as not-alive and silently skip it at teardown, which is why the duplication is pinned rather than trusted. Both readers fail closed on a dead pid, so a failed read can never yield a 0 token that would spuriously match a record carrying 0. The darwin CI lane gains a step running the stack suite natively, because cross-compiling type-checks these readers but never executes them, and the sysctl has no Linux stand-in. The readers have NOT run on a real macOS host yet; that step is what will prove them. Promotes golang.org/x/sys to a direct dependency (already in the module graph); go.sum unchanged. Refs RIG-3548, RIG-1662. Co-authored-by: Matt Wilkinson --- .github/workflows/ci.yml | 24 +++++++ go/go.mod | 3 +- go/internal/stack/adapters/groupsignal.go | 34 +++------- .../stack/adapters/groupsignal_darwin.go | 53 +++++++++++++++ .../stack/adapters/groupsignal_darwin_test.go | 61 ++++++++++++++++++ .../stack/adapters/groupsignal_linux.go | 27 ++++++++ .../stack/adapters/groupsignal_other.go | 23 +++++++ .../stack/adapters/groupsignal_test.go | 5 +- go/internal/stack/pgidfile.go | 52 ++++++--------- go/internal/stack/readstarttime_darwin.go | 58 +++++++++++++++++ .../stack/readstarttime_darwin_test.go | 64 +++++++++++++++++++ go/internal/stack/readstarttime_linux.go | 35 ++++++++++ go/internal/stack/readstarttime_other.go | 27 ++++++++ 13 files changed, 407 insertions(+), 59 deletions(-) create mode 100644 go/internal/stack/adapters/groupsignal_darwin.go create mode 100644 go/internal/stack/adapters/groupsignal_darwin_test.go create mode 100644 go/internal/stack/adapters/groupsignal_linux.go create mode 100644 go/internal/stack/adapters/groupsignal_other.go create mode 100644 go/internal/stack/readstarttime_darwin.go create mode 100644 go/internal/stack/readstarttime_darwin_test.go create mode 100644 go/internal/stack/readstarttime_linux.go create mode 100644 go/internal/stack/readstarttime_other.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7b09ed1fe..1d776fd92 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1558,6 +1558,30 @@ jobs: echo "$store/bin" >>"$GITHUB_PATH" done + - name: Stack supervision unit tests (native darwin) + # The ONLY lane that EXECUTES the darwin process start-time identity + # reader. That reader is what lets `compass-stack up` run on macOS at + # all, and it is a two-site swap: the core records a token at spawn and + # the teardown adapter reads one back independently, with + # GroupSignaller.Alive comparing the two for uint64 equality. If the two + # darwin encodings ever drift, `down` matches nothing and SILENTLY skips + # every live child — no error, no signal, just an orphaned stack. Cross + # compiling from ubuntu type-checks those readers but never runs them, + # and the sysctl they call has no Linux equivalent to stand in, so this + # step is the only thing standing between a drifted encoding and a green + # PR. The suite is untagged: internal/stack is `//go:build unix`, which + # darwin satisfies, so a bare `go test` selects the _darwin.go readers + # and their mirrored packing tests with no `-tags` flag. + # + # It runs BEFORE the compile+bundle gate so an identity-reader + # regression reds fast rather than after the ~minutes-long bundle wrap. + # Deliberately NOT affected-guarded like that gate: this is seconds of + # pure-Go test on a runner the job already paid to boot, and the guard's + # own path list is the thing most likely to go stale. + env: + CGO_ENABLED: '0' + run: go -C go test -count=1 ./internal/stack/... + - name: macOS compile + bundle gate # The ONE CI lane that compiles the native shell on darwin + exercises # the macos-bundle tool end to end (compass-distribution T3). It is a diff --git a/go/go.mod b/go/go.mod index 8245db148..9af915170 100644 --- a/go/go.mod +++ b/go/go.mod @@ -55,6 +55,8 @@ require ( google.golang.org/protobuf v1.36.12 ) +require golang.org/x/sys v0.47.0 + require ( github.com/adrg/xdg v0.5.3 // indirect github.com/antithesishq/antithesis-sdk-go v0.7.2-default-no-op // indirect @@ -108,7 +110,6 @@ require ( go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.46.0 // indirect go.opentelemetry.io/proto/otlp v1.11.0 // indirect golang.org/x/crypto v0.55.0 // indirect - golang.org/x/sys v0.47.0 // indirect golang.org/x/text v0.41.0 // indirect golang.org/x/time v0.15.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260819154853-08b0e4226688 // indirect diff --git a/go/internal/stack/adapters/groupsignal.go b/go/internal/stack/adapters/groupsignal.go index 115b4bc2e..b65132c52 100644 --- a/go/internal/stack/adapters/groupsignal.go +++ b/go/internal/stack/adapters/groupsignal.go @@ -5,7 +5,6 @@ package adapters import ( "errors" "fmt" - "os" "strconv" "strings" "syscall" @@ -64,10 +63,10 @@ func (g *GroupSignaller) Signal(pgid int, sig stack.ProcessSignal) error { // gone-or-recycled group as if it were the original child. // // The two checks are ordered existence-then-identity: the kill(0) probe cheaply -// rules out the ESRCH case, then the /proc start-time read confirms the leader -// is the same process. A start-time read failure (the leader vanished between -// the two syscalls, or /proc is unavailable) is treated as not-alive — the safe -// verdict is never to signal. +// rules out the ESRCH case, then the start-time read confirms the leader is the +// same process. A start-time read failure (the leader vanished between the two +// syscalls, or the kernel's process table is unreadable) is treated as +// not-alive — the safe verdict is never to signal. func (g *GroupSignaller) Alive(pgid int, startTime uint64) bool { // A degenerate pgid is never a live compass child: kill(-1, 0) probes the // whole session and kill(0, 0) the caller's own group, both of which would @@ -90,27 +89,12 @@ func (g *GroupSignaller) Alive(pgid int, startTime uint64) bool { return got == startTime } -// readGroupLeaderStartTime reads field 22 (starttime) of /proc//stat — the -// group leader, since pid == pgid for a Setpgid child. It duplicates the core's -// parser (rather than exporting it across the package boundary) because the -// parenthesized-comm gotcha is the same on both sides and the two are read-only -// leaf helpers; see stack.parseStatStartTime for the full explanation. -func readGroupLeaderStartTime(pgid int) (uint64, error) { - data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pgid)) - if err != nil { - return 0, fmt.Errorf("read /proc/%d/stat: %w", pgid, err) - } - startTime, err := parseGroupLeaderStat(string(data)) - if err != nil { - return 0, fmt.Errorf("/proc/%d/stat: %w", pgid, err) - } - return startTime, nil -} - // parseGroupLeaderStat extracts field 22 (starttime) from a /proc//stat -// line. Split out from readGroupLeaderStartTime so the parenthesized-comm parse -// is unit-tested against synthesized lines without a live process — the same -// split (and the same gotcha) as stack.parseStatStartTime. +// line. Split out from the Linux reader (groupsignal_linux.go) so the +// parenthesized-comm parse is unit-tested against synthesized lines without a +// live process — the same split (and the same gotcha) as +// stack.parseStatStartTime. It stays in the unix-built file so that test +// compiles on every unix: the parse rule is pure text handling. func parseGroupLeaderStat(line string) (uint64, error) { // comm (field 2) is parenthesized and may contain spaces AND parens, so // count fields from the LAST ')'; field[0] after it is state (field 3), so diff --git a/go/internal/stack/adapters/groupsignal_darwin.go b/go/internal/stack/adapters/groupsignal_darwin.go new file mode 100644 index 000000000..96a8606f7 --- /dev/null +++ b/go/internal/stack/adapters/groupsignal_darwin.go @@ -0,0 +1,53 @@ +//go:build darwin + +package adapters + +import ( + "fmt" + + "golang.org/x/sys/unix" +) + +// readGroupLeaderStartTime reads the group leader's start time from the +// kernel's process table — the darwin half of the teardown-side identity read. +// There is no /proc on darwin, so the token comes from sysctl +// kern.proc.pid., whose KinfoProc carries the process's creation timeval +// (Proc.P_starttime). The leader's pid is the pgid, since pid == pgid for a +// Setpgid child. +// +// A pgid that names no process yields an error rather than a zero token: the +// kernel returns a short result for an unknown pid, which SysctlKinfoProc +// rejects, and the explicit zero-timeval guard closes the remaining case. Alive +// then reports not-alive, so the identity check fails closed. +func readGroupLeaderStartTime(pgid int) (uint64, error) { + kp, err := unix.SysctlKinfoProc("kern.proc.pid", pgid) + if err != nil { + return 0, fmt.Errorf("sysctl kern.proc.pid.%d: %w", pgid, err) + } + tv := kp.Proc.P_starttime + // Sec alone is the guard, matching the spawn side (stack.readProcessStartTime): + // Sec is signed, so a negative would pack into a huge uint64 that looks like + // a valid token, while Usec == 0 is a legitimate exact-second start and must + // not be rejected. + if tv.Sec <= 0 { + return 0, fmt.Errorf("sysctl kern.proc.pid.%d: no usable start timeval (sec=%d usec=%d)", + pgid, tv.Sec, tv.Usec) + } + return packGroupLeaderTimeval(tv), nil +} + +// packGroupLeaderTimeval flattens a process-creation timeval into the uint64 +// identity token the pgid record carries. +// +// It MUST stay byte-identical in effect to stack.packStartTimeval, which the +// spawn side uses to write the token this reads back. The two packages cannot +// import each other's internals, so the expression is duplicated for the same +// reason parseGroupLeaderStat duplicates stack.parseStatStartTime — and here the +// duplication is the load-bearing one: Alive compares this against a token the +// spawn side produced, for uint64 equality, so any drift would report every live +// child as not-alive and silently skip it at teardown. The mirror test +// (groupsignal_darwin_test.go) feeds one synthetic timeval through both packings +// and asserts the same uint64, so a one-sided change reds. +func packGroupLeaderTimeval(tv unix.Timeval) uint64 { + return uint64(tv.Sec)*1_000_000 + uint64(tv.Usec) +} diff --git a/go/internal/stack/adapters/groupsignal_darwin_test.go b/go/internal/stack/adapters/groupsignal_darwin_test.go new file mode 100644 index 000000000..ef05863e0 --- /dev/null +++ b/go/internal/stack/adapters/groupsignal_darwin_test.go @@ -0,0 +1,61 @@ +//go:build darwin + +package adapters + +import ( + "testing" + + "golang.org/x/sys/unix" +) + +// TestPackGroupLeaderTimevalMatchesSpawnSide is the down half of the mirror-test +// pair that pins the darwin identity encoding, exactly as +// TestParseGroupLeaderStatParsesParenthesizedComm mirrors the Linux parser. It +// feeds the SAME synthetic timeval the spawn-side test +// (stack.TestPackStartTimevalMatchesDownSide) uses and asserts the same literal +// uint64. +// +// The duplication it guards is load-bearing: Alive compares this packing's +// output against a token stack.packStartTimeval produced at spawn, for uint64 +// equality, so a one-sided change to either expression would report every live +// child as not-alive and silently skip it at teardown — the worst teardown +// failure available, because it is silent. +func TestPackGroupLeaderTimevalMatchesSpawnSide(t *testing.T) { + tv := unix.Timeval{Sec: 1_700_000_123, Usec: 456_789} + const want = uint64(1_700_000_123)*1_000_000 + 456_789 + if got := packGroupLeaderTimeval(tv); got != want { + t.Fatalf("packGroupLeaderTimeval(%d.%06d) = %d, want %d", tv.Sec, tv.Usec, got, want) + } +} + +// TestReadGroupLeaderStartTimeSelfIsStable drives the real darwin sysctl reader +// against a live process (this one, its own group leader candidate): the token +// must be non-zero and identical across two reads, or the identity gate would +// stop matching a group moments after it was recorded. +func TestReadGroupLeaderStartTimeSelfIsStable(t *testing.T) { + pid := unix.Getpid() + first, err := readGroupLeaderStartTime(pid) + if err != nil { + t.Fatalf("readGroupLeaderStartTime(%d) = %v", pid, err) + } + if first == 0 { + t.Fatalf("readGroupLeaderStartTime(%d) = 0, want a non-zero identity token", pid) + } + second, err := readGroupLeaderStartTime(pid) + if err != nil { + t.Fatalf("readGroupLeaderStartTime(%d) second read = %v", pid, err) + } + if first != second { + t.Fatalf("start time not stable across reads: %d then %d", first, second) + } +} + +// TestReadGroupLeaderStartTimeDeadPGIDErrors proves the reader fails closed for +// a pgid that names no process, so Alive reports not-alive rather than matching +// on a zero token. +func TestReadGroupLeaderStartTimeDeadPGIDErrors(t *testing.T) { + dead := deadPGID(t) + if got, err := readGroupLeaderStartTime(dead); err == nil { + t.Fatalf("readGroupLeaderStartTime(%d) = %d, nil for a dead pgid; want an error", dead, got) + } +} diff --git a/go/internal/stack/adapters/groupsignal_linux.go b/go/internal/stack/adapters/groupsignal_linux.go new file mode 100644 index 000000000..94dff9b6d --- /dev/null +++ b/go/internal/stack/adapters/groupsignal_linux.go @@ -0,0 +1,27 @@ +//go:build linux + +package adapters + +import ( + "fmt" + "os" +) + +// readGroupLeaderStartTime reads field 22 (starttime, in clock ticks since +// boot) of /proc//stat — the Linux half of the teardown-side identity +// read. The parenthesized-comm parse rule lives in parseGroupLeaderStat +// (groupsignal.go) so it is unit-testable without a live process. +// +// A pgid that names no process has no /proc entry, so the read fails and Alive +// reports not-alive — the identity check fails closed and never signals. +func readGroupLeaderStartTime(pgid int) (uint64, error) { + data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pgid)) + if err != nil { + return 0, fmt.Errorf("read /proc/%d/stat: %w", pgid, err) + } + startTime, err := parseGroupLeaderStat(string(data)) + if err != nil { + return 0, fmt.Errorf("/proc/%d/stat: %w", pgid, err) + } + return startTime, nil +} diff --git a/go/internal/stack/adapters/groupsignal_other.go b/go/internal/stack/adapters/groupsignal_other.go new file mode 100644 index 000000000..8cfaab5e1 --- /dev/null +++ b/go/internal/stack/adapters/groupsignal_other.go @@ -0,0 +1,23 @@ +//go:build unix && !linux && !darwin + +package adapters + +import ( + "fmt" + "runtime" +) + +// readGroupLeaderStartTime refuses on a unix that is neither linux nor darwin, +// for the same reason as the spawn-side reader (stack.readProcessStartTime): +// groupsignal.go is //go:build unix, so this symbol must exist under every +// build constraint the package accepts. +// +// Refusing is fail-closed here too. Alive treats a read error as not-alive, so +// an unsupported host reports no live group rather than claiming one — the safe +// direction, since the alternative is signalling a pid the token cannot vouch +// for. +func readGroupLeaderStartTime(pgid int) (uint64, error) { + return 0, fmt.Errorf( + "reading the start-time identity token for process group %d is not implemented on %s "+ + "(the supervised stack runs on linux and darwin)", pgid, runtime.GOOS) +} diff --git a/go/internal/stack/adapters/groupsignal_test.go b/go/internal/stack/adapters/groupsignal_test.go index b13adc952..402fbd512 100644 --- a/go/internal/stack/adapters/groupsignal_test.go +++ b/go/internal/stack/adapters/groupsignal_test.go @@ -78,10 +78,11 @@ func TestGroupSignallerUnknownSignal(t *testing.T) { // of the /proc//stat field-22 parse against the parenthesized-comm gotcha: // a comm with embedded spaces AND parens must not throw off the field count. It // mirrors stack.TestReadStartTimeProcParsesParenthesizedComm so the two parsers -// (deliberately duplicated, groupsignal.go:93-97) cannot drift on the +// (deliberately duplicated, see readGroupLeaderStartTime) cannot drift on the // load-bearing identity token — a "simplify to strings.Fields(line)" regression // here would be caught rather than only by the real-/proc integration test whose -// comm has no embedded spaces. +// comm has no embedded spaces. The darwin encoding has its own mirrored pair, +// TestPackGroupLeaderTimevalMatchesSpawnSide, for the same reason. func TestParseGroupLeaderStatParsesParenthesizedComm(t *testing.T) { // comm is "(weird )(name)" — embedded spaces and parens; starttime (field 22) // is 987654. diff --git a/go/internal/stack/pgidfile.go b/go/internal/stack/pgidfile.go index db65e00ad..0795d3b3a 100644 --- a/go/internal/stack/pgidfile.go +++ b/go/internal/stack/pgidfile.go @@ -331,40 +331,30 @@ func removePgidFile(stateDir string) error { } // readStartTime is the package-internal seam that reads a process's start time -// (the identity token). It is a var, not a func, so tests can stub it without a -// live process. The wired implementation reads /proc//stat, which exists -// only on Linux — and the embedded stack is Linux/podman-only at runtime anyway -// (the runner loop, compass-native-app design.md:247,346-348), so the seam is a -// test seam, not a cross-OS portability claim: on a non-Linux unix this reader -// fails and up refuses, which is the correct outcome on an unsupported host. -var readStartTime = readStartTimeProc - -// readStartTimeProc reads field 22 (starttime, in clock ticks since boot) of -// /proc//stat. +// — the identity token that closes the pid-recycling window. It is a var, not a +// func, so tests can stub it without a live process. // -// The parse gotcha: field 2 (comm) is the executable name wrapped in -// parentheses and MAY itself contain spaces AND parentheses (e.g. a process -// named "(ec) foo"), so splitting the whole line on whitespace miscounts. The -// robust parse the kernel documents (proc(5)) is to find the LAST ')' — comm is -// the only parenthesized field and everything after it is space-separated -// fixed-position fields — then count fields from there. After the last ')': -// field[0] is state (field 3), so starttime (field 22) is field[22-3] = index -// 19 of the post-comm split. -func readStartTimeProc(pid int) (uint64, error) { - data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pid)) - if err != nil { - return 0, fmt.Errorf("read /proc/%d/stat: %w", pid, err) - } - startTime, err := parseStatStartTime(string(data)) - if err != nil { - return 0, fmt.Errorf("/proc/%d/stat: %w", pid, err) - } - return startTime, nil -} +// It is BOTH a test seam and a cross-OS seam. The wired implementation is +// per-OS (readstarttime_linux.go reads /proc//stat, readstarttime_darwin.go +// reads the KinfoProc start timeval via sysctl), and each OS's encoding is its +// own: Linux clock-ticks-since-boot and darwin microseconds-since-epoch are +// never compared against each other, because a token is written and read on one +// host. +// +// The invariant that IS load-bearing: this spawn-side reader and the down-side +// reader (adapters.readGroupLeaderStartTime) must produce the IDENTICAL encoding +// on a given OS. GroupSignaller.Alive compares the two for uint64 equality, so a +// disagreement would report every live child as not-alive and silently skip it +// at teardown. The two darwin readers therefore share one packing rule +// (sec*1e6 + usec), pinned by mirrored unit tests in both packages. +var readStartTime = readProcessStartTime // parseStatStartTime extracts field 22 (starttime) from a /proc//stat line. -// Split out from readStartTimeProc so the parenthesized-comm parse is unit-tested -// against synthesized lines without a live process. +// Split out from the Linux reader (readstarttime_linux.go) so the +// parenthesized-comm parse is unit-tested against synthesized lines without a +// live process. It stays in the unix-built file, not the _linux one, so that +// test compiles and runs on every unix — the parse rule is pure text handling +// with no /proc dependency of its own. func parseStatStartTime(line string) (uint64, error) { rparen := strings.LastIndexByte(line, ')') if rparen < 0 { diff --git a/go/internal/stack/readstarttime_darwin.go b/go/internal/stack/readstarttime_darwin.go new file mode 100644 index 000000000..68ca2e2a8 --- /dev/null +++ b/go/internal/stack/readstarttime_darwin.go @@ -0,0 +1,58 @@ +//go:build darwin + +package stack + +import ( + "fmt" + + "golang.org/x/sys/unix" +) + +// readProcessStartTime reads the process's start time from the kernel's process +// table — the darwin half of the readStartTime seam. There is no /proc on +// darwin, so the token comes from sysctl kern.proc.pid., whose KinfoProc +// carries the process's creation timeval (Proc.P_starttime). +// +// A pid that does not exist yields an error rather than a zero token: the +// kernel returns a short (zero-length) result for an unknown pid, which +// SysctlKinfoProc rejects, and the explicit zero-timeval guard below closes the +// remaining case. This matters because the token feeds an equality check — a +// reader that quietly returned 0 for a dead pid would match any record that +// happened to carry 0, so the read must fail closed. +func readProcessStartTime(pid int) (uint64, error) { + kp, err := unix.SysctlKinfoProc("kern.proc.pid", pid) + if err != nil { + return 0, fmt.Errorf("sysctl kern.proc.pid.%d: %w", pid, err) + } + tv := kp.Proc.P_starttime + // Sec alone is the guard. A real process start time is a wall-clock epoch + // second, so any value <= 0 means the kernel gave us nothing usable — and + // Sec is signed, so a negative would otherwise pack into a huge uint64 that + // looks like a valid token. Usec is deliberately NOT part of the condition: + // a process really can start on an exact second boundary, and rejecting + // Usec == 0 would fail a legitimate read about once in a million. + if tv.Sec <= 0 { + return 0, fmt.Errorf("sysctl kern.proc.pid.%d: no usable start timeval (sec=%d usec=%d)", + pid, tv.Sec, tv.Usec) + } + return packStartTimeval(tv), nil +} + +// packStartTimeval flattens a process-creation timeval into the uint64 identity +// token the pgid record carries. +// +// This expression is DUPLICATED in adapters.packGroupLeaderTimeval, which the +// teardown side uses, for the same reason the /proc field-22 parse is +// duplicated there: the two are read-only leaf helpers and the packages cannot +// reach into each other. The duplication is load-bearing rather than incidental +// — GroupSignaller.Alive compares a spawn-side token against a down-side read +// for uint64 equality, so a drift between the two packings would report every +// live child as not-alive and silently skip it at teardown. Mirrored tests in +// both packages feed one synthetic timeval through both and assert the same +// uint64, so a change to one packing without the other reds. +// +// Microseconds since the epoch fits a uint64 for ~584,000 years, so the +// multiply cannot overflow for any real process. +func packStartTimeval(tv unix.Timeval) uint64 { + return uint64(tv.Sec)*1_000_000 + uint64(tv.Usec) +} diff --git a/go/internal/stack/readstarttime_darwin_test.go b/go/internal/stack/readstarttime_darwin_test.go new file mode 100644 index 000000000..bdcc09d24 --- /dev/null +++ b/go/internal/stack/readstarttime_darwin_test.go @@ -0,0 +1,64 @@ +//go:build darwin + +package stack + +import ( + "os" + "testing" + + "golang.org/x/sys/unix" +) + +// TestPackStartTimevalMatchesDownSide is the spawn half of the mirror-test pair +// that pins the darwin identity encoding. The down side +// (adapters.TestPackGroupLeaderTimevalMatchesSpawnSide) feeds the SAME synthetic +// timeval through its own packing and asserts this same literal. +// +// It exists because the two packings are deliberately duplicated across a +// package boundary the packages cannot cross, and GroupSignaller.Alive compares +// their outputs for uint64 equality — a drift would report every live child as +// not-alive and silently skip it at teardown. Both tests must be updated +// together or one reds, which is the point. +func TestPackStartTimevalMatchesDownSide(t *testing.T) { + tv := unix.Timeval{Sec: 1_700_000_123, Usec: 456_789} + const want = uint64(1_700_000_123)*1_000_000 + 456_789 + if got := packStartTimeval(tv); got != want { + t.Fatalf("packStartTimeval(%d.%06d) = %d, want %d", tv.Sec, tv.Usec, got, want) + } +} + +// TestReadProcessStartTimeSelfIsStable drives the real darwin sysctl reader +// against a live process (this one): the token must be non-zero and identical +// across two reads. A start time that moved between reads, or came back zero, +// would break the identity gate — Alive would stop matching a group it spawned +// moments earlier and skip it at teardown. +func TestReadProcessStartTimeSelfIsStable(t *testing.T) { + pid := os.Getpid() + first, err := readProcessStartTime(pid) + if err != nil { + t.Fatalf("readProcessStartTime(%d) = %v", pid, err) + } + if first == 0 { + t.Fatalf("readProcessStartTime(%d) = 0, want a non-zero identity token", pid) + } + second, err := readProcessStartTime(pid) + if err != nil { + t.Fatalf("readProcessStartTime(%d) second read = %v", pid, err) + } + if first != second { + t.Fatalf("start time not stable across reads: %d then %d", first, second) + } +} + +// TestReadProcessStartTimeDeadPidErrors proves the reader fails closed for a pid +// that names no process. It must NOT return a zero token: the token feeds an +// equality check, so a silent 0 would match any record carrying 0 and signal a +// group that is not ours. The dead pid comes from the package's existing +// deadPID scan (lockfile_test.go), not a guessed constant a busy host could +// have live. +func TestReadProcessStartTimeDeadPidErrors(t *testing.T) { + dead := deadPID(t) + if got, err := readProcessStartTime(dead); err == nil { + t.Fatalf("readProcessStartTime(%d) = %d, nil for a dead pid; want an error", dead, got) + } +} diff --git a/go/internal/stack/readstarttime_linux.go b/go/internal/stack/readstarttime_linux.go new file mode 100644 index 000000000..4a532adc7 --- /dev/null +++ b/go/internal/stack/readstarttime_linux.go @@ -0,0 +1,35 @@ +//go:build linux + +package stack + +import ( + "fmt" + "os" +) + +// readProcessStartTime reads field 22 (starttime, in clock ticks since boot) of +// /proc//stat — the Linux half of the readStartTime seam. +// +// The parse gotcha: field 2 (comm) is the executable name wrapped in +// parentheses and MAY itself contain spaces AND parentheses (e.g. a process +// named "(ec) foo"), so splitting the whole line on whitespace miscounts. The +// robust parse the kernel documents (proc(5)) is to find the LAST ')' — comm is +// the only parenthesized field and everything after it is space-separated +// fixed-position fields — then count fields from there. After the last ')': +// field[0] is state (field 3), so starttime (field 22) is field[22-3] = index +// 19 of the post-comm split. That rule lives in parseStatStartTime (pgidfile.go) +// so it is unit-testable without a live process. +// +// A pid that does not exist has no /proc entry, so the read fails and the +// identity check fails closed — it never matches a recorded token. +func readProcessStartTime(pid int) (uint64, error) { + data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pid)) + if err != nil { + return 0, fmt.Errorf("read /proc/%d/stat: %w", pid, err) + } + startTime, err := parseStatStartTime(string(data)) + if err != nil { + return 0, fmt.Errorf("/proc/%d/stat: %w", pid, err) + } + return startTime, nil +} diff --git a/go/internal/stack/readstarttime_other.go b/go/internal/stack/readstarttime_other.go new file mode 100644 index 000000000..ca30e5b55 --- /dev/null +++ b/go/internal/stack/readstarttime_other.go @@ -0,0 +1,27 @@ +//go:build unix && !linux && !darwin + +package stack + +import ( + "fmt" + "runtime" +) + +// readProcessStartTime refuses on a unix that is neither linux nor darwin. +// +// The seam must RESOLVE on every OS this package is built for: pgidfile.go is +// //go:build unix, so readProcessStartTime has to exist under every build +// constraint the package accepts or the package does not compile there at all. +// The BSDs and solaris satisfy `unix`, and each reads process start time its +// own way, so there is no reader here to share. +// +// A named refusal, not a build break: the supervised stack ships on linux and +// darwin, and a compile failure on an OS nobody targets is a worse outcome than +// a legible runtime error for anyone who tries. The refusal is also the +// fail-closed answer — an identity token that cannot be read must never come +// back as a value that might compare equal. +func readProcessStartTime(pid int) (uint64, error) { + return 0, fmt.Errorf( + "reading the start-time identity token for pid %d is not implemented on %s "+ + "(the supervised stack runs on linux and darwin)", pid, runtime.GOOS) +} From d9245577d68903c25267d761d9a771d8547b3916 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 8 Sep 2026 15:52:37 -0400 Subject: [PATCH 2/3] feat(app): make the darwin podman-machine check real and provisioning-aware (RIG-3202) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Embedded preflight guarded the podman-machine check with `GOOS == "darwin" && MachineReady != nil`, and realPreflight never assigned MachineReady. So on a Mac the check did not fail — it VANISHED from the results, and a caller classifying by result read that as a pass. A user got an all-green preflight followed by an undiagnosed failure further down. Wire a real darwin adapter and close the hole that let it be skipped: - The darwin branch keys on GOOS alone. A nil adapter is now reported as a failed check naming itself a wiring defect, so a wiring regression can no longer turn a broken host into a green preflight. Linux behaviour is unchanged (no machine there, check correctly absent). - The adapter probes `machine ls` then `machine inspect`, and CONNECTS to the forwarded API socket rather than stat-ing it, since a stale forward from a half-stopped machine satisfies a stat. Three failing states are distinguished in the copy — no machine, stopped, running with an unreachable socket — each naming the command to run. - Provisioning is an ensure step (init/start, then RE-PROBE), mirroring how up ensures the image and database rather than gating on them. Readiness comes from the re-probe, never from init's exit status. An unclassifiable CLI answer provisions nothing and is reported from the first probe: init over a machine whose state cannot be read is destructive, and re-probing would report the second answer over the first. - Unparseable output classifies as unknown, which is never ready. machineStatus's zero value is unknown so a failed classification cannot read as ready. - The bring-up window now budgets for a cold provision on darwin. A cold machine init downloads a VM image before any container image is pulled, so the previous 180s ceiling would have deadlined every first launch on a fresh Mac and blamed the timeout rather than the download. The darwin CI lane now runs these tests. They sit behind `(linux && gtk4) || darwin`, so the untagged moon lane excluded them and the gtk4 lane's `-run E2E` filter never matched them — the wiring assertion that catches this exact silent-skip regression ran nowhere. Each required test is asserted by its own PASS line, since `-run` exits 0 when it matches nothing. The OS is a parameter rather than a build tag, so the regression this closes is testable from a linux host. The macOS spike has NOT run. Every podman-CLI output shape here is designed-against-assumption per the record's own convention: the parse accepts both the Running bool and the State string, and degrades rather than trusting either. macOS provisioning is not claimed to work. Refs RIG-3202, RIG-1662. Co-authored-by: Matt Wilkinson --- .github/workflows/ci.yml | 47 ++- go/cmd/compass-app/embedded.go | 32 +- go/cmd/compass-app/embedded_test.go | 66 ++++ go/cmd/compass-app/machine.go | 423 ++++++++++++++++++++++++ go/cmd/compass-app/machine_test.go | 371 +++++++++++++++++++++ go/cmd/compass-app/main.go | 31 +- go/cmd/compass-app/main_test.go | 28 ++ go/internal/preflight/preflight.go | 33 +- go/internal/preflight/preflight_test.go | 44 ++- 9 files changed, 1032 insertions(+), 43 deletions(-) create mode 100644 go/cmd/compass-app/machine.go create mode 100644 go/cmd/compass-app/machine_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1d776fd92..12bddb218 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1674,21 +1674,44 @@ jobs: # Run the darwin-tagged unit suite for the shell entrypoint — this is # the ONLY lane that executes it. The moon `compass-go:test` lane runs # untagged (`go test ./...`), which compiles the non-gtk4 stub and - # excludes main_test.go; the gtk4-e2e lane compiles the gtk4 build but - # `-run E2E`-filters, so it never executes TestDistDirForExecutable. - # That test defends the .app dist-resolution contract (the resolver - # returns Contents/Resources/dist under a Contents/MacOS executable, - # else dist beside it), which is exactly the behavioral change this - # lane ships — so its regression guard lives here or nowhere. + # excludes main_test.go, machine_test.go and embedded_test.go; the + # gtk4-e2e lane compiles the gtk4 build but `-run E2E`-filters, so it + # never reaches any of them. These tests defend contracts that ARE the + # darwin behaviour this lane ships, so their regression guards live + # here or nowhere: + # - DistDirForExecutable: the .app dist-resolution contract + # (Contents/Resources/dist under a Contents/MacOS executable, + # else dist beside it). + # - Machine*/EnsureMachine*: the podman-machine probe + ensure step, + # including that an unclassifiable CLI answer provisions nothing. + # - RealPreflightDeps*/ClassifyPreflight*: that the darwin machine + # adapter is actually WIRED and that an unmet machine check is + # fatal. This is the pair that catches the silent-skip regression + # (a nil adapter making the check vanish into an all-green + # preflight), so it is the last thing that should run nowhere. + # - BringUpTimeout*: that darwin keeps a window a cold + # `podman machine init` can fit inside. # `-run` alone exits 0 when it matches nothing (a rename → false - # green), so require the test's own PASS line — a rename or skip reds. + # green), so require each group's own PASS line — a rename or skip + # reds. The filter is explicit rather than the whole package because + # the package also holds GUI E2E tests that need a display. CGO_ENABLED=1 go -C go test -trimpath \ - -run 'TestDistDirForExecutable' -count=1 -v \ + -run 'TestDistDirForExecutable|TestMachineReady|TestEnsureMachineReady|TestMachineResourceFloorIsExplicit|TestRealPreflightDeps|TestClassifyPreflight|TestBringUpTimeout' \ + -count=1 -v \ ./cmd/compass-app/ | tee /tmp/darwin-unit.log - grep -q '^--- PASS: TestDistDirForExecutable' /tmp/darwin-unit.log || { - echo "::error::darwin: TestDistDirForExecutable did not run+pass (renamed or skipped?)" - exit 1 - } + for t in TestDistDirForExecutable \ + TestMachineReadyRunning \ + TestMachineReadyNoMachine \ + TestEnsureMachineReadyNoMachineProvisions \ + TestEnsureMachineReadyUnclassifiedDoesNotProvision \ + TestRealPreflightDepsWiresDarwinMachineAdapter \ + TestClassifyPreflightUnwiredDarwinMachineIsFatal \ + TestBringUpTimeoutBudgetsDarwinColdProvisioning; do + grep -q "^--- PASS: $t" /tmp/darwin-unit.log || { + echo "::error::darwin: $t did not run+pass (renamed or skipped?)" + exit 1 + } + done # The UI dist the .app stages into Contents/Resources/dist. moon run compass-ui:build diff --git a/go/cmd/compass-app/embedded.go b/go/cmd/compass-app/embedded.go index 8b8e8830d..3a7d50d82 100644 --- a/go/cmd/compass-app/embedded.go +++ b/go/cmd/compass-app/embedded.go @@ -373,21 +373,37 @@ func resolveImage(flagValue string) string { // the app-side DSN duplicate are gone (§A2 reconciliation 1): under DL-260 // postgres is a container the stack itself starts, so a pre-`up` reachability // probe has no signal on the cold-start path — `up`-Ready is the DB -// verification. On darwin the machine adapter is wired by T-6; a nil -// MachineReady here leaves that check absent until then (design §A5). +// verification. MachineReady comes from the per-OS machineReadyAdapter: on +// darwin it is the podman-machine ensure step (provision or start the Linux VM, +// then re-probe — design §A5), and on linux it is nil because there is no +// machine. The preflight core keys the check off GOOS and FAILS on darwin when +// the adapter is nil, so this wiring cannot regress into a silently-skipped +// check. func realPreflight(image string) func(ctx context.Context) error { - deps := preflight.Deps{ - GOOS: runtime.GOOS, - PodmanRootless: podmanRootless, - PodmanVersion: podmanVersionAtLeastFloor, - ImagePresent: imagePresent, - } + deps := realPreflightDeps(runtime.GOOS) params := preflight.Params{AgentImage: image} return func(ctx context.Context) error { return classifyPreflight(deps.Run(ctx, params)) } } +// realPreflightDeps assembles the Deps literal for the given host OS. It takes +// goos as an argument, rather than reading runtime.GOOS itself, so a test +// running on ANY host can assert what the darwin wiring carries — the machine +// check going missing on darwin is the exact regression this seam exists to +// catch, and it is unobservable from a linux test if the builder resolves its +// own OS. The one goos value feeds both the core's check selection and the +// machine adapter, so the two cannot disagree about which host this is. +func realPreflightDeps(goos string) preflight.Deps { + return preflight.Deps{ + GOOS: goos, + PodmanRootless: podmanRootless, + PodmanVersion: podmanVersionAtLeastFloor, + MachineReady: machineReadyAdapter(goos), + ImagePresent: imagePresent, + } +} + // classifyPreflight splits the preflight results by severity at the wiring // boundary and returns only the FATAL failures folded into one legible error // (nil when none are fatal). diff --git a/go/cmd/compass-app/embedded_test.go b/go/cmd/compass-app/embedded_test.go index 9eb033b09..2a1130822 100644 --- a/go/cmd/compass-app/embedded_test.go +++ b/go/cmd/compass-app/embedded_test.go @@ -489,6 +489,72 @@ func TestClassifyPreflightHostCapFatalEvenWithAdvisoryUnmet(t *testing.T) { } } +// TestRealPreflightDepsWiresDarwinMachineAdapter is the regression guard for the +// silent skip: realPreflightDeps must carry a non-nil MachineReady on darwin. It +// asserts the WIRING, not a probe result, so it fails on a linux CI host the +// moment the adapter is dropped from the Deps literal — the defect was +// invisible precisely because a missing adapter produced no failing check. +func TestRealPreflightDepsWiresDarwinMachineAdapter(t *testing.T) { + deps := realPreflightDeps("darwin") + if deps.MachineReady == nil { + t.Fatal("realPreflightDeps left MachineReady nil on darwin; the machine check would be a wiring failure") + } + if deps.GOOS != "darwin" { + t.Errorf("GOOS = %q, want the injected darwin", deps.GOOS) + } +} + +// TestRealPreflightDepsLeavesLinuxMachineUnwired: linux podman is native, so +// there is no machine adapter — the core keys the check off GOOS and omits it +// here. This pins that closing the darwin hole did not add a bogus linux check. +// It asserts only the WIRING: running deps here would shell the real podman +// probes, and the core package already owns the absent-on-linux assertion +// hermetically (preflight.TestRunMachineCheckAbsentOnLinux). +func TestRealPreflightDepsLeavesLinuxMachineUnwired(t *testing.T) { + deps := realPreflightDeps("linux") + if deps.MachineReady != nil { + t.Fatal("realPreflightDeps wired a machine adapter on linux; there is no machine to check") + } + if deps.GOOS != "linux" { + t.Errorf("GOOS = %q, want the injected linux", deps.GOOS) + } +} + +// TestClassifyPreflightMachineUnmetIsFatal verifies — rather than assumes — that +// a failing machine check reaches the FATAL fold. classifyPreflight special-cases +// only CheckImage as advisory, so the machine check falls to the default arm; +// this exercises that path end-to-end so the doc comment's "fatal on darwin" +// claim is enforced by a test rather than by reading the switch. +func TestClassifyPreflightMachineUnmetIsFatal(t *testing.T) { + machineErr := errors.New("no podman machine exists") + deps := classifyDeps() + deps.GOOS = "darwin" + deps.MachineReady = func(context.Context) error { return machineErr } + err := classify(t, deps) + if err == nil { + t.Fatal("machine unmet on darwin: classify err = nil, want fatal") + } + if !strings.Contains(err.Error(), machineErr.Error()) { + t.Errorf("fatal error %q does not carry the machine failure", err.Error()) + } +} + +// TestClassifyPreflightUnwiredDarwinMachineIsFatal: the wiring defect itself is +// fatal, not advisory — a darwin build whose machine adapter went missing +// refuses to launch instead of proceeding on an unverified host. +func TestClassifyPreflightUnwiredDarwinMachineIsFatal(t *testing.T) { + deps := classifyDeps() + deps.GOOS = "darwin" + deps.MachineReady = nil + err := classify(t, deps) + if err == nil { + t.Fatal("unwired machine adapter on darwin: classify err = nil, want fatal") + } + if !strings.Contains(err.Error(), "no podman machine adapter is wired") { + t.Errorf("fatal error %q does not name the wiring defect", err.Error()) + } +} + // TestRunStackUpDeadlineExceededNamesBringUpWindow: when the child fails because // the context deadline was exceeded, the error names the bring-up window (the // likely cause) rather than surfacing a bare deadline error. Driven with an diff --git a/go/cmd/compass-app/machine.go b/go/cmd/compass-app/machine.go new file mode 100644 index 000000000..26c26b754 --- /dev/null +++ b/go/cmd/compass-app/machine.go @@ -0,0 +1,423 @@ +//go:build (linux && gtk4) || darwin + +// The podman-machine probe and ensure step behind an injected seam. On macOS the +// podman CLI drives a Linux VM ("the machine") and a fresh Mac has no machine at +// all, so embedded mode must both DETECT the machine's state and PROVISION it — +// mirroring how `compass-stack up` ensures the agent image and the database +// rather than gating on them. The OS choice is a parameter, not a build tag +// (machineReadyAdapter takes the GOOS the preflight core is already keyed on), +// so the darwin wiring is reachable from a test on any host — a build-tagged +// darwin adapter would make the regression this closes untestable on every +// lane that actually runs. The classification, the error copy, and the ensure +// orchestration are inverted over machineDeps, so all four states (no machine +// / stopped / running / init fails) are unit-testable with no podman present. +// +// Every shape this file reads out of the podman CLI is an ASSUMPTION about +// external behavior — the design record marks the `machine inspect` socket path +// and the `machine ls --format json` no-machine-vs-stopped distinction as +// spike-verified, and the spike has not run. So the parsing is deliberately +// defensive and TOLERANT of shape drift (both the `Running` bool and the `State` +// string are accepted; a missing field degrades, never panics), and an +// unparseable answer is classified UNKNOWN, which is never ready. The failure +// copy always names the podman command the operator can run themselves. +package main + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net" + "os/exec" //nolint:depguard // podman machine seam: fixed-arg `podman machine ls|inspect|init|start` subprocesses + "strconv" + "strings" +) + +// podmanBin is the podman executable name, resolved on PATH. +const podmanBin = "podman" + +// The machine resource floor passed to `podman machine init`. `podman machine +// init`'s own defaults are modest (2 GiB of memory), and the embedded stack runs +// FOUR containers inside the VM — postgres, the collector, the server, and an +// agent container per session — so the default leaves the machine thrashing or +// OOM-killing the agent under a normal session. The floor below is sized for +// that set with headroom for a second concurrent agent; the disk figure covers +// the pulled images (agent + postgres + collector) plus the postgres data +// directory's growth over a long-lived install. +// +// UNVERIFIED: no macOS host has run the spike, so this floor is a reasoned +// choice, not a measured one, and the units are the podman CLI's documented ones +// (--memory in MiB, --disk-size in GiB). The spike is what turns it into a +// number recorded in the self-host doc. +const ( + machineMemoryFloorMiB = 8192 + machineDiskFloorGiB = 100 +) + +// machineStatus is the classified state of the podman machine. The zero value is +// machineUnknown so a failed classification can never read as ready. +type machineStatus int + +const ( + // machineUnknown: the podman CLI's answer could not be classified (it failed, + // or its output did not parse). Never treated as ready. + machineUnknown machineStatus = iota + // machineAbsent: no machine exists at all — the fresh-Mac state. Fixed by init. + machineAbsent + // machineStopped: a machine exists but is not running. Fixed by start. + machineStopped + // machineRunning: the machine reports itself running; its socket still has to + // be reachable before the machine counts as ready. + machineRunning +) + +// machineDeps is the seam the machine probe and ensure step are inverted over: +// one field per genuine external effect, exactly as preflight.Deps inverts the +// host checks it runs. The real adapters shell the podman CLI +// (realMachineDeps); tests supply deterministic stubs. +type machineDeps struct { + // list runs `podman machine ls --format json` and returns its stdout. Its + // presence-or-absence is the one thing the classification depends on: an + // empty list means no machine exists. What `inspect` does for a stopped + // machine is one of the unspiked assumptions this file's doc flags, so + // nothing is inferred from it failing. + list func(ctx context.Context) ([]byte, error) + // inspect runs `podman machine inspect` (no machine named, so the DEFAULT + // machine) and returns its stdout: the authoritative state plus the host-side + // API socket path the VM forwards. + inspect func(ctx context.Context) ([]byte, error) + // initMachine runs `podman machine init` with the resource floor. On a fresh + // host this DOWNLOADS a VM image and takes minutes. + initMachine func(ctx context.Context) error + // startMachine runs `podman machine start` on the default machine. + startMachine func(ctx context.Context) error + // dialSocket proves the machine's forwarded API socket is actually reachable + // from the host (a running machine whose socket does not answer is not ready). + dialSocket func(ctx context.Context, path string) error +} + +// machineInfo is one classification of the machine: its status, the machine name +// to name in operator copy, and the host-side API socket path when known. +type machineInfo struct { + status machineStatus + name string + socket string +} + +// machineListEntry is the subset of `podman machine ls --format json` this code +// reads. Running is a pointer and State is a string because the field set +// differs across podman versions and this parse must not depend on either being +// present: presence in the list is what establishes existence, and running-ness +// is read from whichever field the CLI supplied. +// +// A machine mid-start is deliberately NOT a distinct case. It classifies as +// stopped and gets a `machine start`, which is a no-op on a machine already +// coming up — one redundant command on a rare path, against a third state to +// carry through the whole ensure step. +type machineListEntry struct { + Name string `json:"Name"` + Running *bool `json:"Running"` + State string `json:"State"` + Default bool `json:"Default"` +} + +// running reports whether this entry says the machine is up, accepting either +// the boolean or the string spelling. +func (e machineListEntry) running() bool { + if e.Running != nil && *e.Running { + return true + } + return strings.EqualFold(strings.TrimSpace(e.State), "running") +} + +// machineInspectEntry is the subset of `podman machine inspect` this code reads: +// the state and the forwarded podman API socket path +// (.ConnectionInfo.PodmanSocket.Path). +type machineInspectEntry struct { + Name string `json:"Name"` + State string `json:"State"` + ConnectionInfo struct { + PodmanSocket struct { + Path string `json:"Path"` + } `json:"PodmanSocket"` + } `json:"ConnectionInfo"` +} + +// errMachineUnclassified is the sentinel for "the podman CLI's answer could not +// be turned into a state". Callers wrap it with the command to run; it exists so +// the ensure step can tell an unclassifiable answer (do nothing, surface it) +// from a state it knows how to fix. +var errMachineUnclassified = errors.New("the podman machine state could not be determined") + +// probeMachine classifies the machine from the podman CLI. It calls `machine ls` +// FIRST — that is the only call that separates no-machine from stopped-machine — +// and then `machine inspect` for the authoritative state and the socket path of +// the default machine. A CLI or parse failure yields machineUnknown with an +// error naming the command to run by hand; it never guesses ready. +func probeMachine(ctx context.Context, d machineDeps) (machineInfo, error) { + out, err := d.list(ctx) + if err != nil { + return machineInfo{}, fmt.Errorf("%w: `%s machine ls --format json` failed: %w", + errMachineUnclassified, podmanBin, err) + } + var listed []machineListEntry + if err := json.Unmarshal(out, &listed); err != nil { + return machineInfo{}, fmt.Errorf( + "%w: `%s machine ls --format json` output did not parse (%w); run it by hand to see what podman reports", + errMachineUnclassified, podmanBin, err) + } + if len(listed) == 0 { + return machineInfo{status: machineAbsent}, nil + } + + // A machine exists. Prefer the default entry for the name, since the podman + // CLI resolves its connection to the default machine. + entry := listed[0] + for _, e := range listed { + if e.Default { + entry = e + break + } + } + + insp, err := inspectMachine(ctx, d) + if err != nil { + return machineInfo{name: entry.Name}, err + } + name := insp.Name + if name == "" { + name = entry.Name + } + + // Running-ness: inspect's state is authoritative when it says running; + // otherwise fall back to the list entry, so a podman version that omits + // State from one of the two commands still classifies. + running := strings.EqualFold(strings.TrimSpace(insp.State), "running") || entry.running() + if !running { + return machineInfo{status: machineStopped, name: name}, nil + } + return machineInfo{ + status: machineRunning, + name: name, + socket: strings.TrimSpace(insp.ConnectionInfo.PodmanSocket.Path), + }, nil +} + +// inspectMachine runs the inspect seam and pulls out the single entry for the +// default machine. inspect returns a JSON ARRAY even for one machine; an empty +// array or a parse failure is unclassified, never ready. +func inspectMachine(ctx context.Context, d machineDeps) (machineInspectEntry, error) { + out, err := d.inspect(ctx) + if err != nil { + return machineInspectEntry{}, fmt.Errorf("%w: `%s machine inspect` failed: %w", + errMachineUnclassified, podmanBin, err) + } + var entries []machineInspectEntry + if err := json.Unmarshal(out, &entries); err != nil { + return machineInspectEntry{}, fmt.Errorf( + "%w: `%s machine inspect` output did not parse (%w); run it by hand to see what podman reports", + errMachineUnclassified, podmanBin, err) + } + if len(entries) == 0 { + return machineInspectEntry{}, fmt.Errorf( + "%w: `%s machine inspect` described no machine even though `%s machine ls` listed one", + errMachineUnclassified, podmanBin, podmanBin) + } + return entries[0], nil +} + +// machineReady is the probe half: nil when the machine is up AND its forwarded +// API socket answers, and otherwise an error whose copy distinguishes the three +// failing states the operator can act on — no machine, a stopped machine, and a +// running machine with an unreachable socket — each naming the command to run. +func machineReady(ctx context.Context, d machineDeps) error { + info, err := probeMachine(ctx, d) + if err != nil { + return err + } + switch info.status { + case machineAbsent: + return fmt.Errorf("no podman machine exists; create one with `%s machine init --memory %d --disk-size %d` "+ + "(the first run downloads a VM image and takes several minutes)", + podmanBin, machineMemoryFloorMiB, machineDiskFloorGiB) + case machineStopped: + return fmt.Errorf("the podman machine %q exists but is not running; start it with `%s machine start %s`", + info.name, podmanBin, info.name) + case machineRunning: + return machineSocketReachable(ctx, d, info) + case machineUnknown: + return machineStateUnreadable() + default: + return machineStateUnreadable() + } +} + +// machineStateUnreadable is the error for a machine state the podman CLI would +// not tell us. Shared by the probe and the ensure step so both report the same +// copy, and pointing at the command whose output could not be classified. +func machineStateUnreadable() error { + return fmt.Errorf("%w; run `%s machine ls --format json` to see what podman reports", + errMachineUnclassified, podmanBin) +} + +// machineSocketReachable checks the third failing state: the machine is running +// but the API socket the VM forwards to the host does not answer. An empty path +// counts as unreachable — a running machine that reports no socket is exactly +// the unparseable-response case, and treating it as ready is what would produce +// a green preflight followed by an undiagnosable failure. +func machineSocketReachable(ctx context.Context, d machineDeps, info machineInfo) error { + if info.socket == "" { + return fmt.Errorf("the podman machine %q is running but `%s machine inspect` reported no API socket path; "+ + "restart it with `%s machine stop %s && %s machine start %s`", + info.name, podmanBin, podmanBin, info.name, podmanBin, info.name) + } + if err := d.dialSocket(ctx, info.socket); err != nil { + return fmt.Errorf("the podman machine %q is running but its API socket %s is unreachable (%w); "+ + "restart it with `%s machine stop %s && %s machine start %s`", + info.name, info.socket, err, podmanBin, info.name, podmanBin, info.name) + } + return nil +} + +// ensureMachineReady is what the darwin MachineReady adapter wires: it makes the +// machine ready rather than merely reporting on it. On no-machine it inits then +// starts; on a stopped machine it starts; then it RE-PROBES, because the +// authority on readiness is the probe, never the exit status of init/start. A +// state it cannot fix (an unclassifiable CLI answer, or a running machine whose +// socket does not answer) is surfaced from the probe unchanged. +// +// The init download is minutes long and runs under the caller's context, which +// the embedded pipeline bounds with its bring-up window. On darwin that window +// is sized for a cold provision (bringUpTimeoutFor in main.go), so a healthy +// first run fits inside it. The copy on the failure path still names the init +// command, so an operator who does exhaust the window gets something to run by +// hand rather than a bare deadline error. +func ensureMachineReady(ctx context.Context, d machineDeps) error { + info, err := probeMachine(ctx, d) + if err != nil { + return err + } + switch info.status { + case machineAbsent: + if err := d.initMachine(ctx); err != nil { + return fmt.Errorf("provisioning a podman machine with `%s machine init --memory %d --disk-size %d` "+ + "failed (%w); run it by hand — the first run downloads a VM image and takes several minutes", + podmanBin, machineMemoryFloorMiB, machineDiskFloorGiB, err) + } + if err := d.startMachine(ctx); err != nil { + return fmt.Errorf("the podman machine was created but `%s machine start` failed (%w); "+ + "run it by hand to see what podman reports", podmanBin, err) + } + case machineStopped: + if err := d.startMachine(ctx); err != nil { + return fmt.Errorf("starting the podman machine %q with `%s machine start %s` failed (%w); "+ + "run it by hand to see what podman reports", info.name, podmanBin, info.name, err) + } + case machineRunning: + // Nothing to provision; a running machine only needs its socket checked, + // which the re-probe below does. + case machineUnknown: + // Not something init/start can fix, and re-probing would only ask the + // same unintelligible question again — reporting the second answer + // instead of the first, which is worse if the machine changed state + // between the two. Report what the probe already told us. + return machineStateUnreadable() + default: + return machineStateUnreadable() + } + return machineReady(ctx, d) +} + +// machineReadyAdapter returns the preflight.Deps.MachineReady adapter for the +// given host OS: on darwin the podman-machine ENSURE step (provision or start +// the Linux VM, then re-probe), and nil elsewhere — linux podman is native, so +// there is no machine and the preflight core omits the check. +// +// The OS is a PARAMETER rather than a build tag, matching preflight.Deps.GOOS: +// a build-tagged darwin-only adapter would be uncompilable from a linux test, so +// the very regression this closes — a darwin build reaching preflight with no +// machine adapter — could not be tested anywhere the CI actually runs. Keyed off +// GOOS instead, a linux host can assert the darwin wiring. +func machineReadyAdapter(goos string) func(ctx context.Context) error { + if goos != "darwin" { + return nil + } + deps := realMachineDeps() + return func(ctx context.Context) error { + return ensureMachineReady(ctx, deps) + } +} + +// realMachineDeps builds the machine seam over the real podman CLI. Each field +// is one fixed-argv subprocess; the argv carries no caller-supplied strings +// except the resource floor constants, so there is nothing to inject into it. +func realMachineDeps() machineDeps { + return machineDeps{ + list: func(ctx context.Context) ([]byte, error) { + return machineOutput(ctx, "ls", "--format", "json") + }, + inspect: func(ctx context.Context) ([]byte, error) { + return machineOutput(ctx, "inspect") + }, + initMachine: func(ctx context.Context) error { + return machineRun(ctx, "init", + "--memory", strconv.Itoa(machineMemoryFloorMiB), + "--disk-size", strconv.Itoa(machineDiskFloorGiB)) + }, + startMachine: func(ctx context.Context) error { + return machineRun(ctx, "start") + }, + dialSocket: dialUnixSocket, + } +} + +// machineOutput runs `podman machine ` and returns its STDOUT only — +// the JSON readers must not be fed podman's warnings — wrapping a failure with +// the captured stderr so the copy names why podman refused. +func machineOutput(ctx context.Context, args ...string) ([]byte, error) { + //nolint:gosec // G204: fixed argv. Every caller is a closure in + // realMachineDeps passing literal subcommands and the two resource-floor + // constants, so nothing caller-supplied reaches the argv. + cmd := exec.CommandContext(ctx, podmanBin, append([]string{"machine"}, args...)...) + out, err := cmd.Output() + if err != nil { + if exitErr, ok := errors.AsType[*exec.ExitError](err); ok { + if msg := strings.TrimSpace(string(exitErr.Stderr)); msg != "" { + return nil, fmt.Errorf("%w: %s", err, msg) + } + } + return nil, err + } + return out, nil +} + +// machineRun runs a `podman machine ` mutation and discards its output, +// wrapping a failure with the combined output so the copy names why podman +// refused (init and start report their progress and their reasons there). +func machineRun(ctx context.Context, args ...string) error { + //nolint:gosec // G204: fixed argv, same as machineOutput above. + cmd := exec.CommandContext(ctx, podmanBin, append([]string{"machine"}, args...)...) + out, err := cmd.CombinedOutput() + if err != nil { + if msg := strings.TrimSpace(string(out)); msg != "" { + return fmt.Errorf("%w: %s", err, msg) + } + return err + } + return nil +} + +// dialUnixSocket proves the forwarded podman API socket answers a connect. A +// stat would only prove the file exists, which a stale forward from a +// half-stopped machine also satisfies. +func dialUnixSocket(ctx context.Context, path string) error { + var dialer net.Dialer + conn, err := dialer.DialContext(ctx, "unix", path) + if err != nil { + return err + } + // The connect itself is the whole signal; nothing is written or read, and a + // close error on a socket we only probed is not actionable. + _ = conn.Close() + return nil +} diff --git a/go/cmd/compass-app/machine_test.go b/go/cmd/compass-app/machine_test.go new file mode 100644 index 000000000..5e56814a7 --- /dev/null +++ b/go/cmd/compass-app/machine_test.go @@ -0,0 +1,371 @@ +//go:build (linux && gtk4) || darwin + +package main + +import ( + "context" + "errors" + "strings" + "testing" +) + +// Fixtures shaped like the podman CLI output the machine code parses. Every one +// of these shapes is an ASSUMPTION about external podman behavior (the design +// record marks the inspect socket path and the `machine ls --format json` +// no-machine-vs-stopped distinction as spike-verified, and no macOS host has run +// the spike), so the parse is written to tolerate drift and the tests pin the +// tolerance, not one exact vendor shape. +const ( + listEmpty = `[]` + listRunning = `[{"Name":"podman-machine-default","Default":true,"Running":true}]` + listStopped = `[{"Name":"podman-machine-default","Default":true,"Running":false}]` + listStateOnly = `[{"Name":"podman-machine-default","Default":true,"State":"running"}]` + inspectRunning = `[{"Name":"podman-machine-default","State":"running",` + + `"ConnectionInfo":{"PodmanSocket":{"Path":"/tmp/podman.sock"}}}]` + inspectStopped = `[{"Name":"podman-machine-default","State":"stopped","ConnectionInfo":{}}]` + inspectNoSocket = `[{"Name":"podman-machine-default","State":"running","ConnectionInfo":{}}]` + inspectEmptyList = `[]` +) + +// stubMachineDeps returns a machineDeps whose every effect succeeds against a +// running machine with a reachable socket. Tests override one field at a time. +// The counters let a test assert the ensure step's ORDER and idempotence (that a +// running machine is never re-initialized). +type machineRecorder struct { + listCalls int + inspectCalls int + initCalls int + startCalls int + dialCalls int + dialed string + // listOut is returned by list; it is a field so the ensure step's re-probe + // can observe a DIFFERENT state than the first probe, which is how a real + // init/start becomes visible. + listOut string + inspectOut string +} + +func stubMachineDeps(rec *machineRecorder) machineDeps { + return machineDeps{ + list: func(context.Context) ([]byte, error) { + rec.listCalls++ + return []byte(rec.listOut), nil + }, + inspect: func(context.Context) ([]byte, error) { + rec.inspectCalls++ + return []byte(rec.inspectOut), nil + }, + initMachine: func(context.Context) error { + rec.initCalls++ + // A real init creates the machine, so the next probe sees it stopped. + rec.listOut = listStopped + rec.inspectOut = inspectStopped + return nil + }, + startMachine: func(context.Context) error { + rec.startCalls++ + rec.listOut = listRunning + rec.inspectOut = inspectRunning + return nil + }, + dialSocket: func(_ context.Context, path string) error { + rec.dialCalls++ + rec.dialed = path + return nil + }, + } +} + +// runningRecorder is the all-good starting state: a machine that exists and runs. +func runningRecorder() *machineRecorder { + return &machineRecorder{listOut: listRunning, inspectOut: inspectRunning} +} + +// TestMachineReadyRunning: the running state — the probe passes and it proves +// readiness by DIALING the forwarded socket inspect reported, not by trusting +// the state string alone. +func TestMachineReadyRunning(t *testing.T) { + rec := runningRecorder() + if err := machineReady(context.Background(), stubMachineDeps(rec)); err != nil { + t.Fatalf("machineReady on a running machine = %v, want nil", err) + } + if rec.dialCalls != 1 { + t.Errorf("dial calls = %d, want 1 (readiness must probe the socket)", rec.dialCalls) + } + if rec.dialed != "/tmp/podman.sock" { + t.Errorf("dialed %q, want the inspect ConnectionInfo.PodmanSocket.Path", rec.dialed) + } +} + +// TestMachineReadyStateStringOnly: a podman version that reports running-ness as +// a State string rather than a Running bool still classifies as running — the +// parse must not depend on either single spelling. +func TestMachineReadyStateStringOnly(t *testing.T) { + rec := &machineRecorder{listOut: listStateOnly, inspectOut: inspectRunning} + if err := machineReady(context.Background(), stubMachineDeps(rec)); err != nil { + t.Fatalf("machineReady with a State-only list entry = %v, want nil", err) + } +} + +// TestMachineReadyNoMachine: the fresh-Mac state. `machine ls` lists nothing, so +// the copy must say no machine exists and name the init command — and must NOT +// have consulted inspect (which fails identically for absent and stopped). +func TestMachineReadyNoMachine(t *testing.T) { + rec := &machineRecorder{listOut: listEmpty} + err := machineReady(context.Background(), stubMachineDeps(rec)) + if err == nil { + t.Fatal("machineReady with no machine = nil, want an error") + } + for _, tok := range []string{"no podman machine exists", "machine init", "--memory", "--disk-size"} { + if !strings.Contains(err.Error(), tok) { + t.Errorf("no-machine copy %q missing %q", err.Error(), tok) + } + } + if rec.inspectCalls != 0 { + t.Errorf("inspect calls = %d, want 0 (absence is established by ls alone)", rec.inspectCalls) + } +} + +// TestMachineReadyStopped: a machine exists but is not running — distinguishable +// from no-machine, and the copy names start (not init) plus the machine name. +func TestMachineReadyStopped(t *testing.T) { + rec := &machineRecorder{listOut: listStopped, inspectOut: inspectStopped} + err := machineReady(context.Background(), stubMachineDeps(rec)) + if err == nil { + t.Fatal("machineReady on a stopped machine = nil, want an error") + } + for _, tok := range []string{"is not running", "machine start", "podman-machine-default"} { + if !strings.Contains(err.Error(), tok) { + t.Errorf("stopped copy %q missing %q", err.Error(), tok) + } + } + if strings.Contains(err.Error(), "no podman machine exists") { + t.Errorf("stopped copy %q conflates a stopped machine with an absent one", err.Error()) + } + if rec.dialCalls != 0 { + t.Errorf("dial calls = %d, want 0 (a stopped machine has no socket to dial)", rec.dialCalls) + } +} + +// TestMachineReadyUnreachableSocket: the third distinguishable state — running, +// but the forwarded API socket does not answer. This is the state a state-string +// check alone would call ready. +func TestMachineReadyUnreachableSocket(t *testing.T) { + rec := runningRecorder() + deps := stubMachineDeps(rec) + deps.dialSocket = func(context.Context, string) error { + return errors.New("connect: connection refused") + } + err := machineReady(context.Background(), deps) + if err == nil { + t.Fatal("machineReady with an unreachable socket = nil, want an error") + } + for _, tok := range []string{"is running", "unreachable", "/tmp/podman.sock", "machine stop", "machine start"} { + if !strings.Contains(err.Error(), tok) { + t.Errorf("unreachable-socket copy %q missing %q", err.Error(), tok) + } + } +} + +// TestMachineReadyRunningWithoutSocketPath: a running machine whose inspect +// reports NO socket path is the unparseable-response case, and must never read +// as ready — that is precisely how a green preflight would precede an +// undiagnosable downstream failure. +func TestMachineReadyRunningWithoutSocketPath(t *testing.T) { + rec := &machineRecorder{listOut: listRunning, inspectOut: inspectNoSocket} + err := machineReady(context.Background(), stubMachineDeps(rec)) + if err == nil { + t.Fatal("machineReady with no reported socket path = nil, want an error") + } + if !strings.Contains(err.Error(), "no API socket path") { + t.Errorf("copy %q does not name the missing socket path", err.Error()) + } + if rec.dialCalls != 0 { + t.Errorf("dial calls = %d, want 0 (there is no path to dial)", rec.dialCalls) + } +} + +// TestMachineReadyUnparseableOutput: every way the podman CLI can answer +// unintelligibly — a failing command, non-JSON output, an inspect that describes +// no machine — classifies as UNKNOWN and errors. None of them may read as ready. +func TestMachineReadyUnparseableOutput(t *testing.T) { + cliErr := errors.New("podman: command not found") + cases := map[string]struct { + mutate func(*machineDeps) + want string + }{ + "ls fails": { + mutate: func(d *machineDeps) { + d.list = func(context.Context) ([]byte, error) { return nil, cliErr } + }, + want: "machine ls --format json` failed", + }, + "ls is not json": { + mutate: func(d *machineDeps) { + d.list = func(context.Context) ([]byte, error) { return []byte("Error: unknown flag"), nil } + }, + want: "did not parse", + }, + "inspect fails": { + mutate: func(d *machineDeps) { + d.inspect = func(context.Context) ([]byte, error) { return nil, cliErr } + }, + want: "machine inspect` failed", + }, + "inspect is not json": { + mutate: func(d *machineDeps) { + d.inspect = func(context.Context) ([]byte, error) { return []byte("not json"), nil } + }, + want: "did not parse", + }, + "inspect describes no machine": { + mutate: func(d *machineDeps) { + d.inspect = func(context.Context) ([]byte, error) { return []byte(inspectEmptyList), nil } + }, + want: "described no machine", + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + deps := stubMachineDeps(runningRecorder()) + tc.mutate(&deps) + err := machineReady(context.Background(), deps) + if err == nil { + t.Fatalf("%s: machineReady = nil, want an error (unparseable is never ready)", name) + } + if !errors.Is(err, errMachineUnclassified) { + t.Errorf("%s: err %v does not wrap errMachineUnclassified", name, err) + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("%s: copy %q missing %q", name, err.Error(), tc.want) + } + }) + } +} + +// TestEnsureMachineReadyNoMachineProvisions: the fresh-Mac path — init, then +// start, then RE-PROBE to nil. The re-probe is what establishes readiness; the +// exit status of init/start is not trusted on its own. +func TestEnsureMachineReadyNoMachineProvisions(t *testing.T) { + rec := &machineRecorder{listOut: listEmpty} + if err := ensureMachineReady(context.Background(), stubMachineDeps(rec)); err != nil { + t.Fatalf("ensureMachineReady from no machine = %v, want nil after provisioning", err) + } + if rec.initCalls != 1 { + t.Errorf("init calls = %d, want 1", rec.initCalls) + } + if rec.startCalls != 1 { + t.Errorf("start calls = %d, want 1 (a freshly-created machine is not running)", rec.startCalls) + } + if rec.listCalls < 2 { + t.Errorf("list calls = %d, want >= 2 (the ensure step must re-probe)", rec.listCalls) + } + if rec.dialCalls != 1 { + t.Errorf("dial calls = %d, want 1 (the re-probe proves the socket answers)", rec.dialCalls) + } +} + +// TestEnsureMachineReadyStoppedStarts: a stopped machine is STARTED, never +// re-initialized (init on an existing machine would fail, and would re-download). +func TestEnsureMachineReadyStoppedStarts(t *testing.T) { + rec := &machineRecorder{listOut: listStopped, inspectOut: inspectStopped} + if err := ensureMachineReady(context.Background(), stubMachineDeps(rec)); err != nil { + t.Fatalf("ensureMachineReady from stopped = %v, want nil after start", err) + } + if rec.initCalls != 0 { + t.Errorf("init calls = %d, want 0 (the machine already exists)", rec.initCalls) + } + if rec.startCalls != 1 { + t.Errorf("start calls = %d, want 1", rec.startCalls) + } +} + +// TestEnsureMachineReadyRunningIsNoOp: an already-ready machine provisions +// nothing — the ensure step is idempotent, so a normal launch pays only the probe. +func TestEnsureMachineReadyRunningIsNoOp(t *testing.T) { + rec := runningRecorder() + if err := ensureMachineReady(context.Background(), stubMachineDeps(rec)); err != nil { + t.Fatalf("ensureMachineReady on a running machine = %v, want nil", err) + } + if rec.initCalls != 0 || rec.startCalls != 0 { + t.Errorf("provisioned an already-running machine: init=%d start=%d", rec.initCalls, rec.startCalls) + } +} + +// TestEnsureMachineReadyInitFails: the init-fails state. The error names the +// init command WITH the resource floor so the operator can run it by hand, and +// the step does not press on to start a machine that was never created. +func TestEnsureMachineReadyInitFails(t *testing.T) { + rec := &machineRecorder{listOut: listEmpty} + deps := stubMachineDeps(rec) + deps.initMachine = func(context.Context) error { + rec.initCalls++ + return errors.New("no space left on device") + } + err := ensureMachineReady(context.Background(), deps) + if err == nil { + t.Fatal("ensureMachineReady with a failing init = nil, want an error") + } + for _, tok := range []string{"machine init", "--memory", "--disk-size", "no space left on device"} { + if !strings.Contains(err.Error(), tok) { + t.Errorf("init-failure copy %q missing %q", err.Error(), tok) + } + } + if rec.startCalls != 0 { + t.Errorf("start calls = %d, want 0 (nothing was created to start)", rec.startCalls) + } +} + +// TestEnsureMachineReadyStartFails: a start that refuses surfaces podman's +// reason and names the command, rather than reporting a bare unready machine. +func TestEnsureMachineReadyStartFails(t *testing.T) { + rec := &machineRecorder{listOut: listStopped, inspectOut: inspectStopped} + deps := stubMachineDeps(rec) + deps.startMachine = func(context.Context) error { + rec.startCalls++ + return errors.New("vfkit: not permitted") + } + err := ensureMachineReady(context.Background(), deps) + if err == nil { + t.Fatal("ensureMachineReady with a failing start = nil, want an error") + } + for _, tok := range []string{"machine start", "podman-machine-default", "vfkit: not permitted"} { + if !strings.Contains(err.Error(), tok) { + t.Errorf("start-failure copy %q missing %q", err.Error(), tok) + } + } +} + +// TestEnsureMachineReadyUnclassifiedDoesNotProvision: an unintelligible CLI +// answer is NOT something init/start can fix, so the ensure step must surface it +// rather than blindly initializing over a machine whose state it cannot read. +func TestEnsureMachineReadyUnclassifiedDoesNotProvision(t *testing.T) { + rec := runningRecorder() + deps := stubMachineDeps(rec) + deps.list = func(context.Context) ([]byte, error) { + rec.listCalls++ + return []byte("Error: unknown flag: --format"), nil + } + err := ensureMachineReady(context.Background(), deps) + if !errors.Is(err, errMachineUnclassified) { + t.Fatalf("ensureMachineReady on an unparseable ls = %v, want errMachineUnclassified", err) + } + if rec.initCalls != 0 || rec.startCalls != 0 { + t.Errorf("provisioned against an unreadable state: init=%d start=%d", rec.initCalls, rec.startCalls) + } +} + +// TestMachineResourceFloorIsExplicit: the floor must stay ABOVE `podman machine +// init`'s own 2 GiB memory default — the whole reason the flags are passed is +// that the default starves the four containers the embedded stack runs. A future +// edit that drops the floor back to the default silently reintroduces that. +func TestMachineResourceFloorIsExplicit(t *testing.T) { + const podmanDefaultMemoryMiB = 2048 + if machineMemoryFloorMiB <= podmanDefaultMemoryMiB { + t.Errorf("memory floor %d MiB does not exceed podman's own default %d MiB", + machineMemoryFloorMiB, podmanDefaultMemoryMiB) + } + if machineDiskFloorGiB <= 0 { + t.Errorf("disk floor %d GiB is not a usable size", machineDiskFloorGiB) + } +} diff --git a/go/cmd/compass-app/main.go b/go/cmd/compass-app/main.go index 75ba3daf7..8f0b8fa97 100644 --- a/go/cmd/compass-app/main.go +++ b/go/cmd/compass-app/main.go @@ -29,6 +29,7 @@ import ( "log/slog" "os" "path/filepath" + "runtime" "time" "github.com/RigelBuild/compass/go/internal/appconfig" @@ -42,9 +43,33 @@ import ( // context-bound. It is generous because a cold first run pulls THREE images — // the agent image from GHCR plus the stock postgres and collector images // (DL-260) — before the stack reaches Ready, so the window covers three -// sequential registry pulls, not one. (darwin machine-init time is A5/T-6's -// concern and not folded in here.) -const bringUpTimeout = 180 * time.Second +// sequential registry pulls, not one. +// +// On darwin the window is wider still. The machine ensure step runs inside it, +// and a cold `podman machine init` downloads a VM image before any of the +// above starts — minutes on its own, on a link whose speed we do not control. +// A budget that cannot fit the work it wraps is not a backstop; it is a +// deadline the first launch on a fresh Mac loses every time, and the error it +// produces names the timeout rather than the download. So darwin gets a window +// sized for cold provisioning plus the same three pulls. Both remain backstops +// against a wedge, not performance targets. +// +// The bring-up runs entirely BEFORE the window opens, so on darwin a genuinely +// wedged provision is now a silent wait of this length with no UI at all. The +// provisioning state that would make a long-but-healthy first run legible is +// not built yet; until it is, this number buys a working first launch at the +// cost of a worse failure mode for a hung one. +var bringUpTimeout = bringUpTimeoutFor(runtime.GOOS) + +// bringUpTimeoutFor returns the bring-up budget for the given host OS. It takes +// the OS as a parameter rather than reading runtime.GOOS so the per-OS choice +// is unit-testable from any host. +func bringUpTimeoutFor(goos string) time.Duration { + if goos == "darwin" { + return 15 * time.Minute + } + return 180 * time.Second +} func main() { if err := run(); err != nil { diff --git a/go/cmd/compass-app/main_test.go b/go/cmd/compass-app/main_test.go index cbad08cd8..9f9db6e6e 100644 --- a/go/cmd/compass-app/main_test.go +++ b/go/cmd/compass-app/main_test.go @@ -5,6 +5,7 @@ package main import ( "path/filepath" "testing" + "time" ) // TestDistDirForExecutable pins the packaging-layout dist resolution: a macOS @@ -50,3 +51,30 @@ func TestDistDirForExecutable(t *testing.T) { } }) } + +// TestBringUpTimeoutBudgetsDarwinColdProvisioning pins that darwin gets a +// materially wider bring-up window than linux. The machine ensure step runs +// inside this budget, and a cold `podman machine init` downloads a VM image +// before the stack pulls a single container image, so a linux-sized window +// would deadline every first launch on a fresh Mac and report the timeout +// instead of the download. The assertion is a floor and a relation, not the +// literal figures: re-tuning either budget is fine, collapsing the darwin one +// back onto the linux one is the regression. +func TestBringUpTimeoutBudgetsDarwinColdProvisioning(t *testing.T) { + linux := bringUpTimeoutFor("linux") + darwin := bringUpTimeoutFor("darwin") + + if darwin <= linux { + t.Errorf("bringUpTimeoutFor(darwin) = %v, not greater than linux %v; a cold "+ + "podman machine init cannot fit a linux-sized window", darwin, linux) + } + // A cold VM-image download plus three registry pulls does not fit in five + // minutes on an ordinary connection. + if darwin < 10*time.Minute { + t.Errorf("bringUpTimeoutFor(darwin) = %v, too tight for a cold machine init "+ + "plus three image pulls", darwin) + } + if linux <= 0 { + t.Errorf("bringUpTimeoutFor(linux) = %v, want a positive backstop", linux) + } +} diff --git a/go/internal/preflight/preflight.go b/go/internal/preflight/preflight.go index aec124e14..0b0105ef5 100644 --- a/go/internal/preflight/preflight.go +++ b/go/internal/preflight/preflight.go @@ -27,10 +27,15 @@ type Deps struct { // front door instead (design §A3 delta 4). PodmanVersion func(ctx context.Context) error // MachineReady probes that the darwin podman machine (the Linux VM podman - // runs inside on macOS) is up. Consulted ONLY on darwin; nil on linux (there - // is no machine to check). A nil error means ready; a non-nil error explains - // why not. The darwin adapter that supplies it lands in T-6 (design §A5); a - // nil MachineReady on darwin leaves the check absent until then. + // runs inside on macOS) is up, provisioning it if needed. Consulted ONLY on + // darwin; nil on linux (there is no machine to check). A nil error means + // ready; a non-nil error explains why not. + // + // On darwin it is REQUIRED: a nil MachineReady there is a wiring defect, and + // Run reports it as a FAILED machine check rather than omitting the check. + // Omitting it is the worse outcome — a Mac with no machine would pass + // preflight all-green and then fail somewhere downstream with nothing + // pointing at the cause. MachineReady func(ctx context.Context) error // ImagePresent probes that the given agent image ref is present in the local // container store. A nil error means present; a non-nil error means it is not @@ -110,13 +115,21 @@ func (d Deps) Run(ctx context.Context, p Params) Results { } results = append(results, pvRes) - // (4) Darwin podman machine ready. macOS runs podman inside a Linux VM; the - // check is consulted ONLY on darwin, and only when an adapter is wired (the - // darwin adapter lands in T-6). On linux there is no machine, so the check - // is absent. - if d.GOOS == "darwin" && d.MachineReady != nil { + // (4) Darwin podman machine ready. macOS runs podman inside a Linux VM. On + // linux there is no machine, so the check is correctly absent. On darwin the + // check ALWAYS appears: a missing adapter is reported as a failure, never + // skipped, so a wiring regression cannot turn a broken host into a green + // preflight. It is reported rather than panicked because the caller's + // failure path already surfaces legible copy, and a panic in a GUI binary + // would replace that copy with a stack trace. + if d.GOOS == "darwin" { machineRes := Result{Name: checkMachine, OK: true} - if err := d.MachineReady(ctx); err != nil { + if d.MachineReady == nil { + machineRes.OK = false + machineRes.Detail = "no podman machine adapter is wired on darwin; " + + "embedded mode cannot verify the Linux VM podman runs inside " + + "(this is a build/wiring defect, not a host condition)" + } else if err := d.MachineReady(ctx); err != nil { machineRes.OK = false machineRes.Detail = fmt.Sprintf("the podman machine is not ready: %v", err) } diff --git a/go/internal/preflight/preflight_test.go b/go/internal/preflight/preflight_test.go index 4c15aa1b9..d162d631a 100644 --- a/go/internal/preflight/preflight_test.go +++ b/go/internal/preflight/preflight_test.go @@ -120,20 +120,44 @@ func TestRunMachineNotReadyOnDarwin(t *testing.T) { assertErrContains(t, rs.Err(), "machine stopped") } -// TestRunMachineAbsentOnDarwinWithoutAdapter: on darwin with no MachineReady -// adapter wired (the pre-T-6 state), the machine check is absent rather than a -// spurious failure — the seam is wired, the adapter lands in T-6. -func TestRunMachineAbsentOnDarwinWithoutAdapter(t *testing.T) { +// TestRunMachineCheckFailsOnDarwinWithoutAdapter: a nil MachineReady on darwin +// is a wiring defect, and the check FAILS rather than vanishing. Omitting it +// would hand a Mac with no podman machine an all-green preflight followed by an +// undiagnosable downstream failure — the silent skip this behavior removes. +func TestRunMachineCheckFailsOnDarwinWithoutAdapter(t *testing.T) { ctx := context.Background() rs := okDeps("darwin").Run(ctx, testParams) - for _, r := range rs { - if r.Name == checkMachine { - t.Fatalf("machine check present on darwin without an adapter: %v", rs) - } + got := resultByName(t, rs, checkMachine) + if got.OK { + t.Fatal("machine check passed on darwin with no adapter wired; it must fail, never be skipped") } - if err := rs.Err(); err != nil { - t.Errorf("want nil error on darwin with no machine adapter, got %v", err) + if !strings.Contains(got.Detail, "no podman machine adapter is wired") { + t.Errorf("machine detail %q does not name the missing adapter", got.Detail) + } + assertErrContains(t, rs.Err(), "no podman machine adapter is wired") +} + +// TestRunMachineCheckAlwaysPresentOnDarwin: the machine check is present in the +// results on darwin for EVERY adapter state — ready, failing, or unwired. The +// regression this pins is the check being absent from a darwin run, which reads +// as a pass to any caller that classifies by result. +func TestRunMachineCheckAlwaysPresentOnDarwin(t *testing.T) { + ctx := context.Background() + adapters := map[string]func(context.Context) error{ + "ready": func(context.Context) error { return nil }, + "failing": func(context.Context) error { return errors.New("machine down") }, + "unwired": nil, + } + for name, adapter := range adapters { + t.Run(name, func(t *testing.T) { + d := okDeps("darwin") + d.MachineReady = adapter + rs := d.Run(ctx, testParams) + // resultByName t.Fatalf's when the check is missing, which IS the + // assertion: an absent machine check fails this test. + resultByName(t, rs, checkMachine) + }) } } From 9a2137b2cf1605f84742cdd2df30daec5c2dd6d3 Mon Sep 17 00:00:00 2001 From: mintaka Date: Wed, 9 Sep 2026 16:28:03 -0400 Subject: [PATCH 3/3] feat(ui): emit the PostHog session id on outbound requests (RIG-2874) --- apps/ui/src/analytics/analytics.test.ts | 61 ++++ apps/ui/src/analytics/analytics.ts | 16 +- apps/ui/src/index.tsx | 62 ++-- apps/ui/src/live/client.test.ts | 21 ++ apps/ui/src/live/client.ts | 12 +- packages/compass-client/src/index.test.ts | 357 +++++++++++++++++++++- packages/compass-client/src/index.ts | 135 +++++++- 7 files changed, 616 insertions(+), 48 deletions(-) diff --git a/apps/ui/src/analytics/analytics.test.ts b/apps/ui/src/analytics/analytics.test.ts index e2bb13fd5..24da649e6 100644 --- a/apps/ui/src/analytics/analytics.test.ts +++ b/apps/ui/src/analytics/analytics.test.ts @@ -14,6 +14,7 @@ interface FakePostHog { capture: (...args: unknown[]) => void; identify: (...args: unknown[]) => void; reset: (...args: unknown[]) => void; + get_session_id: (...args: unknown[]) => string; } function makeFake(): FakePostHog { @@ -29,6 +30,10 @@ function makeFake(): FakePostHog { capture: record("capture"), identify: record("identify"), reset: record("reset"), + get_session_id: () => { + calls.push({ method: "get_session_id", args: [] }); + return ""; + }, }; } @@ -46,6 +51,38 @@ describe("createAnalytics", () => { expect(fake.calls).toHaveLength(0); }); + test("disabled sessionId returns undefined with ZERO posthog calls", () => { + const fake = makeFake(); + const analytics = createAnalytics(undefined, { + posthog: fake as unknown as PostHog, + }); + + expect(analytics.sessionId()).toBeUndefined(); + expect(fake.calls).toHaveLength(0); + }); + + test("enabled sessionId delegates to posthog", () => { + const fake = makeFake(); + fake.get_session_id = () => "session-123"; + const analytics = createAnalytics( + { key: "phc_abc", host: "https://us.i.posthog.com" }, + { posthog: fake as unknown as PostHog }, + ); + + expect(analytics.sessionId()).toBe("session-123"); + }); + + test("enabled sessionId maps an empty posthog id to undefined", () => { + const fake = makeFake(); + fake.get_session_id = () => ""; + const analytics = createAnalytics( + { key: "phc_abc", host: "https://us.i.posthog.com" }, + { posthog: fake as unknown as PostHog }, + ); + + expect(analytics.sessionId()).toBeUndefined(); + }); + test("disabled with NO deps (the production shape) is a callable no-op", () => { // index.tsx calls createAnalytics(analyticsConfigFromEnv()) with no deps, // so the disabled production path is createAnalytics(undefined) — the real @@ -109,6 +146,30 @@ describe("createAnalytics", () => { expect(identifies).toHaveLength(1); expect(identifies[0]?.args).toEqual(["acct-1"]); }); + + // The session id ROTATES under the app (posthog mints a new one on idle and + // at max length), so the value must be re-read per call and never memoized. + // The interceptor side is pinned for this too, but a memo added HERE would + // defeat that: the interceptor would faithfully re-read a stale cache. + test("enabled sessionId re-reads posthog on every call, never memoizing", () => { + const fake = makeFake(); + const ids = ["sess-1", "sess-2"]; + let call = 0; + fake.get_session_id = () => { + fake.calls.push({ method: "get_session_id", args: [] }); + return ids[call++] ?? ""; + }; + const analytics = createAnalytics( + { key: "phc_abc", host: "https://us.i.posthog.com" }, + { posthog: fake as unknown as PostHog }, + ); + + expect(analytics.sessionId()).toBe("sess-1"); + expect(analytics.sessionId()).toBe("sess-2"); + expect( + fake.calls.filter((c) => c.method === "get_session_id"), + ).toHaveLength(2); + }); }); describe("$ai_trace_id stamping", () => { diff --git a/apps/ui/src/analytics/analytics.ts b/apps/ui/src/analytics/analytics.ts index 941a9f66c..379e84643 100644 --- a/apps/ui/src/analytics/analytics.ts +++ b/apps/ui/src/analytics/analytics.ts @@ -27,6 +27,8 @@ export interface Analytics { capture(event: string, props?: Record): void; /** Associate subsequent events with a stable distinct id (the caller). */ identify(distinctId: string): void; + /** Return the current PostHog session id, when one exists. */ + sessionId(): string | undefined; /** Tear down the identified session (logout / app teardown). */ shutdown(): void; } @@ -36,6 +38,9 @@ export interface Analytics { class NoopAnalytics implements Analytics { capture(): void {} identify(): void {} + sessionId(): string | undefined { + return undefined; + } shutdown(): void {} } @@ -45,8 +50,10 @@ class NoopAnalytics implements Analytics { class PostHogAnalytics implements Analytics { private readonly client: PostHog; /** The trace-id source, read at CAPTURE time rather than construction time: - * the transport that records trace ids is built before this client exists, - * so a value read once at construction would always be undefined. + * boot builds analytics BEFORE the transport, so this getter closes over a + * `clients` binding that is not yet initialized — reading it at construction + * would throw a ReferenceError, while reading it at capture time is long + * after boot bound it. * * A getter, not the sink object, on purpose — analytics reads one string and * has no business depending on compass-client's transport types, so the @@ -106,6 +113,11 @@ class PostHogAnalytics implements Analytics { this.client.identify(distinctId); } + sessionId(): string | undefined { + const sessionId = this.client.get_session_id(); + return sessionId === "" ? undefined : sessionId; + } + shutdown(): void { // PostHog's de-identify: reset the distinct id and start a fresh // anonymous session. The browser SDK batch-sends on its own; there is no diff --git a/apps/ui/src/index.tsx b/apps/ui/src/index.tsx index 6574779f5..65bfab74b 100644 --- a/apps/ui/src/index.tsx +++ b/apps/ui/src/index.tsx @@ -88,39 +88,53 @@ async function main( root: HTMLElement, connection: ResolvedConnection, ): Promise { - const clients = createLiveClients(connection); - - const callerId = await bootCaller(root, () => resolveCaller(clients.compass)); - // Undefined is bootCaller's stop signal — it already painted the WhoAmI - // failure screen, so the app must not come up (no caller to scope it). - if (!callerId) { - return; - } - // Product analytics, OFF by default: analyticsConfigFromEnv returns undefined // unless a PostHog project key is configured, and createAnalytics then hands // back a no-op that never touches posthog — an unconfigured deployment emits - // zero analytics. Identify the caller we just learned via WhoAmI so events - // attach to a stable distinct id. + // zero analytics. // - // The inbound half of correlation is wired here: `clients.traceId` is the slot - // the transport records each reply's server trace id into, and analytics reads - // it at capture time. Reading through a getter is what makes the ordering work - // — the clients exist before this line, but the first trace id only lands once - // a call has returned. + // Built FIRST, before the clients, because correlation now runs in both + // directions and the outbound half needs a real analytics object to read + // from. Both directions are lazy getters, and they point opposite ways: // - // Best-effort by construction, on two counts. The slot holds the LAST reply's - // trace id, so an event fired before any call has returned carries nothing, - // and one fired between calls carries the previous call's trace rather than - // its own. And the server sets `traceresponse` only on UNARY replies, and - // only when an OTel provider is installed — an unconfigured deployment - // (empty exporter endpoint ⇒ no span ⇒ no header) stamps nothing at all. + // inbound `clients.traceId` → analytics: the transport records each + // reply's server trace id into that slot, and analytics reads it + // at capture time. `clients` is a forward reference from inside + // this getter, which is safe because the getter only runs once + // an event is captured — long after the next statement binds it. + // outbound `analytics.sessionId()` → the transport: every request asks + // for the current PostHog session id and sends it as + // X-POSTHOG-SESSION-ID, so backend spans carry the same session + // the frontend recorded. // - // The OUTBOUND half (sending the PostHog session id to the server so its - // spans carry it) is deliberately not wired here. + // The inbound half is best-effort by construction, on two counts. The slot + // holds the LAST reply's trace id, so an event fired before any call has + // returned carries nothing, and one fired between calls carries the previous + // call's trace rather than its own. And the server sets `traceresponse` only + // on UNARY replies, and only when an OTel provider is installed — an + // unconfigured deployment (empty exporter endpoint ⇒ no span ⇒ no header) + // stamps nothing at all. + // + // The outbound half is best-effort too: the getter returns undefined until a + // PostHog session exists, and the interceptor then sends no header and + // self-heals on the next request. Only the TLS network door reads the header. const analytics = createAnalytics(analyticsConfigFromEnv(), { traceId: () => clients.traceId.current, }); + + const clients = createLiveClients(connection, { + sessionId: () => analytics.sessionId(), + }); + + const callerId = await bootCaller(root, () => resolveCaller(clients.compass)); + // Undefined is bootCaller's stop signal — it already painted the WhoAmI + // failure screen, so the app must not come up (no caller to scope it). + if (!callerId) { + return; + } + + // Identify the caller we just learned via WhoAmI so events attach to a stable + // distinct id. This stays AFTER bootCaller: the id is its output. analytics.identify(callerId); // One app-lifetime QueryClient — the server-state cache the query layer keys diff --git a/apps/ui/src/live/client.test.ts b/apps/ui/src/live/client.test.ts index 3c135f261..655ae8707 100644 --- a/apps/ui/src/live/client.test.ts +++ b/apps/ui/src/live/client.test.ts @@ -43,6 +43,27 @@ describe("createLiveClients (query record T1)", () => { expect(compassTransport).toBe(clients.transport); expect(commsTransport).toBe(compassTransport); }); + + test("passes the session id getter through to the transport factory", () => { + const transportSpy = spyOn(compassClient, "createCompassWebTransport"); + spies.push(transportSpy); + const sessionId = () => "session-id"; + + createLiveClients(conn, { sessionId }); + + const opts = transportSpy.mock.calls[0]?.[2]; + expect(opts?.sessionId).toBe(sessionId); + }); + + test("omits the session id option when deps are omitted", () => { + const transportSpy = spyOn(compassClient, "createCompassWebTransport"); + spies.push(transportSpy); + + createLiveClients(conn); + + const opts = transportSpy.mock.calls[0]?.[2]; + expect(opts).not.toHaveProperty("sessionId"); + }); }); describe("resolveCaller (WhoAmI boot probe)", () => { diff --git a/apps/ui/src/live/client.ts b/apps/ui/src/live/client.ts index bdbd651b6..5d8f5693a 100644 --- a/apps/ui/src/live/client.ts +++ b/apps/ui/src/live/client.ts @@ -41,9 +41,9 @@ export interface LiveClients { * * It rides on LiveClients because this is the one place that owns transport * construction — the sink is WRITTEN by the transport layer and READ above - * it (analytics stamps it on captured events), and boot builds the clients - * before analytics exists, so a shared mutable slot handed out here is what - * connects a writer and a reader that can never meet at construction. */ + * it (analytics stamps it on captured events). A shared mutable slot is what + * connects them because the trace id arrives on a REPLY: the writer has no + * value to hand over at construction time, whatever order boot runs in. */ readonly traceId: TraceIdSink; } @@ -54,11 +54,15 @@ export interface LiveClients { * cache-coherent with the clients' calls. `conn.fetchImpl` threads the resolved * transport fetch through: undefined (browser dev) uses the platform fetch; a * shell-provided fetch tunnels over IPC — the seam is invisible above here. */ -export function createLiveClients(conn: ResolvedConnection): LiveClients { +export function createLiveClients( + conn: ResolvedConnection, + deps?: { sessionId?: () => string | undefined }, +): LiveClients { const traceId: TraceIdSink = { current: undefined }; const transport = createCompassWebTransport(conn.baseUrl, conn.token, { fetch: conn.fetchImpl, traceSink: traceId, + ...(deps?.sessionId === undefined ? {} : { sessionId: deps.sessionId }), }); return { comms: createCommsClient(transport), diff --git a/packages/compass-client/src/index.test.ts b/packages/compass-client/src/index.test.ts index aa43d7bd1..37d3e77ae 100644 --- a/packages/compass-client/src/index.test.ts +++ b/packages/compass-client/src/index.test.ts @@ -17,7 +17,9 @@ import { createRouterTransport, GetServerInfoResponseSchema, parseTraceResponse, + posthogSessionHeader, SubscribeCommsResponseSchema, + sessionIdInterceptor, type TraceIdSink, type Transport, traceResponseInterceptor, @@ -78,7 +80,11 @@ type FetchLike = ( // transport has already invoked fetch and the capture is populated. async function captureRequest( run: (fetch: FetchLike) => Promise, -): Promise<{ url: string; authorization: string | null }> { +): Promise<{ + url: string; + authorization: string | null; + sessionId: string | null; +}> { let url = ""; let headers = new Headers(); const fetch: FetchLike = async (input, init) => { @@ -87,7 +93,14 @@ async function captureRequest( throw new Error("captureRequest: short-circuit before response"); }; await expect(run(fetch)).rejects.toThrow(); - return { url, authorization: headers.get("authorization") }; + return { + url, + authorization: headers.get("authorization"), + // Reported so the session-id cases can assert PRESENCE with a value and + // ABSENCE as `null` — an empty-string header would read back as "" and is + // a distinct (and forbidden) outcome from absent. + sessionId: headers.get(posthogSessionHeader), + }; } describe("bearerAuthInterceptor", () => { @@ -590,6 +603,278 @@ describe("traceResponseInterceptor round-trips through a real transport", () => }); }); +// A counting `next` returning a Symbol sentinel, plus a REAL `Headers` request +// — the direct-interceptor seam, following the bearerAuthInterceptor precedent +// above. The real `Headers` is what makes a `Headers.set` TypeError reachable, +// and the sentinel is what proves nothing threw. +// +// This seam exists because `captureRequest` CANNOT witness a would-throw case: +// its capturing fetch always throws, so it must gate on +// `rejects.toThrow()` — which a broken interceptor's own TypeError satisfies — +// and its header readback is a pre-initialized empty `Headers`, so absence also +// passes. Both assertions go green on exactly the defect. Never route a +// would-throw value onto the capture seam. +function directSeam() { + let calls = 0; + const sentinel = Symbol("next-response"); + const next = (_req: unknown) => { + calls++; + return Promise.resolve(sentinel); + }; + const req = { header: new Headers() }; + return { + req, + sentinel, + next, + get calls() { + return calls; + }, + }; +} + +describe("sessionIdInterceptor stamps only a sendable session id", () => { + const validId = "0199a1b2-3c4d-7e8f-9012-3456789abcde"; + + // The header name is a WIRE CONTRACT with the server's J1 interceptor + // (go/internal/otel/interceptor.go, PostHogSessionHeader) and is CORS-allowed + // by exactly that string in the network door. Every other case reads the + // header through the exported const, so all of them would stay green under a + // rename; only this pins the literal both ends must agree on. + test("the header name is exactly X-POSTHOG-SESSION-ID", () => { + expect(posthogSessionHeader).toBe("X-POSTHOG-SESSION-ID"); + }); + + test("a valid id is carried on the request as X-POSTHOG-SESSION-ID", async () => { + const { url, sessionId } = await captureRequest((fetch) => + createCompassClient( + createCompassWebTransport("http://compass.localhost", undefined, { + fetch: fetch as typeof globalThis.fetch, + sessionId: () => validId, + }), + ).getServerInfo({}), + ); + + expect(url).toBe( + "http://compass.localhost/compass.v1.CompassService/GetServerInfo", + ); + expect(sessionId).toBe(validId); + }); + + // Analytics off (NoopAnalytics.sessionId() ⇒ undefined) must send NO header + // at all, not an empty one: an empty header spends wire bytes asserting a + // correlation that does not exist, and the server trim-drops it anyway. + test("getter returns undefined ⇒ the header is absent, not empty", async () => { + const { sessionId } = await captureRequest((fetch) => + createCompassClient( + createCompassWebTransport("http://compass.localhost", undefined, { + fetch: fetch as typeof globalThis.fetch, + sessionId: () => undefined, + }), + ).getServerInfo({}), + ); + + expect(sessionId).toBeNull(); + }); + + // posthog-js's get_session_id() legitimately returns "" before it is fully + // initialized; the guard's `+` quantifier rejects it. + test('getter returns "" ⇒ the header is absent', async () => { + const { sessionId } = await captureRequest((fetch) => + createCompassClient( + createCompassWebTransport("http://compass.localhost", undefined, { + fetch: fetch as typeof globalThis.fetch, + sessionId: () => "", + }), + ).getServerInfo({}), + ); + + expect(sessionId).toBeNull(); + }); + + test("201 ASCII chars ⇒ over the cap, the header is absent", async () => { + const { sessionId } = await captureRequest((fetch) => + createCompassClient( + createCompassWebTransport("http://compass.localhost", undefined, { + fetch: fetch as typeof globalThis.fetch, + sessionId: () => "a".repeat(201), + }), + ).getServerInfo({}), + ); + + expect(sessionId).toBeNull(); + }); + + // The boundary the `<=` in `id.length <= MAX_SESSION_ID_LEN` owns: a `<` + // typo reddens HERE and nowhere else, because the 201 case stays green under + // both operators. 200 is legal on the server too — its check is + // `len(id) > maxSessionIDLen` — so refusing it would be needlessly stricter + // than the wire contract. + test("exactly 200 ASCII chars ⇒ at the cap, the header is PRESENT", async () => { + const atCap = "a".repeat(200); + const { sessionId } = await captureRequest((fetch) => + createCompassClient( + createCompassWebTransport("http://compass.localhost", undefined, { + fetch: fetch as typeof globalThis.fetch, + sessionId: () => atCap, + }), + ).getServerInfo({}), + ); + + expect(sessionId).toBe(atCap); + }); + + // A Latin-1 value does NOT throw — `Headers.set` accepts U+0080–U+00FF — so + // the capture seam is the right one here and the readback IS the whole + // assertion. What the ASCII guard buys: without it a browser emits this as + // a single raw high byte, which fails the server's utf8.ValidString check, + // so the id is silently DROPPED. Not asserting transmitted bytes here — + // that is a transport-encoding property, not this interceptor's contract. + test("a Latin-1 value (sess-é) ⇒ the header is absent", async () => { + const { sessionId } = await captureRequest((fetch) => + createCompassClient( + createCompassWebTransport("http://compass.localhost", undefined, { + fetch: fetch as typeof globalThis.fetch, + sessionId: () => "sess-é", + }), + ).getServerInfo({}), + ); + + expect(sessionId).toBeNull(); + }); + + // ONE transport, hence ONE interceptor instance, driven across two requests + // with a different capturing fetch each time. Building a fresh transport per + // request would let a construction-time memo re-read the getter and pass + // both multi-request cases below, which is exactly the defect they exist to + // catch — so the fetch is indirected through a mutable slot instead. + function oneClientAcrossRequests(sessionId: () => string | undefined) { + let currentFetch: FetchLike = () => + Promise.reject(new Error("oneClientAcrossRequests: no fetch installed")); + const client = createCompassClient( + createCompassWebTransport("http://compass.localhost", undefined, { + fetch: ((input: RequestInfo | URL, init?: RequestInit) => + currentFetch(input, init)) as typeof globalThis.fetch, + sessionId, + }), + ); + return (fetch: FetchLike) => { + currentFetch = fetch; + return client.getServerInfo({}); + }; + } + + test("the getter is called per request, so a fresh value is sent", async () => { + const ids = ["sess-first", "sess-second"]; + let call = 0; + const build = oneClientAcrossRequests(() => ids[call++]); + + const first = await captureRequest(build); + const second = await captureRequest(build); + + expect(first.sessionId).toBe("sess-first"); + expect(second.sessionId).toBe("sess-second"); + }); + + // Self-healing, the direction forward-propagation does NOT cover: a + // construction-time cache or a first-value memo still passes the laziness + // case above while failing this one, because it would pin the early empty + // value forever. + test('"" on the first call then a valid id on the second ⇒ only the second carries the header', async () => { + const ids: (string | undefined)[] = ["", validId]; + let call = 0; + const build = oneClientAcrossRequests(() => ids[call++]); + + const first = await captureRequest(build); + const second = await captureRequest(build); + + expect(first.sessionId).toBeNull(); + expect(second.sessionId).toBe(validId); + }); + + // The guard is deliberately NARROWER than `Headers.set`: set accepts a space, + // a tab, and DEL, and this rejects all three. Nothing else asserts that extra + // narrowing, so widening the class one codepoint (0x20 for 0x21) would put a + // space-bearing value on the wire unnoticed. Capture seam is correct here — + // none of these makes `Headers.set` throw, so the readback IS the assertion. + const narrowerThanHeadersSet: [string, string][] = [ + ["a space", "sess id"], + ["a tab", "sess\tid"], + ["a DEL byte", "sess\x7f"], + ]; + + for (const [label, value] of narrowerThanHeadersSet) { + test(`${label} ⇒ the header is absent, though Headers.set would take it`, async () => { + const { sessionId } = await captureRequest((fetch) => + createCompassClient( + createCompassWebTransport("http://compass.localhost", undefined, { + fetch: fetch as typeof globalThis.fetch, + sessionId: () => value, + }), + ).getServerInfo({}), + ); + + expect(sessionId).toBeNull(); + }); + } +}); + +// Three values that make `Headers.set` THROW a TypeError, so a missing guard +// would fail the whole RPC rather than merely lose a correlation key. Each case +// asserts the full triple: header absent, `next` ran exactly once, and the +// awaited result IS the sentinel — the third is load-bearing, since it cannot +// pass if the interceptor threw before reaching `next`. Values are kept SHORT +// on purpose: a long non-ASCII value is rejected by the length cap first and +// never reaches `header.set`, so it could not exercise the throw at all. +describe("sessionIdInterceptor rejects would-throw values without failing the request", () => { + const wouldThrow: [string, string][] = [ + ["a non-Latin-1 id (> U+00FF)", "sess-日本語"], + ["a value containing CRLF", "sess\r\nx"], + ["a lone surrogate", "\uD800"], + ]; + + for (const [label, value] of wouldThrow) { + test(`${label} ⇒ no header, next still ran, nothing threw`, async () => { + const seam = directSeam(); + + const result = await sessionIdInterceptor(() => value)( + seam.next as never, + )(seam.req as never); + + expect(seam.req.header.get(posthogSessionHeader)).toBeNull(); + expect(seam.calls).toBe(1); + expect(result as unknown).toBe(seam.sentinel); + }); + } +}); + +// `undefined` and `""` are absent-value cases rather than values `Headers.set` +// rejects — but the GUARD can throw on them, which puts them in the class above: +// a bare `SENDABLE.test(undefined)` coerces to the string "undefined" and +// PASSES, so evaluation reaches `id.length` on undefined and throws. So the +// capture seam cannot witness them either, and `undefined` is the case that +// matters most: it is the shipped default (`NoopAnalytics.sessionId()`), so a +// guard that throws on it fails EVERY request whenever analytics is off. +describe("sessionIdInterceptor survives an absent session id", () => { + const absent: [string, string | undefined][] = [ + ["undefined — analytics off, the shipped default", undefined], + ['"" — posthog before it has initialized', ""], + ]; + + for (const [label, value] of absent) { + test(`${label} ⇒ no header, next still ran, nothing threw`, async () => { + const seam = directSeam(); + + const result = await sessionIdInterceptor(() => value)( + seam.next as never, + )(seam.req as never); + + expect(seam.req.header.get(posthogSessionHeader)).toBeNull(); + expect(seam.calls).toBe(1); + expect(result as unknown).toBe(seam.sentinel); + }); + } +}); + describe("callInterceptors installs only what was asked for", () => { // The behavioral claim of the omitted-means-off rule: neither concern // requested ⇒ the transport is handed `undefined`, NOT an empty list, so an @@ -634,4 +919,72 @@ describe("callInterceptors installs only what was asked for", () => { expect(opts.interceptors).toHaveLength(2); }); + + // The direction that catches an append placed inside the old early return: + // `callInterceptors` used to be `const bearer = ...; if (!traceSink) return + // bearer;`, so a session interceptor appended after that guard is skipped + // entirely whenever no trace sink is configured — and every OTHER + // membership direction below still passes. This is the only one that reddens. + test("a sessionId getter alone ⇒ exactly one interceptor", () => { + const opts = transportOptionsFor(() => + createCompassWebTransport("http://compass.localhost", undefined, { + sessionId: () => "sess-1", + }), + ); + + expect(opts.interceptors).toHaveLength(1); + }); + + test("sessionId and sink ⇒ exactly two interceptors", () => { + const sink: TraceIdSink = { current: undefined }; + const opts = transportOptionsFor(() => + createCompassWebTransport("http://compass.localhost", undefined, { + traceSink: sink, + sessionId: () => "sess-1", + }), + ); + + expect(opts.interceptors).toHaveLength(2); + }); + + test("token, sink and sessionId ⇒ exactly three interceptors", () => { + const sink: TraceIdSink = { current: undefined }; + const opts = transportOptionsFor(() => + createCompassWebTransport("http://compass.localhost", "tok", { + traceSink: sink, + sessionId: () => "sess-1", + }), + ); + + expect(opts.interceptors).toHaveLength(3); + }); + + // Composition ORDER, not just membership. The restructure from an early + // return to an accumulating list made order a fresh degree of freedom, and + // every case above is order-blind (`toHaveLength` counts). Order is benign + // TODAY — the session interceptor writes a request header and the trace one + // reads a response header, so they commute — and this pins it so the suite + // notices if that stops being true. Each interceptor is identified by its + // observable effect: position 0 stamps authorization, position 2 stamps the + // session header, so trace is the middle by elimination. + test("token, sink and sessionId ⇒ bearer first, session last", async () => { + const sink: TraceIdSink = { current: undefined }; + const opts = transportOptionsFor(() => + createCompassWebTransport("http://compass.localhost", "tok", { + traceSink: sink, + sessionId: () => "sess-1", + }), + ); + const interceptors = opts.interceptors ?? []; + + const first = directSeam(); + await interceptors[0]?.(first.next as never)(first.req as never); + expect(first.req.header.get("authorization")).toBe("Bearer tok"); + expect(first.req.header.get(posthogSessionHeader)).toBeNull(); + + const last = directSeam(); + await interceptors[2]?.(last.next as never)(last.req as never); + expect(last.req.header.get(posthogSessionHeader)).toBe("sess-1"); + expect(last.req.header.get("authorization")).toBeNull(); + }); }); diff --git a/packages/compass-client/src/index.ts b/packages/compass-client/src/index.ts index b0e76ee1d..5c79abc01 100644 --- a/packages/compass-client/src/index.ts +++ b/packages/compass-client/src/index.ts @@ -46,12 +46,11 @@ const traceResponseHeader = "traceresponse"; /** * A one-slot mailbox holding the trace id of the most recent server reply. * - * Mutable on purpose, and the mutability is the whole point: the transport is - * constructed during boot BEFORE the analytics client exists, so the writer - * (this package's response interceptor) and the reader (the analytics wrapper, - * layers above) cannot be introduced to each other at construction time. A - * stable reference handed to both closes that gap without reordering boot and - * without the transport layer taking a dependency on analytics. + * Mutable on purpose, and the mutability is the whole point: the trace id + * arrives on a REPLY, so the writer (this package's response interceptor) has + * no value to hand the reader (the analytics wrapper, layers above) at + * construction time, whatever order boot runs in. A stable reference handed to + * both closes that gap without the transport layer depending on analytics. * * The write discipline — the transport interceptor writes, everything above it * only reads — is a CONVENTION, not a type guarantee: `current` is structurally @@ -161,21 +160,104 @@ export function traceResponseInterceptor(sink: TraceIdSink): Interceptor { }; } +/** The PostHog session-id REQUEST header the server's J1 interceptor reads + * (go/internal/otel/interceptor.go, PostHogSessionHeader). Already CORS-allowed + * by the network door, so a browser may send it cross-origin. */ +export const posthogSessionHeader = "X-POSTHOG-SESSION-ID"; + +// Printable ASCII only — no space (0x20), no control byte, nothing above 0x7E. +const SENDABLE = /^[\x21-\x7E]+$/; + +// Mirrors the server's maxSessionIDLen (go/internal/otel/interceptor.go), whose +// check is `len(id) > maxSessionIDLen`, so 200 is legal on both sides and the +// cap here is inclusive too. On input this guard accepts, `.length` IS the +// UTF-8 byte count, so no TextEncoder is needed to mean the same thing as Go's +// `len()`. +const MAX_SESSION_ID_LEN = 200; + +/** + * Whether `id` can be put on the wire as a session-id header value at all. + * + * Deliberately printable-ASCII rather than UTF-8-shaped, because `req.header` + * is a fetch `Headers` and `Headers.set` takes a WebIDL ByteString: + * + * - A perfectly well-formed id containing any code point above U+00FF makes + * `Headers.set` THROW a `TypeError`, which would fail the whole RPC. An + * analytics nicety that can kill every request is worse than any + * sender-side rejection, so such a value must be rejected BEFORE `set`. + * - U+0080–U+00FF does not throw: `set` accepts it and a browser emits it as a + * single raw high byte, which the server then rejects as invalid UTF-8 and + * silently DROPS — the same lost key, harder to notice. + * - A value containing CRLF also throws in `Headers.set`; `\x21-\x7E` excludes + * it, so it fails quietly here instead of failing the request. + * + * That makes this a strict SUBSET of what `Headers.set` accepts (`set` takes + * space and tab; this does not) and strictly stronger than the server's own + * `<=200 bytes` + valid-UTF-8 pair. Whitespace-only and empty values are + * rejected by the regex, which the server would trim-and-drop anyway. No + * `isWellFormed` either: a lone surrogate is non-ASCII, so it is already out. + */ +export function isSendableSessionId(id: string): boolean { + return SENDABLE.test(id) && id.length <= MAX_SESSION_ID_LEN; +} + +/** + * Sets `X-POSTHOG-SESSION-ID` on every request from a lazy session-id source. + * + * No usable value (undefined, empty, oversized, non-ASCII) ⇒ the header is not + * set at all — never an empty header, which would spend wire bytes asserting a + * correlation that does not exist and be trim-dropped by the server regardless. + * + * `sessionId` is called PER REQUEST and never cached: posthog-js's + * `get_session_id()` can legitimately return `""` before it is fully + * initialized, so an early request simply carries no header and the next one + * self-heals once a session exists. A construction-time read would pin that + * degraded state forever. + * + * Sent on ALL requests, unary and stream alike (`req.stream` is deliberately + * not inspected). The server reads the header only on unary — its + * `NewSessionIDInterceptor` is a `connect.UnaryInterceptorFunc` — so a stream + * request carries an unread header, which costs bytes, not correctness. Gating + * on `req.stream` would couple this client to a server-side interceptor kind it + * cannot observe, for a few bytes per stream. + */ +export function sessionIdInterceptor( + sessionId: () => string | undefined, +): Interceptor { + return (next) => (req) => { + const id = sessionId(); + if (id !== undefined && isSendableSessionId(id)) { + req.header.set(posthogSessionHeader, id); + } + return next(req); + }; +} + // The full interceptor list every client/transport factory installs, and the one -// place the two concerns compose. The bearer rule is unchanged (and still throws -// first on a misconfigured credential). The trace sink follows the same -// omitted-means-off discipline: no sink ⇒ no trace interceptor at all, so a -// caller that does not ask for correlation gets byte-identical behavior — -// including `undefined` rather than an empty list when neither is asked for. +// place the three concerns compose. The bearer rule is unchanged (and still +// throws first on a misconfigured credential). The trace sink and the session-id +// source follow the same omitted-means-off discipline: no sink ⇒ no trace +// interceptor at all and no getter ⇒ no session interceptor at all, so a caller +// that does not ask for correlation gets byte-identical behavior — including +// `undefined` rather than an empty list when nothing is asked for. +// +// Built as one list with no early return on purpose: an append placed after an +// `if (!traceSink) return bearer` guard would be skipped entirely whenever no +// trace sink is configured, so `sessionId` alone would silently install nothing. function callInterceptors( token?: string, traceSink?: TraceIdSink, + sessionId?: () => string | undefined, ): Interceptor[] | undefined { const bearer = bearerInterceptors(token); - if (!traceSink) { - return bearer; + const interceptors = [...(bearer ?? [])]; + if (traceSink) { + interceptors.push(traceResponseInterceptor(traceSink)); } - return [...(bearer ?? []), traceResponseInterceptor(traceSink)]; + if (sessionId) { + interceptors.push(sessionIdInterceptor(sessionId)); + } + return interceptors.length > 0 ? interceptors : undefined; } /** A typed client for the Compass server over a given transport. */ @@ -205,11 +287,21 @@ export function createCompassClient(transport: Transport): CompassClient { * `opts.traceSink` opts this transport into recording each reply's * `traceresponse` trace id; omitted, no trace interceptor is installed and the * transport behaves exactly as before. + * + * `opts.sessionId` opts this transport into stamping the PostHog session id on + * every outgoing request, read fresh from the getter per request; omitted, no + * session interceptor is installed and the transport behaves exactly as before. + * This is the shipped path for the header — see the note on the per-client + * factories below. */ export function createCompassWebTransport( baseUrl: string, token?: string, - opts?: { fetch?: typeof globalThis.fetch; traceSink?: TraceIdSink }, + opts?: { + fetch?: typeof globalThis.fetch; + traceSink?: TraceIdSink; + sessionId?: () => string | undefined; + }, ): Transport { return createGrpcWebTransport({ baseUrl, @@ -218,7 +310,7 @@ export function createCompassWebTransport( // truthiness guard keeps the browser dev path (no injected fetch) building // the same `{ baseUrl, interceptors }` config as before. ...(opts?.fetch ? { fetch: opts.fetch } : {}), - interceptors: callInterceptors(token, opts?.traceSink), + interceptors: callInterceptors(token, opts?.traceSink, opts?.sessionId), }); } @@ -233,6 +325,17 @@ export type { Transport } from "@connectrpc/connect"; // layer. Dev/test-only; the shipped app dials `createCompassWebTransport`. export { createRouterTransport } from "@connectrpc/connect"; +// The four per-client factories below — `createCompassWebClient`, +// `createCompassClientOverFetch`, `createCommsWebClient`, +// `createCommsClientOverFetch` — deliberately do NOT take a `sessionId` +// option, so a client built through any of them sends NO +// `X-POSTHOG-SESSION-ID` header. That is not an oversight: the shipped path +// for the session-id header is `createLiveClients` → `createCompassWebTransport` +// (the sole production transport construction; both native-shell modes route +// through it via `conn.fetchImpl`), and these four have no production caller. +// A future caller that needs the header must dial `createCompassWebTransport` +// with `opts.sessionId` rather than assume it rides along here. + /** * Create a compass.v1 client over gRPC-Web at `baseUrl` — the door the web UI * uses. Bundles the transport so UI code imports only `@compass/client`. When