Skip to content

update(distributed): transport and remote pipeline robustness follow-ups - #2720

Merged
inureyes merged 4 commits into
mainfrom
update/issue-2712-distributed-robustness-followups
Oct 28, 2026
Merged

inureyes merged 4 commits into
mainfrom
update/issue-2712-distributed-robustness-followups

Conversation

@inureyes

Copy link
Copy Markdown
Member

Summary

Implements the six transport and remote pipeline residuals of #2712 in one PR, each wired into the live path (stage service, coordinator exchange, connection pool, TCP transport, router startup) and each backed by a test that fails on 752db4f.

  1. Stage errors reach the coordinator. RemoteStageService::handle_activation reports an execution failure, or a middle stage's failed downstream forward, as RemoteStageResponse::Error { nonce, request_id: Some(..), state: Some(..) } to the coordinator (via send_run_error); without a reply nonce it logs one warn and sends nothing. The upstream nonce: 0 send is gone. ExchangeMatch gains error_peers (&self.stage_peers[..] for RunEntry, from_ref(peer) otherwise) and requires the Error to name the run's request_id; forward_sequence now fails in well under 1 s instead of after stage_timeout.
  2. ServerAtCapacity backoff. connect_with_retry retries the at-capacity verdict with the refused-connect backoff and returns it, type preserved in the chain, only when the budget is spent; other handshake failures stay terminal. The warn goes through handshake_warn_allowed with the new AtCapacity class.
  3. Warning budget. HandshakeWarnClass::RouterDrop (router-drop) and AtCapacity (at-capacity), one window slot per class, router_front.rs uses RouterDrop; WarnDecision::previous_window_ended_ago feeds the summary N cluster warnings ({class}) were logged at debug during the 60s window that ended Ms ago.
  4. Unknown authenticated kind is logged. tcp_frame::UnknownAuthenticatedKind { kind } (same Display text), returned by read_frame, classified as diagnostic by handshake_warn_class, and handle_connection logs Closing connection from {peer}: {err} (version skew?) through the budget.
  5. Pool reuse and accounting. ProtectedStream::probe_idle() -> IdleProbe { Idle, PendingData, Closed } (peer_closed() kept); acquire reuses only Idle and returns a PooledStream guard (Deref/DerefMut, Arc<AtomicUsize> per peer, decrement on drop) so failed and cancelled sends release their slot; PoolConfig::router() (2 retries from 50 ms, 2 s connect) is used by the router transport in startup.rs.
  6. TCP rpc_call round-trips. The response is read on the pooled connection the request went out on; the connection is released only after the matching response and dropped on timeout or error. rpc_pending and the RpcResponse arm of the accept loop are removed (an RpcResponse on an accepted connection is logged at debug).

Docs: docs/distributed.md (capacity backoff, four classes, summary wording), docs/CONTINUOUS_BATCHING.md (router pool profile), and the module docs of remote_service, connection_pool, handshake_warn, tcp_frame, tcp_transport, cluster_handshake. No env vars changed.

For #2714

Item 1's addressing goes through two helpers on RemoteStageService in src/distributed/pipeline/remote_service.rs: fn reply_target(&self, _request_key: &str) -> String (today coordinator_identity with the coordinator.configured() fallback; the Logits RunResult uses it too) and async fn send_run_error(&self, nonce: u64, request_key: String, message: String) -> Result<()>, called from report_activation_failure. Switching reply_target to the per-sequence owner needs no call-site changes.

Failing-first evidence

The behavioral tests were committed first (d7fd980) and run against the unchanged code (752db4f plus only the test files): 11 of 13 failed, each with the expected failure mode.

