[core] Make WaitForActorRefDeleted RPC Fault Tolerant by Sparks0219 · Pull Request #57116 · ray-project/ray

Sparks0219 · 2025-10-02T01:08:45Z

Why are these changes needed?

Making WaitForActorRefDeleted RPC Fault Tolerant. Added c++ unit tests to verify idempotency of HandleWaitForActorRefDeleted, and added a python integration test.

Related issue number

Closes #53797

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run pre-commit jobs to lint the changes in this PR. (pre-commit setup)
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 :(

Note

^{Cursor Bugbot is generating a summary for commit e3234ca. Configure here.}

Signed-off-by: joshlee <joshlee@anyscale.com>

gemini-code-assist

Code Review

This pull request makes the WaitForActorRefDeleted RPC fault-tolerant by enabling retries. This is a good improvement for the robustness of actor lifecycle management. The changes include updating the RPC client to be retryable, modifying the WaitForActorRefDeleted method signature to use move semantics for better performance, and adding comprehensive tests. The new C++ tests for idempotency on the server side are well-structured. I've suggested a small improvement to the new Python test to cover response failures in addition to request failures, and a minor style improvement in the C++ tests to mark unused lambda parameters for better readability. Overall, this is a solid contribution to Ray's fault tolerance.

gemini-code-assist · 2025-10-02T01:10:35Z

python/ray/tests/test_core_worker_fault_tolerance.py

+@pytest.mark.parametrize("deterministic_failure", ["request"])
+def test_wait_for_actor_ref_deleted_rpc_retry_and_idempotency(
+    monkeypatch, shutdown_only, deterministic_failure
+):
+    """Test that WaitForActorRefDeleted RPC retries work correctly.
+    Verify that the RPC is idempotent when network failures occur.
+    The GCS actor manager will trigger this RPC during actor initialization
+    to monitor when the actor handles have gone out of scope and the actor should be destroyed.
+    """
+
+    monkeypatch.setenv(
+        "RAY_testing_rpc_failure",
+        "CoreWorkerService.grpc_client.WaitForActorRefDeleted=1:100:0",
+    )


The test test_wait_for_actor_ref_deleted_rpc_retry_and_idempotency is only parameterized to test request failures. For better test coverage, it should also be parameterized to test response failures, similar to test_get_object_status_rpc_retry_and_idempotency.

Additionally, the deterministic_failure parameter is currently unused in the test body.

Suggested change

@pytest.mark.parametrize("deterministic_failure", ["request"])

def test_wait_for_actor_ref_deleted_rpc_retry_and_idempotency(

monkeypatch, shutdown_only, deterministic_failure

):

"""Test that WaitForActorRefDeleted RPC retries work correctly.

Verify that the RPC is idempotent when network failures occur.

The GCS actor manager will trigger this RPC during actor initialization

to monitor when the actor handles have gone out of scope and the actor should be destroyed.

"""

monkeypatch.setenv(

"RAY_testing_rpc_failure",

"CoreWorkerService.grpc_client.WaitForActorRefDeleted=1:100:0",

)

@pytest.mark.parametrize("deterministic_failure", ["request", "response"])

def test_wait_for_actor_ref_deleted_rpc_retry_and_idempotency(

monkeypatch, shutdown_only, deterministic_failure

):

"""Test that WaitForActorRefDeleted RPC retries work correctly.

Verify that the RPC is idempotent when network failures occur.

The GCS actor manager will trigger this RPC during actor initialization

to monitor when the actor handles have gone out of scope and the actor should be destroyed.

"""

monkeypatch.setenv(

"RAY_testing_rpc_failure",

"CoreWorkerService.grpc_client.WaitForActorRefDeleted=1:"

+ ("100:0" if deterministic_failure == "request" else "0:100"),

)

gemini-code-assist · 2025-10-02T01:10:35Z

src/ray/core_worker/tests/core_worker_test.cc

+      [&callback_count](Status s,
+                        std::function<void()> success,
+                        std::function<void()> failure) { callback_count++; });


The lambda parameters s, success, and failure are unused. It's good practice to either omit their names or mark them as unused to improve readability and signal intent.

Suggested change

[&callback_count](Status s,

std::function<void()> success,

std::function<void()> failure) { callback_count++; });

[&callback_count](Status /*s*/,

std::function<void()> /*success*/,

std::function<void()> /*failure*/) { callback_count++; });

