[DO NOT MERGE][core] Fix actor arg fetch pipelining v2#63765
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the CoreWorker's actor task execution queues to separate queue bookkeeping (running on io_service_) from user task execution (running on task_execution_service_ or concurrency-group pools) to prevent long-running tasks from blocking gRPC handlers. Feedback on these changes highlights critical thread-safety and data race hazards in both OrderedActorTaskExecutionQueue and UnorderedActorTaskExecutionQueue due to concurrent queue state mutations across different threads in AcceptRequestOrRejectIfCanceled. Additionally, it is recommended to defensively post regular actor cancellations to io_service_ rather than executing them inline to avoid potential thread-safety issues.
| task_execution_service_.post( | ||
| [this, request = std::move(request), task_id]() mutable { | ||
| AcceptRequestOrRejectIfCanceled(task_id, request); | ||
| }, | ||
| "OrderedActorTaskExecutionQueue.AcceptRequest"); |
There was a problem hiding this comment.
Thread-Safety and Data Race Hazard
By posting AcceptRequestOrRejectIfCanceled to task_execution_service_ (or running it on a concurrency-group pool thread), the execution of the task and any subsequent queue bookkeeping/cleanup inside AcceptRequestOrRejectIfCanceled will run on a different thread than io_service_.
Since OrderedActorTaskExecutionQueue is not thread-safe and does not use internal locking, concurrent access to the queue's state (such as group_states_, pending_tasks_, or canceled_tasks_) from io_service_ (e.g., in EnqueueTask or CancelTaskIfFound) and task_execution_service_ / pool threads will result in data races and undefined behavior.
Furthermore, if there are any thread-affinity checks like RAY_CHECK(std::this_thread::get_id() == main_thread_id_) inside AcceptRequestOrRejectIfCanceled or functions it calls, they will fail and crash the worker because main_thread_id_ is now initialized to the io_service_ thread ID.
Recommendation:
Ensure that all queue state mutations and lookups are strictly serialized on io_service_, or introduce proper synchronization (e.g., a mutex) to protect the queue's internal state.
| task_execution_service_.post( | ||
| [this, request = std::move(request), task_id]() mutable { | ||
| AcceptRequestOrRejectIfCanceled(task_id, request); | ||
| }, | ||
| "UnorderedActorTaskExecutionQueue.AcceptRequest"); |
There was a problem hiding this comment.
Thread-Safety and Data Race Hazard
Similar to OrderedActorTaskExecutionQueue, posting AcceptRequestOrRejectIfCanceled to task_execution_service_ (or running it on a concurrency-group pool thread) means that the task execution and subsequent queue bookkeeping/cleanup will run on a different thread than io_service_.
Since UnorderedActorTaskExecutionQueue is not thread-safe and does not use internal locking, concurrent access to the queue's state (such as running_tasks_, pending_tasks_, or canceled_tasks_) from io_service_ (e.g., in EnqueueTask or CancelTaskIfFound) and task_execution_service_ / pool threads will result in data races and undefined behavior.
For example, in AcceptRequestOrRejectIfCanceled, when a deferred RunRequest is identified and posted back to io_service_, the queue's internal state is accessed/mutated on the task_execution_service_ / pool thread, which directly races with io_service_.
Recommendation:
Ensure that all queue state mutations and lookups are strictly serialized on io_service_, or introduce proper synchronization (e.g., a mutex) to protect the queue's internal state.
| } else { | ||
| // For regular actor, we cannot post it to task_execution_service because | ||
| // main thread is blocked. Threaded actor can do both (dispatching to | ||
| // task execution service, or just directly call it in io_service). | ||
| // There's no special reason why we don't dispatch | ||
| // cancel to task_execution_service_ for threaded actors. | ||
| // For regular (sync) actor, we are already on io_service_ here (this is | ||
| // invoked from a gRPC handler), so call inline. | ||
| cancel(); | ||
| } |
There was a problem hiding this comment.
Defensive Threading: Avoid Inline Execution on Assumed Thread
For regular (sync) actors, the code calls cancel() inline under the assumption that CancelActorTaskOnExecutor is always invoked on the io_service_ thread (e.g., from a gRPC handler).
However, if this method is ever called from a different thread (for example, during local cancellation, testing, or if the gRPC server thread pool configuration changes), calling cancel() inline will bypass the thread-safety guarantees of io_service_ and cause data races on the queue's bookkeeping state.
Recommendation:
To be defensive and robust against future refactorings or different invocation paths, consider posting the cancellation to io_service_ for regular actors as well, or add a thread-assertion before calling cancel() inline.
} else {
io_service_.post([cancel = std::move(cancel)]() { cancel(); },
"CoreWorker.CancelActorTaskOnExecutor");
}
stephanie-wang
left a comment
There was a problem hiding this comment.
The logic looks good. I wonder if you can also add an integration test to check that actor arg fetch is being pipelined properly.
a1f7d89 to
f7a1e74
Compare
Added the test in |
| The IPC that tells the local raylet to pull a task's args runs on | ||
| io_service_. The user task body runs on task_execution_service_ (or a | ||
| concurrency-group pool). These are separate event loops, so while one | ||
| task body is executing, args for the next queued task can be pulled in | ||
| parallel. | ||
|
|
||
| Previously the IPC and the body shared task_execution_service_, so once | ||
| a body was running, IPCs for queued tasks could not fire and pulling was | ||
| delayed until the body completed. |
There was a problem hiding this comment.
nit: these are core worker / C++ implementation details, they don't belong in a python integration test. there's also no need to describe the previous state of the code prior to the change.
| able to pull any args. | ||
|
|
||
| """ | ||
| from ray.experimental import get_object_locations |
There was a problem hiding this comment.
no need for delayed import
| assert set(node_ids) == worker_node_ids | ||
|
|
||
|
|
||
| def test_actor_arg_fetch_is_pipelined_with_task_execution(ray_start_cluster): |
There was a problem hiding this comment.
Did you run this w/o the code change and validate that it failed?
There was a problem hiding this comment.
yes. w/o code change, no args pulled and test times out
| // wait_timer_ is bound to io_service_ because the queue's bookkeeping | ||
| // runs on io_service_ now. |
There was a problem hiding this comment.
again no need to comment about the changes from previous impl like this (claude likes to do this)
There was a problem hiding this comment.
cleaned up the comments
| if (pool == nullptr) { | ||
| task_execution_service_.post(std::move(execute_handler), | ||
| "OrderedActorTaskExecutionQueue.Execute"); | ||
| } else { | ||
| pool->Post(std::move(execute_handler)); | ||
| } |
There was a problem hiding this comment.
would be nice to put these behind some kind of shared interface so we don't need the branches
There was a problem hiding this comment.
made the change. LMK if this looks fine
| /// \param task_execution_service The io_context that user task bodies | ||
| /// (`request.Execute()`) are posted to when no concurrency-group thread pool | ||
| /// is available. Decoupled from `io_service` so a long-running user task | ||
| /// never blocks bookkeeping / arg-fetch IPCs on `io_service`. |
There was a problem hiding this comment.
Related to my comment above, a cleaner way to structure this would be to have some kind of "postable" interface that both the thread pool and task execution service can implement. Then here you can pass in a "default task executor postable" (vague, hopefully you get the idea) and avoid exposing the details of "task_execution_service" here
| post_execute = [fiber = std::move(fiber)](std::function<void()> task) mutable { | ||
| fiber->EnqueueFiber(std::move(task)); | ||
| }; |
There was a problem hiding this comment.
PostExecuteFn looks a lot like what I suggested above :)
| } | ||
|
|
||
| // Accept can be very long, and we shouldn't hold a lock. | ||
| // Helper: run the post-task-completion bookkeeping . |
| }; | ||
|
|
||
| if (is_canceled) { | ||
| // Note that cancel is now called on io_service_ |
There was a problem hiding this comment.
ditto again -- please spend time cleaning up the AI slop comments
f7a1e74 to
e46325d
Compare
| "OrderedActorTaskExecutionQueue.Execute"); | ||
| } else { | ||
| pool->Post(std::move(execute_handler)); | ||
| } |
There was a problem hiding this comment.
Cancel race after async execute post
Medium Severity
After AcceptRequestOrRejectIfCanceled sees is_canceled as false, it posts request.Execute() to another executor and returns on io_service_. A later CancelTaskIfFound on the same loop can set the cancel flag and report success while the already-posted handler still runs the task body, because execute_handler never re-reads the flag before calling Execute().
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e46325d. Configure here.
|
There are a bunch of failing serve tests that I am guessing are related: https://buildkite.com/ray-project/premerge/builds/68205 Perhaps there are unintentional changes to asyncio actor behavior? |
| std::make_unique<OrderedActorTaskExecutionQueue>( | ||
| task_execution_service_, | ||
| io_service_, | ||
| std::move(default_postable), |
There was a problem hiding this comment.
Stale executors at queue creation
High Severity
Per-caller actor queues copy pool_manager_ and fiber_state_manager_ at first PushTask, but actor tasks now enqueue on io_service_ immediately while creation still runs on task_execution_service_. Pipelined arg completion can dispatch execution before creation installs real fiber/pool managers, leaving asyncio actors with a null fiber_state_manager_ or wrong concurrency settings permanently.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 39072cc. Configure here.
| namespace ray { | ||
| namespace core { | ||
|
|
||
| /// Handle for posting a piece of work onto an execution context |
There was a problem hiding this comment.
There is something similar in gcs:
ray/src/ray/gcs/postable/postable.h
Line 30 in 5d4bc1b
Should see if it makes sense to unify
| RAY_CHECK(op_status == boost::fibers::channel_op_status::success); | ||
| } | ||
|
|
||
| void Post(std::function<void()> fn) override { EnqueueFiber(std::move(fn)); } |
There was a problem hiding this comment.
any reason to keep EnqueueFiber around instead of just renaming to Post?
|
LGTM, but serve tests still failing which is concerning |
Signed-off-by: Kartica Modi <karticamodi@gmail.com>
Signed-off-by: Kartica Modi <karticamodi@gmail.com>
Signed-off-by: Kartica Modi <karticamodi@gmail.com>
Signed-off-by: Kartica Modi <karticamodi@gmail.com>
Signed-off-by: Kartica Modi <karticamodi@gmail.com>
Signed-off-by: Kartica Modi <karticamodi@gmail.com>
Signed-off-by: Kartica Modi <karticamodi@gmail.com>
Signed-off-by: Kartica Modi <karticamodi@gmail.com>
3ad7ff5 to
464d288
Compare
Signed-off-by: Kartica Modi <karticamodi@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Reviewed by Cursor Bugbot for commit 5a8b83d. Configure here.
| }, | ||
| "CoreWorker.MarkActorTaskArgsReady"); | ||
| RAY_LOG(DEBUG) << "Actor task args are ready for tag: " << request.tag(); | ||
| actor_task_execution_arg_waiter_->MarkReady(request.tag()); |
There was a problem hiding this comment.
Late arg wait after fiber Stop
High Severity
For asyncio actors, shutdown now runs task_receiver_->Stop() on io_service_ and closes the fiber channel while HandleActorCallArgWaitComplete still calls MarkReady inline with no shutdown guard. Pipelined arg prefetch leaves more in-flight waits; a late completion can still dispatch execution via EnqueueFiber on a closed channel and hit a fatal RAY_CHECK.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 5a8b83d. Configure here.
Include <Windows.h> before boost/asio in thread_pool.h to reproduce the WinSock.h / WinSock2.h header conflict on Windows builds. This mirrors the issue seen in PR ray-project#63765 where header ordering in task_execution files caused the same compilation failure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Robert Nishihara <rkn@anyscale.com>
Compilation errors can happen if `Windows.h` gets included before `Boost.Asio` headers (this causes `WinSock.h`, which is included by `Windows.h`, to conflict with `WinSock2.h`, which is included by `Boost.Asio`). The current solution is to define `WIN32_LEAN_AND_MEAN` in a bunch of files. However, this is a bit fragile (and I noticed a compilation error in #63765 caused by this). Separately, I verified this in #64331 (the first commit triggers the compilation error, the second resolves it by setting the global compiler flag). The goal of this PR is to solve the compilation issue more globally. This PR does two things - Add `-DWIN32_LEAN_AND_MEAN` to the Windows compiler flags. This prevents Windows.h from including WinSock.h, which conflicts with the WinSock2.h required by Boost.Asio. It seems pretty normal to set this as a global compiler flag. We already do this for hiredis. - Remove redundant include guards and definitions of `WIN32_LEAN_AND_MEAN` from files. Note that we continue to define it in cpp/include/ray/api/logging.h because in theory that file can be consumed externally without the compiler flags. Some details on what `WIN32_LEAN_AND_MEAN` excludes are in this post https://stackoverflow.com/questions/11040133/what-does-defining-win32-lean-and-mean-exclude-exactly --------- Signed-off-by: Robert Nishihara <rkn@anyscale.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…oject#64361) Compilation errors can happen if `Windows.h` gets included before `Boost.Asio` headers (this causes `WinSock.h`, which is included by `Windows.h`, to conflict with `WinSock2.h`, which is included by `Boost.Asio`). The current solution is to define `WIN32_LEAN_AND_MEAN` in a bunch of files. However, this is a bit fragile (and I noticed a compilation error in ray-project#63765 caused by this). Separately, I verified this in ray-project#64331 (the first commit triggers the compilation error, the second resolves it by setting the global compiler flag). The goal of this PR is to solve the compilation issue more globally. This PR does two things - Add `-DWIN32_LEAN_AND_MEAN` to the Windows compiler flags. This prevents Windows.h from including WinSock.h, which conflicts with the WinSock2.h required by Boost.Asio. It seems pretty normal to set this as a global compiler flag. We already do this for hiredis. - Remove redundant include guards and definitions of `WIN32_LEAN_AND_MEAN` from files. Note that we continue to define it in cpp/include/ray/api/logging.h because in theory that file can be consumed externally without the compiler flags. Some details on what `WIN32_LEAN_AND_MEAN` excludes are in this post https://stackoverflow.com/questions/11040133/what-does-defining-win32-lean-and-mean-exclude-exactly --------- Signed-off-by: Robert Nishihara <rkn@anyscale.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
#63079) Issue: The current design for actors arg fetch doesn't pipeline fetching args when there is an actor task running on `task_execution_service`, even though arg pulling is done by the raylet and not the core_worker. This is because the IPC call made by the `core_worker` to the `raylet` to start pulling args is posted on the `task_execution_service`. Therefore, if a task is running, the IPC call can't be made and thus, arg pulling is not done. Fix: To fix this, we trigger the raylet IPC call from the `io_service` and then queue the task execution on `task_execution_service`. This PR breaks down the `ActorTaskExecutionArgWaiter` API into three parts - 1. `BeginArgsFetch`: Make the raylet IPC call and get a tag for this call. 2. `OnArgsReady`: Register a callback which will be triggered when args are ready for this task (identified by the tag returned in 1.). 3. `MarkReady`: Fire the registered callback when args are ready. 2 & 3 are done on the `task_execution_service`. In most cases, 2 would always be called before 3 by virtue of `task_execution_service` being FIFO. HandlePushTask (which queues 2) would be finished to completion before `HandleActorCallArgWaitComplete` could run and post 3 on `task_execution_service`. For the cases, where this doesn't hold true (for eg: unordered actors where some tasks are executed later), and `MarkReady` is called before the callback is registered, in `OnArgsReady` the callback is executed synchronously. Results: I tested the change against a ray script which spins up a producer actor and a consumer actor on different nodes. The producer produces large objects which are consumed by the consumer triggering plasma object pulls. The consumer is slow (sleeps infinitely). Without the fix, the consumer pulls some objects, but as soon as it starts executing the task (which sleeps infinitely), object pulling stops and as a result, only some of the objects are pulled from the producer side. With the fix, all objects are pulled from producer. Observe the object store memory column below (each object is 128MB) <img width="1580" height="723" alt="image" src="https://github.com/user-attachments/assets/4b8e28b0-6cd1-487b-996f-645a3b321edd" /> Producer produced 10 objects, consumer pulled only 3 (buggy case) <img width="1578" height="726" alt="image" src="https://github.com/user-attachments/assets/02af1966-781a-4187-8bf7-e23d41d375c0" /> Producer produced 10 objects, consumer pulled all (fixed case) NOTES: 1. Another caveat with this is that we might run into OOMs or object spillings if a large number of tasks are submitted to actors with significant argument size since we are fetching args right on `HandlePushTask`. However, in the existing design too, this guarantee isn't provided since it also triggers arg fetch for all tasks until the time first arg fetch is complete and the task starts executing. In the worst case, it could also trigger arg fetches enough to OOM if the first arg fetch takes long enough. EDIT: Another solution was considered as per some suggestions in this PR was to restructure the code so that only the user task body (request.Execute()) runs on task_execution_service_ (or a concurrency-group pool / asyncio fiber). Everything else — queue bookkeeping, args-fetch IPC dispatch, cancellation decisions, dependency resolution, timer management — runs on io_service_. Here is the PR for that approach: #63765 This led to several issues with async actors which were hard to debug: 1. Serve tests started to fail - `EnqueueFiber` which pushes the function to an unbuffered channel blocks, and since the operation is done on `io_service_`, this results in health-check RPC failing and the ServeController force-killed healthy replicas. 2. `test_async_actor_cancel`: Became flaky due to running cancellation path on `io_service_`. 3. `test_asyncio_actor_shutdown_when_non_async_method_mixed` started to fail as well. Due to this, we have decided to go ahead with the targeted approach in this PR for now. --------- Signed-off-by: Kartica Modi <karticamodi@gmail.com> Co-authored-by: Edward Oakes <ed.nmi.oakes@gmail.com>


Issue
Actor task arg-fetching was getting blocked by in-flight actor task execution. When a PushTask RPC arrives, the worker sends an IPC to the raylet asking it to pull the args; that IPC was being dispatched on
task_execution_service_, the same loop that runs user actor task bodies. If the actor was busy executing a task, the IPC sat in the queue behind it — defeating the whole point of pipelining args-fetch with execution.Fix
Restructure so that only the user task body (request.Execute()) runs on
task_execution_service_(or a concurrency-group pool / asyncio fiber). Everything else — queue bookkeeping, args-fetch IPC dispatch, cancellation decisions, dependency resolution, timer management — runs onio_service_Changes:
OrderedActorTaskExecutionQueueandUnorderedActorTaskExecutionQueuenow take bothio_service_andtask_execution_service_. Thewait_timer_and all internal state live onio_service_.TaskReceiver::QueueTaskForExecutionfor actor tasks is no longer wrapped intask_execution_service_.post(...)— it runs inline onio_service_fromHandlePushTask.CoreWorker::HandleActorCallArgWaitCompleteinvokesMarkReadyinline onio_service_instead of posting totask_execution_service_.CoreWorker::CancelActorTaskOnExecutorasync-actor branch posts toio_service_(where the queue state now lives) instead oftask_execution_service_; the post is now a self-post and can be done inline.CoreWorkerShutdownExecutor::ExecuteExitposts the drain/Stop chain toio_service_instead oftask_execution_service_.AcceptRequestOrRejectIfCanceled: the cancellation check runs onio_service_; onlyrequest.Execute()is posted off to the pool / fiber /task_execution_service_.Notes:
Additionally I also found two potential bugs in existing code:
actor_task_execution_queues_without a mutex.In master:
QueueTaskForExecutionwrote the per-caller map (find+emplace) ontask_execution_service_.CancelQueuedActorTaskfor sync actors read the map onio_service_Accessing the hashmap from two threads is UB. After this change, both access paths are on
io_service_.In master,
AcceptRequestOrRejectIfCanceledadnCancelTaskIfFoundcan run on different threads, Due to the release ofmu_beforerequest.Execute(), we could have this race:a. Accept reads is_canceled=false, releases mu_.
b. CancelTaskIfFound (concurrently) sets pending_task_id_to_is_canceled[task_id]=true, returns task_present=true.
c. The cancel RPC reports success=true ("cancelled in queue") to the caller.
d. Accept then calls request.Execute(). Task runs to completion anyway.
After this PR, both functions run on
io_service_, and therefore are serialized.Results:
I tested the change against a ray script which spins up a producer actor and a consumer actor on different nodes. The producer produces large objects which are consumed by the consumer triggering plasma object pulls. The consumer is slow (sleeps infinitely). Without the fix, the consumer pulls some objects, but as soon as it starts executing the task (which sleeps infinitely), object pulling stops and as a result, only some of the objects are pulled from the producer side. With the fix, all objects are pulled from producer.
Observe the object store memory column below (each object is 128MB)
TODO:
With the change in this PR, all submitted actor tasks will have their arguments prefetched. This might result in OOM issues or starvation of one actor tasks due to prefetching args of another actor tasks. To fix this, the solution would be to impose a limit on prefetches per concurrency group. I will take that up in a followup PR.
It is also important to note that the current design too doesn't protect against it intentionally. The current implementation also queues up arg fetches for all tasks until the time first arg fetch is complete and the task starts executing. In the worst case, it could also tigger prefetches enough to OOM if the first fetch takes a long time and the task is not scheduled.