feat(async): track current running coroutine for graph export#81
feat(async): track current running coroutine for graph export#8116bit-ykiko wants to merge 2 commits intomainfrom
Conversation
…ph export Add a thread-local async_node* that always points to the coroutine currently executing on this thread. This allows calling async_node::current() or async_node::dump_current_dot() from anywhere inside a coroutine body to export the async graph on demand. Tracking points: - initial_tracking_suspend sets the node on first coroutine entry - async_node::resume() and resume_and_drain() save/restore around resumes - handle_subtask_result(), deliver_deferred(), propagate_fail() set the node before returning a handle for symmetric transfer Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdded thread-local tracking of the currently executing async_node and plumbing to set/restore it across coroutine resume/propagation. Changes
Sequence Diagram(s)sequenceDiagram
participant Thread as Thread-Local
participant Detail as detail API
participant Node as async_node
participant Coro as Coroutine
rect rgba(120,180,240,0.5)
Note over Thread,Detail: Resume entry with node tracking
Thread->>Node: save prev = current_node()
Node->>Detail: set_current_node(this)
Detail->>Thread: update thread-local
Node->>Coro: resume()
Coro->>Detail: query current_node()
Detail->>Coro: return this
Coro-->>Node: returns/finishes
Node->>Thread: detail::set_current_node(prev)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/async/runtime/frame.cpp (1)
66-75:⚠️ Potential issue | 🔴 Critical
resume_and_drain()now captures the wrong restore point.This helper assumes
current_running_nodestill holds the outer context on entry, but the new pre-transfer writes inhandle_subtask_result()/deliver_deferred()overwrite it before callback paths such assystem_op::complete()reach Line 68. The thread can then leave the callback still pointing at the resumed task instead of the real outer context, and if that task finishes in the same chaindrain_pending_destroys()can leave TLS dangling.One way to make the restore point explicit
-void detail::resume_and_drain(std::coroutine_handle<> handle) { +void detail::resume_and_drain(async_node* restore_to, std::coroutine_handle<> handle) { if(handle) { - auto* prev = current_running_node; handle.resume(); - current_running_node = prev; + current_running_node = restore_to; } `#if` ETD_WORKAROUND_MSVC_COROUTINE_ASAN_UAF drain_pending_destroys(); `#endif` }The callback entry points then need to capture
detail::current_node()before they callhandle_subtask_result()ordeliver_deferred(), and pass that saved value through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/async/runtime/frame.cpp` around lines 66 - 75, resume_and_drain() currently saves the restore point from the global TLS current_running_node too late and can be clobbered by pre-transfer writes in handle_subtask_result()/deliver_deferred(), causing TLS to remain pointing at the resumed task; change resume_and_drain to accept an explicit previous node parameter (e.g., resume_and_drain(detail::node* prev, std::coroutine_handle<> handle) or similar) and restore that value instead of reading current_running_node inside the function, and update all call sites (notably places that invoke resume_and_drain() via system_op::complete and other callback paths) so they capture detail::current_node() before calling handle_subtask_result()/deliver_deferred() and pass that saved node through to resume_and_drain; keep drain_pending_destroys() behavior unchanged.
🧹 Nitpick comments (1)
include/eventide/async/runtime/frame.h (1)
125-131: Makecurrent()read-only.The new API is for inspection, but
async_node*lets callers mutatestate, callcancel(), orresume()on the live runtime node. Returningconst async_node*keeps the graph-export/debugging use case without widening the mutation surface.Safer public signature
- static async_node* current() noexcept; + static const async_node* current() noexcept;
dump_dot()is alreadyconst, so the export path still works unchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/eventide/async/runtime/frame.h` around lines 125 - 131, current() currently returns a mutable async_node* allowing callers to mutate runtime state; change its signature to return const async_node* noexcept so callers only get a read-only view (update the declaration of static async_node* current() noexcept to static const async_node* current() noexcept and ensure any callers that relied on a mutable pointer are adjusted), leaving dump_current_dot() and async_node::dump_dot() unchanged since dump_dot() is const.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/async/runtime/frame.cpp`:
- Around line 66-75: resume_and_drain() currently saves the restore point from
the global TLS current_running_node too late and can be clobbered by
pre-transfer writes in handle_subtask_result()/deliver_deferred(), causing TLS
to remain pointing at the resumed task; change resume_and_drain to accept an
explicit previous node parameter (e.g., resume_and_drain(detail::node* prev,
std::coroutine_handle<> handle) or similar) and restore that value instead of
reading current_running_node inside the function, and update all call sites
(notably places that invoke resume_and_drain() via system_op::complete and other
callback paths) so they capture detail::current_node() before calling
handle_subtask_result()/deliver_deferred() and pass that saved node through to
resume_and_drain; keep drain_pending_destroys() behavior unchanged.
---
Nitpick comments:
In `@include/eventide/async/runtime/frame.h`:
- Around line 125-131: current() currently returns a mutable async_node*
allowing callers to mutate runtime state; change its signature to return const
async_node* noexcept so callers only get a read-only view (update the
declaration of static async_node* current() noexcept to static const async_node*
current() noexcept and ensure any callers that relied on a mutable pointer are
adjusted), leaving dump_current_dot() and async_node::dump_dot() unchanged since
dump_dot() is const.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1c8aa54-d09a-4fc9-b10a-ab8b2eb18bc3
📒 Files selected for processing (3)
include/eventide/async/runtime/frame.hinclude/eventide/async/runtime/task.hsrc/async/runtime/frame.cpp
| // ============================================================================ | ||
| // initial_tracking_suspend — sets current node when coroutine body first runs | ||
| // ============================================================================ | ||
|
|
||
| struct initial_tracking_suspend { | ||
| async_node* node; | ||
|
|
||
| bool await_ready() const noexcept { | ||
| return false; | ||
| } | ||
|
|
||
| void await_suspend(std::coroutine_handle<>) const noexcept {} | ||
|
|
||
| void await_resume() const noexcept { | ||
| detail::set_current_node(node); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Tracking is lost after co_awaiting pass-through awaitables.
initial_tracking_suspend only seeds TLS on the first resume. Since task_promise_object::await_transform() still forwards arbitrary awaitables unchanged at Lines 367-371, any awaitable that later resumes this coroutine via a raw handle bypasses the new bookkeeping and async_node::current() becomes null/stale in the continued body. Either narrow the contract to Eventide-managed awaitables or route external awaits through a tracked adapter.
- resume_and_drain now takes an explicit restore_to parameter so the correct previous node is restored even after handle_subtask_result / deliver_deferred overwrite the thread-local - async_node::current() returns const async_node* to prevent mutation of the live runtime node through the diagnostic API Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Re: pass-through awaitables losing tracking This is an intentional limitation. All of Eventide's own awaitables (tasks, sync primitives, IO ops, aggregates) go through the tracked resume paths ( Third-party awaitables that resume a coroutine via a raw |
1 participant
Summary
thread_local async_node*that tracks which coroutine is currently executing on each threadasync_node::current()andasync_node::dump_current_dot()public API so user code inside any coroutine can export the async graph on demandinitial_tracking_suspend(first entry), save/restore inresume()/resume_and_drain()(entry points), and direct assignment inhandle_subtask_result()/deliver_deferred()/propagate_fail()(symmetric transfer)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes