Repository navigation
fix(storage): stop mixing LWT and plain writes on cassandra partitions - #818
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
|
The first version said an LWT insert and a later plain write occupy different clock domains outright. Measurement narrows it: on Scylla the LWT's *cell* timestamps are gocql's client value, but the *row marker* is ballot-derived and sits ahead of it. A row tombstone only shadows a row whose marker it exceeds, so the hazard is specifically LWT insert vs plain row DELETE, not cell updates. 50 insert-then-delete pairs per engine: - scylladb/scylla 3.0.8 -- plain DELETE 46/50 dropped, IF EXISTS 0/50 - cassandra 4.1.12 -- plain DELETE 0/50 dropped, IF EXISTS 0/50 Cassandra escapes it only because its ballot timestamp is truncated to whole milliseconds, flooring the marker behind the later client timestamp. A margin, not a guarantee. Comments only; no behaviour change. Refs #813
Correction to my own earlier comment, with measurementsIn the tally above I reported that a 300-iteration insert-delete stress loop on
Re-measured properly, 50 LWT-insert-then-plain-delete pairs per cell, gocql v1.6.0, both engines in Docker with the engine clock ~5-6ms ahead of the host:
The row survives, no error is returned. So the defect this PR describes is real, it fires on 92% of tight pairs on the engine our own test suite runs, and Two things worth recording, including one I have not explained:
This also pins down what in #813 this does and does not fix: The probes were throwaway and are not committed — the committed guard is the static one, since the live failure depends on sub-millisecond clock lead between two processes and cannot be asserted on reliably in CI. |
Fixes the
Org_Domain_Operationsfailure reported in #813. Not a release blocker — see Impact.Mechanism
Cassandra and Scylla resolve conflicts by last-write-wins on the mutation timestamp, and an LWT does not take that timestamp from the same clock a plain write does.
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)A row tombstone only shadows a row whose marker (liveness timestamp) it exceeds. An LWT insert's marker is ballot-derived, so when the server clock runs ahead of the client, a plain
DELETEissued afterwards is discarded silently — no error, the row simply survives. A lost write here is not "eventually consistent", it is gone, which is why no read-consistency change helps and the fix has to be on the write side.Measured, not reasoned
50 LWT-insert-then-delete pairs per cell, gocql v1.6.0:
DELETEDELETE … IF EXISTSscylladb/scylla3.0.8 (whatmake test-scylladbruns)cassandra:4.1(4.1.12)Two things to record alongside that, so the limits of what was measured are clear:
UUIDGenballot ms × 1000) against 0/50 for plain writes. On Scylla by direction: 45/50 LWT writetimes land ahead of a host clock reading taken afterExec()returned, by up to 1393µs — a client-supplied timestamp is stamped before the round trip and cannot do that. Scylla's ballot simply has microsecond precision, so the precision tell doesn't discriminate there.UPDATEs escape it. A plainUPDATEafter an LWT insert measured 0/50 lost on both engines, in the identical loop shape that drops 46/50 of deletes. The drop and the fix are solid; the reason deletes and cell overwrites diverge is an open question, not something this PR claims to have answered.Apache Cassandra escapes the drop only because its ballot timestamp is truncated to whole milliseconds, which floors it behind the later client timestamp. A margin, not a guarantee.
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, andExec()never read its applied flag. Email/phone uniqueness comes from the separateGetUserByEmail/GetUserByPhoneNumbercheck-then-insert and is untouched.DeleteUser(user.go:151) is a plain row delete on that same partition, so this was the 46/50 shape. Removing the pointless half puts every write to the table in one timestamp domain and saves a Paxos round-trip on every signup.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 delete within clock-lead distance of each other — measured at ~0.6ms on Scylla. In real use the gap between creating a user or domain and deleting it is seconds to months, so the tombstone'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 a delete of the same user in the same request, andAddOrgDomain/DeleteOrgDomainare separate admin actions. Only the tests, which insert and delete 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 depends on sub-millisecond clock lead between two processes and cannot be asserted on reliably in CI.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 to LWT, which adds a Paxos round-trip to every login — a performance call, not a cleanup.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, includingOrg_Domain_OperationsandUser_SCIM_FieldsplusUser_OperationsandDelete_User_Cascade.go build ./...,go vet ./...,gofmtclean.What this does not fix
#813 also reported
User_SCIM_Fieldsfailing withIsActive=falsenot persisting. That is a cell update, which measured 0/50 lost on both engines — it is not explained by this mechanism. #813 stays open for it.