Repository navigation
[serve] Push-based replica health: replica self-checks and heartbeats - #66421
johntaylor-cell wants to merge 17 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a replica-pushed self-health mechanism to Ray Serve, allowing replicas to periodically run their own health checks and push the results to the controller, thereby reducing active pull-probing overhead. It also adds a mechanism to detect controller ingest lag and avoid incorrectly marking replicas as unhealthy due to probe timeouts when the controller is falling behind. Feedback on the changes highlights a potential ZeroDivisionError in expected_push_rate if the metrics interval or health check period is configured to zero, which should be guarded against.
There was a problem hiding this comment.
Code Review
This pull request introduces a push-based health check mechanism for Ray Serve replicas, allowing replicas to periodically run their own health checks and push the results to the controller. This mechanism includes a fallback to pull probes when pushed results go stale, and an ingest lag gate to suppress probe timeouts when the controller is lagging. Feedback on these changes highlights a potential memory leak in ReplicaHealthPushRegistry where dead replica entries are never pruned if the total count remains below _PRUNE_THRESHOLD (65,536), and suggests triggering pruning periodically regardless of the size threshold.
9c0c77b to
0ea2058
Compare
0ea2058 to
deb1a94
Compare
0a393f2 to
461ec5b
Compare
8f8e1b1 to
fdeced0
Compare
The controller stopped trusting a push after three quarters of the health-check period while the replica kept serving its cached verdict for a full one, so the first fallback probe was always answered from cache. That restarts the probe clock, so a hung user check went unnoticed for a further period and push detected a hang later than pull probing alone would have. The window moves to common.py, which both ends already import, and the replica holds to the same figure: once the controller has stopped trusting the push, the probe it sends runs a real check. Reported by Cursor Bugbot on ray-project#66421. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
The controller stopped trusting a push after three quarters of the health-check period while the replica kept serving its cached verdict for a full one, so the first fallback probe was always answered from cache. That restarts the probe clock, so a hung user check went unnoticed for a further period and push detected a hang later than pull probing alone would have. The window moves to common.py, which both ends already import, and the replica holds to the same figure: once the controller has stopped trusting the push, the probe it sends runs a real check. Reported by Cursor Bugbot on ray-project#66421. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
aa7e614 to
cc83902
Compare
The controller stopped trusting a push after three quarters of the health-check period while the replica kept serving its cached verdict for a full one, so the first fallback probe was always answered from cache. That restarts the probe clock, so a hung user check went unnoticed for a further period and push detected a hang later than pull probing alone would have. The window moves to common.py, which both ends already import, and the replica holds to the same figure: once the controller has stopped trusting the push, the probe it sends runs a real check. Reported by Cursor Bugbot on ray-project#66421. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
expected_push_rate counted every RUNNING replica, but the self-health pusher is Python replica code and a Java deployment's replicas are Java actors that never call record_replica_health. Those replicas raised what the fleet owed without ever contributing an arrival, so a Java deployment read as controller lag and had its probe timeouts suppressed, on replicas that only ever answer probes. Gated on deployment_language rather than is_cross_language: the two are easy to confuse, as the TODO on ActorReplicaWrapper._is_cross_language notes, and the language is what decides whether the replica runs the pusher at all. Reported by Cursor Bugbot on ray-project#66421. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
expected_push_rate counted every RUNNING replica, but the self-health pusher is Python replica code and a Java deployment's replicas are Java actors that never call record_replica_health. Those replicas raised what the fleet owed without ever contributing an arrival, so a Java deployment read as controller lag and had its probe timeouts suppressed, on replicas that only ever answer probes. Gated on deployment_language rather than is_cross_language: the two are easy to confuse, as the TODO on ActorReplicaWrapper._is_cross_language notes, and the language is what decides whether the replica runs the pusher at all. Reported by Cursor Bugbot on ray-project#66421. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
Dropping the stashed push whenever a probe resolved also dropped it when the probe had merely timed out. A timeout heard nothing back, so a heartbeat that landed while it waited was the only real word from the replica: losing it charged a strike against a replica that had reported in, and with nothing left in hand a new probe could start at once. At threshold minus one that strike replaced a replica that had just pushed healthy. The tracker could not tell a timeout from a probe that came back and failed, because the wrapper passed it only the response. It now takes the ProbeOutcome. A timed-out probe started before a stashed push is superseded and the push applies on the next tick; with no push while it waited, the timeout still counts. A probe that came back still drops the stash, as before. Reported by Cursor Bugbot on ray-project#66421. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.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. |
|
for 8k replica can you generate the following data for push vs pull scenario
i recognize that this PR is a set function change before we get to node local aggregation, but we need to demonstrate it does not degrade from the current baseline. Or is the plan to only turn this feature on after node local aggregation is implemented? |
Yes, I do not plan to enable anything until the complete path has been established, including per-node aggregation. The last PR enables the flag and will include final perf results for the controller benchmark. Enabling before aggregation will degrade 8K performance because there will be more messages and a higher chance to overload the controller's inbox. This PR does contain a couple of replica changes that are "live" but are essentially no-ops until later PRs are merged. |
Replicas self-check twice per health-check period and heartbeat the result, so the controller pull-probes only when a pushed result has gone stale. The replica owns the failure count, advancing it once per period rather than once per eval, and the controller mirrors it instead of recomputing. A controller that is behind on ingest looks exactly like a fleet that has gone quiet, so a timed-out probe scores as no information while observed arrivals fall below half of what the running replicas owe. The suppression is bounded, so a replica that has genuinely hung still resolves. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
…erdict The rate window was six attributes and two methods that had to be called in a fixed order once per tick. It is now one method that takes the expectation it judges against, so the caller cannot read a verdict without publishing first, and three attributes instead of six. A window close also reseeds the floor, which nothing pinned: carrying the old minimum forward let one quiet window hold the floor down for good and silently disarm the gate. The probe timeout travels back with its verdict as a ProbeOutcome rather than sitting on the wrapper between calls, where it could be paired with a verdict from the push path. On the replica, the self-health verdict is no longer stored twice: the pushed value was mirrored into an attribute nothing read, and its timestamp was an attribute read only by the next statement. The evaluated flag is folded into the timestamp it duplicated. Remaining names say their units, with _at for an instant and _s for a duration. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
The lag-gate tests were reaching into the wrapper suite as TestPushedHealth._wrapper(TestPushedHealth()), which broke when that class was renamed and was the borrowing idiom the review objected to anyway. The builder never used self, so it moves to module scope and both suites call it. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
The controller stopped trusting a push after three quarters of the health-check period while the replica kept serving its cached verdict for a full one, so the first fallback probe was always answered from cache. That restarts the probe clock, so a hung user check went unnoticed for a further period and push detected a hang later than pull probing alone would have. The window moves to common.py, which both ends already import, and the replica holds to the same figure: once the controller has stopped trusting the push, the probe it sends runs a real check. Reported by Cursor Bugbot on ray-project#66421. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
…private Moving the freshness window into common.py inserted it between @publicapi(stability="alpha") and `class ReplicaID`, so the decorator landed on the new function and ReplicaID lost its annotation. The API consistency check caught it as a public API defined under _private with no documentation, which is the function, not ReplicaID. The window is private to begin with, so it takes the underscore back and sits above the decorator stack rather than inside it. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
P1 landed with a reviewed wrapper fixture and a required consecutive-failure count. The two wrapper builders collapse into the reviewed one, now module level so the lag-gate tests share it instead of keeping a second copy, and the call sites that pushed without a count carry one. Only the push tracker runs on the MockTimer. check_health reads the wall clock for its probe timeout, so the tests that fake a timed-out probe still set _probe_started_at against time.time(), and the helper's docstring says why. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
expected_push_rate counted every RUNNING replica, but the self-health pusher is Python replica code and a Java deployment's replicas are Java actors that never call record_replica_health. Those replicas raised what the fleet owed without ever contributing an arrival, so a Java deployment read as controller lag and had its probe timeouts suppressed, on replicas that only ever answer probes. Gated on deployment_language rather than is_cross_language: the two are easy to confuse, as the TODO on ActorReplicaWrapper._is_cross_language notes, and the language is what decides whether the replica runs the pusher at all. Reported by Cursor Bugbot on ray-project#66421. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
The self-health task is registered only after its callable is set, and the task is its only caller, so the None branch guarding it could never run. It becomes the invariant it always was, the way start_metrics_pusher already narrows an Optional. Three gate tests manufactured a probe result by patching ray.get and check_obj_ref_ready_nowait. They assert on the failure count and the suppression budget, not on the metrics fields, so they now stub _resolve_active_probe and hand the gate a ProbeOutcome directly. No test in this file patches Ray any more. _timed_out_probe deliberately keeps its one patch: those tests assert that the suppression branch clears the latency and failure flags the real resolve sets, so stubbing it would make them pass against nothing. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
…ring The self-check stopped running the user check once it reached the threshold, and nothing else reset the count, so the latch was permanent. The controller only health-checks RUNNING replicas, so a check that failed during a long reconfigure latched while UPDATING and got a recovered replica replaced on its first push. The check now keeps running past the threshold and a pass clears the count. A timeout counted past the suppression bound still went down the branch that refills the budget, so under sustained lag three of every four timeouts dropped and a hung replica took twelve to mark instead of six. Only a real verdict refills it now. A hung user check never reaches the pusher's stop event, so graceful_shutdown waited its full ten seconds on it during every replica shutdown. Shutdown now cancels the self-health task, which only observes, and an interrupted check is neither counted nor pushed. MetricsPusher gains a cancel_task for this. Pushes were ordered against probes by arrival, so a check that failed before a probe started could land after it and override that probe's success. A resolved probe now drops any push stashed before it; a newer state arrives with the next self-check. That leaves the arrival watermark unreachable, so it is removed. Smaller changes from the same review: the lag window runs on a monotonic clock passed in by the caller, which also means no test patches a global clock; expected_push_rate is named for its unit; the cache rule is a named helper; and the replica-side failure count and check time take the names the wire uses. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
Dropping the stashed push whenever a probe resolved also dropped it when the probe had merely timed out. A timeout heard nothing back, so a heartbeat that landed while it waited was the only real word from the replica: losing it charged a strike against a replica that had reported in, and with nothing left in hand a new probe could start at once. At threshold minus one that strike replaced a replica that had just pushed healthy. The tracker could not tell a timeout from a probe that came back and failed, because the wrapper passed it only the response. It now takes the ProbeOutcome. A timed-out probe started before a stashed push is superseded and the push applies on the next tick; with no push while it waited, the timeout still counts. A probe that came back still drops the stash, as before. Reported by Cursor Bugbot on ray-project#66421. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
The controller does not health-check an UPDATING replica, yet the self-check kept running through reconfigure(). A failure it pushed then stayed stashed and could be applied on the first tick after the replica was RUNNING again, replacing a replica that had already recovered. It could also flip the cached verdict behind /-/healthz mid-reconfigure, which pull probes never did. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
Pausing the self-check stopped such a check being counted or pushed, but it still wrote _healthy and its check time first. An unhealthy verdict does not expire, so the first probe after the replica was RUNNING again, and /-/healthz until the next self-check, still served a failure the reconfigure may have caused. With push health off the pause is ignored and probes cache as before. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
cancel_task removes a task's registration before cancelling it, and the self-health check swallows CancelledError so a cancelled user check cannot retire its heartbeat. The loop then reread the removed registration to arm its next sleep and died with a KeyError whenever shutdown interrupted a check. It now stops once its registration is gone. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
9e34355 to
aaffd60
Compare
…om before it Pausing caught only a check that ended while the reconfigure was still running. One that started before the pause and ended after the resume saw the flag clear at both ends, so it was counted, pushed and cached. Each pause now bumps an epoch, and a check that started in an earlier one is dropped. A reconfigure also left the cached verdict in place. An unhealthy one does not expire, so after a reconfigure longer than the push window the controller's first probe was answered with the failure from before it, a strike against a replica the reconfigure may have fixed. The reconfigure now clears it, so that probe runs the user check. Reported by Cursor Bugbot on ray-project#66421. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
The epoch moved only on pause, so a check that started while the pause was on, such as a probe that got the lock mid-reconfigure, saw the same epoch and a clear flag when it ended after the resume, and its verdict was cached. The epoch now moves on resume instead: a check that ends mid-reconfigure sees the pause flag, and any check still running at the resume sees the epoch change. Reported by Cursor Bugbot on ray-project#66421. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
…-check first A restarted controller calls initialize again on every replica it recovers, and its health check read the self-check's cached verdict. An unhealthy one never expires, so a replica that failed a check shortly before the restart but passes now failed recovery and was stopped. Initialize now runs the user check itself. Shutdown ran the user's __del__ before stopping the self-check, so check_health kept running alongside and after the destructor, on whatever it had freed. The self-check is now stopped first, paused before it is cancelled so the check it interrupts is not reported as a failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.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 5f34c27. Configure here.
… __del__ Cancelling the self-check reaches the user code loop only once the replica's loop runs again, so stopping it right before the destructor queued __del__ on that loop ahead of the cancel, and a check in flight there kept running alongside it. The self-check now stops where the controller stops health checking the replica, as graceful shutdown starts. The drain yields before __del__ is queued, so the check is cancelled first. Reported by Cursor Bugbot on ray-project#66421. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>

