[serve] Gate HAProxy startup readiness on admin-socket health, not the process exit code#64213
[serve] Gate HAProxy startup readiness on admin-socket health, not the process exit code#64213eicherseiji wants to merge 2 commits into
Conversation
…xit-code polling _wait_for_hap_availability busy-polled proc.returncode in a loop to detect a crashed HAProxy spawn while waiting for it to take over the admin socket. This replaces the exit-code polling with HAProxy's built-in runtime status as the readiness signal and event-driven crash detection. Readiness still comes from `show info` reporting the spawn's pid as the live worker, which is required during a reload: the admin socket answers through the displaced worker until the new process rebinds it, so a socket-only check could declare a dead spawn ready and strand its -sf target. A crash is now surfaced the moment the process exits by awaiting proc.wait(), so startup fails fast with the spawn's stderr instead of lagging the fixed poll interval. Resolves the "TODO: update this to use health checks" in the startup wait. Signed-off-by: Seiji Eicher <seiji@anyscale.com>
There was a problem hiding this comment.
Code Review
This pull request improves the HAProxy startup availability check by replacing busy-polling with an asynchronous wait on the process exit (proc.wait()), allowing immediate detection of crashes during startup. A unit test was also added to verify this behavior. The review feedback points out a potential issue in the finally block where awaiting the cancelled proc_exited task and catching CancelledError could risk suppressing the outer task's cancellation, and suggests simply calling cancel() without awaiting it.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| finally: | ||
| if not proc_exited.done(): | ||
| proc_exited.cancel() | ||
| try: | ||
| await proc_exited | ||
| except asyncio.CancelledError: | ||
| pass |
There was a problem hiding this comment.
Awaiting proc_exited inside the finally block and catching asyncio.CancelledError is risky. If the outer task (running _wait_for_hap_availability) is cancelled, the CancelledError will be raised at await proc_exited and caught/suppressed by the except asyncio.CancelledError block. This can lead to the outer task's cancellation being silently ignored or delayed.
Since proc_exited is a task wrapping proc.wait(), which has no side effects, there is no need to await it after calling cancel(). Simply calling proc_exited.cancel() is sufficient and avoids any risk of suppressing the outer task's cancellation.
| finally: | |
| if not proc_exited.done(): | |
| proc_exited.cancel() | |
| try: | |
| await proc_exited | |
| except asyncio.CancelledError: | |
| pass | |
| finally: | |
| if not proc_exited.done(): | |
| proc_exited.cancel() |
Cancel the proc.wait() future directly in the finally. Awaiting the cancelled task could swallow an outer-task cancellation, and cancel() is already a no-op once the task is done, so the guard and the await are unnecessary. Addresses review feedback on the startup-readiness change. Signed-off-by: Seiji Eicher <seiji@anyscale.com>
What
The HAProxy startup wait already gated readiness on HAProxy's own runtime status.
_wait_for_hap_availabilitywaits for the admin socket to report the freshly spawned worker's pid as the live one viashow info. On top of that it polledproc.returncodeon every iteration to fail fast if the spawn crashed.This removes that process-exit-code poll. A crashed spawn is now detected event-driven by awaiting
proc.wait(), so readiness is gated on HAProxy's health and the OS process is only watched for a fast crash signal.The pid match is kept deliberately. During a reload the admin socket answers through the displaced worker until the new process rebinds it, so requiring the answering pid to be the spawn avoids declaring a dead spawn ready and stranding its
-sftarget.Behavior change: a startup or reload crash is reported immediately with the spawn's stderr instead of after up to one poll interval.
Resolves the
# TODO: update this to use health checksin the startup wait.Related issue number
N/A
Checks
Run against the branch with
RAY_SERVE_ENABLE_HA_PROXY=1:python/ray/serve/tests/unit/test_haproxy_process_manager.py, addstest_detects_crash_during_waitpython/ray/serve/tests/unit/test_haproxy_binary.pypython/ray/serve/tests/test_haproxy_api.pyreal-subprocess start and reload paths