Repository navigation
fix(storage): stop mixing LWT and plain writes on cassandra partitions - #818
lakhansamani wants to merge 1 commit into
Conversation
Cassandra resolves conflicts by last-write-wins on the mutation timestamp. A lightweight transaction takes its timestamp from the Paxos ballot (server-side); a plain write carries gocql's client-side timestamp (DefaultTimestamp defaults to true). Those are different clocks, so a plain write issued AFTER an LWT on the same partition can carry an earlier timestamp and be discarded silently — no error, the mutation simply never applies. Two tables mixed the two, and both surfaced as read-after-write staleness in the ScyllaDB suite (#813), roughly one run in six: - user: AddUser used INSERT ... IF NOT EXISTS while UpdateUser and DeleteUser are plain. The guard was never load-bearing — it only constrained the partition key, a freshly generated UUID that never collides, and its own comment said so. Dropped, putting every write to the table in one timestamp domain at no cost. - org_domain: AddOrgDomain's LWT IS load-bearing (the normalized domain is the partition key and first-writer-wins is the atomicity guarantee, explicitly contrasted with the scim_endpoint TOCTOU race), so the two deletes gain IF EXISTS instead. The applied flag is discarded; the point is the shared Paxos ordering. A dropped delete here left a domain claimed PERMANENTLY, since nothing re-writes it. Production exposure is negligible and this is not a release blocker: the race needs the LWT insert and the later plain write within clock-skew distance, and no service path creates then modifies the same row in one request — AddUser's four call sites are never followed by an UpdateUser on the same user. Only the tests, which insert and modify microseconds apart, are close enough to lose. TestNoTableMixesLWTAndPlainWrites pins the invariant statically, because the live failure is a sub-millisecond race that cannot be asserted on reliably. It carries an explicit allow-list of the tables that still mix — authenticators, and session_tokens / oauth_states, whose LWT deletes must stay LWT because the applied flag is the exactly-once consumption arbiter. Those need their own change; the allow-list keeps the debt visible rather than silent. Refs #813
ScyllaDB run tally, and a correctionAn earlier set of runs I did for this branch was invalid and should be disregarded: I checked out Re-run cleanly on
How much this proves: the pre-fix rate was roughly 1 failure in 6 runs, so four clean runs is supporting evidence, not proof. The mechanism analysis and the static guard remain the actual argument for this change. I would rather say that than imply the runs settled it. A real finding from the discarded runs, on
|
This branch has not been deployed
1 participant
Fixes the root cause behind #813. Not a release blocker — see Impact.
Mechanism
Cassandra resolves conflicts by last-write-wins on the mutation timestamp. The two kinds of write take that timestamp from different clocks:
INSERT … IF NOT EXISTS/DELETE … IF EXISTS(LWT)USING TIMESTAMPon an LWT at all.INSERT/UPDATE/DELETEDefaultTimestamp: trueby default —gocql@v1.6.0/cluster.go:269; this repo sets no explicitUSING TIMESTAMP)So a plain write issued after an LWT on the same partition can carry an earlier timestamp and be discarded silently. No error is returned; the mutation simply never applies. Mixing LWT and non-LWT writes on one partition is the documented Cassandra/Scylla anti-pattern, and this is why.
Critically, a lost write is not "eventually consistent" — it is gone. That is why no read-consistency change fixes this and the fix has to be on the write side.
What changed
user—AddUserdroppedIF NOT EXISTS. The guard was never load-bearing and the code's own comment said so: it only ever constrained the partition key, a freshly generated UUID (user.ID = uuid.New().String()) that never collides, so it enforced no uniqueness. Email/phone uniqueness comes from the separateGetUserByEmail/GetUserByPhoneNumbercheck-then-insert and is untouched. Removing the pointless half puts every write to the table in one timestamp domain at zero cost.org_domain— here the LWT is load-bearing: the normalized domain is the partition key and first-writer-wins is the atomicity guarantee, explicitly contrasted in-code with thescim_endpointTOCTOU race. So the insert stays an LWT and the two deletes gainIF EXISTSto match. The applied flag is discarded; the point is shared Paxos ordering. A dropped delete here was the worse of the two outcomes — the domain stayed claimed permanently, since nothing re-writes it, and the test's own comment notes that an unfreed domain is "permanently unclaimable".After this,
org_domainis all-LWT anduseris all-plain.Impact — low, and I want to be precise rather than alarming
The race needs the LWT insert and the later plain write within clock-skew distance — microseconds. In real use the gap is seconds to months, so the plain write's timestamp is far ahead and always wins.
I checked for a production path with that proximity and there isn't one:
AddUser's four call sites (signup.go:228,admin_access.go:250,magic_link_login.go:89,scim/scim.go:358) are none of them followed by anUpdateUseron the same user in the same request. Only the tests, which insert and modify microseconds apart, are close enough to lose.So this is a latent correctness defect worth removing, not a live production bug. It does not warrant a patch release on top of 2.4.2.
Testing
TestNoTableMixesLWTAndPlainWritespins the invariant statically — by parsing the provider's ownfmt.Sprintfwrite statements and grouping them byschemas.Collections.X— because the live failure is a sub-millisecond race that reproduced about once in six runs and cannot be asserted on reliably.Mutation-verified: reverting either delete to a plain
DELETEmakes it fail withOrgDomain mixes LWT and plain writes on one partition.It carries a deliberate allow-list of the tables that still mix, so the remaining debt is visible rather than silent:
authenticators— an LWT insert and an LWT update sit alongside two plain updates and a plain delete; which way to converge needs deciding, not guessing.session_tokens/oauth_states— plain insert, LWT deletes. Those deletes must stay LWT: the applied flag is the exactly-once arbiter for token consumption (session_token.go:62-68). Converging means lifting the inserts, on the hottest write path in the system.Both deserve their own change. The test fails if a table leaves the allow-list without being removed from it, so the list can only shrink.
make test-scylladbagainst a real ScyllaDB container: green, including both subtests that were failing in #813 (Org_Domain_Operations,User_SCIM_Fields) plusUser_OperationsandDelete_User_Cascade. Multiple runs; results in the PR thread. Given a 1-in-6 base rate, clean runs are supporting evidence — the mechanism analysis and the static guard are the real argument.go build ./...,go vet ./...clean.