trunk-merge/pr-1095/50ca2d6e-d4a3-4681-837a-933b4e03294b - #1121
Closed
trunk-io[bot] wants to merge 5 commits into
Closed
trunk-io[bot] wants to merge 5 commits into
trunk-io[bot] wants to merge 5 commits into
Conversation
…ings (RIG-3108) The hub's session<->account maps were the only record of a binding, so a Server restart lost every live session's routing. They become a read-through cache over the durable session_bindings table: writes go to the store first, then the maps under h.mu, never holding the lock across a store call or a sink. The invariant conflict this resolves is that enroll clears bindings on a Runner reconnect and fail-closed depends on that, while the rows survive. A naive cache-miss-reads-the-table would repopulate exactly what the reap dropped. So the two cases are distinguished by whether the hub already had a Runner: a RE-enroll durably reaps via DeleteSessionBindingsForRunner and its RETURNING rows drive the presence-offline and held-deliver edges, while a first enroll on a fresh hub (a Server restart, sessions still live) keeps the rows -- which is the availability property the change exists for. Matt ruled the ctx fork: accountForSession and SessionForAccount both gain a ctx parameter, including the public delivery.SessionResolver change and its callers in delivery/dispatch.go, server/lifecycle.go and server/serve.go. That ruling also closes a real hazard. deliverAck and forgeNotificationAck reached accountForSession AFTER store.WithSystemRole, so a cache-miss table read there would have run BYPASSRLS and could return a plausible row from an arbitrary tenant. The binding is now resolved on the request ctx before the escalation, and readThroughAllowed independently refuses any read-through under a system-role ctx, so a future caller fails closed rather than reading cross-tenant. TestSessionForAccountUnderSystemRoleIsUnscoped is unchanged and not weakened: it pins the store's raw behaviour, which is untouched; the hazard is closed one layer up. Cross-instance invalidation is seamed but not wired: serve.go constructs no fabric.Fabric, so SetRoutingFabric(nil) would be inert. RIG-3107 wires the live NATS fabric and the subscribe loop. Four red controls run individually and restored: dropping the enroll durable reap, the displaced eviction, the subscribe-side eviction, and letting the ack path inherit the system-role ctx each turn their own test red.
…ding fakes faithful (RIG-3108) Review fold on the session-binding cache. A re-enroll whose durable reap FAULTED cleared the in-RAM maps but left the rows, so the very next cache miss read one back and resurrected a session the hub had just declared dead — read-through turned the fail-closed reconnect contract into a resurrection. The hub now records that the table disagrees with the cleared cache (reapStale) and refuses read-through until a later reap succeeds. Both resolvers consult it. Verified as a control: reverting the gate alone turns the new re-enroll case red. The test doubles also modelled semantics the real store forbids, which is what hid the gap: - fakeBindingStore.RecordSessionBinding silently stole a session id already bound to a different account; the real store raises ErrConflict on session_bindings_session_key. It now does too, and records the runner id it was passed rather than a constant. - fakeRoutingFabric.PublishBindingChange accepted anything; the real fabric rejects an invalid change and the hub only logs that, so a malformed publish would silently leave every peer cache stale. The fake now mirrors the rejection contract. Adds the four durable-fault cases the fake was built for but no test drove (record, reap, resolve, reverse-resolve), and a witness test pinning a known gap: production mints session ids from a counter that resets on Runner restart, so a joint restart re-mints an id over a surviving row and the resulting ErrConflict is currently swallowed, leaving the durable row on the old account. Harmless while one Server instance shadows it; it needs a decision before multi-instance.
…he ctx signature (RIG-3108) Three callers behind `//go:build pgtest && unix` were missed by the ctx cutover: forge_notify_e2e_pgtest_test.go, lifecycle_wake_pgtest_test.go and trace_continuity_e2e_pgtest_test.go. A default-tag build never compiles them, so both the local gate and a plain grep of the callsites read clean; CI's pgtest job caught it as `go/server [build failed]`. Swept all 63 SessionForAccount/accountForSession callsites across every build tag: these three were the only unmigrated ones. Verified with `go vet -tags 'pgtest unix'`, which is the check that would have caught this before the push.
…ap (RIG-3108) Raising reapStale only after DeleteSessionBindingsForRunner RETURNED left the hole it was meant to close, just narrower. enroll clears the maps, releases the lock, then makes a store round-trip that on the fault path can block for seconds; a resolver arriving in that gap missed the cleared cache, saw reapStale still false, and read back a row the failing reap never deleted — resurrecting a session the reconnect had just declared dead. The flag is now raised inside the same critical section that clears the maps and lowered only by a reap that succeeds, so read-through is refused from the instant the cache is empty. Pessimistic-true during a successful reap costs nothing: the maps are empty, the only durable-but-uncached rows are the dead survivors, and a session promoted after the reconnect writes its cache entry and hits. Fenced by a deterministic test that parks the reap mid-flight and resolves concurrently; it is red against the previous placement and green under -race. Also documents the invariant the hub-wide flag depends on: it is correct only because the MVP has one stable runner id, and a future multi-Runner change must key it by runner id or one runner's successful reap will lower the flag while another's un-reaped rows survive.
|
Compass engineering docs preview: https://9af65ada.compass-eng-docs.pages.dev Deployed from |
trunk-io
Bot
deleted the
trunk-merge/pr-1095/50ca2d6e-d4a3-4681-837a-933b4e03294b
branch
September 11, 2026 23:33
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
1 participant
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 pull request was created and is being managed by Trunk Merge.
This pull request is based on the main branch at SHA 32cbf0d3be7e8544eabc6734649661470afcbcbf.
See more details here.
When CI completes, this pull request will be closed automatically.
Pull Requests Being Tested
This pull request is testing the changes from pull request 1095.