[core] fix detached actor being unexpectedly killed #53562

rueian · 2025-06-04T20:39:58Z

By replacing the inaccurate worker->IsDetachedActor() with worker->GetAssignedTask().GetTaskSpecification().IsDetachedActor().

Why are these changes needed?

In the previous PR #14184, the worker.MarkDetachedActor() that happened on assigning a task to a worker was deleted.

And that causes a leased worker for a detached worker can be killed by HandleUnexpectedWorkerFailure, as mentioned in #40864, which is also even triggered by a normal exit of driver. The reproducible scripts can be found in the comment.

I think actually Worker::IsDetachedActor and Worker::MarkDetachedActor are redundant and better be removed because we can access the info of whether the worker is detached or not through its assigned task.

The info is first ready after worker->SetAssignedTask(task)(L962) during LocalTaskManager::Dispatch and then the worker is inserted into the leased_workers map (L972).

ray/src/ray/raylet/local_task_manager.cc

Lines 962 to 972 in 118c370

    
           worker->SetAssignedTask(task); 
        
           // Pass the contact info of the worker to use. 
        
           reply->set_worker_pid(worker->GetProcess().GetId()); 
        
           reply->mutable_worker_address()->set_ip_address(worker->IpAddress()); 
        
           reply->mutable_worker_address()->set_port(worker->Port()); 
        
           reply->mutable_worker_address()->set_worker_id(worker->WorkerId().Binary()); 
        
           reply->mutable_worker_address()->set_raylet_id(self_node_id_.Binary()); 
        
           RAY_CHECK(leased_workers.find(worker->WorkerId()) == leased_workers.end()); 
        
           leased_workers[worker->WorkerId()] = worker;

Therefore, we can access the info through worker->GetAssignedTask().GetTaskSpecification().IsDetachedActor() safely while looping over the leased_workers_ in the NodeManager. By doing that, we don't need to worry about we could miss worker.MarkDetachedActor() sometimes.

Related issue number

Closes #40864
Related to ray-project/kuberay#3701 and ray-project/kuberay#3700

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
- I've added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

By replacing the inaccurate `worker->IsDetachedActor()` with `worker->GetAssignedTask().GetTaskSpecification().IsDetachedActor()`. Signed-off-by: Rueian <[email protected]>

Test the new behaivor with a private static function so that we don't need to create a node manager in tests. Signed-off-by: Rueian <[email protected]>

kevin85421 · 2025-06-06T05:04:52Z

open an issue to track the progress: ray-project/kuberay#3700

rueian · 2025-06-06T17:03:44Z

src/ray/raylet/node_manager.cc

+  KillWorkersOwnedByNodeID(
+      leased_workers_,
+      [this](const std::shared_ptr<WorkerInterface> &worker) { KillWorker(worker); },
+      node_id);


This PR essentially replaces worker->IsDetachedActor() with worker->GetAssignedTask().GetTaskSpecification().IsDetachedActor().

To make the changes be tested, I followed the practice in TestHandleReportWorkerBacklog to extract the loop on the leased_workers_ to static methods KillWorkersOwnedByNodeID and KillWorkersOwnedByWorkerID for unit testing.

Should we fully remove worker.IsDetachedActor() and/or replace its implementation with what you have here? Looks like it's likely to cause similar bugs in the future.

Sure. Worker::IsDetachedActor will no longer be used after this PR. I was originally planning to remove it in a follow-up PR, but now the removal is included here.

rueian · 2025-06-06T17:09:59Z

open an issue to track the progress: ray-project/kuberay#3700

@kevin85421 Done. This PR is also ready for review. Please take a look. Thanks!

…Actor Signed-off-by: Rueian <[email protected]>

edoakes · 2025-06-09T14:11:14Z

src/ray/raylet/node_manager.h

+  /// This is created for unit test purpose so that we don't need to create
+  /// a node manager in order to test KillWorkersOwnedByNodeID.
+  static void KillWorkersOwnedByNodeID(
+      const absl::flat_hash_map<WorkerID, std::shared_ptr<WorkerInterface>>
+          &leased_workers,
+      const std::function<void(const std::shared_ptr<WorkerInterface> &)> &kill_worker,
+      const NodeID &node_id);


