[serve] Push health: interleave health frames on open response streams - #64909
[serve] Push health: interleave health frames on open response streams#64909johntaylor-cell wants to merge 10 commits into
Conversation
The per-tick rank-consistency pass is O(N) with heavy constants; at 10K+ replicas it monopolizes the controller event loop in steady state (py-spy: it starved handle-report ingest for 20s+ at 16K). Rank consistency can only be violated by membership changes, so gate the pass on a fingerprint of the active replica set. 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 0fa54bd. Configure here.
There was a problem hiding this comment.
Code Review
This pull request implements a replica self-health push mechanism in Ray Serve to replace or defer pull-based probing, allowing replicas to piggyback health status on metric reports, response streams via ReplicaHealthFrame messages, or lightweight heartbeats. It also introduces a systemic stall guard to prevent probe storms during controller ingest lag, and optimizes controller performance by memoizing replica membership lookups and gating rank consistency checks. The review feedback identifies a critical deadlock risk in replica_result.py caused by holding a synchronous lock across await points, a potential performance issue in the health push registry pruning logic under high churn, and a bug where the push stall window remains uninitialized if target state info is missing.
| acquired = self._object_ref_or_gen_sync_lock.acquire(blocking=False) | ||
| if acquired: | ||
| try: | ||
| self._consumption_started = True | ||
| # Double-check under lock | ||
| if self._obj_ref is None: | ||
| self._obj_ref = await self._obj_ref_gen.__anext__() # type: ignore[union-attr] | ||
| while True: | ||
| obj_ref = await self._obj_ref_gen.__anext__() # type: ignore[union-attr] | ||
| if not self._health_frames_possible: | ||
| break | ||
| # Skip health-frame system messages to reach the | ||
| # result. | ||
| try: | ||
| value = await obj_ref | ||
| except Exception: | ||
| # The exception belongs to the request; | ||
| # consumers surface it when they fetch the ref. | ||
| break | ||
| if isinstance(value, ReplicaHealthFrame): | ||
| self._record_health_frame(value) | ||
| continue | ||
| break | ||
| self._obj_ref = obj_ref | ||
| return self._obj_ref | ||
| finally: | ||
| self._object_ref_or_gen_sync_lock.release() |
There was a problem hiding this comment.
Holding the synchronous threading.Lock (self._object_ref_or_gen_sync_lock) across await points (lines 417 and 423) is a critical anti-pattern. If to_object_ref_async is suspended at an await point while holding this lock, any synchronous call to to_object_ref on the same thread will attempt to acquire the lock and block. Since the thread is blocked, the event loop cannot resume to_object_ref_async to release the lock, resulting in a guaranteed deadlock. To resolve this, release the lock before performing any asynchronous operations, and use an asyncio.Future to coordinate concurrent asynchronous callers.
consumption_fut = getattr(self, "_async_consumption_future", None)
if consumption_fut is not None:
return await consumption_fut
acquired = self._object_ref_or_gen_sync_lock.acquire(blocking=False)
if acquired:
try:
if self._obj_ref is not None:
return self._obj_ref
self._consumption_started = True
self._async_consumption_future = asyncio.get_running_loop().create_future()
finally:
self._object_ref_or_gen_sync_lock.release()
try:
while True:
obj_ref = await self._obj_ref_gen.__anext__()
if not self._health_frames_possible:
break
try:
value = await obj_ref
except Exception:
break
if isinstance(value, ReplicaHealthFrame):
self._record_health_frame(value)
continue
break
with self._object_ref_or_gen_sync_lock:
self._obj_ref = obj_ref
self._async_consumption_future.set_result(obj_ref)
return self._obj_ref
except Exception as e:
self._async_consumption_future.set_exception(e)
raise| if len(self._state) > self._PRUNE_THRESHOLD: | ||
| # Age by controller-clock arrival time (v[1]), immune to replica skew. | ||
| cutoff = time.time() - self._PRUNE_MAX_AGE_S | ||
| self._state = {k: v for k, v in self._state.items() if v[1] >= cutoff} |
There was a problem hiding this comment.
If the number of active replicas or churned replicas exceeds _PRUNE_THRESHOLD (65536) and they are still within the _PRUNE_MAX_AGE_S (10 minutes) window, len(self._state) will remain above the threshold even after pruning. This causes every subsequent call to record to execute the O(N) dictionary comprehension, leading to high CPU usage and potential controller lag. Introduce a time-based guard to ensure pruning runs at most once per _PRUNE_MAX_AGE_S interval.
now = time.time()
if len(self._state) > self._PRUNE_THRESHOLD and now - getattr(self, "_last_prune_time", 0.0) > self._PRUNE_MAX_AGE_S:
self._last_prune_time = now
# Age by controller-clock arrival time (v[1]), immune to replica skew.
cutoff = now - self._PRUNE_MAX_AGE_S
self._state = {k: v for k, v in self._state.items() if v[1] >= cutoff}| if self._target_state.info is not None: | ||
| self._push_stall_window_s = _push_freshness_window_s( | ||
| self._target_state.info.deployment_config.health_check_period_s | ||
| ) |
There was a problem hiding this comment.
If self._target_state.info is None, self._push_stall_window_s is not updated and remains at its default value of 0.0. This will cause all pushed health reports to be treated as stale, potentially triggering false push stall verdicts. Fall back to DEFAULT_HEALTH_CHECK_PERIOD_S when self._target_state.info is None.
| if self._target_state.info is not None: | |
| self._push_stall_window_s = _push_freshness_window_s( | |
| self._target_state.info.deployment_config.health_check_period_s | |
| ) | |
| period = ( | |
| self._target_state.info.deployment_config.health_check_period_s | |
| if self._target_state.info is not None | |
| else DEFAULT_HEALTH_CHECK_PERIOD_S | |
| ) | |
| self._push_stall_window_s = _push_freshness_window_s(period) |
5969073 to
74f0497
Compare
Review (gemini): compare the set of active replica ids instead of an xor fingerprint -- no hash-collision risk, drops the reduce/xor imports, and set comparison is C-optimized. Review (cursor): when fail_on_rank_error is off the pass can swallow an error and return []; do not cache that membership as checked, or a stable deployment never retries. DeploymentRankManager now records whether the last op errored; cache only on success. Signed-off-by: john.taylor <john.taylor@anyscale.com>
…ersion A CPU profile of a controller managing 16K replicas showed the loop dominated by O(N) id-collection walks recomputed every tick (alive actor ids, active node ids, running replica ids, the autoscaler id-set rebuild) even though membership rarely changes in steady state. ReplicaStateContainer now bumps a mutation version on add / non-empty pop / non-empty remove, and the derived collections memoize on it at both DeploymentState and DeploymentStateManager level. Caching disarms while STARTING/RECOVERING replicas exist since their actor/node ids materialize in place. The autoscaler skips its id-set rebuild when handed the same (cached) list object, and the rank-consistency fingerprint becomes the version integer. Benchmark at 16K replicas: control loop 352.6ms -> 42.9ms. Signed-off-by: john.taylor <john.taylor@anyscale.com>
…ports P1 of the push-health stack. Replicas run their own health check on a periodic task (half the health-check period) and push the result: on the metric reports they already send when those are at least as frequent, or on a lightweight heartbeat otherwise. The controller keeps a push registry; pull probes become the fallback for stale pushes, guarded by a per-deployment systemic-stall verdict (mass staleness reads as controller ingest lag and defers probes, capped per episode) so an overloaded controller cannot trigger a probe storm. Signed-off-by: john.taylor <john.taylor@anyscale.com>
Review: the over-threshold prune re-ran an O(N) dict rebuild on every record() once all entries were fresh; now at most once per 30s. The stall window defaults to the stock health-check period so deployments without target info (recovery, deletion) cannot misread every push as stale. Signed-off-by: john.taylor <john.taylor@anyscale.com>
Review: suppression assumed a recent report/response carried current health, but a status flip to unhealthy after the last carry would be delayed a full period. Gate suppression on healthy so unhealthy reaches the controller promptly; unhealthy is rare, so the extra heartbeat is negligible. Signed-off-by: john.taylor <john.taylor@anyscale.com>
Review: the dirty-set health-checks only a slice (~N/sweep_ticks) each tick, but the stall guard compared its 64-sample floor against that one slice -- so it only engaged above ~3200 replicas per deployment. Accumulate the tally across ticks and finalize once enough replicas are sampled or a full sweep elapses; a large deployment whose one-tick slice already exceeds the floor still finalizes next tick, so engagement stays prompt where it matters. Extract _reconcile_sweep_ticks (shared with the dirty set). Known follow-up: many small deployments can still each stay under the per-deployment floor while the controller lags overall. Signed-off-by: john.taylor <john.taylor@anyscale.com>
P2 of the push-health stack. Response admission piggybacks the replica self-check on the queue-length info; routers keep the newest entry per replica (age-pruned) and fold it -- plus the host replica own health via a process-local provider -- into the handle metric reports they already send. Handle-owning replicas and replicas behind active handles need no heartbeat RPC; consumption stamps handle-carried recency so the P1 heartbeat stays suppressed. Signed-off-by: john.taylor <john.taylor@anyscale.com>
…sses carriage suppression Review (cursor + gemini): self_health_snapshot stamped handle-carried recency even when _health_kwargs sent nothing (checked_at unset), suppressing a fresh replica first heartbeat; now stamps only when health is actually carried. Also gate the response/handle-report suppression clauses on healthy so an unhealthy flip is never delayed a full period. Signed-off-by: john.taylor <john.taylor@anyscale.com>
P3 of the push-health stack. Replicas holding long-running unary rejection-protocol requests interleave ReplicaHealthFrame system messages on the already-open response stream; the router pumps them into its carried-health map (which P2 folds into handle reports). Pumps are armed only when the replica declares frames at admission (health not already riding reports), and frames stop the moment another channel carries health -- so busy handle-owning replicas need no heartbeat even when requests are held for minutes. Signed-off-by: john.taylor <john.taylor@anyscale.com>
74f0497 to
439a873
Compare
|
Abandoning |

