Reliable WAN split serving + speculative pipelining over the internet - #1028
Closed
michaelneale wants to merge 9 commits into
Closed
Reliable WAN split serving + speculative pipelining over the internet#1028michaelneale wants to merge 9 commits into
michaelneale wants to merge 9 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds forced VerifyWindow pipelining configuration, prepared direct-return socket pooling, direct-route confirmation, serial fallback telemetry, continuous n-gram refill, NativeMTP survival metrics, and tolerant operational stage transport handling. ChangesVerifyWindow and direct-return flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant EmbeddedStageZeroGeneration
participant PreparedPredictionReturnPool
participant PredictionReturnReceiver
participant VerifyWindowScheduler
EmbeddedStageZeroGeneration->>PreparedPredictionReturnPool: checkout and bind prepared socket
EmbeddedStageZeroGeneration->>PredictionReturnReceiver: reset confirmation after generation ACK
PredictionReturnReceiver-->>EmbeddedStageZeroGeneration: observe direct reply
EmbeddedStageZeroGeneration->>VerifyWindowScheduler: seed pipeline when direct route is confirmed
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
When --speculative-verify-window-pipeline-force is set and pipeline_depth>1, bypass the adaptive profitability gate and seed the pipeline from the first eligible composite proposal (shard-style bet-on-the-pipeline). The adaptive gate can never warm up on a low-continuation WAN split because its only evidence comes from the serial path. FIFO completion, reject cooldown, direct-return requirement, parallel_verify_width eligibility and stale draining are all preserved. New telemetry field verify_window_force.
…T gate The verify-window pipelining path hard-required a direct-prediction-return sink, so a WAN split that could not open that sink 502'd every request instead of serving. Root cause: open_stage_transport_stream re-applied the formation-time MAX_SPLIT_RTT_MS (80ms) eligibility ceiling to every fresh operational stream. Pooled forward lanes (opened once at low RTT and reused) kept working, but per-request direct-return sinks opened a fresh bridge stream each request and were rejected under normal WAN RTT jitter, aborting the bridge task and surfacing as a ready-handshake timeout on stage 0. Changes: - Relax the native-MTP verify guard: serve serially over the forward lane when direct-return is unavailable (serial completion already polls both the direct receiver and the forward lane). Keep pipelining gated on a confirmed direct sink, since its completion path is direct-only. - Stop re-litigating formation-time path eligibility on post-formation operational streams in open_stage_transport_stream; warn and proceed. Split admission still gates eligibility using gossiped, hysteresis-smoothed RTT plus re-election. Transient degradation slows a stream; it no longer kills it. - Bounded, privacy-safe telemetry: classify_direct_return_failure_phase emits a phase label instead of raw endpoints/error text in OTLP. Raw detail stays in local stderr only. New stage.verify_window_serial_fallback event. Tests: 6 new classifier unit tests; existing direct_return, split_stage_path, split_readiness, and stage_transport suites pass.
…etup The direct-prediction-return sink's ready handshake used a 5s read timeout, but over a WAN mesh the remote ready byte only arrives after the bridge cold- establishes a fresh stage QUIC connection (~10s budget) and the remote inbound handler dials its local binary server. 5s timed out during that cold setup (observed EAGAIN on a healthy ~26ms split) even though pooled forward lanes — which get a 20s initial connect budget and are then reused — succeeded on the same bridge. Match the forward-lane budget so the cold return path completes instead of falling back to the slower upstream-reply path.
…irmation Step 1 of shard-like WAN pipelining: make direct-prediction-return reliable so speculative verify-window pipelining can be sustained over WAN, not just opened intermittently. Return-sink pool (kills cold-handshake intermittency): - Split the return-sink open into a WAN-flaky connect+ready phase and a cheap per-request bind (PredictionReturnOpen write). - Add PreparedPredictionReturnPool: a single long-lived maintenance worker keeps capacity=generation_concurrency sockets pre-warmed off the hot path, rotates them before they can silently rot (refresh age < max age), and refills FIFO on a checkout signal. No per-request thread spawn; serialized refill cannot overshoot capacity; parked sockets closed deterministically on drop. - Hot path binds a pooled socket (cheap write) and falls back to the cold open, then to serial serving. A request is never failed. Implicit pipeline confirmation (prevents a 300s hang on a false direct route): - A successful PredictionReturnOpen write does not prove the tail selected the direct route. Gate pipelining (depth>1, direct-only completion) on an actual direct reply observed AFTER generation config, not on bind success. - PredictionReturnReceiver trips direct_confirmed() when a reply is delivered on the direct route; the flag is reset right after the generation-config ACK so pre-config restore/prefill replies can't falsely confirm. While unconfirmed, serial completion polls both the direct receiver and the forward lane, so it serves promptly and trips confirmation on the first real direct reply. Telemetry: shared privacy-safe classify_return_failure_phase (bounded phase labels only; raw endpoints/errors stay on local stderr). Both changes reviewed with the expert model (design + implementation). Tests: new pool age-policy units + shared classifier units; full skippy-server lib suite passes; clippy -D warnings clean.
michaelneale
force-pushed
the
wip/wan-direct-prediction-return
branch
from
July 21, 2026 03:34
d729fda to
2b76ba4
Compare
resolve_decode_config exceeded the 200-line clippy limit (208) after adding the pipeline_force override. Extract the verify-window model/global override block into resolve_verify_window_config. No behavior change.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/mesh-llm-host-runtime/src/mesh/stage_transport.rs (1)
1064-1074: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winWarning logs on every stream open could amplify log volume exactly when the path is already degraded.
Since this path is exercised per accepted bridge connection — including "per-request direct-prediction-return sinks... open a fresh bridge stream each request" per the comment above — a sustained degraded/relay path will now emit a
tracing::warn!on every single request instead of failing once. Under load during exactly the failure scenario this change is meant to tolerate, that's a lot of warn-level log lines.Consider rate-limiting this warning (e.g., log once per peer per interval, or downgrade to
debug!after the first occurrence within a window) so operators still get the signal without log-volume amplification during degraded periods.♻️ Sketch of a simple per-peer rate-limited warn
- if let Some(rejection) = split_stage_path_snapshot_from_connection(&conn) - .with_peer_path_fallback(self.peer_stage_path_fallback(peer_id).await) - .stage_path_rejection() - { - tracing::warn!( - "stage transport path to {} reports {} on a formed split; proceeding \ - (operational streams tolerate transient path degradation)", - peer_id.fmt_short(), - rejection.as_str() - ); - } + if let Some(rejection) = split_stage_path_snapshot_from_connection(&conn) + .with_peer_path_fallback(self.peer_stage_path_fallback(peer_id).await) + .stage_path_rejection() + { + // Rate-limit: only warn once per peer within a short window instead + // of once per opened stream (potentially once per request). + if self.should_log_path_degradation(peer_id) { + tracing::warn!( + "stage transport path to {} reports {} on a formed split; proceeding \ + (operational streams tolerate transient path degradation)", + peer_id.fmt_short(), + rejection.as_str() + ); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-host-runtime/src/mesh/stage_transport.rs` around lines 1064 - 1074, Rate-limit the warning emitted in the formed-split rejection branch around split_stage_path_snapshot_from_connection, so repeated stream opens for the same peer do not produce a warn-level log on every request. Preserve the rejection detection and continued operation, while using a per-peer interval or equivalent existing throttling mechanism to retain periodic operational visibility.crates/skippy-server/src/frontend/embedded_generation.rs (1)
945-990: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNew verify-window availability/force logic keeps growing an already-oversized function/file.
embedded_generation.rsis ~2,100 lines andgenerate_embedded_stage_zero_tokensspans nearly the whole file. This PR adds several more self-contained concerns here (pool-preference selection, force-config construction, availability/performance separation, serial-fallback telemetry) without extracting any of them into named helpers/modules, further growing a function that already exceeds typical Clippy line-count/cognitive-complexity limits and a file already over the 2,000-line ceiling.Consider extracting the "direct-return acquisition" (lines ~76-140) and "verify-window availability decision + serial-fallback telemetry" (lines ~945-990) into dedicated helper functions (or a small submodule under
frontend/), consistent with howprepared_return_pool.rswas already split out in this same PR.As per coding guidelines, "Do not add Rust methods or functions exceeding the configured Clippy line-count or cognitive-complexity limits; split them into semantically named helpers" and "When modifying a Rust file already over 1,000 lines, extract any separable responsibility into a named module, keep the new file under 1,000 lines...".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-server/src/frontend/embedded_generation.rs` around lines 945 - 990, The oversized generate_embedded_stage_zero_tokens flow should delegate its separable verify-window availability decision and serial-fallback telemetry, and direct-return acquisition, to named helpers or a dedicated frontend submodule. Extract the logic around direct_prediction_return_opened, pipelined_decode_enabled, mark_direct_prediction_return, and stage.verify_window_serial_fallback while preserving existing behavior and telemetry attributes; also move the direct-return acquisition block into its own helper, keeping the resulting module under the project’s size and complexity limits.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/mesh-llm-host-runtime/src/mesh/stage_transport.rs`:
- Around line 1064-1074: Rate-limit the warning emitted in the formed-split
rejection branch around split_stage_path_snapshot_from_connection, so repeated
stream opens for the same peer do not produce a warn-level log on every request.
Preserve the rejection detection and continued operation, while using a per-peer
interval or equivalent existing throttling mechanism to retain periodic
operational visibility.
In `@crates/skippy-server/src/frontend/embedded_generation.rs`:
- Around line 945-990: The oversized generate_embedded_stage_zero_tokens flow
should delegate its separable verify-window availability decision and
serial-fallback telemetry, and direct-return acquisition, to named helpers or a
dedicated frontend submodule. Extract the logic around
direct_prediction_return_opened, pipelined_decode_enabled,
mark_direct_prediction_return, and stage.verify_window_serial_fallback while
preserving existing behavior and telemetry attributes; also move the
direct-return acquisition block into its own helper, keeping the resulting
module under the project’s size and complexity limits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 229dab6d-57ba-416d-a51d-93983ad476a7
📒 Files selected for processing (19)
crates/mesh-llm-cli/src/parser/commands.rscrates/mesh-llm-config/src/model.rscrates/mesh-llm-config/src/model/built_in_schema.rscrates/mesh-llm-config/src/model/built_in_schema/control_behavior/speculative.rscrates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rscrates/mesh-llm-host-runtime/src/mesh/stage_transport.rscrates/mesh-llm/src/lib.rscrates/skippy-server/src/binary_transport.rscrates/skippy-server/src/binary_transport/direct_return.rscrates/skippy-server/src/binary_transport/options.rscrates/skippy-server/src/frontend/decode_scheduler.rscrates/skippy-server/src/frontend/embedded_generation.rscrates/skippy-server/src/frontend/generation.rscrates/skippy-server/src/frontend/generation/prepared_return_pool.rscrates/skippy-server/src/frontend/generation/server.rscrates/skippy-server/src/frontend/generation/types.rscrates/skippy-server/src/frontend/generation_flow.rscrates/skippy-server/src/frontend/speculative.rscrates/skippy-server/src/frontend/tests/multimodal.rs
The defaults_ui_schema_export_snapshot test compares generated schema against a checked-in reference. Adding the pipeline_force config field made the generated schema include defaults.speculative.verify_window_pipeline_force, which the fixture lacked. Add the missing entry (alphabetical, after pipeline_depth).
Instrument acceptance depth g per drafter source, so we can measure where acceptance craters by depth and decide whether pipelining depth pays (our prior depth-4 A/B lost to divergence waste — this shows why, by source). Per retired logical composite proposal, observe_hybrid_proposal now records cumulative survival counts (eligible_ge/survived_ge for depths 1..=8) split into native-MTP prefix vs ngram tail. Decomposition (expert-reviewed): native survival = min(accepted, N); ngram tail only counted once the native prefix fully survived, so native failure never contaminates ngram quality. Stale- drained proposals are excluded (they retire via a different path). Emitted to response timings as arrays plus logical_proposal_count; P(A>=k) per source = survived_ge[k-1]/eligible_ge[k-1]. Measurement-only, no behavior change. 4 unit tests cover full-accept, native partial-reject (ngram excluded), ngram partial, and pure-ngram proposals.
Replace drain-and-rebuild at proposal boundaries with in-loop continuous refill, so pipeline depth stays full across proposal boundaries on cache-hit continuations instead of emptying and reseeding. - CompositeProposalPipeline gains optimistic_suffix() (uncommitted suffix used only as read-only n-gram lookup context), append_ngram_candidates() (extends proposal + candidate deque), and a sealed-tail invariant: once a window is dispatched without a reserved free-target bridge, refill is refused so it can never splice behind a stale optimistic frontier (expert-reviewed guard). - The dispatch loop tops up n-gram candidates before planning each window when the candidate deque is below verify_width+1. The n-gram cache indexes only committed history; wrong refilled candidates are discarded by the existing FIFO stale-drain + KV rewind, so refill can waste target work but never corrupt committed state. - Defers the general epoch/frontier ledger to the draft-model phase (Phase 2), where an independent drafter with its own state needs reset/replay. Scoped to Option A per expert review (9/10 vs 3/10 for the full ledger now). 6 new unit tests: optimistic-suffix tracking, refill extend, sealed-tail refusal, empty no-op.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/skippy-server/src/frontend/native_mtp/hybrid.rs (1)
163-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove empty slice check to the beginning.
For a slightly more idiomatic and optimal approach, consider checking for an empty slice and returning early before performing the extend and add operations.
💡 Proposed refactor
pub(in crate::frontend) fn append_ngram_tokens(&mut self, tokens: &[i32]) { + if tokens.is_empty() { + return; + } self.tokens.extend_from_slice(tokens); self.ngram_token_count = self.ngram_token_count.saturating_add(tokens.len()); - if tokens.is_empty() { - return; - } // A refilled tail means an n-gram continuation was available. self.ngram_span_available = true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-server/src/frontend/native_mtp/hybrid.rs` around lines 163 - 174, Update append_ngram_tokens to return immediately when tokens is empty before extending self.tokens or updating ngram_token_count. Keep the existing token extension, count increment, and ngram_span_available update unchanged for non-empty slices.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/skippy-server/src/frontend/native_mtp/hybrid.rs`:
- Around line 163-174: Update append_ngram_tokens to return immediately when
tokens is empty before extending self.tokens or updating ngram_token_count. Keep
the existing token extension, count increment, and ngram_span_available update
unchanged for non-empty slices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b7140a85-1e6f-4a7a-a2ce-1ee3b69ebc5b
📒 Files selected for processing (4)
crates/skippy-server/src/frontend/embedded_generation.rscrates/skippy-server/src/frontend/embedded_generation/lifecycle.rscrates/skippy-server/src/frontend/native_mtp/hybrid.rscrates/skippy-server/src/frontend/native_mtp/pipeline.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/skippy-server/src/frontend/embedded_generation.rs
ndizazzo
approved these changes
Jul 21, 2026
Collaborator
There was a problem hiding this comment.
Looks fine to me!
@michaelneale Just FYI, this verify_window_pipeline_force = true|false option will automatically appear in the configuration TOML and React options on the frontend, if you are okay with that. This is because of the recent work to enumerate settings through the schema (which was updated in the PR)... So if you don't want it exposed you'll have to hide that setting.
michaelneale
marked this pull request as draft
July 22, 2026 00:55
Contributor
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
Collaborator
Author
|
🤖 Posted by micn's AI agent after a triage of this branch, Where this branch actually standsThe transport layer is proven (WAN split 502 → serving, warm 12.7s → 0.6s, direct-return confirmed, depth 4 in flight). The payoff layer is not: pipelining only engaged under PlanWhat to keep from this branchKeep (proven, land as the split-out PR):
Keep conditionally:
Cut / park:
Protocol: no wire change anywhere in the keep set; older peers just stay on the serial path. Mixed-version safe. |
Transport-core status/handoff for PR #1028: what is proven over WAN, the landed keep-set, why fixed pipeline depth is diagnostic-only, the go-forward suffix-ngram + adaptive-depth direction (#1037), and the 2-node bringup config trap. Draft-model work is deferred on wip/wan-draft-ahead. Assisted-by: goose
michaelneale
force-pushed
the
wip/wan-direct-prediction-return
branch
from
July 22, 2026 01:59
5a50db8 to
3be9189
Compare
This was referenced Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this enables
Split serving now works reliably over a WAN link, and speculative "verify-window" pipelining can actually engage across it. Before this change, a 2-node split across the internet (e.g. a coordinator in one region and a GPU box in another) would 502 every request — the direct-prediction-return channel that pipelining depends on could not be established, and even when it could, normal WAN latency jitter tore it down mid-request.
After this change:
verify_window_max_in_flight > 1), the mechanism that hides remote round-trip latency.Validated live on a Sydney↔AU 4090 split (~26ms RTT): direct-return confirmed working, warm requests dropped from ~12.7s to ~0.6s once the channel was reliable.
Architecture
Four changes, smallest-first:
Serve without direct-return, and stop killing operational streams on RTT jitter. The native-MTP verify path used to hard-require a direct-return sink and 502 without it. It now falls back to serial completion over the forward lane (which polls both routes), so the split serves regardless. Separately,
open_stage_transport_streamno longer re-applies the formation-timeMAX_SPLIT_RTT_MSceiling to every fresh operational stream — split admission already gates path eligibility with gossiped, hysteresis-smoothed RTT plus re-election. Re-checking instantaneous per-stream RTT was rejecting per-request return sinks under normal jitter while pooled forward lanes stayed healthy.Return-sink ready-handshake timeout 5s → 20s. Over WAN the return sink connects to a local bridge alias, but the remote
readybyte only arrives after the bridge cold-establishes a fresh stage QUIC connection (~10s budget) and the remote handler dials its local server. 5s timed out during that cold setup; forward lanes already used a 20s budget. Matching it lets the cold return path complete.Pre-warmed return-sink pool. A single long-lived maintenance worker keeps
capacity = generation_concurrencyreturn sockets pre-warmed off the hot path (connect + ready handshake done, not yet bound), age-rotated before they can silently rot, and refilled FIFO on checkout. The hot path binds a pooled socket with a cheap write and falls back to a cold open, then to serial. This removes the cold-handshake intermittency that made direct-return work only sporadically.Implicit pipeline confirmation. A successful
PredictionReturnOpenwrite does not prove the tail selected the direct route (it may still reply on the forward lane), and the pipelined completion path is direct-only — so a false positive could hang up to 300s. Pipelining now gates on an actual direct reply observed after generation config, not on bind success. While unconfirmed, serial completion polls both routes (serving promptly) and trips confirmation on the first real direct reply. The flag is reset right after the generation-config ACK so pre-config restore/prefill replies can't falsely confirm.Protocol
No wire-format change. All behavior is additive and compatible with mixed-version meshes:
Validation
direct_return,split_stage_path,split_readiness,stage_transportsuites pass.skippy-serverlib suite green;cargo clippy -D warningsclean onskippy-serverandmesh-llm-host-runtime;cargo fmt --all --checkclean.verify_window_direct_prediction_return=true; pipelining reachedmax_in_flightup to 4 under forced mode.Notes / follow-up (not in this PR)
Depth tuning is intentionally out of scope. A separate A/B on this branch showed that pushing proposal depth from 2→4 on GLM-4.7-Flash over WAN did fill the pipeline and hide latency (downstream-wait
700ms→335ms) but reduced net throughput (45→37 tok/s) because divergence discarded far more speculative windows. The throughput lever for that model/workload is speculative acceptance rate and keeping the pipeline full without wasting work on divergence, not raw depth — tracked separately. This PR is the reliability foundation that makes any of that measurable.Summary by CodeRabbit