fix: retain scan reads until transaction finalization - #526
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe scanner lifecycle now retains eligible read ownership through transaction cleanup, removes the extra-lock release operation, updates local and remote continuation handling, and adds documentation, mock scanner support, and consistency tests for commit, abort, mismatch, and resume behavior. ChangesScan ownership and continuation cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant TransactionExecution
participant CcHandler
participant NonBlockingLock
Client->>TransactionExecution: Close transaction scan
TransactionExecution->>TransactionExecution: DrainScanner(retain_range_resume_tuple)
TransactionExecution->>NonBlockingLock: Retain eligible read intents
TransactionExecution->>CcHandler: Close scanner
CcHandler-->>Client: Complete scan close
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tx_service/src/tx_execution.cpp (1)
6251-6254: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winPass
kickout_data_opto the trace macro.Line 6253 references
kickout_data_all_op, which is not in scope and breaks trace-enabled builds.Proposed fix
TX_TRACE_ACTION_WITH_CONTEXT( this, - &kickout_data_all_op, + &kickout_data_op,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tx_service/src/tx_execution.cpp` around lines 6251 - 6254, Update the TX_TRACE_ACTION_WITH_CONTEXT invocation in the kickout-data execution flow to pass the in-scope kickout_data_op symbol instead of kickout_data_all_op. Preserve the existing trace context lambda and macro behavior.
🧹 Nitpick comments (1)
tx_service/tests/TxConsistency-Test.cpp (1)
250-293: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftAdd lifecycle tests for scanner-only cleanup entries.
These scenarios cover a returned primary tuple, but not the changed range-middle last-tuple, trailing-tuple, or error-drain paths. Add deterministic commit/abort cases verifying those release-only entries remain owned until finalization.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tx_service/tests/TxConsistency-Test.cpp` around lines 250 - 293, Add deterministic lifecycle scenarios covering scanner-only cleanup entries for range-middle last-tuple, trailing-tuple, and error-drain paths, in addition to the existing returned-primary cases. Mirror the Scenario 4 commit and Scenario 5 abort structure, verify each captured scan read remains owned before finalization, and assert ownership is released after Commit or Abort.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/04-transaction-execution.md`:
- Around line 40-45: Update the transaction read-set description to qualify that
only returned tuples requiring locking are retained; tuples returned with NoLock
are excluded unless separately pinned for scanner cleanup. Preserve the existing
explanation of scanner cleanup CCEs and commit/abort release behavior.
In `@tx_service/tests/TxConsistency-Test.cpp`:
- Around line 98-120: Clear the reused batch vector before each subsequent scan
request in the loop around ScanBatchTxRequest, ensuring ScanNextOperation
receives an empty scan_batch and preserving the existing target-search logic.
---
Outside diff comments:
In `@tx_service/src/tx_execution.cpp`:
- Around line 6251-6254: Update the TX_TRACE_ACTION_WITH_CONTEXT invocation in
the kickout-data execution flow to pass the in-scope kickout_data_op symbol
instead of kickout_data_all_op. Preserve the existing trace context lambda and
macro behavior.
---
Nitpick comments:
In `@tx_service/tests/TxConsistency-Test.cpp`:
- Around line 250-293: Add deterministic lifecycle scenarios covering
scanner-only cleanup entries for range-middle last-tuple, trailing-tuple, and
error-drain paths, in addition to the existing returned-primary cases. Mirror
the Scenario 4 commit and Scenario 5 abort structure, verify each captured scan
read remains owned before finalization, and assert ownership is released after
Commit or Abort.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9d27a79-aeb6-4979-9307-afc96f553ed6
📒 Files selected for processing (11)
docs/04-transaction-execution.mddocs/superpowers/plans/2026-07-14-defer-data-read-release.mddocs/superpowers/specs/2026-07-14-defer-data-read-release-design.mdtx_service/include/cc/cc_entry.htx_service/include/read_write_set.htx_service/include/tx_execution.htx_service/include/tx_operation.htx_service/src/tx_execution.cpptx_service/src/tx_operation.cpptx_service/tests/TxConsistency-Test.cpptx_service/tests/include/mock/mock_catalog_factory.h
💤 Files with no reviewable changes (3)
- tx_service/include/tx_operation.h
- tx_service/include/cc/cc_entry.h
- tx_service/src/tx_operation.cpp
|
Reviewed the remaining outside-diff and nitpick feedback:
|
| { | ||
| std::vector<const ScanTuple *> last_tuples; | ||
| scanner->MemoryShardCacheLastTuples(&last_tuples); | ||
| for (const ScanTuple *tuple : last_tuples) |
There was a problem hiding this comment.
does hash partition scan also put a read intent lock on the last tuple for resume? If so we need to handle that too. The old code commented the resume tuple handling logic for some reason but I'm not sure if that's right
There was a problem hiding this comment.
Hash scans do acquire read intents, but they are not last-tuple pins that survive a completed ScanNextBatchCc. The current hash path uses intents only across an internal block/self-reenqueue and releases the prior_cce/end_it intents on re-entry. If the cache is full, it finishes without acquiring a new resume intent; the next transaction-level batch resumes from BucketScanProgress::pause_key_, not a CCE address.
Range scans are different: ScanSliceCc explicitly acquires a read intent on last_cce for SlicePosition::Middle, stores its lock address in the last tuple, and the next request passes that address as prior_cce_lock. Therefore only the range path needs the special ScanClose retention. The old hash block was commented out by the hash-partition scan refactor (#149), when cross-batch resume changed to the pause key. I will qualify the code/doc wording; no hash ScanClose branch is needed.
There was a problem hiding this comment.
There is a separate issue that needs to be resolved: evaluating scan next of hash-partitioned cc map may leave orphaned locks on the cc map side.
There was a problem hiding this comment.
You were right; my previous reply conflated client-visible batch resume with resume inside one ScanNextBatchCc.
A hash scan does not retain the last returned tuple as a cross-batch resume anchor—the next client batch starts from pause_key_. However, when one memory pass reaches the 128-entry budget, the same CC request self-reenqueues and holds counted ReadIntents on the next unprocessed CCE and any finite-end CCE in blocking_info_.
That internal pin could be orphaned. The deterministic interleaving is: stale memory_scan_is_finished_ == true with KV still unfinished; the memory pass yields and pins; the final KV callback runs ahead of the queued continuation and marks the shard drained; the continuation then returns through ShardIsDrained() -> SetFinish() before the old normal-release block. This is a serialized same-shard queue-order race, not a C++ data race.
The updated branch fixes the lifecycle rather than adding a hash ScanClose last-tuple branch:
SetFinish/SetErrorconsume pending continuation/end ownership exactly once for local and remote scans.- Normal and terminal paths use exact
DecrReadIntent, so another intent count owned by the transaction is preserved; blocked paths release the actually granted lock type. Resetclears stale memory-finished/error state unless memory and all KV buckets are truly complete.- Each saved wrapper address carries its generation. A transient term error can still release a matching live pin, while teardown/reuse cannot consume ownership belonging to the new object at the same address.
- Scan-error scanner ownership is retained in the transaction read set for final commit/abort cleanup.
TDD: the original reproducer was RED at 203/205 assertions; an error-code-only term fix was RED at 402/403; disabling the normal-resume generation guard was RED at 406/408. The final focused test is GREEN at 408/408, repeated 10/10, and the full suite passes 51/51. The branch and PR description are now updated at b2d794e.
There was a problem hiding this comment.
Corrected in f48bff6.
The orphan risk is real, but my previous diagnosis and the generation-based fix were unnecessarily broad. In particular, memory_scan_is_finished_ is cross-batch scan progress, not stale request state that should be reset. The scan may enter the CC map again while KV backfill is still incomplete, and a continuation may then be pending when the shard becomes drained. This does not require recreating the table, and it is not limited to inserting 128 new keys: the 128-entry visit budget is one way to self-reenqueue, while lock/future blocking can also create a pending continuation.
The actual ownership bug is the order inside Execute: ShardIsDrained()/SetFinish() ran before the existing code that consumes blocking_info_. Therefore a resumed request could finish without decrementing its continuation and finite-end ReadIntent references.
The final fix is intentionally small:
- keep the term check first, so a term mismatch never dereferences old lock addresses;
- when a continuation address exists, run the existing resume cleanup before the terminal checks;
- use
DecrReadIntentto consume exactly this request's reference, preserving any semantic intent held by the transaction; - make the same ordering change in the local and remote paths;
- leave
memory_scan_is_finished_unchanged and remove all generation/reset machinery from the final diff.
TDD on the original ordering was RED: the terminal path left the intent count at 2 instead of 1 (117/118 assertions). The fixed path is GREEN at 121/121, and the full suite passes 51/51.
There was a problem hiding this comment.
Corrected again in d8b1493 after tracing Merge, SetFinish, and both local/remote hash-scan paths end to end.
My previous callback/continuation race diagnosis was wrong. A live NoBlocking continuation is created only when the current memory pass is unfinished, while memory_scan_is_finished_ becomes true only when that pass reaches the CC-map end and immediately calls SetFinish. Therefore the state used by the earlier reproducer — finished memory plus a live continuation pin — is not produced by the real execution path.
The actual bug is simpler: on a later client batch, if memory was already finished but any KV bucket was still unfinished, ShardIsDrained() returned false and Execute entered the CC map again. That redundant memory scan could materialize the same CCE and add another ReadIntent.
The minimal fix is now:
- start unfinished KV fetches as before;
- if this is a first-entry path and the core memory source is already finished, call
SetFinishbefore constructing the CC-map iterator; - make the same change in local and remote paths;
- retain existing transaction-owned semantic intents until commit/abort;
- keep exact
DecrReadIntentcleanup for genuine in-batch continuations; - add no generation tracking and do not reset completed memory progress.
This is safe because HashParitionCcScanner::Merge already resets memory_scan_is_finished_ to false whenever trimming memory tuples leaves work behind the new pause key. If the flag remains true, there are no unconsumed memory tuples for this snapshot-like scan.
TDD: the real TestNode regression was RED on the prior code because the finished memory cache contained 1 tuple instead of 0. It is GREEN after the fix: memory cache 0, existing semantic intent count remains 1, and commit releases it to 0. The focused test passes 121/121 assertions and the full CTest suite passes 51/51.
d8b1493 to
a78965a
Compare
3 participants
Context
Fixes #508.
This PR fixes two scan-ownership defects:
ReadIntentthat was not part of the intended batch ownership.The second defect is not the previously proposed KV-callback/continuation race. A live in-batch continuation exists only while the memory pass is unfinished;
memory_scan_is_finished_becomes true only after the pass reaches the CC-map end.Behavior before and after
Before:
ReadIntentand duplicate memory-source work.After:
ReadIntent;Implementation
DrainScanner, using release-only entries for implementation pins that must not participate in OCC version validation.Executepaths:SetFinishbefore constructing a CC-map iterator;DecrReadIntentcleanup for a real in-batch continuation.FetchBucketDatafills a separate KV scan cache rather than the CC map.docs/superpowers/working notes ignored and outside the PR.Design decisions and alternatives
HashParitionCcScanner::Mergealready provides the required frontier invariant. If it trims memory tuples behind the new pause key, it resetsmemory_scan_is_finished_to false. If the flag remains true, the snapshot-like memory source has no unconsumed tuples for the next batch, so rescanning it is unnecessary.This reuses the existing progress flag and
SetFinishfetch-wait/merge path. It does not add generation tracking and does not reset finished memory progress on every request reset. Table read-lock lifetime already prevents concurrent drop/recreate while the scan accesses the table.The local and remote paths use their existing equivalent flags:
BucketScanProgress::memory_scan_is_finished_andRemoteScanNextBatch::memory_is_drained_.Test plan
Commands and results:
TDD evidence for the finished-memory regression:
Not run locally: a dedicated remote/failover runtime fixture, a real asynchronous-KV callback/re-drive fixture, recovery/WAL compatibility tests, and performance benchmarks. Parent EloqKV integration is triggered separately by advancing its submodule pointer.
Risk assessment
Mergepreserves the finished flag; this is the intendedScanBatchbehavior.Rollback plan
Revert this PR. No migration or configuration rollback is required.
Reviewer guide
tx_service/include/cc/template_cc_map.h: local and remote finished-memory skip after KV fetch launch and before CC-map iteration.tx_service/include/cc/ccm_scanner.h: existingMergefrontier reset that makes the skip safe.tx_service/tests/TxConsistency-Test.cpp: real CC-map regression proving empty memory cache and unchanged intent ownership.tx_service/src/tx_execution.cpp: transaction-final scanner ownership transfer.docs/03-concurrency-control.mdanddocs/04-transaction-execution.md: scan-source and ownership invariants.Follow-up work
Add a dedicated async store-handler fixture if callback/re-drive behavior needs direct unit coverage.