Repository navigation
[Serve] Base handle metric staleness on controller receive time - #66307
Merged
abrarsheikh merged 3 commits intoSep 22, 2026
Merged
abrarsheikh merged 3 commits into
abrarsheikh merged 3 commits into
Conversation
drop_stale_handle_metrics treated producer report.timestamp as receive age, so clock skew between routers and the controller could drop fresh metrics early or retain phantoms past the timeout. Record a controller-local monotonic receive time in _HandleMetricStore on accepted reports and use that only for the timeout path. Keep producer timestamps for is_fresher / cross-format ordering, timeseries aggregation, and ingest-delay metrics. Dead-actor dropping is unchanged. Fixes ray-project#66271 Signed-off-by: Dundy Pasupuleti <dundysm@gmail.com>
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
Author
|
cc @ray-project/ray-serve @edoakes @zcin @johntaylor-cell @suppagoddo approach a from the issue: controller-local monotonic receive time for drop_stale_handle_metrics only. producer timestamps stay for is_fresher / cross-format ordering (#66070), timeseries, and ingest-delay metrics. dead-actor path unchanged. fixes #66271 |
ray-gardener
Bot
added
serve
Contributor
|
@dundysm - Can you fix the failing tests? |
johntaylor-cell
approved these changes
Sep 21, 2026
Contributor
Contributor
|
pre-commit checks are failing |
Apply @johntaylor-cell minimal patch: black/isort formatting in test_columnar.py and conftest.py, plus treat missing receive stamps as timed out in drop_stale_handle_metrics so a broken invariant cannot keep handles forever. accept() always stamps _received_at; forget() clears it, so the None path is defensive only. Signed-off-by: Dundy Pasupuleti <dundysm@gmail.com>
Contributor
Author
|
@abrarsheikh applied @johntaylor-cell's minimal lint patch (black/isort wraps plus the defensive timed_out None receive-stamp handling in drop_stale_handle_metrics) and pushed to this branch. please re-check pre-commit. |
Contributor
Author
|
@johntaylor-cell thanks for the verify, approval, and minimal diff. applied and pushed as-is. the timed_out None change is intentional defensive only: accept() always stamps _received_at after the fresher gate, and forget() clears it, so the happy path is unchanged. |
abrarsheikh
approved these changes
Sep 22, 2026
owenowenisme
pushed a commit
to owenowenisme/ray
that referenced
this pull request
Sep 23, 2026
…project#66307) ## Fixes Fixes ray-project#66271 ## Problem `drop_stale_handle_metrics` is documented as dropping handles the controller has not **received** updates from within `max(2 * metrics_interval_s, RAY_SERVE_MIN_HANDLE_METRICS_TIMEOUT_S)`, but it compared controller `time.time()` to the producer's `report.timestamp`. That mixes wall clocks from different machines, so skew can drop fresh metrics early (producer behind) or retain phantoms past the timeout (producer ahead). ## What changes - `_HandleMetricStore` records a controller-local **monotonic** receive time in `_received_at` when `accept()` succeeds. - `drop_stale_handle_metrics` ages handles as `time.monotonic() - received_at` for the timeout path. - `forget()` clears `_received_at` with the rest of the handle state. - Rejected (stale) accepts do **not** refresh receive time. ## What is preserved (intentional) - Producer timestamps remain the sole key for `is_fresher` / cross-format ordering (ray-project#66070). - Timeseries sample timestamps and aggregation look-back stay producer-based. - Ingest-delay metrics (`_record_metrics_delay`) stay producer-based. - Dead Serve-actor dropping is unchanged. - No wire / public API change; receive time is never put on the wire. This is scoped only to the drop freshness mismatch with the documented "received" semantics. Aggregation `last_window_s` skew and delay-metric docs are left alone on purpose. ## Tests - New unit coverage for producer clock behind/ahead, rejected accept not refreshing receive time, fresher accept refreshing it, dead-actor path, and columnar parity. - Existing columnar drop / fresher / cross-format tests updated so timeout cases patch `time.monotonic` alongside `time.time`. - `mock_deployment_state_manager` also patches `time.monotonic` so handle-timeout deployment-state tests keep working. ## Related Credit to @suppagoddo for the issue writeup and receive-time approach. Coordinates with ray-project#66070 (ordering gate) and does not wait on ray-project#66240. cc @ray-project/ray-serve @edoakes @zcin @johntaylor-cell @suppagoddo --------- Signed-off-by: Dundy Pasupuleti <dundysm@gmail.com> Co-authored-by: Abrar Sheikh <abrar@anyscale.com>
nadongjun
pushed a commit
to nadongjun/ray
that referenced
this pull request
Oct 6, 2026
…project#66307) ## Fixes Fixes ray-project#66271 ## Problem `drop_stale_handle_metrics` is documented as dropping handles the controller has not **received** updates from within `max(2 * metrics_interval_s, RAY_SERVE_MIN_HANDLE_METRICS_TIMEOUT_S)`, but it compared controller `time.time()` to the producer's `report.timestamp`. That mixes wall clocks from different machines, so skew can drop fresh metrics early (producer behind) or retain phantoms past the timeout (producer ahead). ## What changes - `_HandleMetricStore` records a controller-local **monotonic** receive time in `_received_at` when `accept()` succeeds. - `drop_stale_handle_metrics` ages handles as `time.monotonic() - received_at` for the timeout path. - `forget()` clears `_received_at` with the rest of the handle state. - Rejected (stale) accepts do **not** refresh receive time. ## What is preserved (intentional) - Producer timestamps remain the sole key for `is_fresher` / cross-format ordering (ray-project#66070). - Timeseries sample timestamps and aggregation look-back stay producer-based. - Ingest-delay metrics (`_record_metrics_delay`) stay producer-based. - Dead Serve-actor dropping is unchanged. - No wire / public API change; receive time is never put on the wire. This is scoped only to the drop freshness mismatch with the documented "received" semantics. Aggregation `last_window_s` skew and delay-metric docs are left alone on purpose. ## Tests - New unit coverage for producer clock behind/ahead, rejected accept not refreshing receive time, fresher accept refreshing it, dead-actor path, and columnar parity. - Existing columnar drop / fresher / cross-format tests updated so timeout cases patch `time.monotonic` alongside `time.time`. - `mock_deployment_state_manager` also patches `time.monotonic` so handle-timeout deployment-state tests keep working. ## Related Credit to @suppagoddo for the issue writeup and receive-time approach. Coordinates with ray-project#66070 (ordering gate) and does not wait on ray-project#66240. cc @ray-project/ray-serve @edoakes @zcin @johntaylor-cell @suppagoddo --------- Signed-off-by: Dundy Pasupuleti <dundysm@gmail.com> Co-authored-by: Abrar Sheikh <abrar@anyscale.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes #66271
Problem
drop_stale_handle_metricsis documented as dropping handles the controller has not received updates from withinmax(2 * metrics_interval_s, RAY_SERVE_MIN_HANDLE_METRICS_TIMEOUT_S), but it compared controllertime.time()to the producer'sreport.timestamp. That mixes wall clocks from different machines, so skew can drop fresh metrics early (producer behind) or retain phantoms past the timeout (producer ahead).What changes
_HandleMetricStorerecords a controller-local monotonic receive time in_received_atwhenaccept()succeeds.drop_stale_handle_metricsages handles astime.monotonic() - received_atfor the timeout path.forget()clears_received_atwith the rest of the handle state.What is preserved (intentional)
is_fresher/ cross-format ordering ([serve] Columnar zero-copy autoscaling-metrics ingest (plumbing) #66070)._record_metrics_delay) stay producer-based.This is scoped only to the drop freshness mismatch with the documented "received" semantics. Aggregation
last_window_sskew and delay-metric docs are left alone on purpose.Tests
time.monotonicalongsidetime.time.mock_deployment_state_manageralso patchestime.monotonicso handle-timeout deployment-state tests keep working.Related
Credit to @suppagoddo for the issue writeup and receive-time approach. Coordinates with #66070 (ordering gate) and does not wait on #66240.
cc @ray-project/ray-serve @edoakes @zcin @johntaylor-cell @suppagoddo