chore: update submodule data_substrate for cc request bthread mutex deadlock fix - #491
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:
WalkthroughThis PR updates the ChangesEloqKV2RDB Tool Refactoring
Data Substrate Submodule Update
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
af65ebb to
440a66e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@data_substrate`:
- Line 1: Update the submodule bump metadata and add runtime evidence: correct
the tx_service submodule attribution to reference the actual commit(s) (e.g.,
39b801d58e67d6539d1ceab5aed69b294ccea781 and doc commit f62f0fa) and remove or
fix the erroneous “#491” PR tag since the deadlock fix is the commit
9ff97842aaa00138faca6dfff8041864ef6de5c6; include a brief changelog entry that
cites these exact commits (or PR/issue links) for traceability, and attach
concrete runtime/soak test artifacts proving the deadlock fix
(automated/regression tests or logs showing scenarios exercising db size growth,
standby subscribe/resubscribe, and leader failover) so reviewers can validate
the concurrency fix rather than relying on “Debug build passes.”
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
440a66e to
c49c402
Compare
c49c402 to
7eec40e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@data_substrate`:
- Line 1: The current verification relies only on a successful build, which
cannot validate a concurrency deadlock fix since bthread mutex deadlocks
manifest at runtime under concurrent load, not compile-time. Execute and
document three categories of runtime tests before merging: (1) DBSIZE broadcast
under concurrent load to stress the CcRequest broadcast path, (2) Standby
replication scenarios (subscribe, resubscribe, failover) to exercise
EscalateStandbyCcmCc, ClearCcNodeGroup, and related request paths, and (3) a
sustained soak/stress test exercising the converted CcRequest types
(ActiveTxMaxTsCc, WaitableCc, ClearTxCc, DbSizeCc, UploadBatchCc) to validate
the atomic completion count and bthread_usleep polling approach prevents worker
thread parking deadlock. Document the test methodology, duration, and results
(logs showing successful completion under load or automated regression test
output).
- Line 1: The PR description incorrectly references tx_service#491 when it
should reference tx_service#470, which contains the actual bthread mutex
deadlock fix. Update the PR description to replace the reference from `#491` to
`#470` and clarify that this PR restructures the flush pipeline through
coroutine-friendly lock management to eliminate the deadlock issue.
Additionally, a Debug build passing does not adequately validate a bthread mutex
deadlock fix since these are runtime concurrency issues; add evidence of runtime
verification such as soak test results or concurrent load testing output (e.g.,
high throughput scenarios with leader failover) to demonstrate the fix resolves
the deadlock under real-world conditions before merging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| @@ -1 +1 @@ | |||
| Subproject commit 887e352ac7a00b782e1ed444e75277613c56c75b | |||
| Subproject commit 302ef9a6df44dbd945a0038e3f7f4251e30f8fd3 | |||
There was a problem hiding this comment.
Provide runtime verification for the bthread mutex deadlock fix.
The PR description states "full Debug build with the bumped submodule passes via install/build.sh (exit 0)" as verification. A successful build cannot validate a concurrency deadlock fix—bthread mutex deadlocks manifest under concurrent load at runtime, not at compile time. The suggested runtime coverage (dbsize, standby subscribe/resubscribe, leader failover) must be executed and results documented before merging.
📋 Required runtime verification
Before merging, provide evidence of runtime testing covering the affected code paths:
-
DBSIZE broadcast: Execute Redis DBSIZE commands under concurrent load to stress the broadcast path that uses the updated CcRequest implementations.
-
Standby replication: Test standby subscribe, resubscribe, and failover scenarios to exercise the EscalateStandbyCcmCc, ClearCcNodeGroup, and related request paths.
-
Soak/stress test: Run a sustained concurrent workload that exercises the converted CcRequest types (ActiveTxMaxTsCc, WaitableCc, ClearTxCc, DbSizeCc, UploadBatchCc) to increase confidence that the atomic completion count + bthread_usleep polling approach prevents the worker thread parking deadlock.
Document test methodology, duration, and results (logs showing successful completion under load, or ideally automated regression test output).
🤖 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 `@data_substrate` at line 1, The current verification relies only on a
successful build, which cannot validate a concurrency deadlock fix since bthread
mutex deadlocks manifest at runtime under concurrent load, not compile-time.
Execute and document three categories of runtime tests before merging: (1)
DBSIZE broadcast under concurrent load to stress the CcRequest broadcast path,
(2) Standby replication scenarios (subscribe, resubscribe, failover) to exercise
EscalateStandbyCcmCc, ClearCcNodeGroup, and related request paths, and (3) a
sustained soak/stress test exercising the converted CcRequest types
(ActiveTxMaxTsCc, WaitableCc, ClearTxCc, DbSizeCc, UploadBatchCc) to validate
the atomic completion count and bthread_usleep polling approach prevents worker
thread parking deadlock. Document the test methodology, duration, and results
(logs showing successful completion under load or automated regression test
output).
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
NEW_DS_SHA="302ef9a6df44dbd945a0038e3f7f4251e30f8fd3"
DS_REPO="https://github.com/eloqdata/data_substrate.git"
work_dir="temp_ds_verify"
rm -rf "$work_dir"
mkdir -p "$work_dir"
cd "$work_dir"
echo "=== Cloning data_substrate at $NEW_DS_SHA ==="
git clone --no-checkout "$DS_REPO" ds_repo
cd ds_repo
git fetch origin "$NEW_DS_SHA"
git checkout "$NEW_DS_SHA"
echo -e "\n=== Commit message for data_substrate $NEW_DS_SHA ==="
git show -s --format='%s%n%b' "$NEW_DS_SHA" | head -n 40
echo -e "\n=== Check for tx_service submodule pointer ==="
if [ -f .gitmodules ]; then
cat .gitmodules
git submodule status || echo "No submodules initialized"
# Get tx_service commit if it exists
if git config -f .gitmodules --get-regexp 'submodule\..*\.path' | grep -q tx_service; then
echo -e "\n=== tx_service submodule commit ==="
git ls-tree HEAD tx_service || echo "tx_service not found in tree"
fi
else
echo "No .gitmodules file found"
fi
echo -e "\n=== Search commit message for '`#491`' or 'PR 491' references ==="
git log --oneline --max-count=50 "$NEW_DS_SHA" --grep -E "(`#491`|\bPR.?491\b)" || echo "No '`#491`' references found in last 50 commits"
cd ../..
rm -rf "$work_dir"Repository: eloqdata/eloqkv
Length of output: 350
🌐 Web query:
eloqdata tx_service PR 491 deadlock bthread mutex
💡 Result:
In the eloqdata/tx_service repository, the issue relating to deadlocks, bthread mutexes, and flush worker operations is primarily addressed in Pull Request #470, titled "refactor flush data worker" [1][2]. While there is no single PR explicitly numbered 491 associated with a deadlock fix in the provided technical logs, PR #470 was a significant architectural change aimed at resolving deadlocks and concurrency issues in the flush data worker pipeline [2]. Key technical details regarding this resolution include: 1. Deadlock Prevention via Coroutine Yielding: The refactor introduced coroutine-friendly yield/resume behavior [1][2]. Previously, the system used bthread::Mutex to synchronize flush work, which could lead to deadlocks when a worker thread waited for a condition while holding a lock that a resuming coroutine needed [1]. The update ensures that locks are unlocked before invoking a resume function if that function might acquire other critical locks [1]. 2. Architectural Changes: The flush workflow was restructured to use per-worker queues and buffers, replacing centralized locking mechanisms that were prone to contention and deadlocks [2]. The changes also included updating datastore handlers to accept optional yield/resume callbacks, preventing blocking operations during data synchronization [2]. 3. Broader Concurrency Management: The codebase includes other mechanisms for deadlock management, such as explicit deadlock detection logic integrated into transaction operations (e.g., in AcquireAllOp and blocking command processing) [3][4]. Earlier fixes in the repository also addressed similar issues, such as ensuring proper cleanup of lock state and avoiding unnecessary blocking when waiting for task completion [5][6][7].
Citations:
- 1: eloqdata/tx_service@985017e
- 2: refactor flush data worker tx_service#470
- 3: eloqdata/tx_service@3be56b8
- 4: eloqdata/tx_service@6e9720a
- 5: eloqdata/tx_service@60cd6f7
- 6: eloqdata/tx_service@41e111a
- 7: eloqdata/tx_service@da35c15
Correct PR description: Reference tx_service#470, not #491, for the bthread mutex deadlock fix.
The PR description claims "eloqdata/tx_service#491" addresses the deadlock issue, but web search confirms there is no PR #491 related to deadlock fixes in tx_service. The actual fix is in PR #470 ("refactor flush data worker"), which restructures the flush pipeline to eliminate bthread::Mutex deadlocks through coroutine-friendly lock management. Update the PR description to reference the correct issue.
Additionally, a Debug build passing cannot validate a bthread mutex deadlock fix—these are runtime concurrency issues requiring soak tests or concurrent load testing (e.g., high throughput with leader failover scenarios). Provide runtime verification evidence before merging.
🤖 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 `@data_substrate` at line 1, The PR description incorrectly references
tx_service#491 when it should reference tx_service#470, which contains the
actual bthread mutex deadlock fix. Update the PR description to replace the
reference from `#491` to `#470` and clarify that this PR restructures the flush
pipeline through coroutine-friendly lock management to eliminate the deadlock
issue. Additionally, a Debug build passing does not adequately validate a
bthread mutex deadlock fix since these are runtime concurrency issues; add
evidence of runtime verification such as soak test results or concurrent load
testing output (e.g., high throughput scenarios with leader failover) to
demonstrate the fix resolves the deadlock under real-world conditions before
merging.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/tools/eloqkv2rdb/eloqkv2rdb.cpp`:
- Around line 870-873: The multiplication operation in the `ParseSizeBytes`
function at the line where `std::stoull(number_part) * multiplier` is calculated
can silently overflow when processing very large input values, causing the
result to wrap to an invalid smaller value that still passes downstream checks.
Add an overflow guard before the multiplication by checking if `number_part >
UINT64_MAX / multiplier` would be true; if so, either cap the value at
UINT64_MAX, throw an exception, or return an error to prevent the silent
misconfiguration of size thresholds.
🪄 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: b70da788-4f67-4241-8033-03da3e9f516c
📒 Files selected for processing (1)
src/tools/eloqkv2rdb/eloqkv2rdb.cpp
| try | ||
| { | ||
| bytes = std::stoull(number_part) * multiplier; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "eloqkv2rdb.cpp" -type fRepository: eloqdata/eloqkv
Length of output: 97
🏁 Script executed:
wc -l src/tools/eloqkv2rdb/eloqkv2rdb.cppRepository: eloqdata/eloqkv
Length of output: 100
🏁 Script executed:
sed -n '820,880p' src/tools/eloqkv2rdb/eloqkv2rdb.cppRepository: eloqdata/eloqkv
Length of output: 1561
Add overflow check in ParseSizeBytes multiplication.
At line 872, multiplying std::stoull(number_part) * multiplier can silently overflow. If a very large numeric value (e.g., "18446744073709551615KB") is parsed, std::stoull succeeds and returns a uint64_t within range, but the subsequent multiplication overflows and wraps to a smaller positive value. This wrapped value passes the bytes > 0 check and is returned as valid, silently misconfiguring write-buffer flush thresholds.
Proposed fix
try
{
- bytes = std::stoull(number_part) * multiplier;
+ const uint64_t value = std::stoull(number_part);
+ if (value > (std::numeric_limits<uint64_t>::max() / multiplier))
+ {
+ return false;
+ }
+ bytes = value * multiplier;
}🤖 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 `@src/tools/eloqkv2rdb/eloqkv2rdb.cpp` around lines 870 - 873, The
multiplication operation in the `ParseSizeBytes` function at the line where
`std::stoull(number_part) * multiplier` is calculated can silently overflow when
processing very large input values, causing the result to wrap to an invalid
smaller value that still passes downstream checks. Add an overflow guard before
the multiplication by checking if `number_part > UINT64_MAX / multiplier` would
be true; if so, either cap the value at UINT64_MAX, throw an exception, or
return an error to prevent the silent misconfiguration of size thresholds.
…eadlock fix Pulls in eloqdata/tx_service#491, which fixes the same deadlock class as the CkptTsCc hang (#483) in the remaining CC requests (ActiveTxMaxTsCc, WaitableCc, ClearCcNodeGroup, EscalateStandbyCcmCc, ClearTxCc, DbSizeCc, UploadBatchCc): a bthread::Mutex shared between tx processors (the brpc worker main stack) and bthread waiters can park the worker forever when the butex wake is routed to a bthread bound to that worker. For eloqkv this also covers the Redis dbsize command path. Also picks up the upstream standby replication protocol documentation commit (f62f0fa). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
eloqkv_to_rdb is built for WITH_DATA_STORE=ROCKSDB as well as the RocksDB-Cloud backends, but ParseSizeBytes and ShardProgressPrinter were defined only inside `#if ROCKSDB_CLOUD_EXPORT` while main() referenced ParseSizeBytes and ShardProgressPrinter::kMaxScanThreads unconditionally. In a non-cloud build (the cloud data-store macros are undefined, so ROCKSDB_CLOUD_EXPORT is undefined) those symbols do not exist and the tool fails to compile -- a latent regression since #485. ParseSizeBytes parses --write_block_size into write_block_size_bytes, which the non-cloud writer (ParseWorker / Rocksdb2RDB) also reads, so it must be available in both builds: move it (and its <algorithm>/<cctype> includes) out of the cloud-only block and keep its validation in main() unconditional. ShardProgressPrinter is genuinely cloud-only, so guard only its kMaxScanThreads check with ROCKSDB_CLOUD_EXPORT. Verified with -fsyntax-only in both the non-cloud and cloud configs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
tx_service#491 (bthread-mutex CC-request deadlock fix) was squash-merged to tx_service main as 4621885. Re-point the submodule from the now-merged feature-branch commit to that main commit. The squash also brings main forward to include tx_service#507 (test-only multi-process cluster harness, under tests/). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
7819ab4 to
dce49a6
Compare
2 participants
Summary
Bumps
data_substrateto pull in eloqdata/tx_service#491, which fixes the same deadlock class as the CkptTsCc hang (tx_service#483) in the remaining CC requests.Bug recap: a
bthread::Mutex/ConditionVariableshared betweenCcRequest::Execute()(runs on the brpc worker main stack via the tx processor module) and a bthread waiter can hang the worker forever: the worker parks as a pthread waiter in the mutex's butex queue, and an unlock routes the wake to a bthread bound to that same blocked worker, which then can never be scheduled (_bound_rqis never stolen).Requests fixed:
ActiveTxMaxTsCc,WaitableCc,ClearCcNodeGroup,EscalateStandbyCcmCc,ClearTxCc,DbSizeCc/RemoteDbSizeCc,UploadBatchCc(+CcNodeService::UploadBatchhandler and the sk_generator upload path). All converted to atomic completion counts +bthread_usleepbackoff polling, same as the #483 fix.For eloqkv specifically this covers the Redis
dbsizecommand (broadcast to all cores with the handler bthread blocked on the request mutex) as well as the standby subscribe/failover RPC paths.Also picks up the upstream standby replication protocol documentation commit (f62f0fa).
Verification
install/build.shpasses with the bumped submodule (exit 0).dbsize, standby subscribe/resubscribe, leader failover.🤖 Generated with Claude Code
Summary by CodeRabbit
eloqkv2rdbutility used for converting data, including more consistent handling of size options and thread-count checks in cloud builds.