Skip to content

feat(rlix): bound the resize chain with lock/RPC timeouts + param validation#35

Open
JunzheJoe wants to merge 1 commit into
zhenyu/miles-mvp-e2efrom
joe/resize-timeouts
Open

feat(rlix): bound the resize chain with lock/RPC timeouts + param validation#35
JunzheJoe wants to merge 1 commit into
zhenyu/miles-mvp-e2efrom
joe/resize-timeouts

Conversation

@JunzheJoe

Copy link
Copy Markdown
Collaborator

What

Review findings F2-design-1/2, F3-TIMEOUT-1/2/3 and S12-RESIZE-NOTIMEOUT (F1-F12 code review). Same problem family as the guide's open MED1 (concurrent-resize under max_concurrency=4): the resize chain had unbounded blocking surfaces at three layers, so one wedged component could hang callers forever — and through the central scheduling loop, stall every pipeline's request/release processing. The fail-fast policy only covers exceptions, not hangs.

1. MilesCoordinator lock timeout (new _resize_locked)

PipelineCoordinator acquires _resize_sync_lock with RLIX_RESIZE_LOCK_TIMEOUT_S (default 180s) and raises on timeout; MilesCoordinator used bare with self._resize_sync_lock: in 9 places. New _resize_locked(op) contextmanager mirrors the same env var and raises with the blocked op's name. All 9 sites converted.

2. Resize-chain ray.get timeouts (MILES_RESIZE_RPC_TIMEOUT_S, default 180s)

shrink_engines / get_engine_states / expand_engines / sync_selected_workers / activate_routing were awaited without timeout. Default leaves margin over the service-internal ROLL_SELECTIVE_MODEL_UPDATE_TIMEOUT_S (150s) and the shrink-side 30s drain. <=0 disables (standard parse_env_timeout_s semantics).

3. Scheduler fan-out timeout (RLIX_RESIZE_RPC_TIMEOUT_S, default 300s)

_execute_resize_calls gathers coordinator resize_infer RPCs outside the scheduler lock with no bound. New _gather_resize_rpcs wraps both fan-outs in asyncio.wait_for; a timeout raises and routes through scheduling_cycle's existing fail-fast shutdown path (consistent with the fail-fast-only operational policy). Default sits above the coordinator-side worst case.

4. resize_infer param validation

Reuses rlix.pipeline.utils.validate_resize_params (exactly one of remove/add non-empty) — same contract as PipelineCoordinator.resize_infer. The scheduler already issues shrinks and expands as separate one-sided RPCs, so a mixed call indicates a caller bug.

Not in scope

The full concurrent-resize stress test stays an M11.3 follow-up (guide MED1); this is the minimal bound-everything version.

Tests

tests/test_miles_coordinator_resize_guards.py:

  • resize_infer rejects mixed shrink+expand and double-empty calls
  • lock acquisition times out with a RuntimeError instead of blocking when the lock is wedged, and succeeds after release

Full suite: 69 passed, 3 skipped (with #33/#34's tests included).

…idation

The resize chain had three unbounded blocking surfaces, so one wedged
component (e.g. a stuck SGLang drain inside shrink_engines) could hang
callers forever — and via the central scheduling loop, stall every
pipeline's request/release processing (guide MED1 family):

1. MilesCoordinator used bare 'with self._resize_sync_lock:' everywhere.
   New _resize_locked(op) contextmanager mirrors PipelineCoordinator's
   RLIX_RESIZE_LOCK_TIMEOUT_S discipline (default 180s) and raises with
   the op name on timeout. All 9 sites converted.

2. The 5 resize-chain ray.get calls into miles (shrink_engines /
   get_engine_states / expand_engines / sync_selected_workers /
   activate_routing) now carry MILES_RESIZE_RPC_TIMEOUT_S (default
   180s, margin over the service-internal 150s; <=0 disables).

3. Scheduler-side _execute_resize_calls wraps both gathers in
   asyncio.wait_for via RLIX_RESIZE_RPC_TIMEOUT_S (default 300s, above
   the coordinator-side worst case). Timeout raises and routes through
   scheduling_cycle's existing fail-fast shutdown path.

Also: resize_infer now validates params via validate_resize_params
(exactly one of remove/add non-empty), matching the
PipelineCoordinator.resize_infer contract; the scheduler already issues
shrinks and expands as separate one-sided RPCs.

The full concurrent-resize stress test remains an M11.3 follow-up; this
is the minimal bound-everything version.
@JunzheJoe
JunzheJoe requested a review from taoluo July 7, 2026 02:24
@TianyeGGBond

Copy link
Copy Markdown
Collaborator

The timeout layering is inverted — the scheduler bound fires before the coordinator's

The PR description says the scheduler-side default "sits above the coordinator-side worst case". Working through the actual numbers, it's the other way around.

With both defaults in place (RLIX_RESIZE_LOCK_TIMEOUT_S=180, MILES_RESIZE_RPC_TIMEOUT_S=180), the coordinator-side worst case for a single resize_infer expand is:

step bound
_expand_workers(snapshot) lock acquire 180s
ray.get(get_engine_states) 180s
ray.get(expand_engines) 180s
ray.get(sync_selected_workers) 180s
ray.get(activate_routing) 180s
_expand_workers(commit) lock acquire 180s
total 1080s

Even ignoring both lock waits, the RPC chain alone is 4 × 180 = 720s. The shrink path is 180s of RPC plus up to 360s of lock waits.

All of these exceed the scheduler's _RESIZE_EXEC_TIMEOUT_S default of 300s. So the outer bound is always the one that fires first, and the inner per-RPC bounds this PR adds can essentially never be reached in the default configuration.

That matters because the two layers fail very differently. A coordinator-side ray.get timeout surfaces as a GetTimeoutError on one pipeline's resize. A scheduler-side timeout raises out of _gather_resize_rpcs, which routes through scheduling_cycle's fail-fast path and takes down the whole orchestrator. As written, a slow-but-healthy expand — say a large sync_selected_workers broadcast plus a contended lock — gets converted into a full shutdown rather than a bounded per-pipeline error. That's a worse failure mode than the unbounded hang this is meant to replace, because it's no longer isolated to the wedged component.

Two ways to fix, either is fine:

  1. Raise the scheduler default above the coordinator's worst case (≥1200s to cover the lock waits too), so the inner bounds genuinely fire first and the outer one stays a true last-resort backstop.

  2. Give the coordinator a single shared deadline instead of a per-RPC budget: compute deadline = time.monotonic() + T on entry to _expand_workers / _shrink_workers and pass the remaining time to each ray.get. Then MILES_RESIZE_RPC_TIMEOUT_S bounds the whole chain rather than each hop, the number is meaningful to whoever tunes it, and it composes cleanly with a scheduler default that only has to clear it by a margin.

I'd lean towards (2) — with (1), the 180s per-hop number doesn't correspond to anything an operator can reason about, and the totals shift again if another RPC is added to the chain later.

Worth adding a comment recording the intended ordering (inner bounds < outer bound) wherever the defaults land, so a future change to one side doesn't silently re-invert it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants