Fix slice index out of range

[yi-xmu]

Here are some reminders before you submit the pull request

• Add tests for the change
• Document changes
• Reference the link of issue using fixes eloqdb/tx_service#issue_id
• Reference the link of RFC if exists
• Pass ./mtr --suite=mono_main,mono_multi,mono_basic

Summary by CodeRabbit

• Bug Fixes
  • Added runtime validation to catch out-of-bounds slice indexing during scans, reducing rare runtime errors.
  • Adjusted end-of-batch handling to more reliably decide when persisted keys are exported, improving batch processing correctness.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Added a runtime assertion to validate slice index bounds in RangePartitionDataSyncScanCc::IsSlicePinned and changed TemplateCcMap to treat batch-end as unpinned (setting slice_pinned false) instead of querying IsSlicePinned.

Changes

Cohort / File(s)	Summary
Slice Index Validation tx_service/include/cc/cc_request.h	Added a runtime assertion in RangePartitionDataSyncScanCc::IsSlicePinned to ensure curr_slice_index_[core_id] is within slices_to_scan_ when export_base_table_item_ is false.
Batch-End Slice Pinning Logic tx_service/include/cc/template_cc_map.h	Updated logic so slice_pinned is set to false when TheBatchEnd(shard_->core_id_) is true; otherwise retain IsSlicePinned(shard_->core_id_). Affects end-of-batch persisted-key export decisions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• Update datasyncscan execution #324 — touches IsSlicePinned, TheBatchEnd, and curr_slice_index_ semantics (closely related changes).
• Fix datasyncscan paused key with debug log #332 — modifies slice-bound handling and end-of-slice pause logic in the same components.
• Update RangePartitionDataSyncScanCc execution for performing pinslice by only one core #298 — refactors slice-pinning coordination and interacts with the same pin/unpin paths.

Suggested reviewers

• liunyl
• MrGuin

Poem

🐰 A nibble, a hop, I check the slice,
I guard the index, keep bounds precise,
At batch's end I let it rest,
No pinned surprise — that's my quest! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description only contains an unchecked template checklist without any substantive details about the changes, implementation, or issue being fixed.	Provide a detailed description explaining the issue being fixed, the root cause, how the fix works, and complete the template checklist items with actual information or evidence.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fix slice index out of range' is directly related to the main change: adding a runtime assertion to validate curr_slice_index_ against slices_to_scan_ size to prevent out-of-range access.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix_slice_index_outofrange

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

tx_service/include/cc/template_cc_map.h (1)

5613-5617: Guarding IsSlicePinned at batch end correctly avoids invalid slice index access

Short‑circuiting slice_pinned when req.TheBatchEnd(shard_->core_id_) is true ensures IsSlicePinned is never called once there is no current slice, which aligns with the new bounds assertion in RangePartitionDataSyncScanCc::IsSlicePinned and prevents the original out‑of‑range bug. Since the scan loop is gated on key_it != slice_end_it, treating batch‑end as “unpinned” does not change behavior, only removes an invalid state.

As a small readability/duplication tweak, you could collapse the ternary to a more idiomatic form and/or factor a tiny helper, e.g.:

bool slice_pinned =
    !req.TheBatchEnd(shard_->core_id_) &&
    req.IsSlicePinned(shard_->core_id_);

and reuse that at both call sites.

Also applies to: 5725-5729

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 69341d3 and 4d23ba4.

📒 Files selected for processing (1)

• tx_service/include/cc/template_cc_map.h

🧰 Additional context used

🧠 Learnings (6)

📓 Common learnings

Learnt from: liunyl
Repo: eloqdata/tx_service PR: 149
File: include/cc/cc_request.h:1876-1927
Timestamp: 2025-10-20T04:30:07.884Z
Learning: ScanNextBatchCc in include/cc/cc_request.h is used only for hash-partition scans; range-partition scans are handled by ScanSliceCc.

