[v4xsh]

Fixes #7713

Proposed Changes

• Keep the function's result in invoke_directly so reset() doesn't turn it into unknown. A stackful child run directly now ends terminated, like stackless after futures: run stackless run_as_child children directly #7718.
• Drop the deleted rewrite in stackful call_directly. It was the only producer of deleted and never fired.
• execute_thread, the future_data assert and the task_completed hook accept only terminated.

Any background context you want to provide?

The places that still read deleted are left as is to keep this small.

The regression test fails on master and passes with the fix at 1 and 4 workers, with no backend, ITT and Tracy (Debug).

Checklist

Not all points below apply to all pull requests.

• I have added a new feature and have added tests to go along with it.
• I have fixed a bug and have added a regression test.
• I have added a test using random numbers; I have made sure it uses a seed, and that random numbers generated are valid inputs for the tests.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 40a19d3a-7bc0-4421-90a0-5df354a58f3a

📥 Commits

Reviewing files that changed from the base of the PR and between 6456abf and 79e67b2.

📒 Files selected for processing: 6

• libs/core/coroutines/src/detail/coroutine_impl.cpp
• libs/core/futures/src/detail/execute_thread.cpp
• libs/core/futures/src/future_data.cpp
• libs/core/futures/tests/regressions/CMakeLists.txt
• libs/core/futures/tests/regressions/stackful_direct_execution_7713.cpp
• libs/core/threading_base/include/hpx/threading_base/thread_data_stackful.hpp

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Summary

Summary by CodeRabbit

• Bug Fixes

  • Improved reliability of directly executed child tasks by requiring them to reach a proper terminated state before completion is reported.
  • Prevented completed child tasks from being incorrectly classified as deleted or unknown.
  • Ensured coroutine results are preserved and returned correctly after direct execution.

• Tests

  • Added regression coverage for stackful child tasks, including result validation, termination checks, and inline execution with a single worker.

Direct execution now preserves a stackful child’s termination state through coroutine reset. The futures execution paths accept only terminated as completion. A regression test checks child results and termination with one and four workers.

Changes: Stackful Child Direct Execution

Layer / File(s)	Summary
Preserve and propagate child termination libs/core/coroutines/src/detail/coroutine_impl.cpp, libs/core/threading_base/include/hpx/threading_base/thread_data_stackful.hpp, libs/core/futures/src/detail/execute_thread.cpp, libs/core/futures/src/future_data.cpp	invoke_directly preserves the function result across coroutine reset. call_directly returns that result without changing terminated to deleted. The futures execution paths accept only terminated as completion.
Verify direct child execution libs/core/futures/tests/regressions/stackful_direct_execution_7713.cpp, libs/core/futures/tests/regressions/CMakeLists.txt	The regression test posts 100 child tasks, checks their results and termination, and checks inline execution when one worker is available. The test is registered for the default and four-thread variants.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 92cde

Merge Risk: ⚪ Minimal · up to 92cde

The direct child-execution fix is ready for normal merge checks; no blocking issue was identified.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 5 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly identifies the main change: preserving the result of a stackful child executed directly with run_as_child.
Description check		The description directly explains the bug fix, related state changes, regression test, and validation results. It is clearly related to the changeset.
Linked Issues check		For directly linked issue #7713, invoke_directly stores the function result in result_last, calls reset(), and then binds result_last. The code asserts that the result is terminated. `thread…
Out of Scope Changes check		The reported changes are connected to #7713. The coroutine result preservation fixes the reported state loss. The call_directly, execute_thread, and future_data changes update the affected state…

Full details: Linked Issues check

Explanation

For directly linked issue #7713, invoke_directly stores the function result in result_last, calls reset(), and then binds result_last. The code asserts that the result is terminated. thread_data_stackful::call_directly returns this result without rewriting it to deleted. The supplied change summary also records matching execute_thread and future_data updates. The new regression test posts run_as_child stackful tasks, checks successful futures, and verifies terminated state. These changes satisfy the direct-execution coding requirement in #7713. The issue's broader scheduler concern is outside the stated direct-execution scope and is explicitly left unchanged.

Full details: Out of Scope Changes check

Explanation

The reported changes are connected to #7713. The coroutine result preservation fixes the reported state loss. The call_directly, execute_thread, and future_data changes update the affected state consumers. The regression test and CMake registration verify the fix. Leaving other deleted reads unchanged does not add unrelated behavior.

Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 5 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches: 💡 1

🛠️ Fix failing CI checks: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[StellarBot]

Can one of the admins verify this patch?

[hkaiser]

Thanks, this looks right to me. Keeping the result in a local through reset() matches what operator() already does, and since the scheduling loop releases the leftover queue entry of a child that already ran (#7718), the stackful child can now end terminated just like the stackless one. Dropping the deleted rewrite is safe, it could never trigger before and would now only diverge from the stackless path.

The failing "Linux CI (Debug) with HWLoc fetch" and CodeQL jobs fail at the CMake generate step on the fetched hwloc, which #7821 fixed on master since; they are unrelated.

One effect listed in #7713 remains: a child run directly is still not counted in /threads/count/cumulative, since neither execute_thread nor the scheduling loop (which releases its queue entry without counting it) counts it. That's the open "where should such a thread be counted" question from the issue, so it's fine to leave it for a follow-up.