diff --git a/go/internal/delivery/consumer.go b/go/internal/delivery/consumer.go index 02652dfd7..d04dda3d5 100644 --- a/go/internal/delivery/consumer.go +++ b/go/internal/delivery/consumer.go @@ -52,8 +52,11 @@ type ControlDispatcher interface { // runnerhub.Hub implements it. type SessionResolver interface { // SessionForAccount returns the live session bound to account, or ok=false - // when the account has no live session (deliver falls to the D2 sweep). - SessionForAccount(account store.AccountID) (sessionID string, ok bool) + // when the account has no live session (deliver falls to the D2 sweep). ctx + // is threaded so the hub's read-through binding cache (RIG-3108) can scope a + // cache-miss table read: under the consumer's system-role ctx the read-through + // is refused and the miss falls to the sweep, exactly this method's contract. + SessionForAccount(ctx context.Context, account store.AccountID) (sessionID string, ok bool) // LiveAgentSessions snapshots every live (account -> session) binding — the // set the lag-resync sweep iterates so it redelivers to every live recipient. LiveAgentSessions() map[store.AccountID]string diff --git a/go/internal/delivery/dispatch.go b/go/internal/delivery/dispatch.go index 784d70340..e77435644 100644 --- a/go/internal/delivery/dispatch.go +++ b/go/internal/delivery/dispatch.go @@ -66,7 +66,7 @@ func (c *Consumer) onMessagePosted(ctx context.Context, msg *compassv1.Message) // otherwise deliver now, re-reading the settled blocks from the store (no // live turn to wait on) — mirroring fireHeld, never the posted (possibly // partial) wire message (design.md:177-178, :306). - authorSession, live := c.resolver.SessionForAccount(author) + authorSession, live := c.resolver.SessionForAccount(ctx, author) if !live { wire, channel, author, err := c.storeMessageToWire(ctx, messageID) if err != nil { @@ -128,7 +128,7 @@ func (c *Consumer) fanOut(ctx context.Context, channel store.ChannelID, author s if mentioned[agent] { continue // steer-only precedence: a mentioned agent never also gets a deliver } - sessionID, live := c.resolver.SessionForAccount(agent) + sessionID, live := c.resolver.SessionForAccount(ctx, agent) if !live { c.wake(ctx, agent) // best-effort resume; the D2 sweep is the durable backstop continue @@ -166,7 +166,7 @@ func (c *Consumer) routeMentionsFor(ctx context.Context, channel store.ChannelID mentioned := c.resolveMentioned(ctx, channel, author, handles) fromHandle := c.authorHandle(ctx, msg) for agent := range mentioned { - sessionID, live := c.resolver.SessionForAccount(agent) + sessionID, live := c.resolver.SessionForAccount(ctx, agent) if live { c.dispatchSteerTo(ctx, sessionID, msg, fromHandle) continue @@ -183,7 +183,7 @@ func (c *Consumer) routeMentionsFor(ctx context.Context, channel store.ChannelID // The no-loss edge: an owed mention that fails to record is lost. c.log.ErrorContext(ctx, "delivery: record owed mention for offline out-of-sweep-set member", "error", err, "agent", string(agent), "channel", string(channel), "message_id", msg.GetId()) - } else if sessionID, live := c.resolver.SessionForAccount(agent); live { + } else if sessionID, live := c.resolver.SessionForAccount(ctx, agent); live { // Now-live between the first resolve and the record: steer directly, // closing the record-vs-wake race. c.dispatchSteerTo(ctx, sessionID, msg, fromHandle) @@ -259,7 +259,7 @@ func (c *Consumer) routeAskAnswerFor(ctx context.Context, channel store.ChannelI // this is the latency path). The owed sweep dispatches as a STEER, so the // direct dispatch matches — both render through the same T6 ask_answer arm // and dedup by msg.id absorbs any overlap. - if sessionID, live := c.resolver.SessionForAccount(asker); live { + if sessionID, live := c.resolver.SessionForAccount(ctx, asker); live { c.dispatchSteerTo(ctx, sessionID, msg, c.authorHandle(ctx, msg)) } } diff --git a/go/internal/delivery/helpers_test.go b/go/internal/delivery/helpers_test.go index 1af028242..3e483771c 100644 --- a/go/internal/delivery/helpers_test.go +++ b/go/internal/delivery/helpers_test.go @@ -206,7 +206,7 @@ func newFakeResolver() *fakeResolver { return &fakeResolver{sessions: map[store.AccountID]string{}} } -func (r *fakeResolver) SessionForAccount(account store.AccountID) (string, bool) { +func (r *fakeResolver) SessionForAccount(_ context.Context, account store.AccountID) (string, bool) { r.mu.Lock() defer r.mu.Unlock() s, ok := r.sessions[account] diff --git a/go/internal/runnerhub/binding_cache_test.go b/go/internal/runnerhub/binding_cache_test.go new file mode 100644 index 000000000..40279e077 --- /dev/null +++ b/go/internal/runnerhub/binding_cache_test.go @@ -0,0 +1,618 @@ +//go:build unix + +package runnerhub + +// RIG-3108 / RIG-2861 §T4 — the session-binding read-through cache. The hub's +// in-RAM sessionAccounts/accountSessions maps are now a cache over the durable +// session_bindings table (a SessionBindingStore), invalidated across Server +// instances by a RoutingFabric. These white-box tests drive the unexported +// binding lifecycle (promoteSession/unbindSession/enroll) and the two resolvers +// (accountForSession + SessionForAccount) against hand-written fakes — the same +// fake-double style as deliveryarm_test.go — so each pins one contract a +// plausible regression would break: +// +// - a Server RESTART resolves a pre-restart session from the durable row, +// both directions; +// - a stopped / never-seen / post-RECONNECT session fails closed; +// - a binding change on one instance evicts a peer instance's cache; +// - a displaced session (an account re-pointed onto a new one) resolves +// nowhere; +// - the ack path's binding read never runs under the BYPASSRLS system role. +// +// The durable SQL itself is proven in store/session_bindings_pgtest_test.go; the +// cross-instance fabric fan-out in fabric/routing_fabric_test.go. These tests own +// the HUB's cache logic — the read-through gate, the reap, the eviction wiring — +// which neither of those exercises. + +import ( + "context" + "errors" + "fmt" + "sync" + "testing" + + "github.com/RigelBuild/compass/go/internal/fabric" + "github.com/RigelBuild/compass/go/internal/store" +) + +// fakeBindingStore is an in-memory SessionBindingStore double keyed on session +// id, modelling the durable table's account-keyed UPSERT: RecordSessionBinding +// displaces any prior binding for the same account and returns that session, and +// DeleteSessionBindingsForRunner returns the rows it removed (the reconnect +// sweep's authoritative reap set). It also models session_bindings_session_key: +// re-binding a session id that belongs to a DIFFERENT account is ErrConflict, +// never a silent steal. resolveCtxSystemRole records whether the LAST +// ResolveSessionAccount ran under the system role — the direct probe for the +// ack-path hazard fix. Concurrency-safe for parity with the real store. +type fakeBindingStore struct { + mu sync.Mutex + tenant store.TenantID + bindings map[string]store.SessionBinding // session id -> binding + + resolveCalled bool + resolveCtxSystemRole bool + recordErr error + resolveErr error + reverseErr error + deleteForRunnerErr error +} + +func newFakeBindingStore() *fakeBindingStore { + return &fakeBindingStore{tenant: "tenant-a", bindings: map[string]store.SessionBinding{}} +} + +func (f *fakeBindingStore) RecordSessionBinding(_ context.Context, sessionID string, accountID store.AccountID, runnerID string) (string, error) { + f.mu.Lock() + defer f.mu.Unlock() + if f.recordErr != nil { + return "", f.recordErr + } + // session_bindings_session_key: this session id already belongs to a + // DIFFERENT account. The real store raises ErrConflict rather than + // stealing the id, which is what keeps ResolveSessionAccount + // single-valued; a fake that silently overwrote would hide the whole + // class (a Runner restart re-mints "sess-1", so id reuse is routine). + if b, ok := f.bindings[sessionID]; ok && b.AccountID != accountID { + return "", fmt.Errorf("%w: session %q is already bound to a different agent", store.ErrConflict, sessionID) + } + var displaced string + // Account-keyed UPSERT: a prior binding for this account is displaced. + for sid, b := range f.bindings { + if b.AccountID == accountID && sid != sessionID { + displaced = sid + delete(f.bindings, sid) + break + } + } + f.bindings[sessionID] = store.SessionBinding{SessionID: sessionID, AccountID: accountID, RunnerID: runnerID} + return displaced, nil +} + +func (f *fakeBindingStore) ResolveSessionAccount(ctx context.Context, sessionID string) (store.AccountID, error) { + f.mu.Lock() + defer f.mu.Unlock() + f.resolveCalled = true + f.resolveCtxSystemRole = store.IsSystemRole(ctx) + if f.resolveErr != nil { + return "", f.resolveErr + } + b, ok := f.bindings[sessionID] + if !ok { + return "", store.ErrNotFound + } + return b.AccountID, nil +} + +func (f *fakeBindingStore) SessionForAccount(_ context.Context, accountID store.AccountID) (string, error) { + f.mu.Lock() + defer f.mu.Unlock() + if f.reverseErr != nil { + return "", f.reverseErr + } + for sid, b := range f.bindings { + if b.AccountID == accountID { + return sid, nil + } + } + return "", store.ErrNotFound +} + +func (f *fakeBindingStore) DeleteSessionBinding(_ context.Context, sessionID string) error { + f.mu.Lock() + defer f.mu.Unlock() + delete(f.bindings, sessionID) + return nil +} + +func (f *fakeBindingStore) DeleteSessionBindingsForRunner(_ context.Context, runnerID string) ([]store.SessionBinding, error) { + f.mu.Lock() + defer f.mu.Unlock() + if f.deleteForRunnerErr != nil { + return nil, f.deleteForRunnerErr + } + var removed []store.SessionBinding + for sid, b := range f.bindings { + if b.RunnerID == runnerID { + removed = append(removed, b) + delete(f.bindings, sid) + } + } + return removed, nil +} + +func (f *fakeBindingStore) EffectiveTenant(context.Context) store.TenantID { + f.mu.Lock() + defer f.mu.Unlock() + return f.tenant +} + +// testRunnerID is the single Runner every fake binding is seeded under. +const testRunnerID = "runner-1" + +// seed inserts a binding directly (test setup), bypassing the displacement path. +func (f *fakeBindingStore) seed(sessionID string) { + f.mu.Lock() + defer f.mu.Unlock() + f.bindings[sessionID] = store.SessionBinding{SessionID: sessionID, AccountID: testAgentAccount, RunnerID: testRunnerID} +} + +// fakeRoutingFabric is an in-memory RoutingFabric double: PublishBindingChange +// fans SYNCHRONOUSLY to every registered receiver, modelling the real fabric's +// fan-out-to-every-instance contract (fabric/routing_fabric_test.go proves the +// NATS wire; this proves the HUB's publish->peer-evict wiring without a broker). +// Synchronous fan-out is what lets a two-instance test assert eviction with no +// sleep. published records every change for the publish-side assertions. +type fakeRoutingFabric struct { + mu sync.Mutex + receivers []func(fabric.BindingChange) + published []fabric.BindingChange +} + +// PublishBindingChange mirrors the real fabric's REJECTION contract before +// recording: (*Fabric).PublishBindingChange drops a change that fails +// BindingChange.valid() (empty tenant or session id, an op outside +// bound/unbound) and one whose Tenant disagrees with the subject tenant, and +// the hub only LOGS that error — so a malformed publish would silently leave +// every peer cache stale. A fake that accepted anything could not see it. +func (f *fakeRoutingFabric) PublishBindingChange(_ context.Context, tenant string, b fabric.BindingChange) error { + if tenant == "" || b.Tenant != tenant { + return fmt.Errorf("publish on tenant %q with change tenant %q: must agree and be non-empty", tenant, b.Tenant) + } + if b.SessionID == "" { + return fmt.Errorf("binding change for tenant %q has an empty session id", tenant) + } + if b.Op != fabric.BindingBound && b.Op != fabric.BindingUnbound { + return fmt.Errorf("binding change %s/%s has op %q, want %q or %q", b.Tenant, b.SessionID, b.Op, fabric.BindingBound, fabric.BindingUnbound) + } + f.mu.Lock() + f.published = append(f.published, b) + recv := make([]func(fabric.BindingChange), len(f.receivers)) + copy(recv, f.receivers) + f.mu.Unlock() + for _, fn := range recv { + fn(b) + } + return nil +} + +func (f *fakeRoutingFabric) subscribe(fn func(fabric.BindingChange)) { + f.mu.Lock() + defer f.mu.Unlock() + f.receivers = append(f.receivers, fn) +} + +func (f *fakeRoutingFabric) publishedSnapshot() []fabric.BindingChange { + f.mu.Lock() + defer f.mu.Unlock() + out := make([]fabric.BindingChange, len(f.published)) + copy(out, f.published) + return out +} + +func runnerSubject() store.Subject { + return store.Subject{Kind: store.SubjectRunner, ID: "runner-1"} +} + +// TestRestartResolvesPreRestartBindingBothDirections is the availability +// property this whole PR exists for. A Server restart brings up a FRESH hub with +// EMPTY maps, then the Runner (whose sessions are still alive) enrolls for the +// FIRST time on that hub — so no durable reap runs and the pre-restart rows +// survive. Both resolvers must then fall through the cache miss to the durable +// table: accountForSession (session->account) AND SessionForAccount +// (account->session), the two directions delivery + comms depend on. +func TestRestartResolvesPreRestartBindingBothDirections(t *testing.T) { + hub := newHubOnly() + bindings := newFakeBindingStore() + bindings.seed("sess-1") + hub.SetSessionBindingStore(bindings) + + // A FIRST enroll on a fresh hub (the restart case): reattached is false, so + // enroll does NOT reap the durable rows — they are still valid. + if reattached := hub.enroll(context.Background(), "runner-1", runnerSubject()); reattached { + t.Fatal("first enroll on a fresh hub reported reattached=true, want false (no durable reap)") + } + + // Forward: session -> account, resolved from the durable row (map is empty). + account, ok := hub.accountForSession(context.Background(), "sess-1") + if !ok || account != testAgentAccount { + t.Fatalf("accountForSession(sess-1) after restart = (%q, %v), want (%s, true) — the pre-restart binding must resolve from the durable table", account, ok, testAgentAccount) + } + // Reverse: account -> session, the delivery consumer's direction. + sessionID, ok := hub.SessionForAccount(context.Background(), testAgentAccount) + if !ok || sessionID != "sess-1" { + t.Fatalf("SessionForAccount(%s) after restart = (%q, %v), want (sess-1, true) — the reverse binding must resolve from the durable table too", testAgentAccount, sessionID, ok) + } +} + +// TestFailClosedStoppedNeverSeenAndPostReconnect pins all three fail-closed +// misses in one place, each returning ok=false (the CodeNotFound the caller +// mints): a STOPPED session, a NEVER-SEEN session, and a session that predates a +// Runner RECONNECT. The reconnect case is the crux of the read-through design: +// the durable rows SURVIVE a process death, so a naive "cache miss -> read the +// table" would resolve a dead session; the re-enroll's durable reap +// (DeleteSessionBindingsForRunner) is what keeps it fail-closed. +func TestFailClosedStoppedNeverSeenAndPostReconnect(t *testing.T) { + hub := newHubOnly() + bindings := newFakeBindingStore() + hub.SetSessionBindingStore(bindings) + hub.enroll(context.Background(), "runner-1", runnerSubject()) + + // Never-seen: no binding anywhere. + if acct, ok := hub.accountForSession(context.Background(), "ghost"); ok { + t.Fatalf("accountForSession(ghost) = (%q, true), want ok=false (never seen)", acct) + } + + // Stopped: bound, then Stop's unbindSession removes it (durable delete too). + hub.bindContainer("cont-1", testAgentAccount) + hub.promoteSession(context.Background(), "cont-1", "sess-stop") + if acct, ok := hub.accountForSession(context.Background(), "sess-stop"); !ok || acct != testAgentAccount { + t.Fatalf("accountForSession(sess-stop) before stop = (%q, %v), want (%s, true)", acct, ok, testAgentAccount) + } + hub.unbindSession(context.Background(), "sess-stop") + if acct, ok := hub.accountForSession(context.Background(), "sess-stop"); ok { + t.Fatalf("accountForSession(sess-stop) after stop = (%q, true), want ok=false (stopped, durable row deleted)", acct) + } + + // Post-reconnect: a live binding whose row SURVIVES process death, then the + // Runner reconnects (a re-enroll on the SAME hub). The re-enroll durably + // reaps the rows, so the pre-reconnect session must fail closed even though a + // row existed a moment ago. + bindings.seed("sess-pre") + // Populate the cache so the assertion is not merely a cold miss. + if acct, ok := hub.accountForSession(context.Background(), "sess-pre"); !ok || acct != testAgentAccount { + t.Fatalf("accountForSession(sess-pre) before reconnect = (%q, %v), want (%s, true)", acct, ok, testAgentAccount) + } + // Runner reconnects: a re-enroll (reattached=true) durably reaps. + if reattached := hub.enroll(context.Background(), "runner-1", runnerSubject()); !reattached { + t.Fatal("second enroll reported reattached=false, want true (a Runner reconnect)") + } + if acct, ok := hub.accountForSession(context.Background(), "sess-pre"); ok { + t.Fatalf("accountForSession(sess-pre) after reconnect = (%q, true), want ok=false — the reconnect reap must fail-close a pre-reconnect session", acct) + } + if _, ok := bindings.bindings["sess-pre"]; ok { + t.Fatal("the durable row for sess-pre survived the reconnect reap; DeleteSessionBindingsForRunner must have removed it") + } +} + +// TestPeerBindingChangeEvictsOtherInstanceCache is the two-instance property: +// every Server keeps its own cache, so a binding change on ONE must invalidate +// the others. hubB re-points an account onto a new session; its promoteSession +// publishes BindingUnbound(old)+BindingBound(new) over the shared fabric, and +// hubA — subscribed to the fabric via OnBindingChange — must drop its now-stale +// cache entry for the old session. Without the subscribe-side eviction hubA +// would serve the stale binding forever, with no error to reveal it. +func TestPeerBindingChangeEvictsOtherInstanceCache(t *testing.T) { + routing := &fakeRoutingFabric{} + + // Instance A: holds a cached binding for sess-old and receives peer changes. + hubA := newHubOnly() + routing.subscribe(hubA.OnBindingChange) + hubA.mu.Lock() + hubA.sessionAccounts["sess-old"] = testAgentAccount + hubA.accountSessions[testAgentAccount] = "sess-old" + hubA.mu.Unlock() + + // Instance B: the writer. A durable store seeded with the same account->old + // binding (so recording the new one displaces it), an enrolled Runner (so the + // record fires), and the shared routing fabric. + hubB := newHubOnly() + bindingsB := newFakeBindingStore() + bindingsB.seed("sess-old") + hubB.SetSessionBindingStore(bindingsB) + hubB.SetRoutingFabric(routing) + hubB.enroll(context.Background(), "runner-1", runnerSubject()) + hubB.bindContainer("cont-new", testAgentAccount) + + // B re-points the account onto sess-new: displaces sess-old, publishes the + // two changes, which fan synchronously to hubA.OnBindingChange. + hubB.promoteSession(context.Background(), "cont-new", "sess-new") + + // A published both an unbound-old and a bound-new (the publish-side contract). + pub := routing.publishedSnapshot() + var sawUnboundOld, sawBoundNew bool + for _, c := range pub { + if c.SessionID == "sess-old" && c.Op == fabric.BindingUnbound { + sawUnboundOld = true + } + if c.SessionID == "sess-new" && c.Op == fabric.BindingBound { + sawBoundNew = true + } + } + if !sawUnboundOld || !sawBoundNew { + t.Fatalf("promote published %+v, want a BindingUnbound(sess-old) and a BindingBound(sess-new)", pub) + } + + // The receive-side property: hubA evicted its stale sess-old entry. hubA has + // no store and no enrolled Runner, so a resolved sess-old could only come + // from the cache — ok=false proves the eviction landed. + if acct, ok := hubA.accountForSession(context.Background(), "sess-old"); ok { + t.Fatalf("hubA.accountForSession(sess-old) after peer re-point = (%q, true), want ok=false — the peer's BindingUnbound must have evicted hubA's cache", acct) + } +} + +// TestDisplacedSessionResolvesNowhere pins the displacement contract on a SINGLE +// instance: promoting an account onto a NEW session displaces the account's +// PRIOR session, which must then resolve nowhere (the account moved off it). The +// store's RecordSessionBinding returns the displaced session; promoteSession +// evicts it from the forward map. Without that eviction sess-old would keep +// resolving to an account it no longer speaks for. +func TestDisplacedSessionResolvesNowhere(t *testing.T) { + hub := newHubOnly() + bindings := newFakeBindingStore() + hub.SetSessionBindingStore(bindings) + hub.enroll(context.Background(), "runner-1", runnerSubject()) + + // Bind the account to sess-old. + hub.bindContainer("cont-old", testAgentAccount) + hub.promoteSession(context.Background(), "cont-old", "sess-old") + if acct, ok := hub.accountForSession(context.Background(), "sess-old"); !ok || acct != testAgentAccount { + t.Fatalf("accountForSession(sess-old) = (%q, %v), want (%s, true)", acct, ok, testAgentAccount) + } + + // Re-point the SAME account onto sess-new: displaces sess-old. + hub.bindContainer("cont-new", testAgentAccount) + hub.promoteSession(context.Background(), "cont-new", "sess-new") + + // sess-new resolves; sess-old resolves nowhere. + if acct, ok := hub.accountForSession(context.Background(), "sess-new"); !ok || acct != testAgentAccount { + t.Fatalf("accountForSession(sess-new) = (%q, %v), want (%s, true)", acct, ok, testAgentAccount) + } + if acct, ok := hub.accountForSession(context.Background(), "sess-old"); ok { + t.Fatalf("accountForSession(sess-old) after displacement = (%q, true), want ok=false — the displaced session must resolve nowhere", acct) + } + // And the reverse points only at the new session. + if sess, ok := hub.SessionForAccount(context.Background(), testAgentAccount); !ok || sess != "sess-new" { + t.Fatalf("SessionForAccount(%s) = (%q, %v), want (sess-new, true)", testAgentAccount, sess, ok) + } +} + +// TestAckPathBindingReadNeverRunsUnderSystemRole is the security property PR3 +// closes. deliverAck and forgeNotificationAck escalate ctx to the BYPASSRLS +// system role for the cursor advance — but the binding read must run BEFORE that +// escalation, on the request ctx, or a cache-miss table read could return a +// plausible row from an ARBITRARY tenant +// (store/session_bindings_pgtest_test.go::TestSessionForAccountUnderSystemRoleIsUnscoped). +// This forces a cache MISS (so the read-through table read actually runs) and +// asserts the ctx that read saw was NOT system-role, for BOTH ack arms. +func TestAckPathBindingReadNeverRunsUnderSystemRole(t *testing.T) { + // deliverAck arm. + t.Run("delivery_ack", func(t *testing.T) { + hub := newHubOnly() + bindings := newFakeBindingStore() + bindings.seed("sess-1") + hub.SetSessionBindingStore(bindings) + del := newFakeDeliveryStore() + del.channels["m1"] = "chan-1" + hub.SetDeliveryStore(del) + // Enrolled Runner + empty maps (no bindSession) => the ack's account + // resolve is a cache MISS that falls through to the read-through table. + hub.enroll(context.Background(), "runner-1", runnerSubject()) + + if err := hub.Deliver(context.Background(), RunnerEvent{ + RunnerSeq: 1, SessionID: "sess-1", Frame: deliveryAckFrame("m1"), + }); err != nil { + t.Fatalf("Deliver(delivery_ack) = %v, want nil", err) + } + + if !bindings.resolveCalled { + t.Fatal("the ack path never read the binding table; the cache-miss read-through did not run, so this test proves nothing") + } + if bindings.resolveCtxSystemRole { + t.Fatal("the ack path's binding read ran under the system role — a BYPASSRLS read can return a foreign tenant's row; it must resolve on the request ctx BEFORE escalation") + } + // And the ack still applied (the resolve fed a real cursor advance). + if acks := del.ackSnapshot(); len(acks) != 1 || acks[0].agent != testAgentAccount { + t.Fatalf("ack cursor advances = %+v, want exactly one for %s", acks, testAgentAccount) + } + }) + + // forgeNotificationAck arm. + t.Run("forge_notification_ack", func(t *testing.T) { + hub := newHubOnly() + bindings := newFakeBindingStore() + bindings.seed("sess-1") + hub.SetSessionBindingStore(bindings) + del := newFakeDeliveryStore() + hub.SetDeliveryStore(del) + hub.enroll(context.Background(), "runner-1", runnerSubject()) + + if err := hub.Deliver(context.Background(), RunnerEvent{ + RunnerSeq: 1, SessionID: "sess-1", Frame: forgeAckFrame("sub-1"), + }); err != nil { + t.Fatalf("Deliver(forge_notification_ack) = %v, want nil", err) + } + + if !bindings.resolveCalled { + t.Fatal("the forge-ack path never read the binding table; the cache-miss read-through did not run") + } + if bindings.resolveCtxSystemRole { + t.Fatal("the forge-ack path's binding read ran under the system role — it must resolve on the request ctx BEFORE escalation") + } + if adv := del.forgeSnapshot(); len(adv) != 1 || adv[0].agent != testAgentAccount || adv[0].revision != testForgeRevision { + t.Fatalf("forge cursor advances = %+v, want exactly one (%s, sub-1, rev-1)", adv, testAgentAccount) + } + }) +} + +// TestStoreFaultsFallBackWithoutLosingFailClosed drives the four durable-fault +// branches the design's fail-closed reasoning rests on. Each fallback is a +// deliberate availability choice (a store fault must not fail a Start that +// already succeeded on the Runner, nor wedge a reconnect), and each is only +// safe because it degrades toward the in-RAM truth rather than toward an +// unbound session resolving. Nothing else in the suite sets these error fields. +func TestStoreFaultsFallBackWithoutLosingFailClosed(t *testing.T) { + faultErr := errors.New("durable fault") + + t.Run("promote with a record fault still resolves on this instance", func(t *testing.T) { + hub := newHubOnly() + bindings := newFakeBindingStore() + bindings.recordErr = faultErr + routing := &fakeRoutingFabric{} + hub.SetSessionBindingStore(bindings) + hub.SetRoutingFabric(routing) + hub.enroll(context.Background(), "runner-1", runnerSubject()) + + hub.bindContainer("cont-1", testAgentAccount) + hub.promoteSession(context.Background(), "cont-1", "sess-1") + + if acct, ok := hub.accountForSession(context.Background(), "sess-1"); !ok || acct != testAgentAccount { + t.Fatalf("accountForSession(sess-1) = (%q, %v), want (%s, true): a durable fault must not lose the live session", acct, ok, testAgentAccount) + } + // The write never landed, so there is nothing for a peer to invalidate. + if pub := routing.publishedSnapshot(); len(pub) != 0 { + t.Fatalf("published = %+v, want none: a failed durable write must not announce a binding that does not exist", pub) + } + }) + + t.Run("re-enroll with a reap fault still clears and fires offline", func(t *testing.T) { + hub := newHubOnly() + bindings := newFakeBindingStore() + hub.SetSessionBindingStore(bindings) + hub.enroll(context.Background(), "runner-1", runnerSubject()) + hub.bindContainer("cont-1", testAgentAccount) + hub.promoteSession(context.Background(), "cont-1", "sess-1") + + // The reconnect sweep now faults; the in-RAM snapshot must still drive it. + bindings.deleteForRunnerErr = faultErr + hub.enroll(context.Background(), "runner-1", runnerSubject()) + + if acct, ok := hub.accountForSession(context.Background(), "sess-1"); ok { + t.Fatalf("accountForSession(sess-1) = (%q, true) after a reconnect, want fail-closed: a reap fault must not leave a dead session resolvable", acct) + } + }) + + t.Run("a resolve fault fails closed", func(t *testing.T) { + hub := newHubOnly() + bindings := newFakeBindingStore() + bindings.seed("sess-1") + bindings.resolveErr = faultErr + hub.SetSessionBindingStore(bindings) + hub.enroll(context.Background(), "runner-1", runnerSubject()) + + if acct, ok := hub.accountForSession(context.Background(), "sess-1"); ok { + t.Fatalf("accountForSession(sess-1) = (%q, true), want fail-closed: a store fault must never resolve", acct) + } + }) + + t.Run("a reverse-resolve fault fails closed", func(t *testing.T) { + hub := newHubOnly() + bindings := newFakeBindingStore() + bindings.seed("sess-1") + bindings.reverseErr = faultErr + hub.SetSessionBindingStore(bindings) + hub.enroll(context.Background(), "runner-1", runnerSubject()) + + if sess, ok := hub.SessionForAccount(context.Background(), testAgentAccount); ok { + t.Fatalf("SessionForAccount = (%q, true), want fail-closed: a store fault must never resolve", sess) + } + }) +} + +// TestReusedSessionIDConflictIsSwallowed WITNESSES a known gap rather than a +// guarantee: production mints session ids with a counter that resets on every +// Runner restart, so a joint Server+Runner restart re-mints "sess-1" over a +// surviving row. The store answers ErrConflict to keep the binding +// single-valued; promoteSession currently treats it as a transient fault and +// falls back to RAM, leaving the durable row pointing at the OLD account. The +// single-instance MVP shadows that row and later retires it, but a second +// Server instance would read the stale durable truth. Pinned so the behaviour +// cannot change silently while the fix is decided (RIG-3108 review F1). +func TestReusedSessionIDConflictIsSwallowed(t *testing.T) { + hub := newHubOnly() + bindings := newFakeBindingStore() + hub.SetSessionBindingStore(bindings) + hub.enroll(context.Background(), "runner-1", runnerSubject()) + + // A row from before the joint restart, under a DIFFERENT account. + bindings.mu.Lock() + bindings.bindings["sess-1"] = store.SessionBinding{SessionID: "sess-1", AccountID: "acct-stale", RunnerID: "runner-1"} + bindings.mu.Unlock() + + hub.bindContainer("cont-1", testAgentAccount) + hub.promoteSession(context.Background(), "cont-1", "sess-1") + + // This instance resolves the NEW account from RAM. + if acct, ok := hub.accountForSession(context.Background(), "sess-1"); !ok || acct != testAgentAccount { + t.Fatalf("accountForSession(sess-1) = (%q, %v), want (%s, true)", acct, ok, testAgentAccount) + } + // ...but the durable row still names the stale account: the divergence. + bindings.mu.Lock() + got := bindings.bindings["sess-1"].AccountID + bindings.mu.Unlock() + if got != "acct-stale" { + t.Fatalf("durable binding for sess-1 = %q, want %q — if this now agrees with the cache, the ErrConflict gap was fixed and this witness test should become a real assertion", got, "acct-stale") + } +} + +// TestConcurrentResolveDuringAFaultingReapCannotResurrect fences the window +// between the re-enroll map-clear and the reap's return. The reap is a store +// round-trip that can block for seconds, and a resolver arriving in that gap +// misses the just-cleared cache — so if read-through were still permitted it +// would read back a row the failing reap never deleted, resurrecting a +// session the reconnect declared dead. blockingBindingStore holds the reap +// open until the concurrent resolve has run, which makes the interleaving +// deterministic rather than hoping the scheduler produces it. +func TestConcurrentResolveDuringAFaultingReapCannotResurrect(t *testing.T) { + hub := newHubOnly() + bindings := &blockingBindingStore{ + fakeBindingStore: newFakeBindingStore(), + entered: make(chan struct{}), + release: make(chan struct{}), + } + hub.SetSessionBindingStore(bindings) + hub.enroll(context.Background(), "runner-1", runnerSubject()) + hub.bindContainer("cont-1", testAgentAccount) + hub.promoteSession(context.Background(), "cont-1", "sess-1") + + // The reconnect's reap will fault, leaving the sess-1 row in place. + bindings.deleteForRunnerErr = errors.New("durable fault") + + done := make(chan struct{}) + go func() { + defer close(done) + hub.enroll(context.Background(), "runner-1", runnerSubject()) + }() + + <-bindings.entered // maps are cleared; the reap is in flight and will fail + acct, ok := hub.accountForSession(context.Background(), "sess-1") + close(bindings.release) + <-done + + if ok { + t.Fatalf("accountForSession(sess-1) = (%q, true) while a faulting reap was in flight, want fail-closed: the surviving row must not be readable between the map-clear and the reap's return", acct) + } +} + +// blockingBindingStore parks DeleteSessionBindingsForRunner so a test can act +// inside the reap window; every other method is the plain fake's. +type blockingBindingStore struct { + *fakeBindingStore + entered chan struct{} + release chan struct{} +} + +func (b *blockingBindingStore) DeleteSessionBindingsForRunner(ctx context.Context, runnerID string) ([]store.SessionBinding, error) { + close(b.entered) + <-b.release + return b.fakeBindingStore.DeleteSessionBindingsForRunner(ctx, runnerID) +} diff --git a/go/internal/runnerhub/commands.go b/go/internal/runnerhub/commands.go index 70bcd9e62..be3d17f60 100644 --- a/go/internal/runnerhub/commands.go +++ b/go/internal/runnerhub/commands.go @@ -78,7 +78,7 @@ func (h *Hub) Start(ctx context.Context, requestID string, req *compassv1.StartA // id the Runner minted, so RelayCommsCall for this session resolves the // agent account (comms-tools design T2). A container with no recorded // account leaves no session binding, and its comms calls fail closed. - h.promoteSession(req.GetContainerName(), resp.GetSessionId()) + h.promoteSession(ctx, req.GetContainerName(), resp.GetSessionId()) // The initial secret materialize no longer rides a signal: the Runner // materializes the container's set pre-exec at Start (host.Start, // FetchSecretsByContainer authorized on the Provision-time container→account @@ -99,7 +99,7 @@ func (h *Hub) Stop(ctx context.Context, requestID string, req *compassv1.StopAge // Drop the session's account binding: a RelayCommsCall for a stopped session // fails closed CodeNotFound, the same answer as a never-seen session — never // a stale reuse. - h.unbindSession(req.GetSessionId()) + h.unbindSession(ctx, req.GetSessionId()) return result.GetStop(), nil } diff --git a/go/internal/runnerhub/commands_test.go b/go/internal/runnerhub/commands_test.go index e380bdc9b..5be44ff5b 100644 --- a/go/internal/runnerhub/commands_test.go +++ b/go/internal/runnerhub/commands_test.go @@ -99,7 +99,7 @@ func TestStartRelaySurfacesAlreadyRunningAsAlreadyExists(t *testing.T) { hub := newHubOnly() // Enroll a Runner and bind a send that answers every command with an // ALREADY_RUNNING error result correlated by the pushed request id. - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) router, _, err := hub.routerFor("any") if err != nil { t.Fatalf("routerFor after enroll = %v, want a router", err) @@ -130,7 +130,7 @@ func TestStartRelaySurfacesAlreadyRunningAsAlreadyExists(t *testing.T) { // exercised. func TestStartRelayReturnsSessionIdOnSuccess(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) router, _, _ := hub.routerFor("any") router.attach(func(cmd *compassv1internal.SessionsResponse) error { go router.complete(&compassv1internal.SessionsRequest{ @@ -156,7 +156,7 @@ func TestStartRelayReturnsSessionIdOnSuccess(t *testing.T) { // from. A non-zero count means the initial-signal path was re-introduced. func TestStartEmitsNoInitialSignal(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) hub.bindContainer("c1", testAgentAccount) router, _, _ := hub.routerFor("any") rec := newRecordingSend() @@ -187,7 +187,7 @@ func TestStartEmitsNoInitialSignal(t *testing.T) { // variant, and the typed result flows back. func TestRemoveRelayReturnsResponseOnSuccess(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) router, _, _ := hub.routerFor("any") var sawRemove bool router.attach(func(cmd *compassv1internal.SessionsResponse) error { @@ -223,7 +223,7 @@ func TestRemoveRelayReturnsResponseOnSuccess(t *testing.T) { // true after teardown and reddens this. func TestRemoveClearsContainerBinding(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) hub.bindContainer("c1", testAgentAccount) if !hub.HasContainerBinding("c1") { t.Fatal("precondition: container c1 should be bound after bindContainer") @@ -250,7 +250,7 @@ func TestRemoveClearsContainerBinding(t *testing.T) { // the pushed request id — the seam the SessionState fallback tests drive. func attachStatusResponder(t *testing.T, hub *Hub, statuses []*compassv1.AgentSessionStatus) { t.Helper() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) router, _, err := hub.routerFor("any") if err != nil { t.Fatalf("routerFor after enroll = %v, want a router", err) diff --git a/go/internal/runnerhub/config_fetch_test.go b/go/internal/runnerhub/config_fetch_test.go index 6ec4c3461..191aae20f 100644 --- a/go/internal/runnerhub/config_fetch_test.go +++ b/go/internal/runnerhub/config_fetch_test.go @@ -83,7 +83,7 @@ func drainConfigStream(t *testing.T, stream *connect.ServerStreamForClient[compa // from the CodeUnavailable of a transient transport fault. func TestFetchAgentConfigNoConfigStoreFailsPrecondition(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) url := newMountedH2CServerWithConfig(t, hub, runnerResolverForFetch().resolve, nil) client := newRawRunnerClient(t, url, "runner-tok") @@ -108,7 +108,7 @@ func TestFetchAgentConfigNoConfigStoreFailsPrecondition(t *testing.T) { // loop is exercised (a bug that sent one frame, or dropped the tail, reds). func TestFetchAgentConfigStreamsVersionThenChunks(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) // A bundle spanning multiple chunk frames. want := bytes.Repeat([]byte("compass-config-"), configChunkBytes/10) cfg := &fakeConfigStore{version: "v-1", bundle: want} @@ -137,7 +137,7 @@ func TestFetchAgentConfigStreamsVersionThenChunks(t *testing.T) { // state (the Runner materializes an empty dir), never an error. func TestFetchAgentConfigUnconfiguredEmptyVersion(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) cfg := &fakeConfigStore{err: store.ErrNotFound} url := newMountedH2CServerWithConfig(t, hub, runnerResolverForFetch().resolve, cfg) client := newRawRunnerClient(t, url, "runner-tok") @@ -161,7 +161,7 @@ func TestFetchAgentConfigUnconfiguredEmptyVersion(t *testing.T) { // not read as "no config". func TestFetchAgentConfigStoreErrorMapsToInternal(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) cfg := &fakeConfigStore{err: errors.New("db boom")} url := newMountedH2CServerWithConfig(t, hub, runnerResolverForFetch().resolve, cfg) client := newRawRunnerClient(t, url, "runner-tok") @@ -186,7 +186,7 @@ func TestFetchAgentConfigStoreErrorMapsToInternal(t *testing.T) { // that streamed the bundle anyway (wasting the reconnect) reds. func TestFetchAgentConfigIfVersionMatchStreamsVersionOnly(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) cfg := &fakeConfigStore{version: "v-held", bundle: []byte("should-not-be-sent")} url := newMountedH2CServerWithConfig(t, hub, runnerResolverForFetch().resolve, cfg) client := newRawRunnerClient(t, url, "runner-tok") @@ -210,7 +210,7 @@ func TestFetchAgentConfigIfVersionMatchStreamsVersionOnly(t *testing.T) { // a stale Runner reconnecting with an old version still gets the new bytes. func TestFetchAgentConfigIfVersionMismatchStreamsBundle(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) cfg := &fakeConfigStore{version: "v-new", bundle: []byte("new-bytes")} url := newMountedH2CServerWithConfig(t, hub, runnerResolverForFetch().resolve, cfg) client := newRawRunnerClient(t, url, "runner-tok") diff --git a/go/internal/runnerhub/config_signal_test.go b/go/internal/runnerhub/config_signal_test.go index a58c7a4e4..5e5e860b8 100644 --- a/go/internal/runnerhub/config_signal_test.go +++ b/go/internal/runnerhub/config_signal_test.go @@ -11,6 +11,7 @@ package runnerhub // - No live sessions, and no Runner enrolled, are clean no-op successes. import ( + "context" "testing" compassv1internal "github.com/RigelBuild/compass/go/internal/gen/compass/v1" @@ -24,7 +25,7 @@ import ( // version), never a minted token. func TestSignalConfigVersionPushesStoreVersion(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) bindSession(hub, "sess-a") bindSession(hub, "sess-b") router, _, err := hub.routerFor("any") @@ -59,7 +60,7 @@ func TestSignalConfigVersionPushesStoreVersion(t *testing.T) { // fleet-cleared marker the Runner reads as "materialize an empty dir". func TestSignalConfigVersionEmptyVersionIsTheClearedMarker(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) bindSession(hub, "sess-a") router, _, err := hub.routerFor("any") if err != nil { @@ -86,7 +87,7 @@ func TestSignalConfigVersionEmptyVersionIsTheClearedMarker(t *testing.T) { // (nothing bound) pushes nothing and is a clean success. func TestSignalConfigVersionNoLiveSessionsIsNoop(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) router, _, _ := hub.routerFor("any") rec := newRecordingSend() router.attach(rec.send) diff --git a/go/internal/runnerhub/deliveryarm_test.go b/go/internal/runnerhub/deliveryarm_test.go index b6a3235f5..66cb1be08 100644 --- a/go/internal/runnerhub/deliveryarm_test.go +++ b/go/internal/runnerhub/deliveryarm_test.go @@ -349,7 +349,7 @@ func TestDeliverSessionNilPresenceSinkIsSafe(t *testing.T) { // DispatchControl returns without blocking. func TestDispatchControlSendOnlyDoesNotBlock(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) router, _, err := hub.routerFor("sess-1") if err != nil { t.Fatalf("routerFor: %v", err) @@ -400,7 +400,7 @@ func TestDispatchControlSendOnlyDoesNotBlock(t *testing.T) { // is observed (counted), not dropped as unknown. func TestDispatchControlNoLiveStreamRefuses(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) // No stream attached: send is nil. op := &compassv1internal.AgentControl{ Control: &compassv1internal.AgentControl_Deliver{ diff --git a/go/internal/runnerhub/enroll_reap_test.go b/go/internal/runnerhub/enroll_reap_test.go index 5eb952dc5..18fd6c6dd 100644 --- a/go/internal/runnerhub/enroll_reap_test.go +++ b/go/internal/runnerhub/enroll_reap_test.go @@ -12,6 +12,7 @@ package runnerhub // synchronously-recorded fact. import ( + "context" "slices" "sync" "testing" @@ -52,11 +53,11 @@ func TestEnrollFiresReapSinkWithClearedSessionIDs(t *testing.T) { hub.SetSessionReapSink(fake) // A first enroll binds the Runner, then two live sessions promote onto it. - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) hub.bindContainer("c1", "acct-a") - hub.promoteSession("c1", "sess-a") + hub.promoteSession(context.Background(), "c1", "sess-a") hub.bindContainer("c2", "acct-b") - hub.promoteSession("c2", "sess-b") + hub.promoteSession(context.Background(), "c2", "sess-b") // The first enroll fired the reap edge once with no ids (nothing was bound); // drop it so the assertion below covers only the re-enroll's reap. @@ -65,7 +66,7 @@ func TestEnrollFiresReapSinkWithClearedSessionIDs(t *testing.T) { } // The Runner reconnects: enroll clears both bindings and reaps both ids. - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) calls := fake.snapshot() if len(calls) != 2 { @@ -84,14 +85,14 @@ func TestEnrollFiresReapSinkWithClearedSessionIDs(t *testing.T) { func TestEnrollNilReapSinkStillClears(t *testing.T) { hub := newHubOnly() // no SetSessionReapSink - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) hub.bindContainer("c1", "acct-a") - hub.promoteSession("c1", "sess-a") + hub.promoteSession(context.Background(), "c1", "sess-a") // A re-enroll with no reap sink clears the binding without panicking. - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) - if sess, ok := hub.SessionForAccount("acct-a"); ok { + if sess, ok := hub.SessionForAccount(context.Background(), "acct-a"); ok { t.Fatalf("SessionForAccount(acct-a) = %q ok=true after re-enroll, want ok=false (binding cleared)", sess) } } diff --git a/go/internal/runnerhub/handler.go b/go/internal/runnerhub/handler.go index 2784fcc0a..943ec071a 100644 --- a/go/internal/runnerhub/handler.go +++ b/go/internal/runnerhub/handler.go @@ -82,7 +82,7 @@ func (h *Handler) Enroll(ctx context.Context, req *connect.Request[compassv1inte // enroll under an identity other than its token's. return nil, errUnauthenticated } - reattached := h.hub.enroll(subj.ID, subj) + reattached := h.hub.enroll(ctx, subj.ID, subj) return connect.NewResponse(&compassv1internal.EnrollResponse{Reattached: reattached}), nil } diff --git a/go/internal/runnerhub/helpers_test.go b/go/internal/runnerhub/helpers_test.go index 2631779d9..6e36fb6e5 100644 --- a/go/internal/runnerhub/helpers_test.go +++ b/go/internal/runnerhub/helpers_test.go @@ -169,7 +169,7 @@ const testAgentAccount store.AccountID = "acct-agent" func bindSession(hub *Hub, sessionID string) { container := "container-for-" + sessionID hub.bindContainer(container, testAgentAccount) - hub.promoteSession(container, sessionID) + hub.promoteSession(context.Background(), container, sessionID) } // commsCall records one CommsCaller invocation: the account the hub resolved diff --git a/go/internal/runnerhub/hub.go b/go/internal/runnerhub/hub.go index f95b82b16..2432cdd4b 100644 --- a/go/internal/runnerhub/hub.go +++ b/go/internal/runnerhub/hub.go @@ -22,6 +22,7 @@ import ( "sync/atomic" compassv1 "github.com/RigelBuild/compass/go/gen/compass/v1" + "github.com/RigelBuild/compass/go/internal/fabric" compassv1internal "github.com/RigelBuild/compass/go/internal/gen/compass/v1" "github.com/RigelBuild/compass/go/internal/store" ) @@ -171,6 +172,61 @@ type DeliveryStore interface { AdvanceForgeDeliveredRevision(ctx context.Context, agent store.AccountID, subscriptionID, revision string) error } +// SessionBindingStore is the durable session-binding surface the hub's maps are +// demoted to a read-through cache over (RIG-3108 / RIG-2861 §T4): the write path +// (record on promote, delete on unbind, sweep on re-enroll) and the two +// request-scoped reads a cache miss falls through to. *store.Store implements +// it; the hub depends only on this narrow surface (pattern: DeliveryStore). +// Wired via SetSessionBindingStore after construction so no NewHub caller +// signature changes, and nil-safe: a hub with no binding store keeps its maps as +// truth (today's behaviour, and every existing hub test) — writes and cache-miss +// reads simply do not touch a table. +type SessionBindingStore interface { + // RecordSessionBinding upserts the (account -> session, runner) binding and + // returns the session id it DISPLACED (empty when the account held none) — + // the prior session the hub must evict from both maps. It runs on the + // request ctx (tenant-scoped), so the write lands under the acting tenant. + RecordSessionBinding(ctx context.Context, sessionID string, accountID store.AccountID, runnerID string) (displaced string, err error) + // ResolveSessionAccount resolves the agent account a live session speaks + // for — the cache-miss read behind accountForSession. store.ErrNotFound is + // the fail-closed miss (mapped to ok=false), byte-identical to today's + // CodeNotFound. + ResolveSessionAccount(ctx context.Context, sessionID string) (store.AccountID, error) + // SessionForAccount resolves the live session bound to an account — the + // cache-miss read behind SessionForAccount (the reverse direction). Same + // fail-closed store.ErrNotFound contract. + SessionForAccount(ctx context.Context, accountID store.AccountID) (string, error) + // DeleteSessionBinding releases one session's binding — the unbind write. + // Idempotent: releasing an already-released session is a no-op success. + DeleteSessionBinding(ctx context.Context, sessionID string) error + // DeleteSessionBindingsForRunner is the reconnect sweep: it releases every + // binding attached to runnerID and RETURNS the rows it removed, driving the + // re-enroll reap (offline edges + held-deliver reap) from durable truth + // rather than the in-RAM snapshots. + DeleteSessionBindingsForRunner(ctx context.Context, runnerID string) ([]store.SessionBinding, error) + // EffectiveTenant names the tenant a request-scoped call resolves against, + // so a post-write BindingChange publishes on that tenant's routing subject. + EffectiveTenant(ctx context.Context) store.TenantID +} + +// RoutingFabric is the binding-cache invalidation seam (RIG-3108 / §T4): a +// post-write PublishBindingChange fans an invalidation to every OTHER Server +// instance, and SubscribeBindingChanges receives every instance's changes so +// this hub evicts a cached entry a peer re-pointed. fabric.Fabric implements it; +// the hub depends only on this narrow surface (pattern: DeliveryStore). It rides +// core NATS — best-effort at-most-once — because Postgres is the arbiter on any +// cache miss, so a dropped invalidation degrades to a cache-miss re-read. +// +// Wired via SetRoutingFabric after construction so no NewHub caller signature +// changes, and nil-safe: a single-instance hub (the MVP, and every existing +// test) wires none, so a binding change publishes nothing and this hub receives +// none — its own writes already keep its own cache honest. The subscribe wiring +// is the caller's (server assembly), so the hub exposes only the two verbs it +// drives. +type RoutingFabric interface { + PublishBindingChange(ctx context.Context, tenant string, b fabric.BindingChange) error +} + // TranscriptStore is the durable transcript surface the hub's commit arm writes // a relayed transcript_entry frame to (RIG-1667 T4, the durable counterpart to // the loss-tolerant Deliver path). *store.Store implements it; the hub depends @@ -322,6 +378,31 @@ type Hub struct { // ReconstructSessionBody closed CodeUnavailable — the resume read leg is not // mounted. reader TranscriptReader + // bindings is the durable session-binding store the maps below are a + // read-through cache over (RIG-3108 §T4). Nil until SetSessionBindingStore + // wires it; read under mu. Nil-safe: a hub with none wired keeps its maps as + // truth (today's behaviour) — no write and no cache-miss read touches a + // table, so every existing hub test is unchanged. + bindings SessionBindingStore + // routing is the binding-cache invalidation fabric a post-write + // PublishBindingChange fans over (RIG-3108 §T4). Nil until SetRoutingFabric + // wires it; read under mu. Nil-safe: a single-instance hub wires none, so a + // binding change publishes nothing — its own writes keep its own cache + // honest without a fabric round-trip. + routing RoutingFabric + // reapStale records that the table may still hold rows for sessions this + // hub has already declared dead: it is raised with the re-enroll map-clear + // and lowered only by a reap that succeeds. A read-through in between would + // resurrect one and break fail-closed, so readThroughAllowed refuses while + // it is set. Read and written under mu. + // + // Hub-wide is correct only under the single-Runner-id MVP (h.runner is + // never nilled and the id is the pinned token subject). A future + // multi-Runner change MUST key this by runner id: DeleteSessionBindingsForRunner + // targets one id, so a second runner's successful reap would otherwise + // lower the flag while the first runner's un-reaped rows survive and + // become readable again. + reapStale bool // lifecycleCaller is the spawn/despawn execution seam RelayLifecycleCall // delegates a resolved lifecycle call to (spawn/despawn record T4). Nil until // SetLifecycleCaller wires it (after both hub and lifecycleService exist, @@ -503,6 +584,27 @@ func (h *Hub) SetDeliveryStore(delivery DeliveryStore) { h.delivery = delivery } +// SetSessionBindingStore wires the durable session-binding store the hub's maps +// are a read-through cache over (RIG-3108 §T4), after construction so no NewHub +// caller signature changes. Called once at server assembly; nil-safe (a hub with +// none wired keeps its maps as truth — today's behaviour). Wired under mu; read +// under mu. +func (h *Hub) SetSessionBindingStore(bindings SessionBindingStore) { + h.mu.Lock() + defer h.mu.Unlock() + h.bindings = bindings +} + +// SetRoutingFabric wires the binding-cache invalidation fabric a post-write +// PublishBindingChange fans over (RIG-3108 §T4), after construction so no NewHub +// caller signature changes. Called once at server assembly; nil-safe (a +// single-instance hub wires none). Wired under mu; read under mu. +func (h *Hub) SetRoutingFabric(routing RoutingFabric) { + h.mu.Lock() + defer h.mu.Unlock() + h.routing = routing +} + // SetTranscriptStore wires the durable transcript store the commit arm writes a // relayed transcript_entry frame to (RIG-1667 T4), after construction so no // NewHub caller signature changes. Called once at server assembly; nil-safe (a @@ -569,7 +671,7 @@ func (h *Hub) Deliver(ctx context.Context, ev RunnerEvent) error { } switch f := oneof.(type) { case *compassv1internal.AgentFrame_Session: - h.deliverSession(ev.SessionID, f.Session) + h.deliverSession(ctx, ev.SessionID, f.Session) return nil case *compassv1internal.AgentFrame_DeliveryAck: h.deliverAck(ctx, ev, f.DeliveryAck) @@ -686,7 +788,7 @@ func (h *Hub) fireRunnerReady() { // the frame carries a lifecycle transition, extracts the AgentSessionStatus onto // SubscribeEvents. A session frame can carry a trace event, a lifecycle // transition, or both; UNSPECIFIED means "trace only, no transition". -func (h *Hub) deliverSession(sessionID string, sf *compassv1internal.SessionFrame) { +func (h *Hub) deliverSession(ctx context.Context, sessionID string, sf *compassv1internal.SessionFrame) { h.tail.RelaySessionFrame(sessionID, sf) state := sf.GetState() if state == compassv1.AgentSessionState_AGENT_SESSION_STATE_UNSPECIFIED { @@ -699,7 +801,7 @@ func (h *Hub) deliverSession(sessionID string, sf *compassv1internal.SessionFram // unbindSession has not yet dropped it) carries its account; one published // after a Runner reconnect cleared the maps carries none (the stated residual // gap). accountForSession takes h.mu; deliverSession holds no lock here. - account, hasAccount := h.accountForSession(sessionID) + account, hasAccount := h.accountForSession(ctx, sessionID) status := &compassv1.AgentSessionStatus{SessionId: sessionID, State: state} if hasAccount { status.AgentAccountId = string(account) @@ -746,11 +848,6 @@ func (h *Hub) deliverSession(sessionID string, sf *compassv1internal.SessionFram // ack must not kill the Runner's whole event stream. A nil delivery store (a // Deliver-only hub) drops the ack silently: no cursor exists to advance. func (h *Hub) deliverAck(ctx context.Context, ev RunnerEvent, ack *compassv1internal.DeliveryAck) { - // N5/OQ-4: the delivery-ack cursor advance is a cross-tenant system path — - // the ack resolves the acking agent's tenant only implicitly, and the cursor - // tables are RLS-policied, so this runs under the BYPASSRLS system role - // rather than a fail-closed request scope that would drop every ack. - ctx = store.WithSystemRole(ctx) h.mu.Lock() delivery := h.delivery h.mu.Unlock() @@ -762,25 +859,41 @@ func (h *Hub) deliverAck(ctx context.Context, ev RunnerEvent, ack *compassv1inte h.countDroppedAck(ev, "delivery_ack carries no message id") return } - agent, ok := h.accountForSession(ev.SessionID) + // RIG-3108 hazard fix: resolve the acking session's account on the REQUEST + // ctx — BEFORE the system-role escalation below. The binding read is a + // read-through cache over session_bindings, whose row is single-valued only + // because RLS narrows it to one tenant; a BYPASSRLS (system-role) read could + // return a plausible row from an ARBITRARY tenant + // (store/session_bindings_pgtest_test.go::TestSessionForAccountUnderSystemRoleIsUnscoped). + // Resolving here keeps the binding read tenant-scoped and fail-closed; only + // the cursor advance below — a cross-tenant system path by N5/OQ-4 — runs + // under the system role. A cache miss with the ctx still request-scoped is + // exactly the correct scoping for the fallback table read. + agent, ok := h.accountForSession(ctx, ev.SessionID) if !ok { h.countDroppedAck(ev, "no agent account bound to the acking session") return } + // N5/OQ-4: the delivery-ack cursor advance is a cross-tenant system path — + // the cursor tables are RLS-policied and the ack's tenant is only implicit, + // so the advance runs under the BYPASSRLS system role rather than a + // fail-closed request scope that would drop every ack. Escalate ONLY now, + // after the binding is resolved, so the binding read never inherited it. + sysCtx := store.WithSystemRole(ctx) // MessageChannel only resolves ANY message's channel; it is NOT the // membership/owed clamp. store.AckDelivery is the clamp: it resolves // messageID WHERE id=$1 AND channel_id=$2 and only UPDATEs an existing // seeded cursor (never inserts), so the message must resolve for this // (agent, channel) there or the ack is a no-op — a future reader must not // mistake MessageChannel for the guard. - channel, err := delivery.MessageChannel(ctx, messageID) + channel, err := delivery.MessageChannel(sysCtx, messageID) if err != nil { // An unknown or foreign message id: fail-closed no-op, never a teardown. // A fabricated id cannot advance a cursor; the resolution IS the guard. h.countDroppedAck(ev, "delivery_ack for an unresolvable message: "+err.Error()) return } - if err := delivery.AckDelivery(ctx, agent, channel, messageID); err != nil { + if err := delivery.AckDelivery(sysCtx, agent, channel, messageID); err != nil { // A store fault advancing the cursor: log + count and drop. A missed ack // costs only a redundant redeliver on the recipient's next reconnect // sweep (the cursor stays where it was), so it never justifies tearing @@ -807,11 +920,6 @@ func (h *Hub) deliverAck(ctx context.Context, ev RunnerEvent, ack *compassv1inte // from the durable gap. A nil delivery store (a Deliver-only hub) drops the ack // silently: no cursor exists to advance. func (h *Hub) forgeNotificationAck(ctx context.Context, ev RunnerEvent, ack *compassv1internal.ForgeNotificationAck) { - // N5/OQ-4: like deliverAck, the forge-delivery cursor advance is a - // cross-tenant system path — RLS-policied agent_forge_subscriptions rows - // advanced from a Runner event whose tenant is only implicit. Run under the - // BYPASSRLS system role rather than a fail-closed request scope. - ctx = store.WithSystemRole(ctx) h.mu.Lock() delivery := h.delivery h.mu.Unlock() @@ -823,12 +931,21 @@ func (h *Hub) forgeNotificationAck(ctx context.Context, ev RunnerEvent, ack *com h.countDroppedAck(ev, "forge_notification_ack carries no subscription id") return } - agent, ok := h.accountForSession(ev.SessionID) + // RIG-3108 hazard fix (mirrors deliverAck): resolve the acking session's + // account on the REQUEST ctx, BEFORE the system-role escalation, so the + // binding read stays tenant-scoped and cannot return a foreign tenant's row + // under BYPASSRLS. Only the cursor advance below runs under the system role. + agent, ok := h.accountForSession(ctx, ev.SessionID) if !ok { h.countDroppedAck(ev, "no agent account bound to the acking session") return } - if err := delivery.AdvanceForgeDeliveredRevision(ctx, agent, subscriptionID, ack.GetRevision()); err != nil { + // N5/OQ-4: the forge-delivery cursor advance is a cross-tenant system path — + // RLS-policied agent_forge_subscriptions rows advanced from a Runner event + // whose tenant is only implicit. Escalate ONLY now, after the binding is + // resolved on the request ctx. + sysCtx := store.WithSystemRole(ctx) + if err := delivery.AdvanceForgeDeliveredRevision(sysCtx, agent, subscriptionID, ack.GetRevision()); err != nil { // A store fault (or the ErrNotFound of a subscription unsubscribed // mid-flight / owned by a different agent) advancing the cursor: log + // count and drop. A missed advance costs only a redundant re-notify on @@ -902,21 +1019,47 @@ type promotedPair struct { // re-minted id under the fresh Runner session resolves CodeNotFound until it is // bound anew, never a stale account. Single-Runner MVP — every binding belongs // to the one enrolled Runner, so a reconnect clears the whole map. -func (h *Hub) enroll(id string, subject store.Subject) (reattached bool) { +// +// RIG-3108 — durable reap vs durable survival, the invariant conflict this +// method resolves. The maps are now a read-through cache over session_bindings, +// and the rows SURVIVE a process death. Two cases, distinguished by reattached: +// +// - A RE-ENROLL (reattached: this live hub already had a Runner, and it +// reconnected). Its sessions are dead, so fail-closed requires the rows gone +// too — otherwise a cache miss would fall through to the table and resolve a +// dead session (the naive read-through breaks fail-closed). So a re-enroll +// DURABLY reaps: DeleteSessionBindingsForRunner deletes every row for this +// Runner and RETURNS them, and those returned rows — not the in-RAM snapshot +// — drive the presence-OFFLINE edges and the held-deliver reap. The durable +// delete is what makes the durable read safe. +// - A FIRST enroll (!reattached: a fresh hub, i.e. a Server restart with the +// Runner and its sessions still live). Here the pre-restart rows are VALID +// and must survive so a comms call resolves the session from the durable +// binding — the availability property this whole PR exists for. So a first +// enroll does NOT durably reap; it only clears the (empty) maps. +// +// The reap runs under the request ctx (the Enroll RPC's), tenant-scoped by RLS +// — the one binding mutation not already inside a request read, made an +// explicitly request-scoped call rather than the system role, so it cannot reach +// another tenant's rows. +// +// A hub with no binding store wired keeps the original in-RAM snapshot behaviour +// (every existing enroll test), driving offline/reapedSessions from the maps. +func (h *Hub) enroll(ctx context.Context, id string, subject store.Subject) (reattached bool) { h.mu.Lock() reattached = h.runner != nil router := newCommandRouter() router.log = h.log h.runner = &attachedRunner{id: id, subject: subject, router: router} - // Snapshot the live (account -> session) bindings BEFORE clearing them: each - // previously-bound account loses its live session on this re-enroll and must - // be driven to presence OFFLINE (RIG-1569 T8). enroll emits no lifecycle - // frames of its own, so without this a long-WORKING agent whose Runner - // reconnected would stay WORKING in the projection forever. A first-ever - // enroll (empty maps) snapshots nothing and fires nothing. - offline := make([]promotedPair, 0, len(h.accountSessions)) + // Snapshot the live (account -> session) bindings BEFORE clearing them, for + // the no-store path: each previously-bound account loses its live session on + // this re-enroll and must be driven to presence OFFLINE (RIG-1569 T8). enroll + // emits no lifecycle frames of its own, so without this a long-WORKING agent + // whose Runner reconnected would stay WORKING in the projection forever. A + // first-ever enroll (empty maps) snapshots nothing and fires nothing. + ramOffline := make([]promotedPair, 0, len(h.accountSessions)) for account, sessionID := range h.accountSessions { - offline = append(offline, promotedPair{account: account, sessionID: sessionID}) + ramOffline = append(ramOffline, promotedPair{account: account, sessionID: sessionID}) } // Snapshot the session ids whose bindings are about to be cleared, so the // delivery consumer can reap any held-deliver registry entries a no-frame @@ -924,17 +1067,59 @@ func (h *Hub) enroll(id string, subject store.Subject) (reattached bool) { // is keyed by session id, and Consumer.held is keyed by that same author // session id, so these are exactly the keys to drop. A first-ever enroll // (empty map) snapshots nothing. - reapedSessions := make([]string, 0, len(h.sessionAccounts)) + ramReaped := make([]string, 0, len(h.sessionAccounts)) for sessionID := range h.sessionAccounts { - reapedSessions = append(reapedSessions, sessionID) + ramReaped = append(ramReaped, sessionID) } + bindings := h.bindings presence := h.presence reap := h.reap clear(h.containerAccounts) clear(h.sessionAccounts) clear(h.accountSessions) + // Refuse read-through from the instant the maps are cleared, not after the + // reap returns: the reap is a round-trip that can block for seconds, and a + // concurrent resolver in that gap would miss the cleared cache and read a + // not-yet-deleted row back, resurrecting a session this reconnect just + // declared dead. Pessimistic-true costs nothing even when the reap + // succeeds — the maps are empty, so the only durable-but-uncached rows are + // the dead survivors, and a session promoted after the reconnect writes its + // cache entry and hits. + if bindings != nil && reattached { + h.reapStale = true + } h.mu.Unlock() + // Choose the reap set. A re-enroll with a durable store reaps the ROWS and + // drives the edges from what it removed (the authoritative set); everything + // else falls back to the in-RAM snapshot taken above. + offline := ramOffline + reapedSessions := ramReaped + if bindings != nil && reattached { + rows, err := bindings.DeleteSessionBindingsForRunner(ctx, id) + if err != nil { + // A durable-reap fault must not wedge the reconnect: log it and fall + // back to the in-RAM snapshot (still cleared above), so the presence + // edges and held-deliver reap fire from what the cache last knew. + // The rows SURVIVE, and they name sessions just declared dead, so + // the pessimistic reapStale raised with the clear STAYS raised: a + // read-through would resurrect one and defeat fail-closed. + h.log.Error("durable session-binding reap failed on re-enroll; using in-RAM snapshot, read-through disabled until a reap succeeds", + "runner_id", id, "error", err) + } else { + offline = make([]promotedPair, 0, len(rows)) + reapedSessions = make([]string, 0, len(rows)) + for _, b := range rows { + offline = append(offline, promotedPair{account: b.AccountID, sessionID: b.SessionID}) + reapedSessions = append(reapedSessions, b.SessionID) + } + // The table now agrees with the cleared cache again. + h.mu.Lock() + h.reapStale = false + h.mu.Unlock() + } + } + // Fire the terminal edges AFTER releasing the lock (the sink enqueues into // the presence loop and returns promptly, so it must not run under h.mu) — // the exact lock-then-release-then-fire discipline promoteSession uses. diff --git a/go/internal/runnerhub/hub_test.go b/go/internal/runnerhub/hub_test.go index defb3e06e..48894092b 100644 --- a/go/internal/runnerhub/hub_test.go +++ b/go/internal/runnerhub/hub_test.go @@ -167,10 +167,10 @@ func TestEnrollDuplicateReattaches(t *testing.T) { hub := newHubOnly() subj := store.Subject{Kind: store.SubjectRunner, ID: "runner-1"} - if reattached := hub.enroll("runner-1", subj); reattached { + if reattached := hub.enroll(context.Background(), "runner-1", subj); reattached { t.Fatal("first enroll reattached = true, want false (fresh registration)") } - if reattached := hub.enroll("runner-1", subj); !reattached { + if reattached := hub.enroll(context.Background(), "runner-1", subj); !reattached { t.Fatal("second enroll reattached = false, want true (single-Runner MVP re-attaches)") } // A router is resolvable after enrollment (a session command has a Runner to diff --git a/go/internal/runnerhub/provision_dedup_test.go b/go/internal/runnerhub/provision_dedup_test.go index e618b1e68..46f6b90cd 100644 --- a/go/internal/runnerhub/provision_dedup_test.go +++ b/go/internal/runnerhub/provision_dedup_test.go @@ -52,7 +52,7 @@ type provisionOutcome struct { // Hub.Provision through the real router. func enrollAttached(t *testing.T, hub *Hub, send *recordingSend) *commandRouter { t.Helper() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) router, _, err := hub.routerFor("any") if err != nil { t.Fatalf("routerFor after enroll = %v, want the live router", err) diff --git a/go/internal/runnerhub/relay_board.go b/go/internal/runnerhub/relay_board.go index 9bddd06f2..81f7070bb 100644 --- a/go/internal/runnerhub/relay_board.go +++ b/go/internal/runnerhub/relay_board.go @@ -65,7 +65,7 @@ func (h *Hub) RelayBoardCall( //nolint:dupl // deliberate structural mirror of R if caller == nil { return nil, connect.NewError(connect.CodeUnavailable, errBoardUnavailable) } - account, ok := h.accountForSession(req.GetSessionId()) + account, ok := h.accountForSession(ctx, req.GetSessionId()) if !ok { // Fail closed: no live session maps to this id. Never a stale account, // never the bootstrap admin — a hard CodeNotFound the Runner surfaces. diff --git a/go/internal/runnerhub/relay_comms.go b/go/internal/runnerhub/relay_comms.go index 4b475a4c2..19d8bfdba 100644 --- a/go/internal/runnerhub/relay_comms.go +++ b/go/internal/runnerhub/relay_comms.go @@ -25,6 +25,7 @@ import ( "go.opentelemetry.io/otel/trace" compassv1 "github.com/RigelBuild/compass/go/gen/compass/v1" + "github.com/RigelBuild/compass/go/internal/fabric" compassv1internal "github.com/RigelBuild/compass/go/internal/gen/compass/v1" otelx "github.com/RigelBuild/compass/go/internal/otel" "github.com/RigelBuild/compass/go/internal/store" @@ -49,7 +50,18 @@ func (h *Hub) bindContainer(containerName string, agentAccountID store.AccountID // session id. If the container had no recorded account (a provision that named // none, or a container from before this leg existed), no session binding is // created and a later comms call for that session fails closed CodeNotFound. -func (h *Hub) promoteSession(containerName, sessionID string) { +// +// RIG-3108: the maps are a read-through cache, so the durable binding is written +// FIRST (RecordSessionBinding, on the request ctx so it lands tenant-scoped), +// and only then are the maps updated under h.mu. The store returns the session +// this account DISPLACED — the prior session now resolving nowhere — which is +// evicted from the forward map here (without it, sess-old would keep resolving +// to the account it no longer speaks for) and invalidated on peer instances via +// a BindingUnbound publish. A BindingBound publish for the new session lets peer +// instances drop any stale cache entry for it. The store write and the publish +// both run with h.mu RELEASED — never hold the lock across a store call or a +// sink — exactly the lock-then-store-then-map discipline the design requires. +func (h *Hub) promoteSession(ctx context.Context, containerName, sessionID string) { if containerName == "" || sessionID == "" { return } @@ -59,6 +71,43 @@ func (h *Hub) promoteSession(containerName, sessionID string) { h.mu.Unlock() return } + // Capture the store handle, the routing fabric, and the enrolled Runner id + // under the lock, then release BEFORE the store write: the binding row names + // the Runner the session is attached to (the sweep key a re-enroll retires + // it by), and a promote always follows a relay through that Runner, so one is + // enrolled. A nil store or an (unexpected) empty runner id keeps the maps as + // truth — today's behaviour — writing no durable row. + bindings := h.bindings + routing := h.routing + var runnerID string + if h.runner != nil { + runnerID = h.runner.id + } + h.mu.Unlock() + + // Write the durable binding first (h.mu released). The store upsert is keyed + // on the account, so it lands whether or not the account held a prior + // session, and it returns the displaced session id. + var displaced string + tenant := "" + if bindings != nil && runnerID != "" { + d, err := bindings.RecordSessionBinding(ctx, sessionID, account, runnerID) + if err != nil { + // A durable-write fault must not fail the Start that already + // succeeded on the Runner: log it and fall back to the in-RAM cache + // so the session resolves at least on this instance. The next + // re-enroll sweep or a cache-miss re-read reconciles against the + // table. + h.log.Error("record session binding failed; falling back to in-RAM cache", + "session_id", sessionID, "account", string(account), "error", err) + } else { + displaced = d + tenant = string(bindings.EffectiveTenant(ctx)) + } + } + + // Now update the maps under h.mu (store already written). + h.mu.Lock() h.sessionAccounts[sessionID] = account h.accountSessions[account] = sessionID // The container->account entry has served its purpose; the session binding @@ -66,6 +115,13 @@ func (h *Hub) promoteSession(containerName, sessionID string) { // Runner's life cannot resurrect a stale account (reconnect clears both maps // anyway; this keeps the pre-Start map tight in the meantime). delete(h.containerAccounts, containerName) + // Evict the displaced session from the forward map: the account moved off it, + // so it now resolves nowhere. Guard displaced != sessionID for the rebind + // case (an account re-pointed onto the SAME session displaces itself, and + // dropping the entry just written would unbind the live session). + if displaced != "" && displaced != sessionID { + delete(h.sessionAccounts, displaced) + } // Read both after-binding sinks under mu so a setter and this arm never race, // then release BEFORE firing either: each sink only enqueues into its own // consumer/component loop and returns promptly, so promoteSession never blocks @@ -75,6 +131,18 @@ func (h *Hub) promoteSession(containerName, sessionID string) { sessionStart := h.sessionStart presence := h.presence h.mu.Unlock() + + // Invalidate peer instances' caches (h.mu released, nil-safe, best-effort): + // the displaced session has no row any more (BindingUnbound), and the new + // session's binding changed (BindingBound). A single-instance hub wires no + // routing fabric, so this is a no-op there. + if routing != nil && tenant != "" { + if displaced != "" && displaced != sessionID { + h.publishBindingChange(ctx, routing, tenant, displaced, fabric.BindingUnbound) + } + h.publishBindingChange(ctx, routing, tenant, sessionID, fabric.BindingBound) + } + if sessionStart != nil { sessionStart.OnSessionStarted(sessionID, account) } @@ -90,6 +158,22 @@ func (h *Hub) promoteSession(containerName, sessionID string) { } } +// publishBindingChange fans one binding invalidation to peer instances over the +// routing fabric (RIG-3108 §T4), logging a publish failure rather than +// propagating it: the fabric rides core NATS (at-most-once), and a dropped +// invalidation degrades to a peer's cache-miss re-read against Postgres — the +// arbiter — so a publish failure never fails the operation that caused it. +func (h *Hub) publishBindingChange(ctx context.Context, routing RoutingFabric, tenant, sessionID string, op fabric.BindingOp) { + if err := routing.PublishBindingChange(ctx, tenant, fabric.BindingChange{ + Tenant: tenant, + SessionID: sessionID, + Op: op, + }); err != nil { + h.log.Warn("publish binding change failed (peers re-read on cache miss)", + "tenant", tenant, "session_id", sessionID, "op", string(op), "error", err) + } +} + // unbindSession removes a session's account binding. Called from Stop, so a // RelayCommsCall for a stopped session_id fails closed CodeNotFound — the same // answer as a never-seen session, never a stale reuse. @@ -106,7 +190,32 @@ func (h *Hub) promoteSession(containerName, sessionID string) { // Capture the account under mu, release, then fire the sink — the exact // lock-then-release-then-fire discipline promoteSession uses, so the sink (which // enqueues into the presence loop) never runs under h.mu. -func (h *Hub) unbindSession(sessionID string) { +func (h *Hub) unbindSession(ctx context.Context, sessionID string) { + // RIG-3108: the maps are a cache, so the durable row is deleted FIRST + // (DeleteSessionBinding, on the request ctx so it stays tenant-scoped), then + // the maps are evicted under h.mu. DeleteSessionBinding is by session id and + // idempotent — a session promoteSession already displaced has no row (the + // account row now names the newer session), so a stale release matches + // nothing and leaves the live binding alone, exactly the re-point guard the + // map eviction below keeps. + h.mu.Lock() + bindings := h.bindings + routing := h.routing + h.mu.Unlock() + + tenant := "" + if bindings != nil { + if err := bindings.DeleteSessionBinding(ctx, sessionID); err != nil { + // A durable-delete fault must not fail the Stop that already + // succeeded on the Runner: log and continue to evict the cache. The + // next re-enroll sweep retires any surviving row. + h.log.Error("delete session binding failed; evicting cache anyway", + "session_id", sessionID, "error", err) + } else { + tenant = string(bindings.EffectiveTenant(ctx)) + } + } + h.mu.Lock() var ( account store.AccountID @@ -126,6 +235,13 @@ func (h *Hub) unbindSession(sessionID string) { presence := h.presence h.mu.Unlock() + // Invalidate peer instances' caches (h.mu released, nil-safe, best-effort): + // this session has no binding any more (BindingUnbound), so a peer can drop + // its entry outright. A single-instance hub wires no routing fabric. + if routing != nil && tenant != "" { + h.publishBindingChange(ctx, routing, tenant, sessionID, fabric.BindingUnbound) + } + // The account now has NO live session: drive its presence OFFLINE. Skipped // when the account was re-pointed to a newer session (wentOffline is false). // @@ -163,11 +279,68 @@ func (h *Hub) unbindContainer(containerName string) { // false when no live binding exists (never provisioned, stopped, or dropped on a // Runner reconnect) — the fail-closed signal RelayCommsCall turns into // CodeNotFound. -func (h *Hub) accountForSession(sessionID string) (store.AccountID, bool) { +// +// RIG-3108: the map is a read-through cache. A hit returns immediately. A miss +// falls through to the durable binding table ONLY when a Runner is currently +// enrolled AND the ctx is request-scoped — so a Server restart resolves a +// pre-restart session from the durable row, while a miss after a reconnect +// (which durably reaps every binding at enroll) stays a fail-closed miss. The +// enrolled-Runner gate is what keeps fail-closed correct across a reconnect: the +// reap deletes the rows, so even the table read would miss, but the gate makes +// the miss free of a table round-trip. store.ErrNotFound (and any store fault) +// maps to ok=false, so CodeNotFound behaviour is byte-identical to today. +// +// The read-through is refused under a system-role ctx: SessionBindingStore's +// reads are single-valued only because RLS narrows them to the acting tenant, +// and a BYPASSRLS read could return a plausible row from an ARBITRARY tenant +// (store/session_bindings_pgtest_test.go::TestSessionForAccountUnderSystemRoleIsUnscoped). +// The ack arms resolve BEFORE escalating to the system role, so they never reach +// this refusal; the guard is defence in depth against a future system-role +// caller. +func (h *Hub) accountForSession(ctx context.Context, sessionID string) (store.AccountID, bool) { h.mu.Lock() - defer h.mu.Unlock() - account, ok := h.sessionAccounts[sessionID] - return account, ok + if account, ok := h.sessionAccounts[sessionID]; ok { + h.mu.Unlock() + return account, true + } + bindings := h.bindings + enrolled := h.runner != nil + reapStale := h.reapStale + h.mu.Unlock() + + if !h.readThroughAllowed(ctx, bindings, enrolled, reapStale) { + return "", false + } + account, err := bindings.ResolveSessionAccount(ctx, sessionID) + if err != nil { + // store.ErrNotFound (an unbound session) and any store fault both fail + // closed — the same CodeNotFound the caller mints today. + return "", false + } + // Populate the forward cache so a subsequent comms call for this restarted + // session hits without a table round-trip. Re-check under the lock: a + // concurrent promote/unbind may have run between the release and here, so a + // live map entry wins over the row just read (avoids clobbering a fresher + // binding with a staler one). + h.mu.Lock() + if live, ok := h.sessionAccounts[sessionID]; ok { + h.mu.Unlock() + return live, true + } + h.sessionAccounts[sessionID] = account + h.mu.Unlock() + return account, true +} + +// readThroughAllowed reports whether a cache-miss binding read may fall through +// to the durable table: a store must be wired, a Runner must be currently +// enrolled (a miss with none enrolled means the reconnect reap cleared every +// binding — fail closed), the ctx must be request-scoped (a system-role read +// is the unscoped-row hazard, refused), and the last re-enroll's durable reap +// must not have faulted — rows it failed to delete name sessions this hub has +// already declared dead, so reading them back would resurrect them. +func (h *Hub) readThroughAllowed(ctx context.Context, bindings SessionBindingStore, enrolled, reapStale bool) bool { + return bindings != nil && enrolled && !reapStale && !store.IsSystemRole(ctx) } // SessionForAccount resolves the LIVE session bound to an agent account — the @@ -179,11 +352,40 @@ func (h *Hub) accountForSession(sessionID string) (store.AccountID, bool) { // delivery.SessionResolver interface the consumer holds, kept separate from the // ControlDispatcher (DispatchControl) so that stays the established dispatch-only // shape. -func (h *Hub) SessionForAccount(account store.AccountID) (string, bool) { +// +// RIG-3108: a read-through cache exactly as accountForSession is. A miss falls +// through to the durable table only when a Runner is enrolled AND the ctx is +// request-scoped. The delivery consumer's loop runs under the system role +// (delivery/consumer.go Run), so its resolve REFUSES the read-through and falls +// to the D2 cursor sweep — the consumer's own miss contract, unchanged. A +// request-scoped caller (a Server restart resolving a pre-restart recipient) +// resolves from the row. store.ErrNotFound and any store fault map to ok=false. +func (h *Hub) SessionForAccount(ctx context.Context, account store.AccountID) (string, bool) { h.mu.Lock() - defer h.mu.Unlock() - sessionID, ok := h.accountSessions[account] - return sessionID, ok + if sessionID, ok := h.accountSessions[account]; ok { + h.mu.Unlock() + return sessionID, true + } + bindings := h.bindings + enrolled := h.runner != nil + reapStale := h.reapStale + h.mu.Unlock() + + if !h.readThroughAllowed(ctx, bindings, enrolled, reapStale) { + return "", false + } + sessionID, err := bindings.SessionForAccount(ctx, account) + if err != nil { + return "", false + } + h.mu.Lock() + if live, ok := h.accountSessions[account]; ok { + h.mu.Unlock() + return live, true + } + h.accountSessions[account] = sessionID + h.mu.Unlock() + return sessionID, true } // LiveAgentSessions snapshots every live (agent account -> session) binding — the @@ -199,6 +401,46 @@ func (h *Hub) LiveAgentSessions() map[store.AccountID]string { return out } +// OnBindingChange evicts this instance's cache entry for the session named in a +// binding change received from a PEER instance over the routing fabric (RIG-3108 +// §T4). It is the subscribe-side counterpart to promoteSession/unbindSession's +// PublishBindingChange: a peer that re-pointed or released a session tells every +// other Server to drop its now-stale cache, and the next resolution re-reads the +// durable truth (Postgres is the arbiter). +// +// It NEVER trusts the change's contents as data (reference-never-payload): the +// change carries only a session id and an op, never the resolved account, so +// this drops the cached entry and lets the next accountForSession/SessionForAccount +// cache-miss re-read the table. BindingOp is an OPEN SET, so this handles ANY op +// — known or not — as the same invalidate-and-re-read: an unrecognized op still +// means the binding genuinely changed, and on a plane with no ack a drop would +// leave the entry stale with nothing to reveal it (fabric.BindingOp). There is +// no switch on the op here precisely because every value means the same thing. +// +// It never publishes: this is the RECEIVE side, so re-publishing would loop an +// invalidation around the fabric forever. +func (h *Hub) OnBindingChange(change fabric.BindingChange) { + sessionID := change.SessionID + if sessionID == "" { + // A change naming no session would invalidate cache key "" and read as + // "nothing changed"; the fabric decode already rejects this, so it is + // defence in depth. + return + } + h.mu.Lock() + defer h.mu.Unlock() + // Evict the forward entry and, if it still points back at this session, the + // reverse entry too — so neither direction serves a stale binding a peer just + // changed. The re-point guard mirrors unbindSession: a reverse entry the + // account has already moved onto a newer session is left alone. + if account, ok := h.sessionAccounts[sessionID]; ok { + if h.accountSessions[account] == sessionID { + delete(h.accountSessions, account) + } + delete(h.sessionAccounts, sessionID) + } +} + // HasLiveSession reports whether sessionID names a live session bound in the // hub. It mirrors accountForSession's lock discipline but discards the account — // the FetchSecrets authz check only needs "is this a session bound to the (one) @@ -257,7 +499,7 @@ func (h *Hub) RelayCommsCall( if h.comms == nil { return nil, connect.NewError(connect.CodeUnavailable, errCommsUnavailable) } - account, ok := h.accountForSession(req.GetSessionId()) + account, ok := h.accountForSession(ctx, req.GetSessionId()) if !ok { // Fail closed: no live session maps to this id. Never a stale account, // never the bootstrap admin — a hard CodeNotFound the Runner surfaces. @@ -331,7 +573,7 @@ func (h *Hub) CommitConversationFrame( return nil, connect.NewError(connect.CodeUnavailable, errTranscriptsUnavailable) } sessionID := req.GetSessionId() - if _, ok := h.accountForSession(sessionID); !ok { + if _, ok := h.accountForSession(ctx, sessionID); !ok { // Fail closed: no live session maps to this id. Never a stale account, // never the bootstrap admin — a hard CodeNotFound the Runner surfaces. return nil, connect.NewError( diff --git a/go/internal/runnerhub/relay_comms_test.go b/go/internal/runnerhub/relay_comms_test.go index 32682c156..222163700 100644 --- a/go/internal/runnerhub/relay_comms_test.go +++ b/go/internal/runnerhub/relay_comms_test.go @@ -63,7 +63,7 @@ func bindLiveSession(hub *Hub) { account = store.AccountID("acct-agent") ) hub.bindContainer(containerName, account) - hub.promoteSession(containerName, sessionID) + hub.promoteSession(context.Background(), containerName, sessionID) } // 1. An unknown session fails closed CodeNotFound and NEVER reaches the caller — @@ -270,7 +270,7 @@ func TestRelayCommsCallDropsBindingOnRunnerReconnect(t *testing.T) { } // The Runner reconnects (re-enroll), which drops ALL agent-comms bindings. - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) // The SAME session_id now fails closed — the binding is gone, so no stale // account is reachable. @@ -307,7 +307,7 @@ func TestRelayCommsCallStoppedSessionFailsClosedNotFound(t *testing.T) { } // Stop unbinds the session. - hub.unbindSession("sess-1") + hub.unbindSession(context.Background(), "sess-1") _, err := hub.RelayCommsCall(context.Background(), relayPost("sess-1", "tc-9", &compassv1.PostMessageRequest{ Blocks: []*compassv1.MessageBlock{{Block: &compassv1.MessageBlock_Text{Text: "after stop"}}}, @@ -330,7 +330,7 @@ func TestRelayCommsCallStoppedSessionFailsClosedNotFound(t *testing.T) { // helper. func TestProvisionThenStartBindsSessionToProvisionedAccount(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) router, _, err := hub.routerFor("any") if err != nil { t.Fatalf("routerFor after enroll = %v, want a router", err) @@ -363,7 +363,7 @@ func TestProvisionThenStartBindsSessionToProvisionedAccount(t *testing.T) { t.Fatalf("Start = %v, want success", err) } - account, ok := hub.accountForSession("sess-live") + account, ok := hub.accountForSession(context.Background(), "sess-live") if !ok { t.Fatal("accountForSession(sess-live) = not bound, want the provisioned account after Provision->Start") } @@ -377,7 +377,7 @@ func TestProvisionThenStartBindsSessionToProvisionedAccount(t *testing.T) { // RelayCommsCall fails closed CodeNotFound — never an empty-account attribution. func TestProvisionWithEmptyAccountLeavesNoBindingAndFailsClosed(t *testing.T) { hub, comms := newHubWithComms() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) router, _, err := hub.routerFor("any") if err != nil { t.Fatalf("routerFor after enroll = %v, want a router", err) @@ -408,7 +408,7 @@ func TestProvisionWithEmptyAccountLeavesNoBindingAndFailsClosed(t *testing.T) { t.Fatalf("Start = %v, want success", err) } - if _, ok := hub.accountForSession("sess-live"); ok { + if _, ok := hub.accountForSession(context.Background(), "sess-live"); ok { t.Fatal("accountForSession(sess-live) resolved a binding, want none (empty account must not bind)") } // And the fail-closed consequence: RelayCommsCall for that session is @@ -439,19 +439,19 @@ func TestProvisionWithEmptyAccountLeavesNoBindingAndFailsClosed(t *testing.T) { // re-enrolls, and asserts the reverse map is empty. func TestEnrollClearsReverseAccountSessions(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) bindLiveSession(hub) // acct-agent -> sess-1, via the real Provision->Start path // Sanity: the reverse map is populated before the re-enroll. - if _, ok := hub.SessionForAccount("acct-agent"); !ok { + if _, ok := hub.SessionForAccount(context.Background(), "acct-agent"); !ok { t.Fatal("SessionForAccount(acct-agent) not bound before re-enroll; the test setup is wrong") } // A Runner reconnect: enroll re-attaches and MUST drop every stale binding, // forward AND reverse. - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) - if sess, ok := hub.SessionForAccount("acct-agent"); ok { + if sess, ok := hub.SessionForAccount(context.Background(), "acct-agent"); ok { t.Fatalf("SessionForAccount(acct-agent) = %q, ok=true after re-enroll; want ok=false — enroll left a stale reverse entry, so a dead session resolves as live", sess) } if live := hub.LiveAgentSessions(); len(live) != 0 { @@ -470,9 +470,9 @@ func TestUnbindSessionFiresTerminalPresenceEdge(t *testing.T) { pres := &fakePresenceSink{} hub.SetPresenceSink(pres) hub.bindContainer("c1", "acct-a") - hub.promoteSession("c1", "sess-a") // fires one promoted edge, not a lifecycle one + hub.promoteSession(context.Background(), "c1", "sess-a") // fires one promoted edge, not a lifecycle one - hub.unbindSession("sess-a") + hub.unbindSession(context.Background(), "sess-a") life := pres.lifecycleSnapshot() if len(life) != 1 { @@ -494,21 +494,21 @@ func TestUnbindStaleSessionFiresNoTerminalEdgeWhenRepointed(t *testing.T) { pres := &fakePresenceSink{} hub.SetPresenceSink(pres) hub.bindContainer("c1", "acct-a") - hub.promoteSession("c1", "sess-old") + hub.promoteSession(context.Background(), "c1", "sess-old") // A new container/session promotes onto the SAME account, re-pointing the // reverse entry to sess-new (the newer live session). hub.bindContainer("c2", "acct-a") - hub.promoteSession("c2", "sess-new") + hub.promoteSession(context.Background(), "c2", "sess-new") // Unbind the stale session: its forward entry is dropped, but the reverse // entry now points at sess-new, so no terminal edge fires. - hub.unbindSession("sess-old") + hub.unbindSession(context.Background(), "sess-old") if life := pres.lifecycleSnapshot(); len(life) != 0 { t.Fatalf("lifecycle edges after stale unbind = %d, want 0 (account re-pointed, not offline): %+v", len(life), life) } // The live session's reverse binding survives the stale unbind. - if sess, ok := hub.SessionForAccount("acct-a"); !ok || sess != "sess-new" { + if sess, ok := hub.SessionForAccount(context.Background(), "acct-a"); !ok || sess != "sess-new" { t.Fatalf("SessionForAccount(acct-a) = %q ok=%v after stale unbind, want sess-new (live binding must survive)", sess, ok) } } @@ -523,15 +523,15 @@ func TestEnrollFiresTerminalPresenceEdgePerBoundAccountAndClears(t *testing.T) { hub := newHubOnly() pres := &fakePresenceSink{} hub.SetPresenceSink(pres) - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) hub.bindContainer("c1", "acct-a") - hub.promoteSession("c1", "sess-a") + hub.promoteSession(context.Background(), "c1", "sess-a") hub.bindContainer("c2", "acct-b") - hub.promoteSession("c2", "sess-b") + hub.promoteSession(context.Background(), "c2", "sess-b") // A Runner reconnect: enroll drops every binding and drives each previously- // bound account OFFLINE. - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) life := pres.lifecycleSnapshot() if len(life) != 2 { @@ -563,7 +563,7 @@ func TestFirstEnrollFiresNoTerminalPresenceEdge(t *testing.T) { pres := &fakePresenceSink{} hub.SetPresenceSink(pres) - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) if life := pres.lifecycleSnapshot(); len(life) != 0 { t.Fatalf("lifecycle edges after first enroll = %d, want 0 (nothing was bound): %+v", len(life), life) diff --git a/go/internal/runnerhub/relay_forge.go b/go/internal/runnerhub/relay_forge.go index 2b5bb90bc..48a4cb05e 100644 --- a/go/internal/runnerhub/relay_forge.go +++ b/go/internal/runnerhub/relay_forge.go @@ -89,7 +89,7 @@ func (h *Hub) RelayForgeCall( return nil, connect.NewError(connect.CodeUnavailable, errForgeUnavailable) } sessionID := req.GetSessionId() - account, ok := h.accountForSession(sessionID) + account, ok := h.accountForSession(ctx, sessionID) if !ok { // Fail closed: no live session maps to this id. Never a stale account, // never the bootstrap admin — a hard CodeNotFound the Runner surfaces. diff --git a/go/internal/runnerhub/relay_lifecycle.go b/go/internal/runnerhub/relay_lifecycle.go index 5d2976b98..2db08af84 100644 --- a/go/internal/runnerhub/relay_lifecycle.go +++ b/go/internal/runnerhub/relay_lifecycle.go @@ -64,7 +64,7 @@ func (h *Hub) RelayLifecycleCall( //nolint:dupl // deliberate structural mirror if caller == nil { return nil, connect.NewError(connect.CodeUnavailable, errLifecycleUnavailable) } - account, ok := h.accountForSession(req.GetSessionId()) + account, ok := h.accountForSession(ctx, req.GetSessionId()) if !ok { // Fail closed: no live session maps to this id. Never a stale account, // never the bootstrap admin — a hard CodeNotFound the Runner surfaces. diff --git a/go/internal/runnerhub/relay_operator_fault_test.go b/go/internal/runnerhub/relay_operator_fault_test.go index 325f826e0..2b0d328ca 100644 --- a/go/internal/runnerhub/relay_operator_fault_test.go +++ b/go/internal/runnerhub/relay_operator_fault_test.go @@ -24,7 +24,7 @@ import ( // See docs/designs/infra/runtime/compass-runner-gateway-error-sentinels/design.md. func TestProvisionRelaySurfacesOperatorFaultAsFailedPrecondition(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) router, _, err := hub.routerFor("any") if err != nil { t.Fatalf("routerFor after enroll = %v, want a router", err) diff --git a/go/internal/runnerhub/resume_start.go b/go/internal/runnerhub/resume_start.go index 6b8a62d92..6b7437707 100644 --- a/go/internal/runnerhub/resume_start.go +++ b/go/internal/runnerhub/resume_start.go @@ -40,6 +40,6 @@ func (h *Hub) StartResume(ctx context.Context, requestID string, req *compassv1. return nil, err } resp := result.GetStart() - h.promoteSession(req.GetContainerName(), resp.GetSessionId()) + h.promoteSession(ctx, req.GetContainerName(), resp.GetSessionId()) return resp, nil } diff --git a/go/internal/runnerhub/secrets_test.go b/go/internal/runnerhub/secrets_test.go index e0054f3b1..1df0fd830 100644 --- a/go/internal/runnerhub/secrets_test.go +++ b/go/internal/runnerhub/secrets_test.go @@ -68,7 +68,7 @@ func runnerResolverForFetch() *fakeResolver { // can never pull the secret set. func TestFetchSecretsUnboundSessionPermissionDenied(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) // No session bound: the hub has no live session for "sess-unbound". resolver := &fakeResolverSecrets{set: []secrets.ResolvedSecret{{Name: "A", Value: "v"}}} url := newMountedH2CServerWithResolver(t, hub, runnerResolverForFetch().resolve, resolver) @@ -92,7 +92,7 @@ func TestFetchSecretsUnboundSessionPermissionDenied(t *testing.T) { // delivery/kind enums translated at the edge. func TestFetchSecretsBoundSessionReturnsResolvedSet(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) bindSession(hub, "sess-1") resolver := &fakeResolverSecrets{set: []secrets.ResolvedSecret{ {Name: "DB_URL", Value: "postgres://secret", Version: "v1", Delivery: secrets.DeliveryEnv, Kind: secrets.SecretGeneric}, @@ -126,7 +126,7 @@ func TestFetchSecretsBoundSessionReturnsResolvedSet(t *testing.T) { // swallowed as an empty set. func TestFetchSecretsResolveErrorInternal(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) bindSession(hub, "sess-1") resolver := &fakeResolverSecrets{resolveErr: errors.New("resolve boom")} url := newMountedH2CServerWithResolver(t, hub, runnerResolverForFetch().resolve, resolver) @@ -146,7 +146,7 @@ func TestFetchSecretsResolveErrorInternal(t *testing.T) { // window, before any session) resolves the set via the container_name selector. func TestFetchSecretsByBoundContainerReturnsResolvedSet(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) hub.bindContainer("cont-1", testAgentAccount) resolver := &fakeResolverSecrets{set: []secrets.ResolvedSecret{{Name: "A", Value: "v", Version: "v1"}}} url := newMountedH2CServerWithResolver(t, hub, runnerResolverForFetch().resolve, resolver) @@ -166,7 +166,7 @@ func TestFetchSecretsByBoundContainerReturnsResolvedSet(t *testing.T) { // reached — the pre-exec analogue of the unbound-session rejection. func TestFetchSecretsUnboundContainerPermissionDenied(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) resolver := &fakeResolverSecrets{set: []secrets.ResolvedSecret{{Name: "A", Value: "v"}}} url := newMountedH2CServerWithResolver(t, hub, runnerResolverForFetch().resolve, resolver) client := newRawRunnerClient(t, url, "runner-tok") @@ -184,7 +184,7 @@ func TestFetchSecretsUnboundContainerPermissionDenied(t *testing.T) { // is CodeInvalidArgument — a contract skew, never a silent empty set. func TestFetchSecretsMissingSelectorInvalidArgument(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) resolver := &fakeResolverSecrets{set: []secrets.ResolvedSecret{{Name: "A", Value: "v"}}} url := newMountedH2CServerWithResolver(t, hub, runnerResolverForFetch().resolve, resolver) client := newRawRunnerClient(t, url, "runner-tok") @@ -203,7 +203,7 @@ func TestFetchSecretsMissingSelectorInvalidArgument(t *testing.T) { // ambiguous request (CodeInvalidArgument) rather than silently preferring one. func TestFetchSecretsBothSelectorsInvalidArgument(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) hub.bindContainer("cont-1", testAgentAccount) bindSession(hub, "sess-1") resolver := &fakeResolverSecrets{set: []secrets.ResolvedSecret{{Name: "A", Value: "v"}}} @@ -227,7 +227,7 @@ func TestFetchSecretsBothSelectorsInvalidArgument(t *testing.T) { // without also tolerating a transient outage as "no secrets". func TestFetchSecretsNoResolverFailedPrecondition(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) bindSession(hub, "sess-1") url := newMountedH2CServerWithResolver(t, hub, runnerResolverForFetch().resolve, nil) client := newRawRunnerClient(t, url, "runner-tok") @@ -262,7 +262,7 @@ func TestResolvedSecretMappingRedactsValue(t *testing.T) { // content hash — an opaque counter. func TestSignalSecretsVersionPushesMonotonicToken(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) bindSession(hub, "sess-a") bindSession(hub, "sess-b") router, _, err := hub.routerFor("any") @@ -319,7 +319,7 @@ func TestSignalSecretsVersionPushesMonotonicToken(t *testing.T) { // notify is not an error. func TestSignalSecretsVersionNoLiveSessionsIsNoop(t *testing.T) { hub := newHubOnly() - hub.enroll("runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) + hub.enroll(context.Background(), "runner-1", store.Subject{Kind: store.SubjectRunner, ID: "runner-1"}) router, _, _ := hub.routerFor("any") rec := newRecordingSend() router.attach(rec.send) diff --git a/go/internal/runnerhub/sessionstart_test.go b/go/internal/runnerhub/sessionstart_test.go index b5785931a..faeefe6ec 100644 --- a/go/internal/runnerhub/sessionstart_test.go +++ b/go/internal/runnerhub/sessionstart_test.go @@ -12,6 +12,7 @@ package runnerhub // synchronously-recorded fact. import ( + "context" "sync" "testing" @@ -61,7 +62,7 @@ func TestPromoteSessionFiresStartSink(t *testing.T) { // The Provision->Start promotion path: record the container's account, then // promote it onto the minted session id. hub.bindContainer("c1", testAgentAccount) - hub.promoteSession("c1", "sess-1") + hub.promoteSession(context.Background(), "c1", "sess-1") got := sink.snapshot() if len(got) != 1 { @@ -82,13 +83,13 @@ func TestPromoteSessionNoBindingFiresNothing(t *testing.T) { hub.SetSessionStartSink(sink) // No bindContainer: the container has no recorded account. - hub.promoteSession("c-unknown", "sess-1") + hub.promoteSession(context.Background(), "c-unknown", "sess-1") if got := sink.snapshot(); len(got) != 0 { t.Fatalf("start edges = %d, want 0 (a non-binding promotion sweeps nothing)", len(got)) } // And no live binding was created. - if _, ok := hub.SessionForAccount(testAgentAccount); ok { + if _, ok := hub.SessionForAccount(context.Background(), testAgentAccount); ok { t.Fatal("a non-binding promotion created a live session binding, want none") } } @@ -100,14 +101,14 @@ func TestPromoteSessionNilStartSinkStillBinds(t *testing.T) { hub := newHubOnly() // no SetSessionStartSink hub.bindContainer("c1", testAgentAccount) - hub.promoteSession("c1", "sess-1") + hub.promoteSession(context.Background(), "c1", "sess-1") // The binding is live in both directions — promoteSession did its job with no // sink wired. - if acct, ok := hub.accountForSession("sess-1"); !ok || acct != testAgentAccount { + if acct, ok := hub.accountForSession(context.Background(), "sess-1"); !ok || acct != testAgentAccount { t.Fatalf("accountForSession(sess-1) = (%q, %v), want (%s, true)", acct, ok, testAgentAccount) } - if sess, ok := hub.SessionForAccount(testAgentAccount); !ok || sess != "sess-1" { + if sess, ok := hub.SessionForAccount(context.Background(), testAgentAccount); !ok || sess != "sess-1" { t.Fatalf("SessionForAccount(%s) = (%q, %v), want (sess-1, true)", testAgentAccount, sess, ok) } } diff --git a/go/internal/store/tenant.go b/go/internal/store/tenant.go index 9705be6df..09c520cf8 100644 --- a/go/internal/store/tenant.go +++ b/go/internal/store/tenant.go @@ -57,3 +57,16 @@ func (s *Store) resolveTenant(ctx context.Context) TenantID { } return s.bootstrapTenantID } + +// EffectiveTenant returns the tenant a request-scoped call resolves against: +// the context tenant if the auth layer set one, else the bootstrap tenant (the +// OSS single-tenant degenerate path). It is the exported form of resolveTenant, +// for a caller that must name the tenant a binding was written under — the +// RIG-3108 hub, which publishes a BindingChange on the per-tenant routing +// subject after recording a binding on the request ctx. A system-role ctx +// carries no tenant, so the bootstrap fallback applies there too; the hub never +// records a binding under the system role, so that degenerate value is never +// published. +func (s *Store) EffectiveTenant(ctx context.Context) TenantID { + return s.resolveTenant(ctx) +} diff --git a/go/internal/store/tenant_tx.go b/go/internal/store/tenant_tx.go index 1d27b0275..3bf135da6 100644 --- a/go/internal/store/tenant_tx.go +++ b/go/internal/store/tenant_tx.go @@ -55,6 +55,15 @@ func isSystemRole(ctx context.Context) bool { return v } +// IsSystemRole reports whether ctx is the cross-tenant system path (the exported +// form of isSystemRole). A caller that must NOT run a request-scoped read under +// the system role — the RIG-3108 binding cache, whose reads resolve the +// principal a comms call runs under and must stay tenant-scoped — gates on this +// rather than reading the table unscoped under BYPASSRLS. +func IsSystemRole(ctx context.Context) bool { + return isSystemRole(ctx) +} + // scopedDBTX is the db.DBTX the store's *db.Queries is bound to in place of the // bare pool. It wraps the pgxpool and, on EVERY statement, prepends the tenant // scoping — SET LOCAL ROLE + (on the request path) set_config(tenantGUC, ...) — diff --git a/go/server/forge_notify_dispatch_test.go b/go/server/forge_notify_dispatch_test.go index f9de1b926..c8fc0b41b 100644 --- a/go/server/forge_notify_dispatch_test.go +++ b/go/server/forge_notify_dispatch_test.go @@ -30,7 +30,7 @@ type fakeSessionDispatcher struct { dispatchErr error // returned by DispatchControl (nil = success) } -func (f *fakeSessionDispatcher) SessionForAccount(account store.AccountID) (string, bool) { +func (f *fakeSessionDispatcher) SessionForAccount(_ context.Context, account store.AccountID) (string, bool) { s, ok := f.binding[account] return s, ok } diff --git a/go/server/forge_notify_e2e_pgtest_test.go b/go/server/forge_notify_e2e_pgtest_test.go index 19e2ad8b0..eb4357092 100644 --- a/go/server/forge_notify_e2e_pgtest_test.go +++ b/go/server/forge_notify_e2e_pgtest_test.go @@ -254,7 +254,7 @@ func (w *notifyE2EWire) goLive(t *testing.T, account store.AccountID, container, } // The account->session binding must resolve, or the dispatcher would fall to // errNoLiveSession and drop the notification. - if got, ok := w.hub.SessionForAccount(account); !ok || got != session { + if got, ok := w.hub.SessionForAccount(context.Background(), account); !ok || got != session { t.Fatalf("SessionForAccount(%s) = (%q, %v), want (%q, true)", account, got, ok, session) } } diff --git a/go/server/lifecycle.go b/go/server/lifecycle.go index 5c4d8c747..4dedfa505 100644 --- a/go/server/lifecycle.go +++ b/go/server/lifecycle.go @@ -101,7 +101,7 @@ func (l *lifecycleService) WakeAgent(ctx context.Context, agent store.AccountID) // 1. Not-live pre-check (cost control): a live agent is already awake, so // there is nothing to resume. No-op, no log line — a wake is only an attempt // against an OFFLINE agent. - if _, live := l.hub.SessionForAccount(agent); live { + if _, live := l.hub.SessionForAccount(ctx, agent); live { return } @@ -336,7 +336,7 @@ func (l *lifecycleService) DespawnAsAccount( // Authorized. Stop the target's live session first (best-effort, bounded so a // wedged Runner cannot starve the Remove below); skip if none is live. - if sessionID, ok := l.hub.SessionForAccount(target); ok { + if sessionID, ok := l.hub.SessionForAccount(ctx, target); ok { stopCtx, stopCancel := context.WithTimeout(ctx, rollbackStopTimeout) if _, err := l.hub.Stop(stopCtx, "", &compassv1.StopAgentSessionRequest{SessionId: sessionID}); err != nil { slog.ErrorContext(ctx, "despawn: stopping target session failed; continuing to remove", "session_id", sessionID, "error", err) @@ -420,7 +420,7 @@ func (l *lifecycleService) resumeOrReject( // Already spawned and placed: idempotent success. Return the existing // container and its live session (if any) rather than provisioning a // second — a completed-call retry gets its original answer. - sessionID, _ := l.hub.SessionForAccount(existing.ID) + sessionID, _ := l.hub.SessionForAccount(ctx, existing.ID) return &compassv1internal.SpawnPeerResponse{ AgentAccountId: string(existing.ID), ContainerName: container, diff --git a/go/server/lifecycle_wake_pgtest_test.go b/go/server/lifecycle_wake_pgtest_test.go index fa86fbeca..adea1d798 100644 --- a/go/server/lifecycle_wake_pgtest_test.go +++ b/go/server/lifecycle_wake_pgtest_test.go @@ -56,7 +56,7 @@ func TestWakeAgentLiveIsNoOp(t *testing.T) { if _, err := f.hub.Start(ctx, "start-live", &compassv1.StartAgentSessionRequest{ContainerName: fakeContainer}); err != nil { t.Fatalf("Start = %v, want success", err) } - if _, live := f.hub.SessionForAccount(f.agentID); !live { + if _, live := f.hub.SessionForAccount(context.Background(), f.agentID); !live { t.Fatal("precondition: agent should be live after Provision+Start") } f.runner.forget() // drop the setup commands; assert only on the wake diff --git a/go/server/serve.go b/go/server/serve.go index 2ab756fa4..0455b0483 100644 --- a/go/server/serve.go +++ b/go/server/serve.go @@ -1491,7 +1491,7 @@ var errNoLiveSession = errors.New("forge notify: no live session for account") // miss branches and the AgentControl wrapping without a live hub or Postgres. // It mirrors the delivery package's SessionResolver + ControlDispatcher split. type notifySessionDispatcher interface { - SessionForAccount(account store.AccountID) (sessionID string, ok bool) + SessionForAccount(ctx context.Context, account store.AccountID) (sessionID string, ok bool) DispatchControl(ctx context.Context, sessionID string, op *compassv1internal.AgentControl) error } @@ -1508,7 +1508,7 @@ type forgeNotifyDispatcher struct { // a live session dispatches the notification wrapped as an AgentControl and // returns the dispatch error. func (d *forgeNotifyDispatcher) Notify(ctx context.Context, account string, n *compassv1internal.ForgeNotification) error { - sessionID, ok := d.hub.SessionForAccount(store.AccountID(account)) + sessionID, ok := d.hub.SessionForAccount(ctx, store.AccountID(account)) if !ok { return errNoLiveSession } diff --git a/go/server/sinks.go b/go/server/sinks.go index 8df7fbad2..7ebf1df7b 100644 --- a/go/server/sinks.go +++ b/go/server/sinks.go @@ -65,6 +65,16 @@ func newRunnerHub(st *store.Store, brd *board.Projection, tail runnerhub.Session log, ) hub.SetTranscriptStore(st) + // RIG-3108 T4: the same store is the durable session-binding surface the + // hub's in-RAM maps are demoted to a read-through cache over — the write path + // (record on promote, delete on unbind, sweep on re-enroll) and the two + // request-scoped cache-miss reads. Wired here beside the transcript seam so + // the one store instance backs the binding cache too. No RoutingFabric is + // wired: this is the single-Server MVP, so the hub's own writes keep its own + // cache honest and a cross-instance invalidation plane is not yet mounted + // (RIG-3107/T3 lands the NATS fabric; startDeliveryConsumer's subscribe + // wiring rides that). + hub.SetSessionBindingStore(st) // RIG-1667 T5: the same store backs the resume-body reconstructor's read // seam (SessionResumeSnapshot + ReadArchiveSegment), wired here beside the // write seam so the one store instance serves both legs. diff --git a/go/server/trace_continuity_e2e_pgtest_test.go b/go/server/trace_continuity_e2e_pgtest_test.go index 79b2a2bac..ebfa6fb31 100644 --- a/go/server/trace_continuity_e2e_pgtest_test.go +++ b/go/server/trace_continuity_e2e_pgtest_test.go @@ -353,7 +353,7 @@ func bringSessionLive(t *testing.T, w *mentionE2EWire, exp *tracetest.InMemoryEx if got := sresp.GetSessionId(); got != session { t.Fatalf("Start session id = %q, want %q", got, session) } - if got, ok := w.hub.SessionForAccount(account); !ok || got != session { + if got, ok := w.hub.SessionForAccount(context.Background(), account); !ok || got != session { t.Fatalf("SessionForAccount(%s) = (%q, %v), want (%q, true) — the hold/fan-out arms resolve through this binding", account, got, ok, session) } waitForStartSweep(t, exp, sweepsBefore+1)