Enhance congestion control and link management features#15
Merged
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Kalman filter tracks RTT velocity but congestion control ignored it. Now perform_window_recovery() halves the recovery rate when velocity exceeds 2.0 ms/sample, preventing window inflation during active congestion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add a velocity_penalty term to predicted_arrival() when Kalman velocity is positive. This penalises links with rising RTT trends before congestion manifests as loss, giving EDPF proactive avoidance. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Exclude links where in-flight bytes exceed 1.5× the bandwidth-delay product (BDP). Prevents runaway in-flight during RTT inflation on cellular networks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add LinkPhase enum (Registering → Warming → Live → Degraded → Cooldown) to replace implicit state from boolean flags. Scheduler skips non-Live/ Degraded links, eliminating early NAK bursts from newly-connected links. - Warming phase requires 2 RTT probes or 5s timeout before going Live - Housekeeping drives degradation detection and cooldown transitions - All selection strategies (classic, enhanced, RTT-threshold, EDPF, BLEST) now check is_schedulable() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add AsymmetricEwma struct with separate alpha_up / alpha_down smoothing factors. Will be used to replace ad-hoc asymmetric smoothing in capacity and RTT tracking. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add optional TOML configuration file support: - New --config CLI arg for specifying config file path - TomlConfig struct with serde defaults for all tunable constants (congestion control, EDPF scheduler, link lifecycle, selection) - Load at startup with fallback to defaults on error Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Detect keyframe bursts (runs of max-MTU 1316-byte packets) and prefer higher-quality links for keyframe packets. The KeyframeDetector tracks consecutive max-size packets and declares a burst after 5+ in a row. During keyframe bursts, the scheduler selects the link with the highest quality_multiplier among connected/schedulable links, ensuring keyframes travel on the most reliable path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Implement RFC 8382 statistical shared bottleneck detection using per-link OWD samples from Kalman RTT/2. Each interval computes: - Skew (mean−median): queuing delay buildup - Variance (MAD/mean): delay variability - Frequency (sign-change ratio): oscillation pattern - Loss rate from NAK counts Links are bottlenecked when skew > C_S AND (var > C_H OR loss > P_L), then grouped by delay statistics similarity using union-find. In EDPF mode, correlated links have effective capacity reduced by 0.7x. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… items Gate test-only methods behind #[cfg(test)] instead of suppressing dead_code warnings: AsymmetricEwma, edpf::select_from/select_from_indices, sbd::groups(), keyframe::is_in_burst/total_bursts, blest::record_blocking, iods::reset. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Augments the packet-size keyframe heuristic with an out-of-band hint
channel. An upstream encoder (belacoder) can tell srtla_send "the next
N SRT data packets are critical" via the Unix control socket, e.g.
mark-critical 23
Maintains a per-sender budget in DynamicConfig; each data packet
consumes one from the budget. While budget > 0 or the heuristic fires,
the scheduler routes to the highest-quality link. The two signals are
OR-combined so heuristic-only deployments (ffmpeg, libsrt) keep the
existing behaviour while hint-aware encoders gain exact coverage of
IDR / SPS / PPS frames that the 5-packet-burst heuristic misses.
The previous flat text protocol ("mode classic", "mark-critical 3",
"status") was shaped for humans poking with socat. Machine clients
(belacoder, future agents, exporters) need request/response correlation,
typed errors, and schema-discoverable methods.
One message per line, one JSON-RPC 2.0 envelope per message. Requests
without an id are notifications, used for the hot-path mark_critical
hint so encoders don't block on a round-trip when firing a keyframe.
Methods: set_mode, set_quality, set_exploration, set_rtt_delta,
get_status, get_stats, mark_critical. subscribe / unsubscribe names
are reserved for a later streaming upgrade.
Hand-rolled rather than pulling in jsonrpsee or jsonrpc-core: the spec
is one page, srtla_send's control listener is a blocking std::thread
(not tokio), and deps are intentionally lean here. Full protocol docs
in docs/CONTROL_PROTOCOL.md.
BREAKING: the old "mode classic" / "stats" / etc. text commands no
longer work. Update any scripts poking the control socket.
The JSON-RPC mark_critical method had three fragility issues: the packet count was a guess (MPEG-TS overhead unknowable from encoder side), the hint went on a different scheduler path than the SRT data so it could arrive after the packets it described, and in multi-source setups each encoder's hints polluted the shared counter. Swap to a dedicated UDP sidecar. The encoder sends a 5-byte datagram (magic byte + window_ms) that opens a deadline-based critical window. Loopback UDP shares the network stack with SRT data, so the window opens tightly ordered against the packets it protects. The scheduler OR-combines the window with the packet-size heuristic as before. Operator enables with --priority-bind ADDR:PORT. get_status exposes windows_received and malformed_datagrams counters. Full wire-format docs in docs/KEYFRAME_PRIORITY.md. BREAKING: mark_critical RPC removed from the JSON protocol. Encoders that wired the old hint-count API must switch to the UDP sidecar.
New optional HTTP endpoint for scraping. Exposes per-link series (up, RTT, window, in_flight, NAKs, bitrate, quality_multiplier), aggregate counters, scheduling mode as a gauge, and priority-sidecar counters. Enabled via --metrics-bind ADDR:PORT. Hand-rolled over tokio::net::TcpListener with no axum/hyper/tower pulled in. Supports GET /metrics and GET / only; anything else 404s. Responses always close the connection, which is plenty for Prometheus scrape semantics.
Scraping get_stats at 1 Hz loses sub-second link-state changes.
Subscriptions let a client register for a topic and receive server
push events as JSON-RPC notifications on the same connection.
Topics:
- stats — StatsSnapshot pushed once per second alongside the
existing housekeeping update
- priority.window — pushed on each accepted sidecar datagram with
at_ms, window_ms, deadline_ms
The sync std::thread Unix-socket listener couldn't push unsolicited
messages on the same connection. Replaced it with a tokio
UnixListener whose per-connection task tokio::selects between reads
and outbound push-channel writes. Stdin stays blocking — no
subscription support there, subscribe/unsubscribe from stdin returns
method not found.
SubscriptionHub fans out by topic into per-connection mpsc senders;
full channels drop events (backed-up subscriber never blocks the
producer) and closed channels are pruned lazily.
BREAKING: `config::spawn_config_listener` is gone; call
`config::spawn_stdin_listener` and `control_socket::spawn` separately.
drop the unproven scheduling modes (rtt-threshold, edpf) and their auxiliary filters (sbd, blest, iods). only the IRL-tested classic and enhanced modes remain. exploration stays as an off-by-default opt-in flag inside enhanced. removes: --rtt-delta-ms CLI flag, set_rtt_delta JSON-RPC, rtt_delta_ms config and stats fields, edpf_* TOML keys, rtt_threshold tests. removes ~1700 LOC. clears the deck before adding the weak-link classifier and per-link target-rate soft cap to enhanced mode.
new module sender/selection/classifier.rs implementing a three-tier delay cascade with entering/leaving hysteresis (3x ratio). classifies each connection as weak based on: - rtt vs the chosen tier (best=40%, safe=50%, max=60% of estimated budget; budget = max(longest_rtt*3, 500ms) capped at 5s), - bandwidth share vs an enter/leave threshold pair derived from fair share (0.25/N enter, 0.75/N leave). constants picked conservative for first soak; real-world observation may suggest retuning. shadow mode: classifier runs on the housekeeping tick and surfaces weak/reason/share/threshold per link plus selected_delay_ms / estimated_max_delay_ms via the existing get_stats json. selection is unchanged — admission gate wires in after a soak window.
new module sender/selection/link_cc.rs implementing a 3-state cc controller per connection (bootstrap / climbing / holding / backing_off) producing target_bps as a soft cap. inputs: - age-bucketed rtt ewma with 1:1 / 1:4 / 1:8 / 1:16 weights at age bands >=1s / >=500ms / >=250ms / <250ms; 2s gap snaps to the new sample. - rttvar via 1:3 weighted moving deviation. - 1s sliding-window loss permille (nak plumbing follow-up). - observed bps from existing BitrateTracker. state transitions: - loss > 5 permille (0.5%) -> BackingOff (multiplicative -15%). - rtt ewma > 1.5x rtt min -> Holding. - otherwise -> Climbing (additive +2% per tick, capped by 2x measured throughput so idle links don't ramp). shadow mode: snapshots flow into stats json next to existing weak/cc fields. selection is unchanged. wires into Enhanced as a soft cap after a soak window.
promote the weak-link classifier and per-link cc state from shadow mode into the enhanced selection scorer. - new fields weak / cc_backing_off on SrtlaConnection, stamped each housekeeping tick from WeakLinkFilter::classify and LinkCcController::tick_all. - enhanced::select_connection skips weak or backing-off connections when at least one healthy alternative is schedulable. when every link is weak, falls back to the full pool — better to send on a weak link than to drop the packet. regression tests cover the three branches: skip-weak-with-alternative, fallback-when-all-weak, and treats-backing-off-as-weak. 229 lib tests green.
extend the per-link CC state machine from 4 states (Bootstrap / Climbing / Holding / BackingOff) to 5 + a climb sub-mode. closes the gap between our simplified controller and the cellular profile's needs without porting the full 9-mode reference. new state: Drain — one-shot 25% multiplicative decrease when RTT inflation crosses 2.0x without observed loss. catches BDQ overload before ARQ surfaces it. transitions to Climbing on the next tick (or to Holding if RTT didn't recover). new sub-modes for Climbing: Hai — 6%/tick AI when RTT variance ≤ 10% of the smoothed RTT mean. confident headroom signal so we ramp faster. FastRecovery — 4%/tick AI for 5 ticks after exiting BackingOff or Drain. claws back the bandwidth we just gave up without the overshoot risk that Hai would carry. Normal — 2%/tick AI baseline; covers steady-state with no special signal. priority ordering when picking the climb sub-mode: 1. FastRecovery while the post-backoff window is open. 2. Hai when RTT is stable enough. 3. Normal otherwise. precedence in next-state selection: loss observed -> BackingOff rtt_inflation ≥ 2.0 -> Drain rtt_inflation > 1.5 -> Holding else -> Climbing new ClimbMode telemetry exported via LinkCcSnapshot and the per- link stats JSON (cc_climb_mode field). dashboards can render it alongside cc_state. 5 new unit tests: - hai_kicks_in_when_rtt_is_stable - hai_yields_to_normal_when_rtt_is_jittery - fast_recovery_engages_after_backoff - drain_triggers_on_high_rtt_inflation_no_loss - drain_then_recovery_path existing holding_when_rtt_inflates updated: original used 60ms samples (3x inflation) which now triggers Drain rather than Holding; switched to 35ms samples (1.75x — Holding band) so the test still exercises the original intent. 234 srtla_send lib tests pass.
batch_send.rs previously buffered up to a fixed 16 packets / 15ms flush window. on idle links that adds latency for traffic that arrives in bursts; on heavy links it caps how much we can amortise into a single sendmmsg-style call. three regimes, picked per connection from observed bitrate: bitrate regime batch threshold ───────────────── ─────────── ─────────────── ≤ 500 kbps LowActivity 4 500 kbps – 5 Mbps Normal 16 (Moblin sweet spot) > 5 Mbps HighLoad 32 flush interval stays 15ms across regimes — going longer would add latency on traffic resumption, going shorter would erase the syscall amortisation we batch for. new BatchRegime enum + BatchSender::set_regime / regime() accessor. SrtlaConnection grows a recompute_batch_regime() that maps current bitrate_bps → BatchRegime via BatchRegime::from_bps. housekeeping calls it once per tick alongside calculate_bitrate / update_phase. cheap to drive — set_regime is a single field write, no allocation. no-op when the regime hasn't changed (caller doesn't need to track deltas). 2 new unit tests: - regime_from_bps_thresholds — boundary semantics - batch_size_threshold_per_regime — actual flush trigger varies 236 srtla_send lib tests pass.
uplink sockets were always bound by source ip, which only steers egress on a multi-homed host with source-based routing. introduce an UplinkBinder trait so the steering action is injectable: SourceIpBinder keeps the cli behavior, CallbackBinder lets a library consumer steer the raw fd (android Network.bindSocket) while keeping IpAddr as the uplink identity. thread the binder through run_sender_with_config, connection creation, and reconnect.
collapse cooldown match guard, use clamp/iter/if-let/or_default in the selection code, drop the redundant LinkCongestionState::new, and allow constant-invariant assertions in the protocol test modules.
reconcile docs with the code after the edpf / rtt-threshold scheduling work was dropped. only classic and enhanced modes remain. - add CHANGELOG.md covering changes since v3.0.0, including an "explored and removed" section for the edpf/blest/iods pipeline - README: drop rtt-threshold mode, --rtt-delta-ms, and the stale set_rtt_delta / mark_critical control-socket examples; add the real --config, --priority-bind, --metrics-bind flags and the subscribe/unsubscribe methods - CONTROL_PROTOCOL: trim set_mode to classic/enhanced, remove the set_rtt_delta method and the rtt_delta_ms get_status field - remove docs/RTT_THRESHOLD_SCHEDULING.md
before this commit, LinkCongestionState::record_loss was only called from unit tests. the production sliding-window loss tracker stayed at zero permille, which meant CcState::BackingOff was unreachable in production and the cc_backing_off gate that enhanced selection now consults could never fire. CC's loss path was dormant. new LinkCongestionState::observe_traffic(bytes_sent_total, nak_total, now_ms) computes per-tick deltas against a previous-call baseline and forwards them to record_loss. first call stashes a baseline without sampling. byte delta is converted to a packet count using the standard SRT payload (1316 B); the ratio is invariant under uniform packet-size assumptions so the approximation is fine for the loss permille EWMA. LinkCcController::tick_all reads conn.bitrate.bytes_sent_total and conn.total_nak_count() and calls observe_traffic before the existing tick(). zero-traffic ticks are skipped so the window doesn't fill with no-op samples. quiet-link-with-NAK pathological case is bounded by synthesizing a single-packet "sent" baseline, so the ratio never divides by zero. removed the stale "follow-up commit" comment and dropped the #[allow(dead_code)] on record_loss now that it's wired. 3 new unit tests: - observe_traffic_first_call_sets_baseline_without_sample - observe_traffic_delta_flows_into_record_loss - observe_traffic_quiet_tick_with_naks_does_not_panic 239 srtla_send lib tests pass.
before this commit, cc_target_bps was computed per-tick by the CC
controller, surfaced via stats JSON, but never consumed in selection
— the stats comment even said "selection does not yet treat as a
soft cap. After a soak window the cap wires into the Enhanced score."
that wiring never happened.
new on SrtlaConnection:
pub(crate) cc_target_bps: u64,
stamped alongside cc_backing_off in sender/mod.rs from the per-tick
LinkCcSnapshot.
new in enhanced.rs:
fn cc_soft_cap_multiplier(conn) -> f64 in [CC_SOFT_CAP_FLOOR, 1.0]
formula:
headroom = max(0, cc_target_bps - measured_bps)
multiplier = clamp(headroom / cc_target_bps, FLOOR, 1.0)
short-circuits to 1.0 when:
- cc_target_bps == 0 (CC hasn't bootstrapped yet)
- measured_bps == 0 (idle link, plenty of headroom)
floor = 0.10 — saturated links keep 10% of their raw score so a
trickle of keepalive traffic still flows and the CC controller
keeps observing the link. without a floor, a link at exactly its
cap would get score 0 forever.
the multiplier folds into the link's existing quality-aware score:
score = base * quality_mult * cap_mult
same code path covers the non-quality branch:
score = base * cap_mult
the previous binary cc_backing_off gate still runs first as a hard
admission filter. the new multiplier is a soft signal that operates
within the surviving candidate pool — links upshifting close to
their CC ceiling get deprioritised before backoff fires.
4 new unit tests cover the multiplier helper:
- cap_no_signal_returns_unity
- cap_idle_link_returns_unity
- cap_at_target_falls_to_floor
- cap_half_target_returns_half
stats.rs comment updated to reflect production consumption. test
helpers default cc_target_bps to 0. 243 srtla_send lib tests pass.
batch_send.rs has tracked a per-connection BatchRegime (LowActivity / Normal / HighLoad) since the adaptive batch-send commit, driven each housekeeping tick from observed bitrate. it was never plumbed into the stats JSON though, so dashboards couldn't see why one link was batching more aggressively than another. new on LinkStats: pub batch_regime: String, populated from conn.batch_sender.regime().as_str().to_string() in SharedStats::update — same place cc_state and cc_climb_mode land. new BatchRegime re-export from connection::mod alongside BatchSender so external callers (stats, future telemetry) don't have to reach into batch_send.rs directly. format mirrors the existing cc_* string-field convention so the existing dashboard pattern that renders cc_state as a chip works for batch_regime with zero schema gymnastics.
MIN_SWITCH_INTERVAL_MS pinned the enhanced scheduler to its previously chosen link for 15ms regardless of score. That is roughly 24 packets at the rate this sender actually pushes. get_score() counts a link's queued-but-unflushed packets as in-flight specifically so that routing a packet immediately de-prioritises its own link. Selection is therefore a closed feedback loop whose job is to bound queue depth per link. Freezing the decision for 15ms opens that loop: in-flight runs away on whichever link the timer parked on, those packets arrive past the SRT latency budget, and TLPKTDROP discards them. Measured under saturation, enhanced piled 215 packets in flight on one link while the other sat at 58, and skewed traffic toward the *weaker* link (64% of bytes onto a 1000kbit link while a 2000kbit one idled). The cooldown was never a scheduling policy. It existed to stop forward_via_connection from flushing a one-packet batch on every switch; that flush is gone (previous commit) and batches now leave in a single sendmmsg on their own threshold/timer, so switching costs nothing. netem testbed, receiver held at BELABOX srtla_rec, warm start, 30s, n=5, interleaved, idle host. Delivery %: scenario classic enhanced(stock) enhanced(this) tight 83.0 65.2 83.7 tight_asym 84.2 70.3 83.7 +14.6 pts on tight (t=15.2) and +12.8 on tight_asym (t=4.4), restoring parity with classic and with the BELABOX C reference. Removing the batch flush alone does not recover it: with the cooldown still in place, enhanced remains ~14 points down. Score hysteresis (SWITCH_THRESHOLD, 10%) is kept. Damping in score space resists noise-driven flip-flopping without opening the loop; damping in time does not.
Exploration existed to break a real starvation lock: a link demoted by a quality gate wins no packets, so it earns no ACKs, so the signal that demoted it never clears. Keepalives prove liveness (that is how stall_deselect un-gates itself) but cannot prove capacity. Only data can. The old implementation did not do that. It probed second-best -- usually a healthy link that is already earning its own ACKs and needs no probe -- it fired every 30s whether or not anything was wrong, and it had no budget: it diverted roughly half of all traffic for as long as its trigger held, which can be seconds. It stayed survivable only because the switch cooldown happened to rate limit it, and that cooldown is now gone. Replace it with a bounded probe: - targets only starved links (weak, loss_degraded, or over the in-flight cap), never a healthy runner-up - one packet per link per PROBE_INTERVAL_MS (200ms), so a persistently bad link cannot bleed throughput - only when an un-gated link is schedulable: a probe is paid for out of spare capacity, and when nothing is healthy every packet is needed for the stream - data packets only, since a control packet earns no ACK and steering SRT's own control traffic onto a degraded link would delay its control loop for nothing - stall_gated links are excluded: a suspected black hole already has its own liveness-proven recovery path, so aiming data at it only adds latency Still opt-in behind --exploration (exploration_enabled defaults false); this changes what that flag does, not the default path. elapsed_ms/STARTUP_INSTANT go with it -- the deleted 30s periodic trigger was their only caller.
… gate is_schedulable() excluded Registering AND Warming, so a warming link could carry no traffic at all. At go-live every link is warming, which empties the candidate pool: select_connection returns None, packet_handler logs "no available connection to forward packet", and the stream is dropped until the first link is promoted -- either by two keepalive RTT probes or, against a receiver that does not return them promptly, by the 5s WARMING_TIMEOUT_MS fallback. That contradicts the rule the rest of this scheduler follows, and that the phase machine was ported from: a phase weights a link's score, it never removes the link. `weak` and `loss_degraded` crush a score but keep the link rankable (GATED_LINK_PENALTY); `stall_gated` only fires when a healthier link exists. Warming was the one gate with no such escape hatch. Warming now scores at 0.8 instead of being excluded. The de-rating still expresses what the phase is for -- a link whose RTT baseline is one keepalive old should not take a full share while a characterised link is available -- but when every link is warming they are de-rated equally, the relative ranking is unchanged, and traffic flows immediately. Registering remains the sole hard exclusion. That is a protocol constraint, not a quality judgement: without REG3 the receiver discards the data.
The crate did not compile for windows: src/connection/socket.rs imported
std::os::fd unconditionally, and src/control_socket.rs imported the JSON-RPC
dispatch pair outside its own #[cfg(unix)]. Fixing those exposed a further layer
of unix-only code that only ever compiled because nothing downstream of it did.
Each item is gated where it is genuinely unix-bound, rather than silenced:
- CallbackBinder binds a socket by raw fd. It exists for Android, where the host
steers a socket onto a radio via Network.bindSocket. Windows has no fd and no
such integration, so the binder (and its re-export) is unix-only. SourceIpBinder,
which the CLI actually uses, is unaffected.
- analyze_ip_reload is the SIGHUP entry point, and ReloadRefusal::NotFound is
reachable only from it. Windows has no SIGHUP. Startup parsing goes through
analyze_ip_reload_text on every platform, so IP-list handling is unchanged.
- The async control surface (dispatch_async, SubscriptionContext,
handle_subscribe/unsubscribe) serves the Unix-domain control socket and nothing
else. The sync dispatch path used by config.rs stays on every platform.
- SubscriptionHub::{subscribe,unsubscribe,len} are reachable only from that
socket. publish() stays: on windows the hub publishes to nobody, because there
is no way to subscribe.
Also drops dispatch_inner's _subscription_ctx parameter, which was never read.
cargo check --target x86_64-pc-windows-gnu now succeeds. Linux is unchanged:
clippy --all-targets clean, 322 tests pass.
Deleted: the --exploration flag, the set_exploration JSON-RPC method, sender/selection/exploration.rs, the enable_explore plumbing, the exploration_enabled config/TOML/stats/status surface, and SchedulingMode:: is_enhanced (its only caller was effective_exploration_enabled). The original implementation was actively harmful. It probed second-best, which is normally a healthy link already earning its own ACKs and needing no probe; it fired every 30s whether or not anything was wrong; and it had no budget, so it diverted roughly half of all traffic for as long as its trigger held, which can be seconds. It survived only because the switch cooldown incidentally rate limited it, and that cooldown is gone. I rewrote it as a bounded probe of starved links (one data packet per link per 200ms, only when a healthy link exists) and then measured it on the netem testbed against the exact scenario it exists for: link1 gated to 0.00 Mbps by a 70% loss burst, silently healed, then needed when link0 collapsed to 600kbit. probe off: 98.3 +/- 3.9 median 99.2 runs<95%: 1/15 probe on: 99.0 +/- 0.3 median 99.1 runs<95%: 0/15 diff 0.76 pts, Welch t=0.75, n=15 per arm No effect. The starvation lock is real (I watched a link sit at 0.00 Mbps for ~7s after it silently healed) but it is self-clearing: GATED_LINK_PENALTY keeps a gated link rankable rather than excluded, so it retains a 0.2-0.5 Mbps trickle and earns the samples that clear the gate. The classifier's probation re-test covers the share-starvation latch on top of that. The probe was redundant twice over. Keeping an off-by-default knob that cannot be shown to help is how the switch cooldown got here: a mechanism kept because it sounded principled, which then acquired a workaround, which then broke the scheduler. If a real failure mode later shows the trickle is insufficient, that calls for a measurement, not a resurrected flag.
BackingOff multiplied target_bps by 0.85 every tick for as long as loss_permille stayed above 0.5%, with nothing asking whether our own offered rate was what caused the loss. A cellular link sitting at an ordinary 1-2% of wire loss therefore had its soft cap decayed 15% a second regardless of how lightly it was loaded: ~21 seconds from 3 Mbps to the 100 kbps floor, where the BDP in-flight cap deselects it. Steady radio loss threw away a link that was carrying real traffic, and since the loss never cleared, the link never came back. Note Drain already avoided exactly this by being one-shot, and says so: "applying the cut every tick compounds it, collapsing target_bps to the floor within ~11 ticks". The same was true of BackingOff, which compounded anyway. Two guards, covering different halves of the problem: - BACKOFF_MIN_LOAD_PERMILLE gates entry. A link we are barely feeding cannot be the cause of its own loss. This covers the starved link. - BACKOFF_EFFICACY_TICKS bounds the descent on a link we really are driving hard, by asking whether cutting is working at all: congestive loss responds to a lower offered rate, wire loss does not. If a ~39% cut has not moved the loss, stop attributing it to ourselves and let the link climb again. Neither suffices alone. The load gate does not converge by itself, since each cut lowers the target and so raises load, holding the gate open all the way down. The efficacy test cannot protect a starved link, since a link carrying nothing has no throughput to judge a cut against. The loss decrease stays compounding, unlike Drain's one-shot: rtt_min is windowed, so RTT inflation fades as a signal once congestion outlives the window, leaving loss as the only backstop able to walk the cap down to real capacity.
LinkStats::bitrate_bps was computed as mbps * 1_000_000 / 8, so it carried bytes per second under a bits-per-second name, sitting directly next to genuinely bit-denominated fields like cc_target_bps. It also fed a Prometheus gauge, srtla_send_link_bitrate_bps, whose own HELP text admitted "bytes/sec" while the metric name claimed otherwise. Anything comparing the two fields, or graphing the gauge as a bitrate, was silently out by a factor of 8. Rename rather than change the value: a consumer that loses a field breaks loudly, whereas quietly switching the units to bits would be an 8x error that nobody notices. BREAKING: the get_stats JSON field bitrate_bps is now bitrate_bytes_per_sec, and the Prometheus gauge srtla_send_link_bitrate_bps is now srtla_send_link_bitrate_bytes_per_second.
The existing impairment tests cannot exercise congestion control at all. They inject raw zero bytes into srtla_send's listener, which never completes an SRT handshake at the far end, so no ACKs or NAKs ever come back and LinkCongestionState never leaves Bootstrap. That is why they only assert "did not panic" -- there was nothing else to assert. test_loss_triggers_window_reduction cannot be testing what its name says. Add the missing pieces to network-sim: - start_srt_caller() puts a real srt-live-transmit caller in front of srtla_send, so a genuine SRT session runs through the bond and the listener's ACK/NAK stream drives the real feedback path. - get_stats() queries srtla_send's control socket for per-link cc_state, cc_target_bps and nak_count. It runs as root inside the namespace, since the socket is root-owned. - spawn_udp_stream() returns while traffic flows, so a test can sample a control loop under load rather than only after it. - The stream injector takes a payload size and paces against a wall-clock deadline. At 188 bytes a datagram, megabit rates need sub-millisecond sleeps that Python cannot hold, so the pump quietly became the bottleneck instead of the network. netns_wire_loss asserts the fix in ace481b end to end: 2% netem loss on a roomy 8 Mbps link, against a clean link deliberately too small (1.5 Mbps) to carry the stream alone, so the scheduler is forced to use the lossy one while nothing we send can congest it. It checks that the CC target never collapses toward the floor, that the lossy link keeps carrying real traffic, and that the bond aggregates past what the clean link could do by itself. It also asserts NAKs actually reached the sender, and separately that any data crossed the bond, so a broken harness fails loudly instead of passing vacuously. Unverified: netns tests need passwordless sudo, which is unavailable here, so this skips rather than runs.
A loopback repro with 2% random loss showed the previous commit did not work: target_bps still ratcheted 637500 -> 541875 -> ... -> 100000, hit the floor in 12 seconds, reseeded, and did it again. Two reasons, and the second invalidates a premise of the first attempt. The RTT escape nullified the latch. Clearing loss_uncongestive whenever rtt_inflation exceeded RTT_HOLD_FACTOR re-opened the backoff path on every inflated tick, so the controller alternated BackingOff -> Drain -> BackingOff and cut on nearly all of them. The latch might as well not have existed. Congestion visible in RTT is already answered by Drain and Holding; the loss path does not need to double up on it. The verdict now expires on a timer (LOSS_UNCONGESTIVE_RETEST_TICKS) so it is still re-tested rather than trusted forever. target_bps does not pace. It steers the scheduler between links; it does not throttle one. So cutting it does not reduce what the link sends, and measured throughput does not follow the cap down -- the repro shows the link flat at ~4.1 Mbps while its cap collapsed to 100 kbps. The efficacy test had assumed a cut reduces offered load, and the delivered floor was dropped earlier on the reasoning that "observed follows target down". It does not, and cannot. So restore the floor, for the reason that actually holds: never cap a link below the rate it is visibly sustaining. Such a cap is not a conservative estimate, it is a wrong one, and nothing about it self-corrects. It still converges under real congestion, where cutting a link's cap steers traffic off it, throughput genuinely falls, and the floor follows it down. Clamped to prev so a backoff can never raise the cap. Repro after: cap holds at 1000000 through the loss episode instead of sawtoothing to 100000. The netem test still cannot run here (needs passwordless sudo), and it now also dumps any srtla_send panic and fails loudly if the stats snapshot stops changing -- a wedged housekeeping task otherwise reads as a suspiciously stable control loop.
…wedge NamespaceProcess piped the child's stdout/stderr and did not read them until after the process exited. A piped child whose output nobody reads blocks in write() once the 64 KiB pipe buffer fills, and the harness runs srtla_send with RUST_LOG=debug, which at a few Mbps fills that in about a second. The task that does the logging is srtla_send's main select loop, so the sender wedges there while its control socket -- a separate task that logs almost nothing -- carries on answering. The netem run therefore reported a stats snapshot frozen at the values it held one second in, byte for byte, for the rest of the run. That reads like an impossibly stable control loop rather than a deadlocked one, and it is not a measurement of anything. Reproduced outside netns: the same loopback stack that runs clean with stderr redirected to a file freezes from t+1 the moment stderr is a pipe nobody reads and RUST_LOG=debug is set. Drain both pipes on background threads from spawn. stdout_lines and stderr_lines now return a snapshot and are safe to call while the process is still running. Existing netns tests never hit this only because they finish well under 64 KiB of debug output.
The topology gave each uplink its own /24 but hard-coded the receiver address to link 0's near end (10.10.1.2). Every uplink past the first was bound to a source on a different subnet, so its packets routed out link 0's veth and the receiver's strict rp_filter dropped them: the second uplink never registered, and a bonding test quietly ran on one link. The last netem run showed exactly this — link 1 sat in bootstrap with zero traffic the whole time. Model what SRTLA actually does: one receiver endpoint reached by N sender source IPs, each egressing its own interface. That needs per-source policy routing. - Receiver owns a service IP (10.99.0.1) on lo; every uplink connects there, and it is the value of receiver_ip now. - Sender gets a routing table per source IP (ip rule from <src> lookup <table>) whose route to the service IP points out that uplink's veth, so binding a socket to the source pins its traffic to the link. - Receiver returns to each sender source with the service IP as source address, so the sender's connected UDP socket accepts the reply. - rp_filter disabled on both ends: the return path lives in a policy table outside main, which strict mode drops. Retune netns_wire_loss for a real bond: both links 6 Mbps, ~9 Mbps offered, so each carries ~4.5 Mbps -- below its own ceiling (the lossy link's 2% stays wire loss) yet more than one link can supply (both stay busy). Assertions now require the lossy AND the clean link to each carry more than a quarter of the offered rate, and the bond to exceed a single link, so a one-link result fails loudly instead of passing. Still needs passwordless sudo to run, so it skips here rather than executing; the routing is verified by reasoning and compilation only.
With both links finally live, the 9 Mbps / 12 Mbps scenario (75% load) drove itself into congestion collapse: sent rates of 11-14 Mbps on 6 Mbps links, windows pinned at the floor, tens of thousands of NAKs on the clean link, and the SRT session stalling out near the end. Two causes, both fixed here. False-loss retransmit storm. The SRT caller ran at latency=200ms while the shaper buffers up to 1s (tc tbf latency 1s). A packet merely waiting its turn in the TBF aged past SRT's 200ms window and was retransmitted while the original was still queued, doubling offered load right when the bond was busiest. Raise the caller to latency=2000ms, above the buffer depth, so only genuinely lost packets are retransmitted. Too much load. Two equal links force an awkward squeeze: using both needs the offered rate above one link, i.e. above half the total, so the bond always runs hot. 9 Mbps on 12 was too hot. Back off to ~7 Mbps (665 pps) -- still over one link's 6 Mbps so both are used, but ~58% of total, close to the ~50% load the single-link run held stably. Unverified here (needs passwordless sudo); reasoning and compilation only.
The bonded netem run reaches a real regime the single-link run never did: a genuine SRT session over a lossy shaped link retransmits hard enough that the lossy link's raw send rate runs several times its goodput (40 Mbps observed on a 6 Mbps link) and swings widely. That is inherent to SRT-over-loss near capacity, and taming it needs FEC or encoder-rate adaptation that srtla_send does not have. It is orthogonal to the wire-loss ratchet this test exists for. So stop asserting throughput. `sent_bps` counts bytes queued to a link, which under retransmission is a liveness signal, not a goodput one; the old per-link-share and aggregation assertions would have passed on retransmit noise and meant nothing. Assert instead what is both robust and meaningful, and what has held on every run so far, collapsed or not: the lossy link's CC target never ratchets toward MIN_TARGET_BPS (it has stayed in the 350k-1.4M band throughout, floor is 100k), and both links stay alive so the bond is really bonding. That is the fix, and that this harness can now exercise two live links at once.
The constant-rate pump oversubscribes a busy bond and collapses it into a retransmit storm the moment it exceeds capacity, so the netns test could only assert survival, not throughput. Replace it, for tests that opt in, with an adaptive SRT sender: belacoder's congestion response minus the encoder. It generates dummy payload over a real SRT caller and lowers its bitrate when the SRT send buffer backs up or RTT inflates, so the offered rate tracks what the bonded path can carry. Dependency-light on purpose: libsrt only (already required by the whole srtla stack), no GStreamer and no patched encoder, so it stays portable where belacoder would not. The ~180-line C source is embedded via include_str! and compiled on demand with cc + pkg-config srt, gated by check_adaptive_sender_deps, so it never touches the normal cargo build and skips cleanly where the toolchain is absent. (It needs an explicit .gitignore exception because the repo blanket-ignores *.c reference files.) Verified on loopback, no root: against an unshaped 2% lossy path it settles at a stable ~4 Mbps with a shallow send buffer; against a rate-capped lossy relay (a local stand-in for the netns TBF) the RTT-based backoff keeps windows healthy and cuts NAKs ~5x versus buffer-only backoff. It does not make the run pristine — a real SRT session over a hard-capped lossy link still oscillates, since a clean steady state would need production-grade CC tuned for the path, out of scope here. So netns_wire_loss keeps its robust checks (lossy CC target never floors; both links carry a real share) and logs aggregate goodput without asserting on it.
now_ms() read SystemTime, which can step backwards on an NTP correction. Every timeout, RTT sample, and congestion deadline is a difference between two now_ms() reads (the keepalive-echo RTT path subtracts our own stamp), so a backward step clamps an RTT to zero or falsely resets a link timeout. Anchor to a monotonic Instant, keeping epoch-scale magnitude so differential arithmetic and saturating_sub call sites are unaffected.
First leaf of the sans-IO clock unification: BitrateTracker no longer reads a global now_ms(). new()/reset()/calculate() take the timestamp from the caller, which already computes now at every call site (housekeeping, connect_from_ip, reset_state). Drops the Default impl since construction needs a timestamp. Tests now drive a fixed virtual clock and read no real clock at all.
Second leaf of the clock unification. update_estimate/record_keepalive_sent/ handle_keepalive_response/needs_measurement take now from the caller instead of reading now_ms(). The receive-path callers (handle_srt_ack, send_keepalive, process_packet_internal) already compute now locally, so the change stops at the leaf and does not ripple to their callers. rtt and keepalive-interop tests now drive a fixed virtual clock.
Third leaf. handle_nak/handle_srtla_ack_enhanced/perform_window_recovery/ time_since_last_nak_ms and the enhanced free fns take now from the caller. The enhanced tests, previously racy on two separate real-clock reads, now drive a fixed virtual clock. The SrtlaConnection wrappers read now once at the connection layer and pass it down (handle_srtla_ack_specific reuses the stamp it already took); threading those wrapper signatures is deferred to the connection-layer conversion, so the ~30 handle_nak test callers stay untouched.
Fourth leaf. should_attempt_reconnect/record_attempt/reset_startup_grace take now from the caller. The backoff wrappers thread now from housekeeping's per-tick current_ms; reconnect() reads it once at the connection layer for the grace reset. The reconnect-logic test is now deterministic instead of relying on two real-clock reads landing inside the backoff window.
update_phase and clear_pre_registration_state take now from their single callers (housekeeping's current_ms, the receive-path now). Both already held a timestamp, so no ripple. Leaves is_timed_out and the send/reset boundaries, which read tokio::time::Instant, for the dual-clock migration.
The connection liveness fields (last_received/last_sent/last_keepalive_sent) and the housekeeping all_failed_at timer were tokio::time::Instant, a second clock distinct from now_ms(). Convert them to u64 monotonic ms so the whole liveness path runs on one clock. is_timed_out/needs_keepalive now compare now_ms() against the stamp (they already read now_ms for the grace check); handle_housekeeping takes now as an argument, removing its ambient read and making the all-failed timeout injectable. Tests stop advancing tokio's virtual clock (which never moved now_ms anyway) and stamp explicit past timestamps instead, which is both deterministic and honest: the old fake_clock timeout tests were asserting against a clock the logic did not actually read. tokio::time::Instant remains only where it belongs, the event-loop interval timers and the BatchSender send-coalescing window.
…indow_recovery Both are driven from housekeeping's per-tick current_ms; drop their connection-layer ambient now_ms() reads and take now as a parameter.
Both selection callers (quality multiplier, cold quality-state log) already carry current_time_ms; thread it through instead of the wrapper reading now_ms().
The widest connection-layer reader: 22 call sites, most of them deep in the hot selection path. Every caller already carries a timestamp (selection's current_time_ms/packet_time_ms, housekeeping's current_ms, stats' current_time_ms), so is_timed_out takes now as a parameter instead of reading now_ms() itself. classic::select_connection and select_pre_registration_connection gain a now parameter to pass it through; telemetry reads one now per report.
handle_srt_ack/handle_nak/handle_srtla_ack_specific take now from the caller. process_connection_events reads one monotonic timestamp per receive batch and threads it through every ACK/NAK handler (attribute_nak already carried it). This removes the last connection-layer ambient now_ms() reads on the receive path; what remains are the async I/O methods (send/flush/connect/reconnect), which read locally and move to the shell in step 2.
process_registration_packet and the packet-driven handlers (handle_reg_ngp, handle_reg2, handle_reg_err, handle_probe_response) take now from the receive path instead of reading now_ms(). The async send-scheduling methods (send_reg1_to, reg_driver_send_if_needed, try_send_reg1_immediately, probing) still read locally; like the connection send methods, they are the I/O layer that moves to the shell in step 2.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (78)
📝 WalkthroughWalkthroughThis change adds two-mode scheduling, weak-link and congestion-control selection, monotonic timing, adaptive batching, JSON-RPC control, priority windows, Prometheus metrics, TOML/reload configuration, expanded protocol validation, and network-namespace integration coverage. ChangesRuntime scheduling and transport
Control and observability
Validation and tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation