fix: Guard Coro::resume() against completed coroutines

[pratikmankawde]

High Level Overview of Change

Guard Coro::resume() against being called on an already-completed coroutine, fixing an intermittent assertion failure in CI.

Context of Change

A documented race condition (JobQueue.h:354-377) allows post() to schedule a resume() job that executes after the coroutine has already completed. With boost::coroutine v1, calling operator() on a completed coroutine was technically UB but happened to be benign in Release builds (BOOST_ASSERT compiled out). After the switch to boost::coroutine2 (PR #6372), the same call becomes genuine UB — the underlying boost::context::continuation is in a moved-from state.

The XRPL_ASSERT at Coro.ipp:79 caught this in CI Release builds with -Dassert=ON, surfacing as an intermittent crash that killed the test process.

Source analysis:

• v1 pull_coroutine_impl::pull(): guards with BOOST_ASSERT(!is_complete()) — compiled out in Release
• v2 pull_control_block::resume(): no guard at all — unconditionally calls std::move(c).resume() on completed continuation (UB)

Type of Change

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

API Impact

• libxrpl change (any change that may affect libxrpl or dependents of libxrpl)

Before / After

Before: A late resume() call (from the documented race) hits XRPL_ASSERT → process abort in assert-enabled builds, or UB (calling operator() on completed coroutine2) in non-assert builds.

After: A late resume() detects !coro_, cleans up (LocalValues, running_, cv_), and returns early. The race condition is handled safely by design.

Test Plan

• Existing unit tests cover coroutine lifecycle
• The fix addresses an intermittent CI failure — the race depends on thread scheduling and is not reliably reproducible in a unit test
• CI run on this branch will validate no regressions

Jira: RIPD-5379

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.5%. Comparing base (509677a) to head (d594614).
⚠️ Report is 2 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #6608     +/-   ##
=========================================
- Coverage     81.5%   81.5%   -0.0%     
=========================================
  Files          998     998             
  Lines        74456   74456             
  Branches      7578    7558     -20     
=========================================
- Hits         60648   60645      -3     
- Misses       13808   13811      +3     

Files with missing lines	Coverage Δ
include/xrpl/core/Coro.ipp	100.0% <100.0%> (ø)
include/xrpl/core/JobQueue.h	100.0% <ø> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[pratikmankawde]

/ai-review

[xrplf-ai-reviewer]

Deadlock risk found in coroutine lock management.

Review by Claude Opus 4.6 · Prompt: V12

[kuznetsss]

👍

[xrplf-ai-reviewer]

Two low-severity correctness concerns flagged inline: a potential LocalValues TLS lifecycle violation on the early-return path (line 76), and loss of diagnostic coverage from removing the assertion (line 86).

Review by Claude Opus 4.6 · Prompt: V12

[bthomee]

@pratikmankawde are there any action items from the AI reviewer or can they be dismissed?

[pratikmankawde]

@pratikmankawde are there any action items from the AI reviewer or can they be dismissed?

No action needed.

[Copilot]

Pull request overview

This PR hardens JobQueue::Coro::resume() against being invoked after a coroutine has already completed, addressing an intermittent CI assertion/UB triggered by the documented post()-before-yield() race in JobQueue.

Changes:

• Update Coro::resume() to avoid invoking boost::coroutines2 pull coroutines once they are completed.
• Update JobQueue coroutine documentation to describe the completed-coroutine behavior and the boost::coroutine2 UB risk.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
include/xrpl/core/JobQueue.h	Updates API/race-condition documentation to reflect safe handling of late resume().
include/xrpl/core/Coro.ipp	Adds a runtime guard so resume() does not call operator() on a completed coroutine.

---

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

[a1q123456]

i see that you’re now suffering from the same pain I had yesterday

[xrplf-ai-reviewer]

nSuspend_ counter leak on late-resume path — see inline.

Review by Claude Opus 4.6 · Prompt: V12

[pratikmankawde]

i see that you’re now suffering from the same pain I had yesterday

No pain no gain ;-)

[xrplf-ai-reviewer]

One observability concern flagged inline — the silent if (coro_) guard correctly prevents UB from the documented race, but also silently swallows unrelated double-resume bugs that the removed assert would have caught. A warning log in the else branch is recommended.

Review by Claude Opus 4.6 · Prompt: V12

[xrplf-ai-reviewer]

Silent guard swallows double-resume bugs too — add a diagnostic log in the else branch to preserve observability:

    if (coro_)
    {
        coro_();
    }
    else
    {
        JLOG(j_.warn()) << "Coro::resume() called on completed coroutine — possible double-resume bug";
    }

[bthomee]

@pratikmankawde I'll go ahead and merge this change, but if this comment is valid it will be easy to address in a follow-up PR.

[pratikmankawde]

A double resume is quite rare as per the current architecture. So the else part is an extraneous detail to have. And it won't be needed after switching to std::coroutine. So, we can ignore this.