From 2e8062d17ae32f6447be43703887eb8b588081d0 Mon Sep 17 00:00:00 2001 From: "svc-glamr@nvidia.com" Date: Mon, 3 Aug 2026 22:28:59 +0000 Subject: [PATCH 01/10] fix(runtime): fail fast on invalid DYN_REQUEST_PLANE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Closes #12552. `RequestPlaneMode::from_env` discarded the `FromStr` error with `.and_then(|s| s.parse().ok()).unwrap_or_default()`, so a typo such as `DYN_REQUEST_PLANE=nat` was indistinguishable from the variable being unset: both silently resolved to `RequestPlaneMode::Tcp`. An operator who deliberately configured NATS would start on TCP with no error, warning, or log line. `from_env` now returns `anyhow::Result` and makes the three-way contract explicit: absent or empty stays the historical `Tcp` default; a valid value (`nats` / `tcp`, case-insensitive) resolves as before; anything else propagates the existing `FromStr` error, which already names both the offending value and the valid options. No second error message was introduced. `from_env` is private, so the fallibility stays internal to the file. Both call sites — `DistributedConfig::from_settings` and `DistributedConfig::for_cli` — fail fast via `unwrap_or_else(|err| panic!("{err}"))`, the same convention `from_settings` already applies to the sibling `DYN_DISCOVERY_BACKEND` variable twelve lines below. The public signatures of both constructors are unchanged, so no downstream consumer of the published `dynamo-runtime` crate is broken. `DYN_EVENT_PLANE`, which deliberately implements a warn-and-default policy, and the default request plane itself are both untouched. ## Validation New ungated `#[cfg(test)] mod request_plane_env_tests` in `lib/runtime/src/distributed.rs`. It is deliberately *not* behind the `integration` feature: the pre-existing `mod tests` in this file is gated behind `#[cfg(all(test, feature = "integration"))]`, which `cargo test -p dynamo-runtime --lib` does not compile, so tests placed there would silently never run. - `cargo fmt --all --check` — clean. - `cargo check --workspace --all-targets` — clean; no caller churn, confirming the private-seam assumption. - `cargo clippy -p dynamo-runtime --all-targets --no-deps -- -D warnings` — clean. The full-workspace clippy fails on `unknown lint: 'clippy::manual_option_zip'` at `lib/llm/src/discovery/model_manager.rs:1749`; that failure reproduces identically on the unmodified tree and is unrelated to this change. - `cargo test -p dynamo-runtime --lib` — `test result: ok. 507 passed; 0 failed; 2 ignored`, with all five new tests confirmed present in the run output. Regression discrimination was demonstrated rather than assumed: with the old `from_env` body temporarily restored, `invalid_request_plane_is_an_error_naming_ value_and_options` and `from_settings_aborts_on_invalid_request_plane` both FAIL, while the absent, empty, and valid-value controls still pass. Signed-off-by: svc-glamr@nvidia.com --- lib/runtime/src/distributed.rs | 112 ++++++++++++++++++++++++++++++--- 1 file changed, 105 insertions(+), 7 deletions(-) diff --git a/lib/runtime/src/distributed.rs b/lib/runtime/src/distributed.rs index fcc4bcca8583..b867af9682b9 100644 --- a/lib/runtime/src/distributed.rs +++ b/lib/runtime/src/distributed.rs @@ -679,7 +679,10 @@ pub struct DistributedConfig { impl DistributedConfig { pub fn from_settings() -> DistributedConfig { - let request_plane = RequestPlaneMode::from_env(); + // Fail fast on a present-but-invalid DYN_REQUEST_PLANE, surfacing the parse error so the + // operator sees the offending value and the valid options. Same convention as the + // DYN_DISCOVERY_BACKEND handling a few lines below. + let request_plane = RequestPlaneMode::from_env().unwrap_or_else(|err| panic!("{err}")); // Determine the discovery backend first — we need it to compute the NATS default below. // Valid values for DYN_DISCOVERY_BACKEND: "kubernetes", "etcd" (default), "file", "mem" @@ -739,7 +742,7 @@ impl DistributedConfig { attach_lease: false, ..Default::default() }; - let request_plane = RequestPlaneMode::from_env(); + let request_plane = RequestPlaneMode::from_env().unwrap_or_else(|err| panic!("{err}")); let discovery_backend = DiscoveryBackend::KvStore(kv::Selector::Etcd(Box::new(etcd_config))); let event_transport_kind = discovery_backend.resolve_event_transport_kind(); @@ -816,11 +819,21 @@ impl std::str::FromStr for RequestPlaneMode { impl RequestPlaneMode { /// Get the request plane mode from environment variable (uncached) /// Reads from `DYN_REQUEST_PLANE` environment variable. - fn from_env() -> Self { - std::env::var("DYN_REQUEST_PLANE") - .ok() - .and_then(|s| s.parse().ok()) - .unwrap_or_default() + /// + /// The variable has a three-way contract: + /// - Unset, or set to the empty string: the default, [`RequestPlaneMode::Tcp`]. + /// - Set to a valid value (`nats` / `tcp`, case-insensitive): that mode. + /// - Set to anything else: `Err` carrying the [`std::str::FromStr`] error, which names both + /// the offending value and the valid options. A typo such as `DYN_REQUEST_PLANE=nat` must + /// not silently start the process on a transport the operator did not configure. + fn from_env() -> Result { + match std::env::var("DYN_REQUEST_PLANE") { + // Unset or empty: keep the historical default. Empty is treated as absent to match + // the adjacent `DYN_EVENT_PLANE` handling in `resolve_event_transport_kind`. + Err(_) => Ok(Self::default()), + Ok(s) if s.is_empty() => Ok(Self::default()), + Ok(s) => s.parse(), + } } pub fn is_nats(&self) -> bool { @@ -828,6 +841,91 @@ impl RequestPlaneMode { } } +/// Tests for `DYN_REQUEST_PLANE` resolution. +/// +/// Deliberately **not** gated behind the `integration` feature: these exercise pure environment +/// parsing with no I/O, so they must run under a plain `cargo test -p dynamo-runtime --lib`. +#[cfg(test)] +mod request_plane_env_tests { + use super::{DistributedConfig, RequestPlaneMode}; + use std::sync::Mutex; + + /// Environment variables are process-global and Rust runs tests in parallel by default, so + /// every test that mutates `DYN_REQUEST_PLANE` serializes through this lock. A `#[should_panic]` + /// test poisons the mutex on the way out, so recover the guard rather than propagating. + static ENV_LOCK: Mutex<()> = Mutex::new(()); + + fn with_request_plane(value: Option<&str>, f: impl FnOnce() -> T) -> T { + let _guard = ENV_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + match value { + Some(v) => temp_env::with_vars([("DYN_REQUEST_PLANE", Some(v))], f), + None => temp_env::with_vars_unset(["DYN_REQUEST_PLANE"], f), + } + } + + /// Negative control: an absent variable must keep resolving to the historical TCP default. + /// This is the case the fix must *not* disturb. + #[test] + fn absent_request_plane_defaults_to_tcp() { + let mode = with_request_plane(None, RequestPlaneMode::from_env) + .expect("an absent DYN_REQUEST_PLANE must not be an error"); + assert_eq!(mode, RequestPlaneMode::Tcp); + } + + /// An empty value is treated as absent, matching the adjacent `DYN_EVENT_PLANE` handling. + #[test] + fn empty_request_plane_defaults_to_tcp() { + let mode = with_request_plane(Some(""), RequestPlaneMode::from_env) + .expect("an empty DYN_REQUEST_PLANE must not be an error"); + assert_eq!(mode, RequestPlaneMode::Tcp); + } + + /// Valid values, including mixed case, still resolve to the matching variant. + #[test] + fn valid_request_plane_values_resolve() { + for (value, expected) in [ + ("nats", RequestPlaneMode::Nats), + ("tcp", RequestPlaneMode::Tcp), + ("NaTs", RequestPlaneMode::Nats), + ("TCP", RequestPlaneMode::Tcp), + ] { + let mode = with_request_plane(Some(value), RequestPlaneMode::from_env) + .unwrap_or_else(|err| panic!("DYN_REQUEST_PLANE={value} should resolve: {err}")); + assert_eq!(mode, expected, "DYN_REQUEST_PLANE={value}"); + } + } + + /// Regression test for the reported defect: `DYN_REQUEST_PLANE=nat` used to be swallowed and + /// silently resolve to TCP. It must now surface the `FromStr` error, naming both the offending + /// value and the valid options. + #[test] + fn invalid_request_plane_is_an_error_naming_value_and_options() { + let err = with_request_plane(Some("nat"), RequestPlaneMode::from_env) + .expect_err("a misspelled DYN_REQUEST_PLANE must not silently fall back to TCP"); + let message = err.to_string(); + assert!( + message.contains("nat"), + "error should name the offending value, got: {message}" + ); + assert!( + message.contains("'nats'") && message.contains("'tcp'"), + "error should list the valid options, got: {message}" + ); + } + + /// The fail-fast must reach the real configuration entry point, not stop at the private helper. + /// Before the fix this returned a `DistributedConfig` with `request_plane == Tcp`. + #[test] + #[should_panic(expected = "Invalid request plane mode: 'nat'")] + fn from_settings_aborts_on_invalid_request_plane() { + with_request_plane(Some("nat"), || { + let _ = DistributedConfig::from_settings(); + }); + } +} + pub mod distributed_test_utils { //! Common test helper functions for DistributedRuntime tests From 9bd9d77b02d6139d3453db51bc3d2c590f6e77c7 Mon Sep 17 00:00:00 2001 From: "svc-glamr@nvidia.com" Date: Tue, 4 Aug 2026 00:25:49 +0000 Subject: [PATCH 02/10] fix(runtime): reject non-Unicode DYN_REQUEST_PLANE instead of defaulting `std::env::var` reports a present-but-non-Unicode value as `VarError::NotUnicode`, not as a successful read. Matching every `Err` as absence therefore let `DYN_REQUEST_PLANE=$'nat\xff'` silently start the process on TCP -- the same silent-fallback defect this change set exists to remove, reached through a different door. Match `VarError::NotPresent` explicitly for the TCP default and report `NotUnicode` as an invalid value, rendered lossily so the operator can see roughly what they typed. Adds a Unix-only regression test; it fails against the previous `Err(_)` arm (returns Tcp) and passes against this one. Also corrects the request-plane doc page, which still described an invalid value as falling back to `tcp`. Addresses review feedback from CodeRabbit and the change's own review packet. Signed-off-by: svc-glamr@nvidia.com --- .../communication-planes/request-plane.md | 7 ++- lib/runtime/src/distributed.rs | 47 ++++++++++++++++++- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/docs/fern/pages/developer-guide/knowledge-base/concepts/communication-planes/request-plane.md b/docs/fern/pages/developer-guide/knowledge-base/concepts/communication-planes/request-plane.md index 11f6ed300b56..f4d2885b63e2 100644 --- a/docs/fern/pages/developer-guide/knowledge-base/concepts/communication-planes/request-plane.md +++ b/docs/fern/pages/developer-guide/knowledge-base/concepts/communication-planes/request-plane.md @@ -56,7 +56,9 @@ The value is case-insensitive. ### Default Behavior -If `DYN_REQUEST_PLANE` is not set or contains an invalid value, Dynamo defaults to `tcp`. +If `DYN_REQUEST_PLANE` is not set, or is set to the empty string, Dynamo defaults to `tcp`. If it is +set to any other unrecognized value, startup fails with an error naming the offending value and the +valid options, rather than silently falling back to `tcp`. ## Usage Examples @@ -216,7 +218,8 @@ curl http://localhost:8000/v1/chat/completions \ **Solutions:** - Check `DYN_REQUEST_PLANE` spelling (valid values: `nats`, `tcp`) - Value is case-insensitive but must be one of the two options -- If not set, defaults to `tcp` +- If not set, or set to the empty string, defaults to `tcp`; any other unrecognized value is an + error rather than a fallback to `tcp` ### Issue: Port Conflicts diff --git a/lib/runtime/src/distributed.rs b/lib/runtime/src/distributed.rs index b867af9682b9..5b364f802028 100644 --- a/lib/runtime/src/distributed.rs +++ b/lib/runtime/src/distributed.rs @@ -826,13 +826,26 @@ impl RequestPlaneMode { /// - Set to anything else: `Err` carrying the [`std::str::FromStr`] error, which names both /// the offending value and the valid options. A typo such as `DYN_REQUEST_PLANE=nat` must /// not silently start the process on a transport the operator did not configure. + /// + /// "Anything else" includes a value that is not valid Unicode. [`std::env::var`] reports that + /// as [`std::env::VarError::NotUnicode`] rather than as a successful read, so matching every + /// `Err` as absence would reintroduce exactly the silent fallback this function exists to + /// prevent: `DYN_REQUEST_PLANE=$'nat\xff'` is a present, misspelled value, not an unset one. fn from_env() -> Result { match std::env::var("DYN_REQUEST_PLANE") { // Unset or empty: keep the historical default. Empty is treated as absent to match // the adjacent `DYN_EVENT_PLANE` handling in `resolve_event_transport_kind`. - Err(_) => Ok(Self::default()), + Err(std::env::VarError::NotPresent) => Ok(Self::default()), Ok(s) if s.is_empty() => Ok(Self::default()), Ok(s) => s.parse(), + // Present but not valid Unicode. Neither valid option contains a non-UTF-8 byte, so + // such a value is always invalid; report it in the same shape as any other invalid + // value, rendered lossily so the operator can see roughly what they typed. + Err(std::env::VarError::NotUnicode(raw)) => Err(anyhow::anyhow!( + "Invalid request plane mode: '{}' is not valid Unicode. \ + Valid options are: 'nats', 'tcp'", + raw.to_string_lossy() + )), } } @@ -915,6 +928,38 @@ mod request_plane_env_tests { ); } + /// A present-but-non-Unicode value is a *present* value, and must not be mistaken for an unset + /// one. `std::env::var` surfaces it as `VarError::NotUnicode`, so a blanket `Err(_) => default` + /// arm would silently start the process on TCP — the very defect this change removes, reached + /// by a different door. Unix-only: constructing a non-Unicode `OsString` from raw bytes needs + /// `OsStringExt`, whose Windows counterpart takes `u16` and has different validity rules. + #[cfg(unix)] + #[test] + fn non_unicode_request_plane_is_an_error_not_a_default() { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + // "nat" plus a lone 0xFF continuation byte: invalid UTF-8 in any position. + let raw = OsString::from_vec(b"nat\xff".to_vec()); + let _guard = ENV_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let err = temp_env::with_vars( + [("DYN_REQUEST_PLANE", Some(&raw))], + RequestPlaneMode::from_env, + ) + .expect_err("a non-Unicode DYN_REQUEST_PLANE must not silently fall back to TCP"); + let message = err.to_string(); + assert!( + message.contains("not valid Unicode"), + "error should say the value was not valid Unicode, got: {message}" + ); + assert!( + message.contains("'nats'") && message.contains("'tcp'"), + "error should list the valid options, got: {message}" + ); + } + /// The fail-fast must reach the real configuration entry point, not stop at the private helper. /// Before the fix this returned a `DistributedConfig` with `request_plane == Tcp`. #[test] From b3b5539d7b3724c5e19edaa837d09d592fc57f39 Mon Sep 17 00:00:00 2001 From: Matej Kosec Date: Thu, 20 Aug 2026 15:40:47 -0700 Subject: [PATCH 03/10] refactor(runtime): trim request-plane commentary Signed-off-by: Matej Kosec --- lib/runtime/src/distributed.rs | 47 +++------------------------------- 1 file changed, 3 insertions(+), 44 deletions(-) diff --git a/lib/runtime/src/distributed.rs b/lib/runtime/src/distributed.rs index 8d44f8c946a3..f815a834f447 100644 --- a/lib/runtime/src/distributed.rs +++ b/lib/runtime/src/distributed.rs @@ -694,9 +694,6 @@ pub struct DistributedConfig { impl DistributedConfig { pub fn from_settings() -> DistributedConfig { - // Fail fast on a present-but-invalid DYN_REQUEST_PLANE, surfacing the parse error so the - // operator sees the offending value and the valid options. Same convention as the - // DYN_DISCOVERY_BACKEND handling a few lines below. let request_plane = RequestPlaneMode::from_env().unwrap_or_else(|err| panic!("{err}")); // Determine the discovery backend first — we need it to compute the NATS default below. @@ -832,30 +829,12 @@ impl std::str::FromStr for RequestPlaneMode { } impl RequestPlaneMode { - /// Get the request plane mode from environment variable (uncached) - /// Reads from `DYN_REQUEST_PLANE` environment variable. - /// - /// The variable has a three-way contract: - /// - Unset, or set to the empty string: the default, [`RequestPlaneMode::Tcp`]. - /// - Set to a valid value (`nats` / `tcp`, case-insensitive): that mode. - /// - Set to anything else: `Err` carrying the [`std::str::FromStr`] error, which names both - /// the offending value and the valid options. A typo such as `DYN_REQUEST_PLANE=nat` must - /// not silently start the process on a transport the operator did not configure. - /// - /// "Anything else" includes a value that is not valid Unicode. [`std::env::var`] reports that - /// as [`std::env::VarError::NotUnicode`] rather than as a successful read, so matching every - /// `Err` as absence would reintroduce exactly the silent fallback this function exists to - /// prevent: `DYN_REQUEST_PLANE=$'nat\xff'` is a present, misspelled value, not an unset one. + /// Reads `DYN_REQUEST_PLANE`, defaulting only when it is absent or empty. fn from_env() -> Result { match std::env::var("DYN_REQUEST_PLANE") { - // Unset or empty: keep the historical default. Empty is treated as absent to match - // the adjacent `DYN_EVENT_PLANE` handling in `resolve_event_transport_kind`. Err(std::env::VarError::NotPresent) => Ok(Self::default()), Ok(s) if s.is_empty() => Ok(Self::default()), Ok(s) => s.parse(), - // Present but not valid Unicode. Neither valid option contains a non-UTF-8 byte, so - // such a value is always invalid; report it in the same shape as any other invalid - // value, rendered lossily so the operator can see roughly what they typed. Err(std::env::VarError::NotUnicode(raw)) => Err(anyhow::anyhow!( "Invalid request plane mode: '{}' is not valid Unicode. \ Valid options are: 'nats', 'tcp'", @@ -869,18 +848,13 @@ impl RequestPlaneMode { } } -/// Tests for `DYN_REQUEST_PLANE` resolution. -/// -/// Deliberately **not** gated behind the `integration` feature: these exercise pure environment -/// parsing with no I/O, so they must run under a plain `cargo test -p dynamo-runtime --lib`. #[cfg(test)] mod request_plane_env_tests { use super::{DistributedConfig, RequestPlaneMode}; use std::sync::Mutex; - /// Environment variables are process-global and Rust runs tests in parallel by default, so - /// every test that mutates `DYN_REQUEST_PLANE` serializes through this lock. A `#[should_panic]` - /// test poisons the mutex on the way out, so recover the guard rather than propagating. + // Environment variables are process-global. The panic test poisons this lock, + // so subsequent acquisitions recover its guard. static ENV_LOCK: Mutex<()> = Mutex::new(()); fn with_request_plane(value: Option<&str>, f: impl FnOnce() -> T) -> T { @@ -893,8 +867,6 @@ mod request_plane_env_tests { } } - /// Negative control: an absent variable must keep resolving to the historical TCP default. - /// This is the case the fix must *not* disturb. #[test] fn absent_request_plane_defaults_to_tcp() { let mode = with_request_plane(None, RequestPlaneMode::from_env) @@ -902,7 +874,6 @@ mod request_plane_env_tests { assert_eq!(mode, RequestPlaneMode::Tcp); } - /// An empty value is treated as absent, matching the adjacent `DYN_EVENT_PLANE` handling. #[test] fn empty_request_plane_defaults_to_tcp() { let mode = with_request_plane(Some(""), RequestPlaneMode::from_env) @@ -910,7 +881,6 @@ mod request_plane_env_tests { assert_eq!(mode, RequestPlaneMode::Tcp); } - /// Valid values, including mixed case, still resolve to the matching variant. #[test] fn valid_request_plane_values_resolve() { for (value, expected) in [ @@ -925,9 +895,6 @@ mod request_plane_env_tests { } } - /// Regression test for the reported defect: `DYN_REQUEST_PLANE=nat` used to be swallowed and - /// silently resolve to TCP. It must now surface the `FromStr` error, naming both the offending - /// value and the valid options. #[test] fn invalid_request_plane_is_an_error_naming_value_and_options() { let err = with_request_plane(Some("nat"), RequestPlaneMode::from_env) @@ -943,18 +910,12 @@ mod request_plane_env_tests { ); } - /// A present-but-non-Unicode value is a *present* value, and must not be mistaken for an unset - /// one. `std::env::var` surfaces it as `VarError::NotUnicode`, so a blanket `Err(_) => default` - /// arm would silently start the process on TCP — the very defect this change removes, reached - /// by a different door. Unix-only: constructing a non-Unicode `OsString` from raw bytes needs - /// `OsStringExt`, whose Windows counterpart takes `u16` and has different validity rules. #[cfg(unix)] #[test] fn non_unicode_request_plane_is_an_error_not_a_default() { use std::ffi::OsString; use std::os::unix::ffi::OsStringExt; - // "nat" plus a lone 0xFF continuation byte: invalid UTF-8 in any position. let raw = OsString::from_vec(b"nat\xff".to_vec()); let _guard = ENV_LOCK .lock() @@ -975,8 +936,6 @@ mod request_plane_env_tests { ); } - /// The fail-fast must reach the real configuration entry point, not stop at the private helper. - /// Before the fix this returned a `DistributedConfig` with `request_plane == Tcp`. #[test] #[should_panic(expected = "Invalid request plane mode: 'nat'")] fn from_settings_aborts_on_invalid_request_plane() { From b829bdae716a0728c7f355efc1e8c73ef9411f7f Mon Sep 17 00:00:00 2001 From: Matej Kosec Date: Mon, 31 Aug 2026 10:05:30 -0700 Subject: [PATCH 04/10] style(runtime): apply rustfmt Signed-off-by: Matej Kosec --- lib/runtime/src/distributed.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/runtime/src/distributed.rs b/lib/runtime/src/distributed.rs index e8bed928e5ae..52068c87754d 100644 --- a/lib/runtime/src/distributed.rs +++ b/lib/runtime/src/distributed.rs @@ -831,9 +831,7 @@ impl std::str::FromStr for RequestPlaneMode { impl RequestPlaneMode { /// Reads `DYN_REQUEST_PLANE`, defaulting only when it is absent or empty. fn from_env() -> Result { - match std::env::var( - crate::config::environment_names::request_plane::DYN_REQUEST_PLANE, - ) { + match std::env::var(crate::config::environment_names::request_plane::DYN_REQUEST_PLANE) { Err(std::env::VarError::NotPresent) => Ok(Self::default()), Ok(s) if s.is_empty() => Ok(Self::default()), Ok(s) => s.parse(), From ceb83fc8ce87a7d4987b7333d2d18800ad2c1a39 Mon Sep 17 00:00:00 2001 From: Matej Kosec Date: Mon, 31 Aug 2026 10:11:20 -0700 Subject: [PATCH 05/10] test(runtime): isolate request-plane environment parsing Signed-off-by: Matej Kosec --- lib/runtime/src/distributed.rs | 52 +++++++++------------------------- 1 file changed, 14 insertions(+), 38 deletions(-) diff --git a/lib/runtime/src/distributed.rs b/lib/runtime/src/distributed.rs index 52068c87754d..158255a8fc64 100644 --- a/lib/runtime/src/distributed.rs +++ b/lib/runtime/src/distributed.rs @@ -831,7 +831,13 @@ impl std::str::FromStr for RequestPlaneMode { impl RequestPlaneMode { /// Reads `DYN_REQUEST_PLANE`, defaulting only when it is absent or empty. fn from_env() -> Result { - match std::env::var(crate::config::environment_names::request_plane::DYN_REQUEST_PLANE) { + Self::from_env_result(std::env::var( + crate::config::environment_names::request_plane::DYN_REQUEST_PLANE, + )) + } + + fn from_env_result(value: std::result::Result) -> Result { + match value { Err(std::env::VarError::NotPresent) => Ok(Self::default()), Ok(s) if s.is_empty() => Ok(Self::default()), Ok(s) => s.parse(), @@ -850,33 +856,18 @@ impl RequestPlaneMode { #[cfg(test)] mod request_plane_env_tests { - use super::{DistributedConfig, RequestPlaneMode}; - use std::sync::Mutex; - - // Environment variables are process-global. The panic test poisons this lock, - // so subsequent acquisitions recover its guard. - static ENV_LOCK: Mutex<()> = Mutex::new(()); - - fn with_request_plane(value: Option<&str>, f: impl FnOnce() -> T) -> T { - let _guard = ENV_LOCK - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - match value { - Some(v) => temp_env::with_vars([("DYN_REQUEST_PLANE", Some(v))], f), - None => temp_env::with_vars_unset(["DYN_REQUEST_PLANE"], f), - } - } + use super::RequestPlaneMode; #[test] fn absent_request_plane_defaults_to_tcp() { - let mode = with_request_plane(None, RequestPlaneMode::from_env) + let mode = RequestPlaneMode::from_env_result(Err(std::env::VarError::NotPresent)) .expect("an absent DYN_REQUEST_PLANE must not be an error"); assert_eq!(mode, RequestPlaneMode::Tcp); } #[test] fn empty_request_plane_defaults_to_tcp() { - let mode = with_request_plane(Some(""), RequestPlaneMode::from_env) + let mode = RequestPlaneMode::from_env_result(Ok(String::new())) .expect("an empty DYN_REQUEST_PLANE must not be an error"); assert_eq!(mode, RequestPlaneMode::Tcp); } @@ -887,9 +878,8 @@ mod request_plane_env_tests { ("nats", RequestPlaneMode::Nats), ("tcp", RequestPlaneMode::Tcp), ("NaTs", RequestPlaneMode::Nats), - ("TCP", RequestPlaneMode::Tcp), ] { - let mode = with_request_plane(Some(value), RequestPlaneMode::from_env) + let mode = RequestPlaneMode::from_env_result(Ok(value.to_string())) .unwrap_or_else(|err| panic!("DYN_REQUEST_PLANE={value} should resolve: {err}")); assert_eq!(mode, expected, "DYN_REQUEST_PLANE={value}"); } @@ -897,7 +887,7 @@ mod request_plane_env_tests { #[test] fn invalid_request_plane_is_an_error_naming_value_and_options() { - let err = with_request_plane(Some("nat"), RequestPlaneMode::from_env) + let err = RequestPlaneMode::from_env_result(Ok("nat".to_string())) .expect_err("a misspelled DYN_REQUEST_PLANE must not silently fall back to TCP"); let message = err.to_string(); assert!( @@ -917,14 +907,8 @@ mod request_plane_env_tests { use std::os::unix::ffi::OsStringExt; let raw = OsString::from_vec(b"nat\xff".to_vec()); - let _guard = ENV_LOCK - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - let err = temp_env::with_vars( - [("DYN_REQUEST_PLANE", Some(&raw))], - RequestPlaneMode::from_env, - ) - .expect_err("a non-Unicode DYN_REQUEST_PLANE must not silently fall back to TCP"); + let err = RequestPlaneMode::from_env_result(Err(std::env::VarError::NotUnicode(raw))) + .expect_err("a non-Unicode DYN_REQUEST_PLANE must not silently fall back to TCP"); let message = err.to_string(); assert!( message.contains("not valid Unicode"), @@ -935,14 +919,6 @@ mod request_plane_env_tests { "error should list the valid options, got: {message}" ); } - - #[test] - #[should_panic(expected = "Invalid request plane mode: 'nat'")] - fn from_settings_aborts_on_invalid_request_plane() { - with_request_plane(Some("nat"), || { - let _ = DistributedConfig::from_settings(); - }); - } } pub mod distributed_test_utils { From 737a658294f3e2f7cc1dcc4e086fc0eab9aade51 Mon Sep 17 00:00:00 2001 From: Coding Agent Date: Mon, 31 Aug 2026 23:06:34 +0000 Subject: [PATCH 06/10] docs(runtime): clarify empty request-plane values Signed-off-by: Coding Agent --- .../concepts/system-architecture/architecture.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/fern/pages/developer-guide/knowledge-base/concepts/system-architecture/architecture.md b/docs/fern/pages/developer-guide/knowledge-base/concepts/system-architecture/architecture.md index 327e8d95d740..a7ef949156da 100644 --- a/docs/fern/pages/developer-guide/knowledge-base/concepts/system-architecture/architecture.md +++ b/docs/fern/pages/developer-guide/knowledge-base/concepts/system-architecture/architecture.md @@ -69,8 +69,10 @@ The request plane carries RPC traffic between Dynamo components. `DYN_REQUEST_PL - `tcp` is the default and uses direct pooled connections. - `nats` uses brokered request transport. -An absent or empty value selects `tcp`. Any other value must be `tcp` or `nats` -(case-insensitive); an unrecognized value stops startup instead of silently selecting `tcp`. +When the runtime reads this setting from its environment, an absent or empty value +selects `tcp`. Components that pass an explicit value reject an empty value. Any +other value must be `tcp` or `nats` (case-insensitive); an unrecognized value stops +startup instead of silently selecting `tcp`. `DYN_REQUEST_PLANE_CODEC` selects `msgpack` or `json`. The destination endpoint advertises its codec, so one client can communicate with endpoints that use different codecs. From 181e99ec86fe62339804ff5e98a1596c795e2c53 Mon Sep 17 00:00:00 2001 From: Coding Agent Date: Tue, 1 Sep 2026 01:54:27 +0000 Subject: [PATCH 07/10] refactor(runtime): remove redundant request-plane comment Signed-off-by: Coding Agent --- lib/runtime/src/distributed.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/runtime/src/distributed.rs b/lib/runtime/src/distributed.rs index 158255a8fc64..1bfe5278c3df 100644 --- a/lib/runtime/src/distributed.rs +++ b/lib/runtime/src/distributed.rs @@ -829,7 +829,6 @@ impl std::str::FromStr for RequestPlaneMode { } impl RequestPlaneMode { - /// Reads `DYN_REQUEST_PLANE`, defaulting only when it is absent or empty. fn from_env() -> Result { Self::from_env_result(std::env::var( crate::config::environment_names::request_plane::DYN_REQUEST_PLANE, From bf20ceead53276e0c7ef629972af47e03bdcee90 Mon Sep 17 00:00:00 2001 From: Matej Kosec Date: Tue, 8 Sep 2026 14:39:33 -0700 Subject: [PATCH 08/10] fix(runtime): reject empty request-plane values Signed-off-by: Matej Kosec --- .../system-architecture/architecture.md | 8 +++---- lib/runtime/src/distributed.rs | 22 ++++++------------- 2 files changed, 11 insertions(+), 19 deletions(-) diff --git a/docs/fern/pages/developer-guide/knowledge-base/concepts/system-architecture/architecture.md b/docs/fern/pages/developer-guide/knowledge-base/concepts/system-architecture/architecture.md index a7ef949156da..85a532593294 100644 --- a/docs/fern/pages/developer-guide/knowledge-base/concepts/system-architecture/architecture.md +++ b/docs/fern/pages/developer-guide/knowledge-base/concepts/system-architecture/architecture.md @@ -69,10 +69,10 @@ The request plane carries RPC traffic between Dynamo components. `DYN_REQUEST_PL - `tcp` is the default and uses direct pooled connections. - `nats` uses brokered request transport. -When the runtime reads this setting from its environment, an absent or empty value -selects `tcp`. Components that pass an explicit value reject an empty value. Any -other value must be `tcp` or `nats` (case-insensitive); an unrecognized value stops -startup instead of silently selecting `tcp`. +When the runtime reads this setting from its environment, an absent value selects +`tcp`. Every present value, including an empty value, must be `tcp` or `nats` +(case-insensitive); any other value stops startup instead of silently selecting +`tcp`. `DYN_REQUEST_PLANE_CODEC` selects `msgpack` or `json`. The destination endpoint advertises its codec, so one client can communicate with endpoints that use different codecs. diff --git a/lib/runtime/src/distributed.rs b/lib/runtime/src/distributed.rs index 1bfe5278c3df..06f05579a120 100644 --- a/lib/runtime/src/distributed.rs +++ b/lib/runtime/src/distributed.rs @@ -838,7 +838,6 @@ impl RequestPlaneMode { fn from_env_result(value: std::result::Result) -> Result { match value { Err(std::env::VarError::NotPresent) => Ok(Self::default()), - Ok(s) if s.is_empty() => Ok(Self::default()), Ok(s) => s.parse(), Err(std::env::VarError::NotUnicode(raw)) => Err(anyhow::anyhow!( "Invalid request plane mode: '{}' is not valid Unicode. \ @@ -865,23 +864,16 @@ mod request_plane_env_tests { } #[test] - fn empty_request_plane_defaults_to_tcp() { - let mode = RequestPlaneMode::from_env_result(Ok(String::new())) - .expect("an empty DYN_REQUEST_PLANE must not be an error"); - assert_eq!(mode, RequestPlaneMode::Tcp); + fn empty_request_plane_is_an_error() { + RequestPlaneMode::from_env_result(Ok(String::new())) + .expect_err("an empty DYN_REQUEST_PLANE must not silently fall back to TCP"); } #[test] - fn valid_request_plane_values_resolve() { - for (value, expected) in [ - ("nats", RequestPlaneMode::Nats), - ("tcp", RequestPlaneMode::Tcp), - ("NaTs", RequestPlaneMode::Nats), - ] { - let mode = RequestPlaneMode::from_env_result(Ok(value.to_string())) - .unwrap_or_else(|err| panic!("DYN_REQUEST_PLANE={value} should resolve: {err}")); - assert_eq!(mode, expected, "DYN_REQUEST_PLANE={value}"); - } + fn valid_request_plane_value_resolves() { + let mode = RequestPlaneMode::from_env_result(Ok("nats".to_string())) + .expect("DYN_REQUEST_PLANE=nats should resolve"); + assert_eq!(mode, RequestPlaneMode::Nats); } #[test] From 8ab3ca6075bce394d561c6a54dd9a50423425c96 Mon Sep 17 00:00:00 2001 From: Matej Kosec Date: Tue, 8 Sep 2026 15:49:25 -0700 Subject: [PATCH 09/10] fix(runtime): propagate request-plane configuration errors Signed-off-by: Matej Kosec --- lib/runtime/src/distributed.rs | 18 +++++++++--------- lib/runtime/src/transports/etcd.rs | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/runtime/src/distributed.rs b/lib/runtime/src/distributed.rs index edb8155684ed..efbc11bc8401 100644 --- a/lib/runtime/src/distributed.rs +++ b/lib/runtime/src/distributed.rs @@ -375,7 +375,7 @@ impl DistributedRuntime { } pub async fn from_settings(runtime: Runtime) -> Result { - let config = DistributedConfig::from_settings(); + let config = DistributedConfig::from_settings()?; Self::new(runtime, config).await } @@ -796,8 +796,8 @@ pub struct DistributedConfig { } impl DistributedConfig { - pub fn from_settings() -> DistributedConfig { - let request_plane = RequestPlaneMode::from_env().unwrap_or_else(|err| panic!("{err}")); + pub fn from_settings() -> Result { + let request_plane = RequestPlaneMode::from_env()?; // Determine the discovery backend first — we need it to compute the NATS default below. // Valid values for DYN_DISCOVERY_BACKEND: "kubernetes", "etcd" (default), "file", "mem" @@ -840,7 +840,7 @@ impl DistributedConfig { crate::discovery::EventTransportKind::Nats ); - DistributedConfig { + Ok(DistributedConfig { discovery_backend, nats_config: if nats_enabled { Some(nats::ClientOptions::default()) @@ -850,15 +850,15 @@ impl DistributedConfig { request_plane, response_plane: None, event_transport_kind, - } + }) } - pub fn for_cli() -> DistributedConfig { + pub fn for_cli() -> Result { let etcd_config = etcd::ClientOptions { attach_lease: false, ..Default::default() }; - let request_plane = RequestPlaneMode::from_env().unwrap_or_else(|err| panic!("{err}")); + let request_plane = RequestPlaneMode::from_env()?; let discovery_backend = DiscoveryBackend::KvStore(kv::Selector::Etcd(Box::new(etcd_config))); let event_transport_kind = discovery_backend.resolve_event_transport_kind(); @@ -868,7 +868,7 @@ impl DistributedConfig { event_transport_kind, crate::discovery::EventTransportKind::Nats ); - DistributedConfig { + Ok(DistributedConfig { discovery_backend, nats_config: if nats_enabled { Some(nats::ClientOptions::default()) @@ -878,7 +878,7 @@ impl DistributedConfig { request_plane, response_plane: None, event_transport_kind, - } + }) } /// A DistributedConfig that isn't distributed, for when the frontend and backend are in the diff --git a/lib/runtime/src/transports/etcd.rs b/lib/runtime/src/transports/etcd.rs index 41e90dd4be96..457d3900b41c 100644 --- a/lib/runtime/src/transports/etcd.rs +++ b/lib/runtime/src/transports/etcd.rs @@ -1256,7 +1256,7 @@ mod tests { fn test_ectd_client() { let rt = Runtime::single_threaded().unwrap(); let rt_clone = rt.clone(); - let config = DistributedConfig::from_settings(); + let config = DistributedConfig::from_settings().unwrap(); rt_clone.primary().block_on(async move { let drt = DistributedRuntime::new(rt, config).await.unwrap(); @@ -1309,7 +1309,7 @@ mod tests { fn test_kv_cache() { let rt = Runtime::single_threaded().unwrap(); let rt_clone = rt.clone(); - let config = DistributedConfig::from_settings(); + let config = DistributedConfig::from_settings().unwrap(); rt_clone.primary().block_on(async move { let drt = DistributedRuntime::new(rt, config).await.unwrap(); From af264e5bf969b1bc0bb1768cf9a26e6116cb8585 Mon Sep 17 00:00:00 2001 From: Matej Kosec Date: Tue, 8 Sep 2026 16:08:31 -0700 Subject: [PATCH 10/10] fix(runtime): preserve distributed config API Signed-off-by: Matej Kosec --- lib/runtime/src/distributed.rs | 14 +++++++++++--- lib/runtime/src/transports/etcd.rs | 4 ++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/lib/runtime/src/distributed.rs b/lib/runtime/src/distributed.rs index efbc11bc8401..3f4de56af113 100644 --- a/lib/runtime/src/distributed.rs +++ b/lib/runtime/src/distributed.rs @@ -375,7 +375,7 @@ impl DistributedRuntime { } pub async fn from_settings(runtime: Runtime) -> Result { - let config = DistributedConfig::from_settings()?; + let config = DistributedConfig::try_from_settings()?; Self::new(runtime, config).await } @@ -796,7 +796,11 @@ pub struct DistributedConfig { } impl DistributedConfig { - pub fn from_settings() -> Result { + pub fn from_settings() -> DistributedConfig { + Self::try_from_settings().unwrap_or_else(|err| panic!("{err}")) + } + + pub fn try_from_settings() -> Result { let request_plane = RequestPlaneMode::from_env()?; // Determine the discovery backend first — we need it to compute the NATS default below. @@ -853,7 +857,11 @@ impl DistributedConfig { }) } - pub fn for_cli() -> Result { + pub fn for_cli() -> DistributedConfig { + Self::try_for_cli().unwrap_or_else(|err| panic!("{err}")) + } + + pub fn try_for_cli() -> Result { let etcd_config = etcd::ClientOptions { attach_lease: false, ..Default::default() diff --git a/lib/runtime/src/transports/etcd.rs b/lib/runtime/src/transports/etcd.rs index 457d3900b41c..41e90dd4be96 100644 --- a/lib/runtime/src/transports/etcd.rs +++ b/lib/runtime/src/transports/etcd.rs @@ -1256,7 +1256,7 @@ mod tests { fn test_ectd_client() { let rt = Runtime::single_threaded().unwrap(); let rt_clone = rt.clone(); - let config = DistributedConfig::from_settings().unwrap(); + let config = DistributedConfig::from_settings(); rt_clone.primary().block_on(async move { let drt = DistributedRuntime::new(rt, config).await.unwrap(); @@ -1309,7 +1309,7 @@ mod tests { fn test_kv_cache() { let rt = Runtime::single_threaded().unwrap(); let rt_clone = rt.clone(); - let config = DistributedConfig::from_settings().unwrap(); + let config = DistributedConfig::from_settings(); rt_clone.primary().block_on(async move { let drt = DistributedRuntime::new(rt, config).await.unwrap();