[Serve] Fix proxy state crash on ActorUnschedulableError during RayService incremental upgrade

[ryanaoleary]

Description

Fixes an issue where the Ray Serve controller loop crashes with an uncaught ActorUnschedulableError when attempting to shut down a proxy pinned to a dead or non-existant node. This PR wraps the shutdown ray.get() call in a try...except block, allowing the controller to gracefully clean up the dead proxy handle.

When does this bug occur?

This race condition occurs when a proxy actor is created on a node that dies while the creation is still pending. It can be seen during RayService incremental upgrades when GCS Fault Tolerance with Redis is enabled:

1. During an incremental upgrade a new RayCluster spins up to serve traffic in parallel with the old cluster and connects to Redis, restoring old proxy actors pinned to pre-upgrade node IDs.
2. Since the old node is dead/doesn't exist on the new cluster, proxy creation gets stuck in PENDING_CREATION.
3. The Serve controller detects the node ID is not in alive_node_ids and calls shutdown() on the pending proxy handle. ray.get() throws an ActorUnschedulableError, crashing the controller.

Related issues

This issue was brought up in discussion in the KubeRay slack channel.

Additional information

[gemini-code-assist]

Code Review

This pull request improves the robustness of the proxy actor shutdown and health check logic in proxy_state.py by handling ActorUnschedulableError when a node dies during creation, and wrapping shutdown and kill operations in try-except blocks. The review feedback suggests avoiding redundant ray.kill calls when the actor is already shut down, and using the imported ActorUnschedulableError directly instead of its fully qualified name.

[jeffreywang88]

great find! can we add some tests?

[abrarsheikh]

@ryanaoleary please rebase your PR after #64403 merged. your covers the kill case as well which is worth fixing.

[abrarsheikh]

connects to Redis, restoring old proxy actors pinned to pre-upgrade node IDs.

this is true? why should the new cluster restore its state from old clusters checkpoint? that sounds like a design flow in RayService incremental upgrade no?

[ryanaoleary]

connects to Redis, restoring old proxy actors pinned to pre-upgrade node IDs.

this is true? why should the new cluster restore its state from old clusters checkpoint? that sounds like a design flow in RayService incremental upgrade no?

My understanding is that since the new, upgraded RayCluster points to the same Redis as the old RayCluster (which is still serving traffic), it restores the old actor table and state from GCS on the new cluster as though it's recovering from a crash. When it restores the old cluster's Proxy Actors, it uses their original node IDs with a NodeAffinitySchedulingStrategy. This leaves the actors unschedulable, which then causes the aforementioned error when the controller tries to clean them up.

I think you're right that the main flaw is with KubeRay. It shouldn't load the old Proxy actors into the new cluster during an upgrade, and instead should be aware that it's a migration, not a recovery from a crash. I think the simplest way to achieve this would be for the upgraded cluster to not inherit the same Redis name/namespace and instead start from a fresh slate for the upgraded RayCluster. externalStorageNamespace should be set to some dynamically generated value based on the RayCluster. I can put out a change in KubeRay for the upcoming release with this fix and test coverage. cc: @win5923

I think this PR is still worth adding though to gracefully handle garbage collecting unschedulable actors without crashing the controller.

Edit: I think if users don't set ExternalStorageNamespace in the RayService spec this issue is avoided in Kuberay since that sets the namespace to the unique UID (https://github.com/ray-project/kuberay/blob/556d1b43dd273497cb1a7247431cd8a2bdcb42de/ray-operator/controllers/ray/common/pod.go#L119), which in Ray then is used to set

ray/src/ray/gcs/store_client/redis_store_client.h

Line 38 in 9cb0c45

const std::string external_storage_namespace;

[ryanaoleary]

Rebased on master and added test coverage

[ryanaoleary]

related KubeRay PR: ray-project/kuberay#5052

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit b61d0b6. Configure here.}

[ryanaoleary]

I made a couple changes in b61d0b6 besides removing the mocks from tests, these are:

1. Updated kill() to catch RayError instead of Exception - this addresses a race where is_shutdown() is still pending before unschedulability is detected
2. Moved the 5s graceful shutdown timeout to constants.py (PROXY_GRACEFUL_SHUTDOWN_TIMEOUT_S) and grouped RayActorError with ActorUnschedulableError at INFO level so expected node removals don't spam warnings