From cd2066781101675afdaef3e1cca35fa8770f6c01 Mon Sep 17 00:00:00 2001 From: Arjun Ashok Date: Tue, 7 Jul 2026 18:11:51 -0700 Subject: [PATCH] CASSANDRA-21356: Fix same-timestamp tombstone/expiring-cell tie-break in cursor compaction --- CHANGES.txt | 1 + .../cassandra/db/ReusableLivenessInfo.java | 4 +- .../db/compaction/CursorCompactor.java | 4 +- .../db/ReusableLivenessInfoTest.java | 57 ++++++++++ .../CursorCompactionEquivalenceTest.java | 103 ++++++++++++++++++ 5 files changed, 166 insertions(+), 3 deletions(-) create mode 100644 test/unit/org/apache/cassandra/db/ReusableLivenessInfoTest.java create mode 100644 test/unit/org/apache/cassandra/db/compaction/CursorCompactionEquivalenceTest.java diff --git a/CHANGES.txt b/CHANGES.txt index 5b33484d30c7..87e76468fb3a 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 7.0 + * Fix same-timestamp tombstone/expiring-cell tie-break in cursor compaction (CASSANDRA-21356) * Don't increment client metrics on messaging service connection unpause (CASSANDRA-21491) * Add nodetool getreplicas (CASSANDRA-17665) * Implementation of CEP-49: Hardware-accelerated compression (CASSANDRA-20975) diff --git a/src/java/org/apache/cassandra/db/ReusableLivenessInfo.java b/src/java/org/apache/cassandra/db/ReusableLivenessInfo.java index 1bc2b5cb4f85..5560945964e9 100644 --- a/src/java/org/apache/cassandra/db/ReusableLivenessInfo.java +++ b/src/java/org/apache/cassandra/db/ReusableLivenessInfo.java @@ -46,7 +46,9 @@ public long localExpirationTime() @Override public boolean isExpiring() { - return localExpirationTime != NO_EXPIRATION_TIME; + // Check for TTL (not localExpirationTime as it will incorrectly return true for tombstones) + // Matches AbstractCell.isExpiring(). + return ttl != NO_TTL; } @Override diff --git a/src/java/org/apache/cassandra/db/compaction/CursorCompactor.java b/src/java/org/apache/cassandra/db/compaction/CursorCompactor.java index 7ffe51c89676..4376774cbbe9 100644 --- a/src/java/org/apache/cassandra/db/compaction/CursorCompactor.java +++ b/src/java/org/apache/cassandra/db/compaction/CursorCompactor.java @@ -828,8 +828,8 @@ private static CellResolution resolveRegular(LivenessInfo left, LivenessInfo rig // (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(); + boolean leftIsTombstone = left.ttl() == LivenessInfo.NO_TTL; // ttl=0 → tombstone; ttl>0 → expiring + boolean rightIsTombstone = right.ttl() == LivenessInfo.NO_TTL; if (leftIsTombstone != rightIsTombstone) return leftIsTombstone ? LEFT : RIGHT; diff --git a/test/unit/org/apache/cassandra/db/ReusableLivenessInfoTest.java b/test/unit/org/apache/cassandra/db/ReusableLivenessInfoTest.java new file mode 100644 index 000000000000..24139f6b4ddb --- /dev/null +++ b/test/unit/org/apache/cassandra/db/ReusableLivenessInfoTest.java @@ -0,0 +1,57 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.db; + +import org.junit.Test; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +/** + * CASSANDRA-21356: ReusableLivenessInfo.isExpiring() checked {@code localExpirationTime != + * NO_EXPIRATION_TIME} instead of {@code ttl != NO_TTL}. A tombstone cell also has a non-default + * localExpirationTime (it stores the deletion timestamp there), so the old check returned true + * for tombstones as well as expiring cells — violating the LivenessInfo contract that + * IS_DELETED_MASK and IS_EXPIRING_MASK are mutually exclusive, and matching the canonical + * definition in AbstractCell.isExpiring() (ttl() != NO_TTL). + */ +public class ReusableLivenessInfoTest +{ + @Test + public void tombstoneIsNotExpiring() + { + ReusableLivenessInfo info = new ReusableLivenessInfo(); + // A tombstone cell (e.g. from INSERT ... null or DELETE): ttl is NO_TTL, but + // localExpirationTime is still set — it stores the deletion timestamp. + info.reset(1L, LivenessInfo.NO_TTL, 12345L); + assertTrue("ttl=NO_TTL with a set localExpirationTime is a tombstone", info.isTombstone()); + assertFalse("isExpiring() must not fire for a tombstone cell: it and IS_DELETED_MASK " + + "are mutually exclusive in the SSTable format", + info.isExpiring()); + } + + @Test + public void expiringCellIsExpiringNotTombstone() + { + ReusableLivenessInfo info = new ReusableLivenessInfo(); + info.reset(1L, 3600, 12345L); + assertTrue("A cell with a positive ttl is expiring", info.isExpiring()); + assertFalse("An expiring cell is not a tombstone", info.isTombstone()); + } +} diff --git a/test/unit/org/apache/cassandra/db/compaction/CursorCompactionEquivalenceTest.java b/test/unit/org/apache/cassandra/db/compaction/CursorCompactionEquivalenceTest.java new file mode 100644 index 000000000000..9cd6589086e5 --- /dev/null +++ b/test/unit/org/apache/cassandra/db/compaction/CursorCompactionEquivalenceTest.java @@ -0,0 +1,103 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.db.compaction; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.cql3.CQLTester; +import org.apache.cassandra.cql3.UntypedResultSet; +import org.apache.cassandra.db.ColumnFamilyStore; +import org.apache.cassandra.io.sstable.format.big.BigFormat; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assume.assumeTrue; + +/** + * Validates the logical correctness of cursor compaction's resolveRegular() same-timestamp + * tie-break, the bug CASSANDRA-21356 owns: a tombstone cell must beat an expiring cell at an + * identical timestamp. + * + * Before the fix, ReusableLivenessInfo.isExpiring() checked {@code localExpirationTime != + * NO_EXPIRATION_TIME} instead of {@code ttl != NO_TTL}. A tombstone cell also has a non-default + * localExpirationTime (it stores the deletion timestamp there), so isExpiring() returned true for + * both tombstone and expiring cells. resolveRegular() used {@code !isExpiring()} to identify + * tombstones, so both cells looked identical to it and it fell through to comparing + * localExpirationTime values — an expiring cell's is a future timestamp, a tombstone's is a past + * deletion timestamp, so the expiring cell always won, resurrecting an explicitly deleted column. + * See ReusableLivenessInfoTest for direct unit coverage of the root-cause isExpiring() check + * itself (the general tombstone case, independent of any tie-break). + * + * This is asserted by querying the compacted table back rather than by comparing raw Data.db / + * Index.db bytes against the iterator compaction path. SSTableCursorWriter has other, unrelated + * byte-encoding gaps (tracked separately as CASSANDRA-21336, CASSANDRA-21357, and CASSANDRA-21358) + * that make cursor and iterator compaction produce different raw bytes for reasons that have + * nothing to do with this bug — a byte-for-byte comparison here would fail regardless of whether + * this specific bug is fixed, so this test checks queryable behavior instead. + */ +public class CursorCompactionEquivalenceTest extends CQLTester +{ + private boolean origCursorEnabled; + + @Before + public void guardAndSave() + { + assumeTrue("Cursor compaction requires BIG SSTable format", BigFormat.isSelected()); + origCursorEnabled = DatabaseDescriptor.cursorCompactionEnabled(); + DatabaseDescriptor.setCursorCompactionEnabled(true); + } + + @After + public void restore() + { + DatabaseDescriptor.setCursorCompactionEnabled(origCursorEnabled); + } + + // ── same-timestamp tombstone vs expiring cell tie-break ────────────────────── + // Exercises resolveRegular(): tombstone must beat expiring cell at identical timestamp. + // Without the fix, ReusableLivenessInfo.isExpiring() returns true for tombstones, + // causing resolveRegular() to misidentify the tombstone and pick the expiring cell instead. + + @Test + public void testSameTimestampTieBreak() throws Throwable + { + createTable("CREATE TABLE %s (pk int, ck int, v text, PRIMARY KEY (pk, ck))" + + " WITH compression = {'enabled': 'false'}"); + ColumnFamilyStore cfs = getCurrentColumnFamilyStore(); + cfs.disableAutoCompaction(); + + // SSTable 1: tombstone cell for v at timestamp 100 + execute("INSERT INTO %s (pk, ck, v) VALUES (0, 0, null) USING TIMESTAMP 100"); + cfs.forceBlockingFlush(ColumnFamilyStore.FlushReason.UNIT_TESTS); + + // SSTable 2: expiring cell for v at the SAME timestamp 100 — tombstone must win + execute("INSERT INTO %s (pk, ck, v) VALUES (0, 0, 'x') USING TIMESTAMP 100 AND TTL 3600"); + cfs.forceBlockingFlush(ColumnFamilyStore.FlushReason.UNIT_TESTS); + + cfs.forceMajorCompaction(); + + UntypedResultSet rs = execute("SELECT v FROM %s WHERE pk = 0 AND ck = 0"); + assertEquals(1, rs.size()); + assertFalse("Tombstone must beat the expiring cell at an identical timestamp", + rs.one().has("v")); + } +}