feat(store): add the durable session_bindings table and its store methods (RIG-3108) - #990
Open
rigel-mintaka wants to merge 4 commits into
Conversation
This was referenced Sep 7, 2026
|
Compass engineering docs preview: https://compass-managed-rig-3108-ses.compass-eng-docs.pages.dev Deployed from |
…hods (RIG-3108)
The RunnerHub holds the (session -> agent account, Runner) binding only in RAM,
so a Server restart loses every binding and the relay resolves nothing for a
session it minted moments earlier. Add the durable table and the store surface
over it. Nothing reads these methods yet — demoting the hub's in-RAM maps to
caches over this table is the next slice.
A binding is not authorization, exactly as `agent_placements` is not:
`SubscribeAgentSession` still authorizes through
`agent_sessions -> agent_accounts -> channel_members` and never reads this
table. The two reads it exists for are the relay resolving the account that owns
an inbound session, and the delivery consumer resolving the live session of an
already-authorized recipient account.
UNIQUE index on `agent_account_id`, not merely an index: the in-RAM
`accountSessions` map it replaces is 1:1, so a second concurrent session for one
account would split that account's deliveries across two sessions and let the
reverse lookup resolve whichever row Postgres happened to return. The unique
index makes the double-bind unrepresentable rather than unlikely, and a rebind
onto a new session id raises a unique violation mapped to `ErrConflict` instead
of silently double-binding.
`DeleteSessionBindingsForRunner` is `:many ... RETURNING`, not a bare DELETE.
`Hub.enroll` snapshots the bindings *before* clearing them, because each cleared
binding drives a presence DISCONNECTED edge and each cleared session id must be
reaped from the delivery held-deliver registry; enroll emits no lifecycle frames
of its own. A DELETE that returned nothing would satisfy the fail-closed
invariant while silently dropping both side-effects, leaving a long-WORKING
agent stuck WORKING in the projection forever.
Both lookups fail closed: a miss is `ErrNotFound`, never a zero-value id with a
nil error, because a zero-value account id would flow onward as a real (wrong)
principal instead of stopping the call.
`runner_id` is NOT NULL and rejects `''`. The `''` unknown-runner sentinel does
not carry over from `agent_placements`, where a placement must outlive its
Runner's attachment and the next provision self-heals it. A binding has the
opposite lifetime — it exists only while a Runner is attached, and `runner_id`
is the sweep key that retires it — so a binding stamped `''` could never be
swept by any real Runner's re-enroll and would linger as a stale session
outliving its Runner.
Two deviations from the frozen record's §T4 schema, both ruled by Matt
(2026-09-07) and recorded on RIG-3108:
- No `0002_session_bindings.sql`. Migrations are collapsed into the squashed
init per the standing 2026-08-07 ruling in that file's header ("the same
reasoning folds each later migration in as it accretes"); §T4's `NNNN_`
wording predates it.
- `TIMESTAMPTZ`, not `bound_at_unix_ms`. The schema's split is wire exposure:
`_unix_ms BIGINT` is used where the value crosses the protobuf wire as an
`int64`, `TIMESTAMPTZ` for server-internal bookkeeping, and no `TIMESTAMPTZ`
column name appears as an `int64` proto field. A binding is server-internal.
Verified: 9 pgtest cases — round-trip both directions, fail-closed miss on both
lookups, the `ON CONFLICT` rebind in place, `ErrConflict` on a second session
for a bound account, `ErrInvalidArgument` on an unknown account, idempotent
delete freeing the unique slot, the runner sweep returning every swept binding
while leaving another Runner's binding alive, `updated_at` advancing via the
trigger, and cross-tenant isolation. Red control: dropping the table from
`tenant_tables` fails with `tenant B ResolveSessionAccount(sess-a) err = <nil>,
want ErrNotFound — cross-tenant read leak`, caught by both the isolation test
and the RLS catalog floor.
Gates: `sqlc-drift`, `sqlc-vet` (every query PREPAREs against the live schema),
`sql-migration-gate:check` (0 issues), `go build ./...`, `golangci-lint run
./internal/store/...` (0 issues), and the full `-tags pgtest` store suite.
Co-authored-by: Matt Wilkinson <matt@rigel.build>
Round-1 review found that the UNIQUE index on `agent_account_id` forbids a path
the hub relies on today. The in-RAM `accountSessions` map is 1:1, which is why
the index looked right — but the hub re-points an account onto a NEWER session
while the older one is still bound, and there is a test named for exactly that:
`relay_comms_test.go:492-501` promotes `sess-old` then `sess-new` onto the same
account before unbinding the stale one. Against the previous schema the second
bind raised `duplicate key value violates unique constraint
"session_bindings_account_key"`.
`promoteSession` also returns nothing (`relay_comms.go:49`), so the next slice
had nowhere to put the `ErrConflict` the index produced. The doc comment's
prescribed remedy — release the stale binding first — is a delete-then-insert
race with a window where the account resolves to no session at all, and the
stale session id is usually unknown at promotion time because the hub reaches it
only through the very map this table replaces.
Key by the account instead, with the tenant folded in:
PRIMARY KEY (tenant_id, agent_account_id)
A re-point is now one atomic upsert on the account, which is what
`promoteSession` already does to its map. `session_id` becomes a plain column
with `UNIQUE INDEX session_bindings_session_key (tenant_id, session_id)`, so one
session still speaks for exactly one account and a second account claiming a
live session is refused — now the only reachable unique violation, remapped to
`ErrConflict` accordingly.
Folding `tenant_id` into both keys follows the convention the forge-coordinate
tables already use (`0001_init.sql:775`, `:821`, `:854`), which the RLS block
header states is deliberate so two tenants may hold the same coordinate without
colliding. Previously a cross-tenant `session_id` collision surfaced as an
opaque SQLSTATE 42501 RLS error rather than a mapped sentinel, because the
colliding row was invisible to the inserting tenant and its `ON CONFLICT` could
not fire.
`RecordSessionBinding` now returns the session id it displaced, in one
statement, via a CTE that reads the prior row in the same snapshot as the
upsert. The next slice needs it: a displaced session must be reaped from the
delivery held-deliver registry, and the previous shape gave the caller no way to
learn what it had replaced. `COALESCE` to the empty string rather than NULL,
because sqlc types the expression as a non-nullable `string` that pgx cannot
scan a NULL into — the fresh-insert case would have failed at runtime.
Also from the review:
- State in the schema comment that a binding is NOT cross-checked against
`agent_sessions`, which remains the authz root. There is no FK from
`session_id`, deliberately, so the comment no longer implies an integrity the
schema does not have.
- Add the account-moves rebind test. Dropping an assignment from the upsert's
SET list previously passed all 12 cases, because no test ever rebound a
session onto a different account.
- Seed the sweep-order fixture in reverse (`sess-z`, `sess-b`, `sess-a`).
`DELETE ... RETURNING` emits physical heap order, which for freshly inserted
rows equals insertion order, so the previous fixture was already sorted and
the assertion could not fail if the sort were deleted.
- Add a catalog-floor test for `updated_at_tables`, mirroring the one that
defends `tenant_tables`: enumerate columns named `updated_at` and fail on any
whose table lacks the `set_updated_at` trigger. Without it a future table
could gain the column and silently miss the trigger — the rot the trigger
exists to prevent.
- Use `context.Background()` in both new test files, matching 19 of the 20
existing pgtest files rather than planting a second convention.
- Close the sqlc comment gap so the sweep's block comment attaches to its own
query instead of the preceding one's godoc.
Verified: 25 pgtest cases green against Postgres 16.14. Red controls — dropping
`session_id = EXCLUDED.session_id` fails the re-point test; dropping
`runner_id = EXCLUDED.runner_id` fails two tests with `runner-1 still holds
[...] after the re-point, want none`; removing `slices.SortFunc` fails the
sweep-order test with the rows returned backwards. Every revert verified
byte-identical by md5. The fresh-insert `displaced == ""` case is pinned in the
suite, not just probed. `sqlc-drift`, `sql-migration-gate:check`,
`go build ./...`, `gofmt`, and `golangci-lint run ./internal/store/...` all
clean.
Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
force-pushed
the
compass-managed/rig-3108-session-bindings
branch
from
September 7, 2026 22:19
85c42b5 to
fc48fd0
Compare
rigel-mintaka
marked this pull request as ready for review
September 7, 2026 22:29
Contributor
Correction to commit
|
| claim | stated | actual |
|---|---|---|
| pgtest cases | 25 | 14 test functions across the two touched files (10 + 4, no subtests) |
pgtest files using context.Background() |
19 of 20 | 20 of 20 — zero used t.Context() before this stack |
| tenant-folded key convention cited at | 0001_init.sql:775, :821, :854 |
:799, :845, :878 |
The three originally cited lines are not key declarations at all: :775 is a section-header
comment, :821 is a created_at TIMESTAMPTZ column, :854 is prose about a SMALLINT CHECK.
The convention itself is real and the substantive claim holds — only the line numbers were
wrong, most likely because the re-key's own comment expansion shifted the file after I read
them.
The red-control quotes in that message do check out verbatim, and the relay_comms_test.go:492-501
citation is exact.
Lesson recorded for my own lane: a line-number citation taken before an edit that changes the
same file's length is stale by construction. Re-read the anchor after the edit, or cite by
symbol rather than by line.
…ted once (RIG-3108)
Round-2 review reproduced a real defect against Postgres 16: the CTE upsert
returned a wrong `displaced` session id under concurrency. The query claimed the
`prev` read and the write shared a snapshot. They do not. This path runs at READ
COMMITTED (the only `IsoLevel` in the package is an unrelated read-only snapshot
at `agent_transcripts.go:321`), where `prev` uses the statement-start snapshot
but `ON CONFLICT DO UPDATE` blocks on the row lock and then re-reads the latest
committed version, so the write acts on a newer row than the read reported.
Two interleavings, both reproduced with real concurrent sessions armed exactly
as the store arms its own:
- Both re-point from `sess-A`: both callers returned `displaced = sess-A`, so
`sess-A` is reaped twice and `sess-B` — genuinely displaced — is reaped by
nobody.
- Both first-bind: the second returned `displaced = ""` having destroyed the
first's binding, reporting the loss to nobody.
That matters because `displaced` exists so the next slice can reap the
held-deliver registry. A stranded entry holds deliveries for an account that has
already moved on.
Run the prior-value read and the write in one `beginTenantTx`, which arms
`SET LOCAL ROLE` and the tenant GUC at BEGIN so every statement stays
RLS-scoped — a raw `Begin` would run as the owner with no GUC and silently
disable tenant isolation.
A locked read alone is not sufficient, and this is the part worth recording: a
row lock cannot serialize two binds when there is no row yet. Both first-binds
read empty, and the primary key then orders only their *writes* — but
`displaced` is a *read*, so the key structurally cannot cover it. Take a
per-account advisory lock BEFORE the read, following the package's existing
`LockOwnerCoordination`/`AcquireOwnerTreeLock` pattern, keyed on
`(tenant, account)` rather than the account alone because the folded key lets
two tenants hold bindings for one account id and those binds must not block each
other. Both locks are load-bearing: removing either fails a test.
Also from the review:
- The false "same snapshot" claim appeared in four files (the query source, both
generated files, and hand-written in `session_bindings.go`). All four now
state the actual guarantee; the measured count of the old assertion is zero.
- **The tenant fold had no test at all.** Reverting to un-folded keys left every
existing assertion unchanged, so the headline half of the previous commit was
unprotected. Two cases now pin it: two tenants holding the same `session_id`,
and two holding the same `agent_account_id`. Both fail on the un-folded
mutation.
- `ErrConflict` branches on `pgConstraintName(err) == "session_bindings_session_key"`
and falls through to the generic wrap otherwise. The previous comment claimed
the session index was the only unique index left; the primary key is one too,
and `pgErrIs` cannot tell which raised, so any other unique violation was
reported as a session claim.
- `SessionForAccount`'s doc said exactly one row can answer because the account
is the key. True only per tenant — corrected, with the system-role carve-out
stated. Under `WithSystemRole` (BYPASSRLS) the account direction is unscoped
and returns an arbitrary tenant's row; a test pins that hazard rather than
leaving it latent, since `WithSystemRole` is on the path the next slice takes.
- A system-role write does not fail as the review first measured — it usually
*succeeds* and lands an orphan with `tenant_id = ''`, because an ended
`SET LOCAL` leaves a custom GUC defined-and-empty on a pooled backend, which
satisfies NOT NULL. No RLS policy matches `''`, so the row is invisible to
every tenant. The test pins the disjunction rather than pretending either
branch is the contract.
- The schema comment implied the table represents the hub's transient
two-session state. It cannot: the hub keeps `sessionAccounts` (many-to-one)
and `accountSessions` (1:1), and this table faithfully represents only the
latter. It now says so, and says a caller needing the stale-vs-unknown
distinction must keep it in RAM.
- `countBindings`/`bindingTimes` read the owner pool with no RLS, so their
"exactly one row" assertions were cross-tenant counts. Both now take a tenant
predicate — mandatory, not decorative, since the new fold tests deliberately
create a second tenant holding the same account and session ids.
- `mustBind` takes `ctx` second, matching `mustTopic`; the sqlc header separator
stops the file comment being absorbed into `RecordSessionBinding`'s godoc.
Corrections to the previous commit message, which overstated three counts: the
two touched files hold **14** test functions, not 25; **20 of 20** pgtest files
use `context.Background()`, not 19; and the tenant-folded key convention is at
`0001_init.sql:799`, `:845`, `:878` — the lines cited before were prose and a
timestamp column.
Verified: 20 pgtest cases green against Postgres 16. Red controls — removing the
advisory lock fails both concurrency tests ("with no row to lock, that lock is
the ONLY thing serializing two first binds"); un-folding the keys fails both new
tenant tests while the pre-existing isolation test still passes, confirming the
review's mutation proof. Every revert verified byte-identical by md5.
`sqlc-drift`, `sql-migration-gate:check`, `go build ./...`, `gofmt`, and
`golangci-lint run ./internal/store/...` all clean.
Co-authored-by: Matt Wilkinson <matt@rigel.build>
Re-pushed to trigger CI: the base PR was pushed after this head, and the
workflow fires only on opened/synchronize/reopened, so no run was created for
this commit.
rigel-mintaka
force-pushed
the
compass-managed/rig-3108-session-bindings
branch
from
September 7, 2026 23:04
fc48fd0 to
7d0a9f6
Compare
Round-3 review found that removing `FOR UPDATE` from `RecordSessionBinding`'s prior-value read leaves the entire committed suite green. I reproduced that myself before fixing it: the row lock was unprotected. **This corrects a claim in the previous commit message.** It said "Both locks are load-bearing: removing either fails a test." Half false. Removing the advisory lock does fail two tests, with the messages that commit quotes. Removing `FOR UPDATE` failed nothing. The reason no existing test could reach it: both concurrency tests pit a bind against another bind, and a bind takes the per-account advisory lock BEFORE its prior-value read, so the two callers are already serialized by the time either evaluates `SELECT ... FOR UPDATE`. The advisory lock alone is sufficient for bind-vs-bind, which makes the row lock dead weight in that interleaving. The row lock earns its place against a writer that reaches the row WITHOUT the advisory lock, and `DeleteSessionBindingsForRunner` is exactly that — the reconnect sweep is a bare `:many` DELETE and takes no advisory lock at all. So the new test races a re-point against a sweep of the account's current Runner. Both destroy the same binding, and the displaced session id drives the reap from the delivery held-deliver registry, so it must reach exactly one of them: reported to both is a double reap, reported to neither strands its held deliveries. Which side wins is deliberately not asserted — the assertion is on the shape, so it holds on every schedule. No gate transaction drives the interleaving, because the contended resource IS the lock under test and a gate holding it would beg the question. An unsynchronized loop suffices at this count: 120 iterations, measured 108/120 and 113/120 reporting the double-reap with `FOR UPDATE` removed and 0/120 with it present. A single iteration would be a ~90% test. Verified: my own red control removed `FOR UPDATE`, regenerated sqlc, and this test was the ONLY failure of the 16 — 113/120 double-reaps. Restored byte-identically (`queries/session_bindings.sql` md5 `62fb16b446847ee231c0f8a2dc2042e3`, generated `5304b52f8aca86e811ae57c746dab1c9`) and re-ran green. `sqlc-drift`, `go build ./...`, `gofmt`, and `golangci-lint run ./internal/store/...` clean. Two further count corrections to the previous message, both found by the same review: the folded-key citations `0001_init.sql:799`/`:845`/`:878` were correct at the parent commit but that commit's own edit to the file shifted them — they are `:818`/`:864`/`:897` there. And "20 pgtest cases green" was 19. Co-authored-by: Matt Wilkinson <matt@rigel.build>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is part of a stack containing 6 PRs:
mainThe RunnerHub holds the (session -> agent account, Runner) binding only in RAM,
so a Server restart loses every binding and the relay resolves nothing for a
session it minted moments earlier. Add the durable table and the store surface
over it. Nothing reads these methods yet — demoting the hub's in-RAM maps to
caches over this table is the next slice.
A binding is not authorization, exactly as
agent_placementsis not:SubscribeAgentSessionstill authorizes throughagent_sessions -> agent_accounts -> channel_membersand never reads thistable. The two reads it exists for are the relay resolving the account that owns
an inbound session, and the delivery consumer resolving the live session of an
already-authorized recipient account.
UNIQUE index on
agent_account_id, not merely an index: the in-RAMaccountSessionsmap it replaces is 1:1, so a second concurrent session for oneaccount would split that account's deliveries across two sessions and let the
reverse lookup resolve whichever row Postgres happened to return. The unique
index makes the double-bind unrepresentable rather than unlikely, and a rebind
onto a new session id raises a unique violation mapped to
ErrConflictinsteadof silently double-binding.
DeleteSessionBindingsForRunneris:many ... RETURNING, not a bare DELETE.Hub.enrollsnapshots the bindings before clearing them, because each clearedbinding drives a presence DISCONNECTED edge and each cleared session id must be
reaped from the delivery held-deliver registry; enroll emits no lifecycle frames
of its own. A DELETE that returned nothing would satisfy the fail-closed
invariant while silently dropping both side-effects, leaving a long-WORKING
agent stuck WORKING in the projection forever.
Both lookups fail closed: a miss is
ErrNotFound, never a zero-value id with anil error, because a zero-value account id would flow onward as a real (wrong)
principal instead of stopping the call.
runner_idis NOT NULL and rejects''. The''unknown-runner sentinel doesnot carry over from
agent_placements, where a placement must outlive itsRunner's attachment and the next provision self-heals it. A binding has the
opposite lifetime — it exists only while a Runner is attached, and
runner_idis the sweep key that retires it — so a binding stamped
''could never beswept by any real Runner's re-enroll and would linger as a stale session
outliving its Runner.
Two deviations from the frozen record's §T4 schema, both ruled by Matt
(2026-09-07) and recorded on RIG-3108:
0002_session_bindings.sql. Migrations are collapsed into the squashedinit per the standing 2026-08-07 ruling in that file's header ("the same
reasoning folds each later migration in as it accretes"); §T4's
NNNN_wording predates it.
TIMESTAMPTZ, notbound_at_unix_ms. The schema's split is wire exposure:_unix_ms BIGINTis used where the value crosses the protobuf wire as anint64,TIMESTAMPTZfor server-internal bookkeeping, and noTIMESTAMPTZcolumn name appears as an
int64proto field. A binding is server-internal.Verified: 9 pgtest cases — round-trip both directions, fail-closed miss on both
lookups, the
ON CONFLICTrebind in place,ErrConflicton a second sessionfor a bound account,
ErrInvalidArgumenton an unknown account, idempotentdelete freeing the unique slot, the runner sweep returning every swept binding
while leaving another Runner's binding alive,
updated_atadvancing via thetrigger, and cross-tenant isolation. Red control: dropping the table from
tenant_tablesfails withtenant B ResolveSessionAccount(sess-a) err = <nil>, want ErrNotFound — cross-tenant read leak, caught by both the isolation testand the RLS catalog floor.
Gates:
sqlc-drift,sqlc-vet(every query PREPAREs against the live schema),sql-migration-gate:check(0 issues),go build ./...,golangci-lint run ./internal/store/...(0 issues), and the full-tags pgteststore suite.Co-authored-by: Matt Wilkinson matt@rigel.build