From 8ce377aeafb4c489f82d392d1029b1c5a567b452 Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 5 Oct 2026 05:33:26 -0400 Subject: [PATCH 01/13] fix(runnerhub): report a token-store fault as Unavailable, not Unauthenticated (RIG-4529) auth.ResolveToken folded every store error into ErrTokenNotFound, so a database blip during Runner enrollment came back Unauthenticated. The Runner treats that as permanent and exited instead of retrying. ResolveToken now maps only store.ErrNotFound to ErrTokenNotFound. Any other non-revoked error, including ctx cancellation, wraps a new ErrTokenLookupFailed. The Runner door returns a fixed Unavailable for it and logs the cause server-side. Missing, revoked, wrong-kind and malformed tokens stay one byte-identical Unauthenticated. The account door is unchanged. A security second opinion found no new oracle and no fail-open path. Spec-impact: none Refs: RIG-4529, RIG-3691 Co-authored-by: Matt Wilkinson --- go/internal/auth/interceptor.go | 1 + go/internal/auth/token.go | 42 +++++++++++++--------- go/internal/auth/token_test.go | 52 +++++++++++++++++++++++---- go/internal/runnerhub/auth.go | 32 +++++++++++------ go/internal/runnerhub/auth_test.go | 56 ++++++++++++++++++++++++++++-- 5 files changed, 147 insertions(+), 36 deletions(-) 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/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..4d08ad149 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,42 @@ 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 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 +282,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. From b70eefaca6c0c474379ca9beec1155a439d83fa9 Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 5 Oct 2026 05:46:23 -0400 Subject: [PATCH 02/13] test(runnerhub): assert the fixed Unavailable message (RIG-4529) Spec-impact: none Refs: RIG-4529 Co-authored-by: Matt Wilkinson --- go/internal/runnerhub/auth_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/go/internal/runnerhub/auth_test.go b/go/internal/runnerhub/auth_test.go index 4d08ad149..624f31a64 100644 --- a/go/internal/runnerhub/auth_test.go +++ b/go/internal/runnerhub/auth_test.go @@ -104,6 +104,9 @@ func TestAuthenticateLookupFailureIsUnavailable(t *testing.T) { 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) } From 064ebc3b96ffa0cda7d1fb2a645b179b0d442264 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 4 Oct 2026 11:31:16 -0400 Subject: [PATCH 03/13] fix(stack): confirm cross-process teardown by group exit, not socket alone (RIG-3937) DownDetached took a dark socket as proof that the process-backed server and postgres had stopped, so a recorded group could outlive down. It now requires the recorded group to be gone or recycled. A container is confirmed only by its absence. GroupSignaller.Liveness separates gone, owned, orphaned and recycled groups: an orphaned group keeps our pgid and is still signalled. Identity is re-checked before every signal, and the post-SIGKILL zombie shortcut needs a delivered kill. Co-authored-by: Matt Wilkinson --- .../cross_process_podman_test.go | 35 +- go/internal/stack/adapters/groupsignal.go | 47 +- .../stack/adapters/groupsignal_darwin.go | 8 +- .../stack/adapters/groupsignal_darwin_test.go | 6 +- .../stack/adapters/groupsignal_linux.go | 4 +- .../stack/adapters/groupsignal_other.go | 6 +- .../stack/adapters/groupsignal_test.go | 109 +++-- go/internal/stack/adapters/process_test.go | 26 ++ go/internal/stack/deps.go | 35 +- go/internal/stack/downdetached.go | 160 ++++--- go/internal/stack/downdetached_test.go | 402 ++++++++++++++++-- go/internal/stack/harness_test.go | 80 +++- go/internal/stack/pgidfile.go | 4 +- go/internal/stack/readstarttime_darwin.go | 4 +- .../stack/readstarttime_darwin_test.go | 6 +- 15 files changed, 703 insertions(+), 229 deletions(-) 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/stack/adapters/groupsignal.go b/go/internal/stack/adapters/groupsignal.go index b65132c52..af56680f0 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,26 @@ 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 + if err := syscall.Kill(-pgid, 0); errors.Is(err, syscall.ESRCH) { + return stack.GroupGone } - // 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_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/process_test.go b/go/internal/stack/adapters/process_test.go index 61bf4f4c6..fc6fe20f8 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. diff --git a/go/internal/stack/deps.go b/go/internal/stack/deps.go index aaf907605..5fd6a71cc 100644 --- a/go/internal/stack/deps.go +++ b/go/internal/stack/deps.go @@ -195,20 +195,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 leader's start time differs: the pid is someone else's. + 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,7 +229,7 @@ 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 +// analogue of GroupSignaller.Liveness; 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 // SIGKILL-tier escalation that force-removes it (`podman rm -f `). diff --git a/go/internal/stack/downdetached.go b/go/internal/stack/downdetached.go index 92b563e45..b94bcc339 100644 --- a/go/internal/stack/downdetached.go +++ b/go/internal/stack/downdetached.go @@ -66,11 +66,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 @@ -160,18 +158,18 @@ 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. +// 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 +179,66 @@ 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(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(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 { + {comp: ComponentGateway, budget: gatewayDrainBudget, confirm: 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) } }}, - {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) } }}, - {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) } }}, - {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(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 } }}, } @@ -227,7 +252,11 @@ func liveTargets(ctx context.Context, cfg Config, deps Deps, rec pgidRecord) []t if !entryAlive(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 } @@ -258,30 +287,24 @@ func drainTargets(ctx context.Context, deps Deps, targets []target) []Component } // 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(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,17 +377,31 @@ 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. +// 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(deps Deps, e pgidEntry) bool { switch e.Kind { case entryContainer: return deps.Containers.Exists(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 @@ -384,6 +421,10 @@ func signalTerm(deps Deps, e pgidEntry, budget time.Duration) { } } 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 +432,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(deps Deps, e pgidEntry) bool { switch e.Kind { case entryContainer: if err := deps.Containers.Remove(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..f18b14dc1 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) @@ -607,6 +927,30 @@ 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) + } +} + // The stable name a v2 collector container entry carries in these tests. const collectorContainerNameTest = "compass-otel-collector-test01" diff --git a/go/internal/stack/harness_test.go b/go/internal/stack/harness_test.go index d33c5dd1f..551abf77b 100644 --- a/go/internal/stack/harness_test.go +++ b/go/internal/stack/harness_test.go @@ -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,6 +333,24 @@ 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 diff --git a/go/internal/stack/pgidfile.go b/go/internal/stack/pgidfile.go index 12cc37f68..79db9caaf 100644 --- a/go/internal/stack/pgidfile.go +++ b/go/internal/stack/pgidfile.go @@ -343,8 +343,8 @@ 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 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() From 5ca6d3e3a8d26ac7a6d93193197ede78eef0ecca Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 5 Oct 2026 12:52:00 -0400 Subject: [PATCH 04/13] fix(stack): treat another uid's process group as recycled (RIG-3937) kill(0) EPERM means the pgid belongs to another uid, and our children share our uid. Falling through to the leader read could call that group orphaned and signal it, or leave a survivor no later down can kill. Co-authored-by: Matt Wilkinson --- go/internal/stack/adapters/groupsignal.go | 6 ++- .../stack/adapters/groupsignal_linux_test.go | 49 +++++++++++++++++++ go/internal/stack/deps.go | 2 +- 3 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 go/internal/stack/adapters/groupsignal_linux_test.go diff --git a/go/internal/stack/adapters/groupsignal.go b/go/internal/stack/adapters/groupsignal.go index af56680f0..064707ec6 100644 --- a/go/internal/stack/adapters/groupsignal.go +++ b/go/internal/stack/adapters/groupsignal.go @@ -64,8 +64,12 @@ func (g *GroupSignaller) Liveness(pgid int, startTime uint64) stack.GroupLivenes if pgid <= 1 { return stack.GroupGone } - if err := syscall.Kill(-pgid, 0); errors.Is(err, syscall.ESRCH) { + 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 } got, err := readGroupLeaderStartTime(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/deps.go b/go/internal/stack/deps.go index 5fd6a71cc..bbc934e70 100644 --- a/go/internal/stack/deps.go +++ b/go/internal/stack/deps.go @@ -207,7 +207,7 @@ const ( GroupOwned // GroupOrphaned means the group exists but its leader is gone or unreadable. GroupOrphaned - // GroupRecycled means the leader's start time differs: the pid is someone else's. + // GroupRecycled means the pid is someone else's: a different leader start time, or another uid's group. GroupRecycled ) From d86a7d96952c256f90a1be1f3c3611132b309099 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 6 Oct 2026 00:39:02 -0400 Subject: [PATCH 05/13] feat(stack): skip groups recorded in an earlier boot (RIG-4570) A pgid from a prior boot may now lead a stranger group, so down drops those entries unsignalled. Container entries keep the confirm-by-absence path. Co-authored-by: Matt Wilkinson --- .../release/compass-distribution/design.md | 8 ++ .../design.md | 2 + go/internal/stack/bootid_darwin.go | 18 ++++ go/internal/stack/bootid_linux.go | 18 ++++ go/internal/stack/bootid_other.go | 13 +++ go/internal/stack/downdetached.go | 26 +++++- go/internal/stack/downdetached_test.go | 64 ++++++++++++++ go/internal/stack/harness_test.go | 1 + go/internal/stack/pgidfile.go | 34 ++++++-- go/internal/stack/pgidfile_test.go | 84 ++++++++++++++++++- 10 files changed, 259 insertions(+), 9 deletions(-) create mode 100644 go/internal/stack/bootid_darwin.go create mode 100644 go/internal/stack/bootid_linux.go create mode 100644 go/internal/stack/bootid_other.go diff --git a/docs/designs/infra/release/compass-distribution/design.md b/docs/designs/infra/release/compass-distribution/design.md index 63e6b4c76..1b972873b 100644 --- a/docs/designs/infra/release/compass-distribution/design.md +++ b/docs/designs/infra/release/compass-distribution/design.md @@ -419,6 +419,14 @@ 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.boottime`). 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. A missing boot id (older builds, or an unreadable id) is + *unknown* and falls back to the per-entry identity check, so the + extension stays compatible. 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/internal/stack/bootid_darwin.go b/go/internal/stack/bootid_darwin.go new file mode 100644 index 000000000..33cd0a7b3 --- /dev/null +++ b/go/internal/stack/bootid_darwin.go @@ -0,0 +1,18 @@ +//go:build darwin + +package stack + +import ( + "fmt" + + "golang.org/x/sys/unix" +) + +// readCurrentBootID renders kern.boottime, which is fixed for the life of one boot. +func readCurrentBootID() (string, error) { + tv, err := unix.SysctlTimeval("kern.boottime") + if err != nil { + return "", fmt.Errorf("sysctl kern.boottime: %w", err) + } + return fmt.Sprintf("%d.%06d", tv.Sec, tv.Usec), 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/downdetached.go b/go/internal/stack/downdetached.go index b94bcc339..03fd32a75 100644 --- a/go/internal/stack/downdetached.go +++ b/go/internal/stack/downdetached.go @@ -100,7 +100,9 @@ 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 record from an earlier boot + // has no live group at all, so its process entries are dropped unsignalled. + rec = dropPriorBootGroups(rec) targets := liveTargets(ctx, cfg, deps, rec) // 4/5/6. SIGTERM every live target up front (reverse order), then per-target @@ -158,6 +160,28 @@ func consumeRecord(ctx context.Context, cfg Config, deps Deps) (rec pgidRecord, return rec, true, nil } +// 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. +func dropPriorBootGroups(rec pgidRecord) pgidRecord { + if rec.BootID == "" { + return rec + } + current, err := readBootID() + if err != nil || current == "" || current == rec.BootID { + return rec + } + 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 +} + // 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 { diff --git a/go/internal/stack/downdetached_test.go b/go/internal/stack/downdetached_test.go index f18b14dc1..1a1115cd1 100644 --- a/go/internal/stack/downdetached_test.go +++ b/go/internal/stack/downdetached_test.go @@ -1231,6 +1231,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) @@ -1243,6 +1244,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}, @@ -1252,3 +1254,65 @@ func TestSurvivorRecordV1RoundTrip(t *testing.T) { t.Fatalf("reread record = %+v; want %+v", got, want) } } + +// TestDownDetachedRebootedRecordSignalsNothing proves a record from an earlier +// boot signals no group (its pgids cannot be ours) and is removed. +func TestDownDetachedRebootedRecordSignalsNothing(t *testing.T) { + cfg, h := newHarness(t) + seedFullRecord(t, cfg, h) + deps := downTestDeps(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) +} + +// 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) + stubBootID(t, "99999999-8888-7777-6666-555555555555") + h.containers.onStop[gatewayContainerNameTest] = func() { h.containers.setExistsName(gatewayContainerNameTest, false) } + + 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 " + 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/harness_test.go b/go/internal/stack/harness_test.go index 551abf77b..1dd9ddaa8 100644 --- a/go/internal/stack/harness_test.go +++ b/go/internal/stack/harness_test.go @@ -731,6 +731,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 79db9caaf..77005e4c1 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 || boot == "" || strings.ContainsAny(boot, " \t\n") { + // An unknown boot only forgoes the reboot shortcut; identity checks still guard every signal. + slog.Debug("pgid record written without a boot id", "err", err) + } else { + fmt.Fprintf(&b, " %s", boot) + } + b.WriteString("\n") for _, e := range rec.Entries { switch e.Kind { case entryContainer: @@ -187,18 +199,25 @@ 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 { + rec.BootID = header[2] + } for _, line := range lines[1:] { if line == "" { @@ -349,6 +368,9 @@ func removePgidFile(stateDir string) error { // (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..9d86f690c 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,8 @@ 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", "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 +328,75 @@ 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) + } +} From a5fd62ede014965f4bc07a56cb34bceca049faf2 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 6 Oct 2026 00:57:25 -0400 Subject: [PATCH 06/13] fix(stack): read a per-boot UUID and refuse a reboot drop while the socket answers (RIG-4570) kern.boottime moves on a clock step, so darwin now reads kern.bootsessionuuid. A live socket proves the stack is from this boot, so down refuses instead. Co-authored-by: Matt Wilkinson --- .../release/compass-distribution/design.md | 12 +++++--- go/internal/stack/bootid_darwin.go | 9 +++--- go/internal/stack/downdetached.go | 28 +++++++++++++------ go/internal/stack/downdetached_test.go | 25 +++++++++++++++-- go/internal/stack/pgidfile.go | 27 ++++++++++++++++-- go/internal/stack/pgidfile_test.go | 20 +++++++++++++ 6 files changed, 101 insertions(+), 20 deletions(-) diff --git a/docs/designs/infra/release/compass-distribution/design.md b/docs/designs/infra/release/compass-distribution/design.md index 1b972873b..1c368d73f 100644 --- a/docs/designs/infra/release/compass-distribution/design.md +++ b/docs/designs/infra/release/compass-distribution/design.md @@ -421,12 +421,16 @@ Mechanics, grounded in the current seams: 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.boottime`). A + (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. A missing boot id (older builds, or an unreadable id) is - *unknown* and falls back to the per-entry identity check, so the - extension stays compatible. A v1 header never carries the column. + 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/go/internal/stack/bootid_darwin.go b/go/internal/stack/bootid_darwin.go index 33cd0a7b3..68f3edbf6 100644 --- a/go/internal/stack/bootid_darwin.go +++ b/go/internal/stack/bootid_darwin.go @@ -8,11 +8,12 @@ import ( "golang.org/x/sys/unix" ) -// readCurrentBootID renders kern.boottime, which is fixed for the life of one boot. +// 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) { - tv, err := unix.SysctlTimeval("kern.boottime") + id, err := unix.Sysctl("kern.bootsessionuuid") if err != nil { - return "", fmt.Errorf("sysctl kern.boottime: %w", err) + return "", fmt.Errorf("sysctl kern.bootsessionuuid: %w", err) } - return fmt.Sprintf("%d.%06d", tv.Sec, tv.Usec), nil + return id, nil } diff --git a/go/internal/stack/downdetached.go b/go/internal/stack/downdetached.go index 03fd32a75..625619340 100644 --- a/go/internal/stack/downdetached.go +++ b/go/internal/stack/downdetached.go @@ -100,9 +100,8 @@ 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. A record from an earlier boot - // has no live group at all, so its process entries are dropped unsignalled. - rec = dropPriorBootGroups(rec) + // 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 @@ -151,6 +150,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. @@ -164,13 +167,22 @@ func consumeRecord(ctx context.Context, cfg Config, deps Deps) (rec pgidRecord, // 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. -func dropPriorBootGroups(rec pgidRecord) pgidRecord { +// 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 + return rec, nil } current, err := readBootID() - if err != nil || current == "" || current == rec.BootID { - return rec + 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} @@ -179,7 +191,7 @@ func dropPriorBootGroups(rec pgidRecord) pgidRecord { out.Entries = append(out.Entries, e) } } - return out + return out, nil } // target is one live child to tear down: its recorded identity, confirmation diff --git a/go/internal/stack/downdetached_test.go b/go/internal/stack/downdetached_test.go index 1a1115cd1..9cd216755 100644 --- a/go/internal/stack/downdetached_test.go +++ b/go/internal/stack/downdetached_test.go @@ -1256,11 +1256,11 @@ func TestSurvivorRecordV1RoundTrip(t *testing.T) { } // TestDownDetachedRebootedRecordSignalsNothing proves a record from an earlier -// boot signals no group (its pgids cannot be ours) and is removed. +// 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 := downTestDeps(t, h) + deps := darkSocketDownDeps(t, h) // The live groups now belong to whoever holds those pids this boot. stubBootID(t, "99999999-8888-7777-6666-555555555555") @@ -1273,12 +1273,33 @@ func TestDownDetachedRebootedRecordSignalsNothing(t *testing.T) { 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() { h.containers.setExistsName(gatewayContainerNameTest, false) } diff --git a/go/internal/stack/pgidfile.go b/go/internal/stack/pgidfile.go index 77005e4c1..be8b5acdf 100644 --- a/go/internal/stack/pgidfile.go +++ b/go/internal/stack/pgidfile.go @@ -126,9 +126,9 @@ type pgidRecord struct { func writePgidFile(stateDir string, rec pgidRecord) error { var b strings.Builder fmt.Fprintf(&b, "%s %d", pgidFileVersion, rec.WriterPid) - if boot, err := readBootID(); err != nil || boot == "" || strings.ContainsAny(boot, " \t\n") { + if boot, err := readBootID(); err != nil || !isBootUUID(boot) { // An unknown boot only forgoes the reboot shortcut; identity checks still guard every signal. - slog.Debug("pgid record written without a boot id", "err", err) + slog.Warn("pgid record written without a boot id", "boot_id", boot, "err", err) } else { fmt.Fprintf(&b, " %s", boot) } @@ -216,6 +216,9 @@ func readPgidFile(stateDir string) (pgidRecord, error) { } 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] } @@ -232,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 diff --git a/go/internal/stack/pgidfile_test.go b/go/internal/stack/pgidfile_test.go index 9d86f690c..49dc497f7 100644 --- a/go/internal/stack/pgidfile_test.go +++ b/go/internal/stack/pgidfile_test.go @@ -269,6 +269,9 @@ func TestReadPgidFileMalformed(t *testing.T) { "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", @@ -400,3 +403,20 @@ func TestReadPgidFileLegacyHeaderNoBootID(t *testing.T) { 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) + } +} From c1745f90b7ea2a780eadd66f5426601263097974 Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 5 Oct 2026 15:32:27 -0400 Subject: [PATCH 07/13] fix(stack): bound container teardown by ctx and never block Phase A (RIG-4571) Container stops took no ctx and Phase A ran a blocking podman stop per container, so a teardown could run about 65s before the server budget began. The app then SIGKILLed down after its record was consumed, which skipped the survivor rewrite and leaked the stack. ContainerController and Process.Signal now take ctx, Phase A sends a non-blocking stop signal, the gateway confirm removes an exited container, and runStackDown cancels with SIGTERM plus a wait delay. Co-authored-by: Matt Wilkinson --- go/cmd/compass-app/embedded.go | 5 + go/cmd/compass-app/embedded_test.go | 33 ++++++ go/cmd/compass-app/lifecycle.go | 4 + .../stack/adapters/collector_container.go | 19 ++-- .../adapters/collector_container_test.go | 19 ++-- .../stack/adapters/gateway_container.go | 25 +++-- .../stack/adapters/gateway_container_test.go | 26 +++-- go/internal/stack/adapters/nats_container.go | 25 ++--- .../stack/adapters/nats_container_test.go | 19 ++-- .../stack/adapters/postgres_container.go | 90 +++++++++++++--- .../stack/adapters/postgres_container_test.go | 69 ++++++++++-- go/internal/stack/adapters/process.go | 4 +- go/internal/stack/adapters/process_test.go | 14 +-- go/internal/stack/deps.go | 23 ++-- go/internal/stack/downdetached.go | 63 +++++------ go/internal/stack/downdetached_test.go | 100 ++++++++++++++++-- go/internal/stack/gateway_harness_test.go | 2 +- go/internal/stack/harness_test.go | 49 ++++++--- go/internal/stack/stack.go | 4 +- 19 files changed, 445 insertions(+), 148 deletions(-) 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/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/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..4b0283313 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,6 +323,24 @@ func (e *podmanExec) stop(ctx context.Context, name string, timeout time.Duratio return nil } +// 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 (`podman rm -f`). An absent container is // already removed — not an error. func (e *podmanExec) remove(ctx context.Context, name string) error { @@ -328,6 +353,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. @@ -364,9 +402,33 @@ 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 + 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 +} + // 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..091dabcb1 100644 --- a/go/internal/stack/adapters/postgres_container_test.go +++ b/go/internal/stack/adapters/postgres_container_test.go @@ -21,7 +21,9 @@ type fakeContainerCLI struct { runErr error waited []string stopped []string + termed []string removed []string + rmExited []string existsResp map[string]bool existsErr error } @@ -41,11 +43,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 +156,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 +168,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 +177,67 @@ 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) { + prog := filepath.Join(t.TempDir(), "podman") + script := "#!/bin/sh\necho '" + tc.stderr + "' >&2\nexit 2\n" + if err := os.WriteFile(prog, []byte(script), 0o700); err != nil { + t.Fatal(err) + } + e := &podmanExec{program: prog, timeout: 5 * time.Second} + if err := e.removeExited(context.Background(), "x"); (err != nil) != tc.wantErr { + t.Fatalf("removeExited() = %v, wantErr %v", err, tc.wantErr) + } + }) + } +} + // 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 +247,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 fc6fe20f8..9e0d0c280 100644 --- a/go/internal/stack/adapters/process_test.go +++ b/go/internal/stack/adapters/process_test.go @@ -181,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) } @@ -270,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 { @@ -293,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 { @@ -314,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 { @@ -329,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 { @@ -377,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/deps.go b/go/internal/stack/deps.go index bbc934e70..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 } @@ -230,13 +231,19 @@ 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.Liveness; 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 +// 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 625619340..3182321c1 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 @@ -222,14 +222,14 @@ func liveTargets(ctx context.Context, cfg Config, deps Deps, rec pgidRecord) []t }{ {comp: ComponentRunner, budget: runnerDrainBudget, confirm: func(e pgidEntry) func() bool { if e.Kind == entryContainer { - return func() bool { return !deps.Containers.Exists(e.ContainerName) } + 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) } }}, {comp: ComponentServer, budget: serverDrainBudget, confirm: func(e pgidEntry) func() bool { if e.Kind == entryContainer { - return func() bool { return !deps.Containers.Exists(e.ContainerName) } + return func() bool { return !deps.Containers.Exists(ctx, e.ContainerName) } } // A dark UDS can precede process exit during graceful shutdown. return func() bool { @@ -245,27 +245,32 @@ func liveTargets(ctx context.Context, cfg Config, deps Deps, rec pgidRecord) []t } }}, {comp: ComponentGateway, budget: gatewayDrainBudget, confirm: 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) } + // 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) + } }}, {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) } }}, {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) } }}, {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(e.ContainerName) } + return func() bool { return !deps.Containers.Exists(ctx, e.ContainerName) } } // A dark DSN can precede process exit during postgres shutdown. return func() bool { @@ -285,7 +290,7 @@ 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 } target := target{entry: e, budget: o.budget, confirm: o.confirm(e)} @@ -307,8 +312,9 @@ func drainTargets(ctx context.Context, deps Deps, targets []target) []Component // 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). + // No signal here waits for its target to exit: every drain budget runs in Phase B. for _, t := range targets { - signalTerm(deps, t.entry, t.budget) + signalTerm(ctx, deps, t.entry) } // Phase B: per-target confirm with bounded SIGKILL escalation. @@ -333,7 +339,7 @@ func drainOne(ctx context.Context, deps Deps, t target) bool { return true // SIGTERM sufficed (or the group was already gone) } - killed := signalKill(deps, t.entry) + killed := signalKill(ctx, deps, t.entry) if killed && ctx.Err() == nil { if t.entry.Component == ComponentRunner { return true @@ -415,10 +421,10 @@ func logSignalMiss(sig string, e pgidEntry, err error) { // 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(deps Deps, e pgidEntry) bool { +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 groupOurs(deps, e) } @@ -441,21 +447,16 @@ func groupReleased(deps Deps, e pgidEntry) bool { } // 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 Phase B's waitDead 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) { @@ -470,10 +471,10 @@ 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. It reports true only when a // process-group SIGKILL was delivered, the precondition for the zombie shortcut. -func signalKill(deps Deps, e pgidEntry) bool { +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 diff --git a/go/internal/stack/downdetached_test.go b/go/internal/stack/downdetached_test.go index 9cd216755..4954ffae6 100644 --- a/go/internal/stack/downdetached_test.go +++ b/go/internal/stack/downdetached_test.go @@ -792,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 @@ -832,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) @@ -951,6 +951,66 @@ func TestDownDetachedContainerDarkDBStillExistsIsSurvivor(t *testing.T) { } } +// 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) + } +} + +// TestDownDetachedPhaseADoesNotWaitOnContainerDrain: Phase A only signals. A +// container's removal belongs to the drain phase, so the TERM to every later +// target goes out before any container is removed. +func TestDownDetachedPhaseADoesNotWaitOnContainerDrain(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) + } + events := h.rec.snapshot() + pgTerm := indexOf(events, "group-term "+strconv.Itoa(pgPgid)) + if pgTerm < 0 { + t.Fatalf("postgres never SIGTERMed: %v", events) + } + for i, e := range events[:pgTerm] { + if strings.HasPrefix(e, "ctr-rm") { + t.Fatalf("event %d %q removed a container before the last target was signalled: %v", i, e, events) + } + } +} + // The stable name a v2 collector container entry carries in these tests. const collectorContainerNameTest = "compass-otel-collector-test01" @@ -1017,7 +1077,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) } @@ -1114,7 +1174,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) } @@ -1175,7 +1235,10 @@ 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) @@ -1183,15 +1246,38 @@ 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.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) + for _, p := range []int{pgPgid, serverPgid, runnerPgid} { + pgid := p + h.groupSig.onTerm[pgid] = func() { h.groupSig.set(pgid, pgToken(pgid), 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. 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 1dd9ddaa8..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 } @@ -356,14 +356,17 @@ func (f *fakeGroupSignaller) failSignal(pgid int, sig ProcessSignal, err error) // 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() } @@ -371,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 } -func (c *fakeContainerController) Remove(name string) error { +// 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(_ context.Context, name string) error { c.mu.Lock() c.rec.add("ctr-rm " + name) cb := c.onRemove[name] @@ -416,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 @@ -463,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 @@ -522,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 @@ -622,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 diff --git a/go/internal/stack/stack.go b/go/internal/stack/stack.go index a2126fda8..4e4c3ab9b 100644 --- a/go/internal/stack/stack.go +++ b/go/internal/stack/stack.go @@ -231,7 +231,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 { @@ -705,7 +705,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 } From fe93f42379a4ec1298d3032e4f7cd0509f85fafc Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 5 Oct 2026 16:34:27 -0400 Subject: [PATCH 08/13] fix(stack): drain consumers before infra; hard-kill without stop-timeout (RIG-4571) Phase A signalled every tier at once, so postgres and nats stopped while the server still drained. Teardown now signals and drains the consumer tier before the infra tier. rm --force skips the stop-timeout, podman calls get a wait delay, and the gateway confirm only removes a present container. Co-authored-by: Matt Wilkinson --- .../stack/adapters/postgres_container.go | 13 ++- .../stack/adapters/postgres_container_test.go | 90 +++++++++++++++++-- go/internal/stack/downdetached.go | 51 +++++++---- go/internal/stack/downdetached_test.go | 82 ++++++++++++++--- go/internal/stack/harness_test.go | 25 +++--- go/internal/stack/stack.go | 2 + 6 files changed, 214 insertions(+), 49 deletions(-) diff --git a/go/internal/stack/adapters/postgres_container.go b/go/internal/stack/adapters/postgres_container.go index 4b0283313..e575ab448 100644 --- a/go/internal/stack/adapters/postgres_container.go +++ b/go/internal/stack/adapters/postgres_container.go @@ -341,10 +341,11 @@ func (e *podmanExec) term(ctx context.Context, name string) error { return nil } -// remove force-removes the container (`podman rm -f`). An absent container is -// already removed — not an error. +// 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 } @@ -390,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 { @@ -408,6 +410,7 @@ 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() @@ -420,6 +423,10 @@ func (e *podmanExec) output(ctx context.Context, args []string) (string, error) 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. diff --git a/go/internal/stack/adapters/postgres_container_test.go b/go/internal/stack/adapters/postgres_container_test.go index 091dabcb1..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" @@ -225,12 +226,7 @@ func TestRemoveExitedToleratesRunningAndAbsent(t *testing.T) { {"engine failure", "Error: database is locked", true}, } { t.Run(tc.name, func(t *testing.T) { - prog := filepath.Join(t.TempDir(), "podman") - script := "#!/bin/sh\necho '" + tc.stderr + "' >&2\nexit 2\n" - if err := os.WriteFile(prog, []byte(script), 0o700); err != nil { - t.Fatal(err) - } - e := &podmanExec{program: prog, timeout: 5 * time.Second} + 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) } @@ -238,6 +234,88 @@ func TestRemoveExitedToleratesRunningAndAbsent(t *testing.T) { } } +// 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 diff --git a/go/internal/stack/downdetached.go b/go/internal/stack/downdetached.go index 3182321c1..2b18ba2f0 100644 --- a/go/internal/stack/downdetached.go +++ b/go/internal/stack/downdetached.go @@ -104,8 +104,8 @@ func DownDetached(ctx context.Context, cfg Config, deps Deps) error { // 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. @@ -247,6 +247,9 @@ func liveTargets(ctx context.Context, cfg Config, deps Deps, rec pgidRecord) []t {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 !deps.Containers.Exists(ctx, e.ContainerName) { + return true + } if err := deps.Containers.RemoveExited(ctx, e.ContainerName); err != nil { logContainerSignalMiss("rm", e, err) } @@ -302,28 +305,38 @@ func liveTargets(ctx context.Context, cfg Config, deps Deps, rec pgidRecord) []t 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). - // No signal here waits for its target to exit: every drain budget runs in Phase B. + var consumers, infra []target for _, t := range targets { - signalTerm(ctx, deps, t.entry) + if consumerTier[t.entry.Component] { + consumers = append(consumers, t) + } else { + infra = append(infra, t) + } } + return append(drainTier(ctx, deps, consumers), drainTier(ctx, deps, infra)...) +} - // 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 } @@ -448,7 +461,7 @@ func groupReleased(deps Deps, e pgidEntry) bool { // signalTerm delivers the graceful-stop tier, dispatched on kind: a group // SIGTERM for a process, the container's stop signal for a container. Neither -// waits for exit, so Phase B's waitDead measures every drain budget. A delivery +// 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) { diff --git a/go/internal/stack/downdetached_test.go b/go/internal/stack/downdetached_test.go index 4954ffae6..302887cd6 100644 --- a/go/internal/stack/downdetached_test.go +++ b/go/internal/stack/downdetached_test.go @@ -983,10 +983,10 @@ func TestDownDetachedCancelledDuringSlowContainerStopRewritesSurvivors(t *testin } } -// TestDownDetachedPhaseADoesNotWaitOnContainerDrain: Phase A only signals. A -// container's removal belongs to the drain phase, so the TERM to every later -// target goes out before any container is removed. -func TestDownDetachedPhaseADoesNotWaitOnContainerDrain(t *testing.T) { +// 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) @@ -994,19 +994,59 @@ func TestDownDetachedPhaseADoesNotWaitOnContainerDrain(t *testing.T) { 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() - pgTerm := indexOf(events, "group-term "+strconv.Itoa(pgPgid)) - if pgTerm < 0 { - t.Fatalf("postgres never SIGTERMed: %v", events) + serverKill := indexOf(events, "group-kill "+strconv.Itoa(serverPgid)) + if serverKill < 0 { + t.Fatalf("server never escalated: %v", events) } - for i, e := range events[:pgTerm] { - if strings.HasPrefix(e, "ctr-rm") { - t.Fatalf("event %d %q removed a container before the last target was signalled: %v", i, e, 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) } } } @@ -1258,6 +1298,28 @@ func TestDownDetachedGatewayExitedIsRemovedAndConfirmed(t *testing.T) { assertPgidFileGone(t, cfg.StateDir) } +// TestDownDetachedGatewayGoneSkipsRemoveExited: once the gateway is absent the +// confirm is done; it must not issue a podman rm for a container already gone. +func TestDownDetachedGatewayGoneSkipsRemoveExited(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.setExistsName(gatewayContainerNameTest, false) } + if err := DownDetached(context.Background(), cfg, deps); err != nil { + t.Fatal(err) + } + h.containers.mu.Lock() + calls := h.containers.rmExitedCalls[gatewayContainerNameTest] + h.containers.mu.Unlock() + if calls != 0 { + t.Fatalf("RemoveExited called %d times on an absent gateway, want 0", calls) + } +} + // 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) { diff --git a/go/internal/stack/harness_test.go b/go/internal/stack/harness_test.go index ab763021d..585db2c80 100644 --- a/go/internal/stack/harness_test.go +++ b/go/internal/stack/harness_test.go @@ -362,21 +362,23 @@ func (f *fakeGroupSignaller) failSignal(pgid int, sig ProcessSignal, err error) // 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 - exited map[string]bool - onStop map[string]func(ctx context.Context) - onRemove map[string]func() + rec *recorder + mu sync.Mutex + exists map[string]bool + exited map[string]bool + rmExitedCalls map[string]int + onStop map[string]func(ctx context.Context) + onRemove map[string]func() } func newFakeContainerController(rec *recorder) *fakeContainerController { return &fakeContainerController{ - rec: rec, - exists: map[string]bool{}, - exited: map[string]bool{}, - onStop: map[string]func(context.Context){}, - onRemove: map[string]func(){}, + rec: rec, + exists: map[string]bool{}, + exited: map[string]bool{}, + rmExitedCalls: map[string]int{}, + onStop: map[string]func(context.Context){}, + onRemove: map[string]func(){}, } } @@ -402,6 +404,7 @@ func (c *fakeContainerController) Stop(ctx context.Context, name string) error { func (c *fakeContainerController) RemoveExited(_ context.Context, name string) error { c.mu.Lock() defer c.mu.Unlock() + c.rmExitedCalls[name]++ if c.exists[name] && c.exited[name] { c.rec.add("ctr-rm-exited " + name) c.exists[name] = false diff --git a/go/internal/stack/stack.go b/go/internal/stack/stack.go index 4e4c3ab9b..dca5b272f 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 { From 0fc7c01a81adc4eccfcb4c14563d2739b00818c4 Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 5 Oct 2026 16:42:05 -0400 Subject: [PATCH 09/13] fix(stack): still stop infra when down is cancelled mid-teardown (RIG-4571) Co-authored-by: Matt Wilkinson --- go/internal/stack/downdetached.go | 20 +++++++-- go/internal/stack/downdetached_test.go | 62 +++++++++++++++++--------- go/internal/stack/harness_test.go | 25 +++++------ 3 files changed, 67 insertions(+), 40 deletions(-) diff --git a/go/internal/stack/downdetached.go b/go/internal/stack/downdetached.go index 2b18ba2f0..df9b6a90d 100644 --- a/go/internal/stack/downdetached.go +++ b/go/internal/stack/downdetached.go @@ -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 @@ -247,9 +250,6 @@ func liveTargets(ctx context.Context, cfg Config, deps Deps, rec pgidRecord) []t {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 !deps.Containers.Exists(ctx, e.ContainerName) { - return true - } if err := deps.Containers.RemoveExited(ctx, e.ContainerName); err != nil { logContainerSignalMiss("rm", e, err) } @@ -322,7 +322,19 @@ func drainTargets(ctx context.Context, deps Deps, targets []target) []Component infra = append(infra, t) } } - return append(drainTier(ctx, deps, consumers), drainTier(ctx, deps, infra)...) + 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 } // drainTier signals every target in the tier, then drains each one in turn. diff --git a/go/internal/stack/downdetached_test.go b/go/internal/stack/downdetached_test.go index 302887cd6..31b5a284d 100644 --- a/go/internal/stack/downdetached_test.go +++ b/go/internal/stack/downdetached_test.go @@ -1051,6 +1051,46 @@ func TestDownDetachedInfraTierWaitsForConsumerTier(t *testing.T) { } } +// 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" @@ -1298,28 +1338,6 @@ func TestDownDetachedGatewayExitedIsRemovedAndConfirmed(t *testing.T) { assertPgidFileGone(t, cfg.StateDir) } -// TestDownDetachedGatewayGoneSkipsRemoveExited: once the gateway is absent the -// confirm is done; it must not issue a podman rm for a container already gone. -func TestDownDetachedGatewayGoneSkipsRemoveExited(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.setExistsName(gatewayContainerNameTest, false) } - if err := DownDetached(context.Background(), cfg, deps); err != nil { - t.Fatal(err) - } - h.containers.mu.Lock() - calls := h.containers.rmExitedCalls[gatewayContainerNameTest] - h.containers.mu.Unlock() - if calls != 0 { - t.Fatalf("RemoveExited called %d times on an absent gateway, want 0", calls) - } -} - // 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) { diff --git a/go/internal/stack/harness_test.go b/go/internal/stack/harness_test.go index 585db2c80..ab763021d 100644 --- a/go/internal/stack/harness_test.go +++ b/go/internal/stack/harness_test.go @@ -362,23 +362,21 @@ func (f *fakeGroupSignaller) failSignal(pgid int, sig ProcessSignal, err error) // 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 - exited map[string]bool - rmExitedCalls map[string]int - onStop map[string]func(ctx context.Context) - onRemove map[string]func() + rec *recorder + mu sync.Mutex + exists map[string]bool + exited map[string]bool + onStop map[string]func(ctx context.Context) + onRemove map[string]func() } func newFakeContainerController(rec *recorder) *fakeContainerController { return &fakeContainerController{ - rec: rec, - exists: map[string]bool{}, - exited: map[string]bool{}, - rmExitedCalls: map[string]int{}, - onStop: map[string]func(context.Context){}, - onRemove: map[string]func(){}, + rec: rec, + exists: map[string]bool{}, + exited: map[string]bool{}, + onStop: map[string]func(context.Context){}, + onRemove: map[string]func(){}, } } @@ -404,7 +402,6 @@ func (c *fakeContainerController) Stop(ctx context.Context, name string) error { func (c *fakeContainerController) RemoveExited(_ context.Context, name string) error { c.mu.Lock() defer c.mu.Unlock() - c.rmExitedCalls[name]++ if c.exists[name] && c.exited[name] { c.rec.add("ctr-rm-exited " + name) c.exists[name] = false From 3062afc7f0b9a2ccbaccd3854d697f5d5be33ff5 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 6 Oct 2026 00:40:29 -0400 Subject: [PATCH 10/13] test(stack): drive the rebooted-record gateway through ctx teardown (RIG-4571) Co-authored-by: Matt Wilkinson --- go/internal/stack/downdetached_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/internal/stack/downdetached_test.go b/go/internal/stack/downdetached_test.go index 31b5a284d..02d8265ce 100644 --- a/go/internal/stack/downdetached_test.go +++ b/go/internal/stack/downdetached_test.go @@ -1467,7 +1467,7 @@ func TestDownDetachedRebootedRecordStillChecksContainers(t *testing.T) { deps := sidecarContainerDownDeps(t, h) deps.Prober = fixedServerProber(false) stubBootID(t, "99999999-8888-7777-6666-555555555555") - h.containers.onStop[gatewayContainerNameTest] = func() { h.containers.setExistsName(gatewayContainerNameTest, false) } + 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) @@ -1476,7 +1476,7 @@ func TestDownDetachedRebootedRecordStillChecksContainers(t *testing.T) { if got := signalEvents(events); len(got) != 0 { t.Fatalf("prior-boot record signalled groups: %v", got) } - want := []string{"ctr-stop " + gatewayContainerNameTest, "ctr-rm " + gatewayContainerNameTest} + 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) } From de20f34b171ce91dd50af764f099411a530e2759 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 4 Oct 2026 23:38:00 -0400 Subject: [PATCH 11/13] fix(stack): default the microVM runner run root to the runtime dir (RIG-4514) compass-stack up on the microVM backend never passed --microvm-runroot, so the runner died at preflight ("run-root is not configured") unless the operator exported COMPASS_MICROVM_RUNROOT by hand. runnerSpec now passes --microvm-runroot on the microVM backend when COMPASS_MICROVM_RUNROOT is unset. The flag would beat the inherited env, so an operator's value is left alone. The RuntimeDir budget Validate already enforces for the agent socket also covers the worst-case microVM gateway socket; a test pins that. Making up fail while the runner is dead is a separate decision (RIG-4519). Spec-impact: none Refs: RIG-4514 Co-authored-by: Matt Wilkinson --- go/internal/stack/config_test.go | 10 ++++++ go/internal/stack/spec.go | 8 ++++- go/internal/stack/spec_test.go | 52 +++++++++++++++++++++++++++++--- go/internal/stack/stack.go | 2 +- go/internal/stack/stack_test.go | 2 +- 5 files changed, 66 insertions(+), 8 deletions(-) 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/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..e82f2c0db 100644 --- a/go/internal/stack/stack.go +++ b/go/internal/stack/stack.go @@ -277,7 +277,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) } 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) From da5149a2366448e51c1508817d929e79ad6cc118 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 6 Oct 2026 22:12:04 -0400 Subject: [PATCH 12/13] fix(fabric): give the max-deliveries advisory fetch its own deadline (RIG-4786) The advisory fetched the parked message under an AckWait timeout. The server emits that advisory only after AckWait expires, so the budget measured nothing and a loaded run timed the fetch out, which left the park to the callback after the test had moved on. Floor the fetch at 5s. Under concurrent package load the two advisory tests failed 10/200 and 14/200 before and 0/400 after. Refs: RIG-4786 Co-authored-by: Matt Wilkinson --- go/internal/fabric/SUBJECTS.md | 2 ++ go/internal/fabric/event_fabric.go | 8 +++++-- go/internal/fabric/event_fabric_test.go | 32 ++++++++++++------------- 3 files changed, 23 insertions(+), 19 deletions(-) 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..cabdf2e3d 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,8 @@ 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) + if budget := <-fetchBudget; budget <= ackWait { + t.Fatalf("advisory fetch budget = %s, want greater than AckWait %s", budget, ackWait) } releaseCallbackOnce() select { @@ -2618,10 +2616,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 +2629,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) From b14207312fa6907f9ebd4358d8fe7aeeb0a14283 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 6 Oct 2026 22:40:38 -0400 Subject: [PATCH 13/13] test(fabric): pin the advisory fetch deadline near its floor (RIG-4786) Asserting only "above AckWait" let a 2x AckWait bound pass; the test now requires about the 5s floor and fails that mutant. Refs: RIG-4786 Co-authored-by: Matt Wilkinson --- go/internal/fabric/event_fabric_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/go/internal/fabric/event_fabric_test.go b/go/internal/fabric/event_fabric_test.go index cabdf2e3d..092414ac4 100644 --- a/go/internal/fabric/event_fabric_test.go +++ b/go/internal/fabric/event_fabric_test.go @@ -2601,8 +2601,9 @@ func TestCallbackRetriesAfterAdvisoryFetchFailure(t *testing.T) { case <-ctx.Done(): t.Fatal("advisory did not claim the park before fetching the message") } - if budget := <-fetchBudget; budget <= ackWait { - t.Fatalf("advisory fetch budget = %s, want greater than AckWait %s", budget, ackWait) + // 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 {