Address partition ownership overlap possibility [SPLIT UP TO 4 PRs; STALE] - #828
Conversation
070eb86 to
deabb3b
Compare
6404307 to
4424630
Compare
4424630 to
40eca48
Compare
- replace non-ASCII em-dashes with hyphens in comments/scaladoc - group cachingTransactional params into KafkaPersistenceModule.TransactionalConfig (both factories now take <=8 params), update call sites and the persistence.md snippet - shorten methods over 50 lines: extract KafkaSnapshotWriteDatabase.groupCommitSend from transactional; lift makeKeysOf/makeSnapshotPersistenceOf out of of; split staleFlushScenario into staleFlowFold/staleFlowRecords/allocateStaleFlow No behaviour change; full test matrix (Scala 2.13.18 + 3.3.7, incl. it-tests) green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous extraction left groupCommitSend itself over Codacy's 50-line method limit; moving its steps into a small private class keeps each method short. Pure code move, no behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TransactionalWriteThroughputSpec is a performance/rationale experiment, not a regression test - it adds no coverage beyond TransactionalKafkaPersistenceSpec yet ran on every CI/IT build under coverage. Gate the whole suite behind the KAFKA_FLOW_PERF env var via munitIgnore (env var, not -D, because Test/fork is on); opt in to refresh the design-doc numbers. Docs: lead each protection section with a concrete "Cost of enabling" paragraph (Paxos round-trip cost for Cassandra; measured ~15% overhead + per-partition producer/coordinator state for Kafka), separate the correctness limitations, link the design doc, and trim duplicated batch-formation prose. Drop the internal design doc from the public sidebar (still linked from persistence.md). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0f14c1e to
06af239
Compare
06af239 to
47dd994
Compare
|
The current design for Kafka as snapshot persistence is not sufficient. It neither fully have have the proper "partition ownership" through "transactional snapshot write + offset commit" as described in in #732 nor prevents stale snapshot writes. For Kafka it is a partial solution right now from my analysis. For Cassandra it should be sufficient. |
…rship coupling) Replace the epoch-only transactional snapshot mode with one that also commits the input-topic offset through the per-partition producer transaction via sendOffsetsToTransaction(offsets, consumerGroupMetadata). The broker fences a stale consumer generation (KIP-447), so a stale owner is rejected from advancing offsets AND writing snapshots together - closing the producer-epoch ordering race that fencing alone leaves open. Output stays outside the transaction (at-least-once, duplicates possible): this is corruption prevention, not exactly-once. - core: Consumer.groupMetadata, captured on rebalance (poll thread) - persistence-kafka: KafkaSnapshotWriteDatabase.transactionalWithOffsetCommit and a transactional ScheduleCommit; cachingTransactional gains inputTopic + groupMetadata; package.scala substitutes the producer-backed ScheduleCommit - docs: ownership-coupling section + wiring/limitations - tests: StatefulProcessingWithKafkaSpec wired consumer-first (real offset binding); TransactionalKafkaPersistenceSpec generation-fencing test - the no-binding KafkaSnapshotWriteDatabase.transactional is kept temporarily (tests only) and marked for removal before merge Requires skafka Producer.sendOffsetsToTransaction; depends on the local 20.1.0-sendoffsets-SNAPSHOT until that skafka change is released. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
KafkaSnapshotWriteDatabase.transactionalWithOffsetCommit is the only Kafka path that fully closes evolution-gaming#732 (binding the input-offset commit into the snapshot transaction, fenced by consumer generation). The no-binding transactional gave only producer-epoch fencing (mutual exclusion, with an init-ordering race), so it is removed and transactionalWithOffsetCommit is renamed to transactional: offset binding is now intrinsic to the Kafka transactional mode. GroupCommit takes the input partition + group metadata directly (no Option). The tests that have no consumer to take a generation from pass ConsumerGroupMetadata .Empty, which keeps binding inert while no offsets are scheduled. All specs pass (TransactionalKafkaPersistenceSpec 7, StatefulProcessingWithKafkaSpec 3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ing incidental No code-path change. Now that the input-offset commit is bound into the snapshot transaction (KIP-447 generation fencing), that is the load-bearing guarantee for evolution-gaming#732 - producer-epoch fencing is subsumed (mutual exclusion only, with an init-ordering race) and demoted to incidental defense-in-depth. - design doc: lead with "transactional write and offset commit (generation fencing)"; move epoch fencing to an "incidental" subsection; fold read_committed in; label group-commit + read_committed orthogonal; fix rejected-alternatives - persistence.md + TransactionalConfig scaladoc: the transactionalIdPrefix is no longer a correctness contract (a non-stable/colliding prefix cannot reintroduce evolution-gaming#732) - it only bounds coordinator state / avoids cross-group epoch fencing - reword KafkaSnapshotWriteConflict, cachingTransactional scaladoc and the initTransactions comment to match Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add run.sh: runs all 26 TLC configs with the right flags and asserts each outcome (holds / named invariant violation), so the suite is reproducible in one command and can't silently drift. Rewrite README.md grounded in actual TLC runs: document epochfence_stale (TLC halts at the first violated invariant, so the two epoch-fencing holes need two configs), correct the -deadlock guidance, fix guarded_holds's invariant names, and tighten the model map. Drop gc_4_2: regime-redundant with gc_3_2 (both partial-batch); its only role, larger-scope confidence, is now a coverage note (re-confirmed at N 4-5, Cap 1..N). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ty pass Three adversarial reviews found the implementation correct; this is verification, documentation, and readability work — no behaviour change. Verification: - Add GroupCommitOffset: the committed offset never leads the durable write prefix even under cap-splitting (gco_ungated is the negative control) -- the safety property GroupCommitConc abstracts away. - Drop the tautological INV_NoStaleApply / replay_fix_off_safety (verified nothing). Solution (docs/comments only): - KafkaSnapshotWriteDatabase: comment the committed-offset-never-leads-durable invariant and the safe canceled-submit behaviour. - cassandra-single-writer-design: QUORUM/QUORUM (R+W>N) recovery requirement and the ttl=None tombstone-residue caveat. Structure: - One directory per model (Model/ with its configs); run.sh auto-discovers every config and asserts the outcome each declares inline (\* expect: / \* flags:). - README rewritten: leads with a Map (model -> code -> test), then one self-contained section per model with each config listed once, plus a "What this does NOT verify". Readability (cold-read audits, then verified and fixed each finding): - Remove cryptic numbering: R1/R2/R6 -> plain descriptions; INV_F -> INV_CaptureCoupled; gctx -> capturedGen; revive_r1/r2 -> revive_remove/revive_tombstone; genfence_decoupled_F -> genfence_decoupled_coupling. - Fix the stale INV_NoStaleApply reference; align "freshness" -> "recovered generation"; uniform terminology, first-use glosses (LWT/IT/hw), and a evolution-gaming#732/evolution-gaming#828 pointer per header. - Re-justify every model header comment box to a uniform width. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the TLA+ model suite with a single abstract specification, SingleWriterStore, that each backend is model-checked to refine (Cassandra, Kafka); the rejected design as a spec whose refinement theorem is false (Epoch); and the finer grain-of-atomicity / liveness concerns as refinements underneath (CasFirstWrite, ReplayFence, GroupCommit). Correctness of a backend means Backend => SingleWriterStore, checked in TLC by a refinement mapping. Each backend models its fence faithfully, so its hazards are reachable rather than abstracted away: Cassandra folds onto its recovered base (the delete-then-revive contents corruption) and gates on the stored offset + the offset-carrying tombstone; Kafka captures the consumer generation coupled to flow teardown, and seeds the offset. Flat layout (the tower is cross-module); the self-checking run.sh classifies HOLDS / VIOLATES / VIOLATES-TEMPORAL / VIOLATES-REFINEMENT and runs -workers auto. 24 configs, all asserted (./run.sh green); 13 are expected failures (the pairing that makes a green run mean something). Reconcile the two single-writer design docs to the tower, and add the paired integration tests (concurrent first-writers race; the committed offset never leads the durable write prefix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Scalastyle NonASCIICharacterChecker flagged a `…` (U+2026) in the compare-and-set delete Scaladoc; replace with `...`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codacy ShellCheck flagged the unquoted $flags on the TLC invocation
(word-splitting). Read the optional `\* flags:` directive into a bash
array and expand it quoted and nounset-safe (`"${flags[@]+"${flags[@]}"}"`),
which also works under bash 3.2 (macOS) where an empty `"${arr[@]}"`
trips `set -u`. Full suite still 24/24.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Consumer.onPartitionsLost: drop the groupMetadata capture so the wrapped listener's cleanup always runs; a fenced consumer cannot commit and a capture failure cannot be recovered inside a RebalanceCallback. - SnapshotsOf.backedBy: document that it has no offset fence and that a compare-and-set (KafkaSnapshot) backend must wire through snapshotsOf. - CassandraSnapshots.withSchema: note CAS-mode deletes are logical tombstones reaped only by ttl, so ttl bounds table/Paxos growth. - KafkaPersistenceModule: build snapshotTopicPartition once and pass it into transactionalWriteDatabase instead of reconstructing it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Consumer: capture group metadata on assignment only; revoke/lost run the wrapped listener directly. The generation only changes at the next assignment (which re-captures), so a refresh on revoke is redundant, and running the listener unconditionally guarantees its cleanup (flush, commit-on-revoke) is never suppressed by a groupMetadata failure. - PartitionFlow: note the periodic commit is intentionally not error-handled like the revoke path - a fenced periodic commit must crash the stale instance; that is the fencing. - persistence.md: attribute the swallowed flush-on-revoke conflict to the cache (scache) release-error handling rather than kafka-flow. - models/run.sh: fail loudly (exit 2) when a filter matches zero configs instead of reporting a vacuous pass. - GroupCommitSpec: drop a dangling "invariant F" label (no such scheme). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- run.sh: the zero-match message used literal single quotes around $filter, which ShellCheck flags as a non-expanding expression (SC2016); use escaped double quotes so the intent (and expansion) is unambiguous. - CassandraSnapshots: rewrap the prepareDelete docstring to <=120 cols so scalafmtCheckAll passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pshot Replace the experimental `S <: KafkaSnapshot[_]` bound on the snapshot buffer/wiring with a `ToOffset[S]` typeclass constraint. ToOffset already exists (KafkaSnapshot provides an instance via its companion), so the offset that keeps the buffer monotonic and fences a delete on the key's high-water offset is supplied per type rather than by subtyping or a default argument. - core: Snapshots.of/apply, SnapshotsOf.backedBy/memory now require S: ToOffset and read the offset via the instance; SnapshotDatabase.snapshotsOf relies on KafkaSnapshot's companion instance. - persistence-kafka: KafkaPersistenceModule(Of) factories require S: ToOffset. - tests: offset-carrying snapshots use their natural ToOffset; offset-less backends (Kafka transactional/caching, in-memory) supply `_ => Offset.min`, the explicit no-fence equivalent (fencing there is by transactional generation, not a per-value offset). - docs/comments updated to describe the ToOffset wiring. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Convert the S: ToOffset context bounds (added in 471826e) to explicit toOffset: ToOffset[S] implicit parameters across Snapshots.of, SnapshotsOf.memory/backedBy, and the KafkaPersistenceModule(Of) caching/ cachingTransactional factories. Behaviourally identical - callers resolve toOffset from the same implicit scope the context bound used. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add SnapshotsOfSpec asserting that backedBy and memory, over an offset-carrying KafkaSnapshot type, fence on the snapshot's offset: a lower-offset replayed append is dropped and the high-water snapshot survives. With a neutral _ => Offset.min extractor the later lower-offset append would overwrite it, so this fails loudly if the offset fence is ever silently disabled on these paths - a behavioural guard for what was previously only a compile-time constraint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r BYO Drop the ToOffset constraint (and the major-bump-forcing ceremony it added on the Kafka and bring-your-own-storage surfaces) and instead pass an explicit offsetOf: S => Offset to the per-key buffer: - Snapshots.of/apply take offsetOf directly; the buffer is monotonic and the delete fence is computed from it. - SnapshotsOf.backedBy is plain last-write-wins; a second overload backedBy(db, offsetOf) opts a custom offset-carrying store into fencing without adopting KafkaSnapshot. - SnapshotDatabase.snapshotsOf (the KafkaSnapshot path) passes _.offset, so the Cassandra compare-and-set fence stays live; the Kafka module needs no offset and is untouched by the fence. The ToOffset typeclass itself is unchanged (still used by SnapshotFold); it just no longer leaks into the snapshot-buffer, Kafka, or BYO APIs. Tests: SnapshotsSpec drives offsetOf explicitly; SnapshotsOfSpec pins the three behaviours (snapshotsOf fences, backedBy is last-write-wins, backedBy+offsetOf fences a custom snapshot type). Docs: persistence.md gains a "Custom snapshot storage" section; the design doc describes offsetOf instead of ToOffset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
I think we have something now that works with enough confidence - to review the production code part. It is mildly put bloated though with tests, documentation and the TLA+ [1] models. The latter part gives me more confidence, but it is basically me trying to learn formal verification, so take that AI assisted modeling with a huge grain of salt, even if I have tried to refine it. I think the regular testing, unit tests and integration tests looks good overall. Excluding the models that probably should be dropped, the most controversial I guess is now that [1] https://lamport.azurewebsites.net/tla/book.html?back-link=learning.html#book |
The "Custom snapshot storage" note now spells out that delete(key, offset) receives the offset as a parameter while persist only gets the snapshot value, so a custom store can gate deletes but can only gate persists if the offset is part of an offset-carrying snapshot type — a plain offset-less domain state stays effectively last-write-wins. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There is no default - backedBy(db) is the last-write-wins overload and backedBy(db, offsetOf) is the fenced one; say so. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "Custom snapshot storage" note conflated the two: the stale-writer fence (evolution-gaming#732 safety) is the store's own conditional persist/delete, while the buffer's offsetOf (backedBy(db, offsetOf) / snapshotsOf) is the replay fence - a liveness fix that stops a recovering owner self-fencing on its own conditional writes. Make that explicit, and keep the persist/delete offset-source asymmetry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The replay fence guards two replay-window cases: a re-derived persist below the high-water and a tick-driven delete. SnapshotFold's offset filter already prevents the former (so the monotonic append is belt-and-suspenders there), but the delete comes from a tick that bypasses the fold, making the delete side irreducible. Skip-duplicates and the tombstone cover the persist and resurrection cases, not this one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Restructure docs/cassandra-single-writer-design.md to focus on why over what: brief mechanics, explicit design choices, and the learnings from the deferred full solution, mirroring the Kafka design doc's shape (Problem, mechanism, Testing, Rejected alternatives, Forward-looking): - Problem now explains why the Kafka fix does not transfer to Cassandra (no transaction to bind the offset commit into, no ownership authority), motivating a per-write, per-key fence. - The mechanism is kept brief and its three load-bearing choices are called out explicitly: per-key granularity, `<=` over `<`, and ordering by data rather than identity. - "Why the store is the whole change" makes the SnapshotFold replay dedup explicit as the property that lets persist-only ship without core changes. - A Compatibility section collects the no-API-change / no-migration / write-side-only-fence consequences. - The deferred full solution (offset-gated deletes, PR evolution-gaming#834) is retold as a forced chain - tombstone, breaking delete(key, offset) API, monotonic buffer, tombstone-floor recovery read, independent events-recovery seeding - with a livelock diagram, the TLA+ results (all defects were liveness, never safety), and the explicit deferral rationale (cost/benefit and KIP-939 timing). - A process note links the stacked PRs (evolution-gaming#828/evolution-gaming#833/evolution-gaming#834/evolution-gaming#835/evolution-gaming#838) so the git history, review conversations and models stay discoverable. Also make the rolling-deploy clock-skew caveat in persistence.md concrete (coordinator vs client write timestamps), and update a test comment referencing a renamed doc section. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018jfVxUNrjxpcSFF2hgiar5
SPLIT UP TO:
Protect snapshot persistence from stale writers (single-writer guarantees)
Problem
During a partition rebalance, the previous owner and the new owner of a partition can briefly
overlap. If the old owner flushes its (now stale) in-memory state after the new owner has already
recovered and advanced, the stale flush overwrites the newer snapshot — the classic
ownership-overlap corruption (reproduced as issue #732: a stale flush-on-revoke silently
overwrites the newer snapshot, and the lost events never come back).
kafka-flow had no mechanism to reject a write from a writer that no longer owns the partition. This
PR adds one for both built-in snapshot backends (behind opt-in flags), and gives a custom backend a
hook to do the same.
Solution
Two backends, two fencing mechanisms, one shared invariant: a write from a writer that no longer
owns the partition must not land.
Cassandra — compare-and-set (
compareAndSet = true)(
IF offset <= :offset): a stale writer carrying a lower offset is rejected withSnapshotWriteConflictinstead of overwriting the newer snapshot.SET value = null, keeping the row'soffset) rather than a hardDELETE. Keeping the row preserves the offset guard across thedelete, so a stale writer can neither erase a newer snapshot nor resurrect the key at a lower
offset.
getreads the tombstone back as absent. The tombstone is reaped by the optionalttl.is not self-fenced by the owner's own higher-water offset (this was a liveness trap — fixed and
regression-tested).
Kafka — transactional offset binding (
cachingTransactional)(
sendOffsetsToTransaction), so the snapshot and the offset advance atomically.aborting the transaction, so neither the writes nor the offset land. The consumer's
group metadata is captured on assignment and read live per transaction.
read_committed, so records from an aborted (fenced) transaction are neverrecovered as snapshots. Output stays at-least-once.
Custom storage (bring-your-own)
A custom
SnapshotDatabaseis last-write-wins — exposed to #732 like last-write-wins Cassandra — unlessits own
persist/deletereject a write when a newer offset is stored, the wayCassandraSnapshotscompare-and-set does. That conditional write is the stale-writer fence; the buffer wiring is not.
Conditional writes then need the buffer's replay fence for liveness —
backedBy(db, offsetOf)(your ownoffset-carrying type) or
KafkaSnapshot[S]via.snapshotsOf— otherwise the owner self-fences duringrecovery (its current offset lags a recovered snapshot's) and crashes. See "Custom snapshot storage" in
docs/persistence.md.Formal models (TLA+)
The designs are backed by a refinement tower (Specifying Systems §5.8): one abstract spec,
SingleWriterStore, defines the safety + liveness contract, and each backend refines it —Cassandra(offset compare-and-set + tombstone) andKafka(the consumer-generation fence,KIP-447). Finer-grained models sit under the backend they belong to:
GroupCommit(the Kafka writeorchestration — termination + offset ordering),
CasFirstWrite/CasFirstWriteAtomic(thenon-atomic first-write compound refining one atomic CAS), and
ReplayFence(the replay-windowself-fence / liveness).
Crucially the suite also encodes rejected designs and removed guards that must fail model
checking. The clearest is
Epoch— the producer-epoch variant (a stabletransactional.id)whose refinement theorem is deliberately false: epochs are handed out in
initTransactionsarrivalorder, independent of ownership, so a late-initialising stale owner can win the epoch and its write
lands. That counterexample is precisely why the Kafka path fences on the consumer generation, not
the producer epoch. Alongside it sit the removed-guard variants (unguarded, ungated, unseeded,
not-tombstoned, decoupled…). 13 of the 24 configs are expected failures, so a green run means the
guards are load-bearing — not that only the happy path was checked.
models/run.shis self-checkingand runs the whole suite.
Testing
GroupCommitSpec(group-commit orchestration against a fake producer),SnapshotReplayFencingSpec(offset-gated CAS semantics: resurrection, equal-offset, idempotentreplay),
SnapshotsOfSpec(the wiring fences/doesn't fence as expected), plusSnapshots/Persistenceunit coverage.TransactionalKafkaPersistenceSpec,TransactionalWriteThroughputSpec, and the issue-Partition ownership overlap during unideal circumstances #732 reproduction + prevention pair inFlowSpec/SnapshotSpec.Documentation
docs/cassandra-single-writer-design.mdanddocs/kafka-single-writer-design.md— design,limitations, costs, and rollout for each backend.
docs/persistence.md— how a stale write is rejected and handled uniformly across backends, plusa "Custom snapshot storage" section for bring-your-own backends.
Change breakdown
Of 4,986 added lines across 74 files:
Roughly 58% code · 29% models · 13% docs — and ~69% of the diff (tests + models) is
verification, reflecting that the value here is the guarantee, not the line count.
Compatibility & rollout
SnapshotWriteDatabase.deletegains anoffsetparameter so deletescan be offset-gated. This is a source- and binary-incompatible change for anyone implementing a
custom snapshot backend (the built-in backends are updated here).
SnapshotsOf.backedBy(db, offsetOf)overload lets a custom offset-carryingstore opt into the buffer's offset fence; the existing
backedBy(db)stays last-write-wins.non-transactional) is unchanged, so deployments on the built-in modules with default settings
behave as before.
design doc); the Kafka path reaches full protection once all instances are transactional.
ttlto bound tombstone (and Paxos-partition) growth.