Repository navigation
fix(supervisor): evict dead replica from round-robin when auto-recover exhausted - #5006
Conversation
There was a problem hiding this comment.
Code Review
This pull request improves Out-Of-Memory (OOM) recovery by switching from sys.exit(1) to os._exit(1) in the model actor, ensuring xoscar can detect subprocess failures and trigger pool recovery. It also implements replica eviction in the supervisor when recovery limits are exhausted and adds tests for these scenarios. The review feedback correctly points out that the new asynchronous tests use blocking time.sleep(1) calls, which should be replaced with await asyncio.sleep(1) to prevent blocking the event loop.
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.
exhausted When worker exhausts AUTO_RECOVER_LIMIT and stops recreating a model actor, the dead replica remains in supervisor's round-robin scheduler and model_unexpected_termination gauge is never lit. Requests routed to the dead replica fail silently, and ops has no observability signal that N-1 out of N replicas are healthy. Add supervisor.mark_replica_dead() that evicts the dead replica from the round-robin scheduler, lights up model_unexpected_termination, and advances base_uid to TERMINATED when the single/last replica dies. The method is idempotent and deliberately does not call back worker.terminate_model (the worker already terminated the model locally). Worker calls back supervisor.mark_replica_dead() after "Stop recreating model actor." with a 5s timeout guard so a stalled supervisor does not hold up the worker's local shutdown. Add regression tests in both sync and async client test suites. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ffe4c39 to
8d2afe8
Compare
qinxuye
left a comment
There was a problem hiding this comment.
Two issues to address before LGTM.
Resolve three review findings on PR xorbitsai#5006: - supervisor.py: mark_replica_dead now tears down the distributed (Xavier) rank0 actor and the collective-manager/block-tracker mappings when the last replica dies. Extract the shared cleanup from terminate_model_replica into _cleanup_distributed_actors so auto-recover exhaustion no longer leaks supervisor-side actors/state on relaunch. The marker is kept lit for the failure gauge (caller-controlled), and the worker RPC is skipped on the mark_replica_dead path since the worker already terminated locally. - worker.py: bound the whole supervisor notification -- get_supervisor_ref plus mark_replica_dead -- in a single xo.wait_for(5s). get_supervisor_ref itself issues blocking xo.actor_ref calls when the cached ref is missing, so the previous timeout started too late to bound the recover tail path. - test_async_client.py: replace blocking time.sleep(1) with await asyncio.sleep(1) in the async retry loops; drop the now-unused time import. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…stion rank0 is a separate subpool (launch_rank0_model appends its own sub pool with its own address) that a regular replica's OOM never terminates -- recover_sub_pool only acts on the subpool whose address died. mark_replica_dead previously called _cleanup_distributed_actors with terminate_rank0_on_worker =False, dropping only the supervisor mapping and leaking the live rank0 actor/subpool; a later launch reusing the uid could hit stale actors. Pass terminate_rank0_on_worker=True so the worker is RPC'd to terminate rank0, bounded by xo.wait_for(5s) to keep the death-recovery tail path from being held up by a stalled worker. Add unit tests covering the rank0 cleanup branch and the last-replica mark_replica_dead path (Xavier). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
AUTO_RECOVER_LIMITand stops recreating a model actor, the dead replica remains in supervisor's round-robin scheduler andmodel_unexpected_terminationgauge is never lit. Requests routed to the dead replica fail silently, and ops has no observability signal that N-1 out of N replicas are healthy.supervisor.mark_replica_dead()that evicts the dead replica from the round-robin scheduler, lights upmodel_unexpected_termination, and advancesbase_uidtoTERMINATEDwhen the single/last replica dies. The method is idempotent and deliberately does not call backworker.terminate_model(the worker already terminated the model locally before giving up recreating).supervisor.mark_replica_dead()after "Stop recreating model actor." with a 5sxo.wait_fortimeout guard so a stalled supervisor does not hold up the worker's local shutdown. Failure/timeout is non-fatal — the next death detection or redeploy will reconcile.test_model_oom_recover_exhausted_evicts_replica).Depends on #5005 (fix/model: use os._exit for OOM to trigger pool recovery) which provides the
os._exit(1)fix that makes OOM-triggered recovery cycles actually work. This PR includes #5005's commits as its base; the PR3-specific changes are in the supervisor.pymark_replica_deadmethod, the worker callback, and the eviction test.Test plan
test_model_oom_recover_exhausted_evicts_replicain bothtest_client.pyandtest_async_client.py— verifies dead replica is evicted after recover limit exhaustedtest_model_oom_triggers_restart(from fix(model): use os._exit for OOM to trigger pool recovery #5005) — verifies OOM still triggers subprocess exittest_auto_recover— verifies normal auto-recovery still worksXINFERENCE_MODEL_ACTOR_AUTO_RECOVER_LIMIT=1, trigger repeated OOM, verifymodel_unexpected_terminationgauge lights up andlist_models()drains to empty🤖 Generated with Claude Code