Test Failure on 752db4f
remote_service::tests::final_stage_execution_error_goes_to_coordinator stage must answer within 5s: the coordinator receives nothing (the Error went upstream with nonce 0)
remote_service::tests::final_stage_drops_execution_error_without_reply_nonce expect_silence(upstream) fails: the upstream received the nonce 0 Error
runtime::correlation_tests::error_from_last_stage_fails_forward_promptly elapsed assertion: the exchange waited out the 5 s timeout
runtime::correlation_tests::error_from_last_stage_with_wrong_{nonce,request_id}_is_dropped pass on both sides (main drops every non-addressed Error); they guard the new acceptance rule
tcp_transport::tests::robustness::server_at_capacity_is_retried_with_backoff cluster handshake with 127.0.0.1:N failed: returned at once, no retry
tcp_transport::tests::robustness::server_at_capacity_exhausts_retry_budget elapsed under 60 ms: no backoff spent
tcp_transport::tests::robustness::failed_send_releases_active_slot total_active stayed 1
tcp_transport::tests::robustness::cancelled_send_releases_active_slot total_active stayed 1
tcp_transport::tests::robustness::rpc_call_round_trips_over_authenticated_tcp RPC call timed out
tcp_transport::tests::robustness::timed_out_rpc_call_drops_its_stream total_idle was 1: the stream was pooled before the wait
tcp_transport::tests::frame_protection::authenticated_unknown_kind_is_logged_before_close zero close lines captured
connection_pool::tests::acquire_drops_idle_connection_with_pending_data second accept timed out: the idle connection was reused