gemini-code-assist · 2025-10-02T01:10:35Z

src/ray/core_worker/tests/core_worker_test.cc

+      [&callback_count](Status s,
+                        std::function<void()> success,
+                        std::function<void()> failure) { callback_count++; });


The lambda parameters s, success, and failure are unused. It's good practice to either omit their names or mark them as unused to improve readability and signal intent.

Suggested change

[&callback_count](Status s,

std::function<void()> success,

std::function<void()> failure) { callback_count++; });

[&callback_count](Status /*s*/,

std::function<void()> /*success*/,

std::function<void()> /*failure*/) { callback_count++; });

src/ray/gcs/gcs_actor_manager.cc

dayshah · 2025-10-02T01:44:46Z

src/ray/core_worker/tests/core_worker_test.cc

+  rpc::WaitForActorRefDeletedReply reply2;
+
+  // For requests containing the same ActorID, the send_reply_callback is overwritten
+  // and only the last callback is triggered.


should assert false in the first callback and assert true in the second callback

Put an ASSERT_TRUE(false) in the first callback since I wanted to guarantee it wouldn't be called.

src/ray/core_worker/tests/core_worker_test.cc

Signed-off-by: joshlee <joshlee@anyscale.com>

src/ray/gcs/gcs_actor_manager.cc

Signed-off-by: joshlee <joshlee@anyscale.com>

…-for-actor-ref-deleted-fault-tolerant

Signed-off-by: joshlee <joshlee@anyscale.com>

jjyao · 2025-10-06T21:24:46Z

python/ray/tests/test_core_worker_fault_tolerance.py

    assert final_result == [0, 2, 4, 6, 8]


+@pytest.mark.parametrize("deterministic_failure", ["request"])


why not also response?

I think it's a bit tricky to test response because there's another RPC that the owner uses to notify the GCS that the actor goes out of scope: AsyncReportActorOutOfScope so even if the response is lost the GCS will still deallocate the actor.

Request is easy because if it's lost the callback immediately calls DestroyActor so we get an "actor creation cancelled" error

can you put a comment there so the person after knows

python/ray/tests/test_core_worker_fault_tolerance.py

jjyao · 2025-10-06T21:29:09Z

python/ray/tests/test_core_worker_fault_tolerance.py

+
+    result = ray.get(actor.ping.remote())
+    assert result == "pong"
+


we should do

del actor // check to make sure actor is marked as dead with cause equals REF_DELETED

AsyncReportActorOutOfScope will report this, note the prior comment

I mean we should add some checks to make sure actor is marked as dead with cause equals REF_DELETED

src/ray/core_worker/tests/core_worker_test.cc

Signed-off-by: joshlee <joshlee@anyscale.com>

jjyao

LGTM

jjyao · 2025-10-07T23:31:44Z

src/ray/core_worker/reference_counter.cc

    return false;
  }
-  it->second.on_object_ref_delete = callback;
+  it->second.object_ref_deleted_callbacks.push_back(callback);


Suggested change

it->second.object_ref_deleted_callbacks.push_back(callback);

it->second.object_ref_deleted_callbacks.push_back(std::move(callback));

Makes sense, done

jjyao · 2025-10-07T23:32:04Z

src/ray/core_worker/reference_counter.h

  /// Returns true if the object was in the reference table and the callback was added
  /// else false.