Learnt from: lokax
Repo: eloqdata/tx_service PR: 254
File: tx_service/src/cc/local_cc_shards.cpp:2949-3188
Timestamp: 2025-12-02T10:43:27.431Z
Learning: In tx_service/src/cc/local_cc_shards.cpp, whenever TryPinNodeGroupData is used, only call Sharder::Instance().UnpinNodeGroupData(node_group) if the recorded term is >= 0 (i.e., pin succeeded). Example: LocalCcShards::PostProcessFlushTaskEntries guards the unpin with `if (term >= 0)`.

Learnt from: lokax
Repo: eloqdata/tx_service PR: 149
File: src/remote/cc_stream_receiver.cpp:1066-1075
Timestamp: 2025-10-21T06:46:53.700Z
Learning: In src/remote/cc_stream_receiver.cpp, for ScanNextRequest handling, BucketIds() on RemoteScanNextBatch should never be empty—this is an expected invariant of the scan protocol.

📚 Learning: 2025-10-20T04:30:07.884Z

Learnt from: liunyl
Repo: eloqdata/tx_service PR: 149
File: include/cc/cc_request.h:1876-1927
Timestamp: 2025-10-20T04:30:07.884Z
Learning: ScanNextBatchCc in include/cc/cc_request.h is used only for hash-partition scans; range-partition scans are handled by ScanSliceCc.

Applied to files:

• tx_service/include/cc/template_cc_map.h

📚 Learning: 2025-12-02T10:43:27.431Z

Learnt from: lokax
Repo: eloqdata/tx_service PR: 254
File: tx_service/src/cc/local_cc_shards.cpp:2949-3188
Timestamp: 2025-12-02T10:43:27.431Z
Learning: In tx_service/src/cc/local_cc_shards.cpp, whenever TryPinNodeGroupData is used, only call Sharder::Instance().UnpinNodeGroupData(node_group) if the recorded term is >= 0 (i.e., pin succeeded). Example: LocalCcShards::PostProcessFlushTaskEntries guards the unpin with `if (term >= 0)`.

Applied to files:

• tx_service/include/cc/template_cc_map.h

📚 Learning: 2025-10-09T03:56:58.811Z

Learnt from: thweetkomputer
Repo: eloqdata/tx_service PR: 150
File: include/cc/local_cc_shards.h:626-631
Timestamp: 2025-10-09T03:56:58.811Z
Learning: For the LocalCcShards class in include/cc/local_cc_shards.h: Writer locks (unique_lock) should continue using the original meta_data_mux_ (std::shared_mutex) rather than fast_meta_data_mux_ (FastMetaDataMutex) at this stage. Only reader locks may use the FastMetaDataMutex wrapper.

Applied to files:

• tx_service/include/cc/template_cc_map.h

📚 Learning: 2025-11-11T07:10:40.346Z

Learnt from: lzxddz
Repo: eloqdata/tx_service PR: 199
File: include/cc/local_cc_shards.h:233-234
Timestamp: 2025-11-11T07:10:40.346Z
Learning: In the LocalCcShards class in include/cc/local_cc_shards.h, the EnqueueCcRequest methods use `shard_code & 0x3FF` followed by `% cc_shards_.size()` to distribute work across processor cores for load balancing. This is intentional and separate from partition ID calculation. The 0x3FF mask creates a consistent distribution range (0-1023) before modulo by actual core count.

Applied to files:

• tx_service/include/cc/template_cc_map.h

📚 Learning: 2025-10-21T06:46:53.700Z

Learnt from: lokax
Repo: eloqdata/tx_service PR: 149
File: src/remote/cc_stream_receiver.cpp:1066-1075
Timestamp: 2025-10-21T06:46:53.700Z
Learning: In src/remote/cc_stream_receiver.cpp, for ScanNextRequest handling, BucketIds() on RemoteScanNextBatch should never be empty—this is an expected invariant of the scan protocol.

Applied to files:

• tx_service/include/cc/template_cc_map.h