diff --git a/docs/designs/infra/release/compass-distribution/design.md b/docs/designs/infra/release/compass-distribution/design.md index 63e6b4c76..1c368d73f 100644 --- a/docs/designs/infra/release/compass-distribution/design.md +++ b/docs/designs/infra/release/compass-distribution/design.md @@ -419,6 +419,18 @@ Mechanics, grounded in the current seams: `readPgidFile` dispatches on the tag; the hard-error-on-malformed discipline is unchanged ("signaling off a half-understood record is exactly the blast radius the design forbids", `pgidfile.go:100-103`). + + **Boot identity (RIG-4570 option A).** The v2 header is + ` []`: the writer stamps the current boot + (Linux `/proc/sys/kernel/random/boot_id`, darwin `kern.bootsessionuuid`; + both are per-boot UUIDs that a wall-clock step cannot move). A + `down` that reads a boot id differing from the current boot signals no + process entry and drops the record; container entries keep their name + teardown. If the stack socket still answers under a mismatch, `down` + refuses, keeps the record, and signals nothing. A boot id that is not a + UUID (8-4-4-4-12 hex) is a malformed header. A missing boot id (older + builds, or an unreadable id) is *unknown* and falls back to the + per-entry identity check. A v2 reader that predates the boot id refuses a + three-column header as malformed. A v1 header never carries the column. + **Cross-version rule.** A v1-only binary never half-parses a v2 record — but by the *entry-line grammar*, not a header-version check: shipped v1 `readPgidFile` stores `header[0]` as `Version` and never compares it to diff --git a/docs/designs/ui/compass-stack-cross-process-teardown/design.md b/docs/designs/ui/compass-stack-cross-process-teardown/design.md index 79f92e291..8fd0b933c 100644 --- a/docs/designs/ui/compass-stack-cross-process-teardown/design.md +++ b/docs/designs/ui/compass-stack-cross-process-teardown/design.md @@ -214,6 +214,8 @@ supervised children** (containers scoped out — Open Question 0). because `up` always exits after a successful spawn (`main.go:235-238`), the writer pid is dead in every linger teardown, so it discriminates nothing about whether the *children* are alive. Plain text, trailing newline, 0600. + (Since DL-262 v2 the header also carries an optional boot id; a record from + an earlier boot has no live group to signal — see the DL-262 record.) - **Write timing**: rewritten **atomically (temp + rename in the state dir) after each successful child spawn** in `spawnChain` (`go/internal/stack/stack.go:171-228`), i.e. the file always reflects the set diff --git a/go/cmd/compass-app/embedded.go b/go/cmd/compass-app/embedded.go index 14c10217f..2301fc5a9 100644 --- a/go/cmd/compass-app/embedded.go +++ b/go/cmd/compass-app/embedded.go @@ -27,6 +27,7 @@ import ( "path/filepath" "runtime" "strings" + "syscall" "connectrpc.com/connect" @@ -245,6 +246,10 @@ func runStackDown(bin string) func(ctx context.Context, args []string) error { //nolint:gosec // G204: bin is operator/PATH-resolved (resolveStackBin) and // the argv is pipeline-assembled (stackDownArgs), not user input. cmd := exec.CommandContext(ctx, bin, args...) + // down has already consumed its teardown record, so a timeout must SIGTERM + // it: SIGKILL would skip the survivor rewrite and leak the stack. + cmd.Cancel = func() error { return cmd.Process.Signal(syscall.SIGTERM) } + cmd.WaitDelay = stackDownCancelGrace cmd.Env = prependExecDirToPath(os.Environ(), filepath.Dir(bin)) stderr, cleanup, capErr := captureStderr(cmd) if capErr != nil { diff --git a/go/cmd/compass-app/embedded_test.go b/go/cmd/compass-app/embedded_test.go index 2a1130822..b9472830d 100644 --- a/go/cmd/compass-app/embedded_test.go +++ b/go/cmd/compass-app/embedded_test.go @@ -16,9 +16,11 @@ import ( "errors" "net" "net/http" + "os" "path/filepath" "slices" "strings" + "syscall" "testing" "time" @@ -383,6 +385,37 @@ func TestRunStackDownZeroExitSucceeds(t *testing.T) { } } +// TestRunStackDownCancelSendsSIGTERM: a timed-out down has already consumed its +// teardown record, so cancel must SIGTERM it (letting it rewrite survivors), not +// SIGKILL it. The child traps TERM and leaves a marker only a SIGTERM can write. +func TestRunStackDownCancelSendsSIGTERM(t *testing.T) { + dir := t.TempDir() + ready := filepath.Join(dir, "ready") + marker := filepath.Join(dir, "rewrote") + if err := syscall.Mkfifo(ready, 0o600); err != nil { + t.Fatalf("mkfifo: %v", err) + } + ctx, cancel := context.WithTimeout(context.Background(), embeddedTestTimeout) + defer cancel() + go func() { + // Opening the FIFO blocks until the child has armed its trap. + f, err := os.Open(ready) + if err == nil { + _ = f.Close() // read end of a gate FIFO; nothing to flush + } + cancel() + }() + + script := "trap 'echo ok > \"$1\"; exit 3' TERM; echo > \"$2\"; while :; do sleep 1 & wait; done" + err := runStackDown("/bin/sh")(ctx, []string{"-c", script, "sh", marker, ready}) + if err == nil { + t.Fatal("stackDown err = nil, want the cancelled child's exit error") + } + if _, statErr := os.Stat(marker); statErr != nil { + t.Fatalf("child did not run its SIGTERM handler (marker: %v); err = %v", statErr, err) + } +} + // classifyDeps returns a preflight.Deps whose every injected effect passes (host // GOOS linux) — mirroring preflight_test.go's okDeps. Tests override individual // fields to drive one failing check at a time through classifyPreflight. diff --git a/go/cmd/compass-app/lifecycle.go b/go/cmd/compass-app/lifecycle.go index a2c7ee442..ebd76ec40 100644 --- a/go/cmd/compass-app/lifecycle.go +++ b/go/cmd/compass-app/lifecycle.go @@ -35,6 +35,10 @@ import ( // reusing it. const stackDownTimeout = 60 * time.Second +// stackDownCancelGrace is how long a timed-out down gets after SIGTERM to +// rewrite its survivor record before os/exec escalates to SIGKILL. +const stackDownCancelGrace = 10 * time.Second + // quitController is the explicit "Quit and stop stack" orchestration over its // injected effects. It holds the teardown seam (stackDown), the argv inputs // (params, resolved once in run()), the app-quit indirection (quit, wired to diff --git a/go/cmd/compass-stack/cross_process_podman_test.go b/go/cmd/compass-stack/cross_process_podman_test.go index 8f4edfa98..51fe815dc 100644 --- a/go/cmd/compass-stack/cross_process_podman_test.go +++ b/go/cmd/compass-stack/cross_process_podman_test.go @@ -384,7 +384,7 @@ func waitServerAnswering(t *testing.T, deps stack.Deps, socketPath string) { // its process-group id and the group leader's start-time token. The start time // is what closes the pid-recycling window — the same (Pgid, StartTime) identity // the production teardown checks (internal/stack/pgidfile.go pgidEntry, -// adapters/groupsignal.go Alive). +// adapters/groupsignal.go Liveness). type recordedGroup struct { pgid int startTime uint64 @@ -392,13 +392,11 @@ type recordedGroup struct { // waitGroupsGone polls every recorded process group until it is gone or the // budget elapses — the authoritative "the children are actually dead" proof -// after a cross-process down. "Gone" is identity-checked, mirroring production's -// teardown gate (adapters/groupsignal.go Alive): a group is gone when it is -// ESRCH, OR it still exists but its leader's start time no longer equals the -// recorded token (the kernel recycled the pid to an unrelated leader). Without -// the identity check the probe is a false-FAILURE risk on the shared box — a -// dead child's pgid reused by another process would read as "still alive" and -// fail the test at budget even though teardown worked. +// after a cross-process down. "Gone" mirrors production's teardown gate +// (adapters/groupsignal.go Liveness): a group is gone when it is ESRCH, OR its +// leader's start time no longer equals the recorded token (a recycled pid). +// Without the identity check a dead child's pgid reused by another process +// would read as "still alive" and fail the test even though teardown worked. // // It signals nothing: kill(-pgid, 0) is the existence probe (signal 0), run // only on pgids read from the stack's OWN stack.pgids record, never a scan @@ -429,13 +427,10 @@ func waitGroupsGone(t *testing.T, groups []recordedGroup, budget time.Duration) } } -// groupAlive reports whether the process group named by pgid still exists AND -// its leader's start time equals the recorded token — the same existence-then- -// identity gate production teardown uses (adapters/groupsignal.go Alive). A -// group that is ESRCH, or whose leader start time no longer matches (a recycled -// pid), or whose /proc entry cannot be read is reported not-alive: for a -// post-down liveness probe the safe verdict is "gone", never a false "alive" -// off a pid the kernel reused. It signals nothing — kill(-pgid, 0) is signal 0. +// groupAlive reports whether the recorded process group still needs teardown, +// mirroring adapters/groupsignal.go Liveness: ESRCH or a recycled leader is +// gone; a matching leader or an unreadable one (members outliving a reaped +// leader keep the pgid) is alive. It signals nothing — kill(-pgid, 0) is signal 0. // An unexpected kill errno (not ESRCH/EPERM) is surfaced as an error. func groupAlive(pgid int, startTime uint64) (bool, error) { err := syscall.Kill(-pgid, 0) @@ -445,16 +440,10 @@ func groupAlive(pgid int, startTime uint64) (bool, error) { case err != nil && !errors.Is(err, syscall.EPERM): return false, err // unexpected errno } - // Exists (nil or EPERM). Confirm identity via the leader's start time; a read - // failure means the leader vanished or /proc is unreadable — treat as gone. got, rerr := readLeaderStartTime(pgid) if rerr != nil { - // Deliberate: a /proc read failure on an existing pgid means the leader - // vanished between the two syscalls (or /proc is unreadable) — the safe - // post-down verdict is "gone", never a false "alive". Mirrors production - // adapters/groupsignal.go Alive, which also treats a read failure as - // not-alive. - return false, nil //nolint:nilerr // read failure => leader gone => not-alive (see comment) + // The group exists without a readable leader: orphaned members remain. + return true, nil //nolint:nilerr // read failure on an existing group => orphaned => alive } return got == startTime, nil } diff --git a/go/internal/auth/interceptor.go b/go/internal/auth/interceptor.go index 44a3e106c..ee41d6dc5 100644 --- a/go/internal/auth/interceptor.go +++ b/go/internal/auth/interceptor.go @@ -140,6 +140,7 @@ func resolveBearer(ctx context.Context, st *store.Store, header string) (store.A // Oracle-safe: every resolution failure — unknown, revoked, or a cross-door // (Runner) token — is one indistinguishable CodeUnauthenticated to the client. // The distinct sentinel is logged (debug) as a server-side audit signal only. + // A store fault (ErrTokenLookupFailed) also stays Unauthenticated on this door. slog.DebugContext(ctx, "network door rejected bearer token", "reason", err) return store.AccountID(""), "", connect.NewError(connect.CodeUnauthenticated, errInvalidToken) } diff --git a/go/internal/auth/token.go b/go/internal/auth/token.go index c0dae5311..7ca908072 100644 --- a/go/internal/auth/token.go +++ b/go/internal/auth/token.go @@ -64,14 +64,14 @@ func IssueAccountToken(ctx context.Context, st *store.Store, account store.Accou return "", errors.New("persisting issued token: hash collision on two successive mints") } -// Sentinel resolution failures returned by ResolveToken. They exist so the server -// can LOG which case fired (audit), NOT so a door tells them apart to the client: -// a door MUST map all three to the same bare CodeUnauthenticated, or the response -// becomes an oracle for whether a token is unknown, revoked, or for the other door. +// Sentinel resolution failures returned by ResolveToken. The three credential +// verdicts exist so the server can LOG which case fired (audit), NOT so a door +// tells them apart to the client: a door MUST map them to the same bare +// CodeUnauthenticated, or the response becomes an oracle for whether a token is +// unknown, revoked, or for the other door. ErrTokenLookupFailed is not a verdict. var ( // ErrTokenNotFound: the presented token was never issued (or the store has - // no live record of it). Any unexpected store error folds here too, so - // resolution fails closed. + // no live record of it). ErrTokenNotFound = errors.New("auth: token not found") // ErrTokenRevoked: the token was issued but has since been withdrawn. ErrTokenRevoked = errors.New("auth: token revoked") @@ -79,6 +79,10 @@ var ( // token presented to the account door, or an account token to the Runner // door. The OQ7 cross-door rejection (design.md:1308-1314). ErrWrongKind = errors.New("auth: token subject kind mismatch") + // ErrTokenLookupFailed: the store could not answer, so no verdict exists. It + // still fails closed; a door MAY report it as Unavailable since it says nothing + // about the token. + ErrTokenLookupFailed = errors.New("auth: token lookup failed") ) // ResolveToken authenticates a presented bearer to a subject of the required @@ -88,12 +92,12 @@ var ( // comparison never touches a stored plaintext — there is none), then verifies // the resolved subject's kind against want. // -// It returns a distinct sentinel per failure — ErrTokenNotFound (never issued or -// an unexpected store error, folded here to fail closed), ErrTokenRevoked -// (withdrawn), ErrWrongKind (issued for the other door) — so the server can log -// which fired. Every caller MUST map all three to the same bare +// It returns a distinct sentinel per failure — ErrTokenNotFound (never issued), +// ErrTokenRevoked (withdrawn), ErrWrongKind (issued for the other door) — so the +// server can log which fired. Every caller MUST map those three to the same bare // CodeUnauthenticated: the distinction is a server-side audit signal, never a // client-visible one (a distinguishable response is a token-existence oracle). +// Any other store error, including ctx cancellation, wraps ErrTokenLookupFailed. // Both the account door (want=SubjectAccount) and the Runner door // (want=SubjectRunner) share this one resolver, so the security-critical // resolve+kind-gate lives and is tested in exactly one place; each door adds only @@ -101,12 +105,7 @@ var ( func ResolveToken(ctx context.Context, st *store.Store, presented string, want store.SubjectKind) (store.Subject, error) { subj, err := st.ResolveTokenHash(ctx, hashToken(presented)) if err != nil { - if errors.Is(err, store.ErrTokenRevoked) { - return store.Subject{}, ErrTokenRevoked - } - // store.ErrNotFound — and any other store error — is not a live - // credential; fail closed as not-found. - return store.Subject{}, ErrTokenNotFound + return store.Subject{}, tokenResolutionError(err) } if subj.Kind != want { return store.Subject{}, ErrWrongKind @@ -114,6 +113,17 @@ func ResolveToken(ctx context.Context, st *store.Store, presented string, want s return subj, nil } +// tokenResolutionError distinguishes credential verdicts from store failures. +func tokenResolutionError(err error) error { + if errors.Is(err, store.ErrTokenRevoked) { + return ErrTokenRevoked + } + if errors.Is(err, store.ErrNotFound) { + return ErrTokenNotFound + } + return fmt.Errorf("%w: %w", ErrTokenLookupFailed, err) +} + // RevokeToken withdraws a bearer token by its presented plaintext, marking the // stored hash revoked so ResolveToken thereafter fails it as ErrTokenRevoked. // Hashing lives here (hashToken), the one place issuance and resolution agree on diff --git a/go/internal/auth/token_test.go b/go/internal/auth/token_test.go index 21c3df657..cff765e3e 100644 --- a/go/internal/auth/token_test.go +++ b/go/internal/auth/token_test.go @@ -38,9 +38,8 @@ func TestIssueThenResolveRoundTripsToTheIssuedAccount(t *testing.T) { } } -// unknown_token_resolves_to_none: a token the store never issued must not -// resolve — even with an unrelated live token present — and the failure is the -// distinct ErrTokenNotFound sentinel. +// Unknown tokens return ErrTokenNotFound, while operational lookup errors use +// ErrTokenLookupFailed and retain their cause. func TestUnknownTokenResolvesToNotFound(t *testing.T) { ctx := context.Background() st, admin, _ := openTestStore(t) @@ -56,10 +55,49 @@ func TestUnknownTokenResolvesToNotFound(t *testing.T) { } } -// a revoked token stops resolving and surfaces the distinct ErrTokenRevoked -// sentinel — separate from ErrTokenNotFound so the server can tell a withdrawn -// credential from an unknown one (the distinction is audit-only; the door still -// maps both to one CodeUnauthenticated). +func TestResolveTokenLookupFailuresAreNotNotFound(t *testing.T) { + ctx := context.Background() + st, _, _ := openTestStore(t) + + _, err := ResolveToken(ctx, st, "never-issued", store.SubjectAccount) + if !errors.Is(err, ErrTokenNotFound) { + t.Fatalf("not-found lookup error = %v, want ErrTokenNotFound", err) + } + canceledCtx, cancel := context.WithCancel(ctx) + cancel() + _, err = ResolveToken(canceledCtx, st, "lookup-failure", store.SubjectAccount) + if !errors.Is(err, ErrTokenLookupFailed) { + t.Fatalf("canceled lookup error = %v, want ErrTokenLookupFailed", err) + } + if errors.Is(err, ErrTokenNotFound) { + t.Fatalf("canceled lookup error = %v, must not be ErrTokenNotFound", err) + } +} + +func TestResolveTokenStoreLookupFailureIsNotNotFound(t *testing.T) { + ctx := context.Background() + st, _, _ := openTestStore(t) + st.Close() + + _, err := ResolveToken(ctx, st, "lookup-failure", store.SubjectAccount) + if !errors.Is(err, ErrTokenLookupFailed) { + t.Fatalf("closed-store lookup error = %v, want ErrTokenLookupFailed", err) + } + if errors.Is(err, ErrTokenNotFound) { + t.Fatalf("closed-store lookup error = %v, must not be ErrTokenNotFound", err) + } +} + +func TestTokenResolutionErrorPreservesCause(t *testing.T) { + cause := errors.New("conn refused") + err := tokenResolutionError(cause) + if !errors.Is(err, ErrTokenLookupFailed) || !errors.Is(err, cause) { + t.Fatalf("lookup error = %v, want lookup sentinel and original cause", err) + } +} + +// a revoked token stops resolving and surfaces ErrTokenRevoked, separate from +// ErrTokenNotFound; both are credential verdicts that doors map to Unauthenticated. func TestRevokedTokenResolvesToRevoked(t *testing.T) { ctx := context.Background() st, admin, _ := openTestStore(t) diff --git a/go/internal/fabric/SUBJECTS.md b/go/internal/fabric/SUBJECTS.md index 50868e4b7..d4542face 100644 --- a/go/internal/fabric/SUBJECTS.md +++ b/go/internal/fabric/SUBJECTS.md @@ -277,6 +277,8 @@ instance handles each advisory. - The reason reads `fabric: dropped by the server after N delivery attempts (callback outlived ack_wait)`. - A message that aged out of the stream before the fetch is logged, not parked. +- The fetch has its own bounded deadline. This is needed because the advisory arrives + after `AckWait`. - The fabric's NATS user needs subscribe permission on `$JS.EVENT.ADVISORY.CONSUMER.MAX_DELIVERIES.>`. diff --git a/go/internal/fabric/event_fabric.go b/go/internal/fabric/event_fabric.go index b7c2aebe2..f45b30f48 100644 --- a/go/internal/fabric/event_fabric.go +++ b/go/internal/fabric/event_fabric.go @@ -579,6 +579,9 @@ func (f *Fabric) publishDLQ(ctx context.Context, subject string, data []byte, ca return reason, nil } +// minAdvisoryGetTimeout floors the advisory's stream fetch under a short AckWait. +const minAdvisoryGetTimeout = 5 * time.Second + // maxDeliveriesAdvisory is the server's notice that a consumer gave up on a // message after MaxDeliver attempts. Only the fields the park needs. type maxDeliveriesAdvisory struct { @@ -615,8 +618,9 @@ func (f *Fabric) parkOnMaxDeliveries(ctx context.Context, subject string) (*nats f.notifyParkDecided(path, false) return } - // Bounded so a stalled fetch cannot hold the claim in flight and park its waiters. - getCtx, cancelGet := context.WithTimeout(context.WithoutCancel(ctx), f.cfg.ackWait()) + // Bounded so a stalled fetch cannot hold the claim; floored because AckWait + // has already expired by the advisory and says nothing about fetch latency. + getCtx, cancelGet := context.WithTimeout(context.WithoutCancel(ctx), max(f.cfg.ackWait(), minAdvisoryGetTimeout)) getParkedMsg := f.getParkedMsg if getParkedMsg == nil { getParkedMsg = func(ctx context.Context, seq uint64) (*jetstream.RawStreamMsg, error) { diff --git a/go/internal/fabric/event_fabric_test.go b/go/internal/fabric/event_fabric_test.go index a80c47b1b..092414ac4 100644 --- a/go/internal/fabric/event_fabric_test.go +++ b/go/internal/fabric/event_fabric_test.go @@ -2538,7 +2538,7 @@ func newParkRaceFabric(t *testing.T, ackWait time.Duration) (*Fabric, context.Co return f, ctx, raw, dlq, advisories, wildcard } -func TestCallbackRetriesAfterAdvisoryFetchTimeout(t *testing.T) { +func TestCallbackRetriesAfterAdvisoryFetchFailure(t *testing.T) { t.Parallel() ackWait := 100 * time.Millisecond f, ctx, raw, dlq, advisories, wildcard := newParkRaceFabric(t, ackWait) @@ -2547,18 +2547,22 @@ func TestCallbackRetriesAfterAdvisoryFetchTimeout(t *testing.T) { releaseCallbackOnce := sync.OnceFunc(func() { close(releaseCallback) }) defer releaseCallbackOnce() fetchStarted := make(chan struct{}) - fetchResult := make(chan error, 1) releaseFetch := make(chan struct{}) releaseFetchOnce := sync.OnceFunc(func() { close(releaseFetch) }) defer releaseFetchOnce() waitingClaim := make(chan string, 1) f.parkClaimWaiting = func(path string) { waitingClaim <- path } + fetchBudget := make(chan time.Duration, 1) f.getParkedMsg = func(ctx context.Context, _ uint64) (*jetstream.RawStreamMsg, error) { + deadline, ok := ctx.Deadline() + if !ok { + fetchBudget <- 0 + } else { + fetchBudget <- time.Until(deadline) + } close(fetchStarted) - <-ctx.Done() - fetchResult <- ctx.Err() <-releaseFetch - return nil, context.DeadlineExceeded + return nil, errors.New("injected fetch failure") } decisions := make(chan parkDecision, 2) f.parkDecided = func(path string, published bool) { @@ -2581,7 +2585,7 @@ func TestCallbackRetriesAfterAdvisoryFetchTimeout(t *testing.T) { releaseFetchOnce() unsub() }() - if err := f.Publish(ctx, subject, EventRef{Tenant: "t1", Kind: KindMessagePosted, RowID: "fetch-timeout"}); err != nil { + if err := f.Publish(ctx, subject, EventRef{Tenant: "t1", Kind: KindMessagePosted, RowID: "fetch-failure"}); err != nil { t.Fatalf("Publish: %v", err) } select { @@ -2597,14 +2601,9 @@ func TestCallbackRetriesAfterAdvisoryFetchTimeout(t *testing.T) { case <-ctx.Done(): t.Fatal("advisory did not claim the park before fetching the message") } - var fetchErr error - select { - case fetchErr = <-fetchResult: - case <-ctx.Done(): - t.Fatalf("advisory fetch did not time out before the test context: %v", ctx.Err()) - } - if !errors.Is(fetchErr, context.DeadlineExceeded) { - t.Fatalf("advisory fetch error = %v, want context deadline exceeded", fetchErr) + // Near the floor, not merely above AckWait; the slack absorbs scheduling delay. + if budget := <-fetchBudget; budget <= minAdvisoryGetTimeout-time.Second { + t.Fatalf("advisory fetch budget = %s, want about the %s floor", budget, minAdvisoryGetTimeout) } releaseCallbackOnce() select { @@ -2618,10 +2617,10 @@ func TestCallbackRetriesAfterAdvisoryFetchTimeout(t *testing.T) { releaseFetchOnce() seen := awaitTwoParkDecisions(t, ctx, decisions) if seen["advisory:"+durableName(wildcard)] { - t.Fatal("timed-out advisory fetch was reported as published") + t.Fatal("failed advisory fetch was reported as published") } if !seen["callback:"+durableName(wildcard)] { - t.Fatal("callback did not reclaim and publish after advisory fetch timed out") + t.Fatal("callback did not reclaim and publish after advisory fetch failure") } if err := f.nc.FlushWithContext(ctx); err != nil { t.Fatalf("flushing fabric publish: %v", err) @@ -2631,7 +2630,7 @@ func TestCallbackRetriesAfterAdvisoryFetchTimeout(t *testing.T) { } parked, err := dlq.NextMsgWithContext(ctx) if err != nil { - t.Fatalf("callback did not park the event after advisory fetch timeout: %v", err) + t.Fatalf("callback did not park the event after advisory fetch failure: %v", err) } if got := parked.Header.Get(dlqHeaderSubject); got != subject { t.Fatalf("parked subject = %q, want %q", got, subject) diff --git a/go/internal/runnerhub/auth.go b/go/internal/runnerhub/auth.go index 2231b7472..234593179 100644 --- a/go/internal/runnerhub/auth.go +++ b/go/internal/runnerhub/auth.go @@ -4,23 +4,26 @@ // presented token to SubjectRunner on every RPC. Any other token (account, // revoked, not-found) collapses to a bare CodeUnauthenticated — no oracle, // fail-closed; distinct store sentinels are for server-side logging only. +// A store fault is not a verdict: it returns a fixed Unavailable so the Runner retries. package runnerhub import ( "context" "errors" + "log/slog" "strings" "connectrpc.com/connect" + "github.com/RigelBuild/compass/go/internal/auth" "github.com/RigelBuild/compass/go/internal/store" ) // TokenResolver is the shared credential-resolution seam: sha256 the presented -// token, resolve it in the store, and Kind-gate it against want. It mirrors the -// T3 lane's exported auth.ResolveToken(ctx, st, presented, want) with the store -// closed over. Returns the resolved subject, or a store sentinel (ErrNotFound / -// ErrTokenRevoked / a wrong-kind error) the door collapses to Unauthenticated. +// token, resolve it in the store, and Kind-gate it against want. It mirrors +// auth.ResolveToken(ctx, st, presented, want) with the store closed over. Returns +// the resolved subject, a credential sentinel the door collapses to +// Unauthenticated, or an auth.ErrTokenLookupFailed-wrapped store fault. type TokenResolver func(ctx context.Context, presented string, want store.SubjectKind) (store.Subject, error) // runnerSubjectKey carries the authenticated Runner subject on the request @@ -44,15 +47,19 @@ func withRunnerSubject(ctx context.Context, subj store.Subject) context.Context // account door (compass.proto:246 "authorization: Bearer "). const bearerPrefix = "Bearer " -// errUnauthenticated is the single opaque error every auth failure maps to — no -// detail distinguishes not-found, revoked, or wrong-kind to the client (no +// errUnauthenticated is the single opaque error every credential failure maps to — +// no detail distinguishes not-found, revoked, or wrong-kind to the client (no // oracle). The distinct store sentinels are logged server-side only. var errUnauthenticated = connect.NewError(connect.CodeUnauthenticated, errors.New("unauthenticated")) +// errLookupUnavailable is the fixed store-fault response; the cause can name DB hosts, so it stays server-side. +var errLookupUnavailable = connect.NewError(connect.CodeUnavailable, errors.New("credential check unavailable")) + // authenticate extracts the bearer token from the request header, resolves it as // a SubjectRunner token, and returns a context carrying the subject. Any -// failure — missing/malformed header, not-found, revoked, wrong kind — returns -// errUnauthenticated with no distinguishing detail. +// credential failure — missing/malformed header, not-found, revoked, wrong kind — +// returns errUnauthenticated with no distinguishing detail; a store fault returns +// errLookupUnavailable. func (b *bearerAuth) authenticate(ctx context.Context, header interface{ Get(key string) string }) (context.Context, error) { raw := header.Get("Authorization") if !strings.HasPrefix(raw, bearerPrefix) { @@ -64,9 +71,12 @@ func (b *bearerAuth) authenticate(ctx context.Context, header interface{ Get(key } subj, err := b.resolve(ctx, token, store.SubjectRunner) if err != nil { - // A store sentinel (ErrNotFound / ErrTokenRevoked / wrong-kind) collapses - // to a bare Unauthenticated: the client learns only that it is not - // authenticated, never which. The resolver logs the distinct cause. + // The client learns only that it is not authenticated, never which; a store + // fault is not a credential verdict, so it is retryable Unavailable instead. + if errors.Is(err, auth.ErrTokenLookupFailed) { + slog.WarnContext(ctx, "runner credential check unavailable", "error", err) + return nil, errLookupUnavailable + } return nil, errUnauthenticated } return withRunnerSubject(ctx, subj), nil diff --git a/go/internal/runnerhub/auth_test.go b/go/internal/runnerhub/auth_test.go index d94dd8cf6..624f31a64 100644 --- a/go/internal/runnerhub/auth_test.go +++ b/go/internal/runnerhub/auth_test.go @@ -17,12 +17,14 @@ package runnerhub import ( "context" "errors" + "fmt" "io" "strings" "testing" "connectrpc.com/connect" + "github.com/RigelBuild/compass/go/internal/auth" compassv1internal "github.com/RigelBuild/compass/go/internal/gen/compass/v1" "github.com/RigelBuild/compass/go/internal/store" ) @@ -74,13 +76,45 @@ func TestAuthenticateNoOracleAcrossCauses(t *testing.T) { tc.name, err.Error(), refErr.Error()) } // And it must not leak the underlying store sentinel. - if errors.Is(err, store.ErrNotFound) || errors.Is(err, store.ErrTokenRevoked) { - t.Fatalf("%s error wraps a store sentinel (%v) — the cause leaks to the client", tc.name, err) + if errors.Is(err, auth.ErrTokenLookupFailed) { + t.Fatalf("%s error wraps lookup-failure sentinel", tc.name) } }) } } +func TestAuthenticateLookupFailureIsUnavailable(t *testing.T) { + cause := errors.New("connection failed with db-password-secret") + lookupErr := fmt.Errorf("%w: %w", auth.ErrTokenLookupFailed, cause) + b := &bearerAuth{resolve: func(context.Context, string, store.SubjectKind) (store.Subject, error) { + return store.Subject{}, lookupErr + }} + handlerCalled := false + next := b.unaryInterceptor()(func(context.Context, connect.AnyRequest) (connect.AnyResponse, error) { + handlerCalled = true + return nil, errors.New("unexpected handler invocation") + }) + + request := connect.NewRequest(&compassv1internal.EnrollRequest{}) + request.Header().Set("Authorization", "Bearer runner-tok") + _, err := next(context.Background(), request) + if err == nil { + t.Fatal("Enroll during resolver failure succeeded, want Unavailable") + } + if got := connect.CodeOf(err); got != connect.CodeUnavailable { + t.Fatalf("code = %v, want Unavailable", got) + } + if ce, ok := errors.AsType[*connect.Error](err); !ok || ce.Message() != "credential check unavailable" { + t.Fatalf("error = %v, want fixed message %q", err, "credential check unavailable") + } + if strings.Contains(err.Error(), cause.Error()) { + t.Fatalf("wire error %q contains lookup cause", err) + } + if handlerCalled { + t.Fatal("Enroll handler ran despite resolver failure") + } +} + // A valid SubjectRunner token is accepted and its subject is set on the returned // context, so the handler can read it. A bug that dropped the subject would fail // the defense-in-depth check in Enroll. @@ -251,6 +285,27 @@ func TestSuccessfulDurableReapEnrollsNormallyOverWire(t *testing.T) { } } +func TestResolverFaultReturnsUnavailableOverWire(t *testing.T) { + hub := newHubOnly() + const secretCause = "db-password-secret" + resolve := func(context.Context, string, store.SubjectKind) (store.Subject, error) { + return store.Subject{}, fmt.Errorf("%w: %s", auth.ErrTokenLookupFailed, secretCause) + } + url := newMountedH2CServer(t, hub, resolve) + client := newRawRunnerClient(t, url, "runner-tok") + + _, err := client.Enroll(context.Background(), connect.NewRequest(&compassv1internal.EnrollRequest{RunnerId: "runner-1"})) + if err == nil { + t.Fatal("Enroll during resolver failure succeeded, want Unavailable") + } + if got := connect.CodeOf(err); got != connect.CodeUnavailable { + t.Fatalf("Enroll code = %v, want Unavailable", got) + } + if strings.Contains(err.Error(), secretCause) { + t.Fatalf("Enroll error %q contains resolver cause", err) + } +} + // A missing/empty bearer credential over the wire is Unauthenticated too — the // no-credential path, distinct from a bad credential but the same client-visible // code. diff --git a/go/internal/stack/adapters/collector_container.go b/go/internal/stack/adapters/collector_container.go index e1f3ae837..dd43cd615 100644 --- a/go/internal/stack/adapters/collector_container.go +++ b/go/internal/stack/adapters/collector_container.go @@ -93,23 +93,28 @@ func (c *CollectorContainer) Start(ctx context.Context, spec stack.CollectorCont // treated as PRESENT, not absent: a false "absent" would drop the teardown // target after the pgid record is consumed, stranding a live container. Stop/ // Remove are idempotent, so assuming-present is safe. -func (c *CollectorContainer) Exists(name string) bool { - present, err := c.cli.exists(context.Background(), name) +func (c *CollectorContainer) Exists(ctx context.Context, name string) bool { + present, err := c.cli.exists(ctx, name) if err != nil { return true // cannot confirm absence → assume present and drive teardown } return present } -// Stop requests a graceful `podman stop -t ` (stack.ContainerController). -func (c *CollectorContainer) Stop(name string, timeout time.Duration) error { - return c.cli.stop(context.Background(), name, timeout) +// Stop sends the container its stop signal without waiting (stack.ContainerController). +func (c *CollectorContainer) Stop(ctx context.Context, name string) error { + return c.cli.term(ctx, name) +} + +// RemoveExited removes the container once it has exited (stack.ContainerController). +func (c *CollectorContainer) RemoveExited(ctx context.Context, name string) error { + return c.cli.removeExited(ctx, name) } // Remove force-removes the container, the SIGKILL-tier escalation // (stack.ContainerController): `podman rm -f`. -func (c *CollectorContainer) Remove(name string) error { - return c.cli.remove(context.Background(), name) +func (c *CollectorContainer) Remove(ctx context.Context, name string) error { + return c.cli.remove(ctx, name) } // ProbeCollector issues an HTTP GET against the collector's health_check diff --git a/go/internal/stack/adapters/collector_container_test.go b/go/internal/stack/adapters/collector_container_test.go index 6aaa22aaa..7e64a3b7d 100644 --- a/go/internal/stack/adapters/collector_container_test.go +++ b/go/internal/stack/adapters/collector_container_test.go @@ -155,25 +155,26 @@ func TestCollectorProbeUnhealthy(t *testing.T) { } // TestCollectorControllerDispatch pins the ContainerController seam this adapter -// also fills: Exists reads the fake's existence map, Stop and Remove drive the -// respective podman calls by name. +// also fills: Exists reads the fake's existence map, Stop sends the non-blocking +// stop signal, and Remove drives the force-remove, all by name. func TestCollectorControllerDispatch(t *testing.T) { + ctx := context.Background() cli := &fakeContainerCLI{existsResp: map[string]bool{"compass-otel-collector-x": true}} cc := &CollectorContainer{cli: cli, health: &fakeHealthGetter{}} - if !cc.Exists("compass-otel-collector-x") { + if !cc.Exists(ctx, "compass-otel-collector-x") { t.Error("Exists(present) = false, want true") } - if cc.Exists("absent") { + if cc.Exists(ctx, "absent") { t.Error("Exists(absent) = true, want false") } - if err := cc.Stop("compass-otel-collector-x", 10*time.Second); err != nil { + if err := cc.Stop(ctx, "compass-otel-collector-x"); err != nil { t.Fatalf("Stop() = %v", err) } - if !reflect.DeepEqual(cli.stopped, []string{"compass-otel-collector-x"}) { - t.Errorf("stop calls = %v, want one stop", cli.stopped) + if len(cli.stopped) != 0 || !reflect.DeepEqual(cli.termed, []string{"compass-otel-collector-x"}) { + t.Errorf("stop calls = %v, term calls = %v, want one term", cli.stopped, cli.termed) } - if err := cc.Remove("compass-otel-collector-x"); err != nil { + if err := cc.Remove(ctx, "compass-otel-collector-x"); err != nil { t.Fatalf("Remove() = %v", err) } if !reflect.DeepEqual(cli.removed, []string{"compass-otel-collector-x"}) { @@ -188,7 +189,7 @@ func TestCollectorControllerDispatch(t *testing.T) { func TestCollectorExistsAssumesPresentOnEngineError(t *testing.T) { cli := &fakeContainerCLI{existsErr: errors.New("podman daemon wedged")} cc := &CollectorContainer{cli: cli, health: &fakeHealthGetter{}} - if !cc.Exists("compass-otel-collector-x") { + if !cc.Exists(context.Background(), "compass-otel-collector-x") { t.Fatal("Exists on engine error = false, want true (assume present, drive teardown)") } } diff --git a/go/internal/stack/adapters/gateway_container.go b/go/internal/stack/adapters/gateway_container.go index f227131f5..fe3146886 100644 --- a/go/internal/stack/adapters/gateway_container.go +++ b/go/internal/stack/adapters/gateway_container.go @@ -71,18 +71,25 @@ func ensureGatewayToken(path string) error { } // Exists reports presence; an engine error counts as present so teardown still runs. -func (c *GatewayContainer) Exists(name string) bool { - present, err := c.cli.exists(context.Background(), name) +func (c *GatewayContainer) Exists(ctx context.Context, name string) bool { + present, err := c.cli.exists(ctx, name) return err != nil || present } -// Stop is the ContainerController graceful stop (`podman stop -t`). -func (c *GatewayContainer) Stop(name string, timeout time.Duration) error { - return c.cli.stop(context.Background(), name, timeout) +// Stop is the ContainerController graceful stop: the stop signal, sent without waiting. +func (c *GatewayContainer) Stop(ctx context.Context, name string) error { + return c.cli.term(ctx, name) +} + +// RemoveExited removes the gateway once it has exited; it runs without --rm. +func (c *GatewayContainer) RemoveExited(ctx context.Context, name string) error { + return c.cli.removeExited(ctx, name) } // Remove is the ContainerController hard kill (`podman rm -f`). -func (c *GatewayContainer) Remove(name string) error { return c.cli.remove(context.Background(), name) } +func (c *GatewayContainer) Remove(ctx context.Context, name string) error { + return c.cli.remove(ctx, name) +} // ProbeGateway returns nil once GET /healthz answers 200. func (c *GatewayContainer) ProbeGateway(ctx context.Context, endpoint string) error { @@ -121,14 +128,14 @@ type gatewayProcess struct { var _ stack.Process = (*gatewayProcess)(nil) -func (p *gatewayProcess) Signal(sig stack.ProcessSignal) error { +func (p *gatewayProcess) Signal(ctx context.Context, sig stack.ProcessSignal) error { if sig != stack.SignalTerm { return fmt.Errorf("unknown process signal %d", int(sig)) } - if err := p.cli.stop(context.Background(), p.name, p.stopTimeout); err != nil { + if err := p.cli.stop(ctx, p.name, p.stopTimeout); err != nil { return fmt.Errorf("podman stop gateway %q: %w", p.name, err) } - if err := p.cli.remove(context.Background(), p.name); err != nil { + if err := p.cli.remove(ctx, p.name); err != nil { return fmt.Errorf("podman remove gateway %q: %w", p.name, err) } return nil diff --git a/go/internal/stack/adapters/gateway_container_test.go b/go/internal/stack/adapters/gateway_container_test.go index d2428a05d..21b33c874 100644 --- a/go/internal/stack/adapters/gateway_container_test.go +++ b/go/internal/stack/adapters/gateway_container_test.go @@ -136,13 +136,13 @@ func TestGatewayProbeHealthyAndUnhealthy(t *testing.T) { func TestGatewaySignalStopsThenRemoves(t *testing.T) { cli := &gatewayFakeCLI{} p := &gatewayProcess{cli: cli, name: "gateway", stopTimeout: 25 * time.Second} - if err := p.Signal(stack.SignalTerm); err != nil { + if err := p.Signal(context.Background(), stack.SignalTerm); err != nil { t.Fatal(err) } if !reflect.DeepEqual(cli.ops, []string{"stop gateway", "remove gateway"}) { t.Fatalf("ops = %v", cli.ops) } - if err := p.Signal(stack.SignalKill); err == nil { + if err := p.Signal(context.Background(), stack.SignalKill); err == nil { t.Fatal("SignalKill accepted") } } @@ -166,17 +166,25 @@ func (f *gatewayFakeCLI) stop(_ context.Context, name string, _ time.Duration) e f.ops = append(f.ops, "stop "+name) return nil } +func (f *gatewayFakeCLI) term(_ context.Context, name string) error { + f.ops = append(f.ops, "term "+name) + return nil +} func (f *gatewayFakeCLI) remove(_ context.Context, name string) error { f.ops = append(f.ops, "remove "+name) return nil } +func (f *gatewayFakeCLI) removeExited(_ context.Context, name string) error { + f.ops = append(f.ops, "remove-exited "+name) + return nil +} func (f *gatewayFakeCLI) exists(_ context.Context, _ string) (bool, error) { return false, nil } var _ containerCLI = (*gatewayFakeCLI)(nil) func TestGatewayExistsAssumesPresentOnEngineError(t *testing.T) { c := &GatewayContainer{cli: &gatewayExistsErrorCLI{}} - if !c.Exists("gateway") { + if !c.Exists(context.Background(), "gateway") { t.Fatal("Exists reported absent when engine errored") } } @@ -186,20 +194,26 @@ type gatewayExistsErrorCLI struct{} func (*gatewayExistsErrorCLI) run(context.Context, []string) error { return nil } func (*gatewayExistsErrorCLI) wait(context.Context, string) error { return nil } func (*gatewayExistsErrorCLI) stop(context.Context, string, time.Duration) error { return nil } +func (*gatewayExistsErrorCLI) term(context.Context, string) error { return nil } func (*gatewayExistsErrorCLI) remove(context.Context, string) error { return nil } +func (*gatewayExistsErrorCLI) removeExited(context.Context, string) error { return nil } func (*gatewayExistsErrorCLI) exists(context.Context, string) (bool, error) { return false, errors.New("engine unavailable") } func TestGatewayControllerDispatchesStopAndRemove(t *testing.T) { + ctx := context.Background() cli := &gatewayFakeCLI{} c := &GatewayContainer{cli: cli} - if err := c.Stop("gateway", 25*time.Second); err != nil { + if err := c.Stop(ctx, "gateway"); err != nil { t.Fatal(err) } - if err := c.Remove("gateway"); err != nil { + if err := c.RemoveExited(ctx, "gateway"); err != nil { t.Fatal(err) } - if !reflect.DeepEqual(cli.ops, []string{"stop gateway", "remove gateway"}) { + if err := c.Remove(ctx, "gateway"); err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(cli.ops, []string{"term gateway", "remove-exited gateway", "remove gateway"}) { t.Fatalf("controller ops = %v", cli.ops) } } diff --git a/go/internal/stack/adapters/groupsignal.go b/go/internal/stack/adapters/groupsignal.go index b65132c52..064707ec6 100644 --- a/go/internal/stack/adapters/groupsignal.go +++ b/go/internal/stack/adapters/groupsignal.go @@ -12,10 +12,10 @@ import ( "github.com/RigelBuild/compass/go/internal/stack" ) -// GroupSignaller is the real stack.GroupSignaller: it signals and -// identity-checks a persisted child process group by pgid for the cross-process -// teardown. It targets the whole group (negative pgid), the same primitive the -// in-process escalation uses (process.go: syscall.Kill(-pid, SIGKILL)). +// GroupSignaller is the real stack.GroupSignaller: it signals and classifies a +// persisted child process group by pgid for the cross-process teardown. It +// targets the whole group (negative pgid), the same primitive the in-process +// escalation uses (process.go: syscall.Kill(-pid, SIGKILL)). // // It is the only teardown seam that touches groups this process did not spawn, // so every operation is scoped to a caller-supplied pgid read from the stack's @@ -56,37 +56,30 @@ func (g *GroupSignaller) Signal(pgid int, sig stack.ProcessSignal) error { return nil } -// Alive reports whether the process group named by pgid exists AND its leader's -// current start time equals startTime — the identity gate. A group that no -// longer exists (kill(-pgid, 0) == ESRCH) or whose leader's start time no longer -// matches (a recycled pid) is reported not-alive, so the caller never signals a -// 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 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 - // falsely report "alive". Treat pgid <= 1 as not-alive so it can never be - // selected as a signal target. +// Liveness classifies a group as gone, owned, orphaned, or recycled. Only ESRCH +// means gone: any other kill(0) error falls through to the leader read, so a +// probe failure can never report a live group torn down. +func (g *GroupSignaller) Liveness(pgid int, startTime uint64) stack.GroupLiveness { + // kill(-1, 0) and kill(0, 0) probe far beyond one child group. if pgid <= 1 { - return false + return stack.GroupGone } - // Existence: signal 0 to the group. ESRCH means gone; EPERM means it exists - // but is not ours (still "exists"); nil means exists. - if err := syscall.Kill(-pgid, 0); err != nil && !errors.Is(err, syscall.EPERM) { - return false + switch err := syscall.Kill(-pgid, 0); { + case errors.Is(err, syscall.ESRCH): + return stack.GroupGone + case errors.Is(err, syscall.EPERM): + // Another uid's group: our children share our uid, so this pgid was reused. + return stack.GroupRecycled } - // Identity: the group leader's pid is the pgid; its start time must match the - // recorded token, closing the pid-recycling window. got, err := readGroupLeaderStartTime(pgid) if err != nil { - return false + // Members outlive a reaped leader, and Linux never reuses a live pgid. + return stack.GroupOrphaned + } + if got != startTime { + return stack.GroupRecycled } - return got == startTime + return stack.GroupOwned } // parseGroupLeaderStat extracts field 22 (starttime) from a /proc//stat diff --git a/go/internal/stack/adapters/groupsignal_darwin.go b/go/internal/stack/adapters/groupsignal_darwin.go index 96a8606f7..97db84cfc 100644 --- a/go/internal/stack/adapters/groupsignal_darwin.go +++ b/go/internal/stack/adapters/groupsignal_darwin.go @@ -17,8 +17,8 @@ import ( // // 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. +// rejects, and the explicit zero-timeval guard closes the remaining case, so a +// zero token can never match a recorded identity. func readGroupLeaderStartTime(pgid int) (uint64, error) { kp, err := unix.SysctlKinfoProc("kern.proc.pid", pgid) if err != nil { @@ -43,9 +43,9 @@ func readGroupLeaderStartTime(pgid int) (uint64, error) { // 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 +// duplication is the load-bearing one: Liveness 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 +// child as recycled 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 { diff --git a/go/internal/stack/adapters/groupsignal_darwin_test.go b/go/internal/stack/adapters/groupsignal_darwin_test.go index ef05863e0..8972f1aa3 100644 --- a/go/internal/stack/adapters/groupsignal_darwin_test.go +++ b/go/internal/stack/adapters/groupsignal_darwin_test.go @@ -15,10 +15,10 @@ import ( // (stack.TestPackStartTimevalMatchesDownSide) uses and asserts the same literal // uint64. // -// The duplication it guards is load-bearing: Alive compares this packing's +// The duplication it guards is load-bearing: Liveness 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 +// child as recycled 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} @@ -51,7 +51,7 @@ func TestReadGroupLeaderStartTimeSelfIsStable(t *testing.T) { } // TestReadGroupLeaderStartTimeDeadPGIDErrors proves the reader fails closed for -// a pgid that names no process, so Alive reports not-alive rather than matching +// a pgid that names no process rather than matching // on a zero token. func TestReadGroupLeaderStartTimeDeadPGIDErrors(t *testing.T) { dead := deadPGID(t) diff --git a/go/internal/stack/adapters/groupsignal_linux.go b/go/internal/stack/adapters/groupsignal_linux.go index 94dff9b6d..eaaab39dd 100644 --- a/go/internal/stack/adapters/groupsignal_linux.go +++ b/go/internal/stack/adapters/groupsignal_linux.go @@ -12,8 +12,8 @@ import ( // 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. +// A pgid that names no process has no /proc entry, so the read fails rather +// than returning a token that could match a recorded identity. func readGroupLeaderStartTime(pgid int) (uint64, error) { data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pgid)) if err != nil { diff --git a/go/internal/stack/adapters/groupsignal_linux_test.go b/go/internal/stack/adapters/groupsignal_linux_test.go new file mode 100644 index 000000000..2a94e4fb2 --- /dev/null +++ b/go/internal/stack/adapters/groupsignal_linux_test.go @@ -0,0 +1,49 @@ +//go:build linux + +package adapters + +import ( + "os" + "strconv" + "syscall" + "testing" + + "github.com/RigelBuild/compass/go/internal/stack" +) + +// TestGroupSignallerLivenessForeignUIDIsRecycled pins that another uid's group +// is never ours: kill(0) EPERM must not fall through to an orphaned verdict. +func TestGroupSignallerLivenessForeignUIDIsRecycled(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root can signal every group, so no EPERM is observable") + } + pgid := foreignGroupLeader(t) + // The real token would otherwise read as owned, so only EPERM can say recycled. + startTime, err := readGroupLeaderStartTime(pgid) + if err != nil { + t.Skipf("leader %d start time unreadable: %v", pgid, err) + } + if got := NewGroupSignaller().Liveness(pgid, startTime); got != stack.GroupRecycled { + t.Fatalf("Liveness(%d) for another uid's group = %v, want GroupRecycled", pgid, got) + } +} + +// foreignGroupLeader returns a live pgid > 1 that kill(0) reports as EPERM. +func foreignGroupLeader(t *testing.T) int { + t.Helper() + entries, err := os.ReadDir("/proc") + if err != nil { + t.Fatalf("read /proc: %v", err) + } + for _, e := range entries { + pid, err := strconv.Atoi(e.Name()) + if err != nil || pid <= 1 { + continue + } + if pgid, err := syscall.Getpgid(pid); err == nil && pgid == pid && syscall.Kill(-pid, 0) == syscall.EPERM { + return pid + } + } + t.Skip("no other uid's process group leader is visible on this host") + return 0 +} diff --git a/go/internal/stack/adapters/groupsignal_other.go b/go/internal/stack/adapters/groupsignal_other.go index 8cfaab5e1..1f11e7504 100644 --- a/go/internal/stack/adapters/groupsignal_other.go +++ b/go/internal/stack/adapters/groupsignal_other.go @@ -12,10 +12,8 @@ import ( // 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. +// Refusing is fail-closed here too: the spawn side refuses as well, so no +// record carrying a token for this host is ever written. 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 "+ diff --git a/go/internal/stack/adapters/groupsignal_test.go b/go/internal/stack/adapters/groupsignal_test.go index 402fbd512..e7094392c 100644 --- a/go/internal/stack/adapters/groupsignal_test.go +++ b/go/internal/stack/adapters/groupsignal_test.go @@ -4,63 +4,104 @@ package adapters import ( "context" + "errors" + "path/filepath" + "syscall" "testing" + "time" "github.com/RigelBuild/compass/go/internal/stack" ) -// TestGroupSignallerAliveThenSignalTearsDown drives the real syscall adapter -// against a real child process group (the re-exec helper), with no timing -// guesses: every wait is gated on an event (the child's ready file via -// startHelper, then proc.Wait for exit). -// -// 1. A freshly started, identity-matched group is Alive. -// 2. A wrong start-time token (a recycled pid) reports NOT alive — the identity -// gate, not bare existence. -// 3. A real SIGTERM through Signal delivers to the group; after the child exits -// (gated on proc.Wait), the group is no longer Alive. -func TestGroupSignallerAliveThenSignalTearsDown(t *testing.T) { +// TestGroupSignallerLivenessThenSignalTearsDown drives the real adapter against +// a real child group: owned with the right token, recycled with a wrong one, and +// gone once a delivered SIGTERM ends it. Every wait is event-gated. +func TestGroupSignallerLivenessThenSignalTearsDown(t *testing.T) { proc := startHelper(t, "trap", stack.ComponentServer, nil) pgid := proc.Pid() - gs := NewGroupSignaller() startTime, err := readGroupLeaderStartTime(pgid) if err != nil { t.Fatalf("readGroupLeaderStartTime(%d) = %v", pgid, err) } - - // 1. Identity-matched → alive. - if !gs.Alive(pgid, startTime) { - t.Fatalf("Alive(%d, %d) = false, want true for a live identity-matched group", pgid, startTime) + if got := gs.Liveness(pgid, startTime); got != stack.GroupOwned { + t.Fatalf("Liveness(%d, matching token) = %v, want GroupOwned", pgid, got) } - // 2. Wrong token (recycled pid) → not alive. - if gs.Alive(pgid, startTime+1) { - t.Fatalf("Alive(%d, %d) = true, want false for a mismatched start-time token", pgid, startTime+1) + if got := gs.Liveness(pgid, startTime+1); got != stack.GroupRecycled { + t.Fatalf("Liveness(%d, wrong token) = %v, want GroupRecycled", pgid, got) } - // 3. Real SIGTERM to the group; the trap helper converts it to a clean exit. if err := gs.Signal(pgid, stack.SignalTerm); err != nil { t.Fatalf("Signal(SIGTERM) = %v", err) } - // Gate on the actual exit event, not a sleep. if err := proc.Wait(context.Background()); err != nil { t.Fatalf("proc.Wait after SIGTERM = %v", err) } - // The group leader has exited; a re-check with the original token is not alive. - // (kill(-pgid,0) ESRCH, or the /proc read fails — either way not-alive.) - if gs.Alive(pgid, startTime) { - t.Fatalf("Alive(%d, %d) = true after the group exited, want false", pgid, startTime) + // The reaped single-member group is ESRCH. + if got := gs.Liveness(pgid, startTime); got != stack.GroupGone { + t.Fatalf("Liveness(%d) after exit = %v, want GroupGone", pgid, got) + } +} + +// TestGroupSignallerLivenessOrphanedGroup proves a group whose leader was reaped +// while a member lives is orphaned, never gone, and stays signalable. +func TestGroupSignallerLivenessOrphanedGroup(t *testing.T) { + memberReady := filepath.Join(t.TempDir(), "member-ready") + proc := startHelper(t, "forkmember", stack.ComponentServer, []string{helperMemberReadyKey + "=" + memberReady}) + pgid := proc.Pid() + gs := NewGroupSignaller() + // Until the group is seen gone, only this test's members can hold the pgid. + released := false + t.Cleanup(func() { + if released { + return + } + if err := gs.Signal(pgid, stack.SignalKill); err != nil && !errors.Is(err, syscall.ESRCH) { + t.Logf("cleanup SIGKILL of group %d: %v", pgid, err) + } + }) + waitReady(t, memberReady, proc) + + startTime, err := readGroupLeaderStartTime(pgid) + if err != nil { + t.Fatalf("readGroupLeaderStartTime(%d) = %v", pgid, err) + } + // Kill and reap only the leader, by its own pid; the member keeps the pgid. + if err := syscall.Kill(pgid, syscall.SIGKILL); err != nil { + t.Fatalf("SIGKILL leader %d = %v", pgid, err) } + if err := proc.Wait(context.Background()); err == nil { + t.Fatal("proc.Wait after leader SIGKILL = nil, want the kill exit error") + } + if got := gs.Liveness(pgid, startTime); got != stack.GroupOrphaned { + t.Fatalf("Liveness(%d) with a reaped leader and a live member = %v, want GroupOrphaned", pgid, got) + } + + if err := gs.Signal(pgid, stack.SignalKill); err != nil { + t.Fatalf("Signal(SIGKILL) to orphaned group = %v", err) + } + // init reaps the reparented member; poll that event with a deadline. + deadline := time.Now().Add(5 * time.Second) + ticker := time.NewTicker(time.Millisecond) + defer ticker.Stop() + for gs.Liveness(pgid, startTime) != stack.GroupGone { + if !time.Now().Before(deadline) { + t.Fatalf("orphaned group %d still present 5s after SIGKILL", pgid) + } + <-ticker.C + } + released = true } -// TestGroupSignallerAliveDeadGroup proves a pgid that names no process reports -// not-alive rather than erroring or signaling. -func TestGroupSignallerAliveDeadGroup(t *testing.T) { +// TestGroupSignallerLivenessDeadGroup proves a pgid that names no process, and a +// degenerate pgid, report gone rather than erroring or signaling. +func TestGroupSignallerLivenessDeadGroup(t *testing.T) { gs := NewGroupSignaller() - dead := deadPGID(t) - if gs.Alive(dead, 12345) { - t.Fatalf("Alive(%d, ...) = true for a nonexistent group, want false", dead) + for _, pgid := range []int{deadPGID(t), 1, 0, -1} { + if got := gs.Liveness(pgid, 12345); got != stack.GroupGone { + t.Fatalf("Liveness(%d, ...) = %v, want GroupGone", pgid, got) + } } } @@ -100,12 +141,8 @@ func TestParseGroupLeaderStatParsesParenthesizedComm(t *testing.T) { // a high number until kill(-pgid, 0) reports ESRCH. func deadPGID(t *testing.T) int { t.Helper() - gs := NewGroupSignaller() for pgid := 1 << 20; pgid < (1<<20)+100000; pgid++ { - if !gs.Alive(pgid, 0) { - // Alive is false either because ESRCH or because /proc read failed; - // for a pgid with no process the kill(0) is ESRCH — good enough for a - // "not a live group" pgid the signal tests want. + if errors.Is(syscall.Kill(-pgid, 0), syscall.ESRCH) { return pgid } } diff --git a/go/internal/stack/adapters/nats_container.go b/go/internal/stack/adapters/nats_container.go index 5e81774cd..de519c1f5 100644 --- a/go/internal/stack/adapters/nats_container.go +++ b/go/internal/stack/adapters/nats_container.go @@ -98,30 +98,31 @@ func (c *NatsContainer) Start(ctx context.Context, spec stack.NatsContainerSpec) // the teardown target after the pgid record is consumed, stranding a live // container holding the JetStream store's file locks. Stop/Remove are // idempotent, so assuming-present is safe. -// -// The ContainerController seam takes no ctx (stack/deps.go), so there is no -// caller context to thread here — the podman call is bounded by the CLI's own -// per-command timeout. -func (c *NatsContainer) Exists(name string) bool { - present, err := c.cli.exists(context.Background(), name) +func (c *NatsContainer) Exists(ctx context.Context, name string) bool { + present, err := c.cli.exists(ctx, name) if err != nil { return true // cannot confirm absence → assume present and drive teardown } return present } -// Stop requests a graceful `podman stop -t ` (stack.ContainerController), -// which SIGTERMs nats-server and lets it flush the JetStream store. -func (c *NatsContainer) Stop(name string, timeout time.Duration) error { - return c.cli.stop(context.Background(), name, timeout) +// Stop sends the stop signal without waiting (stack.ContainerController), so +// nats-server can flush the JetStream store within the caller's drain budget. +func (c *NatsContainer) Stop(ctx context.Context, name string) error { + return c.cli.term(ctx, name) +} + +// RemoveExited removes the container once it has exited (stack.ContainerController). +func (c *NatsContainer) RemoveExited(ctx context.Context, name string) error { + return c.cli.removeExited(ctx, name) } // Remove force-removes the container, the SIGKILL-tier escalation // (stack.ContainerController): `podman rm -f`. This kills nats-server mid-flush, // so the store recovers on next boot — the escalation is for a server that // ignored the graceful stop, never the first resort. -func (c *NatsContainer) Remove(name string) error { - return c.cli.remove(context.Background(), name) +func (c *NatsContainer) Remove(ctx context.Context, name string) error { + return c.cli.remove(ctx, name) } // ProbeNats issues an HTTP GET against the NATS server's monitoring /healthz diff --git a/go/internal/stack/adapters/nats_container_test.go b/go/internal/stack/adapters/nats_container_test.go index c2655d6f2..9d3963028 100644 --- a/go/internal/stack/adapters/nats_container_test.go +++ b/go/internal/stack/adapters/nats_container_test.go @@ -191,25 +191,26 @@ func TestNatsProbeUnhealthy(t *testing.T) { } // TestNatsControllerDispatch pins the ContainerController seam this adapter also -// fills: Exists reads the fake's existence map, Stop and Remove drive the -// respective podman calls by name. +// fills: Exists reads the fake's existence map, Stop sends the non-blocking stop +// signal, and Remove drives the force-remove, all by name. func TestNatsControllerDispatch(t *testing.T) { + ctx := context.Background() cli := &fakeContainerCLI{existsResp: map[string]bool{"compass-nats-x": true}} nc := &NatsContainer{cli: cli, health: &fakeHealthGetter{}} - if !nc.Exists("compass-nats-x") { + if !nc.Exists(ctx, "compass-nats-x") { t.Error("Exists(present) = false, want true") } - if nc.Exists("absent") { + if nc.Exists(ctx, "absent") { t.Error("Exists(absent) = true, want false") } - if err := nc.Stop("compass-nats-x", 20*time.Second); err != nil { + if err := nc.Stop(ctx, "compass-nats-x"); err != nil { t.Fatalf("Stop() = %v", err) } - if !reflect.DeepEqual(cli.stopped, []string{"compass-nats-x"}) { - t.Errorf("stop calls = %v, want one stop", cli.stopped) + if len(cli.stopped) != 0 || !reflect.DeepEqual(cli.termed, []string{"compass-nats-x"}) { + t.Errorf("stop calls = %v, term calls = %v, want one term", cli.stopped, cli.termed) } - if err := nc.Remove("compass-nats-x"); err != nil { + if err := nc.Remove(ctx, "compass-nats-x"); err != nil { t.Fatalf("Remove() = %v", err) } if !reflect.DeepEqual(cli.removed, []string{"compass-nats-x"}) { @@ -224,7 +225,7 @@ func TestNatsControllerDispatch(t *testing.T) { func TestNatsExistsAssumesPresentOnEngineError(t *testing.T) { cli := &fakeContainerCLI{existsErr: errors.New("podman daemon wedged")} nc := &NatsContainer{cli: cli, health: &fakeHealthGetter{}} - if !nc.Exists("compass-nats-x") { + if !nc.Exists(context.Background(), "compass-nats-x") { t.Fatal("Exists on engine error = false, want true (assume present, drive teardown)") } } diff --git a/go/internal/stack/adapters/postgres_container.go b/go/internal/stack/adapters/postgres_container.go index c42634ac5..e575ab448 100644 --- a/go/internal/stack/adapters/postgres_container.go +++ b/go/internal/stack/adapters/postgres_container.go @@ -54,14 +54,16 @@ var ( ) // containerCLI is the narrow podman surface this adapter needs: run a detached -// container, block on its exit, stop/remove/exists it by name. *podmanExec +// container, block on its exit, stop/term/remove/exists it by name. *podmanExec // satisfies it; a fake satisfies it in tests, so the argv assembly and the // Process lifecycle are unit-testable without a real podman. type containerCLI interface { run(ctx context.Context, args []string) error wait(ctx context.Context, name string) error stop(ctx context.Context, name string, timeout time.Duration) error + term(ctx context.Context, name string) error remove(ctx context.Context, name string) error + removeExited(ctx context.Context, name string) error exists(ctx context.Context, name string) (bool, error) } @@ -110,23 +112,28 @@ func (c *PostgresContainer) Start(ctx context.Context, spec stack.PostgresContai // Assuming-present is safe: Stop/Remove are idempotent (an already-gone // container normalizes to success), so signaling a container that turns out gone // is harmless, while signaling a still-live one is the whole point. -func (c *PostgresContainer) Exists(name string) bool { - present, err := c.cli.exists(context.Background(), name) +func (c *PostgresContainer) Exists(ctx context.Context, name string) bool { + present, err := c.cli.exists(ctx, name) if err != nil { return true // cannot confirm absence → assume present and drive teardown } return present } -// Stop requests a graceful `podman stop -t ` (stack.ContainerController). -func (c *PostgresContainer) Stop(name string, timeout time.Duration) error { - return c.cli.stop(context.Background(), name, timeout) +// Stop sends the container its stop signal without waiting (stack.ContainerController). +func (c *PostgresContainer) Stop(ctx context.Context, name string) error { + return c.cli.term(ctx, name) +} + +// RemoveExited removes the container once it has exited (stack.ContainerController). +func (c *PostgresContainer) RemoveExited(ctx context.Context, name string) error { + return c.cli.removeExited(ctx, name) } // Remove force-removes the container, the SIGKILL-tier escalation // (stack.ContainerController): `podman rm -f`. -func (c *PostgresContainer) Remove(name string) error { - return c.cli.remove(context.Background(), name) +func (c *PostgresContainer) Remove(ctx context.Context, name string) error { + return c.cli.remove(ctx, name) } // runArgs assembles the S4 `podman run` argv (detached). Split out as a pure @@ -238,14 +245,14 @@ type containerProcess struct { // Compile-time proof the handle satisfies the core seam. var _ stack.Process = (*containerProcess)(nil) -// Signal requests a graceful stop: `podman stop -t `. SignalKill is -// not a valid in-process disposition (the cross-process teardown escalates via -// ContainerController.Remove instead), matching the process handle which also -// rejects anything but SignalTerm. -func (p *containerProcess) Signal(sig stack.ProcessSignal) error { +// Signal requests a graceful stop bounded by ctx: `podman stop -t `. +// SignalKill is not a valid in-process disposition (the cross-process teardown +// escalates via ContainerController.Remove instead), matching the process handle +// which also rejects anything but SignalTerm. +func (p *containerProcess) Signal(ctx context.Context, sig stack.ProcessSignal) error { switch sig { case stack.SignalTerm: - if err := p.cli.stop(context.Background(), p.name, p.stopTimeout); err != nil { + if err := p.cli.stop(ctx, p.name, p.stopTimeout); err != nil { return fmt.Errorf("podman stop postgres %q: %w", p.name, err) } return nil @@ -316,10 +323,29 @@ func (e *podmanExec) stop(ctx context.Context, name string, timeout time.Duratio return nil } -// remove force-removes the container (`podman rm -f`). An absent container is -// already removed — not an error. +// term sends the container's configured stop signal and returns without waiting +// for exit, so the caller's drain wait owns the budget. `podman kill` has no +// "use the stop signal" mode, so the signal is read first; postgres stops on +// SIGINT, not SIGTERM. An absent container is already stopped — not an error. +func (e *podmanExec) term(ctx context.Context, name string) error { + sig, err := e.output(ctx, []string{"container", "inspect", "--format", "{{.Config.StopSignal}}", name}) + if err == nil { + if sig == "" { + sig = "SIGTERM" + } + err = e.fireAndCheck(ctx, []string{"kill", "--signal", sig, name}) + } + if err != nil && !isNoSuchContainer(err) { + return err + } + return nil +} + +// remove force-removes the container without the stop-timeout grace (`podman rm +// -f -t 0`): it is the hard-kill tier, reached only after the drain budget. An +// absent container is already removed — not an error. func (e *podmanExec) remove(ctx context.Context, name string) error { - if err := e.fireAndCheck(ctx, []string{"rm", "--force", "--volumes", name}); err != nil { + if err := e.fireAndCheck(ctx, []string{"rm", "--force", "--time", "0", "--volumes", name}); err != nil { if isNoSuchContainer(err) { return nil } @@ -328,6 +354,19 @@ func (e *podmanExec) remove(ctx context.Context, name string) error { return nil } +// removeExited removes the container without --force (`podman rm`), which podman +// refuses while it runs. That refusal, like an absent container, is not an error: +// the caller polls absence and escalates to remove once its budget expires. +func (e *podmanExec) removeExited(ctx context.Context, name string) error { + if err := e.fireAndCheck(ctx, []string{"rm", "--volumes", name}); err != nil { + if isNoSuchContainer(err) || isContainerRunning(err) { + return nil + } + return err + } + return nil +} + // exists reports whether a container with name is present in any state. `podman // container exists` encodes the answer in its exit code (0 present, 1 absent), // so it is read from the exit code, not treated as an error. @@ -352,6 +391,7 @@ func (e *podmanExec) fireAndCheck(ctx context.Context, args []string) error { cctx, cancel := context.WithTimeout(ctx, e.timeout) defer cancel() cmd := exec.CommandContext(cctx, e.program, args...) //nolint:gosec // G204: the container seam — program is the operator-set engine and args are Stack-built from a state-dir-derived spec, neither attacker-controlled + cmd.WaitDelay = podmanWaitDelay var stderr strings.Builder cmd.Stderr = &stderr if err := cmd.Run(); err != nil { @@ -364,9 +404,38 @@ func (e *podmanExec) fireAndCheck(ctx context.Context, args []string) error { return nil } +// output runs `podman ` under the command timeout and returns its trimmed +// stdout, folding a non-zero exit into an error carrying the captured stderr. +func (e *podmanExec) output(ctx context.Context, args []string) (string, error) { + cctx, cancel := context.WithTimeout(ctx, e.timeout) + defer cancel() + cmd := exec.CommandContext(cctx, e.program, args...) //nolint:gosec // G204: same seam as fireAndCheck; args are Stack-built from a state-dir-derived name + cmd.WaitDelay = podmanWaitDelay + var stderr strings.Builder + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + if msg := strings.TrimSpace(stderr.String()); msg != "" { + return "", fmt.Errorf("podman %s: %w: %s", args[0], err, msg) + } + return "", fmt.Errorf("podman %s: %w", args[0], err) + } + return strings.TrimSpace(string(out)), nil +} + +// podmanWaitDelay bounds how long a podman call waits on stdio a child still +// holds open after podman exits or is killed, so a leaked pipe cannot hang down. +const podmanWaitDelay = 2 * time.Second + // isNoSuchContainer reports whether err is podman's "no such container" (the // container vanished in the verify→signal gap, or was already gone). The // teardown treats it as success — the container is gone, which is the goal. func isNoSuchContainer(err error) bool { return err != nil && strings.Contains(err.Error(), "no such container") } + +// isContainerRunning reports whether err is podman refusing a non-forced rm of a +// running or paused container ("container state improper"). +func isContainerRunning(err error) bool { + return err != nil && strings.Contains(err.Error(), "container state improper") +} diff --git a/go/internal/stack/adapters/postgres_container_test.go b/go/internal/stack/adapters/postgres_container_test.go index 4716c56ed..e5c1a7082 100644 --- a/go/internal/stack/adapters/postgres_container_test.go +++ b/go/internal/stack/adapters/postgres_container_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "reflect" + "strings" "testing" "time" @@ -21,7 +22,9 @@ type fakeContainerCLI struct { runErr error waited []string stopped []string + termed []string removed []string + rmExited []string existsResp map[string]bool existsErr error } @@ -41,11 +44,21 @@ func (f *fakeContainerCLI) stop(_ context.Context, name string, _ time.Duration) return nil } +func (f *fakeContainerCLI) term(_ context.Context, name string) error { + f.termed = append(f.termed, name) + return nil +} + func (f *fakeContainerCLI) remove(_ context.Context, name string) error { f.removed = append(f.removed, name) return nil } +func (f *fakeContainerCLI) removeExited(_ context.Context, name string) error { + f.rmExited = append(f.rmExited, name) + return nil +} + func (f *fakeContainerCLI) exists(_ context.Context, name string) (bool, error) { if f.existsErr != nil { return false, f.existsErr @@ -144,7 +157,7 @@ func TestContainerProcessSignalStopsWaitBlocks(t *testing.T) { cli := &fakeContainerCLI{} p := &containerProcess{cli: cli, name: "compass-postgres-x", stopTimeout: 30 * time.Second} - if err := p.Signal(stack.SignalTerm); err != nil { + if err := p.Signal(context.Background(), stack.SignalTerm); err != nil { t.Fatalf("Signal(SignalTerm) = %v, want nil", err) } if !reflect.DeepEqual(cli.stopped, []string{"compass-postgres-x"}) { @@ -156,7 +169,7 @@ func TestContainerProcessSignalStopsWaitBlocks(t *testing.T) { if !reflect.DeepEqual(cli.waited, []string{"compass-postgres-x"}) { t.Fatalf("wait calls = %v, want one wait of the container", cli.waited) } - if err := p.Signal(stack.SignalKill); err == nil { + if err := p.Signal(context.Background(), stack.SignalKill); err == nil { t.Fatal("Signal(SignalKill) = nil, want a rejection (in-process handle is graceful-only)") } if p.Pid() != 0 { @@ -165,32 +178,144 @@ func TestContainerProcessSignalStopsWaitBlocks(t *testing.T) { } // TestControllerDispatch pins the ContainerController seam this adapter also -// fills: Exists reads the fake's existence map, Stop and Remove drive the -// respective podman calls by name. +// fills: Exists reads the fake's existence map; Stop sends the non-blocking +// stop signal, never the blocking `podman stop`; RemoveExited and Remove drive +// their podman calls by name. func TestControllerDispatch(t *testing.T) { + ctx := context.Background() cli := &fakeContainerCLI{existsResp: map[string]bool{"live": true}} pc := &PostgresContainer{cli: cli, superuser: "bob"} - if !pc.Exists("live") { + if !pc.Exists(ctx, "live") { t.Error("Exists(live) = false, want true") } - if pc.Exists("gone") { + if pc.Exists(ctx, "gone") { t.Error("Exists(gone) = true, want false") } - if err := pc.Stop("live", 10*time.Second); err != nil { + if err := pc.Stop(ctx, "live"); err != nil { t.Fatalf("Stop() = %v", err) } - if err := pc.Remove("live"); err != nil { + if err := pc.RemoveExited(ctx, "live"); err != nil { + t.Fatalf("RemoveExited() = %v", err) + } + if err := pc.Remove(ctx, "live"); err != nil { t.Fatalf("Remove() = %v", err) } - if !reflect.DeepEqual(cli.stopped, []string{"live"}) { - t.Errorf("stop calls = %v, want [live]", cli.stopped) + if len(cli.stopped) != 0 || !reflect.DeepEqual(cli.termed, []string{"live"}) { + t.Errorf("stop calls = %v, term calls = %v, want only a term of [live]", cli.stopped, cli.termed) + } + if !reflect.DeepEqual(cli.rmExited, []string{"live"}) { + t.Errorf("non-forced remove calls = %v, want [live]", cli.rmExited) } if !reflect.DeepEqual(cli.removed, []string{"live"}) { t.Errorf("remove calls = %v, want [live]", cli.removed) } } +// TestRemoveExitedToleratesRunningAndAbsent: a non-forced rm that podman refuses +// because the container still runs, or because it is gone, is not an error; any +// other failure still surfaces. The stderr lines are podman 5's real output. +func TestRemoveExitedToleratesRunningAndAbsent(t *testing.T) { + for _, tc := range []struct { + name string + stderr string + wantErr bool + }{ + {"running", "Error: cannot remove container x as it is running - running or paused containers cannot be removed without force: container state improper", false}, + {"absent", `Error: no container with ID or name "x" found: no such container`, false}, + {"engine failure", "Error: database is locked", true}, + } { + t.Run(tc.name, func(t *testing.T) { + e, _ := fakePodman(t, "echo '"+tc.stderr+"' >&2\nexit 2\n") + if err := e.removeExited(context.Background(), "x"); (err != nil) != tc.wantErr { + t.Fatalf("removeExited() = %v, wantErr %v", err, tc.wantErr) + } + }) + } +} + +// fakePodman returns a podmanExec over a shell script with the given body. Each +// invocation's argv is appended, one line per call, to the returned log path. +func fakePodman(t *testing.T, body string) (*podmanExec, string) { + t.Helper() + dir := t.TempDir() + prog := filepath.Join(dir, "podman") + argvLog := filepath.Join(dir, "argv") + script := "#!/bin/sh\necho \"$*\" >> '" + argvLog + "'\n" + body + if err := os.WriteFile(prog, []byte(script), 0o700); err != nil { + t.Fatal(err) + } + return &podmanExec{program: prog, timeout: 5 * time.Second}, argvLog +} + +// readArgv returns the recorded podman invocations, one per element. +func readArgv(t *testing.T, argvLog string) []string { + t.Helper() + data, err := os.ReadFile(argvLog) + if err != nil { + t.Fatalf("read argv log: %v", err) + } + return strings.Split(strings.TrimSpace(string(data)), "\n") +} + +// TestRemoveSkipsStopTimeout: a force-remove is the hard kill, so it must not +// wait out the container's --stop-timeout grace first. +func TestRemoveSkipsStopTimeout(t *testing.T) { + e, argvLog := fakePodman(t, "exit 0\n") + if err := e.remove(context.Background(), "x"); err != nil { + t.Fatalf("remove() = %v", err) + } + if got, want := readArgv(t, argvLog), []string{"rm --force --time 0 --volumes x"}; !reflect.DeepEqual(got, want) { + t.Fatalf("argv = %q, want %q", got, want) + } +} + +// TestTermSendsConfiguredStopSignal: term reads the container's stop signal and +// kills with it (postgres stops on SIGINT), defaults to SIGTERM when none is +// set, and treats a vanished container as already stopped. +func TestTermSendsConfiguredStopSignal(t *testing.T) { + for _, tc := range []struct { + name string + inspect string + wantArgv []string + }{ + {"configured", "echo SIGINT", []string{"container inspect --format {{.Config.StopSignal}} x", "kill --signal SIGINT x"}}, + {"unset", "echo", []string{"container inspect --format {{.Config.StopSignal}} x", "kill --signal SIGTERM x"}}, + {"absent", "echo 'Error: no such container x' >&2; exit 125", []string{"container inspect --format {{.Config.StopSignal}} x"}}, + } { + t.Run(tc.name, func(t *testing.T) { + e, argvLog := fakePodman(t, "if [ \"$1\" = container ]; then "+tc.inspect+"; fi\n") + if err := e.term(context.Background(), "x"); err != nil { + t.Fatalf("term() = %v", err) + } + if got := readArgv(t, argvLog); !reflect.DeepEqual(got, tc.wantArgv) { + t.Fatalf("argv = %q, want %q", got, tc.wantArgv) + } + }) + } +} + +// TestPodmanCallReturnsWhenGrandchildHoldsStderr: a podman that exits while a +// child it spawned keeps stderr open must not hang the call past WaitDelay. +func TestPodmanCallReturnsWhenGrandchildHoldsStderr(t *testing.T) { + e, _ := fakePodman(t, "sleep 8 &\nexit 0\n") + done := make(chan error, 2) + go func() { done <- e.fireAndCheck(context.Background(), []string{"rm", "x"}) }() + go func() { + _, err := e.output(context.Background(), []string{"inspect", "x"}) + done <- err + }() + deadline := time.NewTimer(e.timeout - time.Second) + defer deadline.Stop() + for range 2 { + select { + case <-done: + case <-deadline.C: + t.Fatal("podman call still blocked on an inherited stderr pipe") + } + } +} + // TestExistsAssumesPresentOnEngineError pins the stranded-container guard: a // genuine podman engine error (not the exit-1 "absent" verdict) makes Exists // report PRESENT, so entryAlive still builds a teardown target instead of @@ -200,7 +325,7 @@ func TestExistsAssumesPresentOnEngineError(t *testing.T) { cli := &fakeContainerCLI{existsErr: errors.New("podman: daemon wedged")} pc := &PostgresContainer{cli: cli, superuser: "bob"} - if !pc.Exists("compass-postgres-x") { + if !pc.Exists(context.Background(), "compass-postgres-x") { t.Error("Exists() on a podman engine error = false, want true (assume present so teardown still drives Stop/Remove)") } } diff --git a/go/internal/stack/adapters/process.go b/go/internal/stack/adapters/process.go index 5f7971647..6cccad429 100644 --- a/go/internal/stack/adapters/process.go +++ b/go/internal/stack/adapters/process.go @@ -93,8 +93,8 @@ var _ stack.Process = (*process)(nil) // Signal requests a stop of the given disposition. SignalTerm sends SIGTERM to // the child for a graceful exit; any other ProcessSignal is an error rather than -// a silent no-op. -func (p *process) Signal(sig stack.ProcessSignal) error { +// a silent no-op. A kill(2) cannot block, so ctx is not consulted. +func (p *process) Signal(_ context.Context, sig stack.ProcessSignal) error { switch sig { case stack.SignalTerm: if err := p.cmd.Process.Signal(syscall.SIGTERM); err != nil { diff --git a/go/internal/stack/adapters/process_test.go b/go/internal/stack/adapters/process_test.go index 61bf4f4c6..9e0d0c280 100644 --- a/go/internal/stack/adapters/process_test.go +++ b/go/internal/stack/adapters/process_test.go @@ -6,6 +6,7 @@ import ( "context" "errors" "os" + "os/exec" "os/signal" "path/filepath" "strings" @@ -30,6 +31,9 @@ const ( // normalization in Wait now folds any post-SIGTERM exit code to nil, so an // exit-code-keyed negative control would silently pass. helperEchoOutKey = "STACK_TEST_ECHO_OUT" + // helperMemberReadyKey names the ready file the forkmember helper's group + // member writes once its SIGTERM handler is armed. + helperMemberReadyKey = "STACK_TEST_MEMBER_READY" ) func TestMain(m *testing.M) { @@ -82,6 +86,12 @@ func TestMain(m *testing.M) { // exits clean. helperTrap() os.Exit(0) + case "forkmember": + // Group leader that first starts a same-group member, so the group can + // outlive the leader (an orphaned group). + helperForkMember() + helperTrap() + os.Exit(0) default: os.Exit(99) } @@ -97,6 +107,22 @@ func helperTrap() { <-ch } +// helperForkMember starts this binary in "sleep" mode as a member of the +// caller's process group, signalling readiness through helperMemberReadyKey. +func helperForkMember() { + self, err := os.Executable() + if err != nil { + os.Exit(4) + } + member := exec.Command(self) + member.Env = append(os.Environ(), + helperEnvVar+"=sleep", + helperReadyKey+"="+os.Getenv(helperMemberReadyKey)) + if err := member.Start(); err != nil { + os.Exit(4) + } +} + // helperReadyNoTrap signals readiness WITHOUT installing a SIGTERM handler, so // the child takes SIGTERM's default (terminate) disposition. The ready write is // the parent's start gate exactly as in helperTrap. @@ -155,7 +181,7 @@ func waitReady(t *testing.T, readyPath string, proc stack.Process) { } time.Sleep(time.Millisecond) //nolint:forbidigo // bounded poll tick; event-gated on the child's ready file above with a deadline (rule://go-no-sleep-in-test poll-until exemption) } - _ = proc.Signal(stack.SignalTerm) + _ = proc.Signal(context.Background(), stack.SignalTerm) t.Fatalf("child never armed (ready file %s absent within deadline)", readyPath) } @@ -244,7 +270,7 @@ func TestLifecycleGracefulStopThreadsEnv(t *testing.T) { echoOut := filepath.Join(t.TempDir(), "echo") proc := startHelper(t, "trapecho", stack.ComponentServer, []string{helperEchoKey + "=expected", helperEchoOutKey + "=" + echoOut}) - if err := proc.Signal(stack.SignalTerm); err != nil { + if err := proc.Signal(context.Background(), stack.SignalTerm); err != nil { t.Fatalf("Signal(SignalTerm) = %v", err) } if err := proc.Wait(context.Background()); err != nil { @@ -267,7 +293,7 @@ func TestLifecycleEnvNotThreadedIsObservablyEmpty(t *testing.T) { echoOut := filepath.Join(t.TempDir(), "echo") proc := startHelper(t, "trapecho", stack.ComponentServer, []string{helperEchoOutKey + "=" + echoOut}) - if err := proc.Signal(stack.SignalTerm); err != nil { + if err := proc.Signal(context.Background(), stack.SignalTerm); err != nil { t.Fatalf("Signal(SignalTerm) = %v", err) } if err := proc.Wait(context.Background()); err != nil { @@ -288,7 +314,7 @@ func TestLifecycleEnvNotThreadedIsObservablyEmpty(t *testing.T) { // window the embedded runner also has must not make a clean drain look failed. func TestWaitNormalizesRawSignalTermDeath(t *testing.T) { proc := startHelper(t, "notrap", stack.ComponentRunner, nil) - if err := proc.Signal(stack.SignalTerm); err != nil { + if err := proc.Signal(context.Background(), stack.SignalTerm); err != nil { t.Fatalf("Signal(SignalTerm) = %v", err) } if err := proc.Wait(context.Background()); err != nil { @@ -303,7 +329,7 @@ func TestWaitNormalizesRawSignalTermDeath(t *testing.T) { // miss (that only normalizes a signaled death, not an exit code). func TestWaitNormalizesNonzeroExitAfterSignal(t *testing.T) { proc := startHelper(t, "trapexit1", stack.ComponentRunner, nil) - if err := proc.Signal(stack.SignalTerm); err != nil { + if err := proc.Signal(context.Background(), stack.SignalTerm); err != nil { t.Fatalf("Signal(SignalTerm) = %v", err) } if err := proc.Wait(context.Background()); err != nil { @@ -351,10 +377,10 @@ func TestUnknownSignal(t *testing.T) { proc := startHelper(t, "trap", stack.ComponentServer, []string{helperEchoKey + "=expected"}) // Clean up the child so the test does not leak it. t.Cleanup(func() { - _ = proc.Signal(stack.SignalTerm) + _ = proc.Signal(context.Background(), stack.SignalTerm) _ = proc.Wait(context.Background()) }) - if err := proc.Signal(stack.ProcessSignal(99)); err == nil { + if err := proc.Signal(context.Background(), stack.ProcessSignal(99)); err == nil { t.Fatal("Signal with unknown disposition err = nil, want error") } } diff --git a/go/internal/stack/bootid_darwin.go b/go/internal/stack/bootid_darwin.go new file mode 100644 index 000000000..68f3edbf6 --- /dev/null +++ b/go/internal/stack/bootid_darwin.go @@ -0,0 +1,19 @@ +//go:build darwin + +package stack + +import ( + "fmt" + + "golang.org/x/sys/unix" +) + +// readCurrentBootID reads kern.bootsessionuuid, a read-only UUID minted per boot. +// Unlike kern.boottime it does not move when the wall clock steps. +func readCurrentBootID() (string, error) { + id, err := unix.Sysctl("kern.bootsessionuuid") + if err != nil { + return "", fmt.Errorf("sysctl kern.bootsessionuuid: %w", err) + } + return id, nil +} diff --git a/go/internal/stack/bootid_linux.go b/go/internal/stack/bootid_linux.go new file mode 100644 index 000000000..3d159db53 --- /dev/null +++ b/go/internal/stack/bootid_linux.go @@ -0,0 +1,18 @@ +//go:build linux + +package stack + +import ( + "fmt" + "os" + "strings" +) + +// readCurrentBootID reads the kernel's per-boot UUID, which changes on every boot. +func readCurrentBootID() (string, error) { + data, err := os.ReadFile("/proc/sys/kernel/random/boot_id") + if err != nil { + return "", fmt.Errorf("read boot id: %w", err) + } + return strings.TrimSpace(string(data)), nil +} diff --git a/go/internal/stack/bootid_other.go b/go/internal/stack/bootid_other.go new file mode 100644 index 000000000..2611cca16 --- /dev/null +++ b/go/internal/stack/bootid_other.go @@ -0,0 +1,13 @@ +//go:build unix && !linux && !darwin + +package stack + +import ( + "fmt" + "runtime" +) + +// readCurrentBootID has no reader here; callers treat the error as an unknown boot. +func readCurrentBootID() (string, error) { + return "", fmt.Errorf("reading the boot identity is not implemented on %s", runtime.GOOS) +} diff --git a/go/internal/stack/config_test.go b/go/internal/stack/config_test.go index 1f13cc3dd..0ccf3024f 100644 --- a/go/internal/stack/config_test.go +++ b/go/internal/stack/config_test.go @@ -3,6 +3,7 @@ package stack import ( + "path/filepath" "strings" "testing" ) @@ -184,3 +185,12 @@ func TestConfigValidateBudgetError(t *testing.T) { t.Fatalf("computed budget %d is implausible (sunPathMax=%d, tail=%d)", budget, sunPathMax, agentSocketTailWidth) } } + +// The microVM run root defaults to RuntimeDir, so the agent-socket budget Validate +// enforces must also fit the runner's worst-case microVM gateway socket path. +func TestAgentSocketBudgetCoversMicroVMRunRoot(t *testing.T) { + gatewayTail := len(filepath.Join("microvm", strings.Repeat("0", 32), "vsock.sock_1025")) + 1 + if gatewayTail > agentSocketTailWidth { + t.Fatalf("microVM gateway tail %d exceeds agent socket tail %d; Validate must budget it separately", gatewayTail, agentSocketTailWidth) + } +} diff --git a/go/internal/stack/deps.go b/go/internal/stack/deps.go index aaf907605..545e627a1 100644 --- a/go/internal/stack/deps.go +++ b/go/internal/stack/deps.go @@ -163,13 +163,14 @@ func (c Component) String() string { } } -// Process is a handle to a started child. Signal requests a graceful stop; Wait -// blocks until the child exits (or ctx is done) and returns its exit error, if +// Process is a handle to a started child. Signal requests a graceful stop, +// bounded by ctx; Wait blocks until the child exits (or ctx is done) and +// returns its exit error, if // any. Pid reports the child's PID, which doubles as its process-group ID (the // adapter sets Setpgid at Start), so the supervisor can persist the pgid for a // cross-process teardown that no longer holds this in-memory handle. type Process interface { - Signal(sig ProcessSignal) error + Signal(ctx context.Context, sig ProcessSignal) error Wait(ctx context.Context) error Pid() int } @@ -195,20 +196,29 @@ type ProcessSupervisor interface { Start(ctx context.Context, spec ProcessSpec) (Process, error) } -// GroupSignaller signals and identity-checks a persisted child process group by -// its process-group id. It is the cross-process teardown primitive: DownDetached -// reads pgids from the state-dir record and drives them here, since the tearing -// process holds no Process handle for a stack a prior up spawned. -// -// Signal delivers sig to the whole group (the real adapter targets the negative -// pgid, matching the in-process escalation's syscall.Kill(-pid, ...)). Alive -// reports whether a group with this pgid exists AND its leader's current start -// time matches startTime — the identity gate that turns "a group with this pgid -// exists" (which a recycled pid passes falsely) into "the ORIGINAL group is -// still alive". A gone group (ESRCH) or a start-time mismatch reports not-alive. +// GroupLiveness classifies a recorded process group against its leader's +// recorded start time. Linux never reuses a pid as a pgid while any member of +// that group lives, so a present group with an unreadable leader is still ours. +type GroupLiveness int + +const ( + // GroupGone means no process is left in the group. + GroupGone GroupLiveness = iota + // GroupOwned means the group exists and its leader's start time matches. + GroupOwned + // GroupOrphaned means the group exists but its leader is gone or unreadable. + GroupOrphaned + // GroupRecycled means the pid is someone else's: a different leader start time, or another uid's group. + GroupRecycled +) + +// GroupSignaller signals and classifies a persisted child process group by its +// pgid. It is the cross-process teardown primitive: DownDetached reads pgids +// from the state-dir record, since it holds no Process handle for them. Callers +// signal only an owned or orphaned group, never a recycled one. type GroupSignaller interface { Signal(pgid int, sig ProcessSignal) error - Alive(pgid int, startTime uint64) bool + Liveness(pgid int, startTime uint64) GroupLiveness } // ContainerController tears down a container child by its stable name for the @@ -220,14 +230,20 @@ type GroupSignaller interface { // // Exists reports whether a container with this name is present (the real adapter // runs `podman container exists `) — the liveness channel, the container -// analogue of GroupSignaller.Alive; a container needs no start-time identity -// token because its name is unique per state dir (S4). Stop requests a graceful -// stop bounded by timeout (`podman stop -t `); Remove is the +// analogue of GroupSignaller.Liveness; a container needs no start-time identity +// token because its name is unique per state dir (S4). Stop delivers the graceful +// stop signal without waiting for the container to exit (`podman kill --signal +// `), so the caller's drain wait measures the budget; Remove is the // SIGKILL-tier escalation that force-removes it (`podman rm -f `). +// RemoveExited removes the container only once it has exited (`podman rm` without +// --force); a still-running container is left alone and is not an error. It lets +// a container run without --rm (the gateway) be confirmed gone by absence. Each +// call is bounded by ctx. type ContainerController interface { - Exists(name string) bool - Stop(name string, timeout time.Duration) error - Remove(name string) error + Exists(ctx context.Context, name string) bool + Stop(ctx context.Context, name string) error + RemoveExited(ctx context.Context, name string) error + Remove(ctx context.Context, name string) error } // PostgresContainer starts the container-backed postgres child (S4): the diff --git a/go/internal/stack/downdetached.go b/go/internal/stack/downdetached.go index 92b563e45..df9b6a90d 100644 --- a/go/internal/stack/downdetached.go +++ b/go/internal/stack/downdetached.go @@ -29,15 +29,15 @@ var ( runnerDrainBudget = 15 * time.Second serverDrainBudget = 30 * time.Second postgresDrainBudget = 10 * time.Second - // collectorDrainBudget bounds the collector container's graceful `podman - // stop` before the `podman rm -f` escalation. The collector holds no on-disk + // collectorDrainBudget bounds the collector container's drain after its stop + // signal, before the `podman rm -f` escalation. The collector holds no on-disk // state to drain (D3 drops rather than buffering), so it stops fast; the // budget matches postgres's container-drain tier for parity. collectorDrainBudget = 10 * time.Second - // natsDrainBudget bounds the nats container's graceful `podman stop` before the - // `podman rm -f` escalation. NATS flushes its JetStream store on SIGTERM, so it - // gets the wider budget its natsStopTimeout also reserves — a `rm -f` mid-flush - // is the unclean-shutdown case the store recovers from on next boot. + // natsDrainBudget bounds the nats container's drain after its stop signal, + // before the `podman rm -f` escalation. NATS flushes its JetStream store on + // SIGTERM, so it gets the wider budget its natsStopTimeout also reserves — a + // `rm -f` mid-flush is the unclean-shutdown case the store recovers from on next boot. natsDrainBudget = 20 * time.Second // gatewayDrainBudget matches gatewayStopTimeout: in-flight model calls drain on SIGTERM. gatewayDrainBudget = 25 * time.Second @@ -49,6 +49,9 @@ var ( // downPollInterval paces the confirmation polls. Real wall-time; a test // shrinks it so the suite does not pay a full interval per poll. downPollInterval = 100 * time.Millisecond + // infraStopOnCancel bounds the detached stop sent to the infra tier when the + // caller's ctx is cancelled before that tier is reached. + infraStopOnCancel = 3 * time.Second ) // ErrStackStarting is returned by DownDetached when a live up holds the state-dir @@ -66,11 +69,9 @@ var ErrNoTeardownRecord = errors.New("a stack is live but this build holds no te // cross-process teardown this process holds no in-memory child handles for. It // reads the persisted pgid record, identity-checks each recorded group, SIGTERMs // the live ones in reverse start order with bounded SIGKILL escalation, and -// confirms teardown per component by the channel each has (server/postgres by -// socket quiescence, the socketless runner by group-ESRCH). Only pgids read from -// this stack's own state-dir file are ever signaled, and each group's identity -// (pgid + leader start-time token) is re-verified immediately before every -// signal. +// confirms process-backed server/postgres entries by socket quiescence AND group +// exit, containers by their own liveness probe, and the runner by group-ESRCH. +// Only recorded pgids are signaled; each identity is re-verified before signal. func DownDetached(ctx context.Context, cfg Config, deps Deps) error { if err := cfg.Validate(); err != nil { return err @@ -102,11 +103,12 @@ func DownDetached(ctx context.Context, cfg Config, deps Deps) error { // 3. Build the live teardown targets in reverse start order, identity-checking // each recorded group. A gone (ESRCH) or recycled (start-time mismatch) group - // is skipped — never signaled, never an error. + // is skipped — never signaled, never an error. A prior-boot record had its + // process entries dropped in consumeRecord. targets := liveTargets(ctx, cfg, deps, rec) - // 4/5/6. SIGTERM every live target up front (reverse order), then per-target - // bounded wait → SIGKILL escalation → per-component confirmation. + // 4/5/6. Per tier (consumers, then infra): SIGTERM every live target, then + // per-target bounded wait → SIGKILL escalation → per-component confirmation. survivors := drainTargets(ctx, deps, targets) // 7. Removal / partial-failure policy. @@ -151,6 +153,10 @@ func consumeRecord(ctx context.Context, cfg Config, deps Deps) (rec pgidRecord, } return pgidRecord{}, false, fmt.Errorf("read pgid record: %w", err) } + rec, err = dropPriorBootGroups(ctx, cfg, deps, rec) + if err != nil { + return pgidRecord{}, false, err + } // Consume: remove the record under the guard so a concurrent down cannot also // act on it. A partial teardown re-publishes the survivor set at the end. @@ -160,18 +166,49 @@ func consumeRecord(ctx context.Context, cfg Config, deps Deps) (rec pgidRecord, return rec, true, nil } -// target is one live child to tear down: its recorded identity plus the -// component-specific confirmation channel and drain budget. +// dropPriorBootGroups removes every process entry when rec was written in an +// earlier boot: a reboot frees every pgid, so a match now would be a stranger. +// An unknown boot on either side (older record, unreadable id) keeps rec as is. +// Container entries stay, since a name is not recycled by a reboot. +// A live server socket contradicts the mismatch, so down refuses and keeps rec. +func dropPriorBootGroups(ctx context.Context, cfg Config, deps Deps, rec pgidRecord) (pgidRecord, error) { + if rec.BootID == "" { + return rec, nil + } + current, err := readBootID() + if err != nil { + // An unreadable current boot is unknown; per-entry identity checks still guard. + slog.Warn("cannot read the current boot id; keeping per-entry identity checks", "err", err) + return rec, nil + } + if current == "" || current == rec.BootID { + return rec, nil + } + if _, perr := deps.Prober.Probe(ctx, cfg.SocketPath); perr == nil { + return pgidRecord{}, fmt.Errorf("pgid record claims boot %s but this is boot %s and the stack socket still answers; refusing to tear down", rec.BootID, current) + } + slog.Info("pgid record is from an earlier boot; its process groups are gone", "record_boot", rec.BootID, "current_boot", current) + out := pgidRecord{WriterPid: rec.WriterPid, Version: rec.Version, BootID: rec.BootID} + for _, e := range rec.Entries { + if e.Kind == entryContainer { + out.Entries = append(out.Entries, e) + } + } + return out, nil +} + +// target is one live child to tear down: its recorded identity, confirmation +// channels, and drain budget. socketDark separates socket state from group liveness. type target struct { - entry pgidEntry - budget time.Duration - confirm func() bool // reports the component confirmed dead by its own channel + entry pgidEntry + budget time.Duration + confirm func() bool // reports the component confirmed dead by its own channel + socketDark func() bool } -// liveTargets returns the identity-matched live groups in reverse start order -// (runner → server → gateway → nats → collector → postgres). Each recorded group is checked with -// GroupSignaller.Alive (existence AND start-time identity); a gone or recycled -// group is omitted — never signaled. +// liveTargets returns the recorded children still ours to tear down, in reverse +// start order (runner → server → gateway → nats → collector → postgres). A gone +// or recycled group, or an absent container, is omitted — never signaled. func liveTargets(ctx context.Context, cfg Config, deps Deps, rec pgidRecord) []target { // Index entries by component so we can emit them in reverse start order // regardless of the file's line order (which is start order by construction). @@ -181,39 +218,71 @@ func liveTargets(ctx context.Context, cfg Config, deps Deps, rec pgidRecord) []t } order := []struct { - comp Component - budget time.Duration - confirm func(pgidEntry) func() bool + comp Component + budget time.Duration + confirm func(pgidEntry) func() bool + socketDark func(pgidEntry) func() bool }{ - {ComponentRunner, runnerDrainBudget, func(e pgidEntry) func() bool { - // Socketless: confirmed only by its group going ESRCH (identity check - // false = gone or recycled-away). - return func() bool { return !deps.GroupSignaller.Alive(e.Pgid, e.StartTime) } + {comp: ComponentRunner, budget: runnerDrainBudget, confirm: func(e pgidEntry) func() bool { + if e.Kind == entryContainer { + return func() bool { return !deps.Containers.Exists(ctx, e.ContainerName) } + } + // Socketless process groups are confirmed only by the group leaving. + return func() bool { return groupReleased(deps, e) } }}, - {ComponentServer, serverDrainBudget, func(pgidEntry) func() bool { - // Socket quiescence: the UDS stops answering GetServerInfo. - return func() bool { _, err := deps.Prober.Probe(ctx, cfg.SocketPath); return err != nil } + {comp: ComponentServer, budget: serverDrainBudget, confirm: func(e pgidEntry) func() bool { + if e.Kind == entryContainer { + return func() bool { return !deps.Containers.Exists(ctx, e.ContainerName) } + } + // A dark UDS can precede process exit during graceful shutdown. + return func() bool { + if _, err := deps.Prober.Probe(ctx, cfg.SocketPath); err == nil { + return false + } + return groupReleased(deps, e) + } + }, socketDark: func(e pgidEntry) func() bool { + return func() bool { + _, err := deps.Prober.Probe(ctx, cfg.SocketPath) + return err != nil + } }}, - {ComponentGateway, gatewayDrainBudget, func(e pgidEntry) func() bool { - // Container existence; signalTerm also removes it, since it runs without --rm. - return func() bool { return !deps.Containers.Exists(e.ContainerName) } + {comp: ComponentGateway, budget: gatewayDrainBudget, confirm: func(e pgidEntry) func() bool { + // It runs without --rm, so an exited gateway lingers; remove it once exited. + return func() bool { + if err := deps.Containers.RemoveExited(ctx, e.ContainerName); err != nil { + logContainerSignalMiss("rm", e, err) + } + return !deps.Containers.Exists(ctx, e.ContainerName) + } }}, - {ComponentNats, natsDrainBudget, func(e pgidEntry) func() bool { + {comp: ComponentNats, budget: natsDrainBudget, confirm: func(e pgidEntry) func() bool { // Container existence: nats is a container child torn down by name, // confirmed gone when `podman container exists` reports absent. Reverse // start order places it after the server and runner (its consumers) so no // live consumer outlives the broker it publishes to. - return func() bool { return !deps.Containers.Exists(e.ContainerName) } + return func() bool { return !deps.Containers.Exists(ctx, e.ContainerName) } }}, - {ComponentCollector, collectorDrainBudget, func(e pgidEntry) func() bool { + {comp: ComponentCollector, budget: collectorDrainBudget, confirm: func(e pgidEntry) func() bool { // Container existence: the collector is a container child torn down by // name, confirmed gone when `podman container exists` reports absent. // Reverse start order places it after the server (which emits to it) and // before postgres. - return func() bool { return !deps.Containers.Exists(e.ContainerName) } + return func() bool { return !deps.Containers.Exists(ctx, e.ContainerName) } }}, - {ComponentPostgres, postgresDrainBudget, func(pgidEntry) func() bool { - // Socket quiescence: postgres stops accepting on the DSN socket. + {comp: ComponentPostgres, budget: postgresDrainBudget, confirm: func(e pgidEntry) func() bool { + if e.Kind == entryContainer { + // The bind-mounted socket can go dark while the container lingers. + return func() bool { return !deps.Containers.Exists(ctx, e.ContainerName) } + } + // A dark DSN can precede process exit during postgres shutdown. + return func() bool { + if deps.DBProber.ProbeDB(ctx, cfg.DatabaseDSN) == nil { + return false + } + return groupReleased(deps, e) + } + }, socketDark: func(e pgidEntry) func() bool { return func() bool { return deps.DBProber.ProbeDB(ctx, cfg.DatabaseDSN) != nil } }}, } @@ -224,64 +293,85 @@ func liveTargets(ctx context.Context, cfg Config, deps Deps, rec pgidRecord) []t if !ok { continue // never recorded (half-spawned prefix) — nothing to tear down } - if !entryAlive(deps, e) { + if !entryAlive(ctx, deps, e) { continue // gone or recycled — skip, never signal } - targets = append(targets, target{entry: e, budget: o.budget, confirm: o.confirm(e)}) + target := target{entry: e, budget: o.budget, confirm: o.confirm(e)} + if o.socketDark != nil { + target.socketDark = o.socketDark(e) + } + targets = append(targets, target) } return targets } -// drainTargets SIGTERMs every live target (reverse order, already ordered by -// liveTargets), then per target waits the drain budget, escalates to a group -// SIGKILL, and confirms per component. It returns the components that were still -// alive at budget expiry after the SIGKILL — the survivor set for the -// partial-failure rewrite. +// consumerTier holds the components that publish to or query the infra tier; +// they drain first so no consumer outlives the broker or database it uses. +var consumerTier = map[Component]bool{ComponentRunner: true, ComponentServer: true, ComponentGateway: true} + +// drainTargets tears targets down in two tiers, consumers (runner, server, +// gateway) then infra (nats, collector, postgres), each in reverse start order. +// It returns the components still alive after their drain budget and hard kill — +// the survivor set for the partial-failure rewrite. func drainTargets(ctx context.Context, deps Deps, targets []target) []Component { - // Phase A: SIGTERM all live groups up front. Signaling the server also makes a - // surviving runner exit when its link drops, belt-and-suspenders alongside - // signaling the runner group. A delivery error is not the verdict — the confirm - // below is — so it is not fatal here (an ESRCH means the group already vanished). + var consumers, infra []target for _, t := range targets { - signalTerm(deps, t.entry, t.budget) + if consumerTier[t.entry.Component] { + consumers = append(consumers, t) + } else { + infra = append(infra, t) + } + } + survivors := drainTier(ctx, deps, consumers) + if ctx.Err() == nil { + return append(survivors, drainTier(ctx, deps, infra)...) } + // Cancelled mid-teardown: exec refuses a done ctx, so stop infra on a short + // detached one and record it all as survivors rather than wait. + sctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), infraStopOnCancel) + defer cancel() + for _, t := range infra { + signalTerm(sctx, deps, t.entry) + survivors = append(survivors, t.entry.Component) + } + return survivors +} - // Phase B: per-target confirm with bounded SIGKILL escalation. +// drainTier signals every target in the tier, then drains each one in turn. +// Signalling never waits for exit, so each drain budget is measured by waitDead. +// A delivery error is not the verdict — the confirm is — so it is not fatal here. +func drainTier(ctx context.Context, deps Deps, tier []target) []Component { + for _, t := range tier { + signalTerm(ctx, deps, t.entry) + } var survivors []Component - for _, t := range targets { - if drainOne(ctx, deps, t) { - continue + for _, t := range tier { + if !drainOne(ctx, deps, t) { + survivors = append(survivors, t.entry.Component) } - survivors = append(survivors, t.entry.Component) } return survivors } // drainOne waits for one target to confirm dead within its drain budget; on -// timeout it escalates to a group SIGKILL and re-confirms. It returns true when -// the component is torn down. +// timeout it escalates to SIGKILL and re-confirms. It returns true when the +// component is torn down. // -// The runner is group-ESRCH confirmed and has no socket: because SIGKILL is -// unblockable, a runner group still non-ESRCH after the group SIGKILL can only -// be zombies awaiting init's reap (a guaranteed-terminal state), so the runner -// is treated as torn down once SIGKILL is sent — the post-SIGKILL zombie window -// is success, not failure. The socket-confirmed components (server, postgres) do -// get a bounded post-SIGKILL confirm: their socket going dark is the proof, and -// a socket still answering past postKillGrace is a genuine survivor. +// Only a delivered group SIGKILL lets residual members count as zombies awaiting +// reap; otherwise the post-kill confirm decides. func drainOne(ctx context.Context, deps Deps, t target) bool { if waitDead(ctx, deps.now, t.budget, t.confirm) { return true // SIGTERM sufficed (or the group was already gone) } - // Escalate: hard-kill. For a process group this is a group SIGKILL; for a - // container it is `podman rm -f`. A delivery error is not the verdict (an - // ESRCH / already-gone means it died during the drain); the confirm decides. - signalKill(deps, t.entry) - - if t.entry.Component == ComponentRunner { - // Socketless + SIGKILL unblockable → any residual non-ESRCH group is a - // zombie awaiting reap, i.e. terminal. Treat as torn down. - return true + killed := signalKill(ctx, deps, t.entry) + if killed && ctx.Err() == nil { + if t.entry.Component == ComponentRunner { + return true + } + if t.socketDark != nil && t.socketDark() { + return true + } } return waitDead(ctx, deps.now, postKillGrace, t.confirm) } @@ -354,36 +444,49 @@ func logSignalMiss(sig string, e pgidEntry, err error) { "signal", sig, "component", e.Component.String(), "pgid", e.Pgid, "error", err) } -// entryAlive reports whether a recorded entry's target is still live, dispatched -// on kind: a process group by identity-checked pgid (existence AND leader -// start-time), a container by name via `podman container exists`. A gone target -// reports not-alive so it is skipped, never signaled. -func entryAlive(deps Deps, e pgidEntry) bool { +// entryAlive reports whether a recorded entry is still ours to signal: a +// container that exists, or a process group that is owned or orphaned. +func entryAlive(ctx context.Context, deps Deps, e pgidEntry) bool { switch e.Kind { case entryContainer: - return deps.Containers.Exists(e.ContainerName) + return deps.Containers.Exists(ctx, e.ContainerName) default: - return deps.GroupSignaller.Alive(e.Pgid, e.StartTime) + return groupOurs(deps, e) } } +// groupOurs reports whether a recorded group is still present and not a recycled +// pid. An orphaned group keeps our pgid, since Linux never reuses a live pgid. +func groupOurs(deps Deps, e pgidEntry) bool { + if e.Pgid <= 0 { + return false + } + l := deps.GroupSignaller.Liveness(e.Pgid, e.StartTime) + return l == GroupOwned || l == GroupOrphaned +} + +// groupReleased reports whether a recorded group no longer needs teardown: it +// is gone, or its pgid now names someone else's process. +func groupReleased(deps Deps, e pgidEntry) bool { + return !groupOurs(deps, e) +} + // signalTerm delivers the graceful-stop tier, dispatched on kind: a group -// SIGTERM for a process, `podman stop -t ` for a container (the budget -// is the container's own drain budget, deliberate parity with the process -// model's capped drain). A delivery error is not the teardown verdict — the -// per-component confirm channel is — so it is logged, never fatal. -func signalTerm(deps Deps, e pgidEntry, budget time.Duration) { +// SIGTERM for a process, the container's stop signal for a container. Neither +// waits for exit, so waitDead in drainTier measures every drain budget. A delivery +// error is not the teardown verdict — the per-component confirm channel is — so +// it is logged, never fatal. +func signalTerm(ctx context.Context, deps Deps, e pgidEntry) { switch e.Kind { case entryContainer: - if err := deps.Containers.Stop(e.ContainerName, budget); err != nil { + if err := deps.Containers.Stop(ctx, e.ContainerName); err != nil { logContainerSignalMiss("stop", e, err) } - if e.Component == ComponentGateway { - if err := deps.Containers.Remove(e.ContainerName); err != nil { - logContainerSignalMiss("remove", e, err) - } - } default: + // Re-check identity: the pgid may have been recycled since selection. + if !groupOurs(deps, e) { + return + } if err := deps.GroupSignaller.Signal(e.Pgid, SignalTerm); err != nil { logSignalMiss("SIGTERM", e, err) } @@ -391,18 +494,25 @@ func signalTerm(deps Deps, e pgidEntry, budget time.Duration) { } // signalKill delivers the hard-kill tier, dispatched on kind: a group SIGKILL -// for a process, `podman rm -f` for a container. A delivery error is not the -// teardown verdict — the per-component confirm decides — so it is logged. -func signalKill(deps Deps, e pgidEntry) { +// for a process, `podman rm -f` for a container. It reports true only when a +// process-group SIGKILL was delivered, the precondition for the zombie shortcut. +func signalKill(ctx context.Context, deps Deps, e pgidEntry) bool { switch e.Kind { case entryContainer: - if err := deps.Containers.Remove(e.ContainerName); err != nil { + if err := deps.Containers.Remove(ctx, e.ContainerName); err != nil { logContainerSignalMiss("rm -f", e, err) } + return false default: + // Re-check identity: the drain budget is a long window for pid reuse. + if !groupOurs(deps, e) { + return false + } if err := deps.GroupSignaller.Signal(e.Pgid, SignalKill); err != nil { logSignalMiss("SIGKILL", e, err) + return false } + return true } } diff --git a/go/internal/stack/downdetached_test.go b/go/internal/stack/downdetached_test.go index 61e3d0e18..02d8265ce 100644 --- a/go/internal/stack/downdetached_test.go +++ b/go/internal/stack/downdetached_test.go @@ -9,6 +9,8 @@ import ( "path/filepath" "reflect" "strconv" + "strings" + "sync" "testing" "time" ) @@ -24,23 +26,44 @@ const ( func pgToken(pgid int) uint64 { return uint64(pgid) * 10 } -// downTestDeps returns deps with shrunk drain budgets and a fast poll so the -// bounded escalation and confirmation loops run without real waiting, plus a -// controllable now-clock. It also points the probers at the group-signaller's -// liveness so "socket dark" tracks "group gone" — one source of truth for the -// server/postgres confirmation channel. +// downTestDeps returns deps with shrunk drain budgets. Existing tests retain +// group-backed probes; regression tests install independent socket fakes. func downTestDeps(t *testing.T, h *harness) Deps { t.Helper() shrinkBudgets(t) deps := h.deps - // Server/postgres confirm by socket quiescence; model the socket as answering - // iff the group is still alive (identity-matched). The prober/dbprober here - // override the harness stubs for the down path. deps.Prober = &groupBackedProber{gs: h.groupSig, pgid: serverPgid, token: pgToken(serverPgid)} deps.DBProber = &groupBackedDBProber{gs: h.groupSig, pgid: pgPgid, token: pgToken(pgPgid)} return deps } +// groupBackedProber answers GetServerInfo iff the server group is alive. +type groupBackedProber struct { + gs *fakeGroupSignaller + pgid int + token uint64 +} + +func (p *groupBackedProber) Probe(context.Context, string) (ServerInfo, error) { + if p.gs.ours(p.pgid, p.token) { + return ServerInfo{Version: testVersion}, nil + } + return ServerInfo{}, errNotAnswering +} + +type groupBackedDBProber struct { + gs *fakeGroupSignaller + pgid int + token uint64 +} + +func (p *groupBackedDBProber) ProbeDB(context.Context, string) error { + if p.gs.ours(p.pgid, p.token) { + return nil + } + return errPostgresNotReady +} + // shrinkBudgets shrinks the package drain budgets/poll for the duration of a // test, restoring them after. Small but nonzero so the deadline math is real. func shrinkBudgets(t *testing.T) { @@ -59,34 +82,70 @@ func shrinkBudgets(t *testing.T) { }) } -// groupBackedProber answers GetServerInfo iff the server group is alive; a dead -// group means the socket is dark. -type groupBackedProber struct { - gs *fakeGroupSignaller - pgid int - token uint64 -} +type fixedServerProber bool -func (p *groupBackedProber) Probe(ctx context.Context, socketPath string) (ServerInfo, error) { - if p.gs.Alive(p.pgid, p.token) { +func (p fixedServerProber) Probe(context.Context, string) (ServerInfo, error) { + if p { return ServerInfo{Version: testVersion}, nil } return ServerInfo{}, errNotAnswering } -type groupBackedDBProber struct { - gs *fakeGroupSignaller - pgid int - token uint64 -} +type fixedDBProber bool -func (p *groupBackedDBProber) ProbeDB(ctx context.Context, dsn string) error { - if p.gs.Alive(p.pgid, p.token) { +func (p fixedDBProber) ProbeDB(context.Context, string) error { + if p { return nil } return errPostgresNotReady } +// darkSocketDownDeps decouples socket state from process-group liveness. +func darkSocketDownDeps(t *testing.T, h *harness) Deps { + t.Helper() + deps := downTestDeps(t, h) + deps.Prober = fixedServerProber(false) + deps.DBProber = fixedDBProber(false) + return deps +} + +// stepClock advances a fixed step on every read, so drain budgets elapse on +// poll count rather than wall time. +type stepClock struct { + mu sync.Mutex + t time.Time + step time.Duration +} + +func newStepClock(step time.Duration) *stepClock { + return &stepClock{t: time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), step: step} +} + +func (c *stepClock) Now() time.Time { + c.mu.Lock() + defer c.mu.Unlock() + t := c.t + c.t = c.t.Add(c.step) + return t +} + +func (c *stepClock) peek() time.Time { + c.mu.Lock() + defer c.mu.Unlock() + return c.t +} + +// assertKillAfterBudget fails unless pgid's SIGKILL came a full budget after its SIGTERM. +func assertKillAfterBudget(t *testing.T, termAt, killAt time.Time, budget time.Duration) { + t.Helper() + if termAt.IsZero() || killAt.IsZero() { + t.Fatalf("SIGTERM at %v, SIGKILL at %v; want both delivered", termAt, killAt) + } + if gap := killAt.Sub(termAt); gap < budget { + t.Fatalf("SIGKILL %v after SIGTERM, want at least the %v drain budget", gap, budget) + } +} + // seedFullRecord writes a complete three-child pgid record and marks all three // groups live in the fake signaller, matching the identity tokens. func seedFullRecord(t *testing.T, cfg Config, h *harness) { @@ -160,7 +219,7 @@ func TestDownDetachedEscalatesToSIGKILL(t *testing.T) { deps := downTestDeps(t, h) // runner + postgres die on SIGTERM; the server ignores SIGTERM and only dies - // on SIGKILL — exercising escalation for a socket-confirmed component. + // on SIGKILL — exercising group-aware escalation. h.groupSig.onTerm[pgPgid] = func() { h.groupSig.set(pgPgid, pgToken(pgPgid), false) } h.groupSig.onTerm[runnerPgid] = func() { h.groupSig.set(runnerPgid, pgToken(runnerPgid), false) } h.groupSig.onKill[serverPgid] = func() { h.groupSig.set(serverPgid, pgToken(serverPgid), false) } @@ -181,6 +240,270 @@ func TestDownDetachedEscalatesToSIGKILL(t *testing.T) { assertPgidFileGone(t, cfg.StateDir) } +// TestDownDetachedServerSocketDarkWaitsForGroupExit proves a dark server socket +// does not confirm a live group: SIGKILL waits out the full drain budget. +func TestDownDetachedServerSocketDarkWaitsForGroupExit(t *testing.T) { + cfg, h := newHarness(t) + seedFullRecord(t, cfg, h) + deps := darkSocketDownDeps(t, h) + clock := newStepClock(time.Millisecond) + deps.Now = clock.Now + var termAt, killAt time.Time + h.groupSig.onTerm[pgPgid] = func() { h.groupSig.set(pgPgid, pgToken(pgPgid), false) } + h.groupSig.onTerm[runnerPgid] = func() { h.groupSig.set(runnerPgid, pgToken(runnerPgid), false) } + h.groupSig.onTerm[serverPgid] = func() { termAt = clock.peek() } + h.groupSig.onKill[serverPgid] = func() { + killAt = clock.peek() + h.groupSig.set(serverPgid, pgToken(serverPgid), false) + } + + if err := DownDetached(context.Background(), cfg, deps); err != nil { + t.Fatalf("DownDetached = %v, want nil after the server group exits", err) + } + if h.groupSig.ours(serverPgid, pgToken(serverPgid)) { + t.Fatal("server group is still alive after DownDetached returned nil") + } + events := signalEvents(h.rec.snapshot()) + if countEvent(events, "group-kill "+strconv.Itoa(serverPgid)) != 1 { + t.Fatalf("server SIGKILL count = %d, want 1: %v", countEvent(events, "group-kill "+strconv.Itoa(serverPgid)), events) + } + assertKillAfterBudget(t, termAt, killAt, serverDrainBudget) + assertPgidFileGone(t, cfg.StateDir) +} + +// TestDownDetachedPostgresSocketDarkWaitsForGroupExit is the postgres analogue: +// a dark DSN does not confirm a live group before the drain budget elapses. +func TestDownDetachedPostgresSocketDarkWaitsForGroupExit(t *testing.T) { + cfg, h := newHarness(t) + seedFullRecord(t, cfg, h) + deps := darkSocketDownDeps(t, h) + clock := newStepClock(time.Millisecond) + deps.Now = clock.Now + var termAt, killAt time.Time + h.groupSig.onTerm[serverPgid] = func() { h.groupSig.set(serverPgid, pgToken(serverPgid), false) } + h.groupSig.onTerm[runnerPgid] = func() { h.groupSig.set(runnerPgid, pgToken(runnerPgid), false) } + h.groupSig.onTerm[pgPgid] = func() { termAt = clock.peek() } + h.groupSig.onKill[pgPgid] = func() { + killAt = clock.peek() + h.groupSig.set(pgPgid, pgToken(pgPgid), false) + } + + if err := DownDetached(context.Background(), cfg, deps); err != nil { + t.Fatalf("DownDetached = %v, want nil after the postgres group exits", err) + } + if h.groupSig.ours(pgPgid, pgToken(pgPgid)) { + t.Fatal("postgres group is still alive after DownDetached returned nil") + } + events := signalEvents(h.rec.snapshot()) + if countEvent(events, "group-kill "+strconv.Itoa(pgPgid)) != 1 { + t.Fatalf("postgres SIGKILL count = %d, want 1: %v", countEvent(events, "group-kill "+strconv.Itoa(pgPgid)), events) + } + assertKillAfterBudget(t, termAt, killAt, postgresDrainBudget) + assertPgidFileGone(t, cfg.StateDir) +} + +// TestDownDetachedOrphanedGroupEscalates proves a group whose leader is gone but +// whose members remain is still ours: it is signalled, killed, and reported. +func TestDownDetachedOrphanedGroupEscalates(t *testing.T) { + cfg, h := newHarness(t) + seedFullRecord(t, cfg, h) + deps := darkSocketDownDeps(t, h) + h.groupSig.onTerm[pgPgid] = func() { h.groupSig.set(pgPgid, pgToken(pgPgid), false) } + h.groupSig.onTerm[serverPgid] = func() { h.groupSig.set(serverPgid, pgToken(serverPgid), false) } + // The runner leader exited; a forked member keeps the group alive through SIGKILL. + h.groupSig.setLeaderUnknown(runnerPgid) + h.groupSig.failSignal(runnerPgid, SignalKill, errors.New("operation not permitted")) + + err := DownDetached(context.Background(), cfg, deps) + if err == nil || !strings.Contains(err.Error(), "runner") { + t.Fatalf("DownDetached = %v, want a survivor error naming the orphaned runner group", err) + } + events := signalEvents(h.rec.snapshot()) + for _, want := range []string{"group-term " + strconv.Itoa(runnerPgid), "group-kill " + strconv.Itoa(runnerPgid)} { + if countEvent(events, want) != 1 { + t.Fatalf("orphaned group should get exactly one %q: %v", want, events) + } + } + rec, readErr := readPgidFile(cfg.StateDir) + if readErr != nil || len(rec.Entries) != 1 || rec.Entries[0].Component != ComponentRunner { + t.Fatalf("survivor record = %+v, err = %v; want only the runner", rec, readErr) + } +} + +// TestDownDetachedOrphanedServerGroupIsAwaited proves an orphaned server group is +// awaited until it is gone, never confirmed while its members remain. +func TestDownDetachedOrphanedServerGroupIsAwaited(t *testing.T) { + cfg, h := newHarness(t) + seedFullRecord(t, cfg, h) + deps := darkSocketDownDeps(t, h) + h.groupSig.onTerm[pgPgid] = func() { h.groupSig.set(pgPgid, pgToken(pgPgid), false) } + h.groupSig.onTerm[runnerPgid] = func() { h.groupSig.set(runnerPgid, pgToken(runnerPgid), false) } + h.groupSig.setLeaderUnknown(serverPgid) + h.groupSig.onKill[serverPgid] = func() { h.groupSig.set(serverPgid, pgToken(serverPgid), false) } + + if err := DownDetached(context.Background(), cfg, deps); err != nil { + t.Fatalf("DownDetached = %v, want nil once the orphaned group exits", err) + } + events := signalEvents(h.rec.snapshot()) + for _, want := range []string{"group-term " + strconv.Itoa(serverPgid), "group-kill " + strconv.Itoa(serverPgid)} { + if countEvent(events, want) != 1 { + t.Fatalf("orphaned server group should get exactly one %q: %v", want, events) + } + } + assertPgidFileGone(t, cfg.StateDir) +} + +// TestDownDetachedRecycledBeforeSIGKILLIsNotSignalled proves identity is re-read +// right before SIGKILL: a pid recycled as the drain budget expires is never killed. +func TestDownDetachedRecycledBeforeSIGKILLIsNotSignalled(t *testing.T) { + cfg, h := newHarness(t) + seedFullRecord(t, cfg, h) + deps := darkSocketDownDeps(t, h) + clock := newStepClock(time.Millisecond) + var termAt time.Time + // The recycle lands on the clock read that expires the runner budget, after + // its last confirm poll saw the group still owned. + deps.Now = func() time.Time { + now := clock.Now() + if !termAt.IsZero() && !now.Before(termAt.Add(runnerDrainBudget)) { + h.groupSig.set(runnerPgid, 777, true) + } + return now + } + h.groupSig.onTerm[pgPgid] = func() { h.groupSig.set(pgPgid, pgToken(pgPgid), false) } + h.groupSig.onTerm[serverPgid] = func() { h.groupSig.set(serverPgid, pgToken(serverPgid), false) } + h.groupSig.onTerm[runnerPgid] = func() { termAt = clock.peek() } + + if err := DownDetached(context.Background(), cfg, deps); err != nil { + t.Fatalf("DownDetached = %v, want nil for a recycled group", err) + } + if n := countEvent(signalEvents(h.rec.snapshot()), "group-kill "+strconv.Itoa(runnerPgid)); n != 0 { + t.Fatalf("recycled runner pgid SIGKILLed %d times, want 0", n) + } + assertPgidFileGone(t, cfg.StateDir) +} + +// TestDownDetachedRecycledBeforeSIGTERMIsNotSignalled proves identity is re-read +// right before SIGTERM, after target selection. +func TestDownDetachedRecycledBeforeSIGTERMIsNotSignalled(t *testing.T) { + cfg, h := newHarness(t) + seedFullRecord(t, cfg, h) + deps := darkSocketDownDeps(t, h) + h.groupSig.onTerm[pgPgid] = func() { h.groupSig.set(pgPgid, pgToken(pgPgid), false) } + // The runner SIGTERM lands just as the server pgid is recycled. + h.groupSig.onTerm[runnerPgid] = func() { + h.groupSig.set(runnerPgid, pgToken(runnerPgid), false) + h.groupSig.set(serverPgid, 888, true) + } + + if err := DownDetached(context.Background(), cfg, deps); err != nil { + t.Fatalf("DownDetached = %v, want nil for a recycled group", err) + } + events := signalEvents(h.rec.snapshot()) + for _, forbidden := range []string{"group-term " + strconv.Itoa(serverPgid), "group-kill " + strconv.Itoa(serverPgid)} { + if countEvent(events, forbidden) != 0 { + t.Fatalf("recycled server pgid was signalled (%q): %v", forbidden, events) + } + } + assertPgidFileGone(t, cfg.StateDir) +} + +// TestDownDetachedFailedSIGKILLDoesNotShortCircuit proves an undelivered group +// SIGKILL never takes the zombie shortcut: the live group is a survivor. +func TestDownDetachedFailedSIGKILLDoesNotShortCircuit(t *testing.T) { + for _, tc := range []struct { + name string + comp Component + pgid int + }{ + {"runner", ComponentRunner, runnerPgid}, + {"server", ComponentServer, serverPgid}, + {"postgres", ComponentPostgres, pgPgid}, + } { + t.Run(tc.name, func(t *testing.T) { + cfg, h := newHarness(t) + seedFullRecord(t, cfg, h) + deps := darkSocketDownDeps(t, h) + for _, pgid := range []int{pgPgid, serverPgid, runnerPgid} { + if pgid != tc.pgid { + h.groupSig.onTerm[pgid] = func() { h.groupSig.set(pgid, pgToken(pgid), false) } + } + } + h.groupSig.failSignal(tc.pgid, SignalKill, errors.New("operation not permitted")) + + err := DownDetached(context.Background(), cfg, deps) + if err == nil || !strings.Contains(err.Error(), tc.comp.String()) { + t.Fatalf("DownDetached = %v, want a survivor error naming %s", err, tc.comp) + } + rec, readErr := readPgidFile(cfg.StateDir) + if readErr != nil || len(rec.Entries) != 1 || rec.Entries[0].Component != tc.comp { + t.Fatalf("survivor record = %+v, err = %v; want only %s", rec, readErr, tc.comp) + } + }) + } +} + +// TestDownDetachedCanceledSIGKILLDoesNotShortCircuit proves a canceled teardown +// re-checks the group instead of assuming post-kill zombies. +func TestDownDetachedCanceledSIGKILLDoesNotShortCircuit(t *testing.T) { + cfg, h := newHarness(t) + seedFullRecord(t, cfg, h) + deps := darkSocketDownDeps(t, h) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + h.groupSig.onTerm[pgPgid] = func() { h.groupSig.set(pgPgid, pgToken(pgPgid), false) } + h.groupSig.onTerm[serverPgid] = func() { h.groupSig.set(serverPgid, pgToken(serverPgid), false) } + // The kill is delivered but the caller gave up; the group is still present. + h.groupSig.onKill[runnerPgid] = cancel + + err := DownDetached(ctx, cfg, deps) + if err == nil || !strings.Contains(err.Error(), "runner") { + t.Fatalf("DownDetached = %v, want a survivor error naming the runner", err) + } +} + +func TestDownDetachedGroupSurvivingSIGKILLIsReported(t *testing.T) { + cfg, h := newHarness(t) + seedFullRecord(t, cfg, h) + deps := darkSocketDownDeps(t, h) + deps.DBProber = fixedDBProber(true) + h.groupSig.onTerm[serverPgid] = func() { h.groupSig.set(serverPgid, pgToken(serverPgid), false) } + h.groupSig.onTerm[runnerPgid] = func() { h.groupSig.set(runnerPgid, pgToken(runnerPgid), false) } + + err := DownDetached(context.Background(), cfg, deps) + if err == nil || !strings.Contains(err.Error(), "postgres") || !strings.Contains(err.Error(), "teardown incomplete") { + t.Fatalf("DownDetached = %v, want survivor error naming postgres", err) + } + if !h.groupSig.ours(pgPgid, pgToken(pgPgid)) { + t.Fatal("fake postgres group did not survive SIGKILL") + } + if got := signalEvents(h.rec.snapshot()); countEvent(got, "group-kill "+strconv.Itoa(pgPgid)) != 1 { + t.Fatalf("postgres SIGKILL count = %d, want 1: %v", countEvent(got, "group-kill "+strconv.Itoa(pgPgid)), got) + } + rec, readErr := readPgidFile(cfg.StateDir) + if readErr != nil || len(rec.Entries) != 1 || rec.Entries[0].Component != ComponentPostgres { + t.Fatalf("survivor record = %+v, err = %v; want only postgres", rec, readErr) + } +} + +func TestDownDetachedSocketDarkZombieGroupIsSuccess(t *testing.T) { + cfg, h := newHarness(t) + seedFullRecord(t, cfg, h) + deps := darkSocketDownDeps(t, h) + // Leave the group visible after SIGKILL, modeling a zombie awaiting reap. + if err := DownDetached(context.Background(), cfg, deps); err != nil { + t.Fatalf("DownDetached = %v, want nil for a socket-dark zombie group", err) + } + events := signalEvents(h.rec.snapshot()) + if countEvent(events, "group-kill "+strconv.Itoa(serverPgid)) != 1 { + t.Fatalf("server SIGKILL count = %d, want 1: %v", countEvent(events, "group-kill "+strconv.Itoa(serverPgid)), events) + } + if !h.groupSig.ours(serverPgid, pgToken(serverPgid)) { + t.Fatal("fake server zombie group should remain visible after SIGKILL") + } + assertPgidFileGone(t, cfg.StateDir) +} + // TestDownDetachedRunnerZombieWindowIsSuccess proves the socketless runner is // confirmed torn down once SIGKILL is sent, even if its group is still non-ESRCH // (a zombie awaiting reap). Because SIGKILL is unblockable, a still-present group @@ -190,12 +513,9 @@ func TestDownDetachedRunnerZombieWindowIsSuccess(t *testing.T) { seedFullRecord(t, cfg, h) deps := downTestDeps(t, h) - // server + postgres drain on SIGTERM. + // server and postgres drain on SIGTERM. h.groupSig.onTerm[pgPgid] = func() { h.groupSig.set(pgPgid, pgToken(pgPgid), false) } h.groupSig.onTerm[serverPgid] = func() { h.groupSig.set(serverPgid, pgToken(serverPgid), false) } - // The runner NEVER goes ESRCH: it ignores SIGTERM and stays "alive" even after - // SIGKILL (the zombie window). DownDetached must still treat it as torn down. - // (no onTerm/onKill flip for runnerPgid → stays alive throughout) if err := DownDetached(context.Background(), cfg, deps); err != nil { t.Fatalf("DownDetached = %v, want nil (zombie window is success)", err) @@ -472,7 +792,7 @@ type containerBackedDBProber struct { } func (p *containerBackedDBProber) ProbeDB(ctx context.Context, dsn string) error { - if p.c.Exists(p.name) { + if p.c.Exists(ctx, p.name) { return nil // container up → socket answers → reachable } return errPostgresNotReady // container gone → socket dark → confirmed dead @@ -512,7 +832,7 @@ func TestDownDetachedContainerGracefulStop(t *testing.T) { // SIGTERM tears the two groups down; `podman stop` removes the container. h.groupSig.onTerm[serverPgid] = func() { h.groupSig.set(serverPgid, pgToken(serverPgid), false) } h.groupSig.onTerm[runnerPgid] = func() { h.groupSig.set(runnerPgid, pgToken(runnerPgid), false) } - h.containers.onStop[pgContainerName] = func() { h.containers.setExists(false) } + h.containers.onStop[pgContainerName] = func(context.Context) { h.containers.setExists(false) } if err := DownDetached(context.Background(), cfg, deps); err != nil { t.Fatalf("DownDetached = %v, want nil", err) @@ -607,6 +927,170 @@ func TestDownDetachedContainerGoneIsSkipped(t *testing.T) { assertPgidFileGone(t, cfg.StateDir) } +// TestDownDetachedContainerDarkDBStillExistsIsSurvivor proves a container +// postgres is confirmed only by its absence: a dark DB never confirms it. +func TestDownDetachedContainerDarkDBStillExistsIsSurvivor(t *testing.T) { + cfg, h := newHarness(t) + seedContainerRecord(t, cfg, h) + deps := containerDownDeps(t, h) + deps.DBProber = fixedDBProber(false) + h.groupSig.onTerm[serverPgid] = func() { h.groupSig.set(serverPgid, pgToken(serverPgid), false) } + h.groupSig.onTerm[runnerPgid] = func() { h.groupSig.set(runnerPgid, pgToken(runnerPgid), false) } + + err := DownDetached(context.Background(), cfg, deps) + if err == nil || !strings.Contains(err.Error(), "postgres") { + t.Fatalf("DownDetached = %v, want a survivor error naming the present postgres container", err) + } + want := []string{"ctr-stop " + pgContainerName, "ctr-rm " + pgContainerName} + if got := ctrEvents(h.rec.snapshot()); !reflect.DeepEqual(got, want) { + t.Fatalf("container escalation:\n got %v\n want %v", got, want) + } + rec, readErr := readPgidFile(cfg.StateDir) + if readErr != nil || len(rec.Entries) != 1 || rec.Entries[0].ContainerName != pgContainerName { + t.Fatalf("survivor record = %+v, err = %v; want only the postgres container", rec, readErr) + } +} + +// TestDownDetachedCancelledDuringSlowContainerStopRewritesSurvivors: a SIGTERM +// to down lands while a slow container stop is in flight. The stop must see the +// cancelled ctx and return, and down must still rewrite the survivor record. +func TestDownDetachedCancelledDuringSlowContainerStopRewritesSurvivors(t *testing.T) { + cfg, h := newHarness(t) + seedContainerRecord(t, cfg, h) + deps := containerDownDeps(t, h) + h.groupSig.onTerm[serverPgid] = func() { h.groupSig.set(serverPgid, pgToken(serverPgid), false) } + h.groupSig.onTerm[runnerPgid] = func() { h.groupSig.set(runnerPgid, pgToken(runnerPgid), false) } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + h.containers.onStop[pgContainerName] = func(stopCtx context.Context) { + cancel() // the down process is signalled mid-stop + if stopCtx.Err() == nil { + t.Error("container stop ran on a ctx the caller's cancellation cannot reach") + } + } + + err := DownDetached(ctx, cfg, deps) + if err == nil || !strings.Contains(err.Error(), "postgres") { + t.Fatalf("DownDetached = %v, want a survivor error naming postgres", err) + } + rec, rerr := readPgidFile(cfg.StateDir) + if rerr != nil { + t.Fatalf("survivor record read = %v, want the rewritten survivor set", rerr) + } + if len(rec.Entries) != 1 || rec.Entries[0].ContainerName != pgContainerName { + t.Fatalf("survivor record = %+v, want exactly the postgres container entry", rec.Entries) + } +} + +// TestDownDetachedTierSignalDoesNotWaitOnContainerDrain: signalling a tier never +// blocks on a container's drain. The gateway's stop is sent first in its tier, +// and the runner and server get their SIGTERM before any container is removed. +func TestDownDetachedTierSignalDoesNotWaitOnContainerDrain(t *testing.T) { + cfg, h := newHarness(t) + seedGatewayRecord(t, cfg, h) + deps := sidecarContainerDownDeps(t, h) + for _, p := range []int{pgPgid, serverPgid, runnerPgid} { + pgid := p + h.groupSig.onTerm[pgid] = func() { h.groupSig.set(pgid, pgToken(pgid), false) } + } + // The gateway is listed after the runner and server in its tier, so move it + // first to prove its stop does not hold up the signals behind it. + rec, err := readPgidFile(cfg.StateDir) + if err != nil { + t.Fatal(err) + } + h.containers.onStop[gatewayContainerNameTest] = func(context.Context) { h.containers.setExited(gatewayContainerNameTest) } + targets := liveTargets(context.Background(), cfg, deps, rec) + for i, tg := range targets { + if tg.entry.Component == ComponentGateway { + targets[0], targets[i] = targets[i], targets[0] + } + } + if survivors := drainTier(context.Background(), deps, targets[:3]); len(survivors) != 0 { + t.Fatalf("survivors = %v, want none", survivors) + } + events := h.rec.snapshot() + runnerTerm := indexOf(events, "group-term "+strconv.Itoa(runnerPgid)) + serverTerm := indexOf(events, "group-term "+strconv.Itoa(serverPgid)) + if indexOf(events, "ctr-stop "+gatewayContainerNameTest) != 0 || runnerTerm < 0 || serverTerm < 0 { + t.Fatalf("want the gateway stop first and both consumers signalled: %v", events) + } + lastTerm := max(runnerTerm, serverTerm) + for i, e := range events[:lastTerm] { + if strings.HasPrefix(e, "ctr-rm") { + t.Fatalf("event %d %q removed a container before its tier was signalled: %v", i, e, events) + } + } +} + +// TestDownDetachedInfraTierWaitsForConsumerTier: nats and postgres are not +// signalled while a consumer still drains. The server ignores SIGTERM, so its +// budget runs out and it is killed; only then may the infra tier get its stop. +func TestDownDetachedInfraTierWaitsForConsumerTier(t *testing.T) { + cfg, h := newHarness(t) + seedNatsRecord(t, cfg, h) + deps := sidecarContainerDownDeps(t, h) + h.groupSig.onTerm[runnerPgid] = func() { h.groupSig.set(runnerPgid, pgToken(runnerPgid), false) } + h.groupSig.onKill[serverPgid] = func() { h.groupSig.set(serverPgid, pgToken(serverPgid), false) } + h.groupSig.onTerm[pgPgid] = func() { h.groupSig.set(pgPgid, pgToken(pgPgid), false) } + h.containers.onStop[natsContainerNameTest] = func(context.Context) { h.containers.setExistsName(natsContainerNameTest, false) } + + if err := DownDetached(context.Background(), cfg, deps); err != nil { + t.Fatal(err) + } + events := h.rec.snapshot() + serverKill := indexOf(events, "group-kill "+strconv.Itoa(serverPgid)) + if serverKill < 0 { + t.Fatalf("server never escalated: %v", events) + } + for _, infra := range []string{"ctr-stop " + natsContainerNameTest, "group-term " + strconv.Itoa(pgPgid)} { + if i := indexOf(events, infra); i < serverKill { + t.Fatalf("%q at %d, before the server's budget expired at %d: %v", infra, i, serverKill, events) + } + } +} + +// TestDownDetachedCancelInConsumerTierStillStopsInfra: a down cancelled while +// consumers drain must still send the infra tier its stop on a live ctx, and +// record every infra target as a survivor so a retry can finish them. +func TestDownDetachedCancelInConsumerTierStillStopsInfra(t *testing.T) { + cfg, h := newHarness(t) + seedNatsRecord(t, cfg, h) + deps := sidecarContainerDownDeps(t, h) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + h.groupSig.onTerm[runnerPgid] = func() { + cancel() // the down process is signalled during the consumer tier + h.groupSig.set(runnerPgid, pgToken(runnerPgid), false) + } + h.groupSig.onTerm[serverPgid] = func() { h.groupSig.set(serverPgid, pgToken(serverPgid), false) } + var stopCtxErr error + h.containers.onStop[natsContainerNameTest] = func(stopCtx context.Context) { stopCtxErr = stopCtx.Err() } + + err := DownDetached(ctx, cfg, deps) + if err == nil { + t.Fatal("DownDetached = nil, want a survivor error for the unconfirmed infra tier") + } + if indexOf(h.rec.snapshot(), "ctr-stop "+natsContainerNameTest) < 0 { + t.Fatalf("nats never got its stop after cancel: %v", h.rec.snapshot()) + } + if stopCtxErr != nil { + t.Fatalf("nats stop ran on a done ctx (%v); exec would refuse it", stopCtxErr) + } + rec, rerr := readPgidFile(cfg.StateDir) + if rerr != nil { + t.Fatalf("survivor record read = %v", rerr) + } + got := map[Component]bool{} + for _, e := range rec.Entries { + got[e.Component] = true + } + if !got[ComponentNats] || !got[ComponentPostgres] { + t.Fatalf("survivor record = %+v, want nats and postgres", rec.Entries) + } +} + // The stable name a v2 collector container entry carries in these tests. const collectorContainerNameTest = "compass-otel-collector-test01" @@ -673,7 +1157,7 @@ func TestDownDetachedCollectorContainerTornDownByName(t *testing.T) { for _, pgid := range []int{pgPgid, serverPgid, runnerPgid} { h.groupSig.onTerm[pgid] = func() { h.groupSig.set(pgid, pgToken(pgid), false) } } - h.containers.onStop[collectorContainerNameTest] = func() { + h.containers.onStop[collectorContainerNameTest] = func(context.Context) { h.containers.setExistsName(collectorContainerNameTest, false) } @@ -770,7 +1254,7 @@ func TestDownDetachedNatsContainerTornDownByName(t *testing.T) { for _, pgid := range []int{pgPgid, serverPgid, runnerPgid} { h.groupSig.onTerm[pgid] = func() { h.groupSig.set(pgid, pgToken(pgid), false) } } - h.containers.onStop[natsContainerNameTest] = func() { + h.containers.onStop[natsContainerNameTest] = func(context.Context) { h.containers.setExistsName(natsContainerNameTest, false) } @@ -831,7 +1315,32 @@ func seedGatewayRecord(t *testing.T, cfg Config, h *harness) { h.groupSig.set(runnerPgid, pgToken(runnerPgid), true) } -func TestDownDetachedGatewayStopsThenRemoves(t *testing.T) { +// TestDownDetachedGatewayExitedIsRemovedAndConfirmed: the gateway runs without +// --rm, so after the stop signal it exits but lingers; the confirm poll removes +// it with a non-forced rm and confirms by absence, with no rm -f escalation. +func TestDownDetachedGatewayExitedIsRemovedAndConfirmed(t *testing.T) { + cfg, h := newHarness(t) + seedGatewayRecord(t, cfg, h) + deps := sidecarContainerDownDeps(t, h) + for _, p := range []int{pgPgid, serverPgid, runnerPgid} { + pgid := p + h.groupSig.onTerm[pgid] = func() { h.groupSig.set(pgid, pgToken(pgid), false) } + } + h.containers.onStop[gatewayContainerNameTest] = func(context.Context) { h.containers.setExited(gatewayContainerNameTest) } + if err := DownDetached(context.Background(), cfg, deps); err != nil { + t.Fatal(err) + } + got := ctrEvents(h.rec.snapshot()) + want := []string{"ctr-stop " + gatewayContainerNameTest, "ctr-rm-exited " + gatewayContainerNameTest} + if !reflect.DeepEqual(got, want) { + t.Fatalf("gateway teardown = %v, want stop then non-forced rm", got) + } + assertPgidFileGone(t, cfg.StateDir) +} + +// TestDownDetachedGatewayStillRunningEscalatesToRemove: while the gateway runs, +// the non-forced rm is a no-op, so it stays present until the budget's rm -f. +func TestDownDetachedGatewayStillRunningEscalatesToRemove(t *testing.T) { cfg, h := newHarness(t) seedGatewayRecord(t, cfg, h) deps := sidecarContainerDownDeps(t, h) @@ -839,15 +1348,16 @@ func TestDownDetachedGatewayStopsThenRemoves(t *testing.T) { pgid := p h.groupSig.onTerm[pgid] = func() { h.groupSig.set(pgid, pgToken(pgid), false) } } - h.containers.onStop[gatewayContainerNameTest] = func() { h.containers.setExistsName(gatewayContainerNameTest, false) } + h.containers.onRemove[gatewayContainerNameTest] = func() { h.containers.setExistsName(gatewayContainerNameTest, false) } if err := DownDetached(context.Background(), cfg, deps); err != nil { t.Fatal(err) } got := ctrEvents(h.rec.snapshot()) want := []string{"ctr-stop " + gatewayContainerNameTest, "ctr-rm " + gatewayContainerNameTest} if !reflect.DeepEqual(got, want) { - t.Fatalf("gateway teardown = %v, want stop then remove", got) + t.Fatalf("gateway teardown = %v, want stop then rm -f", got) } + assertPgidFileGone(t, cfg.StateDir) } // assertPgidFileGone fails if the pgid record still exists. @@ -887,6 +1397,7 @@ func TestSurvivorRecordV1RoundTrip(t *testing.T) { }, } + stubBootID(t, testBootID) survivors := survivorRecord(rec, []Component{ComponentServer, ComponentRunner}) if err := writePgidFile(dir, survivors); err != nil { t.Fatalf("writePgidFile survivor = %v", err) @@ -899,6 +1410,7 @@ func TestSurvivorRecordV1RoundTrip(t *testing.T) { want := pgidRecord{ WriterPid: 7, Version: pgidFileVersion, + BootID: testBootID, Entries: []pgidEntry{ {Kind: entryProc, Component: ComponentServer, Pgid: 201, StartTime: 1000}, {Kind: entryProc, Component: ComponentRunner, Pgid: 202, StartTime: 1001}, @@ -908,3 +1420,86 @@ func TestSurvivorRecordV1RoundTrip(t *testing.T) { t.Fatalf("reread record = %+v; want %+v", got, want) } } + +// TestDownDetachedRebootedRecordSignalsNothing proves a record from an earlier +// boot, with no answering socket, signals no group and is removed. +func TestDownDetachedRebootedRecordSignalsNothing(t *testing.T) { + cfg, h := newHarness(t) + seedFullRecord(t, cfg, h) + deps := darkSocketDownDeps(t, h) + // The live groups now belong to whoever holds those pids this boot. + stubBootID(t, "99999999-8888-7777-6666-555555555555") + + if err := DownDetached(context.Background(), cfg, deps); err != nil { + t.Fatalf("DownDetached on a prior-boot record = %v, want nil", err) + } + if got := signalEvents(h.rec.snapshot()); len(got) != 0 { + t.Fatalf("prior-boot record signalled groups: %v", got) + } + assertPgidFileGone(t, cfg.StateDir) +} + +// TestDownDetachedRebootedRecordLiveSocketRefuses proves an answering socket +// contradicts a boot mismatch: down refuses, keeps the record, and signals nothing. +func TestDownDetachedRebootedRecordLiveSocketRefuses(t *testing.T) { + cfg, h := newHarness(t) + seedFullRecord(t, cfg, h) + deps := downTestDeps(t, h) + deps.Prober = fixedServerProber(true) + stubBootID(t, "99999999-8888-7777-6666-555555555555") + + if err := DownDetached(context.Background(), cfg, deps); err == nil { + t.Fatal("DownDetached with a live socket under a boot mismatch = nil, want a refusal") + } + if got := signalEvents(h.rec.snapshot()); len(got) != 0 { + t.Fatalf("refusal signalled groups: %v", got) + } + if _, err := os.Stat(filepath.Join(cfg.StateDir, pgidFileName)); err != nil { + t.Fatalf("pgid record after refusal: %v; want it kept", err) + } +} + +// TestDownDetachedRebootedRecordStillChecksContainers proves a reboot drops only +// process entries: a container keeps its name identity and is confirmed by absence. +func TestDownDetachedRebootedRecordStillChecksContainers(t *testing.T) { + cfg, h := newHarness(t) + seedGatewayRecord(t, cfg, h) + deps := sidecarContainerDownDeps(t, h) + deps.Prober = fixedServerProber(false) + stubBootID(t, "99999999-8888-7777-6666-555555555555") + h.containers.onStop[gatewayContainerNameTest] = func(context.Context) { h.containers.setExited(gatewayContainerNameTest) } + + if err := DownDetached(context.Background(), cfg, deps); err != nil { + t.Fatalf("DownDetached on a prior-boot record = %v, want nil", err) + } + events := h.rec.snapshot() + if got := signalEvents(events); len(got) != 0 { + t.Fatalf("prior-boot record signalled groups: %v", got) + } + want := []string{"ctr-stop " + gatewayContainerNameTest, "ctr-rm-exited " + gatewayContainerNameTest} + if got := ctrEvents(events); !reflect.DeepEqual(got, want) { + t.Fatalf("prior-boot container teardown = %v, want %v", got, want) + } + assertPgidFileGone(t, cfg.StateDir) +} + +// TestDownDetachedLegacyHeaderKeepsIdentityTeardown proves a record with no boot +// id (an older build) is an unknown boot and tears down exactly as before. +func TestDownDetachedLegacyHeaderKeepsIdentityTeardown(t *testing.T) { + cfg, h := newHarness(t) + legacy := "2 4242\nproc postgres " + strconv.Itoa(pgPgid) + " " + strconv.FormatUint(pgToken(pgPgid), 10) + "\n" + if err := os.WriteFile(filepath.Join(cfg.StateDir, pgidFileName), []byte(legacy), 0o600); err != nil { + t.Fatalf("seed legacy record = %v", err) + } + h.groupSig.set(pgPgid, pgToken(pgPgid), true) + h.groupSig.onTerm[pgPgid] = func() { h.groupSig.set(pgPgid, pgToken(pgPgid), false) } + deps := downTestDeps(t, h) + + if err := DownDetached(context.Background(), cfg, deps); err != nil { + t.Fatalf("DownDetached(legacy) = %v, want nil", err) + } + if got, want := signalEvents(h.rec.snapshot()), []string{"group-term " + strconv.Itoa(pgPgid)}; !reflect.DeepEqual(got, want) { + t.Fatalf("legacy teardown:\n got %v\n want %v", got, want) + } + assertPgidFileGone(t, cfg.StateDir) +} diff --git a/go/internal/stack/gateway_harness_test.go b/go/internal/stack/gateway_harness_test.go index 8303c5a8a..8c382651e 100644 --- a/go/internal/stack/gateway_harness_test.go +++ b/go/internal/stack/gateway_harness_test.go @@ -36,7 +36,7 @@ func (c *fakeGatewayContainer) spec() GatewayContainerSpec { type stubGatewayProcess struct{ rec *recorder } -func (p *stubGatewayProcess) Signal(sig ProcessSignal) error { +func (p *stubGatewayProcess) Signal(_ context.Context, sig ProcessSignal) error { p.rec.add("signal llm-gateway") return nil } diff --git a/go/internal/stack/harness_test.go b/go/internal/stack/harness_test.go index d33c5dd1f..ab763021d 100644 --- a/go/internal/stack/harness_test.go +++ b/go/internal/stack/harness_test.go @@ -43,7 +43,7 @@ type stubProcess struct { rec *recorder } -func (p *stubProcess) Signal(sig ProcessSignal) error { +func (p *stubProcess) Signal(_ context.Context, sig ProcessSignal) error { p.rec.add("signal " + p.name) return nil } @@ -254,32 +254,31 @@ func (i *stubImage) EnsureImage(ctx context.Context, image string) error { return i.err } -// fakeGroupSignaller is the cross-process teardown seam under test: it records -// each signal in order and models per-group liveness as a controllable state -// machine, so DownDetached's ordering / escalation / zombie-window / identity -// logic is exercised with no real processes. alive maps pgid→liveness; identity -// maps pgid→the start-time token a live group answers to (a mismatch models a -// recycled pid). A pgid absent from alive is treated as gone (ESRCH). +// fakeGroupSignaller records signals and models the process-group identity states +// used by detached teardown. alive represents group existence; leaderReadable +// distinguishes a missing/unreadable leader from a recycled leader. type fakeGroupSignaller struct { - rec *recorder - mu sync.Mutex - alive map[int]bool - identity map[int]uint64 - // onKill / onTerm, when set for a pgid, run after a SIGKILL / SIGTERM to that - // group — a test uses them to flip a group dead at the right escalation step - // (or to leave a killed group a zombie: still "alive" for the group-ESRCH - // channel, which DownDetached treats as success for the runner). + rec *recorder + mu sync.Mutex + alive map[int]bool + identity map[int]uint64 + leaderReadable map[int]bool + signalErr map[int]map[ProcessSignal]error + // onKill / onTerm run after a delivered signal, outside the lock, so a test + // can flip group state at the right escalation step. onKill map[int]func() onTerm map[int]func() } func newFakeGroupSignaller(rec *recorder) *fakeGroupSignaller { return &fakeGroupSignaller{ - rec: rec, - alive: map[int]bool{}, - identity: map[int]uint64{}, - onKill: map[int]func(){}, - onTerm: map[int]func(){}, + rec: rec, + alive: map[int]bool{}, + identity: map[int]uint64{}, + leaderReadable: map[int]bool{}, + signalErr: map[int]map[ProcessSignal]error{}, + onKill: map[int]func(){}, + onTerm: map[int]func(){}, } } @@ -297,19 +296,36 @@ func (f *fakeGroupSignaller) Signal(pgid int, sig ProcessSignal) error { case SignalTerm: cb = f.onTerm[pgid] } + err := f.signalErr[pgid][sig] f.mu.Unlock() - // Run the hook OUTSIDE the lock: a hook typically calls set(), which locks - // f.mu, so holding it here would deadlock. + if err != nil { + return err + } if cb != nil { cb() } return nil } -func (f *fakeGroupSignaller) Alive(pgid int, startTime uint64) bool { +func (f *fakeGroupSignaller) Liveness(pgid int, startTime uint64) GroupLiveness { f.mu.Lock() defer f.mu.Unlock() - return f.alive[pgid] && f.identity[pgid] == startTime + switch { + case !f.alive[pgid]: + return GroupGone + case !f.leaderReadable[pgid]: + return GroupOrphaned + case f.identity[pgid] != startTime: + return GroupRecycled + default: + return GroupOwned + } +} + +// ours reports whether the recorded group is still present and ours to signal. +func (f *fakeGroupSignaller) ours(pgid int, startTime uint64) bool { + l := f.Liveness(pgid, startTime) + return l == GroupOwned || l == GroupOrphaned } func (f *fakeGroupSignaller) set(pgid int, startTime uint64, alive bool) { @@ -317,19 +333,40 @@ func (f *fakeGroupSignaller) set(pgid int, startTime uint64, alive bool) { defer f.mu.Unlock() f.alive[pgid] = alive f.identity[pgid] = startTime + f.leaderReadable[pgid] = alive +} + +// setLeaderUnknown models a group whose members survive an exited, reaped leader. +func (f *fakeGroupSignaller) setLeaderUnknown(pgid int) { + f.mu.Lock() + defer f.mu.Unlock() + f.alive[pgid] = true + f.leaderReadable[pgid] = false +} + +func (f *fakeGroupSignaller) failSignal(pgid int, sig ProcessSignal, err error) { + f.mu.Lock() + defer f.mu.Unlock() + if f.signalErr[pgid] == nil { + f.signalErr[pgid] = map[ProcessSignal]error{} + } + f.signalErr[pgid][sig] = err } // fakeContainerController is the container-teardown seam under test: it records // each stop/remove in order and models per-container existence as a controllable // state machine, the container analogue of fakeGroupSignaller. exists maps -// name→presence; a name absent from exists is treated as gone. onStop / onRemove -// hooks flip a container's existence at the right escalation step so a test can -// model a graceful stop, a stop-ignored→rm-f escalation, or a genuine survivor. +// name→presence; a name absent from exists is treated as gone. exited marks a +// present container that has stopped, which RemoveExited may remove. onStop / +// onRemove hooks flip a container's state at the right escalation step so a test +// can model a graceful stop, a stop-ignored→rm-f escalation, or a genuine +// survivor; an onStop hook receives the caller's ctx so it can model a slow stop. type fakeContainerController struct { rec *recorder mu sync.Mutex exists map[string]bool - onStop map[string]func() + exited map[string]bool + onStop map[string]func(ctx context.Context) onRemove map[string]func() } @@ -337,29 +374,42 @@ func newFakeContainerController(rec *recorder) *fakeContainerController { return &fakeContainerController{ rec: rec, exists: map[string]bool{}, - onStop: map[string]func(){}, + exited: map[string]bool{}, + onStop: map[string]func(context.Context){}, onRemove: map[string]func(){}, } } -func (c *fakeContainerController) Exists(name string) bool { +func (c *fakeContainerController) Exists(_ context.Context, name string) bool { c.mu.Lock() defer c.mu.Unlock() return c.exists[name] } -func (c *fakeContainerController) Stop(name string, timeout time.Duration) error { +func (c *fakeContainerController) Stop(ctx context.Context, name string) error { c.mu.Lock() c.rec.add("ctr-stop " + name) cb := c.onStop[name] c.mu.Unlock() if cb != nil { - cb() // outside the lock: a hook calls setExists, which locks c.mu. + cb(ctx) // outside the lock: a hook calls setExists, which locks c.mu. + } + return nil +} + +// RemoveExited removes only an exited container, as a non-forced `podman rm` +// does; a running one is left alone without error. +func (c *fakeContainerController) RemoveExited(_ context.Context, name string) error { + c.mu.Lock() + defer c.mu.Unlock() + if c.exists[name] && c.exited[name] { + c.rec.add("ctr-rm-exited " + name) + c.exists[name] = false } return nil } -func (c *fakeContainerController) Remove(name string) error { +func (c *fakeContainerController) Remove(_ context.Context, name string) error { c.mu.Lock() c.rec.add("ctr-rm " + name) cb := c.onRemove[name] @@ -382,6 +432,13 @@ func (c *fakeContainerController) setExistsName(name string, exists bool) { c.exists[name] = exists } +// setExited marks a present container as stopped but not yet removed. +func (c *fakeContainerController) setExited(name string) { + c.mu.Lock() + defer c.mu.Unlock() + c.exited[name] = true +} + // fakePostgresContainer is the container-START seam under test (the analogue of // stubSupervisor for the container path): Start records the run and the spec it // was handed, so a test can assert the container path was taken and inspect the @@ -429,7 +486,7 @@ type stubContainerProcess struct { stopped atomic.Bool } -func (p *stubContainerProcess) Signal(sig ProcessSignal) error { +func (p *stubContainerProcess) Signal(_ context.Context, sig ProcessSignal) error { p.rec.add("signal postgres") p.stopped.Store(true) return nil @@ -488,7 +545,7 @@ type stubCollectorProcess struct { stopped atomic.Bool } -func (p *stubCollectorProcess) Signal(sig ProcessSignal) error { +func (p *stubCollectorProcess) Signal(_ context.Context, sig ProcessSignal) error { p.rec.add("signal otel-collector") p.stopped.Store(true) return nil @@ -588,7 +645,7 @@ type stubNatsProcess struct { stopped atomic.Bool } -func (p *stubNatsProcess) Signal(sig ProcessSignal) error { +func (p *stubNatsProcess) Signal(_ context.Context, sig ProcessSignal) error { p.rec.add("signal nats") p.stopped.Store(true) return nil @@ -697,6 +754,7 @@ func newHarness(t *testing.T) (Config, *harness) { prev := readStartTime readStartTime = func(pid int) (uint64, error) { return uint64(pid) * 10, nil } t.Cleanup(func() { readStartTime = prev }) + stubBootID(t, testBootID) h := &harness{ rec: rec, serverStarted: started, sup: sup, cert: cert, token: token, image: image, prober: prober, dbProber: dbProber, groupSig: groupSig, containers: containers, collector: collector, collectorProber: collectorProber, nats: nats, natsProber: natsProber, diff --git a/go/internal/stack/pgidfile.go b/go/internal/stack/pgidfile.go index 12cc37f68..be8b5acdf 100644 --- a/go/internal/stack/pgidfile.go +++ b/go/internal/stack/pgidfile.go @@ -5,6 +5,7 @@ package stack import ( "errors" "fmt" + "log/slog" "os" "path/filepath" "strconv" @@ -93,11 +94,14 @@ type pgidEntry struct { // child-liveness signal: up always exits after a successful spawn // (main.go:235-238), so the writer pid is dead in every linger teardown and // discriminates nothing about whether the children are alive. Child liveness is -// decided per entry by pgid identity (Pgid + StartTime), not by the header. +// decided per entry by pgid identity (Pgid + StartTime), not by the header — +// except that a BootID from an earlier boot means no recorded group survives. type pgidRecord struct { WriterPid int Version string - Entries []pgidEntry + // BootID is the writer's boot identity; empty means unknown (a pre-boot-id record). + BootID string + Entries []pgidEntry } // writePgidFile publishes rec to /stack.pgids atomically (temp + @@ -110,17 +114,25 @@ type pgidRecord struct { // // Format (v2): // -// +// // proc (a process-group child) // ctr (a container child) // ... (one line per entry, in start order) // // The leading kind tag makes the entry line a discriminated union; a v1 record // (untagged 3-field proc lines) is read-only back-compat — this build never -// writes it. +// writes it. is always the writer's current boot (rec.BootID is +// ignored), and is omitted when unreadable, which a reader takes as unknown. func writePgidFile(stateDir string, rec pgidRecord) error { var b strings.Builder - fmt.Fprintf(&b, "%s %d\n", pgidFileVersion, rec.WriterPid) + fmt.Fprintf(&b, "%s %d", pgidFileVersion, rec.WriterPid) + if boot, err := readBootID(); err != nil || !isBootUUID(boot) { + // An unknown boot only forgoes the reboot shortcut; identity checks still guard every signal. + slog.Warn("pgid record written without a boot id", "boot_id", boot, "err", err) + } else { + fmt.Fprintf(&b, " %s", boot) + } + b.WriteString("\n") for _, e := range rec.Entries { switch e.Kind { case entryContainer: @@ -187,18 +199,28 @@ func readPgidFile(stateDir string) (pgidRecord, error) { } header := strings.Fields(lines[0]) - if len(header) != 2 { + if len(header) != 2 && len(header) != 3 { return pgidRecord{}, fmt.Errorf("pgid file %q: malformed header %q", path, lines[0]) } version := header[0] if version != pgidFileVersion && version != pgidFileVersionV1 { return pgidRecord{}, fmt.Errorf("pgid file %q: unsupported record version %q (this build reads %q and %q); stop the stack with the build that started it", path, version, pgidFileVersionV1, pgidFileVersion) } + // v1 predates the boot id, so a third column there is a malformed header. + if len(header) == 3 && version == pgidFileVersionV1 { + return pgidRecord{}, fmt.Errorf("pgid file %q: malformed v1 header %q", path, lines[0]) + } writerPid, err := strconv.Atoi(header[1]) if err != nil { return pgidRecord{}, fmt.Errorf("pgid file %q: unparseable writer pid %q: %w", path, header[1], err) } rec := pgidRecord{WriterPid: writerPid, Version: version} + if len(header) == 3 { + if !isBootUUID(header[2]) { + return pgidRecord{}, fmt.Errorf("pgid file %q: malformed boot id in header %q", path, lines[0]) + } + rec.BootID = header[2] + } for _, line := range lines[1:] { if line == "" { @@ -213,6 +235,26 @@ func readPgidFile(stateDir string) (pgidRecord, error) { return rec, nil } +// isBootUUID reports whether s has the 8-4-4-4-12 hex UUID shape, in either case. +func isBootUUID(s string) bool { + if len(s) != 36 { + return false + } + for i, c := range s { + switch i { + case 8, 13, 18, 23: + if c != '-' { + return false + } + default: + if (c < '0' || c > '9') && (c < 'a' || c > 'f') && (c < 'A' || c > 'F') { + return false + } + } + } + return true +} + // parsePgidLine parses one entry line, dispatched on the record version. // // - v1 ("1"): an untagged " " line, parsed as a @@ -343,12 +385,15 @@ func removePgidFile(stateDir string) error { // // 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 +// on a given OS. GroupSignaller.Liveness compares the two for uint64 equality, so a +// disagreement would report every live child as recycled 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 +// readBootID is the boot-identity seam: a var so tests can model a reboot. +var readBootID = readCurrentBootID + // parseStatStartTime extracts field 22 (starttime) from a /proc//stat line. // Split out from the Linux reader (readstarttime_linux.go) so the // parenthesized-comm parse is unit-tested against synthesized lines without a diff --git a/go/internal/stack/pgidfile_test.go b/go/internal/stack/pgidfile_test.go index 7ff05ba54..49dc497f7 100644 --- a/go/internal/stack/pgidfile_test.go +++ b/go/internal/stack/pgidfile_test.go @@ -13,10 +13,12 @@ import ( // TestPgidFileRoundTrip proves the format survives a write→read cycle including // the start-time identity column, in start order. func TestPgidFileRoundTrip(t *testing.T) { + stubBootID(t, testBootID) dir := t.TempDir() rec := pgidRecord{ WriterPid: 4242, Version: pgidFileVersion, + BootID: testBootID, Entries: []pgidEntry{ {Component: ComponentPostgres, Pgid: 1001, StartTime: 10010}, {Component: ComponentServer, Pgid: 1002, StartTime: 10020}, @@ -40,10 +42,12 @@ func TestPgidFileRoundTrip(t *testing.T) { // both entry kinds: a container entry (ctr) interleaved with process entries // (proc) survives a write→read cycle intact, in order. func TestPgidFileRoundTripBothKinds(t *testing.T) { + stubBootID(t, testBootID) dir := t.TempDir() rec := pgidRecord{ WriterPid: 4242, Version: pgidFileVersion, + BootID: testBootID, Entries: []pgidEntry{ {Kind: entryContainer, Component: ComponentPostgres, ContainerName: "compass-postgres-abc123"}, {Kind: entryProc, Component: ComponentServer, Pgid: 1002, StartTime: 10020}, @@ -65,6 +69,7 @@ func TestPgidFileRoundTripBothKinds(t *testing.T) { // TestPgidFileV2ContainerLineGrammar pins the exact on-disk ctr line grammar so a // format drift is caught: "ctr ", no pgid/starttime columns. func TestPgidFileV2ContainerLineGrammar(t *testing.T) { + stubBootID(t, testBootID) dir := t.TempDir() rec := pgidRecord{ WriterPid: 7, @@ -80,7 +85,7 @@ func TestPgidFileV2ContainerLineGrammar(t *testing.T) { if err != nil { t.Fatalf("ReadFile = %v", err) } - want := "2 7\nctr postgres compass-postgres-deadbeef\n" + want := "2 7 " + testBootID + "\nctr postgres compass-postgres-deadbeef\n" if string(data) != want { t.Fatalf("file content = %q, want %q", string(data), want) } @@ -175,6 +180,7 @@ func TestPgidFileMode0600(t *testing.T) { // TestPgidFileTrailingNewline pins the plain-text format: a header line plus one // entry line per child, each newline-terminated. func TestPgidFileTrailingNewline(t *testing.T) { + stubBootID(t, testBootID) dir := t.TempDir() rec := pgidRecord{ WriterPid: 7, @@ -190,7 +196,7 @@ func TestPgidFileTrailingNewline(t *testing.T) { if err != nil { t.Fatalf("ReadFile = %v", err) } - want := "2 7\nproc postgres 200 999\n" + want := "2 7 " + testBootID + "\nproc postgres 200 999\n" if string(data) != want { t.Fatalf("file content = %q, want %q", string(data), want) } @@ -261,6 +267,11 @@ func TestReadPgidFileMalformed(t *testing.T) { cases := map[string]string{ "empty": "", "header only one col": "1\n", + "header four cols": "2 7 boot extra\n", + "v1 header with boot": "1 7 boot\npostgres 200 999\n", + "boot id not a uuid": "2 7 1700000000.000123\nproc postgres 200 999\n", + "boot id short group": "2 7 1111111-2222-3333-4444-555555555555\n", + "boot id non-hex": "2 7 1111111g-2222-3333-4444-555555555555\n", "bad writer pid": "1 notanumber\n", "entry too few fields": "1 7\npostgres 200\n", "unknown component": "1 7\nnot-a-component 200 999\n", @@ -320,3 +331,92 @@ func TestReadStartTimeProcParsesParenthesizedComm(t *testing.T) { t.Fatalf("starttime = %d, want 987654", got) } } + +// testBootID is the boot identity the stubbed seam reports in these tests. +const testBootID = "11111111-2222-3333-4444-555555555555" + +// stubBootID pins the boot-identity seam for one test. +func stubBootID(t *testing.T, id string) { + t.Helper() + prev := readBootID + readBootID = func() (string, error) { return id, nil } + t.Cleanup(func() { readBootID = prev }) +} + +// TestPgidFileStampsCurrentBootID proves the writer stamps the current boot, +// not whatever the caller's record carried, so a survivor rewrite stays current. +func TestPgidFileStampsCurrentBootID(t *testing.T) { + stubBootID(t, testBootID) + dir := t.TempDir() + if err := writePgidFile(dir, pgidRecord{WriterPid: 7, Version: pgidFileVersion, BootID: "stale-boot"}); err != nil { + t.Fatalf("writePgidFile = %v", err) + } + got, err := readPgidFile(dir) + if err != nil { + t.Fatalf("readPgidFile = %v", err) + } + if got.BootID != testBootID { + t.Fatalf("BootID = %q, want the current boot %q", got.BootID, testBootID) + } +} + +// TestPgidFileUnreadableBootIDWritesLegacyHeader proves an unreadable boot id +// never blocks up: the record falls back to the two-column header (unknown boot). +func TestPgidFileUnreadableBootIDWritesLegacyHeader(t *testing.T) { + prev := readBootID + readBootID = func() (string, error) { return "", errors.New("no boot id here") } + t.Cleanup(func() { readBootID = prev }) + dir := t.TempDir() + if err := writePgidFile(dir, pgidRecord{WriterPid: 7, Version: pgidFileVersion}); err != nil { + t.Fatalf("writePgidFile = %v", err) + } + data, err := os.ReadFile(filepath.Join(dir, pgidFileName)) + if err != nil { + t.Fatalf("ReadFile = %v", err) + } + if want := "2 7\n"; string(data) != want { + t.Fatalf("file content = %q, want %q", string(data), want) + } +} + +// TestReadPgidFileLegacyHeaderNoBootID proves a record from a build that wrote +// no boot id still parses, with the boot reported unknown (empty). +func TestReadPgidFileLegacyHeaderNoBootID(t *testing.T) { + dir := t.TempDir() + legacy := "2 7\nproc postgres 200 999\nctr llm-gateway gw-1\n" + if err := os.WriteFile(filepath.Join(dir, pgidFileName), []byte(legacy), 0o600); err != nil { + t.Fatalf("seed legacy file = %v", err) + } + got, err := readPgidFile(dir) + if err != nil { + t.Fatalf("readPgidFile(legacy) = %v, want a parsed record", err) + } + want := pgidRecord{ + WriterPid: 7, + Version: pgidFileVersion, + Entries: []pgidEntry{ + {Kind: entryProc, Component: ComponentPostgres, Pgid: 200, StartTime: 999}, + {Kind: entryContainer, Component: ComponentGateway, ContainerName: "gw-1"}, + }, + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("legacy parse:\n got %+v\n want %+v", got, want) + } +} + +// TestReadPgidFileUppercaseBootID proves the boot id shape check accepts either +// hex case, so a darwin bootsessionuuid (uppercase) parses. +func TestReadPgidFileUppercaseBootID(t *testing.T) { + dir := t.TempDir() + const boot = "A1B2C3D4-E5F6-4A7B-8C9D-0E1F2A3B4C5D" + if err := os.WriteFile(filepath.Join(dir, pgidFileName), []byte("2 7 "+boot+"\n"), 0o600); err != nil { + t.Fatalf("seed file = %v", err) + } + got, err := readPgidFile(dir) + if err != nil { + t.Fatalf("readPgidFile = %v, want a parsed record", err) + } + if got.BootID != boot { + t.Fatalf("BootID = %q, want %q", got.BootID, boot) + } +} diff --git a/go/internal/stack/readstarttime_darwin.go b/go/internal/stack/readstarttime_darwin.go index 689f178bf..2a803d2e1 100644 --- a/go/internal/stack/readstarttime_darwin.go +++ b/go/internal/stack/readstarttime_darwin.go @@ -43,9 +43,9 @@ func readProcessStartTime(pid int) (uint64, error) { // 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 +// — GroupSignaller.Liveness 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 +// live child as recycled 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. // diff --git a/go/internal/stack/readstarttime_darwin_test.go b/go/internal/stack/readstarttime_darwin_test.go index bdcc09d24..464b44c3d 100644 --- a/go/internal/stack/readstarttime_darwin_test.go +++ b/go/internal/stack/readstarttime_darwin_test.go @@ -15,9 +15,9 @@ import ( // 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 +// package boundary the packages cannot cross, and GroupSignaller.Liveness 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 +// recycled 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} @@ -30,7 +30,7 @@ func TestPackStartTimevalMatchesDownSide(t *testing.T) { // 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 +// would break the identity gate — Liveness would stop matching a group it spawned // moments earlier and skip it at teardown. func TestReadProcessStartTimeSelfIsStable(t *testing.T) { pid := os.Getpid() diff --git a/go/internal/stack/spec.go b/go/internal/stack/spec.go index 43ce88d4d..9a4e90a99 100644 --- a/go/internal/stack/spec.go +++ b/go/internal/stack/spec.go @@ -13,6 +13,8 @@ import ( // table (cmd/compass-runner/main.go:105-110). const tokenEnvVar = "COMPASS_RUNNER_TOKEN" +const microVMRunRootEnvVar = "COMPASS_MICROVM_RUNROOT" + // embeddedRunnerID is the fixed identity of the single embedded runner. Embedded // mode is single-user/single-runner by design (DL-106), so the id is an internal // constant rather than a Config knob; it is cross-checked against the minted @@ -58,7 +60,7 @@ func serverSpec(cfg Config, cert CertResult) ProcessSpec { // the server's TLS door over https, trusts the same cert as its --ca anchor, // and mints per-container sockets under cfg.RuntimeDir. The token rides in Env // only; guest is resolved by the caller (zero = the Runner image's baked copy). -func runnerSpec(cfg Config, cert CertResult, token string, guest GuestPaths) ProcessSpec { +func runnerSpec(cfg Config, cert CertResult, token string, guest GuestPaths, microVMRunRootEnv string) ProcessSpec { // The four unconditional flags every runner spawn carries. Each optional // flag below is appended only when set, so a caller that leaves them zero // (the embedded supervisor, the compass-stack CLI's resolveConfig) gets a @@ -76,6 +78,10 @@ func runnerSpec(cfg Config, cert CertResult, token string, guest GuestPaths) Pro args = append(args, "--image", cfg.AgentImage) } args = append(args, "--runtime-dir", cfg.RuntimeDir) + if cfg.microVM() && microVMRunRootEnv == "" { + // The flag overrides the inherited env, so only default it when unset. + args = append(args, "--microvm-runroot", cfg.RuntimeDir) + } // AgentModel: forward a single --agent-model only when pinned. Forwarding // --agent-model "" would break an embedded supervisor that relies on the // runner's own default, so an empty selector must omit the flag entirely. diff --git a/go/internal/stack/spec_test.go b/go/internal/stack/spec_test.go index d668922e7..076b5b08f 100644 --- a/go/internal/stack/spec_test.go +++ b/go/internal/stack/spec_test.go @@ -3,6 +3,7 @@ package stack import ( + "os" "slices" "testing" ) @@ -25,7 +26,11 @@ func baseRunnerArgs(cfg Config, cert CertResult) []string { if !cfg.microVM() { args = append(args, "--image", cfg.AgentImage) } - return append(args, "--runtime-dir", cfg.RuntimeDir) + args = append(args, "--runtime-dir", cfg.RuntimeDir) + if cfg.microVM() { + args = append(args, "--microvm-runroot", cfg.RuntimeDir) + } + return args } // TestRunnerSpecForwardsOptionalFlagsConditionally is the load-bearing red→green @@ -107,7 +112,7 @@ func TestRunnerSpecForwardsOptionalFlagsConditionally(t *testing.T) { cfg.CheckoutDir = tt.checkoutDir cfg.Mounts = tt.mounts - spec := runnerSpec(cfg, cert, token, GuestPaths{}) + spec := runnerSpec(cfg, cert, token, GuestPaths{}, "") want := append(baseRunnerArgs(cfg, cert), tt.wantExtra...) if !slices.Equal(spec.Args, want) { @@ -158,7 +163,7 @@ func TestRunnerSpecGuestArgs(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := runnerSpec(tt.cfg, cert, "token", tt.guest).Args + got := runnerSpec(tt.cfg, cert, "token", tt.guest, "").Args want := append(baseRunnerArgs(tt.cfg, cert), tt.wantEnd...) if !slices.Equal(got, want) { t.Fatalf("runnerSpec Args = %q, want %q", got, want) @@ -176,13 +181,13 @@ func TestRunnerSpecOmitsAgentImageUnderMicroVM(t *testing.T) { cert := CertResult{CertPath: "/state/tls.crt"} cfg := Config{ListenAddr: "127.0.0.1:50052", AgentImage: "agent:latest", RuntimeDir: "/run/compass"} - container := runnerSpec(cfg, cert, "token", GuestPaths{}).Args + container := runnerSpec(cfg, cert, "token", GuestPaths{}, "").Args if i := slices.Index(container, "--image"); i < 0 || container[i+1] != "agent:latest" { t.Fatalf("container-backend args %q must still carry --image agent:latest", container) } cfg.RuntimeBackend = "microvm" - micro := runnerSpec(cfg, cert, "token", GuestPaths{}).Args + micro := runnerSpec(cfg, cert, "token", GuestPaths{}, "").Args if slices.Contains(micro, "--image") { t.Errorf("microVM args %q carry --image, which the runner refuses", micro) } @@ -191,6 +196,43 @@ func TestRunnerSpecOmitsAgentImageUnderMicroVM(t *testing.T) { } } +func TestRunnerSpecMicroVMRunRoot(t *testing.T) { + cert := CertResult{CertPath: "/state/tls.crt"} + base := Config{ + ListenAddr: "127.0.0.1:50052", + RuntimeDir: "/run/compass", + RuntimeBackend: runtimeBackendMicroVM, + } + tests := []struct { + name string + backend string + envRunRoot string + wantRunRoot string + }{ + {name: "microVM defaults to RuntimeDir", backend: runtimeBackendMicroVM, wantRunRoot: base.RuntimeDir}, + {name: "microVM environment override wins", backend: runtimeBackendMicroVM, envRunRoot: "/operator/runroot"}, + {name: "container backend omits microVM run root", backend: "container", envRunRoot: "/operator/runroot"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv(microVMRunRootEnvVar, tt.envRunRoot) + cfg := base + cfg.RuntimeBackend = tt.backend + args := runnerSpec(cfg, cert, "token", GuestPaths{}, os.Getenv(microVMRunRootEnvVar)).Args + index := slices.Index(args, "--microvm-runroot") + if tt.wantRunRoot == "" { + if index >= 0 { + t.Fatalf("runner args %q include --microvm-runroot, want it omitted", args) + } + return + } + if index < 0 || index+1 >= len(args) || args[index+1] != tt.wantRunRoot { + t.Fatalf("runner args %q, want --microvm-runroot %q", args, tt.wantRunRoot) + } + }) + } +} + // The empty arm is the load-bearing one: an unset SecretProvider must yield a // byte-identical argv, since the embedded supervisor and compass-stack's // resolveConfig both leave it zero. diff --git a/go/internal/stack/stack.go b/go/internal/stack/stack.go index a2126fda8..8deea7fd6 100644 --- a/go/internal/stack/stack.go +++ b/go/internal/stack/stack.go @@ -207,6 +207,8 @@ func attachContended(ctx context.Context, cfg Config, deps Deps) (*Stack, error) // cross-process teardown record has nothing left to describe. A partial or // failed drain leaves the file in place so a later fresh down can still finish // the job. An attached stack recorded no children, so removal is a no-op. +// An expired ctx fails each stop and wait fast, so children may survive; the +// record stays in place and a later DownDetached finishes them. func (s *Stack) Down(ctx context.Context) error { err := s.drainChildren(ctx) if err == nil { @@ -231,7 +233,7 @@ func (s *Stack) RestartRunner(ctx context.Context) error { if len(s.pgids) == 0 || s.pgids[len(s.pgids)-1].Component != ComponentRunner { return errors.New("stack: runner teardown record is missing") } - if err := s.runner.Signal(SignalTerm); err != nil { + if err := s.runner.Signal(ctx, SignalTerm); err != nil { return fmt.Errorf("stop runner: %w", err) } if err := s.runner.Wait(ctx); err != nil { @@ -277,7 +279,7 @@ func (s *Stack) startRunner(ctx context.Context) error { if err := s.resolveGuest(ctx); err != nil { return err } - runner, err := s.deps.Supervisor.Start(ctx, runnerSpec(s.cfg, s.cert, token, s.guest)) + runner, err := s.deps.Supervisor.Start(ctx, runnerSpec(s.cfg, s.cert, token, s.guest, os.Getenv(microVMRunRootEnvVar))) if err != nil { return fmt.Errorf("start compass-runner: %w", err) } @@ -705,7 +707,7 @@ func (s *Stack) drainChildren(ctx context.Context) error { if c.p == nil { continue } - if err := c.p.Signal(SignalTerm); err != nil { + if err := c.p.Signal(ctx, SignalTerm); err != nil { errs = errors.Join(errs, fmt.Errorf("signal %s: %w", c.name, err)) continue } diff --git a/go/internal/stack/stack_test.go b/go/internal/stack/stack_test.go index 9d5e7dda9..bdfbd5c17 100644 --- a/go/internal/stack/stack_test.go +++ b/go/internal/stack/stack_test.go @@ -55,7 +55,7 @@ func TestUpColdSequencing(t *testing.T) { // either side stops using embeddedRunnerID. func TestRunnerIDCouplesSpawnAndMint(t *testing.T) { // (a) runnerSpec carries --runner-id with the constant's value. - spec := runnerSpec(Config{ListenAddr: "127.0.0.1:50052"}, CertResult{CertPath: "/c"}, "tok", GuestPaths{}) + spec := runnerSpec(Config{ListenAddr: "127.0.0.1:50052"}, CertResult{CertPath: "/c"}, "tok", GuestPaths{}, "") specID, ok := flagValue(spec.Args, "--runner-id") if !ok { t.Fatalf("runner spec args %v carry no --runner-id", spec.Args)