Tests that use a new API cannot compile on 752db4f; git grep of the symbol on that commit returns nothing, which is the evidence: handshake_warn::tests::router_drops_do_not_suppress_diagnostic_warnings and rollover_summary_reports_the_window_age (HandshakeWarnClass::RouterDrop, WarnDecision::previous_window_ended_ago), classifies_unknown_authenticated_kind_as_diagnostic and frame_protection::read_frame_reports_unknown_authenticated_kind_typed (UnknownAuthenticatedKind), connection_pool::tests::pool_config_router_values and router_pool_config_fails_fast (PoolConfig::router), and the probe_idle assertions added to peer_closed_probe_is_non_consuming (IdleProbe). The behavioral side of each is covered by the table above (items 3 and 4) or by disaggregated_router_e2e (item 5's failover).

Verification

GB10 (spark-102), --profile test-fast --features cuda,test-utils, built outside the lock and run under gpu-lock run --tag issue-2712 with MLXCEL_REQUIRE_MODELS=1 MLXCEL_SDPA_DETERMINISTIC=1 -- --test-threads=1:

Command Result
cargo fmt --check clean
cargo clippy --profile test-fast --features cuda,test-utils -p mlxcel --lib --tests -- -D warnings clean (one deprecated TcpStream::set_linger in a new test was replaced by socket2::SockRef::set_linger on the way)
cargo test ... --lib distributed:: 1513 passed, 0 failed, 4 ignored (42 s)
cargo test ... --lib server::router_front 35 passed, 0 failed
cargo test ... --test zero_config_cluster_init -- --include-ignored 7 passed, 0 failed
cargo test ... --test pipeline_remote_real_models -- --include-ignored --skip glm_moe_dsa 10 passed, 0 failed, 1 filtered (82 s)
cargo test ... --test pipeline_server_remote_real_models -- --include-ignored --skip glm_moe_dsa 7 passed, 0 failed, 1 filtered (61 s)
cargo test ... --test pipeline_ci_multi_stage_real_models -- --include-ignored --skip glm_moe_dsa 4 passed, 0 failed
cargo test ... --test disaggregated_serving_e2e -- --include-ignored 2 passed, 0 failed
cargo test ... --test disaggregated_router_e2e -- --include-ignored 7 passed, 0 failed (53 s)

The new tests (25 selected by name, including the adjusted full_connection_cap_reports_capacity_to_the_client, panicking_rpc_handler_releases_its_connection_slot and peer_closed_probe_is_non_consuming) pass on the branch; greedy parity on the real-model targets is unchanged against #2708.

glm_moe_dsa is skipped as the issue allows (418.6 GB checkpoint). make verify-env-var-docs not run: no env var added or changed.

Closes #2712

…-ups

Behavioral tests for the six robustness residuals of #2712 that compile against the unchanged code, committed before the fix so the failure mode of each is on record. On 752db4f, 11 of the 13 fail: a final-stage execution error ends in a coordinator timeout instead of an Error reply (items 1), a capacity refusal is returned at once without backoff (item 2), an authenticated unknown frame kind closes the connection without a log line (item 4), failed and cancelled sends leave the pool's active count at 1 and an idle connection with pending input is reused (item 5), and TCP rpc_call never receives its response and pools its stream after a timeout (item 6). The two negative correlation variants (an Error with a wrong nonce or request id is dropped) pass on both sides; they guard the new acceptance rule.

The transport-level tests live in a new child module of the tcp_transport tests (tcp_transport_robustness_tests.rs). full_connection_cap_reports_capacity_to_the_client gives client B a zero retry budget so it keeps testing the immediate verdict once item 2 adds backoff.

Refs #2712
Six non-security residuals of #2700, #2705 and #2708 on the authenticated transport, the remote pipeline and the disaggregated router, all failure latency, log visibility or API correctness.

- Remote pipeline: an execution failure on a non-entry stage, or a middle stage's failed downstream forward, is reported to the coordinator as the Error of the RunEntry exchange whose nonce the activation carried (send_run_error, addressed through reply_target), and the coordinator accepts an Error from any configured stage peer when the nonce and request id match (ExchangeMatch::error_peers). forward_sequence fails with the stage's message at once instead of waiting out stage_timeout; the upstream nonce 0 Error that nothing consumed is gone.
- Connection pool: a ServerAtCapacity handshake verdict is retried with the refused-connect backoff and returned, type preserved, only when the budget is spent; every other handshake failure stays terminal. The warn goes through the budget (new AtCapacity class).
- Warning budget: RouterDrop and AtCapacity classes with their own windows, so router drops and capacity refusals cannot spend the Diagnostic budget; the rollover summary states when the suppressing window ended instead of "in the last 60s".
- TCP frames: the authenticated unknown kind is the typed UnknownAuthenticatedKind, classified as diagnostic, and handle_connection logs "Closing connection from {peer}: ... (version skew?)" through the budget instead of closing silently.
- Pool reuse and accounting: probe_idle replaces the boolean probe and acquire drops an idle connection with pending input; acquire returns a PooledStream guard whose drop gives the active slot back, so failed and cancelled sends no longer leak it; PoolConfig::router() (2 retries from 50 ms, 2 s connect) is used by the router transport in startup so a refused connect fails over within about 150 ms instead of 3.1 s.
- TCP rpc_call reads its response on the connection the request went out on, releasing it only after a matching response and dropping it on timeout or error; the rpc_pending map and the RpcResponse arm of the accept loop had no producer and are removed.

Docs: distributed.md (capacity backoff, four warning classes, summary wording), CONTINUOUS_BATCHING.md (router pool profile), module docs of remote_service, connection_pool, handshake_warn, tcp_frame, tcp_transport and cluster_handshake.

Verification on GB10 with --profile test-fast --features cuda,test-utils under gpu-lock: fmt, clippy on --lib --tests with -D warnings, the distributed:: and server::router_front lib modules, zero_config_cluster_init, pipeline_remote_real_models and pipeline_server_remote_real_models (--skip glm_moe_dsa), pipeline_ci_multi_stage_real_models, disaggregated_serving_e2e and disaggregated_router_e2e, all with --include-ignored and --test-threads=1.

Closes #2712
@inureyes inureyes added status:review Under review type:enhancement New features, capabilities, or significant additions priority:medium Medium priority area:inference Generation, sampling, decoding (incl. speculative, DRY) labels Oct 28, 2026
@inureyes

Copy link
Copy Markdown
Member Author

Implementation Review Summary

Intent

Fix the six #2712 robustness residuals on the cluster transport, remote pipeline and router, each wired into the live path with a test that fails on 752db4f.

Findings Addressed

None needed: no CRITICAL or HIGH findings at 89f5c19.

Remaining Items

  • remote_service.rs:617-621: on a non-entry stage, failures before the downstream send (no downstream peer, sequence_length, RequestId parse, serialize_mlx_array) still propagate with ?, so they end in the coordinator's stage timeout and leave the request's caches in place. The issue only names the execution error and the failed send_activation, so this is outside its scope (LOW).
  • connection_pool.rs:279: PooledStream does not record its peer, so release(other_peer, guard) would return the connection to the wrong idle list. The active count stays correct, and every caller passes the same key it acquired with (LOW).
  • tcp_transport.rs:315: after the retry budget is spent, Transport::connect logs the "Failed to pre-connect" warning without the rate limiter. This happens once per connect call, not once per attempt (LOW).

Verification

  • All stated requirements implemented (items 1 to 6, per-item tests present and asserting what the issue specifies)
  • No placeholder/mock code remaining
  • Integrated into project code flow (stage service, coordinator exchange, router startup PoolConfig::router(), TCP transport)
  • Project conventions followed
  • Existing modules reused where applicable
  • No unintended structural changes (edits stay inside the item regions; the fix(distributed): tighten tensor decode limits to the model shape #2716 deserialize_* lines and the fix(server): disaggregated router parity residuals from #2700 #2715 router_front.rs regions are untouched)
  • Tests pass: cargo check and clippy on --lib --tests (-D warnings), cargo fmt --check, plus lib modules distributed::connection_pool (10), distributed::tcp_transport (38), distributed::handshake_warn (6), distributed::tcp_frame (5) and distributed::pipeline:: (368), all under gpu-lock with --test-threads=1

@inureyes

Copy link
Copy Markdown
Member Author

Security and Performance Review

Reviewed at 89f5c19: no CRITICAL, HIGH or MEDIUM findings, so nothing was committed.

Checked and clean

  • RunEntry Error accepted from any stage peer: it still needs the random per-exchange nonce and request_id == Some(<run id>). Only nonce holders can match, and each could already stall the request into the same end (server_runtime.rs:154 panics on any forward error, timeouts included). The Error's state and stage_index are ignored, and every entry-stage RunEntry error already carries Some(request_id).
  • PooledStream pairs one increment with one decrement per guard, covering failed, cancelled and released sends. probe_idle is a non-blocking MSG_PEEK.
  • UnknownAuthenticatedKind logs the bound SocketAddr identity and a u8, so the line cannot be injected into. It needs the secret and is rate limited under Diagnostic.
  • Removing rpc_pending also closes a hole: any authenticated peer could resolve another peer's pending RPC with a guessed sequential id.

Remaining (LOW, not fixed)

  • connection_pool.rs:338, :394: at-capacity retries have no jitter, and each one is a full connect plus handshake. N concurrent acquires against a full peer make up to 6N handshakes (3N with the router profile), each writing an unrated server warn (tcp_transport.rs:614). The peer must be authenticated and the total is bounded. Suggested fix: rate limit that warn under AtCapacity and add jitter.
  • tcp_transport.rs:414: rpc_call reads with the 256 MiB max_frame_size. The length header is unauthenticated until the body is opened (tcp_frame.rs:225), so an on-path party can force one large allocation per in-flight call, as on accepted connections today.
  • remote_service.rs:737: the no-nonce failure warn is not rate limited, like the logits-drop warn at :667. Only the authenticated upstream can trigger it.

Verification

  • cargo fmt --check and clippy --lib --tests -D warnings are clean.
  • Under gpu-lock with --test-threads=1: distributed::{connection_pool,tcp_transport,tcp_frame,handshake_warn,pipeline::} ran 427 tests, all passed.

@inureyes inureyes added status:done Completed and removed status:review Under review labels Oct 28, 2026
@inureyes
inureyes marked this pull request as ready for review October 28, 2026 13:10
@inureyes
inureyes merged commit dbae45f into main Oct 28, 2026
31 of 32 checks passed
@inureyes
inureyes deleted the update/issue-2712-distributed-robustness-followups branch October 28, 2026 13:10
inureyes added a commit that referenced this pull request Oct 28, 2026
)

Pipeline tensor decodes were bounded only by the sender's own header, so an authenticated peer could force about 4 GiB of allocation from a ~21 MiB frame and hand the executor any shape. Receivers now state what they expect, checked right after `TensorHeader::decode` and before the payload is sliced, copied, or allocated.

## What changed

- **Item 1.** `DimBound`, `ShapeBound`, `TensorDecodeLimits` and `deserialize_tensor_with_limits` in `tensor_serialize.rs` (re-exported); `deserialize_tensor` wraps it unrestricted. Shape products use checked u64 arithmetic. `deserialize_activation` always refuses COMPRESSED and QUANTIZED. `wire_tensor.rs` adds `MAX_STAGE_TOKENS_PER_FRAME` (1 << 20), `StageWireLimits` and `deserialize_wire_tensor_with_limits`. `RemoteStageService::load` precomputes the hidden-state, input-id and mask limits once (`PeerDecodeLimits`) and uses them at the four peer-facing decode sites. `RemotePipelineRuntime` decodes logits under `logits(input_seq_len)` with its `vocab_size`, keeps `validate_logits_shape`, and now cleans up the request if that decode fails. `reassemble_layer_payload` takes `max_layer_bytes` and rejects a larger `original_bytes` before decompressing.
- **Inbound width.** `StageExecutor`/`FamilyStageExecutor::inbound_activation_width` default to `None`. `LoadedStageExecutor` stores the family override or `loading::read_hidden_size` (`text_config` first) and fails the load if both are absent. Gemma 4 (Gemma4 and Gemma4Vlm) overrides it through `Gemma4StageModel::inbound_activation_width`: `hidden_size + (num_hidden_layers - layer_range.start) * hidden_size_per_layer_input` when that field is non-zero, else `hidden_size`. `unpack_stage_hidden` slices with the same helper, so the formula exists once.
- **Family audit.** Every other adapter (llama, mistral, mixtral, qwen3, qwen35 incl. VLM, qwen3_next, gemma3, glm4 family, glm_moe_dsa, deepseek_v3, gpt_oss, jamba, llama4, nemotron_h) passes plain `[1, L, hidden_size]` across the boundary; none packs extras. VLM and Llama 4 configs nest `hidden_size` under `text_config`, which `read_hidden_size` reads first. All local checkpoints of these families resolve a width.
- **Item 2.** `LayerTransferHeader.original_bytes`; `num_elements = raw_bytes.len() / 2`; Int8/Int4 refuse an odd layer; level None bounds at `original_bytes`; Int8/Int4 require `original_bytes == original_num_elements * 2`.
- **Item 3.** Level None must decode to exactly `original_bytes`, compressed or not.
- **Item 4.** `[]` is a 1-element scalar via `TensorHeader::shape_num_elements`, shared with `validate_data_size`; the 18-space message is fixed.
- **Item 5.** Decode works from a borrowed `Cow`; only an unflagged payload is copied.
- **Item 6.** `MAX_DECODED_TENSOR_BYTES: u64` plus `max_decoded_tensor_bytes()`. A 32-bit build cannot be run here (mlxcel-core does not build for 32-bit); the constant now const-evaluates there.
- **Docs and existing tests.** `docs/distributed.md` describes the receive-side bounds. `forward_rejects_logits_with_wrong_vocab_dimension` now expects the header-level vocab rejection, and `validate_logits_shape` gained a case for a sequence length strictly between 1 and the input length.

## Failing-first evidence

Every new test was shown to fail by a mutation check on this branch (reverting the specific guard to main 752db4f's behavior), because each targets an API main lacks.

| Test | Mutation | Failure |
|---|---|---|
| `limits_refuse_flags_dtype_and_shape_before_the_payload`, `hidden_states_shape_is_checked_before_the_payload`, `entry_tensor_limits_bound_ids_and_masks`, `logits_limits_bound_sequence_and_vocab` | drop `check_header` | "buffer too short" instead of the limit |
| `activation_decode_refuses_compressed_and_quantized_frames` | stop forcing flags off | LZ4 expansion error, no flag named |
| `run_entry_with_out_of_bounds_ids_answers_limit_error`, `activation_outside_stage_limits_never_executes` | unrestricted limits at the call sites | no limit error / executor ran |
| `per_layer_input_inbound_width_matches_upstream_packing` | width ignores per-layer inputs | 256 != 304 |
| `read_hidden_size_reads_text_config_then_top_level` | top level first | 1152 != 2560 |
| `reassemble_rejects_original_bytes_above_max_layer_bytes` | drop the bound | reached `decompress` |
| `reassemble_round_trips_odd_byte_layer_compressed` | per-tensor halves count | 4095 != 4096 |
| `prepare_rejects_odd_byte_layer_for_quantization` | drop odd check | Ok |
| `reassemble_rejects_quantized_header_bytes_mismatch` | drop bytes check | Ok, 512 bytes |
| `reassemble_rejects_uncompressed_length_mismatch` | drop length checks | Ok, 511 bytes |
| `test_header_num_elements_empty_shape_is_one`, `float16_scalar_round_trips_and_empty_scalar_is_rejected` | `[]` is 0 elements | 0 != 1, serialize refuses scalar |
| `decode_does_not_copy_the_payload_before_decompressing` | `to_vec()` before decompress | assertion |
| `test_max_decoded_tensor_bytes_helper` | const back to `usize` | E0308 compile error |

## Verification

All on spark-102 (GB10) at aaa4daa, `--profile test-fast`, `--test-threads=1`, GPU runs under `gpu-lock run`, real-model runs with `MLXCEL_REQUIRE_MODELS=1 MLXCEL_SDPA_DETERMINISTIC=1`. Every command exited 0.

| Command | Result |
|---|---|
| `cargo fmt --all -- --check` | clean |
| `cargo clippy -p mlxcel --features cuda,test-utils --lib --tests -- -D warnings` | clean |
| `cargo test -p mlxcel --features cuda --lib distributed::` | 1512 passed, 4 ignored |
| `cargo test -p mlxcel-core --features cuda from_bytes` | 6 passed |
| `--features cuda,test-utils --test distributed_integration` | 20 passed |
| `--features cuda,test-utils --test pipeline_e2e` | 30 passed |
| `--test pipeline_remote_real_models -- --ignored --skip glm_moe_dsa` | 10 passed (llama incl. RDMA/Thunderbolt, gemma3, gemma4, glm4, gpt_oss, qwen3, qwen3.5) |
| `--test pipeline_server_remote_real_models -- --ignored --skip glm_moe_dsa` | 7 passed |
| `--test pipeline_ci_multi_stage_real_models -- --skip glm_moe_dsa`, with and without `--ignored` | 2 + 2 passed, including `pipeline_multi_stage_three_host_real_model_parity` |
| `--test disaggregated_e2e` | 51 passed |
| `--test disaggregated_serving_e2e -- --include-ignored` | 2 passed |
| `--test disaggregated_router_e2e -- --include-ignored` | 7 passed |

The gemma4 runs use `gemma-4-e2b-it-4bit` (`hidden_size_per_layer_input=256`), so honest packed frames pass the per-layer width bound. `glm_moe_dsa` needs the 418.6 GB `glm5-4bit` checkpoint, which cannot load on a 128 GB box.

Closes #2716


## Known gap after merging with #2720

On a non-entry stage, a forward activation refused by the new limits still leaves `handle_activation` through `?` before `report_activation_failure`, so the coordinator waits out `stage_timeout` instead of getting an immediate `Error` (stage 0 answers at once). #2720 routes only execution and downstream-send failures to the coordinator. Routing every `handle_activation` failure, including these decode rejections, to the coordinator is folded into #2714, which rewrites the reply target in the same function; `activation_outside_stage_limits_never_executes` will then require the `Error`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:inference Generation, sampling, decoding (incl. speculative, DRY) priority:medium Medium priority status:done Completed type:enhancement New features, capabilities, or significant additions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

update(distributed): transport and remote pipeline robustness follow-ups

1 participant