fix: WaitableCc completion lifetime race

[thweetkomputer]

Context

Fixes a WaitableCc lifetime race observed as a CI SIGSEGV in FinishOne(): the last finisher could publish unfinished_cnt_ == 0, let a waiter return and destroy the stack request, then continue touching resume_fn_/mux_/waiting_.

This PR also carries the nested eloqstore submodule pointer forward to origin/main (12489d3), including the already-merged eloqstore build/docs updates.

Behavior before and after

Before, Wait()/IsFinished() treated unfinished_cnt_ == 0 as sufficient completion. After, waiters also require no thread to still be inside FinishOne(), so stack request owners cannot leave scope while the last finisher is still completing the resume handshake.

The RocksDB ParallelIterateTable error path also now records the error without calling FinishOne() from inside the task body, leaving Execute() to perform the single completion for that shard.

Implementation

WaitableCc now tracks active_finishers_ around FinishOne(). IsFinished() checks both unfinished_cnt_ and active_finishers_. The callback Wait() path uses bthread_usleep(100) while logical completion is published but the final finisher is still active, avoiding a second yield/resume cycle and avoiding bthread condition-variable waits between tx processor context and bthread waiters.

FinishOne() uses a CAS loop so duplicate/extra completion calls cannot underflow unfinished_cnt_. If FinishOne() is called after completion, Release builds log an ERROR and return without corrupting the counter; Debug builds also assert to expose the invariant violation.

SetErrorCode() was split from AbortCcRequest() for task bodies that need to latch an error but still return true to Execute().

Design decisions and alternatives

The new atomic is documented as a lifetime fence: unfinished_cnt_ can reach zero before FinishOne() is done touching this stack-allocated request. The counter operations use seq_cst because IsFinished() derives object lifetime from two atomics that must be observed consistently.

Test plan

• Unit/TCL tests
• Integration or manual validation
• Formatting/build checks
• Compatibility or performance validation, when relevant

Commands and results:

git -C data_substrate diff --check -- tx_service/include/cc/cc_req_misc.h tx_service/src/cc/cc_req_misc.cpp store_handler/rocksdb_handler.cpp
# passed

cmake --build data_substrate/bld --target CcRequestWait-Test -j 4
# passed

data_substrate/bld/tx_service/tests/CcRequestWait-Test
# passed: All tests passed (2008 assertions in 4 test cases)

Not run: clang-format-18 is not installed in this environment. No local RocksDB backend build covered store_handler/rocksdb_handler.cpp; the available local build is ELOQDSS_ELOQSTORE.

Risk assessment

Regression surface is shared WaitableCc completion semantics. The main risk is a waiter polling slightly longer until the final finisher leaves FinishOne(); this is intentional and bounded by the existing completion path. The seq_cst atomics add a small cost only on WaitableCc completion/wait paths, not per-record data operations.

The eloqstore pointer moves to already-merged upstream commits on eloqdata/eloqstore main.

Rollback plan

Revert this PR.

Reviewer guide

Start with tx_service/include/cc/cc_req_misc.h: IsFinished(), Wait(), FinishOne(), and the active_finishers_ comment. Then check store_handler/rocksdb_handler.cpp for the SetErrorCode() usage that avoids double-finishing a WaitableCc. Finally verify the eloqstore submodule pointer bump.

Follow-up work

A targeted fault-injection regression test could widen the old fetch_sub-to-resume window, but this PR keeps scope to the runtime fix.

[coderabbitai]

Walkthrough

WaitableCc now coordinates completion with an active_finishers_ fence, latches errors separately from completion, and updates waiter polling. RocksDB initialization failures use SetErrorCode instead of aborting the request.

Changes

WaitableCc completion handshake

Layer / File(s)	Summary
Completion and error-handling contract tx_service/include/cc/cc_req_misc.h	Adds the bthread include, introduces active_finishers_, updates completion checks and waiting, and separates error latching from request completion.
FinishOne completion race handling tx_service/include/cc/cc_req_misc.h, tx_service/src/cc/cc_req_misc.cpp	Updates FinishOne counter handling and makes Wait use IsFinished() for termination.
RocksDB error signaling store_handler/rocksdb_handler.cpp	Signals InitCcm failures with SetErrorCode rather than AbortCcRequest.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• eloqdata/tx_service#491: Also changes WaitableCc completion polling and error propagation.

Suggested reviewers: liunyl

Poem

I’m a rabbit guarding the wait,
With finishers lined at the gate.
Errors latch tight,
Completion feels right,
And RocksDB signals its state.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly names the main fix: a WaitableCc completion lifetime race.
Description check	✅ Passed	The description follows the template well and covers context, behavior, implementation, risks, rollback, review guidance, and tests.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch agent/waitablecc-lifetime

---

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.}

[coderabbitai]

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 `@tx_service/include/cc/cc_req_misc.h`:
- Around line 966-978: Replace the unconditional unfinished_cnt_.fetch_sub in
FinishOne with a CAS loop that decrements only when the current counter is
nonzero, preserving zero when extra concurrent FinishOne calls occur. Keep the
existing active_finishers_ accounting, assertion, early return, and unfinished
== 1 completion behavior aligned with the successfully reserved decrement.

🪄 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: 389abf29-de10-4bd6-8f90-723e346443f5

📥 Commits

Reviewing files that changed from the base of the PR and between a1162d3 and 1f1be88.

📒 Files selected for processing (3)

• store_handler/rocksdb_handler.cpp
• tx_service/include/cc/cc_req_misc.h
• tx_service/src/cc/cc_req_misc.cpp