Skip to content

Fix index-based Get/Scan to filter out results whose index key no longer matches after lazy recovery rollback#3488

Merged
brfrn169 merged 7 commits into
masterfrom
fix-index-key-filtering-after-lazy-recovery-rollback
Apr 14, 2026
Merged

Fix index-based Get/Scan to filter out results whose index key no longer matches after lazy recovery rollback#3488
brfrn169 merged 7 commits into
masterfrom
fix-index-key-filtering-after-lazy-recovery-rollback

Conversation

@brfrn169

Copy link
Copy Markdown
Collaborator

Description

When a PREPARED record is found via the normal secondary index during an index-based Get/Scan, and lazy recovery rolls back the transaction, the index column value reverts to its before-image value. If this reverted value no longer matches the queried index key, the result should be filtered out. Previously, such results were returned incorrectly.

For example:

  1. Transaction T1 updates col_idx from "B" to "A" (PREPARED state)
  2. Transaction T2 queries col_idx = "A" via index-based Get/Scan
  3. The PREPARED record is found via the normal index (col_idx = "A")
  4. Lazy recovery rolls back T1, reverting col_idx to "B"
  5. Bug: The record with col_idx = "B" was returned for the col_idx = "A" query

This PR addresses this issue.

Related issues and/or PRs

N/A

Changes made

  • Added index key validation after lazy recovery rollback in CrudHandler.read() (Get) and CrudHandler.processScanResult() (Scan). When recoveryResult.rolledBack is true and the operation uses a secondary index, the recovered result is filtered using resultMatchesIndexKey().
  • Added resultMatchesIndexKey() private method in CrudHandler that compares the result's index column value against the queried index key.
  • Added unit tests in CrudHandlerTest covering rollback with matching/non-matching index keys for both Get and Scan operations.
  • Added integration tests in ConsensusCommitSpecificIntegrationTestBase for Get, Scan, and getScanner that verify rolled-back PREPARED records are correctly filtered out when the index key matches the after-image.

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes.
  • I have considered whether similar issues could occur in other products, components, or modules if this PR is for bug fixes.
  • Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
  • Tests (unit, integration, etc.) have been added for the changes.
  • My changes generate no new warnings.
  • Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

  • The filtering is only applied when rolledBack is true because roll-forward (commit) preserves the after-image value, which already matches the queried index key by definition — the record was found via the normal index because its after-image matched the query.

Release notes

Fixed a bug where index-based Get and Scan operations could return incorrect results after lazy recovery rolled back a PREPARED record whose after-image index value matched the query but whose before-image (restored) value did not.

@brfrn169 brfrn169 self-assigned this Apr 10, 2026
Copilot AI review requested due to automatic review settings April 10, 2026 00:47

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces logic to filter out records that no longer match a secondary index key after a lazy recovery (rollback) in the CrudHandler. This ensures consistency when querying via secondary indexes. Feedback suggests deduplicating the filtering logic between the read and processScanResult methods and adding a safety check when accessing partition key columns in the resultMatchesIndexKey helper to prevent potential IndexOutOfBoundsException.

Copilot AI 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.

Pull request overview

Fixes an inconsistency in index-based reads under Consensus Commit where a record found via the (after-image) secondary index could be lazily recovered (rolled back) to a different index value but still be returned to the caller.

Changes:

  • Added post-recovery index-key validation for index-based Get and Scan paths in CrudHandler.
  • Introduced resultMatchesIndexKey() to compare the recovered row’s indexed column value against the requested index key.
  • Added unit + integration coverage for rolled-back PREPARED records whose restored (before-image) index value no longer matches the queried index key.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java Filters recovered results for index-based reads when the returned view is effectively “rolled back” to before-image values.
core/src/test/java/com/scalar/db/transaction/consensuscommit/CrudHandlerTest.java Adds unit tests validating filtering behavior for index-based Get/Scan after rollback.
integration-test/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitSpecificIntegrationTestBase.java Adds integration tests for Get/Scan/getScanner ensuring rolled-back rows are not returned when the index key no longer matches.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@brfrn169 brfrn169 marked this pull request as draft April 11, 2026 01:20
@brfrn169 brfrn169 requested a review from Copilot April 12, 2026 06:12
@brfrn169

Copy link
Copy Markdown
Collaborator Author

/gemini review

Copilot AI 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.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request implements filtering for secondary index results after a lazy recovery (rollback) in the consensus commit transaction protocol. It ensures that if a record's index column value reverts to an original value that no longer matches the query criteria after a rollback, the record is filtered out of the result set. Additionally, the logic prevents caching these filtered-out results as empty in the transaction snapshot to avoid incorrect subsequent reads. I have no feedback to provide as no review comments were submitted.

Copilot AI 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.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@brfrn169 brfrn169 marked this pull request as ready for review April 12, 2026 07:11
@brfrn169 brfrn169 requested a review from Copilot April 12, 2026 07:11

Copilot AI 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.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@brfrn169 brfrn169 force-pushed the fix-index-key-filtering-after-lazy-recovery-rollback branch from 9309986 to 79c6d46 Compare April 12, 2026 07:19
@brfrn169 brfrn169 requested review from a team, Torch3333, feeblefakie and komamitsu and removed request for a team April 12, 2026 08:00
@brfrn169 brfrn169 requested a review from KodaiD April 12, 2026 08:00
Comment thread core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java Outdated
@brfrn169 brfrn169 requested a review from feeblefakie April 13, 2026 05:48
@brfrn169 brfrn169 requested a review from komamitsu April 13, 2026 06:48

@komamitsu komamitsu 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.

LGTM! 👍

@feeblefakie feeblefakie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thank you!

@KodaiD KodaiD left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thank you!

@Torch3333 Torch3333 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.

LGTM, thank you!

@brfrn169 brfrn169 merged commit 24dcb31 into master Apr 14, 2026
13 checks passed
@brfrn169 brfrn169 deleted the fix-index-key-filtering-after-lazy-recovery-rollback branch April 14, 2026 05:52
feeblefakie pushed a commit that referenced this pull request Apr 14, 2026
feeblefakie pushed a commit that referenced this pull request Apr 14, 2026
feeblefakie pushed a commit that referenced this pull request Apr 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants