🔍 Deep Dive: fix: Fix Workers::stop() race between m_allPaused and m_runningTaskCount by pratikmankawde · Pull Request #6574 · XRPLF/rippled · GitHub

pratikmankawde · 2026-03-18T11:58:47Z

High Level Overview of Change

Fix an intermittent assertion failure in Workers::stop() caused by a race condition between m_allPaused (mutex-protected bool) and m_runningTaskCount (atomic int). These two states use independent synchronization, so m_allPaused can be true while a task is still finishing.

Triggered by this CI failure on debian-bookworm-gcc-12-amd64-debug.

Context of Change

Workers::stop() previously asserted numberOfCurrentlyRunningTasks() == 0 after confirming m_allPaused was true, but outside any lock. Since m_allPaused and m_runningTaskCount are synchronized independently (mutex vs. atomic), there is a window where m_allPaused is true while a worker still has m_runningTaskCount > 0.

This fix:

Replaces the assertion with a compound condition variable predicate: m_allPaused && numberOfCurrentlyRunningTasks() == 0
Notifies m_cv when m_runningTaskCount drops to zero (locking m_mut first to prevent lost wakeups)

This is an alternative approach to PR #6302, which used memory fences. The compound predicate is more robust — it handles the logical race (not just memory ordering) and works correctly on all architectures.

Type of Change

Bug fix (non-breaking change which fixes an issue)

API Impact

None — internal threading change only.

Before / After

Before: stop() waits for m_allPaused, then asserts m_runningTaskCount == 0. Assertion can fail if the two states are transiently inconsistent.

After: stop() waits for both conditions in a single predicate. No assertion can fail; stop() simply waits until both are satisfied.

Test Plan

Existing xrpl.core.Workers test suite covers this. The fix eliminates the intermittent crash rather than adding new test cases — the race is timing-dependent and not reliably reproducible in a unit test.

Jira: RIPD-5329
Related: #6302 (RIPD-4759), #5774

Replace the post-wait assertion in Workers::stop() with a compound condition variable predicate that waits for both m_allPaused AND m_runningTaskCount == 0. The old code asserted zero running tasks after confirming m_allPaused, but these two states use independent synchronization (mutex vs atomic), allowing a window where m_allPaused is true while a task is still finishing. Additionally, notify m_cv when m_runningTaskCount drops to zero, locking m_mut first to prevent lost wakeups against the predicate evaluation in stop()'s cv.wait(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

codecov · 2026-03-18T12:15:38Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.8%. Comparing base (2a325e7) to head (385fc76).
⚠️ Report is 51 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #6574     +/-   ##
=========================================
- Coverage     79.8%   79.8%   -0.0%     
=========================================
  Files          878     878             
  Lines        68028   68029      +1     
  Branches      7556    7556             
=========================================
- Hits         54308   54302      -6     
- Misses       13720   13727      +7

Files with missing lines	Coverage Δ
src/libxrpl/core/detail/Workers.cpp	`98.9% <100.0%> (+<0.1%)`	⬆️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Keep the explicit unlock before returning from stop() to release the mutex as soon as it is no longer needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

a1q123456 · 2026-03-20T11:25:01Z

Existing xrpl.core.Workers test suite covers this

I don't think we can stably reproduce this issue with xrpl.core.Workers. I believe we should soak this branch in antithesis

bthomee · 2026-03-20T18:26:07Z

Existing xrpl.core.Workers test suite covers this

I don't think we can stably reproduce this issue with xrpl.core.Workers. I believe we should soak this branch in antithesis

I just kicked off a run in Antithesis, specifically for this branch.

pratikmankawde · 2026-03-24T13:51:49Z

Existing xrpl.core.Workers test suite covers this

I don't think we can stably reproduce this issue with xrpl.core.Workers. I believe we should soak this branch in antithesis

I just kicked off a run in Antithesis, specifically for this branch.
@bthomee

Can you please share the results of the Antithesis run?

bthomee · 2026-03-24T14:22:03Z

Existing xrpl.core.Workers test suite covers this

I don't think we can stably reproduce this issue with xrpl.core.Workers. I believe we should soak this branch in antithesis

I just kicked off a run in Antithesis, specifically for this branch.
@bthomee

Can you please share the results of the Antithesis run?

No new findings 🙂

pratikmankawde · 2026-03-24T14:27:30Z

Existing xrpl.core.Workers test suite covers this

I don't think we can stably reproduce this issue with xrpl.core.Workers. I believe we should soak this branch in antithesis

I just kicked off a run in Antithesis, specifically for this branch.
@bthomee

Can you please share the results of the Antithesis run?

No new findings 🙂

Is that a good thing or not so good? Still crashing or nothing improved? All normal, so nothing to report?

bthomee · 2026-03-24T15:51:33Z

Existing xrpl.core.Workers test suite covers this

I don't think we can stably reproduce this issue with xrpl.core.Workers. I believe we should soak this branch in antithesis

I just kicked off a run in Antithesis, specifically for this branch.
@bthomee

Can you please share the results of the Antithesis run?

No new findings 🙂

Is that a good thing or not so good? Still crashing or nothing improved? All normal, so nothing to report?

Since the shutdown crash occurs randomly, not every Antithesis run will encounter it. The run I kicked off didn't see it, but that's no proof yet.

The easiest way forward is to merge this PR once approved, so the full day fuzzing runs will start using it. If we see the problem reoccur, then we revert this PR and go back to the drawing board.

However, if you are uncertain, then we can kick off several longer Antithesis runs to gain more confidence, before we do the above.

a1q123456 · 2026-03-24T15:53:34Z

We can just modify ripple-antithesis to use this branch for daily antithesis runs

pratikmankawde · 2026-03-24T18:55:11Z

Existing xrpl.core.Workers test suite covers this

I don't think we can stably reproduce this issue with xrpl.core.Workers. I believe we should soak this branch in antithesis

I just kicked off a run in Antithesis, specifically for this branch.
@bthomee

Can you please share the results of the Antithesis run?

No new findings 🙂

Is that a good thing or not so good? Still crashing or nothing improved? All normal, so nothing to report?

Since the shutdown crash occurs randomly, not every Antithesis run will encounter it. The run I kicked off didn't see it, but that's no proof yet.

The easiest way forward is to merge this PR once approved, so the full day fuzzing runs will start using it. If we see the problem reoccur, then we revert this PR and go back to the drawing board.

However, if you are uncertain, then we can kick off several longer Antithesis runs to gain more confidence, before we do the above.

Yeah, I am fine with merging and testing on Antithesis full day runs. Since the issue is around the shutdown step, if we run same tests, but shutdown every time, we can get more surety. This then doesn't need to be done after merging. Is there a way we can automate, say 100 test runs, preferably on CI(or on aws), on this branch(using same binary, to save build times)?

a1q123456 · 2026-03-25T15:42:45Z

anyway, just fyi, if you don't see No unexpected crashes → io svc #0 or AlwaysOrUnreachable assertions → xrpl::JobQueue::stop : no coros suspended after a couple of antithesis runs, then we can confidently say the issue is fixed.

pratikmankawde · 2026-03-26T13:54:42Z

@bthomee So, in that case we can go ahead with the merge.
@a1q123456 @vvysokikh1 Can you please take a look at the PR when you get a chance?

xrplf-ai-reviewer

No issues.

Review by Claude Opus 4.6 · Prompt: V12

a1q123456 · 2026-03-26T14:28:05Z

+    // so m_allPaused can momentarily be true while a task is still finishing.
    std::unique_lock<std::mutex> lk{m_mut};
-    m_cv.wait(lk, [this] { return m_allPaused; });
+    m_cv.wait(lk, [this] { return m_allPaused && numberOfCurrentlyRunningTasks() == 0; });


so according to the code, m_allPaused == true implies that m_activeCount is 0, which further implies that no worker is currently running the for loop at the line 184, and further implies that numberOfCurrentlyRunningTasks() is 0.

If that's true, then adding this condition here doesn't work.

After discussion, this is not a real life bug and it only happens when we change the number of threads dynamically.

IMO, we should remove the functionality that changes the number of threads dynamically because we never need to do it and the function setNumberOfThreads is buggy. After you call setNumberOfThreads there's no guarantee that the current number of threads is the number you specified and there's no way for the user to wait until it finishes changing the number of threads. So a couple of things to do:

Remove the test that changes the number of threads

Remove the function setNumberOfThreads

Add a flag to say whether we're exiting

Simplify the logic in the worker thread entry by removing the outer do-while loop

When we call stop, we simply indicate that we're exiting and wait for worker threads to exit

Sounds like a much better way to go about this.

Yeah, I agree. It would be good to get rid of the complicated code since it is not used.

Question: How do we approach this? Shall we merge this change and then create another ticket to clean it up? Or close this PR, and directly clean the code?

Sorry, forgot to mention that I’ve created a ticket RIPD-5582.
That change isn’t directly relating to this PR so we can do it later in a separate PR

Copilot

Pull request overview

Fixes an intermittent Workers::stop() assertion by removing the logical race between “all workers paused” (m_allPaused) and “no tasks currently executing” (m_runningTaskCount) during shutdown of the Workers thread pool.

Changes:

Update Workers::stop() to wait on a single CV predicate that requires both m_allPaused and numberOfCurrentlyRunningTasks() == 0.
Notify m_cv when the running task count drops to zero, ensuring stop() can make progress when it is waiting on both conditions.
Remove an unused include after dropping the assertion.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…unt (#6574) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

…unt (XRPLF#6574) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

pratikmankawde requested review from a1q123456, bthomee and vvysokikh1 March 18, 2026 11:58

fix: Restore explicit lk.unlock() in Workers::stop()

385fc76

Keep the explicit unlock before returning from stop() to release the mutex as soon as it is no longer needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

pratikmankawde requested review from godexsoft and kuznetsss March 26, 2026 14:05

xrplf-ai-reviewer Bot approved these changes Mar 26, 2026

View reviewed changes

a1q123456 reviewed Mar 26, 2026

View reviewed changes

a1q123456 approved these changes Mar 26, 2026

View reviewed changes

kuznetsss requested a review from Copilot March 27, 2026 13:58

Copilot started reviewing on behalf of kuznetsss March 27, 2026 13:58 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

godexsoft approved these changes Mar 31, 2026

View reviewed changes

pratikmankawde added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Mar 31, 2026

bthomee added this pull request to the merge queue Mar 31, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Mar 31, 2026

bthomee added this pull request to the merge queue Mar 31, 2026

Merged via the queue into develop with commit bb95a7d Mar 31, 2026
3 checks passed

bthomee deleted the pratik/fix-workers-stop-race-condition branch March 31, 2026 15:23

bthomee pushed a commit that referenced this pull request Apr 1, 2026

fix: Fix Workers::stop() race between m_allPaused and m_runningTaskCo…

682dda8

…unt (#6574) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

mvadari added this to the 3.2.0 milestone May 20, 2026

beartec-jpg pushed a commit to beartec-jpg/FalconLedger that referenced this pull request Jun 1, 2026

fix: Fix Workers::stop() race between m_allPaused and m_runningTaskCo…

e8fc618

…unt (XRPLF#6574) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

a1q123456 mentioned this pull request Jun 2, 2026

fix: Fix the memory ordering issue on ARM for Workers #6302

Closed

13 tasks

Uh oh!

Conversation

pratikmankawde commented Mar 18, 2026

High Level Overview of Change

Context of Change

Type of Change

API Impact

Before / After

Test Plan

Uh oh!

codecov Bot commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

a1q123456 commented Mar 20, 2026

Uh oh!

bthomee commented Mar 20, 2026

Uh oh!

pratikmankawde commented Mar 24, 2026

Uh oh!

bthomee commented Mar 24, 2026

Uh oh!

pratikmankawde commented Mar 24, 2026

Uh oh!

bthomee commented Mar 24, 2026

Uh oh!

a1q123456 commented Mar 24, 2026

Uh oh!

pratikmankawde commented Mar 24, 2026

Uh oh!

a1q123456 commented Mar 25, 2026

Uh oh!

pratikmankawde commented Mar 26, 2026

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

a1q123456 Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

a1q123456 Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godexsoft Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

pratikmankawde Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

a1q123456 Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov Bot commented Mar 18, 2026 •

edited

Loading

a1q123456 Mar 26, 2026 •

edited

Loading