This is an anti-pattern, we should be writing tests against the public interface of the relevant class (in this case, NodeManager.

Is it possible to rewrite it in that way instead?

+1. I think we're going to have to setup a NodeManager with the right workers and then call the public API.

edoakes · 2025-06-09T14:11:59Z

@israbbani can you help review this PR please

israbbani

Excellent find and root cause. I left a few comments.

israbbani · 2025-06-09T16:54:35Z

src/ray/raylet/test/util.h

-  void MarkDetachedActor() override { is_detached_actor_ = true; }
-  bool IsDetachedActor() const override { return is_detached_actor_; }


The code would be a simpler if you deleted MarkDetachedActor() and changed the implementation of IsDetachedActor to do the right thing i.e.

bool IsDetachedActor() const override { return assigned_task_.GetTaskSpecification().IsDetachedActor(); }

That way the rest of the code will work as is.

When calling a chain of functions like that, it's also worth making sure that each function in the chain returns a valid object i.e. GetTaskSpecification() always returns a properly constructed TaskSpec object.

When calling a chain of functions like that, it's also worth making sure that each function in the chain returns a valid object i.e. GetTaskSpecification() always returns a properly constructed TaskSpec object.

Hi @israbbani, are you suggesting that I add a check in the GetTaskSpecification() method to ensure that the underlying specification is properly constructed, or make the method fail if it’s not?

Hi @rueian. I looked at the code and in this case RayTask and TaskSpec both have default no-args constructors so we should be okay to just write

bool IsDetachedActor() const override { return assigned_task_.GetTaskSpecification().IsDetachedActor(); }

israbbani · 2025-06-09T18:40:24Z

src/ray/raylet/node_manager.h

+  /// This is created for unit test purpose so that we don't need to create
+  /// a node manager in order to test KillWorkersOwnedByNodeID.
+  static void KillWorkersOwnedByNodeID(
+      const absl::flat_hash_map<WorkerID, std::shared_ptr<WorkerInterface>>
+          &leased_workers,
+      const std::function<void(const std::shared_ptr<WorkerInterface> &)> &kill_worker,
+      const NodeID &node_id);


+1. I think we're going to have to setup a NodeManager with the right workers and then call the public API.

rueian added 2 commits June 4, 2025 13:39

[core] fix detached actor being unexpectedly killed

af8f2ee

By replacing the inaccurate `worker->IsDetachedActor()` with `worker->GetAssignedTask().GetTaskSpecification().IsDetachedActor()`. Signed-off-by: Rueian <[email protected]>

[core] fix detached actor being unexpectedly killed

3f91689

Test the new behaivor with a private static function so that we don't need to create a node manager in tests. Signed-off-by: Rueian <[email protected]>

rueian force-pushed the fix-detached-actor-killed-on-owner-exit branch from 8fc1fa5 to 21ffca5 Compare June 6, 2025 03:22

rueian force-pushed the fix-detached-actor-killed-on-owner-exit branch from 21ffca5 to 6b3205a Compare June 6, 2025 05:29

Merge branch 'master' into fix-detached-actor-killed-on-owner-exit

d6e49c3

rueian force-pushed the fix-detached-actor-killed-on-owner-exit branch from 6b3205a to d6e49c3 Compare June 6, 2025 05:41

rueian commented Jun 6, 2025

View reviewed changes

rueian marked this pull request as ready for review June 6, 2025 17:04

rueian mentioned this pull request Jun 6, 2025

[Umbrella] Remove the max_restart in autoscaler e2e tests ray-project/kuberay#3752

Open

edoakes requested a review from a team June 6, 2025 22:07

edoakes self-assigned this Jun 6, 2025

[core] remove unused Worker::IsDetachedActor and Worker::MarkDetached…

3ed4f15

…Actor Signed-off-by: Rueian <[email protected]>

rueian force-pushed the fix-detached-actor-killed-on-owner-exit branch from 06aecc7 to 3ed4f15 Compare June 6, 2025 23:10

edoakes reviewed Jun 9, 2025

View reviewed changes

edoakes assigned israbbani Jun 9, 2025

israbbani reviewed Jun 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[core] fix detached actor being unexpectedly killed #53562

[core] fix detached actor being unexpectedly killed #53562

Uh oh!

rueian commented Jun 4, 2025 •

edited

Loading

Uh oh!

kevin85421 commented Jun 6, 2025

Uh oh!

rueian Jun 6, 2025 •

edited

Loading

Uh oh!

edoakes Jun 6, 2025

Uh oh!

rueian Jun 7, 2025

Uh oh!

rueian commented Jun 6, 2025

Uh oh!

edoakes Jun 9, 2025

Uh oh!

israbbani Jun 9, 2025

Uh oh!

edoakes commented Jun 9, 2025

Uh oh!

israbbani left a comment

Uh oh!

israbbani Jun 9, 2025

Uh oh!

israbbani Jun 9, 2025

Uh oh!

rueian Jun 9, 2025 •

edited

Loading

Uh oh!

israbbani Jun 9, 2025

Uh oh!

israbbani Jun 9, 2025

Uh oh!

Uh oh!

	worker->SetAssignedTask(task);

	// Pass the contact info of the worker to use.
	reply->set_worker_pid(worker->GetProcess().GetId());
	reply->mutable_worker_address()->set_ip_address(worker->IpAddress());
	reply->mutable_worker_address()->set_port(worker->Port());
	reply->mutable_worker_address()->set_worker_id(worker->WorkerId().Binary());
	reply->mutable_worker_address()->set_raylet_id(self_node_id_.Binary());

	RAY_CHECK(leased_workers.find(worker->WorkerId()) == leased_workers.end());
	leased_workers[worker->WorkerId()] = worker;

		void MarkDetachedActor() override { is_detached_actor_ = true; }
		bool IsDetachedActor() const override { return is_detached_actor_; }

[core] fix detached actor being unexpectedly killed #53562

Are you sure you want to change the base?

[core] fix detached actor being unexpectedly killed #53562

Uh oh!

Conversation

rueian commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

kevin85421 commented Jun 6, 2025

Uh oh!

rueian Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rueian commented Jun 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edoakes commented Jun 9, 2025

Uh oh!

israbbani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rueian Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rueian commented Jun 4, 2025 •

edited

Loading

rueian Jun 6, 2025 •

edited

Loading

rueian Jun 9, 2025 •

edited

Loading