Prove the transactional fence holds on the error-path teardown route (#732) - #27
Prove the transactional fence holds on the error-path teardown route (#732)#27tobiajo wants to merge 2 commits into
Conversation
An error escaping the poll loop tears the stream down, and TopicFlow's release flushes every cached PartitionFlow - including partitions the broker already reassigned (a fold stalled past max.poll.interval.ms gets the member evicted while its flows stay cached). In the default non-transactional Kafka persistence that flush is a plain producer.send with no fence of any kind, so the dying member's stale snapshot lands AFTER the new owner's fresher one while the committed input offset stays at the newer value; the records between the two snapshots are never re-folded - durable data loss, the shape of issue evolution-gaming#732. The spec drives the untouched production machinery (KafkaFlow + kafkaEagerRecovery + KafkaPersistenceModuleOf) with two live members on one broker and evicts the first through a genuine poll-interval stall, then runs that one scenario against both persistences, differing only in the assertions: caching: stale "123" lands over the fresher snapshot, the next owner recovers it at offset 6 and persists "1236" at offset 7 - records 4 and 5 gone cachingTransactional: the fresher "12345" survives, the next owner recovers the consistent pair and carries it forward as "123456" at offset 7 Why both halves matter over the existing evolution-gaming#732 pair in TransactionalKafkaPersistenceSpec, which simulates the overlap with two PartitionFlows over one partition: - the overlap here is real - a broker evicts a real member - so the teardown route is shown to be reachable, not just constructible; - the offset dimension is asserted, and A's teardown offset commit does NOT clobber B's (rejected as a kicked member, swallowed in TopicFlow.commitPending), which is what makes the surviving (stale snapshot, newer offset) pair silently corrupt; - the next owner's own durable outcome is asserted, which is what makes the loss unrecoverable rather than transient: it persists the damaged aggregate and commits past the offsets that would have re-delivered records 4 and 5. For the transactional mode the same column states the invariant positively - every record up to the committed offset is in the persisted snapshot; - transactional safety on THIS route was previously inference (the existing prevention test covers the revoke callback). Running it also settled which fence fires: the producer epoch, not the generation. Under the partition's shared stable transactional.id the new owner's initTransactions has already bumped the epoch, so the broker rejects the flush with InvalidProducerEpochException before it reaches the generation-gated offset commit - the reason A's release still succeeds (the rejection surfaces only as a swallowed, logged scache entry-release error). The caching assertions characterize documented, accepted behavior rather than a fix waiting to happen: last-write-wins stays exposed by design and the transactional mode is the protection (docs/persistence.md). They should be inverted only if the default mode ever gains a fence. What the route does expose as narrowly fixable, independently of the write path, is that teardown flushes every cached partition where the revocation path knows which partitions are gone; not attempted here. The post-teardown observation is a single read behind an explicit release barrier, not an eventually-poll: the dying member is `allocated`, so its release - the flush under test - runs at a controlled point, and the write path awaits the broker's ack. Once release returns, an accepted write is already durable and visible. That makes the reproduction deterministic and, more importantly, makes the prevention assertion sound: there is no settling window that could pass merely because the flush had not happened yet. Tests only - no src/main changes.
The non-transactional mode's exposure is known, documented (docs/persistence.md) and already reproduced by TransactionalKafkaPersistenceSpec's "issue evolution-gaming#732 reproduction". Asserting it a third time bought nothing, cost ~100s of the suite's 145s, and left behind a test that fails if the default mode is ever improved. Dropped. What remains is the half that was not already covered: the transactional mode on the TEARDOWN route, where its safety was previously inference (the existing prevention tests drive the revoke callback with two PartitionFlows and a fabricated stale generation; here a real broker evicts a real member). Dropping the repro removes the contrast that showed the scenario was live, so replace it with a direct assertion instead of relying on it. The dying member's snapshot writer is wrapped (`recording`) to capture the outcome of every broker-answered transactional call, and the test now asserts that its teardown flush was ATTEMPTED and was REJECTED for its producer epoch - InvalidProducerEpochException or ProducerFencedException, whichever the transaction protocol version surfaces. That also pins which fence fires where it is observed rather than inferred from a log line. Verified the guard is load-bearing, not decoration: with flushOnRevoke = false on the dying member every snapshot and offset assertion still holds, and the test fails on the vacuity check ("member A never attempted a snapshot write") - which is exactly the way a prevention-only test rots into proving nothing. Renamed to TeardownFlushFencingSpec: it now asserts a fence holds rather than characterizing an unfenced write. 45s for the suite, down from 145s. 2 runs green against a real broker.
|
Closing: the finding is recorded in the verification corpus instead, which is the better home for it. The reasoning, for the record. The marginal regression value over the retained pins is thin — the mechanism this asserts (epoch-fencing a stale writer's flush) is already pinned by the revoke-route test, and the teardown-specific edges (seeded first flush, the group-metadata None window) have their own tests. Break either fence and those fail too. Worse, the anti-vacuity guard added in the last push would fail the day teardown is fixed to skip reassigned partitions — the same defect as the 'must be inverted when fixed' framing this PR started with, one layer down, and not repairable without giving the guard up. What the work actually produced was a finding, not an artifact:
Both are now in the corpus on the verification branch: a new experiment section plus claim KF17, and the eviction-path clause that named Follow-up still open and unattempted: teardown flushes every cached partition where |
What this is
One new IT spec, tests only — no
src/mainand no docs changes. Split out of #25 (theambiguous-commit half is now #26).
It proves the transactional Kafka persistence fences a stale flush on the error-path teardown
route, through a live rebalance and the untouched production machinery (
KafkaFlow+kafkaEagerRecovery+KafkaPersistenceModuleOf.cachingTransactional).Why this route
The existing evolution-gaming#732 prevention tests in
TransactionalKafkaPersistenceSpecdrive the revokecallback, with two
PartitionFlows over one partition and a fabricated stale generation. This is adifferent path through kafka-flow, exercised for real:
max.poll.interval.ms. A real broker evicts it andhands the partition to member B while A still holds buffered, never-persisted state.
snapshot and commits a newer input offset.
TopicFlow'srelease drops every cached
PartitionFlow— not just the revoked ones, because teardown hasno way to know a partition was already taken away.
flushOnRevokeflushes A's stale state.So transactional safety on this route was previously inference. It no longer is.
What it asserts
12345), paired with the offset the new owner committed (6).wrapped to record the outcome of every broker-answered transactional call. Without this, a passing
prevention assertion could equally mean no flush ever happened.
transactional.id, the new owner'sinitTransactionshas already bumped the epoch, so the brokerrejects the flush (
InvalidProducerEpochException, orProducerFencedExceptiondepending ontransaction protocol version) before it reaches the generation-gated offset commit — which would
raise
CommitFailedExceptioninstead. Asserted where it is observed, not inferred from a log line.12345atoffset 6 and carries it forward as
123456at offset7— every record up to the offset itcommits is in the snapshot it persists.
A's teardown offset commit does not clobber B's either (rejected as a kicked member, swallowed in
TopicFlow.commitPending), so on the unfenced write path the surviving pair would be a stalesnapshot at a newer offset: silent loss. That is what the fence prevents here.
What was dropped since the first push
The original version also ran the same scenario against
cachingand asserted the corrupted outcome.That is removed: the non-transactional exposure is known, documented (
docs/persistence.md) andalready reproduced by
TransactionalKafkaPersistenceSpec's "issue evolution-gaming#732 reproduction", so a thirdassertion of it bought nothing, cost ~100 s of the suite's 145 s, and left behind a test that fails if
the default mode is ever improved.
Dropping it removed the contrast that showed the scenario was live, which is why the
attempted-and-rejected assertion above replaced it — a direct check instead of a costly implicit one.
Determinism
The post-teardown observation is a single read behind an explicit release barrier, not an
eventuallypoll: the dying member is
allocated, so its release — the flush under test — runs at a controlledpoint, and the write path awaits the broker's ack. Once release returns, an accepted write is already
durable and visible. No settling window that could pass merely because the flush had not happened yet.
The next-owner wait is a liveness gate only (committed past record 6); the exact offset is asserted,
not gated on, so a wrong value fails instead of timing out.
Verification
sbt -batch scalafmtCheckAll— clean.sbt -batch "persistence-kafka-it-tests/testOnly *TeardownFlushFencingSpec"— green against a realbroker, 2 runs, ~45 s (down from ~145 s).
flushOnRevoke = falseon the dying member, everysnapshot and offset assertion still holds and the test fails on the vacuity check (
member A never attempted a snapshot write, so the fence was never exercised) — confirming the guard catchesexactly the case that would otherwise pass while proving nothing.
Follow-up, not attempted here
The route exposes something narrowly fixable, independently of the write path: teardown flushes every
cached partition (
TopicFlow'sResource.makerelease) where the revocation path(
TopicFlow.remove) knows which partitions are gone. Noted in the spec's scaladoc; worth its ownissue.
Test plan
scalafmtCheckAllcleansrc/mainand zero docs changes in the diff