Stacked on #66243, which added the controller-side receive path.
The controller health-checks every replica by firing
check_health.remote()on atimer. At 8k replicas the submit alone is ~465 us and GIL-bound, and the sweep
accounts for essentially all of deployment-state update time.
Replicas now run their own check twice per health-check period and heartbeat the
result, so the pull probe fires only when a pushed result goes stale.
Behind
RAY_SERVE_ENABLE_PUSH_HEALTH, default off. The flag exists so the restof the stack can be reviewed without changing behavior; the last PR deletes it.
The in-flight guard is bounded. A push overdue past
RAY_SERVE_MAX_PUSH_IN_FLIGHT_S(20 s) is abandoned rather than waited on. Anunbounded guard lets one stuck receiver silence a replica permanently — measured
during development, one node's replicas dropped from ~420 to ~1.5 reports/s and
stayed there. The pre-existing metric push had the identical unguarded pattern, so
the same bound now covers it too.
Probe timeouts are not charged to a replica while the controller is behind. The
registry compares arrivals against the rate the fleet should be publishing; below
half of it, a timed-out probe scores as no information rather than a failure. It
deliberately declines to decide whose fault the silence is, because the only thing
it gates is whether to count a strike, and declining is safe either way. A returned
failure still counts, and a crashed actor still reports
ACTOR_CRASHED. With thefeature off the expected rate stays 0, so the gate is inert.
Testing. 24 new unit tests. Verified by mutation: each of ten independent
removals — the lag gate, the timeout flag, the lagging ratio, the rate window, the
expected-rate cadence, the stuck bound, once-per-period failure counting, the
threshold latch, the unhealthy bypass, and repeat-unhealthy suppression — is caught
by at least one test.
test_deployment_state.py294 passed andtest_metrics_utils.py108 passed, withtest_replica_backpressure,test_replica_quiesce,test_user_callable_wrapper,test_controller,test_application_stateandtest_deployment_rank_managerunchanged.