Repository navigation
[Mooncake][Worker] Fail only the request whose async tensor read failed, at TP>1 too - #419
Conversation
|
I tested this on an H200 with Mooncake's TCP transport, including injected read failures (results above). I don't have RDMA hardware right now. I will test on RDMA when I get access to it and post the results here. |
merceod
left a comment
There was a problem hiding this comment.
Read the diff and ran it: the 6 new tests pass here and fail on main, the five touched suites pass (77), the CPU core list passes (954). On H200s with Qwen3.5-0.8B, main against this branch. SHM: 112/112 greedy outputs identical and load within the noise of a shared node, as expected since SHM reads carry no future. Mooncake TCP (this Mooncake build needs MC_FORCE_TCP=1 on nodes with IB NICs, otherwise it registers buffers with RDMA and every read returns -1, which incidentally reproduced the #332 wedge on main for real: 8.4 million log lines in three minutes and the server never came up, while this branch came up and answered each request with a fast 500). With a working transport and every 5th Mooncake read raising in both processes: this branch answered 15 of 20 requests with a 500 in 23 to 328 ms and 5 correctly, no stall, no main-loop error, 10 of 10 correct once the flag came off; main under the same hook answered none of 20, every request hit the 90 s client timeout, and its log grew by 21.7 million lines (1.6 GB) in seven minutes. An abort storm of 80 image requests closed 0 to 200 ms after sending: no failed read and no quarantine on either tree, so client aborts do not feed this path. The worker side does what the description says: a failed read is dropped in one poll, its request reported once, other requests keep going, and the teardown keeps the buffers a transfer may still write into.
Three things on the data-worker side need a fix before this merges:
- An output-read failure releases the client but never stops the request. The API server sets
req.errorand parks the rid when the error chunk arrives, and nothing callsabort_request, so the workers keep generating until the walk ends. Every later output of that request is read, dropped, and answered with one more error chunk plus a traceback. In the injection run the data worker logged one traceback per failed output read (5 for 5), and nothing told the conductor to stop those requests. Callingabort_requestwhen the error chunk is recorded (or sending ABORT_REQUEST from the data worker) drains it at once. - Ready outputs of a failed request still go out after the error.
_process_read_tensorshandles failed reads first and then emits whateverreadyholds for the same rid from the same poll, and the chat SSE loop keeps yielding after an error event. With injection on, 2 of 16 streams carried two error events (orders "EEd" and "cEEd" for content, error, done); I did not catch a content chunk after an error in that sample, but the code allows it. Skippingready[rid]for rids with a failed read and ending the stream at the first error fixes both the order and the double error. - The quarantine has no accounting and no bound. Each failed Mooncake read moves the edge's buffers, GPU memory on the worker, into a list that is never trimmed, and the only trace is one warning per event. Under the #332 condition this grows until a restart with nothing a monitor can read: with the RDMA registration failing, the worker parked 1,229 reads and 117 MB in two and a half minutes, 1.5 MB per image request. A bytes and reads counter in the
--log-statsoutput and a byte budget past which the process reports itself unhealthy would make the tradeoff visible. Asking Mooncake whether the batch drained, so the buffer can be freed later, is the longer-term fix.
Notes, none blocking:
- SHM read failures still raise inside
start_read_tensorsand fail every in-flight request on the worker through the main-loop handler. Not a regression, but the title reads as if any read failure is per request now. _fail_requestlogs the same traceback once per tensor of a dropped read.get_ready_tensorsnow callsfuture.done()twice andresult()on waiting entries every poll. On the arena that is an extra event query per pending edge._quarantine_read_bufferspopsuuid_to_shard_dimbut notuuid_to_edge_name.- test/cpu-core.txt merges cleanly against main as of today.
| did_work = False | ||
| for request_id, graph_edges in self.tensor_manager.get_ready_tensors().items(): | ||
| ready = self.tensor_manager.get_ready_tensors() | ||
| for failed in self.tensor_manager.take_failed_reads(): |
There was a problem hiding this comment.
Two ordering problems here. The failed reads are handled first and then ready still holds this rid's outputs from the same poll, so they are emitted after the error chunk; and the chat SSE loop in serving_chat.py keeps yielding after an error event, so a stream can carry error, content, error. With injection on, 2 of 16 streams carried two error events ("EEd", "cEEd"). Skipping ready[rid] for rids with a failed read (and ending the stream at the first error) fixes both.
There was a problem hiding this comment.
Fixed in 5dc5ff4. Nothing goes out for a request after its error: outputs ready in the same poll are released without postprocessing, and outputs held for ordering are dropped. iter_result_chunks now ends the response at the first error. It no longer appends req.error after an in-band error, which was the second "E" in "EEd". On the H200 run with injection (before the #368 rebase), all 16 streams carried exactly one error and nothing after it (rcEd or rEd).
| """One error chunk per output tensor a failed read would have | ||
| delivered; each tensor's output slot is marked answered.""" | ||
| state = self.request_output_state.get(failed.request_id) | ||
| metadata = self.tensor_uuid_to_metadata_per_request.get(failed.request_id, {}) |
There was a problem hiding this comment.
This releases the client but the request keeps running: the API server sets req.error and parks the rid when the error chunk arrives (entrypoint.py 642-656) and never calls abort_request, so the workers generate until the walk ends, and every later output of the rid is read, dropped, acked and answered with one more error chunk plus a traceback from _fail_request. In the injection run the data worker logged one traceback per failed output read, and nothing told the conductor to stop those requests. Aborting the request when the first error chunk is recorded (or sending ABORT_REQUEST from here) drains it at once.
There was a problem hiding this comment.
Fixed in 5dc5ff4. On a request's first failed output read, the data worker aborts it through the client-abort path (ABORT_REQUEST plus its own drain). The conductor stops the walk at once, and outputs still on their way are acked unread. Later finish or abort notices for that request are ignored, so it gets one ABORT_REQUEST and one READS_DONE (tested across 19 orderings with REMOVE_REQUEST), and one traceback (a repeat failure is debug-only). On the H200 run before the #368 rebase, each of the 26 output-read failures logged one traceback and one self-abort. I didn't call APIServer.abort_request when the error chunk is recorded, because it pops the pending request and the client would lose the error before reading it.
| error = self._failed_read_errors.setdefault(ep.request_id, error) | ||
| self._failed_reads.append(FailedRead(ep.request_id, ep.graph_edges, error)) | ||
|
|
||
| def _quarantine_read_buffers(self, ep: FutureAndPointers) -> None: |
There was a problem hiding this comment.
This list is never trimmed and nothing counts it. Each failed Mooncake read parks the whole edge's buffers, GPU memory on the worker, still registered, and the only trace is the one warning per event. Under the #332 condition that grows until a restart with nothing a monitor can read; with Mooncake's RDMA registration failing on an IB node the worker parked 1,229 reads and 117 MB in two and a half minutes. A bytes and reads counter in the --log-stats output and a byte budget past which the process reports itself unhealthy would make the tradeoff visible; asking Mooncake whether the batch drained so the buffer can be freed later is the longer-term fix. Also uuid_to_edge_name is not popped here while uuid_to_shard_dim is.
There was a problem hiding this comment.
Fixed in 5dc5ff4. Kept reads and bytes are counted per process. The --log-stats report has a Read quarantine section on every request that finishes or fails, so it shows up even when every request fails. Past MSTAR_READ_QUARANTINE_BUDGET_MB (default 1 GiB per process), /health returns 503 naming the process, on both frontends. At your ~47 MB/min, the default trips in about 22 minutes. uuid_to_edge_name is popped too. Two things are left for #431: freeing the buffers once Mooncake confirms the batch drained, and checking the receive-side register_memory return (a failed registration is currently kept and counted like a posted read).
6ba48ab to
5dc5ff4
Compare
|
@merceod I rebased onto #368 and addressed your review; ready for another look. Thanks for the review and for reproducing #332 on the IB nodes. All three blocking items are fixed in 5dc5ff4, with replies on each thread. The PR grew for one reason: each submission now gets a unique internal id, so a reused request id can't reach another submission's state once failed requests are aborted and torn down asynchronously. You haven't reviewed that part yet. The description has a section on it, and clients still see only their own id. Your three scenarios on one H200, run before I rebased onto #368 (Qwen3.5-0.8B, against main
The first H200 run turned up a bug in my changes, now fixed in 5dc5ff4. The data worker's teardown could drop a request's entry while the API message loop was reading it. That raised KeyError and skipped that loop pass (3 times in one run; no client saw it). Those maps now share a lock, and one bad request, message or chunk no longer skips the rest of the pass. The rerun had 0 KeyErrors and the same client-visible results. On the notes:
Freeing the kept buffers and the receive-side |
5dc5ff4 to
f79fd67
Compare
|
@merceod could you take another look when you have time? All three points from your review are fixed, with a reply on each thread, and the branch merges cleanly with current main.
The one part you have not reviewed yet is the per-submission internal id (its own section in the description). #471 and #481 conflict with this PR, so landing it first means each of them rebases once. |
|
/review |
|
An earlier review run didn't complete; a later run finished: #419 (comment) |
|
/review |
|
/review continue |
Automated reviewThe PR stops one failed async (Mooncake TCP/RDMA) tensor read from stalling every other request. Findings: none. Points I traced and found correct:
Description:
Questions
Advisory, not a merge gate. Rules: |
It depends on which rank fails:
A CPU repro with the real Thanks for raising it. |
Measured on one H200 with the same SHM workload as before (Qwen3.5 0.8B, the 112 greedy chat requests), on the PR head and on its
All three rows are within run-to-run noise: at 64 the per-run medians span 139.6-158.3 req/s on this PR and 141.0-150.4 on
Added to the table, with the command: 5 and 31 pass on the head. |
f79fd67 to
0bac0c9
Compare
|
Hardening: a |
Live test: [API server][Data worker] Fail only the request whose async (Mooncake) tensor read failedWorks: with Mooncake TCP reads failing on purpose, only the affected requests fail, within tens of milliseconds, where base left most of the batch hanging until the client timed out. 1x H200 (fault injection, Qwen3.5 A/B), 3x H200 (Qwen3-Omni TP2) · a test hook outside the repo fails every 5th Mooncake read · PR head Claims
PerformanceQwen3.5 0.8B, ABBA, text tok/s per restart:
Other
|
merceod
left a comment
There was a problem hiding this comment.
Re-review of 38113a4 on klone H200s, one PR server and one main server per lane, Mooncake TCP with every 5th read made to fail for the failure runs, SHM for the load cells. These nodes run at 1560 MHz under load, so the absolute numbers are low on both trees alike.
The three things from round one are fixed, in the code and in the served runs.
- Nothing goes out after a request's first error. All 20 failing streams ended with one error and
[DONE], no second error and no content after it. Main timed out 16 of 16 streams. - A failed output read stops the request. 7 output failures gave 7 self-aborts, each with one drain and one teardown 4 to 28 ms later, and 55 input failures were each reported once. Main logged 18.4 M lines (1.4 GB) in 7.5 minutes, the PR 1,123 for the whole run.
- Kept buffers are counted and budgeted. 5 of 5 bad
MSTAR_READ_QUARANTINE_BUDGET_MBvalues fail at startup naming the setting, past a 104 B budget/healthis 503 on both frontends with the reason, failed/generatereports carry the quarantine block, every per-request map was empty after the TTL, anduuid_to_edge_nameis popped.
Load over SHM, main against main plus this PR, 3 launches per tree and 15 reps of 448 requests per cell: c=8 37.8 vs 39.4 req/s, c=16 58.1 vs 58.7, c=32 75.9 vs 79.1, c=64 91.2 vs 99.1, per-launch medians overlapping. The 112 greedy outputs were identical on every launch of both trees, and a fixed request_id at temperature 0.8 gave the same text on both. The Python runtime behaves like the Rust one under injection, and the abort storm left no failed reads and no kept buffers on either tree.
The 409 contract holds on the Python frontend and fixes real breakage on main (a duplicate cut the first stream after one line, 16 concurrent duplicates gave 15 empty 200s, drop and retry hung). What is left is the Rust frontend. Drop a stream and resubmit the same id at once and 9 of 15 rounds came back 200 with no output (comment inline). Main hangs there instead, so this turns a hang into a silent empty answer, and it contradicts "the Rust frontend behaves the same". The description lists the stale relay under not here (#519), so either land after #519 and rebase onto its per-submission tokens as you planned, or make the aborted relay skip its done. Requesting changes for that one thing.
Smaller points.
- The merge with #519 conflicts in 7 files and 29 hunks (entrypoint.py 17, bridge.rs 5), not the 4 files listed, and the two contracts differ a little. This PR blocks an id while the answer is unread and refuses at once. #519 frees it when the request finishes and waits 0.5 s first.
- A failed image read keeps about 600 KB here (55 reads, 33 MB), so the 1 GiB default trips after roughly 1,700 failed image reads rather than after the 22 minutes of text reads in the description (comment inline).
_client_facing_errorrewrites a verbatim internal id only. That is fine for#and regex characters (checked live), not for an id escaped inside a repr. Theoretical.
| if routes.contains_key(request_id) { | ||
| return Err(format!("request_id {request_id:?} is already in use")); | ||
| } | ||
| routes.insert(request_id.to_string(), tx.clone()); |
There was a problem hiding this comment.
Here is the drop-and-retry case. abort removes the route first (line 277) and only then tells the backend, the aborted submission's relay then sends done for the client id (rust_frontend.py:157), and that done closes the route the retry just opened here under the same id (lines 192-198). The retry's output arrives with no route and its client already has an empty 200. On klone 9 of 15 rounds (7 of 7 when the retry rides a keep-alive connection opened before the drop, 2 of 8 on a new connection); main hangs to the client timeout on the same path. Keying routes and aborts per submission, as #519 does with sub, fixes it, and so would skipping the done of a relay whose submission was aborted.
There was a problem hiding this comment.
Fixed in this PR, the same way as #519. Each submission now carries its own token (sub) between the Rust frontend and the bridge loop. Routes, replies and aborts go by it, and the 409 check stays per id. abort now sends the abort before it frees the id, so the backend sees it before the retry's submit. The aborted relay's done carries the old token and no longer finds a route. The bridge loop also binds each relay and abort to its own submission at submit time.
- New
test_a_retry_right_after_a_dropped_stream_is_served, with the realmstar-serverand bridge and a stub whose aborted stream ends only after the retry is submitted: 5 of 5 empty 200s before, 5 of 5 served after, and 600 of 600 in a longer CPU loop. - Three Rust unit tests for the route table, including an older submission's
donearriving after the retry. - H200, Qwen3.5 0.8B through
--rust-frontend, a streaming/generatedropped after its first line and the id resubmitted at once: 30 of 30 served (15 on a keep-alive connection, 15 on a new one), no 409. The previous commit gave 2 empty 200s in 30, both on keep-alive.
| - Optional client-supplied id; the server generates one when omitted. An id | ||
| whose earlier request still has a client waiting is refused with ``409`` | ||
| and that request is left alone; the id is free again once the earlier | ||
| client has its answer, aborts or disconnects. |
There was a problem hiding this comment.
True on the Python frontend (5 of 5 retries served at once on klone). Through the Rust frontend a retry right after a disconnect is the empty-200 case above, so until #519 lands this should say Python frontend, or the bridge needs the fix.
There was a problem hiding this comment.
With the bridge fix above the claim holds on both frontends, and docs/clients.rst now says so: the id is free again "on the default and the Rust (--rust-frontend) server alike, including a retry sent right after a disconnect". On an H200, immediate retries after a disconnect were served 30 of 30 through the Rust frontend and 10 of 10 through the Python one.
| - ``1024`` | ||
| - Per process: the buffers of a failed Mooncake/RDMA read are kept for | ||
| the life of the process (the transfer may still write into them), and | ||
| counted (reads, bytes). Past this many MB ``/health`` answers 503, on |
There was a problem hiding this comment.
Worth saying the per-read cost depends on the tensor. Text reads keep about 190 B each, but a failed image input read kept about 600 KB here (55 reads, 33 MB), so the default trips after roughly 1,700 failed image reads, not after the 22 minutes quoted for text.
There was a problem hiding this comment.
Added to docs/environment_variables.rst: what one failed read keeps depends on its tensor, about 190 B for a text read and about 600 KB for an image input read, so the default lasts about 1,700 failed image reads. The Resources section of the description says the same. No new bound: the budget and the /health 503 stay the limit, and freeing the buffers is #431. The injected run on the new head kept 9 worker reads in 1720 B, about 190 B each.
Great catch, done: every failed read goes to also loosened the wall-clock bound in |
merceod
left a comment
There was a problem hiding this comment.
Approve on cf3e84a. The push is the three things asked for and nothing else. Every failed read now goes to read_failed_rids and the failed_rids filter only gates the report, with a test that fails the request first, then its read, then sends a read-checked head. The async follower path builds unread rows through with_unread_rows, with the comment on why the push-back of a never-popped row stays harmless and a test for it. The per-row vote is flagged_on_any_rank in distributed/all_reduce.py, and the wall-clock bound is the no-failure run's time plus the collective allowance. Those three tests pass here in 24 s.
Gaurav-Shah05
left a comment
There was a problem hiding this comment.
@arun-elr you need to rebase before merge
…free its buffers once its transfers stop Fixes mstar-project#418. get_ready_tensors re-raised a failed async read on every poll and never reached the line that removes it, so every request behind it on that worker or data worker stalled until the failed request was torn down. A failed or cancelled read is now dropped and reported once through take_failed_reads. The worker fails its request through FAIL_REQUESTS; the data worker answers it with one error chunk in the output slot of each tensor the read would have delivered. Tensor reads go through TrackedMooncakeReader: one batch per tensor, read with batch_transfer_async_read and polled with transfer_check_status, with the sync read's retries and deadline. A failed read hands over the batch ids still running; the manager keeps the read's buffers (out of the store, so teardown cannot free them) and frees them once every id has reported COMPLETED or FAILED. A failed submit, a status other than those four, an error that names no transfers, or a failed unregister keeps the buffers for good. KV transfers keep the batched sync read.
…down read's outcome from its future
…na's event future offers no more
…so the read delivered before it counts once
…t the batch it targets On a loaded host the reader thread can submit after the test reports a status, which dropped the report and left the read waiting out its deadline.
…s initialized with get_local_topology builds its own engine, so without the TCP device it counted HCAs the engine filtered out, and retried a failed tensor more often than the sync read does.
The tensor manager already logs the read's error; the data worker logged a full traceback per tensor slot on top.
…artup get_local_topology starts a second Mooncake engine, about 1 s per manager, and the count is only used to retry a failed tensor.
… transfer still running reports it stopped
… rank 0 read its copy Two real ranks in their own processes over gloo, running the real Worker.run, scheduler, SHM tensor manager, Engine/StepRunner/KVManager and Qwen3.5's prepare_inputs. Checks behavior only: no hang and only the failed request fails, nothing of the failed row routed or sent, its pages never leasable with prefix_cache_tp, and the request beside it bit-identical to a run without the failure; for an image-grid row and a text-only row.
…nk, before preparing it Rank 0 schedules a lockstep step from its own copy of each input, so a follower whose read of one fails after rank 0 sent the step could never build the row, and the instance hung in its next collective. Rank 0 now marks a step whose rows take an input the followers read from elsewhere (ScheduleTPNode.read_check). A follower builds such a step as soon as each row's read landed or failed, with the rows it could not read in it and no inputs, so every rank has rank 0's rows. On the GPU thread, before prepare_inputs, one gloo all-reduce of a flag per row tells every rank which rows to take out; they go to failed_requests and fail through the conductor, as a per-row failure does. They are never prepared, admitted, committed or sent, so their outputs and pages never exist, and a prepare that runs a collective per row (Qwen3.5's vocab-parallel embedding) stays in step. A read-checked step is not pre-planned, so every rank admits it after the check.
…row vote - A follower's failed read of a request it already failed for another reason now goes to read_failed_rids too (the failed_rids filter stays on the report only). Before, a read-checked head naming it waited on the follower with no warning while rank 0 waited in the check's all-reduce. - The per-row vote moves to distributed/all_reduce.py as flagged_on_any_rank, next to _all_ranks_agree. - The async follower puts its unread rows back through with_unread_rows, as the serial path does; a comment and a test pin why pushing back an unread fresh row on a voided speculation stays harmless. - test_a_step_whose_every_row_failed_runs_empty_on_every_rank bounds its run time against a run with no failure instead of a fixed 8 s, which slow hosts went past on startup alone.
The Mooncake reader now reads with batch_transfer_async_read and polls transfer_check_status, so the fake engine mstar-project#623's slice tests use gets both: each batch is copied on submit and reports completed.
cf3e84a to
1628c8b
Compare
@Gaurav-Shah05 rebased on main, CI's green. one conflict w/ #623 in |
mstar-project#419 fails a request whose input or output tensor read failed. The output side goes through the data worker's _fail_request, which now sends an error id; the worker's input side sent the read's exception text, so it gets an error id too. Both log one line without a traceback, since the tensor manager already logged the read's error where it failed.
Conflicts: - mstar/worker/worker.py: _try_follow_speculation passes main's in_n (#419: unread rows run N's continuation too) as the continuing rids, plus this branch's fresh_final_edges. Adapted to main (no conflict markers): - test/modular/test_tp_read_check.py: the _assemble_speculation stub (#419) takes the new fresh_final_edges positional. - test/modular/test_speculation_fresh_final_stream.py: the follower fake gains what main's _try_follow_speculation now reads (pending.node_batch.commit_done, settle_tp_follow_delta(caught_up=), unread_rows, head.read_check). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tbox Conflicts: - mstar/communication/communicator.py: kept main's requested_transport() (#640) and this branch's make_communicator(nonblocking_send=) on top of it. - mstar/api_server/entrypoint.py: took main's check_request_id (#618, ASCII [A-Za-z0-9._:-] up to 128 chars), which also refuses the lone-surrogate ids this branch's UTF-8 check guarded the wire codec against; deleted the now-dead _is_utf8. Adapted to main (no conflict markers): - test/modular/test_request_id_reuse.py: dropped test_a_request_id_no_message_could_carry_is_refused; main's test_request_id_limit.py covers the submit_request refusal. - test/modular/test_outbox_communicator.py: the tensor-manager fake gains take_failed_reads, which main's preprocess loop now polls (#419/#629). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts: - mstar/model/base.py: kept this branch's ClientChunk; dropped PrefixStream, which main replaced with KVLabel/PrefixKeys (#832/#834). - mstar/api_server/data_worker.py: imports take ClientChunk plus main's PrefixKeys/keyed_labels; this branch's _load_media refactor kept with main's _prefix_streams -> _keyed_labels rename; _finish_reading drains when the rid is already draining (this branch) or still has a failed read's transfers in flight (main #419), docstring merged. - mstar/api_server/entrypoint.py: PendingRequest takes main's output_totals (#844), which counts outputs as they are accepted; this branch's `delivered` counter and _count_outputs, the same fix for the same leak, are deleted. Stream drain keeps this branch's last_activity read with the swap-out; the done path clears req.chunks as main does. /generate/ws: main's empty-id-as-None and check_request_id (#618) move into the receive loop beside this branch's isinstance check, since live-call routing reads the id there; the error reply keeps this branch's call/turn release with main's client_error_text scrubbing (#646), under this branch's `payload` name. - mstar/conductor/conductor.py: imports from both; worker graphs from main's validate_deployment (#640), then this branch's _find_client_streams. - test/modular/test_kv_prefix_request_keys.py: kept this branch's get_partition_topology stub and main's kv_labels. Adapted to main (no conflict markers): - test/modular/test_client_stream_api.py: stub renamed prefix_key_streams -> kv_labels; the tensor-manager fake gains take_failed_reads; the bad-append test expects #646's "internal error" text in place of the model's exception; test_a_streaming_request_lets_go_of_the_chunks_it_has_delivered dropped, as main's test_api_result_delivery.py covers the release and the totals. - test/modular/test_request_id_limit.py: the fake server gains takes_stream_input, submission_key and abort_submission, which this branch's /generate/ws reads. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ake fails no reads - process_prompt returns a ProcessPromptOutput since GLM-5.2 (#387) and GLM-5.3 declare their KV layouts, so the rendered-chat test reads .new_input_tensors. - main's data worker asks the tensor manager for reads that failed (#419, take_failed_reads); the usage test's fake has none.
#651 lowers a (node, walk)'s batch cap after refused admits (MicroScheduler.batch_limits and batch_ceilings), applied each pass where the caps are read. On this stack the caps live in SchedKeys and, for the Rust core, in the runtime, configured once: - SchedKeys.limits holds each key's effective limit (the smaller of the two) and walk_caps / max_batch_size apply it, as #651's _walk_caps did; - the Rust core keeps its configured caps and lowers them on sched_set_limit, which RustSchedCore.set_limit calls whenever MicroScheduler changes a limit; - could_refill reads the core's live backlog and the runtime's ready scan. The scheduler parity test now limits, caps and lifts at random too; with the Rust core ignoring the limits, 54 of its 195 cases fail. #651's and #419's tests take the runtime off the scheduler, where this stack's test_micro_scheduler helpers keep it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Handles TP>1 itself now, so #754 isn't needed (closed).
Fixes #418
What does this PR do?
On Mooncake TCP or RDMA, one failed async tensor read stalls every request on its worker or data worker:
get_ready_tensorsre-raises it on every poll and never removes it, so the requests behind it wait for its teardown (client abort or the request timeout), logging the same error each time (#332). This PR makes a failed read fail only its own request, and frees its buffers once Mooncake says the transfer stopped.get_ready_tensorsdrops a failed or cancelled read and reports it once throughtake_failed_reads(). The worker fails the request withFAIL_REQUESTS; the data worker answers with one error chunk in the output slot of each tensor the read would have delivered.TrackedMooncakeReader: one async batch per tensor, polled withtransfer_check_status, with the sync read's retries and deadline. A failed read hands over the transfers still running; its buffers stay out of the tensor store until each one reports COMPLETED or FAILED, then they are unregistered and freed. Any other outcome keeps them for good. READS_DONE waits for those transfers too, so the producer keeps the memory they read.ScheduleTPNode.read_check), a follower builds it w/ the rows it couldn't read left empty, and beforeprepare_inputsone gloo all-reduce of a flag per row takes those rows out on every rank. They're never prepared, committed or sent, so nothing of them reaches the client, the Talker or the prefix cache, and the request fails through the conductor like above.How was it tested?
On 1628c8b (rebased onto main 3a4aedb):
ruff check .clean; CPU Core (theci.ymlpaths incl. the TP lockstep sim) 6,595 passed, 1,189 skipped, 6 failed, the same 6 as main 3a4aedb (6,531 passed, 1,187 skipped), all from packages missing on this host (ftfy,soundfile/torchcodec,prometheus_client). On the previous head cf3e84a merged into main 3d764d0: CPU Core (plustest/rust) 6,936 passed, 1,280 skipped, 9 failed (main: 6,876 passed, 1,278 skipped, the same 9 failed on this host; list below); TP lockstep sim 11 passed (7 on main).Tests in detail
test/modular/test_failed_tensor_read.py(new, 31 tests): 29 passed, 2 skipped on CPU; on an H200 both real Mooncake TCP cases pass, incl. a 2 GiB transfer still running after its read failed. The 6 read-fix tests fail on main.test/modular/test_tp_read_failure_acceptance.py(new, 7 tests): two real TP2 ranks in their own processes over gloo runningWorker.run, the scheduler, the SHM manager, the engine and Qwen3.5's realprepare_inputs; the follower'simage_grid_thw(or text) read fails after rank 0 read its copy. No hang, only that req fails, none of its outputs leave either rank, its pages are never leasable w/prefix_cache_tpon, and the req beside it is bit-identical to a run w/o the failure. All 7 hang or fail on a2c97da.test/modular/test_tp_read_check.py(new, 22 tests): the same two ranks w/ an embedding that all-reduces insideprepare_inputs(as Qwen3.5's vocab-parallel one does) and w/ a step where every row failed; TP4 and TP2 x SP2 over four gloo ranks; the follower's serial andMSTAR_TP_ASYNC_SCHEDbuilds; rank 0's marking; a read that fails after its request already failed for another reason. All 22 fail on a2c97da, and each fails when the logic it covers is removed (26 mutations).test/modular/test_tp_async_lockstep_sim.py: 4 model checks (tp_read_check_sim.py) over every interleaving of the read outcome, scheduling and the step.Served by @merceod on 2x H200, TP2 Qwen3.5-0.8B over Mooncake TCP (comment): 11 injected follower-failure cells, only the target failed and main wedged; cost at c32 TTFT +2.7%, req/s -5.8% w/ ranges overlapping, c8 within noise.
Known behavior: a slow follower read stalls the whole instance in the all-reduce until it lands or fails (up to
MC_TRANSFER_TIMEOUT), the same as main today. Read-checked steps are never pre-planned.Qwen3-Omni over Mooncake TCP, main vs this PR on one H200,
qwen3omni_2gpu's layout (the Talker reads the Thinker's states from another worker every step) with both workers on GPU 0; median of 3 fresh servers per arm, 0 errors in 360 requests per arm:Notes on the numbers
The two arms' min to max ranges overlap in every cell; c8 audio s / s is 1.5% lower w/ requests per second the same, since this PR's sampled replies ran about 0.3 s shorter on average. With one injected Talker read failure per server, this PR failed only that request (in-band error) and the 20 others completed; main hung it until the 120 s timeout and failed or hung the 12 beside it.
Charts
e2e matrix, Mooncake TCP models (main vs f78bfbc)
The e2e matrix's
tag:mooncakemodels on one H200, base and head back to back, 3 passes per point, 1 session per arm. These serve every node in one worker, so they cover the data worker's reads, not worker-to-worker hops.orpheus_mooncakedid not start on either arm (its gated weights did not download), so it has no row.CPU suite details
CPU Core paths from
ci.ymlplustest/rust,-n 4,OMP_NUM_THREADS=2. The 9 that also fail on main 3d764d0 on this host:test_lingbot_world_shell.py(3, noftfy),test_chatterbox_model.py(2, nosoundfileortorchcodec) andtest_serving_metrics.py::test_refusals_count_from_when_metrics_are_turned_on(noprometheus_client), all of which CI installs;test/rust/test_runtime_lockstep.py::test_only_the_tp_leader_counts_new_tokens[1](the Rust and Python runtimes disagree w/ this host's prebuiltmstar_rust); and two eval signal tests that time out on a loaded host. On the H200:test/distributed/test_tensor_transport.py12 passed with Mooncake TCP.Served run: harness and commands
One spot H200 (no 8-GPU capacity for two physical GPUs), Mooncake 0.3.13.post1 over TCP on loopback (
MC_FORCE_TCP=1), torch 2.9.1+cu128, Rust graph runtime (the default). Each arm imports its own tree (PYTHONPATH, checked in each server's log).python -m mstar.cli.main serve qwen3_omni --config qwen3omni_2gpu_1dev.yaml --tensor-comm-protocol TCP --enable-metrics, where the config isconfigs/qwen3omni_2gpu.yamlplusrank_devices: {1: 0}and the colocated config's KV sizes (thinker_kvandtalker_kv256 pages) so both workers fit on one GPU./v1/chat/completionswithmodalities: [text, audio], temperature 0,max_tokens96, eight fixed prompts in order; warm-up waves of 16 and 8, then 40 requests at c8 and 80 at c16. Replies average 68 tokens and 19 to 20 s of audio on both arms.sitecustomizeshim on the Talker's worker replaces the nextthinker_statesread's future with one that fails 1 s later, armed right before one target request; 12 more requests 0.5 s later, then 8 after those finish, all with a 120 s client timeout. On [Mooncake][Worker] Fail only the request whose async tensor read failed, at TP>1 too #419 the shim also logs each failed readtake_failed_reads()hands back (86 for the target request, its later stream chunks) and the transfer-metrics series show TCP reads on both arms.Not tested
transfer_check_statusreports for a failed RDMA read is untested: none of our VMs has an InfiniBand NIC (the 1-GPU presets have none). In Mooncake 0.3.13.post1's source, a task reports COMPLETED or FAILED only once every slice is accounted for (success + failed == slice_count, in both the RDMA and TCP transports), and the release frees only on those two values, so an unexpected RDMA status keeps the buffers rather than freeing them.qwen3omni_2gpuon two physical GPUs. 8x H200 spot had no capacity, so the same two-worker layout ran on one GPU; the Talker's per-step Mooncake read and its GIL use are the same.qwen3_5_9b_tp2, c64,qwen3omni_full_tp2andbagel_xpu_cfg_tp2(both checked on every step) have no served numbers.MSTAR_TP_ASYNC_SCHED=1cost. The served failure cells ran w/ it on and off; the cost pair didn't (main's async launch failed in FlashInfer's multicast all-reduce setup).Depends on and merge order
#629 is stacked on this. #471 (merged) added the abort of a request whose output failed and the cut of a stream at its first error; a failed read here goes through the same
_fail_request, which aborts the request's walk once.Design
transfer_check_statuslooks at a batch's first task only, and frees the batch on COMPLETED (1) or FAILED (-1); it returns -2 on a slice timeout without freeing, else 0. Mooncake's sync batch read reports FAILED for the whole batch as soon as one task fails, while the other tasks may still be moving, and on its timeout frees nothing it can name. So only a per-tensor batch can say when every transfer into a buffer has stopped.batch_transfer_sync_readandget_batch_transfer_statusreturn only 0 or -1, and on a failure callfreeBatchIDthemselves, which deletes the batches whose tasks finished and keeps, without saying which, those still running.AsyncReadError(running, unknown); from then on only the manager polls the ids it handed over, at the top ofget_ready_tensors(no timer). A read torn down while still running keeps its future, and the manager adopts its ids only once the reader is done, so no id is polled by two threads or after Mooncake freed it.AsyncReadError, or a failed unregister: the buffers move to a list that is never freed, with one warning.has_inflight_readscounts a live request's failed reads whose transfers are still running, and the data worker's drained finish checks it, so READS_DONE (after which the producer frees its source) waits for them.done()andresult()only, which is all the SHM arena's_CudaEventFutureoffers.prepare_inputs. Qwen3.5'sprefill_visionembeds each text part insideprepare_inputsw/ a vocab-parallel embedding, an all-reduce per part. A rank that prepares a row the others don't runs collectives w/ no partner, so the row has to leave every rank before any prepares it (test_a_prepare_that_runs_a_collective_per_row_stays_in_step).MSTAR_TP_ASYNC_SCHED. A read-checked step isn't pre-planned, so every rank admits it after the check.get_local_topologyneeds.Performance (CPU, Mooncake TCP loopback)
Out-of-repo script: two TCP managers in one process (
localhost,lo), host buffers; latency isstart_read_tensorsto the read's future done, 300 reads per shape; "torch loop" is a main thread running smalltorch.addops while another thread reads back to back, as a fraction of its idle rate. Three runs each, main fa37d22 vs this PR before its rebase onto b4297bb (the commits since touch only failed and torn-down reads, the drain check and when the retry count is taken). The host ran other jobs, so read these as relative numbers.A 4 KiB read pays about 0.09 ms at p50 and a 4 MiB read about 0.3 ms (8 percent), about one poll interval plus a GIL wait; 1 MiB and 8 x 512 KiB overlap with main. Beside 8-tensor reads the main thread runs about 0.1 slower on average, with overlapping ranges. Served, this does not show up in the Qwen3-Omni numbers above, where the Talker polls every step.
Tests in detail
test/modular/test_failed_tensor_read.py:done()andresult(), like the SHM arena's_CudaEventFuture, is polled like any other.Changed:
test_empty_tensor_transport.pyandtest_mooncake_flat_layout.pyfake the async read API;test_worker_drain.py's drained-finish test now has no read in flight;test_transfer_metrics.pyexpects a failed read to be dropped and reported, not raised; eight stub tensor managers gaintake_failed_reads.Earlier served runs
Qwen3.5-0.8B over Mooncake TCP on 2 x H200, on 4fbc57e (the production code before the two follow-ups), in a review: against main at c8, c32 and c64 with overlapping TTFT and ITL ranges and 0 errors, and injected read failures with 76 buffer keeps matched by 76 frees. The H200 runs with injected Mooncake TCP read failures in the earlier review threads (first review, later rounds) measured an earlier, wider version of this PR, which also carried the changes now in #471 and the read-buffer budget this version replaces with the release.
Checklist
ruff check .passesAI assistance: written with AI coding agent