From 206303464a09b88942239ac75b3248258c12efd3 Mon Sep 17 00:00:00 2001 From: datagutt Date: Thu, 24 Sep 2026 11:56:23 +0200 Subject: [PATCH] fix(srtla_send): apply the --config toml file --config loaded the file, debug-logged it and dropped it, so no key ever took effect. Keys set in the file now fill every option not typed on the command line (command line, then file, then default). The schema now holds only the six keys that map to a flag. The other eight had nothing to feed: core constants or removed features. Unknown keys and parse errors now stop startup rather than falling back to defaults, so a stale file fails loudly instead of doing nothing. The file is read once at startup; the "reloaded on SIGHUP" claim was never true and is gone. Runtime changes go through JSON-RPC. --- README.md | 2 +- src/main.rs | 105 ++++++++++++++++++++++++++--- src/toml_config.rs | 164 ++++++++++++++++----------------------------- 3 files changed, 153 insertions(+), 118 deletions(-) diff --git a/README.md b/README.md index cd1a38ee..92ddbb12 100644 --- a/README.md +++ b/README.md @@ -133,7 +133,7 @@ srtla_send [OPTIONS] SRT_LISTEN_PORT SRTLA_HOST SRTLA_PORT BIND_IPS_FILE - `--stall-min-in-flight `: In-flight backlog (packets) at or above which a link becomes a stall candidate (default 32) - `--stall-ack-stale-ms `: Delivery-proof staleness window in milliseconds after which a stall candidate is deselected (default 3000) - `--no-rehome`: Disable whole-bond re-home (on by default). When every uplink has been down for longer than the all-links-failed window *and* the receiver hostname no longer resolves to the address the bond is pinned to, the sender moves every uplink together to the newly-resolved address and re-registers from scratch over the ordinary REG1/REG2/REG3 flow. It is deliberately conservative: a bond with any live uplink is never touched, a merely reordered DNS answer is not a move, a failed lookup is not a move, and at most one migration is attempted per minute. Pass this to keep the old behaviour of staying on the cached address until the process is restarted. -- `--config `: Path to a TOML config file (reloaded on SIGHUP) +- `--config `: Path to a TOML config file, read once at startup. Each key is the long flag name with underscores (for example `stall_ack_stale_ms = 2000`), and a flag given on the command line wins over the file. The supported keys are `mode`, `no_quality`, `no_stall_deselect`, `stall_min_in_flight`, `stall_ack_stale_ms` and `conn_timeout_ms`. An unknown key or a file that fails to parse stops startup. - `--control-socket `: Unix domain socket path for remote control (e.g., `/tmp/srtla.sock`) - `--priority-bind `: UDP sidecar address for encoder keyframe priority hints - `--metrics-bind `: Expose a Prometheus scrape endpoint at `/metrics` diff --git a/src/main.rs b/src/main.rs index 3e7fe662..ae8c9221 100644 --- a/src/main.rs +++ b/src/main.rs @@ -5,8 +5,9 @@ // two, and keeps the library's embedder-facing surface (the Android and Apple // binders no CLI ever constructs) from reading as dead code here. use anyhow::{Context, Result}; -use clap::Parser; use clap::builder::{PossibleValuesParser, TypedValueParser}; +use clap::parser::ValueSource; +use clap::{ArgMatches, CommandFactory, FromArgMatches, Parser}; use srtla_core::mode::SchedulingMode; use srtla_send::{ config, control_socket, metrics, net, priority_listener, sender, stats, subscriptions, @@ -53,7 +54,9 @@ struct Cli { #[arg(long = "control-socket")] control_socket: Option, - /// Path to TOML config file (reloaded on SIGHUP) + /// Path to a TOML config file, read once at startup. Keys match the long + /// flag names with underscores (`conn_timeout_ms`); a flag given on the + /// command line wins over the file #[arg(long = "config")] config_file: Option, @@ -128,6 +131,34 @@ struct Cli { metrics_bind: Option, } +impl Cli { + /// Replace each option the user did not type on the command line with its + /// value from the config file. The precedence is command line, then file, + /// then the clap default. + fn apply_config_file(&mut self, matches: &ArgMatches, file: toml_config::TomlConfig) { + let typed = |id: &str| matches.value_source(id) == Some(ValueSource::CommandLine); + // The derive names each arg after its field, so one identifier serves + // as both the arg id and the field to fill. + macro_rules! fill { + ($($field:ident),+ $(,)?) => {$( + if let Some(value) = file.$field { + if !typed(stringify!($field)) { + self.$field = value; + } + } + )+}; + } + fill!( + mode, + no_quality, + no_stall_deselect, + stall_min_in_flight, + stall_ack_stale_ms, + conn_timeout_ms, + ); + } +} + /// Warn when a sidecar is bound to a non-loopback address. These endpoints /// are unauthenticated same-device IPC (encoder front-end and local scrapers), /// so a routable bind exposes an open control / scrape surface. We warn rather @@ -150,23 +181,24 @@ async fn main() -> Result<()> { .with_target(false) .init(); - let args = Cli::parse(); + let matches = Cli::command().get_matches(); + let mut args = Cli::from_arg_matches(&matches).unwrap_or_else(|e| e.exit()); if args.print_version { println!("{}", version::version_line()); return Ok(()); } + if let Some(path) = args.config_file.clone() { + let file = toml_config::TomlConfig::load(std::path::Path::new(&path))?; + tracing::info!("loaded config from {path}"); + args.apply_config_file(&matches, file); + } + let local_srt_port = args.local_srt_port.expect("required"); let receiver_host = args.receiver_host.as_deref().expect("required"); let receiver_port = args.receiver_port.expect("required"); let ips_file = args.ips_file.as_deref().expect("required"); - // Load TOML config (if specified), then apply CLI overrides - if let Some(ref path) = args.config_file { - let toml_cfg = toml_config::TomlConfig::load_or_default(std::path::Path::new(path)); - tracing::debug!("TOML config loaded: {:?}", toml_cfg); - } - let config = config::DynamicConfig::from_cli( args.mode, args.no_quality, @@ -244,3 +276,58 @@ async fn main() -> Result<()> { .await .context("srtla_send failed") } + +#[cfg(test)] +mod tests { + use super::*; + + fn parse_with_file(flags: &[&str], file: &str) -> Cli { + let argv = ["srtla_send", "5000", "rec.example", "5001", "ips.txt"] + .into_iter() + .chain(flags.iter().copied()); + let matches = Cli::command().try_get_matches_from(argv).unwrap(); + let mut cli = Cli::from_arg_matches(&matches).unwrap(); + cli.apply_config_file(&matches, toml::from_str(file).unwrap()); + cli + } + + #[test] + fn file_overrides_clap_defaults() { + let cli = parse_with_file( + &[], + r#" + mode = "classic" + no_quality = true + no_stall_deselect = true + stall_min_in_flight = 64 + stall_ack_stale_ms = 1500 + conn_timeout_ms = 8000 + "#, + ); + assert_eq!(cli.mode, SchedulingMode::Classic); + assert!(cli.no_quality); + assert!(cli.no_stall_deselect); + assert_eq!(cli.stall_min_in_flight, 64); + assert_eq!(cli.stall_ack_stale_ms, 1500); + assert_eq!(cli.conn_timeout_ms, 8000); + } + + #[test] + fn typed_flag_overrides_file_even_at_default_value() { + let default = config::CONN_TIMEOUT_MS.to_string(); + let cli = parse_with_file( + &["--conn-timeout-ms", &default, "--no-quality"], + "conn_timeout_ms = 8000\nno_quality = false", + ); + assert_eq!(cli.conn_timeout_ms, config::CONN_TIMEOUT_MS); + assert!(cli.no_quality); + } + + #[test] + fn absent_file_keys_keep_cli_values() { + let cli = parse_with_file(&["--stall-ack-stale-ms", "2000"], ""); + assert_eq!(cli.mode, SchedulingMode::Enhanced); + assert_eq!(cli.stall_ack_stale_ms, 2000); + assert_eq!(cli.conn_timeout_ms, config::CONN_TIMEOUT_MS); + } +} diff --git a/src/toml_config.rs b/src/toml_config.rs index 55618d6e..c5a979b1 100644 --- a/src/toml_config.rs +++ b/src/toml_config.rs @@ -1,94 +1,44 @@ //! Optional TOML file configuration for srtla_send. //! -//! Loaded at startup via `--config ` and reloaded on SIGHUP. -//! All fields use `#[serde(default)]` so a partial config file is valid. +//! Loaded once at startup via `--config `. Each key mirrors the CLI flag +//! of the same name, and a flag typed on the command line wins over the file. +//! Unknown keys fail the load, so a misspelled or retired key stops startup +//! instead of silently doing nothing. use std::path::Path; -use serde::Deserialize; -use tracing::{info, warn}; +use anyhow::{Context, Result}; +use serde::{Deserialize, Deserializer}; +use srtla_core::mode::SchedulingMode; -/// Top-level TOML configuration. -#[derive(Debug, Clone, Deserialize)] -#[serde(default)] +/// Values read from the config file. `None` means the key was absent. +#[derive(Debug, Default, Deserialize)] +#[serde(default, deny_unknown_fields)] pub struct TomlConfig { - /// Scheduling mode: classic, enhanced. - pub mode: String, - /// Disable quality scoring. - pub no_quality: bool, - /// Disable the stalled-link deselect guard (on by default). - pub no_stall_deselect: bool, - /// In-flight backlog at or above which a link becomes a stall candidate. - pub stall_min_in_flight: i32, - /// Delivery-proof staleness window (ms) before a stall candidate is deselected. - pub stall_ack_stale_ms: u64, - /// Per-link liveness timeout (ms); silence past this re-registers the link. - pub conn_timeout_ms: u64, - - // --- Congestion control --- - /// RTT velocity threshold (ms/sample) above which window recovery is halved. - pub rtt_velocity_gate: f64, - - // --- Link lifecycle --- - /// RTT probes required during warming phase before going Live. - pub warming_rtt_probes: u32, - /// Maximum time (ms) in warming phase before auto-promoting. - pub warming_timeout_ms: u64, - /// Quality threshold below which a Live link becomes Degraded. - pub degraded_quality_threshold: f64, - /// NAK burst count threshold for degradation. - pub degraded_nak_burst_threshold: i32, - /// Cooldown duration (ms) before re-entering Live from Degraded. - pub cooldown_duration_ms: u64, - - // --- Selection --- - /// Minimum time (ms) between connection switches. - pub min_switch_interval_ms: u64, - /// Switch hysteresis threshold (1.10 = 10% better required). - pub switch_hysteresis: f64, + #[serde(deserialize_with = "deserialize_mode")] + pub mode: Option, + pub no_quality: Option, + pub no_stall_deselect: Option, + pub stall_min_in_flight: Option, + pub stall_ack_stale_ms: Option, + pub conn_timeout_ms: Option, } -impl Default for TomlConfig { - fn default() -> Self { - Self { - mode: "enhanced".to_string(), - no_quality: false, - no_stall_deselect: false, - stall_min_in_flight: crate::config::STALL_MIN_IN_FLIGHT_PACKETS, - stall_ack_stale_ms: crate::config::STALL_ACK_STALE_MS, - conn_timeout_ms: crate::config::CONN_TIMEOUT_MS, - rtt_velocity_gate: 2.0, - warming_rtt_probes: 2, - warming_timeout_ms: 5_000, - degraded_quality_threshold: 0.5, - degraded_nak_burst_threshold: 5, - cooldown_duration_ms: 5_000, - min_switch_interval_ms: 15, - switch_hysteresis: 1.10, - } - } +// `SchedulingMode` is a pure core type with no serde dependency, so the file +// goes through the same `FromStr` the CLI uses. +fn deserialize_mode<'de, D: Deserializer<'de>>(d: D) -> Result, D::Error> { + String::deserialize(d)? + .parse() + .map(Some) + .map_err(serde::de::Error::custom) } impl TomlConfig { - /// Load config from a TOML file. - pub fn load(path: &Path) -> Result { - let content = - std::fs::read_to_string(path).map_err(|e| format!("failed to read {path:?}: {e}"))?; - toml::from_str(&content).map_err(|e| format!("failed to parse {path:?}: {e}")) - } - - /// Load config, logging errors and falling back to defaults. - pub fn load_or_default(path: &Path) -> Self { - match Self::load(path) { - Ok(cfg) => { - info!("loaded config from {}", path.display()); - cfg - } - Err(e) => { - warn!("config load failed: {e}, using defaults"); - Self::default() - } - } + pub fn load(path: &Path) -> Result { + let content = std::fs::read_to_string(path) + .with_context(|| format!("failed to read config file {}", path.display()))?; + toml::from_str(&content) + .with_context(|| format!("failed to parse config file {}", path.display())) } } @@ -97,43 +47,41 @@ mod tests { use super::*; #[test] - fn test_defaults() { - let cfg = TomlConfig::default(); - assert_eq!(cfg.mode, "enhanced"); - assert!(!cfg.no_quality); - assert!((cfg.rtt_velocity_gate - 2.0).abs() < f64::EPSILON); + fn absent_keys_are_none() { + let cfg: TomlConfig = toml::from_str("mode = \"classic\"").unwrap(); + assert_eq!(cfg.mode, Some(SchedulingMode::Classic)); + assert_eq!(cfg.no_quality, None); + assert_eq!(cfg.conn_timeout_ms, None); } #[test] - fn test_partial_toml() { + fn full_toml() { let toml_str = r#" - mode = "classic" - rtt_velocity_gate = 3.5 + mode = "enhanced" + no_quality = true + no_stall_deselect = true + stall_min_in_flight = 64 + stall_ack_stale_ms = 1500 + conn_timeout_ms = 8000 "#; let cfg: TomlConfig = toml::from_str(toml_str).unwrap(); - assert_eq!(cfg.mode, "classic"); - assert!((cfg.rtt_velocity_gate - 3.5).abs() < f64::EPSILON); - // Defaults for unspecified fields - assert!(!cfg.no_quality); + assert_eq!(cfg.mode, Some(SchedulingMode::Enhanced)); + assert_eq!(cfg.no_quality, Some(true)); + assert_eq!(cfg.no_stall_deselect, Some(true)); + assert_eq!(cfg.stall_min_in_flight, Some(64)); + assert_eq!(cfg.stall_ack_stale_ms, Some(1500)); + assert_eq!(cfg.conn_timeout_ms, Some(8000)); } #[test] - fn test_full_toml() { - let toml_str = r#" - mode = "classic" - no_quality = true - rtt_velocity_gate = 1.0 - warming_rtt_probes = 3 - warming_timeout_ms = 10000 - degraded_quality_threshold = 0.3 - degraded_nak_burst_threshold = 10 - cooldown_duration_ms = 8000 - min_switch_interval_ms = 30 - switch_hysteresis = 1.20 - "#; - let cfg: TomlConfig = toml::from_str(toml_str).unwrap(); - assert_eq!(cfg.mode, "classic"); - assert!(cfg.no_quality); - assert_eq!(cfg.warming_rtt_probes, 3); + fn unknown_key_is_rejected() { + let err = toml::from_str::("switch_hysteresis = 1.2").unwrap_err(); + assert!(err.to_string().contains("switch_hysteresis"), "{err}"); + } + + #[test] + fn invalid_mode_is_rejected() { + let err = toml::from_str::("mode = \"fastest\"").unwrap_err(); + assert!(err.to_string().contains("fastest"), "{err}"); } }