Fix stuck during recovery #357
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4172,32 +4172,32 @@ void SplitFlushRangeOp::ClearInfos() | |
| void SplitFlushRangeOp::Forward(TransactionExecution *txm) | ||
| { | ||
| if (txm->TxStatus() == TxnStatus::Recovering && | ||
| (Sharder::Instance().CandidateLeaderTerm(txm->TxCcNodeId()) != | ||
| txm->TxTerm() && | ||
| Sharder::Instance().LeaderTerm(txm->TxCcNodeId()) != txm->TxTerm())) | ||
| { | ||
| // Recovered term is invalid. Do not call ForceToFinish as it will | ||
| // cause infinite recursive call. Clean up tx state directly. | ||
| txm->bool_resp_->Finish(false); | ||
|
|
||
| ClearInfos(); | ||
|
|
||
| txm->state_stack_.pop_back(); | ||
| assert(txm->state_stack_.empty()); | ||
|
|
||
| assert(this == txm->split_flush_op_.get()); | ||
| LocalCcShards *shards = Sharder::Instance().GetLocalCcShards(); | ||
| std::unique_lock<std::mutex> lk(shards->split_flush_range_op_pool_mux_); | ||
| shards->split_flush_range_op_pool_.emplace_back( | ||
| std::move(txm->split_flush_op_)); | ||
| assert(txm->split_flush_op_ == nullptr); | ||
| return; | ||
| } | ||
|
Comment on lines
4174
to
4195
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# 1) Find all references to split flush op pooling + lifecycle.
echo "=== SPLIT FLUSH OP POOLING & LIFECYCLE ==="
rg -n --hidden --no-heading -S \
'split_flush_op_|split_flush_range_op_pool_|split_flush_range_op_pool_mux_|SplitFlushRangeOp::Forward|NewSplitFlush|SplitFlushRangeOp::Reset' . 2>/dev/null | head -100
echo -e "\n=== TX RESPONSE FINISH & TXM RECYCLING ==="
rg -n --hidden --no-heading -S \
'bool_resp_->Finish\(|Finish\(false\)|Finish\(true\)|TxnStatus::Recovering' . 2>/dev/null | head -100
echo -e "\n=== SPLIT FLUSH OP SUBOPS & ASYNC WORK ==="
rg -n --hidden --no-heading -S \
'data_sync_op_|ds_upsert_range_op_|ds_clean_old_range_op_|update_key_cache_op_|AsyncOp<' tx_service/src/tx_operation.cpp 2>/dev/null | head -60Repository: eloqdata/tx_service Length of output: 11128 🏁 Script executed: #!/bin/bash
# Get file size and read SplitFlushRangeOp class definition
wc -l tx_service/src/tx_operation.cpp
echo -e "\n=== SEARCHING FOR SPLITFLUSHRANGEOP CLASS ==="
rg -n 'class SplitFlushRangeOp' tx_service/src/tx_operation.cppRepository: eloqdata/tx_service Length of output: 146 🏁 Script executed: #!/bin/bash
# Read the relevant section including the questioned code
sed -n '4160,4220p' tx_service/src/tx_operation.cppRepository: eloqdata/tx_service Length of output: 2327 🏁 Script executed: #!/bin/bash
# Find ClearInfos implementation in SplitFlushRangeOp
rg -n -A 20 'SplitFlushRangeOp.*ClearInfos\(|void.*SplitFlushRangeOp.*ClearInfos' tx_service/src/tx_operation.cppRepository: eloqdata/tx_service Length of output: 870 🏁 Script executed: #!/bin/bash
# Find DataMigrationOp invalid-term cleanup pattern for comparison
rg -n -B 5 -A 15 'DataMigrationOp.*Recovering\|Recovered term is invalid.*DataMigration' tx_service/src/tx_operation.cppRepository: eloqdata/tx_service Length of output: 45 🏁 Script executed: #!/bin/bash
# Find DataMigrationOp invalid-term cleanup for comparison
rg -n -B 3 -A 12 'Recovered term is invalid' tx_service/src/tx_operation.cppRepository: eloqdata/tx_service Length of output: 1578 🏁 Script executed: #!/bin/bash
# Check if DataMigrationOp calls txm->Abort() after recycling
rg -n -B 5 -A 20 'class DataMigrationOp' tx_service/src/tx_operation.cpp | head -100Repository: eloqdata/tx_service Length of output: 45 🏁 Script executed: #!/bin/bash
# Find where DataMigrationOp recycling happens and whether Abort is called
rg -n -B 2 -A 10 'txm->Abort()' tx_service/src/tx_operation.cpp | head -80Repository: eloqdata/tx_service Length of output: 2050 🏁 Script executed: #!/bin/bash
# Check AsyncOp::Forward to see if it spawns persistent worker threads
sed -n '3729,3785p' tx_service/src/tx_operation.cppRepository: eloqdata/tx_service Length of output: 1673 Critical UAF risk: subop worker threads not joined before recycling split_flush_op_ The invalid-term cleanup path (line 4174–4193) calls
Additionally,
Contributor
There was a problem hiding this comment. Yes we can only directly recycle slit flush op if op_ == nullptr There was a problem hiding this comment. @liunyl Thanks — agreed. I’ll gate the early recycle path so we only directly recycle the SplitFlushRangeOp when no sub-op has started (op_ == nullptr). If a sub-op is already in-flight, we’ll fall through and let the normal state-machine paths unwind safely. [plan_code_changes]
If you’d like, I can open a quick PR amendment with this patch. ✏️ Learnings added
🧠 Learnings used
Collaborator
Author
There was a problem hiding this comment. When forward SplitFlushRangeOp, there will definitely be no running sub-ops. There was a problem hiding this comment.
|
||
| // else: allow forwarding recovered split flush tx on the candidate leader, | ||
| // otherwise, a deadlock might occur in the following scenario: If the | ||
| // replay data cannot be completed due to insufficient memory, the recovered | ||
| // split transaction will have to wait for the leader to complete log replay | ||
| // before it can continue forwarding. | ||
| if (op_ == nullptr) | ||
| { | ||
| // Initialize commit ts as the start time of tx. This value will | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only if the tx is processed on coordinator node group, should we allow cc requests to be processed as candidate leader.