[core] Evict dead actors from ActorPool instead of recycling them - #64646
[core] Evict dead actors from ActorPool instead of recycling them#64646verma-divyanshu-git wants to merge 10 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to evict dead actors from the ActorPool upon encountering a RayActorError. However, the feedback identifies critical bugs in both get_next and get_next_unordered where delaying the return of the actor until after ray.get(future) causes actors to leak from the pool if other exceptions (like RayTaskError) are raised. The reviewer suggests using a try...except...finally block to safely return actors in all scenarios. Additionally, the feedback points out a potential UnboundLocalError in get_next_unordered and recommends adding a unit test to ensure standard task errors do not cause actor leaks.
ActorPool returned a crashed actor to the idle set, so submit() kept dispatching to it and every following task failed with RayActorError. _return_actor now tracks liveness and only puts the actor back when it is still alive; a dead actor is dropped so the pool keeps making progress. The actor is returned in a finally block and marked dead only on RayActorError, so a normal task failure (RayTaskError) no longer leaks a healthy actor from the pool. Closes ray-project#50313 Signed-off-by: Divyanshu <bautocrats@gmail.com>
56051f4 to
dfdfb3d
Compare
ray-gardener
Bot
added
core
|
|
||
| self._return_actor(a) | ||
| if raise_timeout_after_ignore: | ||
| self._return_actor(a) |
There was a problem hiding this comment.
what happens if actor dies after timeout? the crash will be detected in the next task submission but we end up skipping the input (ie an innocent task will be sacrificed to detect the actor failure, and that task never gets retried as it is not the reason for the actor death)
There was a problem hiding this comment.
valid concern. i reproduced this locally. if a task times out and the actor dies afterward, the next task can be the one that sees the actor failure and gets lost.
this already happens on master. fixing it here would change the timeout behavior and make this pr larger than #50313 needs.
i think timed-out tasks need separate tracking so the actor is only reused after the original task finishes. if that sounds right, i can open a separate issue and pick it up in a follow-up pr.
There was a problem hiding this comment.
if task times out we should probably call ray.cancel() instead of just ignoring the task and raising a timeout exception. but looks like ray.cancel() does not guarantee cancellation so we will need to go with the approach you are suggesting or introduce a new concept like task retries so that a failed task can be retried again on another actor if it failed due to actor death
There was a problem hiding this comment.
lets add a todo for this scenario and take up fixing it in another pr
There was a problem hiding this comment.
Ill merge this pr once u add this
There was a problem hiding this comment.
added in a219c27 for both get_next and get_next_unordered. the TODO calls out that ignored tasks need to stay tracked and the actor should not be returned until the task finishes.
Use one path for ordered and unordered result retrieval so actor liveness handling stays consistent. Signed-off-by: Divyanshu <bautocrats@gmail.com>
Only evict actors after a confirmed death so temporarily unavailable actors can recover and remain usable. Signed-off-by: Divyanshu <bautocrats@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 77cca7f. Configure here.
Document the delayed actor failure case for both ordered and unordered timeout paths. Signed-off-by: Divyanshu <bautocrats@gmail.com>
4 participants
Description
ActorPool.get_next/get_next_unorderedreturned the actor to the idle pool beforeray.get()surfaced the actor's death. Becausesubmit()pops from the end of the idle list, a dead actor was handed straight back out and every subsequent task on it failed, so a single bad actor could wedge the whole pool with no forward progress.This recycles the actor only when it is still alive. On
RayActorErrorthe actor is dropped from the pool, while queued submits are still drained onto the remaining healthy actors. A normal task exception (RayTaskError) leaves the actor in the pool as before, since the actor itself is still healthy.Related issues
Closes #50313
Additional information
Added
test_dead_actor_is_evicted, which reproduces the issue (one actor that exits on its first task): it fails onmaster(the pool gets stuck and returns no results) and passes with this change. The existingtest_actor_pool.pysuite continues to pass.