Third PR of the push-health stack (stacked on #64912, which stacks on #64913).
Why
After P1 (health rides replica metric reports) and P2 (health rides handle metric
reports via response admission), one class of replica still falls back to heartbeat
RPCs: replicas that don't push metric reports and whose requests are held for a
long time — e.g. handle-owning pipeline replicas whose downstream calls run for
minutes. Admission-time piggyback (P2) only carries health when a request is
admitted; a replica sitting on long-running requests has no admissions, so its
carried health goes stale and the P1 heartbeat kicks in. At high replica counts
those heartbeats are exactly the standalone health RPCs this stack exists to
eliminate.
The observation: such a replica already has an open response stream to its
caller (the rejection-protocol generator). Health can ride that.
What
ReplicaHealthFrame— a small frozen-dataclass system message (healthy,checked_at, consecutive_failures).
calls, the caller sets
RequestMetadata.supports_health_frames; the replicaanswers with
will_send_health_framesin the acceptedReplicaQueueLengthInfo,binding the behavior for that request's lifetime.
awaits the user task in health-period ticks and yields a frame whenever there is
a newer self-check result — the final yield is always the user result.
only if the request is still pending after a short delay) consumes frames into
the router's carried-health map, where P2 folds them into the handle metric
reports it already sends. Frames never reach user code; the result path filters
them.
Suppression (no double delivery)
Frames stop the moment health rides any other channel, re-checked every tick while
a request is held: if metric reports carry health (cadence ≤ health period) or the
replica's health was recently consumed into a handle report, the wrapper skips the
frame. Conversely the router arms a pump only when the replica declared frames —
at 10K+ fan-in, unconditionally-armed idle pumps cost real driver memory.
Limitations
Frames concentrate at the request owner: a single process holding requests on a
very large fleet receives that fleet's frames (measured ~1.6K frames/s on one
process at 16K replicas before declaration gating; gating removes the idle-pump
cost but a genuinely held-everywhere workload still fans in). For one-owner /
huge-fleet topologies the report-aggregation tier remains future work.
Testing
start/settling, per-tick suppression, frame filtering on the result path,
newest-only dedupe (331 lines).
router's carried health past the admission-time entry.
suites, identical to the pre-split composition).
Benchmarks
Full-stack numbers (this PR + 64913 + 64912 on master + the rank-gate and id-walk-cache
PRs), HAProxy off, pinned replicas, 36 samples/cell:
Per-PR attribution (P2-only vs +P3 arms at 8K/16K) is being measured on the same
harness; numbers land here before review.