Pin three store invariants that no backend was checking - #99
Merged
Conversation
juicycleff
marked this pull request as draft
August 26, 2026 03:55
The ssftest and ssotest suites already run all four backends, and an older store_* conformance family covers some of the same ground on memory and sqlite only. Three of its cases have no counterpart in the newer suites, so those invariants go unchecked on postgres and mongo. This moves them across rather than growing a second suite that also needs a postgres runner. SubjectLinkUpsertIsConcurrencySafe fires thirty goroutines at one tuple. SubjectLinkUpsertIsIdempotent already proves a second call updates rather than collides when it arrives afterwards, and says nothing about both arriving at once. That is not hypothetical: a subject link is written on every SSO sign-in, so two writes for one subject is what happens when somebody opens two tabs. A read-then-write implementation loses the race, both readers miss, both insert, and the loser surfaces a raw constraint error. Every call must return nil and exactly one row must survive, so a deterministic write after the storm settles pins down which one. ActiveSignalsAreEnvironmentScoped keeps a signal raised in production from raising the risk on a development sign-in for the same app and user. A query that filters on app and user and forgets the environment predicate fails here and nowhere else. DomainLookupIsOrgScoped covers the tenancy level below DomainLookupIsAppScoped. One company runs SSO for several of its own organizations, and the partial unique index is on (app_id, org_id, domain) where active, so the same domain is legitimately configured twice inside one app. A lookup that drops the org predicate routes a user to a sibling organization's identity provider. This needed an org pair on ssotest.Fixture, added here so the struct and the case land together. org_id carries no foreign key, so they need no rows. The signals case passes a bare time.Now() because risk.go does. A case that builds a tidy truncated UTC input tests the store against a caller that does not exist. make test-integration runs the Store Conformance job locally, mongo replica set included, using the workflow's own -run pattern and -p 1.
juicycleff
force-pushed
the
test/plugin-postgres-conformance
branch
from
August 26, 2026 04:36
0192709 to
103e09e
Compare
juicycleff
marked this pull request as ready for review
August 26, 2026 04:37
Review follow-ups on the three cases. ConnectionNotFound asserted a bare error, so a backend returning something other than ErrConnectionNotFound passed it. Callers branch on that sentinel to tell "no SSO configured for this domain" from a backend that failed, and those two go different ways: the first falls through to password login, the second has to surface. All four backends already return it, so this only closes the hole rather than changing behaviour. The concurrency case claimed its final read proved exactly one row survived. It does not. GetSubjectLink is a FindOne and would return a row either way. What proves the count is the loop above it: a second row for the tuple could only come from a writer that hit the unique index and reported the collision, and that writer would have failed the loop. On memory there is no index and the mutex does that work. The final read proves last-write-wins, which is what it now says. Dropped the non-nil assertion on ListActiveSignals. All four backends return a non-nil empty slice, but by way of make([]*Signal, 0, n), which is a preallocation that happens to be non-nil rather than a contract anybody declared. The one consumer, risk.go, tests len() == 0, and the value is never marshalled, so nil and empty are indistinguishable to every caller. The non-nil contract elsewhere in this repo is stated for lists that do reach JSON, where null and [] differ to a client. This one does not, so the assertion would have failed a later `var out []*Signal` for a difference nothing can observe. The loop that checks no foreign signal leaked in stays.
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
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.
Three invariants in the older
store_*conformance family have no counterpart inssftest/ssotest, so they only ever ran on memory and sqlite. This moves them into the suites that already run all four backends.History of this PR, since it changed shape
It originally added Postgres runners to
plugins/sharedsignals/store_conformance_test.goandplugins/sso/store_conformance_test.go, on the belief that neither plugin had Postgres coverage. That was wrong. Both have a second, newer suite that already runs memory, sqlite, postgres and mongo:I had surveyed a checkout 300 commits behind main, where those files did not exist. Adding a second Postgres runner alongside them would have meant two suites per plugin covering the same constraints with two fixture sets to keep in step, so that approach is dropped entirely.
Comparing the two families case by name, three cases are genuinely uncovered. Those are what this PR now contains.
The three
SubjectLinkUpsertIsConcurrencySafe. Thirty goroutines at one tuple.
SubjectLinkUpsertIsIdempotentproves a second call updates rather than collides when it arrives afterwards, and says nothing about both arriving together. A subject link is written on every SSO sign-in, so two writes for one subject is what happens when somebody opens two tabs. A read-then-write implementation loses that race: both readers miss, both insert, and the loser surfaces a raw constraint error instead of succeeding. Every call must return nil and exactly one row must survive, which a deterministic write after the storm settles confirms.ActiveSignalsAreEnvironmentScoped. A signal raised in production must not raise the risk on a development sign-in for the same app and user. A query filtering on app and user that forgets the environment predicate fails here and nowhere else.
DomainLookupIsOrgScoped. The tenancy level below
DomainLookupIsAppScoped. One company runs SSO for several of its own organizations, and the partial unique index is on(app_id, org_id, domain) where active, so the same domain is legitimately configured twice inside one app. A lookup that drops the org predicate routes a user to a sibling organization's IdP. This needed an org pair onssotest.Fixture, added in the same commit so the struct and the case land together.org_idcarries no foreign key, so they need no rows.Notes for review
The signals case passes a bare
time.Now()becauserisk.godoes. A case that builds a tidy truncated UTC input tests the store against a caller that does not exist, which is how a time-handling bug survives a green suite.Both new ssftest cases use the package's
unique()helper, since postgres and mongo share one store across every case in a backend and nothing is truncated between them.Not in this PR
Retiring the
store_*conformance family, which is the obvious end state once its unique cases are gone. My "only three cases are unique" finding came from comparing case names. That justifies not adding duplication; it does not justify deleting assertions I did not write. That wants a case-by-case audit and its own PR.Verification
All three cases pass on all four backends (memory, sqlite, postgres via testcontainers, mongo via a single-node replica set). Full
make test-integrationgreen in 137s,go test ./...unchanged,gofmt/goimportsclean. Rebased onto 27c17e7.