-  bool SetObjectRefDeletedCallback(const ObjectID &object_id,
+  bool AddObjectRefDeletedCallback(const ObjectID &object_id,


Update docstring

Ah shoot, thanks for catching

src/ray/core_worker/reference_counter.h

…-for-actor-ref-deleted-fault-tolerant

Signed-off-by: joshlee <joshlee@anyscale.com>

cursor · 2025-10-08T00:22:31Z

src/ray/gcs/gcs_actor_manager.cc

-      wait_request,
+  auto client = worker_client_pool_.GetOrConnect(it->second.address_);
+  client->WaitForActorRefDeleted(
+      std::move(wait_request),


Bug: RPC Client Pooling Breaks Retry Mechanism

The WaitForActorRefDeleted RPC now fetches a new client from the pool for each call. This breaks the retryable RPC mechanism, which relies on the same client instance to maintain state across retries.

…-for-actor-ref-deleted-fault-tolerant

cursor

Bug: Missing `const` Qualifier in Callback Parameter

The AddObjectRefDeletedCallback override in ReferenceCounter has a callback parameter that's missing the const qualifier, which differs from the virtual method in ReferenceCounterInterface. This signature mismatch prevents proper overriding and will cause compilation errors.

src/ray/core_worker/reference_counter.h#L48-L51

ray/src/ray/core_worker/reference_counter.h

Lines 48 to 51 in ad1cdd2

    
                                          bool foreign_owner_already_monitoring = false) = 0; 
        
           virtual void AddOwnedObject( 
        
               const ObjectID &object_id, 
        
               const std::vector<ObjectID> &contained_ids,

cursor · 2025-10-08T01:13:57Z

src/ray/gcs/gcs_actor_manager.cc

-      wait_request,
+  auto client = worker_client_pool_.GetOrConnect(it->second.address_);
+  client->WaitForActorRefDeleted(
+      std::move(wait_request),


Bug: Moved Object Access Causes Undefined Behavior

Moving wait_request invalidates the object. While the lambda doesn't use it, accessing a moved-from object can lead to undefined behavior.

omg cursor hates the moves

yea and the client pool too, chill AI

Signed-off-by: joshlee <joshlee@anyscale.com>

cursor · 2025-10-08T05:38:11Z

src/ray/gcs/gcs_actor_manager.cc

-      wait_request,
+  auto client = worker_client_pool_.GetOrConnect(it->second.address_);
+  client->WaitForActorRefDeleted(
+      std::move(wait_request),


Bug: RPC Retry Fails Due to Empty Request

The wait_request is moved into the RPC call, making it empty for any retry attempts. This, combined with fetching a new client from the pool for each call, can lead to failed or inconsistent RPC retries.

Signed-off-by: joshlee <joshlee@anyscale.com>

cursor · 2025-10-08T05:46:53Z

src/ray/core_worker/core_worker.cc


+  /// The callback for each request is stored in the reference counter due to retries
+  /// and message reordering where the callback of the retry of the request could be
+  /// overwritten by the callback of the initial request.


Bug: Double Move of respond Causes Undefined Behavior

The respond callback is moved into the AsyncWaitForActorRegisterFinish lambda capture. If actor registration succeeds, the respond object, already in a moved-from state, is moved a second time into WaitForActorRefDeleted, causing undefined behavior.

it got the problem wrong, the problem is actually just that the second move doesn't really move because function captures are const by default

dayshah

also update the Failure line in the .proto file, not sure if we've missed that for more rpc's...

just a nit

dayshah · 2025-10-09T07:25:26Z

src/ray/core_worker/reference_counter.cc

  }
-  if (it->second.on_object_ref_delete) {
-    it->second.on_object_ref_delete(it->first);
+  if (it->second.object_ref_deleted_callbacks.size() > 0) {


why do you need this if, it's the same without it

oh bleh good catch thanks

dayshah · 2025-10-09T07:27:03Z

src/ray/gcs/gcs_actor_manager.cc

-      wait_request,
+  auto client = worker_client_pool_.GetOrConnect(it->second.address_);
+  client->WaitForActorRefDeleted(
+      std::move(wait_request),


omg cursor hates the moves

Sparks0219 · 2025-10-09T18:05:10Z

also update the Failure line in the .proto file, not sure if we've missed that for more rpc's...

UHHH yea it's a little outdated lol, I went through the list on the doc and updated all the missing ones

Signed-off-by: joshlee <joshlee@anyscale.com>

) Signed-off-by: joshlee <joshlee@anyscale.com> Signed-off-by: Pavitra Bhalla <pavitra@rayai.com>

) Signed-off-by: joshlee <joshlee@anyscale.com> Signed-off-by: Josh Kodi <joshkodi@gmail.com>

) Signed-off-by: joshlee <joshlee@anyscale.com>

Signed-off-by: joshlee <joshlee@anyscale.com>

) Signed-off-by: joshlee <joshlee@anyscale.com>

) Signed-off-by: joshlee <joshlee@anyscale.com> Signed-off-by: xgui <xgui@anyscale.com>

Signed-off-by: joshlee <joshlee@anyscale.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>

) Signed-off-by: joshlee <joshlee@anyscale.com>

) Signed-off-by: joshlee <joshlee@anyscale.com> Signed-off-by: Aydin Abiar <aydin@anyscale.com>

) Signed-off-by: joshlee <joshlee@anyscale.com> Signed-off-by: Future-Outlier <eric901201@gmail.com>

Make WaitForActorRefDeleted Fault Tolerant

e3234ca

Signed-off-by: joshlee <joshlee@anyscale.com>

Sparks0219 requested a review from a team as a code owner October 2, 2025 01:08

Sparks0219 added the go add ONLY when ready to merge, run all tests label Oct 2, 2025

Sparks0219 assigned dayshah and edoakes Oct 2, 2025

gemini-code-assist bot reviewed Oct 2, 2025

View reviewed changes

ray-gardener bot added the core Issues that should be addressed in Ray Core label Oct 2, 2025

dayshah reviewed Oct 2, 2025

View reviewed changes

Addressing comments

7d69b9b

Signed-off-by: joshlee <joshlee@anyscale.com>

This comment was marked as outdated.

Sign in to view

Addressing comments

bb26a08

Signed-off-by: joshlee <joshlee@anyscale.com>

Sparks0219 requested a review from dayshah October 2, 2025 05:52

Addressing comments

a0d8406

Signed-off-by: joshlee <joshlee@anyscale.com>

This comment was marked as outdated.

Sign in to view

Fix failing cpp test

eb7e783

Signed-off-by: joshlee <joshlee@anyscale.com>

jjyao reviewed Oct 3, 2025

View reviewed changes

src/ray/gcs/gcs_actor_manager.cc Outdated Show resolved Hide resolved

dayshah reviewed Oct 3, 2025

View reviewed changes

src/ray/gcs/gcs_actor_manager.cc Outdated Show resolved Hide resolved

Sparks0219 added 4 commits October 3, 2025 22:24

Addressing comments

9290d83

Signed-off-by: joshlee <joshlee@anyscale.com>

Merge conflicts

7164fd2

Signed-off-by: joshlee <joshlee@anyscale.com>

Merge remote-tracking branch 'upstream/master' into joshlee/make-wait…

7b379f3

…-for-actor-ref-deleted-fault-tolerant

Fix failing cpp tests

debf89d

Signed-off-by: joshlee <joshlee@anyscale.com>

This comment was marked as outdated.

Sign in to view

Merge conflicts

2cf110f

Signed-off-by: joshlee <joshlee@anyscale.com>

Sparks0219 requested review from dayshah, edoakes and jjyao October 6, 2025 20:43

Sparks0219 mentioned this pull request Oct 6, 2025

[core] Clean up worker/raylet client pools on node death #56613

Merged

8 tasks

jjyao reviewed Oct 6, 2025

View reviewed changes

Addressing comments

ad82b67

Signed-off-by: joshlee <joshlee@anyscale.com>

This comment was marked as outdated.

Sign in to view

jjyao reviewed Oct 7, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/master' into joshlee/make-wait…

7d3fef2

…-for-actor-ref-deleted-fault-tolerant

Sparks0219 mentioned this pull request Oct 8, 2025

[core] Make Unsubscribe Idempotent #57234

Merged

8 tasks

Addressing comments

2c41679

Signed-off-by: joshlee <joshlee@anyscale.com>

cursor bot reviewed Oct 8, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/master' into joshlee/make-wait…

ad1cdd2

…-for-actor-ref-deleted-fault-tolerant

cursor bot reviewed Oct 8, 2025

View reviewed changes

jjyao approved these changes Oct 8, 2025

View reviewed changes

AI comment

6eb8875

Signed-off-by: joshlee <joshlee@anyscale.com>

cursor bot reviewed Oct 8, 2025

View reviewed changes

Merge conflicts

a1d3259

Signed-off-by: joshlee <joshlee@anyscale.com>

cursor bot reviewed Oct 8, 2025

View reviewed changes

dayshah reviewed Oct 9, 2025

View reviewed changes

dayshah approved these changes Oct 9, 2025

View reviewed changes

Addressing comments

a6ec920

Signed-off-by: joshlee <joshlee@anyscale.com>

Sparks0219 requested review from dayshah and jjyao October 9, 2025 18:41

jjyao enabled auto-merge (squash) October 9, 2025 19:38

jjyao merged commit cef177f into ray-project:master Oct 9, 2025
7 checks passed

pavitrabhalla pushed a commit to superserve-ai/ray that referenced this pull request Oct 10, 2025

[core] Make WaitForActorRefDeleted RPC Fault Tolerant (ray-project#57116

55f88f0

) Signed-off-by: joshlee <joshlee@anyscale.com> Signed-off-by: Pavitra Bhalla <pavitra@rayai.com>

joshkodi pushed a commit to joshkodi/ray that referenced this pull request Oct 13, 2025

[core] Make WaitForActorRefDeleted RPC Fault Tolerant (ray-project#57116

2d0b7c4

) Signed-off-by: joshlee <joshlee@anyscale.com> Signed-off-by: Josh Kodi <joshkodi@gmail.com>

ArturNiederfahrenhorst pushed a commit to ArturNiederfahrenhorst/ray that referenced this pull request Oct 13, 2025

[core] Make WaitForActorRefDeleted RPC Fault Tolerant (ray-project#57116

08d76e8

) Signed-off-by: joshlee <joshlee@anyscale.com>

harshit-anyscale pushed a commit that referenced this pull request Oct 15, 2025

[core] Make WaitForActorRefDeleted RPC Fault Tolerant (#57116)

1b9f5e3

Signed-off-by: joshlee <joshlee@anyscale.com>

justinyeh1995 pushed a commit to justinyeh1995/ray that referenced this pull request Oct 20, 2025

[core] Make WaitForActorRefDeleted RPC Fault Tolerant (ray-project#57116

373e083

) Signed-off-by: joshlee <joshlee@anyscale.com>

xinyuangui2 pushed a commit to xinyuangui2/ray that referenced this pull request Oct 22, 2025

[core] Make WaitForActorRefDeleted RPC Fault Tolerant (ray-project#57116

b13a8c8

) Signed-off-by: joshlee <joshlee@anyscale.com> Signed-off-by: xgui <xgui@anyscale.com>

elliot-barn pushed a commit that referenced this pull request Oct 23, 2025

[core] Make WaitForActorRefDeleted RPC Fault Tolerant (#57116)

81d42f0

Signed-off-by: joshlee <joshlee@anyscale.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>

landscapepainter pushed a commit to landscapepainter/ray that referenced this pull request Nov 17, 2025

[core] Make WaitForActorRefDeleted RPC Fault Tolerant (ray-project#57116

7961917

) Signed-off-by: joshlee <joshlee@anyscale.com>

Aydin-ab pushed a commit to Aydin-ab/ray-aydin that referenced this pull request Nov 19, 2025

[core] Make WaitForActorRefDeleted RPC Fault Tolerant (ray-project#57116

cbd0e63

) Signed-off-by: joshlee <joshlee@anyscale.com> Signed-off-by: Aydin Abiar <aydin@anyscale.com>

Future-Outlier pushed a commit to Future-Outlier/ray that referenced this pull request Dec 7, 2025

[core] Make WaitForActorRefDeleted RPC Fault Tolerant (ray-project#57116

4ccc937

) Signed-off-by: joshlee <joshlee@anyscale.com> Signed-off-by: Future-Outlier <eric901201@gmail.com>

		assert final_result == [0, 2, 4, 6, 8]


		@pytest.mark.parametrize("deterministic_failure", ["request"])


		result = ray.get(actor.ping.remote())
		assert result == "pong"

	it->second.object_ref_deleted_callbacks.push_back(callback);
	it->second.object_ref_deleted_callbacks.push_back(std::move(callback));

	bool foreign_owner_already_monitoring = false) = 0;
	virtual void AddOwnedObject(
	const ObjectID &object_id,
	const std::vector<ObjectID> &contained_ids,

Conversation

Sparks0219 commented Oct 2, 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!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sparks0219 Oct 7, 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!

Uh oh!

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!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

jjyao 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cursor bot Oct 8, 2025

Choose a reason for hiding this comment

Bug: RPC Client Pooling Breaks Retry Mechanism

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

Bug: Missing `const` Qualifier in Callback Parameter

Uh oh!

cursor bot Oct 8, 2025

Choose a reason for hiding this comment

Bug: Moved Object Access Causes Undefined Behavior

Uh oh!

Sparks0219 commented Oct 2, 2025 •

edited

Loading

Sparks0219 Oct 7, 2025 •

edited

Loading

Bug: Double Move of `respond` Causes Undefined Behavior