Skip to content

CASSANDRA-21356: Fix same-timestamp tombstone/expiring-cell tie-break… - #4992

Draft
arjunashok wants to merge 1 commit into
apache:trunkfrom
arjunashok:CASSANDRA-21356
Draft

CASSANDRA-21356: Fix same-timestamp tombstone/expiring-cell tie-break…#4992
arjunashok wants to merge 1 commit into
apache:trunkfrom
arjunashok:CASSANDRA-21356

Conversation

@arjunashok

Copy link
Copy Markdown

… in cursor compaction

CASSANDRA-21356: Fix same-timestamp tombstone/expiring-cell tie-break in cursor compaction

What's the issue

ReusableLivenessInfo.isExpiring() checked localExpirationTime != NO_EXPIRATION_TIME instead of ttl != NO_TTL. Tombstone cells also set localExpirationTime (stores the deletion timestamp there), so the check returned true for tombstones as well as genuinely expiring cells.

This broke CursorCompactor.resolveRegular()'s logic for picking a winner when two cells have the exact same timestamp. That logic relies on !isExpiring() to tell tombstones apart from expiring cells.
Since both now looked "expiring," it fell back to comparing localExpirationTime directly. Now, the expiring cell's value there is a future timestamp, while the tombstone's is a past one, so the expiring cell always won.

Net effect: an explicitly deleted column could be resurrected by cursor compaction if a same-timestamp write landed in another sstable.

Repro

INSERT INTO t (pk, ck, v) VALUES (0, 0, null) USING TIMESTAMP 100;  -- sstable 1
INSERT INTO t (pk, ck, v) VALUES (0, 0, 'x') USING TIMESTAMP 100 AND TTL 3600;  -- sstable 2
-- major compact -> SELECT v FROM t WHERE pk=0 AND ck=0 incorrectly returns 'x'

Fix

  • ReusableLivenessInfo.isExpiring() now checks ttl != NO_TTL, matching AbstractCell.isExpiring().
  • CursorCompactor.resolveRegular() checks ttl() == NO_TTL directly instead of relying on isExpiring(), so it's correct even if that method regresses again.

Tests

  • ReusableLivenessInfoTest — direct unit coverage of the root-cause check.
  • CursorCompactionEquivalenceTest#testSameTimestampTieBreak — end-to-end repro above.

@arjunashok
arjunashok marked this pull request as ready for review August 3, 2026 22:49
@arjunashok
arjunashok marked this pull request as draft August 3, 2026 23:07

@nitsanw nitsanw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider comments

Comment on lines -831 to +832
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code here is a mirror of Cells.resolveRegular, I think it should be kept as is

Comment on lines +49 to +50
// Check for TTL (not localExpirationTime as it will incorrectly return true for tombstones)
// Matches AbstractCell.isExpiring().

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix, I would add a link to AbstractCell.isExpiring() to the method javadoc, but remove the rest of the comment as it only explains the commit and not the code.

* 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not add this test to one of the simple compaction test suites? smaller diff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants