Repository navigation
fix(storage): make the cassandradb provider correct on apache cassandra and scylla - #824
Open
lakhansamani wants to merge 10 commits into
Open
lakhansamani wants to merge 10 commits into
lakhansamani wants to merge 10 commits into
Conversation
buf dep update picks up a newer buf.build/bufbuild/protovalidate, which moved its documentation URLs from buf.build/docs/protovalidate/... to protovalidate.com/..., changing seven doc comments in the generated TS. No API, field or wire change -- comments and the buf.lock digest only. Latent on main: buf.yml runs on changes to proto/, gen/ or the Makefile, so the staleness gate stayed quiet until a PR touched one of those and then failed for a reason unrelated to its own diff.
The provider backs two DB types, cassandradb and scylladb, but only the
ScyllaDB path was ever tested -- cassandradb was absent from the storage
suite's backend list. Wiring it in and pointing it at cassandra:4.1 fails
immediately: three queries use CQL LIKE, which Apache Cassandra accepts
only against a SASI index and rejects against the plain secondary index
this provider creates.
LIKE restriction is only supported on properly indexed columns
ScyllaDB accepts the same statements, which is why it went unnoticed.
- GetWebhookByEventName errored out on every Apache Cassandra
deployment, so webhook dispatch by event name never fired.
- DeleteAllSessionTokensByUserID and DeleteSessionTokensByNamespace
errored the same way.
All three already carried ALLOW FILTERING -- a full table scan -- so
moving the comparison into Go changes nothing but where it runs.
DeleteAllSessionTokensByUserID also matched "%<userId>%" unanchored, so
a delete for "42" took out tokens for "142" or "provider:427". The SQL
provider was anchored long ago; this one kept the defect because the
assertion covering it lived inside a SQL-only subtest. It now runs on
every backend as Anchored Session Token Delete, and fails on ScyllaDB
when the unanchored match is restored.
Also stops Webhook_Operations indexing a possibly-empty result: the
panic aborted the whole run for every backend queued behind it.
Adds make test-cassandradb and includes cassandradb in test-all-db.
Full storage suite now green on cassandra:4.1 and on scylladb.
Two self-inflicted failures that look like storage bugs. Both are invisible in CI, which gets a fresh container per run, and deterministic on any repeated local run -- the "flaky ScyllaDB storage tests" shape. User_Operations asserted the just-created user appears in ListUsers(Limit:10, Offset:0) *unfiltered*. That only holds while the table is nearly empty. At 463 accumulated rows it failed 15 of 15 runs with "created user should appear in ListUsers", i.e. exactly like a write that never landed. Now filtered by the user's own email, which keeps both assertions and makes them independent of how much data is already there. The two phone numbers were fixed literals, so the failure above -- which aborts before the cleanup at the bottom -- left +1234567891 in the table and every later run failed earlier with "user with given phone number already exists". One failure cascading into a different, misleading one. Unique per run now, like the emails. Verified against the same 463-row keyspace: was 15/15 failing, now 5/5 passing. Refs #813
A single-use session token could be claimed twice. DeleteSessionTokenByUserIDAndKey consumes a token with DELETE ... IF EXISTS and treats the applied flag as the exactly-once arbiter behind refresh-token rotation. That LWT is stamped from the Paxos ballot, i.e. the server clock, while AddSessionToken's plain insert carried gocql's client-side timestamp. With the client clock ahead, the tombstone lands BELOW the insert: the row survives, but Paxos saw it and returned applied=true -- so the next claim of the same token also returns applied=true. Measured on scylladb/scylla with the client clock 10ms ahead: 20 of 20 tokens claimed twice, 0 of 20 after the fix. oauth_states has the identical shape for single-use authorization codes. The fix is DefaultTimestamp(false) on the plain writes to the three tables that mixed domains, which hands the timestamp to the coordinator -- the same clock the LWT already uses. Per query, never on the cluster: gocql round-robins coordinators, so a global switch would expose the 20-odd tables that never touch LWT to inter-node skew instead. It adds no Paxos round-trip, so the hot login path is untouched; converging the other way (lifting the inserts to LWT) would have cost one per login. The guard is reframed from "LWT vs plain" to the invariant that actually matters, server-stamped vs client-stamped, and recognises DefaultTimestamp(false) as server-domain. Its allow-list is now empty. It also had a blind spot worth naming: the statement regex matched only "..." and not raw strings, so it never saw AddSessionToken's insert -- one of the two writes it was written to watch. Both quote forms now. Mutation-verified: removing any one of the five server-stamps fails it. Full storage suite green on scylladb and cassandra:4.1, 26 subtests each. Refs #813
This was referenced Oct 15, 2026
wait-for-test-dbs.sh checks with `nc -z`, which on Docker Desktop
connects to the published-port proxy rather than the server: it succeeds
the instant `docker run -p` returns. Measured -- nc -z on a fresh
cassandra:4.1 succeeded at t=1s while cqlsh inside the same container
still failed at t=3s. The gate the script exists to provide was absent
for exactly the two slowest-booting backends.
It stayed hidden because `go test ./...` spends minutes on other
packages before reaching internal/storage, which is long enough for the
database to finish booting. A narrower -run target does not.
wait-for-cql.sh asks the server from inside the container, which a port
proxy cannot answer for. Wired into test-scylladb, test-cassandradb and
test-docker-up.
Cold runs, no container present at start:
make test-cassandradb nc: ready after 0s, CQL after 50s, suite green
make test-scylladb nc: ready after 0s, CQL after 4s, suite green
Found re-reviewing the previous commit. Converting session_tokens, oauth_states and authenticators to the server clock covered their direct call sites and missed the one that is not a direct call site. DeleteUser cascades over schemas.UserOwnedCollections with a single generic DELETE, and three of those seven tables are the ones just converted. That delete stayed client-stamped, so those tables were still mixed: a session token inserted under the server clock, deleted under the client clock, is dropped silently when the client runs behind -- the token outlives the hard delete of its own user. The cascade now consults serverDomainTables, the only place a loop-driven write can learn the domain of a table it picks at run time. Direct call sites keep saying DefaultTimestamp(false) inline, next to the statement and its reason. The guard could not see any of this: the statement names its table through a loop variable, so a source scan cannot classify it. It now (a) requires every write whose table is a variable to consult serverDomainTables, and (b) cross-checks that map against the domains it detected, in both directions. Mutation-verified -- dropping either table from the map, adding a client-domain table to it, or removing the cascade's domain check at all, each fails it with its own diagnostic.
…ackends The previous commit promoted the anchored-delete assertion to every backend but only four of eight were anchored. MongoDB, ArangoDB, DynamoDB and Couchbase still matched an unanchored substring, so the test failed on them -- caught by a security second-pass, not by me: I verified on sqlite, scylladb and cassandradb and stopped, which is exactly the gap AGENTS.md's "run a non-SQL backend" rule exists to close. A delete for "42" took out tokens for "142" or "provider:427". Harmless today because every user id is a UUIDv4, but it is a live over-delete primitive and the SQL provider removed it long ago. - mongodb: anchored regex, and regexp.QuoteMeta -- the old form interpolated the id straight into $regex, so an id containing regex metacharacters was evaluated as a pattern. - arangodb: d.user_id == @user_id OR RIGHT(...) == @suffix. - dynamodb: HasSuffix instead of Contains. - couchbase: CONTAINS stays as the server-side narrowing filter, the anchored predicate runs in Go. Doing it in N1QL would need LIKE wildcard escaping to stay literal. Its comment claimed the substring match mirrored the SQL provider, which stopped being true when SQL was anchored. Also from that review: - the test now covers the bare "<userID>" stored form as well as "<loginMethod>:<userID>", so the equality branch of the predicate is exercised -- it never was, on any backend. - AddOAuthState documents why its delete-then-insert is safe (fresh id per call, so different partitions) and what breaks it. - the cassandra webhook event_name index has no consumer left; say so rather than leave it looking load-bearing. Dropping it is a schema migration and belongs to a change that owns one. - WithContext(ctx) on the two new full scans. Re-measured the timestamp hazard on BOTH engines, which corrects the record: with the client stamp forced 10ms back, Apache Cassandra drops 50/50 and ScyllaDB 48/50. It is not Scylla-specific -- it was merely unobserved on Cassandra at ambient skew, and my earlier millisecond- truncation explanation for that does not survive a +15ms ambient lead showing 0/50. The guard's table now reports both engines and stops explaining what it cannot. All eight backends pass Anchored_Session_Token_Delete. Mutation-verified on mongodb, dynamodb and arangodb: restoring the substring match fails it on each.
…ng match Neither conclusion changes; the reasoning behind both does, and a stale rationale is how the next reader re-derives a wrong answer. memory_store/db/cache.go argued the reserved cache namespace is safe from DeleteAllSessionTokensByUserID because real user ids are UUIDs and so are never a substring of the literal. The match is anchored on every backend now, so the guarantee no longer rests on that -- it holds because the literal is neither an exact user id nor a ":<userId>" suffix. http_handlers/token.go said DeleteAllUserSessions sweeps the reuse-detection index via a "substring/glob match on the user id". It sweeps because the delete matches the session key and ignores the sub-key, which is true before and after.
…passes All from an independent review of the previous three commits. MongoDB was the one backend whose new predicate was NOT equivalent to the others, and it diverged BOTH ways. $regex is PCRE: `.` does not cross a newline, so a stored "a\nb:<id>" survived here while every other backend deleted it; and `$` also matches before a trailing newline, so "<prefix>:<id>\n" was deleted here and kept everywhere else -- the over-delete primitive this work exists to remove, alive on one backend. Now "(?s)^(.*:)?"+QuoteMeta(id)+`\z`. QuoteMeta only ever made the NEEDLE literal; it never made `.` or `$` agree with the other five. The static guard had three demonstrated false passes, each reproduced before and after: - Inverting `if serverDomainTables[table]` passed. That one flip client-stamps the server-domain tables AND server-stamps the client-domain ones -- the likeliest way a refactor breaks the line. The guard now rejects a negated lookup and requires DefaultTimestamp(false) in the same region, so presence is no longer mistaken for correctness. - The 3-line classification window bled into the next statement, so a neighbour's DefaultTimestamp(false) classified a genuinely client-stamped write as server-domain. - A comment merely MENTIONING serverDomainTables satisfied the variable-table check while the code below stamped nothing. Regions are now cut at the next write statement or the end of the enclosing function, and comment lines are dropped from them. wait-for-cql.sh counted sleeps rather than wall time, so its timeout was an undercount and the real ceiling open-ended; it also burned the full 180s in silence when the container was missing or had died, which an OOM at MAX_HEAP_SIZE=512M looks exactly like. Now a date-based deadline, a docker-inspect state check that fails fast and tails the container log, and cqlsh's own connect/request timeouts to bound each attempt -- timeout(1) is absent on macOS, where this runs most. Its knob is TEST_CQL_WAIT_TIMEOUT, not TEST_DB_WAIT_TIMEOUT, which means "wait for a port" and is routinely set below a cold Cassandra boot. The two CQL waits moved out of test-docker-up and into test-all-db's own recipe. As standalone lines in a prerequisite target, a probe failure aborted make before `$(MAKE) test-cleanup` ever ran, leaking all eight containers and every port -- the exact failure that target's comment says was fixed. Also: the "three of seven" count was wrong (oauth_states is in serverDomainTables but is not user-owned, so two of seven are reached); the cascade comment overstated its reach, since session tokens are stored namespaced and this bare `user_id = ?` select finds none of them on any backend; WithContext on the cascade select and the final user delete; and the sibling DeleteSessionTokensByNamespace carried the same two defects on mongodb (raw $regex interpolation) and couchbase (unescaped LIKE, where `_` is a wildcard and every recipe name has one). The test gains the newline shape that exposes the mongo divergence.
…proof Moving the CQL waits wholly into test-all-db's recipe fixed the leak and broke the other half: test-docker-up is also a standalone target, so `make test-docker-up && go test ./internal/storage/` then started against a Cassandra that was still booting. Caught by hitting it -- cassandradb failed the anchored-delete run until the probe was invoked by hand. Advisory copy in test-docker-up so that target leaves the stack usable, prefixed with `-` so a slow boot cannot abort make before test-all-db's cleanup. Authoritative copy stays inside test-all-db's && chain, where a failure is captured and still tears down. Re-running it there is free -- 0s once CQL answers. Verified on a faithful model of the recipe: probe failure skips the tests, runs cleanup and exits non-zero; a test failure runs cleanup and propagates the TESTS' status; a pass exits 0.
This branch has not been deployed
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.
Supersedes #821, #822 and #823 — one PR, no merge-order choreography. Four commits, each reviewable on its own.
Started from one question: had we ever run the storage suite against real Apache Cassandra? No.
cassandradbwas missing from the suite's backend list, so the provider that serves bothcassandradbandscylladbwas only ever exercised through Scylla. Pointing it atcassandra:4.1failed on the first run, at subtest 9, with a panic.1.
chore(proto): regenerate stubs after a protovalidate dep bumpbuf dep updateresolves a newerbuf.build/bufbuild/protovalidate, which moved its documentation URLs:Seven doc comments in
gen/ts/buf/validate/validate_pb.tsplus theproto/buf.lockdigest. No API, field or wire change.Latent on
main:buf.ymltriggers onproto/**,gen/**orMakefile, so the staleness gate stayed quiet until a PR touched one of those — commit 2 adds a test target to the Makefile — and then failed pointing at the wrong change. Included here so this PR's CI is green on its own merits.2.
fix(storage): make the cassandradb provider work on apache cassandraThree queries use CQL
LIKE. Apache Cassandra acceptsLIKEonly against a SASI index and rejects it against the plain secondary index this provider creates:ScyllaDB accepts the same statements, which is exactly why this survived.
GetWebhookByEventNameDeleteAllSessionTokensByUserIDDeleteSessionTokensByNamespaceAll three already carried
ALLOW FILTERING, i.e. a full table scan, so the index was never doing any work. Moving the comparison into Go changes nothing but where it runs — no new scans, no behaviour change on Scylla.A second bug, live on both engines.
DeleteAllSessionTokensByUserIDmatched"%" + userId + "%"— unanchored. A delete for42also took out tokens stored for142orprovider:427. The SQL provider was anchored long ago, with a comment explaining precisely this; the Cassandra provider kept the defect because the assertion covering it sat insidetestSQLCRUDCorrectnessFixes, which only runs for SQL backends. It is nowAnchored Session Token Delete, running on every backend. Mutation-verified: restoring the unanchored match fails it on ScyllaDB.Also:
Webhook_OperationsindexedfetchedByEventName[0]after only anassert.NotNil. An empty-but-non-nil slice panics there, and a panic in one backend's subtest aborts the run for every backend queued behind it — which is how the first Cassandra run ended, hiding everything after it. Nowrequire.NotEmpty.Coverage:
cassandradbadded toallDBTypesandgetTestDBConfigon port 9142 — deliberately not 9042, soTEST_DBS=scylladb,cassandradbcannot bind both types to one engine and silently test Scylla twice. Plusmake test-cassandradb, andcassandradbintest-all-db,test-docker-up,test-cleanup,wait-for-test-dbs.shand theAGENTS.mdtable (7 DBs → 8).3.
test(storage): stop the user suite depending on an almost-empty tableChasing #813's remaining failure, I looped the scylladb storage suite and hit 15 consecutive full-suite failures — on
mainand on pre-#818. Neither was a storage bug; both were the test's own fault, and both produce exactly the "flaky ScyllaDB storage tests" symptom #813 reports.ListUsersfirst-page assumption.User_Operationsasserted the just-created user appears inListUsers(Limit: 10, Offset: 0)unfiltered. That only holds while the table is nearly empty. At 463 accumulated rows it failed every run withcreated user should appear in ListUsers— indistinguishable from a write that never landed. Now filtered by the user's own email, which keeps both assertions and makes them independent of how much data is already there.+1234567891literal in the table. Every later run then failed earlier, withuser with given phone number already exists— one failure mutating into a different, misleading one. Both phone numbers are unique per run now, like the emails already were.Invisible in CI, which gets a fresh container per run. Deterministic locally.
4.
fix(storage): put every cassandra table in one timestamp domainCloses out the allow-list #818 deliberately left behind.
A single-use session token could be claimed twice.
DeleteSessionTokenByUserIDAndKeyconsumes a token withDELETE … IF EXISTSand treats theappliedflag as the exactly-once arbiter behind refresh-token rotation — the comment in the code says so explicitly. That LWT is stamped from the Paxos ballot, i.e. the server clock.AddSessionToken's plain insert carried gocql's client timestamp. When the client clock runs ahead, the tombstone lands below the insert: the row survives, but Paxos saw a live row and returnedapplied=true. The next claim of the same token also returnsapplied=true.Measured on
scylladb/scylla, client clock emulated 10 ms ahead withWithTimestamp, 20 insert-then-two-LWT-deletes cycles:DefaultTimestamp(false)oauth_stateshas the identical shape for single-use authorization codes.authenticatorsmixes an LWT insert and two conditional updates with a plain update and a plain delete.The fix is
DefaultTimestamp(false)on the plain writes to those three tables: gocql sends no timestamp and the coordinator assigns one — the same clock the LWT already uses.cl.DefaultTimestamp = falsewould push the 20-odd tables that never touch an LWT off a single monotonic client clock onto several skewed node clocks. Strictly worse than today.AddSessionTokentoIF NOT EXISTS, i.e. a Paxos round on every login.Re-measured on both engines after review, LWT insert then delete on one partition, 50 pairs per cell:
… IF EXISTS(#818's fix), forced −10ms* an earlier ScyllaDB container, with a different instantaneous client-vs-coordinator clock relationship, dropped 48/50 on that same ambient row. Whether it fires without forcing depends entirely on which way the two clocks happen to sit — which is what made #813 look flaky.
This corrects something I said earlier in this PR. I had written that Apache Cassandra escapes the drop because its ballot timestamp is truncated to whole milliseconds. It does not: with the client stamp forced back 10 ms, Cassandra drops 50/50, and an ambient run with the engine clock 15 ms ahead still dropped 0/50, which the truncation story cannot explain either. The exposure is not engine-specific and I no longer offer a mechanism for the ambient behaviour — only the measurement.
The guard is reframed from "LWT vs plain" to the invariant that actually matters — server-stamped vs client-stamped — and recognises
DefaultTimestamp(false)as server-domain.knownMixedTablesis empty.It also had a blind spot worth naming: the statement regex matched
"…"but not raw strings, so it never sawAddSessionToken's insert — one of the two writes it was written to watch. Both quote forms now, and the five tables it covers are asserted by name so a regex drift fails loudly instead of silently losing coverage.Verification, all on this branch
cassandra:4.1scylladb/scyllaUser_SCIM_Fields/Org_Domain_Operations/Session_Token_Operations× 100TEST_DBS=sqlite go test -p 1 ./...make proto-check,make proto-check-clients,make proto-lintgo build ./...,go vet ./...gofmt -lflagsinternal/http_handlers/oauth_sso_verify_test.go, which is pre-existing onmainand not touched by this PR.What this does not fix
User_SCIM_Fieldsnever reproduced — 0 failures in 100 targeted runs, before and after fix(storage): stop mixing LWT and plain writes on cassandra partitions #818. The 15/15 "flaky" failures were commit 3's test pollution, which is a different thing. If it resurfaces after this lands it deserves a fresh issue.UPDATEafter an LWT insert loses 0/50, in the identical loop shape where a plainDELETEdrops 48/50. The drop and the fixes are measured; why cell overwrites escape it is not established, and I would rather flag that than invent a mechanism.Refs #813
Added after review
A security second-pass (
security-engineer, as AGENTS.md asks for on auth-sensitive storage changes) found a merge blocker I had missed, plus several smaller things. Three more commits.5.
test(storage): probe CQL readiness, not the docker port proxywait-for-test-dbs.shchecks withnc -z, which on Docker Desktop connects to the published-port proxy rather than to the server — it succeeds the instantdocker run -preturns. Measured:nc -zon a freshcassandra:4.1succeeded at t=1s whilecqlshinside the same container still failed at t=3s. The gate that script exists to provide was absent for exactly the two slowest-booting backends.It stayed hidden because
go test ./...spends minutes on other packages before reachinginternal/storage, which is long enough for the database to boot. A narrower-rundoes not.wait-for-cql.shasks the server from inside the container, which a proxy cannot answer for. Cold runs with no container present:6.
fix(storage): stamp the user-delete cascade for the table it hitsFound re-reviewing commit 4 — a hole in my own fix. Converting
session_tokens,oauth_statesandauthenticatorsto the server clock covered their direct call sites and missed the one that is not a direct call site.DeleteUsercascades overschemas.UserOwnedCollectionswith a single genericDELETE, and three of those seven tables are the ones just converted. That delete stayed client-stamped, so those tables were still mixed: a session token inserted under the server clock and deleted under the client clock is dropped silently when the client runs behind — the token outlives the hard delete of its own user.The guard could not see it: the statement names its table through a loop variable, so a source scan cannot classify it. It now (a) requires any write whose table is a variable to consult
serverDomainTables, and (b) cross-checks that map against the domains it detected, in both directions. Mutation-verified — dropping either table from the map, adding a client-domain table to it, or removing the cascade's domain check entirely, each fails it with its own diagnostic.7.
fix(storage): anchor the session-token delete on the four remaining backendsThe blocker. Commit 2 promoted the anchored-delete assertion to every backend, but only four of eight were anchored. MongoDB, ArangoDB, DynamoDB and Couchbase still matched an unanchored substring, so the new test failed on them. I had verified on sqlite, scylladb and cassandradb and stopped — exactly the gap AGENTS.md's "also run a non-SQL backend" rule exists to close.
mongodb/session_token.go:66bson.M{"$regex": userId}— unanchored and unescapedarangodb/session_token.go:96FILTER CONTAINS(d.user_id, @user_id)dynamodb/session_token.go:91strings.Contains(t.UserID, userId)couchbase/session_token.go:103WHERE CONTAINS(user_id, $1)A delete for
42took out tokens for142orprovider:427. Harmless in practice — every user id is a UUIDv4 — but it is a live over-delete primitive, and the SQL provider removed it long ago.regexp.QuoteMeta. The old form interpolated the id straight into$regex, so an id containing regex metacharacters was evaluated as a pattern rather than matched literally.d.user_id == @user_id OR RIGHT(d.user_id, LENGTH(@suffix)) == @suffix.HasSuffixinstead ofContains.CONTAINSstays as the server-side narrowing filter; the anchored predicate runs in Go. Doing the suffix test in N1QL meansLIKE '%:'||$1, whose wildcards would need escaping to stay literal. Its comment claimed the substring match mirrored the SQL provider — untrue since SQL was anchored.Also folded in from the review:
<userID>stored form as well as<loginMethod>:<userID>, so the equality branch of the predicate is exercised. It never was, on any backend.AddOAuthStatedocuments why its delete-then-insert is safe (freshstate.IDper call, so different partitions) and what would break it — a caller supplying a stable id turns it into a same-partition pair where two coordinator timestamps can drop the insert.authorizer_webhook_event_nameindex has no consumer left after commit 2. Said so in place rather than leaving it looking load-bearing; dropping it is a schema migration and belongs to a change that owns one. (authorizer_email_template_event_nameis not dead —email_template.gostill selects on it with=.)WithContext(ctx)on the two new full scans, so they are cancellable.Backward compatibility
session_tokens.user_idis<userID>or<loginMethod>:<userID>(memory_storeSetUserSession, plusservice_account:<id>), so the user id is always the final segment and== userId || HasSuffix(":"+userId)is exhaustive. Every caller passes a bareuser.ID. Therefresh_family_<id>reuse-detection entries live under the same session key and are still swept.DefaultTimestamp(false)and already-stored rows. A row written by the old binary carriesTc = t_insert + skew, whereskewis app-host clock minus cluster clock. A new server-stamped delete atTs = t_deleteis dropped only ift_delete − t_insert < skew. Ifskew ≤ 0there is no new exposure at all; ifskew > 0the window isskewmeasured from each row's own insert, not "until the old rows expire". The revocation paths that matter fire seconds to days after login. Practical advice: sanity-check NTP on app hosts before upgrading — a host minutes off NTP was already losing LWT deletes on the old binary, which is the bug being fixed.|skew|. No new failure class, same magnitude as the one removed.gocql@v1.6.0/frame.go:1533is a baretime.Now().UnixNano()/1000with no per-session counter, so client stamping never gave a same-process ordering guarantee either. The change swaps one unsynchronised clock for a set of loosely-synchronised ones — better, not a proof.Verification after review
make test-all-db— full storage suite, all 8 backendsAnchored_Session_Token_Deleteon all 8 backendscassandra:4.1andscylladbmake test-cassandradb/make test-scylladb, coldgo build ./...,go vet ./...Known gaps, stated rather than papered over
DeleteSessionTokensByNamespaceis still unanchored-by-CONTAINSon the non-Cassandra backends. It has no live caller: its only consumers are ininternal/graphql/_deprecated_update_env.go, and a leading_excludes the file from the build. Left alone deliberately.User_SCIM_Fieldsnever reproduced — 0 failures in 100 targeted runs, before and after fix(storage): stop mixing LWT and plain writes on cassandra partitions #818.UPDATEafter an LWT insert loses 0/50, in the identical loop shape where a plainDELETEdrops 48/50. The drop and the fixes are measured; why cell overwrites escape it is not established, and I would rather flag that than invent a mechanism.Second review round
An independent
principal-engineerpass over commits 5–7 (the ones written in response to the first review, which had had no outside eyes). Verdict was "no blockers", but it found four real things and three demonstrated false passes in the guard — all reproduced before and after. Two more commits.MongoDB was the one backend whose predicate was not equivalent — and it diverged both ways
$regexis PCRE:.does not cross a newline, and$also matches before a trailing newline. Verified live onmongo:4.4.15against the SQL semantics:user_id"T","p:T","a:b:T""xT","p:xT""a\nb:T""p:T\n"The second row is the exact primitive this PR exists to remove, alive on one backend.
regexp.QuoteMetaonly ever made the needle literal — it never made.and$agree with the other five. Now(?s)^(.*:)?+QuoteMeta(id)+\z.Neither edge is reachable from application writes (login methods come from a fixed set, ids are UUIDv4) — but "unreachable today" is precisely the standard the substring match was removed under, so it gets the same treatment. The test now carries the newline shape; mutation-verified that reverting the pattern fails it on MongoDB and passes the other seven.
Arango, DynamoDB and Couchbase were confirmed genuinely equivalent, Arango live against
arangodb:3.10.3— including a stored value shorter than the suffix, andnull/missing/numericuser_id, all with no AQL warnings.The guard had three false passes
Each was demonstrated, then re-run after the fix:
if serverDomainTables[table]toif !…negates the serverDomainTables lookupSessionToken mixes timestamp domainsserverDomainTableswithout consulting serverDomainTablesThe inversion is the one that matters: one flipped
!client-stamps the server-domain tables and server-stamps the client-domain ones, and the guard said nothing. It was checking presence, not correctness.Fixes: classification regions are now cut at the next write statement or the end of the enclosing function (so a neighbour's flag can't classify this statement), comment lines are dropped from regions, and the variable-table check requires
DefaultTimestamp(false)in the same region and rejects a negated lookup. All 9 earlier mutations still caught — 12 of 12 now.wait-for-cql.shand a container leaksleep 2iterations, not wall time, soTIMEOUT=180really meant "90 attempts" and the true ceiling was open-ended — and the "CQL after 50s" figure was measured the same way. Now adate-based deadline.MAX_HEAP_SIZE=512Mis plausible. Nowdocker inspectthe state first and tail the container log on failure. Both paths verified: missing container →no such container, exited container →is 'exited', not running, each immediately.--connect-timeout/--request-timeoutrather thantimeout(1), which is not present on macOS — where this runs most.TEST_CQL_WAIT_TIMEOUT. OverloadingTEST_DB_WAIT_TIMEOUT(which means "wait for a port" and is routinely set low) would have madeTEST_DB_WAIT_TIMEOUT=30 make test-cassandradbfail against this PR's own measured 50s cold boot.test-docker-upprerequisite, a probe failure aborted make before$(MAKE) test-cleanupintest-all-db's recipe ever ran — leaking all eight containers and every port, the exact failure that target's comment says was fixed.Fixing the leak then broke the other half, which I caught by hitting it: with the waits moved wholly into
test-all-db, a standalonemake test-docker-up && go test ./internal/storage/started against a still-booting Cassandra, andcassandradbfailed the anchored-delete run. So: an advisory copy intest-docker-up(prefixed-, so a slow boot can't abort make) and the authoritative copy insidetest-all-db's&&chain. Re-running it there is free — 0s once CQL answers.Verified on a faithful model of the recipe:
Smaller corrections
oauth_statesis inserverDomainTablesbut is not user-owned, so the cascade never reaches it. Two server / five client. Corrected in all three places.user_id, whilememory_storestores session tokens as<loginMethod>:<id>— so that select finds no namespaced session-token rows on any backend. The live beneficiary isauthenticators, whoseuser_idis bare; stamping the session-token half is correct and defensive rather than currently active.WithContext(ctx)on the cascade select and the final user-row delete, in the same function the previous commit edited.DeleteSessionTokensByNamespacecarried the same two defects one function below the one just fixed: raw$regexinterpolation on MongoDB, and an unescaped N1QLLIKEon Couchbase where_is a single-char wildcard and every recipe name contains one (basic_auth,magic_link_login,service_account). Both fixed. It has no live caller — its only consumers are in_deprecated_update_env.go, which the leading_excludes from the build (go listconfirms zero matching build files) — but it is the same family one function away.Final verification
make test-all-dbAnchored_Session_Token_Delete, all 8 backends incl. the newline shapego build ./...,go vet ./...Found but deliberately not fixed here
The service-layer
DeleteUserdoes not revoke namespaced session tokens.admin_users.go:406callsStorageProvider.DeleteUser(cascade on bareuser_id) and purges FGA tuples, but neverDeleteAllUserSessions— and the cascade cannot see<loginMethod>:<id>rows. So a deleted user's session-token rows survive on every backend. Pre-existing, uniform, andtestDeleteUserCascademisses it because it stores tokens with a bare id. I verified the code path, not the live behaviour. It wants its own issue and its own test rather than being folded in here.