Conversation
…read
try_ensure_parallel_info() is documented as a "single non-blocking attempt",
but it performs a synchronous requests.get(..., timeout=5) on the decode
scheduler thread, reached from process_decode_queue() -> pop_preallocated() ->
_resolve_pending_reqs() -> _ensure_prefill_info().
A bootstrap server that stops answering rather than refusing therefore stalls
the whole decode loop for the full 5s timeout per attempt. The 1s retry
interval does not bound this: _ensure_last_attempt_time is stamped before the
call, so the 5s timeout absorbs the interval and attempts land back to back,
up to _max_ensure_retries. This is the state a decode instance enters after
the heartbeat checker evicts a prefill addr from prefill_info_table.
Hand the HTTP fetch to a small executor instead. try_ensure_parallel_info()
becomes a cache lookup plus at most one fetch submission, which is what its
callers already expect ("False" means "not available yet, retry on a later
scheduling cycle"). An in-flight set guarded by connection_lock keeps repeated
cycles from queueing duplicate fetches, and the cache is re-checked under that
lock so a publish racing the check does not cause a redundant fetch. Config
mismatches stay fatal: the fetch thread logs the error and records the
exception, and raise_parallel_info_error() re-raises it on the scheduler
thread, both from try_ensure_parallel_info() and once per scheduling cycle in
_ensure_prefill_info() so a mismatch found on the last allowed attempt is not
masked by the abort path.
_ensure_prefill_info() keeps its retry semantics across the now asynchronous
fetch. It checks the cache before the retry interval, so a result that lands
between two cycles is picked up on the next one rather than after the interval
expires. A cycle that finds a fetch still in flight does not spend a retry
attempt, keeping _max_ensure_retries a count of fetch attempts. And the abort
is decided before spending an attempt instead of after, so the last attempt is
not abandoned before its result is known, and the abort branch re-checks the
cache so a result publishing between the two does not lose its requests.
Signed-off-by: zky <kaiyue.zhou@z.ai>
4cd547d to
4e91af3
Compare
|
Re-verified against current main ( Relationship to the recently merged #35071: complementary, not overlapping — #35071 parallelizes the per-DP-rank bootstrap queries in the decode prealloc path, while the blocking parallel-info fetch addressed here is a different call site and remains synchronous on main. Local verification (head
This is in the PD-disaggregation area. Could a maintainer add the run-ci label when convenient? |
Motivation
CommonKVManager.try_ensure_parallel_info()is documented as a "singlenon-blocking attempt", but it performs a synchronous
requests.get(url, timeout=5). It is called from the decode scheduler thread:So a bootstrap server that stops answering rather than refusing stalls the
entire decode loop — every running request on that instance — for the full
timeout.
The 1 s retry interval in
_ensure_prefill_info()does not bound this.now = time.monotonic()is captured once before the loop and_ensure_last_attempt_time[bootstrap_addr]is stamped before the call, so a5 s timeout fully absorbs the 1 s interval and attempts land essentially back to
back. With
_max_ensure_retries = 15that is up to ~75 s of near-continuousstall for one unreachable addr, and
addr_to_reqsis iterated sequentially, soK such addrs cost K × 5 s within a single scheduling cycle.
This is not a hypothetical state. The decode-side heartbeat checker evicts an
addr from the cache after
max_failuresconsecutive/healthfailures:Requests still routed to that addr then miss the cache, land in
pending_reqs,and the scheduler thread itself starts dialing the unreachable prefill server.
/health_generatepasses as long aslast_receive_tstampadvances withinHEALTH_CHECK_TIMEOUT(default 20 s), so a single 5 s stall only inflates ITL —but a sustained stall exceeds the window, the decode server reports unhealthy,
and a router in front of it will pull it out of rotation while its requests are
still in flight. #20252 reports exactly that shape at scale: a prefill restart
leaves decode servers "stuck continuously retrying to connect to the downed
prefill server", their health checks time out, sglang-router removes them, and
the remaining decoders take the concentrated traffic.
#20785 already set out to fix this — its summary says
add()becomes "a purecache lookup — no network calls, no blocking", and its background notes the
old
ensure_parallel_info()"blocks up to 30s (5 retries × 1s sleep + 5s HTTPtimeout)". It did remove the retry loop and the
time.sleep(). But theremaining single 5 s GET only moved from
add()to thepending_reqsresolution path; both run on the scheduler thread.
Modifications
Hand the HTTP fetch to a small
ThreadPoolExecutor, mirroring the existing_prefill_recompute_executoridiom in the same class:try_ensure_parallel_info()is now a cache lookup plus at most one fetchsubmission per addr, and returns
Falsewhile a fetch is outstanding. Itscallers already treat
Falseas "not available yet, retry on a laterscheduling cycle", so no caller contract changes.
fetches for the same addr. It is mutated under
connection_lock, and the cacheis re-checked inside that lock, so a fetch that publishes between the unlocked
check and the locked section does not cause a redundant fetch. The
parallel_info_fetch_in_flight()predicate reads the set without the lock; astale answer only costs a skipped cycle or one extra call that the locked
re-check rejects.
_fetch_parallel_info()(the blocking GET, HTTPerror handling unchanged — log and return no info),
_check_parallel_info()(the page-size / kv-cache-dtype / DCP validations, unchanged), and
_publish_parallel_info()(the executor-thread body).take the engine down, which is deliberate; an executor would swallow them, so
the fetch thread logs the error and records the exception, and
raise_parallel_info_error()re-raises it on the scheduler thread — both fromtry_ensure_parallel_info()and once per scheduling cycle in_ensure_prefill_info(), so a mismatch found on the last allowed attempt isnot masked by the abort path taking over on that cycle.
_ensure_prefill_info()gets four adjustments so its retry bookkeeping keepsthe meaning it has today, now that a fetch can outlive a scheduling cycle:
asynchronously is consumed on the next cycle instead of waiting out the
interval (otherwise the first request to a new addr would pay 1 s);
Without this,
_max_ensure_retrieswould silently become "15 schedulingcycles" instead of "15 fetch attempts", cutting the tolerance for a slow
bootstrap server by ~5× and making the abort message ("after N attempts")
untrue;
the call was blocking, the outcome of the last attempt was known in the same
cycle; with an asynchronous fetch, aborting right after submitting the last
attempt would discard its result unseen, so a fetch that succeeds on attempt
15 would still lose its requests. Checking the budget first means the abort
can only happen on a cycle where no fetch is outstanding, i.e. after all 15
have finished. The abort therefore waits out the last attempt — up to one
timeout=5later than before, and one retry interval later if the attemptfinishes sooner than that — and the message is unchanged;
publish between the check at the top of the cycle and the abort below it, and
those requests are fine. On
mainthis could not happen —prefill_info_tablewas written only by this thread — so the window is one this change opens.
Behaviour
prefill addr) — a failed fetch is retried as before — and every later call is
the same dict lookup as today.
admitted on a later scheduling cycle instead of after a round trip.
of them complete before the same abort message is logged. For an unresponsive
server the wall-clock time to abort is therefore comparable to today's; the
difference is that the scheduler is not blocked while those attempts run.
ThreadPoolExecutorworker threads are joined at interpreter exit, sooutstanding fetches can delay shutdown until they finish. There is at most one
fetch per bootstrap addr, and
requests'timeout=5bounds connect and readindividually rather than total wall clock. This matches the existing
_prefill_recompute_executorin this class.same addr can still republish it afterwards. That race predates this change
(the inline fetch raced
_handle_node_failureon the heartbeat thread inexactly the same window), so it is left out to keep this change focused.
Tests
test/registered/unit/disaggregation/test_parallel_info_nonblocking.py(new,CPU-only, no sleeps — a fake fetch blocks on one
threading.Eventand signalsthat it has been entered with another, a single-worker executor barrier waits for
a submission to drain, and one custom lock context covers the publish-during-lock
case):
Truewithout touching the network;try_ensure_parallel_info()returns while the GET is still outstanding, andthe info is published once the fetch completes — this is the regression guard;
retried by the next call;
RuntimeErroron the caller thread;
_ensure_prefill_info()admits an addr as soon as the result lands, eveninside the retry interval, while the interval still paces fetch attempts;
instead of being aborted in the cycle that submitted it;
abort below it admits its requests instead of aborting them;
being reported as retry exhaustion.
I checked that these fail without the code they cover: making the submission
blocking again (
.result()) fails the two non-blocking cases and stretches thefile from 12 s to 83 s as the fake fetches serialize; dropping the in-flight
skip in
_ensure_prefill_info()fails the retry-budget case; dropping there-check under the lock fails the duplicate-fetch case; deciding the abort after
submitting rather than before fails the last-attempt case; dropping the cache
re-check in the abort branch fails the publish-before-abort case; dropping the
per-cycle
raise_parallel_info_error()fails the final-attempt-mismatch case.test/registered/unit/disaggregation/passes in full (163 tests).test_decode_queue_cleanup.pyneeded two lines and one value: it mocks the KVmanager with a bare
MagicMock, so the new predicates returned truthy mocks,and it pre-seeds
_ensure_retry_countat 0 with_max_ensure_retries = 1toreach the abort path in a single call. Since the budget is now checked before an
attempt is spent, that seed is 1; what the test covers (a request whose
kv_receiverwas already cleared must not crash on.abort()) is unchanged.Not run by me: a multi-node PD failure smoke test (kill a prefill instance,
confirm the decode instance stays healthy and keeps serving). I do not have a
multi-node rig available; happy to add one if a maintainer prefers it in-tree.
Accuracy Tests
Not applicable: this changes which thread an HTTP GET runs on. No model forward,
kernel, or numerics change.
Speed Tests and Profiling
No steady-state performance change to measure: after the first fetch per
(decode process, prefill addr) the call is a dict lookup on both
mainand thisbranch, and nothing is added to the per-token path. The change is to the failure
path, where the scheduler loop no longer blocks for 5 s per fetch attempt; the
quantities involved (5 s timeout, 1 s interval, 15 attempts,
HEALTH_CHECK_TIMEOUT= 20 s) are all in-tree constants cited above.Checklist
CI States
Latest PR Test (Base): ❌ Run #32809376569
Latest PR Test (Extra): ❌ Run #32809376468
Latest PR Test (AMD ROCm 7.2): ❌ Run #32809376582