From f9d9e548814028730fb275c846e3dfbab10cdc5c Mon Sep 17 00:00:00 2001 From: Jon Haddad Date: Thu, 11 Jun 2026 13:18:10 -0700 Subject: [PATCH] Fix dead tombstone-vs-expiring tie-break in cursor cell reconciliation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ReusableLivenessInfo.isExpiring() means "has an expiration time", which is also true for tombstones (a deletion also carries a localExpirationTime). The tie-break used !left.isExpiring() to mean "is a tombstone", so it was false for BOTH sides whenever both already passed the earlier presence check — making the tie-break dead code and letting the localDeletionTime comparison below it pick an expiring cell over a genuine tombstone at the same timestamp. Tombstone semantics need ttl() == NO_TTL instead: within this code path both sides already have localExpirationTime set, so ttl == NO_TTL is exactly "is a tombstone" and mirrors Cells.resolveRegular's actual rule. EdgeCaseDifferentialCompactionTest.tombstoneVsExpiringTies pins both flush orders and both tombstone shapes (UPDATE SET v = null, DELETE v). --- .../db/compaction/CursorCompactor.java | 11 +++++- .../EdgeCaseDifferentialCompactionTest.java | 37 +++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/src/java/org/apache/cassandra/db/compaction/CursorCompactor.java b/src/java/org/apache/cassandra/db/compaction/CursorCompactor.java index 74610c541f..bd48cfa3b5 100644 --- a/src/java/org/apache/cassandra/db/compaction/CursorCompactor.java +++ b/src/java/org/apache/cassandra/db/compaction/CursorCompactor.java @@ -837,8 +837,15 @@ public class CursorCompactor extends CompactionInfo.Holder // (i.e. before expiry, the pure tombstone; after expiry, whichever is more recent) // this inconsistency has no user-visible distinction, as at this point they are both logically tombstones // (the only possible difference is the time at which the cells become purgeable) - boolean leftIsTombstone = !left.isExpiring(); // !isExpiring() == isTombstone(), but does not need to consider localDeletionTime() - boolean rightIsTombstone = !right.isExpiring(); + // NOTE: ReusableLivenessInfo.isExpiring() means "has an expiration time", which is + // also true for tombstones — !isExpiring() would be false for BOTH sides here (both + // have an expiration time once we are past the presence check above), making this + // tie-break dead and letting the localDeletionTime comparison below pick an expiring + // cell over a tombstone. Tombstone semantics need AbstractCell.isTombstone(): + // within this block both sides have localExpirationTime set, so ttl == NO_TTL is + // exactly "is a tombstone" (mirrors Cells.resolveRegular's !left.isExpiring()). + boolean leftIsTombstone = left.ttl() == LivenessInfo.NO_TTL; + boolean rightIsTombstone = right.ttl() == LivenessInfo.NO_TTL; if (leftIsTombstone != rightIsTombstone) return leftIsTombstone ? LEFT : RIGHT; diff --git a/test/unit/org/apache/cassandra/db/compaction/differential/EdgeCaseDifferentialCompactionTest.java b/test/unit/org/apache/cassandra/db/compaction/differential/EdgeCaseDifferentialCompactionTest.java index 6df2864949..d5d93e96ba 100644 --- a/test/unit/org/apache/cassandra/db/compaction/differential/EdgeCaseDifferentialCompactionTest.java +++ b/test/unit/org/apache/cassandra/db/compaction/differential/EdgeCaseDifferentialCompactionTest.java @@ -538,6 +538,43 @@ public class EdgeCaseDifferentialCompactionTest extends DifferentialCompactionTe assertCursorMatchesIteratorAcrossGenerations(cfs); } + /** + * Cell TOMBSTONE vs EXPIRING cell at the SAME timestamp: rule (b) of the reference + * decision table (Cells.resolveRegular) — the tombstone wins the tie, BEFORE any + * localDeletionTime comparison. This is the one same-timestamp pairing where both + * sides carry a localExpirationTime (tombstone: deletion second; expiring: expiry + * second), so a resolver that classifies "tombstone" via the loose + * ReusableLivenessInfo.isExpiring() predicate (true for tombstones too) never + * fires rule (b) and falls through to the ldt compare, + * which the expiring cell's future expiry second wins: deleted data resurrected + * until the TTL lapses. Both flush orders and both tombstone shapes + * (UPDATE SET v = null, DELETE v) across distinct partitions. + */ + @Test + public void tombstoneVsExpiringTies() throws Exception + { + createTable("CREATE TABLE %s (pk bigint, ck bigint, v text, PRIMARY KEY (pk, ck)) " + + "WITH gc_grace_seconds = 864000"); + ColumnFamilyStore cfs = getCurrentColumnFamilyStore(); + cfs.disableAutoCompaction(); + + // pk 1: tombstone flushed first, expiring second; tombstone via UPDATE SET null + for (long ck = 0; ck < 5; ck++) + execute("UPDATE %s USING TIMESTAMP 5000 SET v = null WHERE pk = ? AND ck = ?", 1L, ck); + // pk 2: expiring flushed first, tombstone second; tombstone via DELETE column + for (long ck = 0; ck < 5; ck++) + execute("UPDATE %s USING TTL 86400 AND TIMESTAMP 5000 SET v = ? WHERE pk = ? AND ck = ?", "live" + ck, 2L, ck); + flush(); + + for (long ck = 0; ck < 5; ck++) + execute("UPDATE %s USING TTL 86400 AND TIMESTAMP 5000 SET v = ? WHERE pk = ? AND ck = ?", "live" + ck, 1L, ck); + for (long ck = 0; ck < 5; ck++) + execute("DELETE v FROM %s USING TIMESTAMP 5000 WHERE pk = ? AND ck = ?", 2L, ck); + flush(); + + assertCursorMatchesIteratorAcrossGenerations(cfs); + } + /** Vector and duration columns: fixed-dimension float vectors and the * variable-length duration encoding as ordinary single cells, overwritten and * null-overwritten (cell tombstone) across sstables. */