Repository navigation
fix(storage): put every cassandra table in one timestamp domain - #822
Closed
lakhansamani wants to merge 1 commit into
Closed
lakhansamani wants to merge 1 commit into
lakhansamani wants to merge 1 commit into
Conversation
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
Contributor
Author
|
Superseded by #824, which carries this change unmodified alongside the other three commits in one PR — no merge-order dependency between them. |
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.
Closes out the allow-list #818 deliberately left behind. Based on #821 (shares
session_token.go); rebase ontomainonce that merges.The bug: 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 of the server, the tombstone lands below the insert. The row survives — but Paxos saw a live row and returned
applied=true. The next claim of the same token sees the row still there and also returnsapplied=true.Measured on
scylladb/scylla, client clock emulated 10ms 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, and why not the other one
DefaultTimestamp(false)on the plain writes to those three tables. gocql then sends no timestamp and the coordinator assigns one — the same clock the LWT already uses.Per query, never on the cluster. gocql round-robins coordinators, so
cl.DefaultTimestamp = falsewould push the 20-odd tables that never touch an LWT off a single monotonic client clock and onto several skewed node clocks. Strictly worse than today.It adds no Paxos round-trip. That is what makes this a routine change rather than the performance decision I flagged on #818: converging the other way means lifting
AddSessionTokentoIF NOT EXISTS, i.e. a Paxos round on every login.For completeness, both directions were measured on the
org_domainshape (LWT insert, then delete), 50 pairs each:… IF EXISTS(#818's fix)The guard
Reframed from "LWT vs plain" to the invariant that actually matters — server-stamped vs client-stamped — and it now 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.Mutation-verified, all five server-stamps:
Verification
scylladband oncassandra:4.1.TEST_DBS=sqlite go test -p 1 ./...: pass.go build ./...,go vet ./...,gofmtclean.