Repository navigation
Conversation
## Summary Closes ai-dynamo#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<Self>` 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 <svc-glamr@nvidia.com>
glamr-agent
temporarily deployed
to
external_collaborator
August 3, 2026 23:38 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
glamr-agent
temporarily deployed
to
external_collaborator
August 3, 2026 23:38 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
github-actions
Bot
added
the
external-contribution
Contributor
Author
Automated evidence record — validation completeValidation status: complete Evidence summary: [1/1 validated] AI review assessment (advisory, not a merge authorization): sound. An AI agent judged the change logically sound on the recorded evidence: the fix sits at the seam where the defect lived, the new tests provably executed rather than being skipped by the crate's Validation result: complete — pass. Evidence audit: complete [1/1 validated] — the evidence table is grounded in recorded runs. Evidence [1/1 validated]Generated from validation/registry.jsonl — do not edit by hand.
|
Contributor
Author
plan.md# Plan — ai-dynamo/dynamo#12552: fail fast on invalid `DYN_REQUEST_PLANE`
Route: implementation
Template: debug-investigation
Engine: vllm
## Existing-review action
None applies. The caller supplied no pull request (`input.md` → `Review requests: - none recorded`,
`Review strategy: unknown`, `Current MR source branch: unknown`), and the discovery survey below
found no open request implementing this fix that we would be competing with or should help land
instead. I am therefore deliberately emitting no `Review request:` line: inventing one would point
the workflow at a request that does not exist. The publisher should open a fresh request on the
working branch `fix/request-plane-invalid-value--935d8be5f4a9` against `main`.
## User intent
The caller wants the `DYN_REQUEST_PLANE` environment variable to stop silently swallowing typos.
Today a value such as `DYN_REQUEST_PLANE=nat` is parsed, the parse fails, the error is discarded,
and the process starts on TCP as though nothing happened — which is materially dangerous for an
operator who deliberately configured NATS, because the transport actually in use is not the one
they asked for and no message ever says so.
The requested behavior is a three-way contract on the variable:
1. **Absent** — keep the existing default (`RequestPlaneMode::Tcp`). This is not a behavior change
and must not become one; a large amount of deployment tooling relies on the unset default.
2. **Present and valid** (`nats` / `tcp`, case-insensitively) — resolve to that mode, exactly as today.
3. **Present and invalid** — fail fast, surfacing the *existing* `RequestPlaneMode::from_str` error
text so the message names both the offending value and the valid options.
The caller also explicitly asks for CPU-only unit coverage of all three cases in
`lib/runtime/src/distributed.rs`. The issue author sized the work at S (11–50 lines) and stated
they are happy with either `Result` propagation or the repository's existing fail-fast
configuration convention, "depending on maintainer preference" — so the shape decision is ours to
make and to justify, and that is the main design content of this plan.
## Non-goals
- **Not** changing the default request plane. TCP remains the default when the variable is unset
or empty-string-absent; `RequestPlaneMode`'s `#[derive(Default)]` on the `Tcp` variant stays.
- **Not** touching `DYN_EVENT_PLANE`. `DiscoveryBackend::resolve_event_transport_kind`
(`lib/runtime/src/distributed.rs:648-665`) deliberately implements a *warn-and-default* policy for
invalid values. That is a different variable with a different, already-explicit contract, and
harmonizing the two is a separate design decision that would need its own issue.
- **Not** touching `DYN_REQUEST_PLANE_CODEC`, `DYN_DISCOVERY_BACKEND`, or the NATS-enablement
heuristic that reads `request_plane.is_nats()`.
- **Not** changing the Python binding path. `lib/bindings/python/rust/lib.rs:1013` already does
`request_plane.parse().map_err(to_pyerr)?` — the explicit-argument path is *already* fail-fast.
This work item closes the gap on the *environment* path only, so that both paths agree.
- **Not** adding integration, GPU, or engine-level tests. The affected function performs no I/O.
- **Not** refactoring `NetworkManager` or the TCP/NATS transport selection at
`lib/runtime/src/pipeline/network/manager.rs:166-190`. That code is the *consumer* of the bad
value and is correct; the defect is upstream of it.
## Discovery
Everything below was read or executed against the checkout at commit `e258f04f6` on `main`.
### The defect, confirmed in source
`lib/runtime/src/distributed.rs:816-829` is the whole bug:
```rust
fn from_env() -> Self {
std::env::var("DYN_REQUEST_PLANE")
.ok()
.and_then(|s| s.parse().ok())
.unwrap_or_default()
}
```
`.ok()` discards the `VarError` (correct — absence is legal) and `.and_then(|s| s.parse().ok())`
discards the `anyhow::Error` (incorrect — a present-but-unparseable value is a configuration
error). Both funnel into the same `.unwrap_or_default()`, so "unset" and "misspelled" are
indistinguishable at the call site.
The error text that gets thrown away is at `lib/runtime/src/distributed.rs:801-814`:
`"Invalid request plane mode: '{}'. Valid options are: 'nats', 'tcp'"`. It is already good; the fix
must reuse it rather than write a second message, exactly as the issue requests.
The value's onward path, confirmed by reading: `from_env` → `DistributedConfig.request_plane`
(field at `:672`) → `config.dissolve()` at `:126-127` → `NetworkManager::new(..., request_plane)`
at `:196-201` → the `match mode { RequestPlaneMode::Tcp => … , RequestPlaneMode::Nats => … }`
branch selection at `lib/runtime/src/pipeline/network/manager.rs:170-190`. The issue's description
of the flow is accurate.
### The caller set — the API-shape question, answered empirically
`from_env` is **private** (`fn from_env()`, no `pub`, at `:819`). I enumerated every caller with
`grep -rn` across the repo excluding `target/`. There are exactly two, both inside
`lib/runtime/src/distributed.rs`:
- `DistributedConfig::from_settings()` at `:682`
- `DistributedConfig::for_cli()` at `:742`
I then enumerated the callers of *those* two, since changing their signatures is what would
actually ripple:
- `DistributedConfig::from_settings()` — three in-repo callers: `DistributedRuntime::from_settings`
at `lib/runtime/src/distributed.rs:313` (already returns `Result<Self>`, so a `?` would suffice),
and two `#[cfg(test)]` uses in `lib/runtime/src/transports/etcd.rs:1045` and `:1098`.
- `DistributedConfig::for_cli()` — **zero** callers anywhere in the repo. It is dead public API.
- `DistributedConfig::process_local()` — many callers (`lib/runtime/src/component/client.rs` ×13,
`lib/runtime/src/transports/event_plane/mod.rs`, `lib/runtime/tests/bidirectional_e2e*.rs`), but
it hardcodes `request_plane: RequestPlaneMode::Tcp` at `:772` and never calls `from_env`, so it
is untouched by any option under consideration.
- The Python bindings never call `DistributedConfig::from_settings`; `lib/bindings/python/rust/lib.rs:1054`
constructs a `DistributedConfig` struct literal. `rs::Worker::from_settings` (used at
`lib/bindings/python/rust/lib.rs:1023` and `backend.rs:519`, `:576`) is a *different* function on
`Worker`, not on `DistributedConfig`.
So the ripple of the most aggressive shape is small and fully enumerable — but it is nonzero and it
crosses a published crate's public surface. That fact drives the Chosen approach below.
### Existing test coverage — and a trap the printer must not fall into
There is exactly one `mod tests` in `distributed.rs`, at `:877`, and it is gated:
```rust
#[cfg(all(test, feature = "integration"))]
mod tests {
```
It already contains `test_request_plane_mode_from_str` at `:936-955`, which asserts `"nats"`,
`"tcp"`, `"NATS"`, `"TCP"` parse correctly and `"invalid"` errors. Verdict: **IGNORE for
duplication purposes but DO NOT extend** — that test covers `FromStr`, which is not the defective
function, and per `learnings/no-tautological-tests.md` re-asserting `FromStr` would prove nothing
about this change.
The load-bearing consequence of the gate: `dynamo-runtime`'s `[features]` block
(`lib/runtime/Cargo.toml:17-19`) has `default = []` and `integration = []`, so a plain
`cargo test -p dynamo-runtime --lib` does **not** compile that module at all. New CPU-only tests
placed inside it would silently never run in the validator's Section 4. They must go in a new,
ungated `#[cfg(test)] mod` — this is the single most likely way this work item quietly fails.
CI does run the gated module (`.github/workflows/dynamo-pipeline.yml:168` passes
`--features=block-manager,media-ffmpeg,testing-nixl,integration`), but the sandbox validator will
not, so an ungated module is required for the evidence to be real here.
Env-var test hygiene: `temp-env = { version = "0.3.6", features = ["async_closure"] }` is already a
dev-dependency at `lib/runtime/Cargo.toml:115`, and the crate already uses it — the gated tests use
`temp_env::async_with_vars` at `:886` and `:913`. The synchronous idiom used elsewhere in the repo
is `temp_env::with_vars` / `with_vars_unset` (see `lib/kvbm-config/src/lib.rs:332-461` for eight
worked examples). New tests must use it: env vars are process-global, and while CI sets
`RUST_TEST_THREADS=1` (`dynamo-pipeline.yml:168`), a local `cargo test` does not, so scoping is the
test's own responsibility, not the runner's.
### Prior art and current direction of this code
`git log --oneline -20 -- lib/runtime/src/distributed.rs` and `git log -S "from_env" --
lib/runtime/src/distributed.rs` show the request-plane subsystem's actual trajectory: `#4365`
introduced the mode flag and `from_env`, `#4845` switched the default to TCP, `#9626` removed the
HTTP request plane, and `#8398` (`fix(runtime): stop unconditional NATS connection with
--discovery-backend file`) last touched `from_env`'s neighborhood. Nothing in that history attempts
or reverts a fail-fast change here, so there is no "this was tried and backed out" hazard.
Host PR searches, all executed successfully against GitHub (no auth or egress failures — see the
caveat subsection):
- `gh pr list --repo ai-dynamo/dynamo --state all --search "12552"` → `[]`. No request references
the issue number.
- `--search "DYN_REQUEST_PLANE"` → 15 hits, all unrelated: `#4246` (NATS-less transport, MERGED),
`#4365` (CLI flag, MERGED), `#4845` (TCP default, MERGED), `#10902` (event-plane ZMQ default,
MERGED), `#9019` (velo request plane, OPEN), plus docs/recipe noise.
- `--search "RequestPlaneMode"` → 14 hits, same merged cluster plus `#9626` (HTTP removal, MERGED)
and `#12470` (unrelated watcher-teardown fix, OPEN).
- `--search "request plane"` → 15 open requests in the area: `#10921` and `#12533` (TLS on the TCP
request plane), `#11283` (Python transcode), `#12154` (inflight counter RAII), `#12447` (PushRouter
dispatch seam), `#9019` (velo). I inspected each title and head branch; none touches env-var
parsing or `from_env`.
- `--search "from_env"` → 15 hits; the only one in this crate's config area is `#11683`
(`chore(runtime): consolidate truthy/bool flag parsing into one helper`, MERGED) which consolidated
*boolean* flag parsing into `dynamo-truthy` and did not touch enum-valued variables.
- Explicit open-and-closed semantic searches via `gh search prs --repo ai-dynamo/dynamo --state
open|closed` for `"Invalid request plane mode"`, `"unwrap_or_default request plane"`, and
`"DYN_REQUEST_PLANE invalid"` → all empty except one closed hit, `#4845`, which is the
already-inspected merged TCP-default change.
- `gh issue view 12552 --json state,labels` → **OPEN**, labels `contribution-request`,
`dynamo-runtime`, `language::rust`, `runtime`. `gh api repos/ai-dynamo/dynamo/issues/12552/timeline`
returns only four `labeled` events — **no cross-referenced pull request of any kind**.
Conclusion: no merged PR implements this, no open PR competes with it, and no closed PR was
superseded away from it. `Template: debug-investigation` stands and no `Disposition:` line is
warranted.
### Discovery caveats — what I could not check
Every host call I made returned data; there were no auth failures, rate limits, or egress denials
to report. Two limits are worth stating plainly rather than glossing:
- `gh search prs` rejects `--state all` (only `open|closed`), so I ran that pathway twice per query
instead. The `gh pr list --state all --search` pathway did accept all-state and is the primary
evidence above.
- GitHub full-text search does not index arbitrary diff bodies, so a hypothetical request that
changes `from_env` without mentioning any of my six search terms in its title or body would not
appear. I consider this residual risk low given the issue timeline shows zero cross-references
and the `git log` over the file is clean, but it is not zero and I am not claiming otherwise.
- GitLab was not queried: the target is GitHub (`Repository URL: https://github.com/ai-dynamo/dynamo`).
## Symptom and ranked hypotheses
**Symptom, reproducible on CPU with no GPU and no network:** with `DYN_REQUEST_PLANE=nat` set,
`DistributedConfig::from_settings().request_plane` evaluates to `RequestPlaneMode::Tcp` and no error,
warning, or log line is emitted. Expected: a configuration failure naming `nat` and listing
`'nats', 'tcp'`.
| # | Hypothesis | Distinguishing observation | Status |
|---|---|---|---|
| H1 | `from_env` discards the `from_str` error via `.and_then(…ok())` before `.unwrap_or_default()` | Read `distributed.rs:819-824`: the `.ok()` on the parse result is literally present | **Confirmed by inspection.** This is the root cause. |
| H2 | `FromStr` itself is too permissive and accepts `nat` | `distributed.rs:804-813` matches only `"nats"`/`"tcp"` after lowercasing; the gated test at `:954` already asserts `"invalid"` errors | **Rejected.** `FromStr` is correct. |
| H3 | The error is surfaced further downstream, e.g. `NetworkManager` rejects an inconsistent mode | `manager.rs:170-190` is an exhaustive two-arm `match` over a valid enum value; by then the typo is unrecoverable | **Rejected.** No downstream check exists or could exist. |
| H4 | Something upstream (Python bindings, CLI flag) already validates and this is unreachable | `lib/bindings/python/rust/lib.rs:1013` validates only the *explicit argument* path; `backend.rs:519` calls `apply_to_env()` then `Worker::from_settings()`, which reaches the unvalidated env path | **Rejected.** The env path is reachable and is the documented operator interface (`backend.rs:534` tells operators to "Set … DYN_REQUEST_PLANE … in the environment instead"). |
Only H1 needs a fix. The distinguishing observation for the regression test is therefore precisely
the difference between "absent" and "present-but-invalid", which the current code collapses.
## Chosen approach
Split `from_env` into a fallible resolver and keep the existing public signatures intact.
**1. Make the private seam fallible.** Change `RequestPlaneMode::from_env` to return
`anyhow::Result<Self>`, distinguishing the three cases explicitly rather than by accident:
- `Err(VarError)` from `std::env::var` (absent) → `Ok(Self::default())`.
- `Ok(s)` → `s.parse()`, propagating the `from_str` error unchanged so the operator sees the exact
existing text. Treating an empty-string value as absent is acceptable and matches the adjacent
`DYN_EVENT_PLANE` handling at `:655`; the printer should pick one and cover it in a test either way.
This function is private, so this half of the change has *zero* ripple beyond the file, and it is
the seam the behavioral tests bind to.
**2. Fail fast at the two call sites using the convention already in the same function.**
`DistributedConfig::from_settings` at `:689-703` *already* fails fast on a sibling variable:
`DYN_DISCOVERY_BACKEND` does `selector.parse().unwrap_or_else(|_| panic!("Unknown
DYN_DISCOVERY_BACKEND value: '{other}'. Valid options: kubernetes, etcd, file, mem"))`. Applying the
same treatment to `request_plane`, twelve lines above it, makes one function consistent with itself
rather than half-fallible and half-panicking. `for_cli` at `:742` gets the identical treatment.
The panic message must be the propagated `from_str` error, not a second hand-written string.
**3. Add an ungated `#[cfg(test)] mod` with behavioral coverage of the three cases**, using
`temp_env::with_vars` / `with_vars_unset` to scope the process-global variable:
- absent → `from_env()` is `Ok(RequestPlaneMode::Tcp)` (the negative control: this is the case that
must *not* change, and it fails loudly if someone later makes absence an error);
- `nats`, `tcp`, and one mixed-case value → `Ok` with the matching variant;
- `nat` → `Err`, and the error string contains both the offending value and the valid options.
This is the regression test: it fails against today's code, which returns `Ok(Tcp)`;
- `DistributedConfig::from_settings()` under `DYN_REQUEST_PLANE=nat` → `#[should_panic]` with an
expected substring, proving the fail-fast reaches the actual configuration entry point rather
than stopping at the private helper.
The last one is what makes the suite non-tautological under `learnings/no-tautological-tests.md`:
revert the production change and it flips from panic to a silent `Tcp`, so the test genuinely
discriminates.
**Why this shape.** It gives the issue exactly what it asks for — the `from_str` error reaching
the operator with the invalid value and the valid options — while leaving
`DistributedConfig::from_settings() -> DistributedConfig` and `for_cli() -> DistributedConfig`
untouched. Those are `pub` items on the published `dynamo-runtime` crate; converting them to
`Result` is a semver-visible break taken on behalf of downstream consumers I cannot enumerate, in
exchange for a behavior difference the operator cannot perceive (both shapes abort startup with the
same text). The issue explicitly sanctions "the repository's existing fail-fast configuration
convention", and that convention is sitting in the same function body.
Expected size: roughly 15 lines of production change plus 40–60 lines of tests, consistent with the
issue's S estimate. `for_cli` having zero callers means step 2's second edit is cheap insurance.
**Optional, at the printer's discretion:** `distributed.rs:820` hardcodes the string literal
`"DYN_REQUEST_PLANE"` while `lib/runtime/src/config/environment_names.rs:665-669` has a
`request_plane` module that today declares only `DYN_REQUEST_PLANE_CODEC`. Adding the
`DYN_REQUEST_PLANE` constant there and referencing it is a tidy three-line improvement, but it
requires also adding the constant to the `vars` array around `environment_names.rs:927`, because
that module's test panics on any name missing from or duplicated in that list. Take it only if it
stays trivial; skip it rather than grow the diff.
## Rejected alternatives
**A. Propagate `Result` all the way out: `from_env() -> Result<Self>`, `from_settings() ->
Result<DistributedConfig>`, `for_cli() -> Result<DistributedConfig>`.** This is the shape the
issue's primary phrasing ("propagate the parse error") leans toward, and it is the better *pure*
engineering answer: a startup misconfiguration surfaces as a typed error rather than a panic
backtrace, and `DistributedRuntime::from_settings` at `:313` already returns `Result<Self>`, so it
would absorb the change with a single `?`.
I rejected it on ripple grounds, and I want to be precise about what that ripple is rather than
hand-wave it. This is exactly the required-versus-optional trade my brief flags: making the
fallibility *required* of every caller forces changes on code I have not read, including
out-of-repo consumers of a published crate. In-repo the cost is genuinely small and I enumerated it
above — `distributed.rs:313`, plus `#[cfg(test)]` uses at `transports/etcd.rs:1045` and `:1098`,
plus zero for `for_cli`. Out-of-repo it is unbounded and unknowable from here. The chosen approach
keeps the fallibility *internal*, which leaves every one of those callers alone while delivering
the identical operator-visible outcome.
If the reviewer prefers A on style grounds, the switch is cheap and the caller list above is the
complete in-repo work. Either way, `cargo check --workspace --all-targets` plus the separate
`cargo check --manifest-path lib/bindings/python/Cargo.toml --all-targets` from
`02-rust-cargo-check` Section 2 is the thing that actually proves the caller set was found — my
`grep` is a hypothesis and the compiler is the evidence. Recipe 02's own preamble documents
`ai-dynamo/dynamo#12146`, where a signature change passed a per-crate check and then failed CI on
two unbuilt workspace members (`deploy/inference-gateway/ext-proc/src/epp.rs` and
`lib/bindings/c/src/lib.rs`); `--workspace` is non-negotiable here for that reason.
**B. Warn and continue, mirroring `resolve_event_transport_kind`.** The event-plane resolver at
`:656-663` logs `tracing::warn!` on an invalid value and defaults to ZMQ. Rejected because it does
not fix the reported problem. The issue's whole complaint is that a misconfigured deployment starts
on the wrong transport; a warning in a log stream nobody reads at 3am still starts on the wrong
transport. It would also be an odd inconsistency to warn on the request plane while panicking on
`DYN_DISCOVERY_BACKEND` in the same function. Aligning `DYN_EVENT_PLANE` upward to fail-fast may
well be right, but it is a separate behavior change to a separate variable and belongs in its own
issue — see Non-goals.
**C. Fix it at `FromStr` instead.** Nothing to fix: `from_str` already rejects `nat` correctly and
the gated test at `:936-955` already proves it. Any change here would be motion without effect.
**D. Extend the existing gated `mod tests` at `:877` rather than adding an ungated module.**
Rejected because `integration` is not a default feature, so those tests do not compile under the
validator's `cargo test -p dynamo-runtime --lib`. The evidence would exist only in CI, and the
sandbox verdict would be reporting a suite that never ran — precisely the kind of unearned green
that `learnings/validator-empirical-only.md` exists to prevent. The new tests need no external
service, so there is no reason to gate them.
**E. Validate eagerly at process start in a separate config-validation pass.** Over-engineered for a
single variable, and it would create a second place where `DYN_REQUEST_PLANE` is interpreted —
which is the class of drift that produced this bug in the first place (the explicit-argument path
validating while the env path did not).
## Validation strategy
One recipe, `02-rust-cargo-check`, covering all four of its sections. This is a pure-Rust change
confined to `lib/runtime/src/distributed.rs`; no Python, no docs, no charts, no Go, no Dockerfile,
no engine code, and no GPU. `compute-env.md` confirms Recipe 02 is available in this sandbox and
that a pure-Rust change needs neither the absent Docker socket nor the single A100.
A one-recipe ladder is correctly sized here rather than thin: for Rust work, Recipe 02 *is* the lint
and unit ladder. Its Section 1 is `git diff --check` plus `cargo fmt --all --check`, Section 2 is
the workspace compile, Section 3 is `cargo clippy --workspace --all-targets --no-deps -- -D warnings`,
and Section 4 is the unit test run. `01-python-lint` would have nothing to look at — no `*.py`,
`*.md`, or CI YAML is touched. `00-dynamo-editable-install` is not required because no selected
recipe imports `dynamo.*`. `05-code-inspection` is explicitly *not* nominated: every claim this
change makes is executable in this sandbox in seconds, and my brief reserves inspection for what
genuinely cannot be run.
How the evidence proves the claim, section by section:
- **Section 2 (`cargo check --workspace --all-targets`, plus the separate
`lib/bindings/python/Cargo.toml` manifest check)** is the caller-completeness proof discussed
under Rejected alternative A. The chosen approach should produce no caller churn at all; if
Section 2 reports `error[E0308]` or `error[E0061]` anywhere, that is the compiler telling us the
private-seam assumption was wrong and the shape needs revisiting. `--all-targets` matters because
the `#[cfg(test)]` uses in `transports/etcd.rs` are only compiled under it.
- **Section 4 (`cargo test -p dynamo-runtime --lib`)** carries the behavioral proof. The validator
must confirm in the recorded output that the *new* test names actually appear in the
`test result: ok. N passed` summary — not merely that the command exited 0. Given the
`#[cfg(all(test, feature = "integration"))]` gate documented in Discovery, a new test placed in
the wrong module would produce a green exit code while never running, and the run count is the
only thing that distinguishes those two outcomes. This check is the single most important line
in the validation record.
- **Regression discrimination.** The invalid-value assertions fail against the pre-change code
(which returns `Ok(Tcp)` and does not panic) and pass after; the absent-value assertion passes
both before and after and is the negative control proving the TCP default was preserved. Together
they satisfy the debug-investigation template's requirement for a regression test plus an
unchanged control, and they are not tautological under `learnings/no-tautological-tests.md`
because reverting the production hunk flips their outcome.
- **Sections 1 and 3** guard the mechanical risks of this specific diff: `cargo fmt --all --check`
because a new `mod tests` block is easy to mis-indent, and clippy under `-D warnings` because an
unused import left behind after restructuring `from_env` would fail CI later. Per
`learnings/format-changed-only.md`, if Section 1 flags a file this change never touched, note it
and leave it alone.
No recipe here needs GPU, Docker, Kubernetes, or network egress, so no hardware disposition is
anticipated and every nominated recipe is expected to end in a green recorded run.
```validation-recipes
02-rust-cargo-check
```
## Required deliverables
- `change.md` + `change.diff` — the production fix in `lib/runtime/src/distributed.rs` (fallible
private `from_env`, fail-fast at both `DistributedConfig` constructors) plus the new ungated
`#[cfg(test)]` module with absent / valid / invalid coverage and the `#[should_panic]`
end-to-end case. Nonempty and reviewable; the debug-investigation template forbids a no-change
incident report here. Commit signed off (`git commit -s`), Conventional Commit title in the
`fix(runtime): …` form, and the PR body carrying `## Summary` and `## Validation` sections that
reference issue #12552.
- `change-validation.md` — the recorded `02-rust-cargo-check` run, all four sections, with the
test-count evidence called out explicitly per the Section 4 note above. Separate
`## Investigation outcome:` from the final `## Verdict: pass|fail|blocked`.
- `review.md` — independent assessment ending in exactly `## Assessment: sound` or
`## Assessment: needs_changes`. The reviewer should specifically confirm that the new tests ran
rather than merely compiled, that the absent-value default was not disturbed, and that no second
error message was introduced alongside the existing `from_str` text. |
Contributor
Author
change.md# Change — ai-dynamo/dynamo#12552: fail fast on invalid `DYN_REQUEST_PLANE`
- **Branch:** `fix/request-plane-invalid-value--935d8be5f4a9`, cut from `main` at `e258f04f6`.
- **Commit:** `2e8062d17ae32f6447be43703887eb8b588081d0` — `fix(runtime): fail fast on invalid DYN_REQUEST_PLANE`
- **Diff:** 1 file changed, 105 insertions(+), 7 deletions(-). Working tree clean, stash list empty.
- **Not pushed.** Publishing is the publisher stage's job.
DCO verified after committing — `git log -1 --format='%ae | %(trailers:key=Signed-off-by,valueonly)'`
returns `svc-glamr@nvidia.com | svc-glamr@nvidia.com <svc-glamr@nvidia.com>`; author email and
sign-off trailer match.
## Provenance of this run
This run resumed an interrupted one. The edit to `lib/runtime/src/distributed.rs` already existed
uncommitted in the checkout when I picked it up. I did not accept it on faith: I read the full
`git diff`, re-read the surrounding source (`RequestPlaneMode`, its `FromStr`, both call sites, and
the import block), checked the plan's constraints against it, and separately verified the one
behavioral assumption in the test helper that could have made the suite lie (see *Safety constraints
inspected*). I found it correct as written and kept it unmodified rather than churning it for the
sake of authorship. Everything in *Validation* below I executed myself in this run.
## Per-file summary
### `lib/runtime/src/distributed.rs` (only file touched)
Three hunks, all within the request-plane area.
**1. `RequestPlaneMode::from_env` (private) — signature `Self` → `Result<Self>`.** The old body was
`std::env::var("DYN_REQUEST_PLANE").ok().and_then(|s| s.parse().ok()).unwrap_or_default()`. The
first `.ok()` correctly discards `VarError` (absence is legal); the second, inside `and_then`,
incorrectly discarded the `anyhow::Error` from `FromStr`, funnelling "unset" and "misspelled" into
the same `.unwrap_or_default()`. The new body is an explicit three-arm `match` over
`std::env::var`: `Err(_)` → `Ok(Self::default())`; `Ok(s) if s.is_empty()` → `Ok(Self::default())`;
`Ok(s)` → `s.parse()`. The parse error is propagated unchanged, so the operator sees the *existing*
message `"Invalid request plane mode: '{}'. Valid options are: 'nats', 'tcp'"`. **No second error
message was written** — the plan and the issue both require reusing the existing text, and this
does. `Result` here is `anyhow::Result` (imported at line 33), which is what `FromStr::Err` already
is, so `s.parse()` needs no conversion.
**2. Both call sites fail fast.** `DistributedConfig::from_settings` and `DistributedConfig::for_cli`
now call `RequestPlaneMode::from_env().unwrap_or_else(|err| panic!("{err}"))`. This is the
convention already in use twelve lines below in `from_settings` itself, where an unparseable
`DYN_DISCOVERY_BACKEND` panics with a value-and-options message. Both functions keep their public
signatures returning `DistributedConfig`, so this is not a semver-visible break for the published
`dynamo-runtime` crate — which is precisely why the plan chose this shape over full `Result`
propagation (its Rejected alternative A).
**3. New ungated `#[cfg(test)] mod request_plane_env_tests`.** Five tests, listed under *Validation*.
The module is deliberately **not** behind the `integration` feature. This is the load-bearing detail
the plan flags as the most likely way the work item quietly fails: the pre-existing `mod tests` in
this same file is `#[cfg(all(test, feature = "integration"))]`, and `dynamo-runtime`'s `default = []`
means `cargo test -p dynamo-runtime --lib` does not compile it. Tests placed there would produce a
green exit code while never running. I proved these tests ran rather than merely compiled — see the
name-by-name output below.
The tests serialize on a module-local `static ENV_LOCK: Mutex<()>` and recover from poisoning via
`unwrap_or_else(|poisoned| poisoned.into_inner())`, because the `#[should_panic]` test poisons the
mutex on its way out and a propagated poison error would fail whichever test happened to run next.
`DYN_REQUEST_PLANE` itself is scoped with `temp_env::with_vars` / `with_vars_unset`; `temp-env` is
already a dev-dependency of this crate and is already used by the gated tests here.
## Validation — actual results
All four commands run in this checkout with the commit in place. The cargo cache was warm; each
completed in seconds.
**1. `cargo fmt --all --check` → PASS**, exit 0, no output. Re-run after the discrimination
experiment below and still clean. Nothing outside the touched file was reformatted
(`learnings/format-changed-only.md`).
**2. `cargo check --workspace --all-targets` → PASS**, exit 0, `Finished dev profile`. Only
pre-existing CUDA build-script `warning:` lines from `dynamo-llm` and `kvbm-kernels`, unrelated to
this change. No `error[E0308]` / `error[E0061]` anywhere, which is the compiler confirming the
plan's private-seam claim: making `from_env` fallible produced zero caller churn beyond the two
call sites edited in the same file. `--all-targets` matters because the `#[cfg(test)]` uses of
`from_settings` in `lib/runtime/src/transports/etcd.rs` compile only under it.
**3. Clippy — one pre-existing workspace failure, and a green scoped run.**
`cargo clippy --workspace --all-targets --no-deps -- -D warnings` → **FAILS**, exit 101:
```
error: unknown lint: `clippy::manual_option_zip`
--> lib/llm/src/discovery/model_manager.rs:1749:17
= note: `-D unknown-lints` implied by `-D warnings`
```
**This is pre-existing and unrelated, and I verified that rather than asserting it.** I stashed the
edit (`git stash push lib/runtime/src/distributed.rs`), confirmed a clean tree, and re-ran the exact
same command on unmodified `main`-at-`e258f04f6` code. It failed identically — same lint, same file,
same line, same exit 101. I then `git stash pop`-ed and confirmed the diffstat was restored to
+105/-7. The cause is a toolchain-version mismatch: the local clippy no longer knows the
`manual_option_zip` lint name that `model_manager.rs:1749` `#[allow(...)]`s, and it even suggests
`clippy::manual_option_as_slice` as the replacement. `lib/llm/src/discovery/model_manager.rs` is a
file this change does not touch and is out of scope per the plan's non-goals, so I left it alone.
Scoped to the crate actually changed, `cargo clippy -p dynamo-runtime --all-targets --no-deps --
-D warnings` → **PASS**, exit 0, `Finished dev profile in 10.34s`. No warnings in the new or edited
code — in particular no unused import left behind by restructuring `from_env`.
**4. `cargo test -p dynamo-runtime --lib` → PASS**, exit 0. Exact summary line:
```
test result: ok. 507 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 19.04s
```
A green exit with tests that never compiled is the precise failure mode the plan warns about, so I
grepped the run output for the new module. All five appear, each with an `ok` verdict:
```
test distributed::request_plane_env_tests::absent_request_plane_defaults_to_tcp ... ok
test distributed::request_plane_env_tests::empty_request_plane_defaults_to_tcp ... ok
test distributed::request_plane_env_tests::valid_request_plane_values_resolve ... ok
test distributed::request_plane_env_tests::invalid_request_plane_is_an_error_naming_value_and_options ... ok
test distributed::request_plane_env_tests::from_settings_aborts_on_invalid_request_plane - should panic ... ok
```
The 2 `ignored` are pre-existing and unrelated to this module.
## Regression discrimination — demonstrated, not claimed
The coordinator offered an honest "skip and say so" fallback here. I did not need it: the experiment
was cheap, so I ran it, and it is the evidence that these tests are not tautological under
`learnings/no-tautological-tests.md`.
I backed the file up, then temporarily replaced the new `from_env` body with the old
swallow-the-error expression (wrapped in `Ok(...)` so the `Result` signature still compiled and the
tests were exercised against the *old semantics* rather than against a compile error), and ran
`cargo test -p dynamo-runtime --lib request_plane_env_tests`:
```
test distributed::request_plane_env_tests::absent_request_plane_defaults_to_tcp ... ok
test distributed::request_plane_env_tests::empty_request_plane_defaults_to_tcp ... ok
test distributed::request_plane_env_tests::valid_request_plane_values_resolve ... ok
test distributed::request_plane_env_tests::from_settings_aborts_on_invalid_request_plane - should panic ... FAILED
test distributed::request_plane_env_tests::invalid_request_plane_is_an_error_naming_value_and_options ... FAILED
test result: FAILED. 3 passed; 2 failed; 0 ignored; 0 measured; 504 filtered out
```
Exactly the split the plan predicted. The two invalid-value tests flip to failing against the old
code, so they genuinely bind to this change. The three controls — absent, empty, and the four valid
values — pass both before and after, which is what proves the TCP default was preserved rather than
merely asserted. I then restored the file from the backup and re-ran both `cargo fmt --all --check`
and the full `cargo test -p dynamo-runtime --lib`; both green again (`507 passed; 0 failed`), with
all five names present. The committed tree is the fix, not the experiment.
## Safety constraints inspected
- **The absent-value default is untouched.** `RequestPlaneMode` keeps `#[derive(Default)]` with
`#[default]` on `Tcp`, and the `Err(_)` arm returns `Ok(Self::default())`.
`absent_request_plane_defaults_to_tcp` is the negative control and passes both before and after.
- **Empty string treated as absent.** The plan left this open provided the choice is covered by a
test. It is chosen to match the adjacent `DYN_EVENT_PLANE` handling in
`resolve_event_transport_kind`, and `empty_request_plane_defaults_to_tcp` covers it.
- **`temp_env` restores the variable even when the closure panics.** This was the one assumption
that could have made the suite lie — a leaked `DYN_REQUEST_PLANE=nat` from the `#[should_panic]`
test would contaminate whatever ran next. I read the vendored source at
`/scratch/.cargo/registry/src/index.crates.io-*/temp-env-0.3.6/src/lib.rs`: it restores via
`impl<'a> Drop for RestoreEnv<'a>` (line 110) and the rustdoc states the previous values are
"restored when the closure completes or panics, before unwinding the stack". Combined with the
poison-recovering `ENV_LOCK`, the ordering is safe.
- **No second error message.** The panic is `panic!("{err}")` over the propagated `FromStr` error,
so there is exactly one place `DYN_REQUEST_PLANE`'s valid options are spelled out.
- **Non-goals honoured.** `DYN_EVENT_PLANE`'s warn-and-default policy, `DYN_REQUEST_PLANE_CODEC`,
`DYN_DISCOVERY_BACKEND`, the Python bindings' already-fail-fast explicit-argument path, and
`NetworkManager`'s transport selection are all untouched. The optional
`environment_names.rs` constant tidy-up the plan offered "only if it stays trivial" was **skipped**
— it would have required a second file plus an entry in that module's `vars` array, and the plan
says skip rather than grow the diff.
- **Public API unchanged.** `from_settings` and `for_cli` still return `DistributedConfig`.
`cargo check --workspace --all-targets` passing with zero caller edits is the evidence.
## Fallbacks and things worth flagging to the reviewer
- I did **not** run `cargo check --manifest-path lib/bindings/python/Cargo.toml --all-targets`, which
the plan mentions under Rejected alternative A as extra caller-completeness insurance. It is not
one of the four commands assigned to this run. The risk is low — the bindings construct
`DistributedConfig` as a struct literal and never call `from_settings`, and no public signature
changed — but I am stating the gap rather than implying full coverage. The validator stage runs
recipe `02-rust-cargo-check`, which covers it.
- The full-workspace clippy failure in item 3 is real and will reproduce for anyone on this
toolchain. I deliberately did not "fix" `model_manager.rs`; scope creep into an untouched file is
exactly what `learnings/format-changed-only.md` and the plan's non-goals forbid. The scoped
`-p dynamo-runtime` run is the meaningful lint signal for this change.
- Nothing was pushed. No commit was amended or rewritten.
MR URL: https://github.com/ai-dynamo/dynamo/pull/12612 |
Contributor
Author
change-validation.md# Validation — ai-dynamo/dynamo#12552: fail fast on invalid `DYN_REQUEST_PLANE`
- **Branch/commit under test:** `fix/request-plane-invalid-value--935d8be5f4a9` at `2e8062d17`
(parent `e258f04f6`). Working tree clean and stash list empty at the start and the end of this
session; the checkout was never modified.
- **Recipe run:** `02-rust-cargo-check`, all four sections, plus two extra runs I added as controls.
- **Hardware target:** none. This is a pure-Rust, CPU-only change confined to
`lib/runtime/src/distributed.rs`; the function under test performs no I/O. The sandbox's single
A100 and the absent Docker socket are both irrelevant here, exactly as `compute-env.md` and the
plan anticipated. No recipe was blocked for want of hardware.
- **Toolchain:** `rustc 1.93.1` / `clippy 0.1.93` (2026-02-11) in this sandbox. This matters for
Section 3 — see below.
Everything below was executed through the customs recorder. I am independent of the printer: where
`change.md` asserts a result, I re-derived it myself rather than accepting it, and in one case
(Section 2's bindings manifest) I ran a command the printer explicitly did not.
## Section 1 — format and whitespace: PASS
`git diff --check` on the clean tree, `git diff --check HEAD~1 HEAD` on the committed diff (the
former checks nothing on a clean tree, so the latter is the one that actually inspects the change),
and `cargo fmt --all --check`. All exit 0 with no output. `cargo fmt --all --check` only reads, so
there is no tension with `learnings/format-changed-only.md`, and it flagged no file — touched or
untouched — so that learning's "note it and leave it alone" clause never came into play.
## Section 2 — compile check: PASS, and this is the caller-completeness proof
`cargo check --workspace --all-targets` → exit 0, `Finished dev profile in 17.06s`. No `error[E…]`
of any kind, and specifically none of the `error[E0308]` / `error[E0061]` signatures the plan named
as the tell that the private-seam assumption had failed.
This is the load-bearing half of the change's design argument, so it is worth being precise about
what it proves. The printer made `RequestPlaneMode::from_env` fallible on the strength of a `grep`
showing only two callers, both in the same file. A `grep` is a hypothesis; the compiler is the
evidence. `--workspace` is what converts one into the other, and recipe 02's preamble documents why:
in `ai-dynamo/dynamo#12146` a per-crate check passed and CI then failed on two unbuilt workspace
members. I confirmed in the recorded output that both of those specific members were compiled this
time — `Checking dynamo-ext-proc … deploy/inference-gateway/ext-proc` and `Checking libdynamo_llm …
lib/bindings/c`. The exact scope that missed #12146 was covered here.
`--all-targets` also matters on its own: the `#[cfg(test)]` uses of `DistributedConfig::from_settings`
in `lib/runtime/src/transports/etcd.rs` compile only under it, and they are the only callers outside
`distributed.rs` that touch the affected constructor.
I then ran the second command the recipe calls for and the printer flagged as a gap in its own
coverage: `cargo check --manifest-path lib/bindings/python/Cargo.toml --all-targets` → exit 0,
`Finished dev profile in 1m 40s`, ending in `Checking dynamo-py3 v1.4.0`. `lib/bindings/python` has
its own `[workspace]` and is not reached by the root `--workspace`, so without this the PyO3 crate
would have been unbuilt against the changed runtime. It builds clean. The gap the printer stated
honestly rather than glossed is now closed, and the plan's claim that the bindings never call
`from_settings` survives contact with the compiler.
Taken together: zero caller churn anywhere in the workspace or in the separate bindings workspace.
The private-seam shape the plan chose over full `Result` propagation is vindicated empirically, not
just argued.
## Section 3 — lint: workspace run FAILS on a pre-existing, unrelated defect; changed crate is clean
`cargo clippy --workspace --all-targets --no-deps -- -D warnings` → **exit 101**:
```
error: unknown lint: `clippy::manual_option_zip`
--> lib/llm/src/discovery/model_manager.rs:1749:17
|
1749 | #[allow(clippy::manual_option_zip)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: did you mean: `clippy::manual_option_as_slice`
|
= note: `-D unknown-lints` implied by `-D warnings`
```
I am reporting this plainly rather than burying it. It is a real failure of a section the recipe
requires, and it is recorded in `validation/registry.jsonl` with its exit code and full log — not
suppressed. The question that decides how it should weigh on the verdict is whether **this change**
caused it, and the answer is demonstrably no. I established that independently rather than taking
the printer's word:
1. **The file is untouched.** `git diff --stat HEAD~1 HEAD -- lib/llm/src/discovery/model_manager.rs`
is empty. The change is one file, `lib/runtime/src/distributed.rs`. `dynamo-llm` does not depend
on anything this change altered in a way clippy's lint-name resolution could observe — the failure
is an *unknown lint name in an `#[allow]` attribute*, which is resolved from clippy's own lint
table and cannot be influenced by another crate's source.
2. **It reproduces on pristine parent-commit source.** Rather than stash inside the checkout (which
would have meant modifying the printer's branch — forbidden to me), I extracted the parent commit
to a scratch tree with `git archive HEAD~1 | tar -x -C /tmp/…`, confirmed that tree contains the
*original* error-swallowing `from_env` and none of the new tests, and verified that
`model_manager.rs` is byte-identical between the two trees. Recorded run of
`cargo clippy -p dynamo-llm --lib --no-deps -- -D warnings` against that pristine tree → **exit
101, same lint, same file, same line 1749**. The change is not present anywhere in that tree.
3. **The mechanism is a toolchain skew, not a code defect.** The `#[allow(clippy::manual_option_zip)]`
line was introduced by `18b6e7eed` (2026-07-10, `feat(llm): route requests through encode workers
(#11460)`). CI builds on Rust **1.96.1** (`container/templates/dynamo_base.Dockerfile:60`,
`RUST_VERSION=1.96.1`); this sandbox has **1.93.1**. The older clippy does not know that lint name
and, under `-D warnings`, `-D unknown-lints` promotes not knowing it into a hard error. I want to
be careful about what I am and am not claiming here: I have verified the version pin, the local
version, and the reproduction on unmodified source. I have **not** been able to run 1.96.1 to
confirm it accepts the name, so "CI will be green on this lint" is my inference from the pin, not
something I measured.
Scoped to the crate this change actually touches:
`cargo clippy -p dynamo-runtime --all-targets --no-deps -- -D warnings` → **exit 0**,
`Finished dev profile in 10.34s`, zero warnings. In particular no unused import survived the
restructuring of `from_env`, which was the specific mechanical risk the plan asked Section 3 to
guard.
**Why I recorded no disposition for this.** My brief offered `evidence note` if it were the honest
representation, and I concluded it is not. Every available kind would assert something false:
`failing` attributes the fault to this change (it does not belong to it, and my brief forbids
reporting a pre-existing failure as if this change caused it); `missing-dep` / `sandbox-denied` /
`needs-hardware` / `timed-out` would each claim the recipe could not be run, when it ran fine and the
sections that bear on this change all passed. The failing run stands in the registry as itself,
which is the accurate record, and recipe 02's own guidance for the analogous Section 1 case — "if it
flags a file you never touched, note that in your narrative and leave it — do not widen your diff to
make it happy" — is the documented precedent for handling it in prose rather than by disposition or
by scope creep into `lib/llm`. A reviewer who disagrees with that reading has every fact needed to
overrule me: the failing run, its log, and the pristine-tree control are all recorded.
## Section 4 — unit tests: PASS, and the tests provably ran
`cargo test -p dynamo-runtime --lib` → exit 0:
```
test result: ok. 507 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 19.09s
```
The exit code alone would be worthless here, and this is the single most important check in the
record. The other test module in this file is `#[cfg(all(test, feature = "integration"))]`, and
`dynamo-runtime` has `default = []`, so a new test misplaced into it would never compile under this
command and would still produce a green exit. I therefore grepped the recorded log for the five new
names. All five appear, each with an `ok` verdict:
```
test distributed::request_plane_env_tests::absent_request_plane_defaults_to_tcp ... ok
test distributed::request_plane_env_tests::empty_request_plane_defaults_to_tcp ... ok
test distributed::request_plane_env_tests::valid_request_plane_values_resolve ... ok
test distributed::request_plane_env_tests::invalid_request_plane_is_an_error_naming_value_and_options ... ok
test distributed::request_plane_env_tests::from_settings_aborts_on_invalid_request_plane - should panic ... ok
```
As the complementary half of the same check: `grep -c test_request_plane_mode_from_str` on the same
log returns **0**. That is the gated module's existing test, and its absence confirms the gate is
genuinely closed under this command — which is precisely what makes the presence of the five new
names meaningful rather than incidental. The trap the plan identified as "the single most likely way
this work item quietly fails" was real, and the new ungated module escaped it.
The 2 `ignored` are pre-existing and belong to other modules.
## An integrity problem I hit, and how I resolved it
I am recording this because it briefly produced a false red and could just as easily have produced a
false green for someone else.
After building my pristine-parent control tree in `/tmp`, I grafted the printer's new test module
onto that tree's *old* `from_env` semantics to run the discrimination experiment (below). Both trees
share the sandbox's job-scoped `CARGO_TARGET_DIR`, and cargo's fingerprint for
`dynamo_runtime-8418295324029aa1` collided across them. My next `cargo test` against the real
checkout reported `Finished in 0.32s` — no recompile — and re-executed the **control's** binary,
reporting 2 failures whose panic line numbers (`:1020`, `:1036`) matched the control tree, not the
checkout. Exit 101 on a change that is in fact correct.
I caught it because the timing and the line numbers were wrong for the tree I thought I was testing.
Resolution: I deleted the `/tmp` control tree, forced a rebuild by touching
`lib/runtime/src/distributed.rs` (an mtime-only touch — `git status --porcelain` stayed empty and
`git diff HEAD` remained zero-length, so the file's content was never altered), and re-ran. That run
shows `Compiling dynamo-runtime v1.4.0 (/home/sandbox/…/repo/lib/runtime)` and takes 50.31s — a
genuine build from the checkout — and all five tests pass. The deciding full-suite run then follows
on that binary.
I did not settle for timing as proof. I verified the executed binary's provenance directly:
`strings` on it yields **0** occurrences of the `/tmp` control path, **1** occurrence of the
work-item checkout path, and it contains both `Invalid request plane mode` and the new tests'
`must not silently fall back to TCP` assertion message. The binary that produced the green result
was built from the change under test. The vacuous run remains in the registry as a failing entry
rather than being erased; customs' "most recent run decides" rule means the deciding row is the
genuine one, and I would rather leave the misstep visible than tidy it away.
## Regression discrimination — re-derived independently, both polarities
The debug-investigation template asks for the original reproduction (or the regression test standing
in for it), the fixed behaviour, and a negative control. I have all three as paired command logs,
derived independently of the printer's own experiment.
**Negative polarity (the reproduction).** On the pristine parent-commit tree, I restored the old
defective semantics — `std::env::var(...).ok().and_then(|s| s.parse().ok()).unwrap_or_default()`,
wrapped in `Ok(...)` so it satisfies the new signature and the tests run against *old behaviour*
rather than dying at a compile error — and grafted the printer's test module in verbatim. Result:
```
test result: FAILED. 3 passed; 2 failed; 0 ignored; 0 measured; 504 filtered out
```
with the failure text being the bug itself, in the operator's own terms:
```
a misspelled DYN_REQUEST_PLANE must not silently fall back to TCP: Tcp
note: test did not panic as expected
```
That is the reported symptom reproduced empirically: `DYN_REQUEST_PLANE=nat` resolving to `Tcp` in
silence, and `from_settings()` returning normally instead of aborting.
**Positive polarity (the fix).** The same five tests against the committed change: `5 passed;
0 failed`, and within the full suite `507 passed; 0 failed`.
**The split is exactly the right one.** The two tests that flip are precisely the two invalid-value
tests — the regression tests. The three that pass in *both* directions are the controls:
`absent_request_plane_defaults_to_tcp`, `empty_request_plane_defaults_to_tcp`, and
`valid_request_plane_values_resolve`. That is the evidence for the plan's most important non-goal:
the unset default was genuinely preserved rather than merely asserted to be. A change that had
broken the TCP default would have turned those three red, and they are green on both sides.
This also settles the tautology question under `learnings/no-tautological-tests.md` empirically
rather than by argument: tests that fail against the old code and pass against the new one are bound
to the behaviour change, not to the implementation's shape.
## What I did not verify
- **CI's clippy on Rust 1.96.1.** I verified the version pin and reproduced the failure locally on
unmodified source; I could not execute 1.96.1 to confirm it knows `manual_option_zip`. If it does
not, `lib/llm/src/discovery/model_manager.rs:1749` is a pre-existing CI problem that predates this
branch by roughly a month and is not this change's to fix.
- **Runtime behaviour of the panic in a live process.** The tests exercise `from_env` and
`DistributedConfig::from_settings` directly, which is where the defect lives; nothing downstream of
`NetworkManager` was started. The plan scoped this deliberately and the affected function does no
I/O, so there is nothing a live process would reveal that the unit tests do not.
## Investigation outcome:
The reported defect is real, I reproduced it, and the change fixes it. `DYN_REQUEST_PLANE=nat` no
longer resolves silently to TCP: it now surfaces the existing `FromStr` error naming both the
offending value and the valid options, and it does so at the real configuration entry point
(`DistributedConfig::from_settings`), not merely at the private helper. The three-way contract the
issue asked for holds — absent and empty keep the TCP default, valid values resolve, invalid values
abort — with each arm covered by a test I watched run in the real crate binary.
Three of the recipe's four sections are green without qualification. Section 3's workspace clippy
run fails, and I want the verdict to rest on an honest reading of that rather than on a convenient
one. It fails on `lib/llm/src/discovery/model_manager.rs:1749`, a file this change does not touch, in
a crate it does not modify; it reproduces identically on pristine parent-commit source where the
change does not exist; the offending line predates this branch by about a month; and the mechanism —
an `#[allow]` naming a lint that this sandbox's older clippy does not know — is one no edit to
`lib/runtime` could cause. The lint signal that does bear on this change, `cargo clippy -p
dynamo-runtime --all-targets --no-deps -- -D warnings`, is green with zero warnings. I would report
a `fail` if the failure were attributable to the change; it is not, and attributing it would be the
dishonest reading, not the cautious one.
The evidence that carries the most weight is the pairing: the five new tests fail 2-of-5 against the
old code with the bug's own symptom in the failure text, and pass 5-of-5 against the new code, with
the three controls green on both sides. Combined with `cargo check` over the full workspace *and* the
separate bindings workspace showing zero caller churn, and with the confirmation that the new tests
actually executed rather than sitting behind the `integration` gate, every claim this change makes is
empirically demonstrated in the real binary.
## Verdict: pass |
Contributor
Author
review.md# Review — ai-dynamo/dynamo#12552: fail fast on invalid `DYN_REQUEST_PLANE`
> 🤖 **Automated AI review — advisory.** An AI agent's judgment of
> whether this change is logically sound on the evidence; not a merge
> authorization. CI, customs, and a human reviewer hold that.
## Assessment: sound
The change does what the issue asked, at the seam where the defect actually lived, with tests that
provably ran and provably discriminate. Both nits below are advisory; neither undermines the
change's correctness, and neither requires another printer iteration.
### Repo state at inspection
Branch `fix/request-plane-invalid-value--935d8be5f4a9` (not the upstream default), HEAD
`2e8062d17ae32f6447be43703887eb8b588081d0` — matching the SHA recorded in `change.md` — with
`git status --porcelain` empty and no stash entries. One file changed, +105/-7.
### What I confirmed against source
The four confirmations the plan asked for, each grounded in the checkout rather than in the
packets' prose:
- **The new tests genuinely ran.** `lib/runtime/src/distributed.rs:848` is a bare `#[cfg(test)]`;
the file's other module at `lib/runtime/src/distributed.rs:975` is
`#[cfg(all(test, feature = "integration"))]` and `dynamo-runtime` has `default = []`. The
deciding log (`validation/logs/2026-08-03T23-05-19.532Z-cargo-871d.log`) carries all five new
names with `... ok` at lines 64–68, `507 passed; 0 failed`, and — the complementary half —
**zero** occurrences of the gated module's `test_request_plane_mode_from_str`. The gate is
closed under this command, which is what makes the five names meaningful rather than incidental.
The trap the plan flagged as the likeliest silent failure was real and was avoided.
- **The absent-value TCP default was not disturbed.** `lib/runtime/src/distributed.rs:786-793`
keeps `#[derive(..., Default)]` with `#[default]` on `Tcp`, untouched by the diff, and
`lib/runtime/src/distributed.rs:832` returns `Ok(Self::default())` on `Err(_)`. The negative
control passes on **both** polarities (3-of-5 green against old semantics, 5-of-5 against new),
so preservation is demonstrated, not asserted. Empty-string is likewise unchanged: the old
`.parse().ok()` chain also yielded `Tcp` for `""`.
- **No second error message.** The string `"Invalid request plane mode: '{}'. Valid options are:
'nats', 'tcp'"` exists at exactly one place, `lib/runtime/src/distributed.rs:812`, inside
`from_str`. Both call sites (`lib/runtime/src/distributed.rs:685` and `:745`) use
`panic!("{err}")` over the propagated error, and the end-to-end test at
`lib/runtime/src/distributed.rs:921` binds `#[should_panic(expected = "Invalid request plane
mode: 'nat'")]` to that same existing text. A grep across the workspace finds no competing
message.
- **Non-goals respected.** One file, one subsystem. `DYN_EVENT_PLANE`'s warn-and-default policy at
`lib/runtime/src/distributed.rs:648-665`, `DYN_REQUEST_PLANE_CODEC`, `NetworkManager`, the
Python binding path, and the default mode are all untouched. The optional `environment_names.rs`
tidy-up the plan offered conditionally was correctly skipped rather than grown into a second file.
**Not tautological.** Verified empirically rather than by argument: logs
`2026-08-03T23-00-49.623Z-cargo-9c29.log` and `...T23-03-27.552Z-cargo-5121.log` show the two
invalid-value tests FAILED against old semantics with the bug's own symptom in the failure text
(`a misspelled DYN_REQUEST_PLANE must not silently fall back to TCP: Tcp`), and green after. The
tests bind to the behavior change, not to the implementation's shape.
**Caller completeness.** `cargo check --workspace --all-targets` (exit 0) covered the two members
that escaped `ai-dynamo/dynamo#12146` — I confirmed `Checking dynamo-ext-proc … ext-proc` and
`Checking libdynamo_llm … lib/bindings/c` in the log — and the separate
`lib/bindings/python/Cargo.toml` check reached `Checking dynamo-py3` at exit 0. Zero caller churn,
so the plan's private-seam claim survives the compiler, not just the grep.
### Evidence-table audit
The plan's `validation-recipes` block names exactly one recipe, `02-rust-cargo-check`. The customs
table shows it `validated` and reports `cleared [1/1 validated]`. **No named recipe is `missing`.**
The cited log is the deciding `cargo test -p dynamo-runtime --lib` run, and I confirmed by reading
it that the command executed against the change and passed. No `validated` row is contradicted by
its own evidence, so no absent `failing` disposition is owed.
## Findings
| # | Location | Severity | Claim | Evidence |
|---|---|---|---|---|
| 1 | `docs/fern/pages/developer-guide/knowledge-base/concepts/communication-planes/request-plane.md:59` | nit | The canonical user-facing page for this variable still states the behavior this change deliberately inverts: "If `DYN_REQUEST_PLANE` is not set **or contains an invalid value**, Dynamo defaults to `tcp`." After this change an invalid value aborts startup. An operator reading the docs is told the misconfiguration is safe — the exact belief the issue set out to correct. | Read at `file:line`; contradicted by `lib/runtime/src/distributed.rs:834` (`Ok(s) => s.parse()`) and `:685`/`:745` (`panic!`), and by the passing `from_settings_aborts_on_invalid_request_plane`. Note the two nearby staleness hits — `distributed-runtime.md:57` and `architecture-flow.md:43` still advertise an HTTP request plane removed in #9626 — are pre-existing and **not** this change's doing; only the `:59` line is newly falsified by it. |
| 2 | `change-validation.md:253` | nit | The declared `## Verdict: pass` diverges from recipe 02's literal verdict guidance ("Any section fails → `## Verdict: fail`"), since the Section 3 workspace clippy run exits 101. I concur with the validator on substance (reasoning below), but a reader should not take the `[1/1 validated]` tally as meaning all four sections were green — customs keys one row per recipe on the most recent run, so a Section 3 failure followed by a Section 4 run is structurally invisible in the derived table. | `validation/registry.jsonl` records `cargo clippy --workspace --all-targets --no-deps -- -D warnings` at exit 101 (22:48) and the deciding `cargo test` at exit 0 (23:05); the customs report renders one `validated` row. |
## Judgment on the two questions the plan flagged
**The pre-existing clippy failure — is the claim supported, and was leaving it undispositioned
honest?** The claim is supported, on four independent grounds I checked myself:
`git diff --stat HEAD~1 HEAD -- lib/llm/src/discovery/model_manager.rs` is empty; the recorded
control run (`...T22-57-11.264Z-cargo-6c83.log`) reproduces the identical lint, file, and line
1749 on a tree whose own log lines read `/tmp/clippy-baseline-e258f04f6/…`, i.e. genuinely not the
checkout; the `#[allow(clippy::manual_option_zip)]` at `lib/llm/src/discovery/model_manager.rs:1749`
was introduced by `18b6e7eed` on 2026-07-10, roughly a month before this branch; and the mechanism
— an *unknown lint name in an `#[allow]` attribute*, resolved from clippy's own lint table — is one
no edit under `lib/runtime` could cause. I independently confirmed the skew: local
`clippy 0.1.93 (2026-02-11)` against `RUST_VERSION=1.96.1` at
`container/templates/dynamo_base.Dockerfile:60`. The validator was appropriately careful to label
"CI will be green on this lint" as inference from the pin rather than measurement.
Leaving it undispositioned was the honest call, not a coverage gap. Against the actual controlled
vocabulary, every kind would have asserted something false: `failing` attributes the fault to this
change, and `needs-hardware` / `missing-dep` / `timed-out` / `sandbox-denied` each claim the recipe
could not be run, when it ran fine. There is no `pre-existing` kind. It is also worth stating that
the choice changed nothing at the gate — customs decides per recipe on the most recent run, and a
later green run already superseded the clippy row — so this was not a validator dodging a
downgrade. The failing run stands in the registry with its exit code and full log, which is the
accurate record and leaves a human every fact needed to overrule the reading.
**The fingerprint collision — handled correctly, and is the final evidence sound?** Yes to both,
and the evidence is stronger than the validator's own account claims. The collision window opened
at 22:57 when the `/tmp` control tree first appears in a log; the clean full-suite run at
**22:53** (`...T22-53-06.777Z-cargo-3b52.log`) entirely **predates** it, shows
`Compiling dynamo-runtime … /home/sandbox/…/repo/lib/runtime`, and already carries all five new
names green at `507 passed`. So the central claim needs no fingerprint reasoning at all. The
vacuous run at 23:03 was a false **red**, not a false green — it cost the change nothing and was
caught on the right signals (0.32s with no recompile, and panic line numbers belonging to the
control tree). Leaving it visible in the registry was correct; erasing it would have been
tampering, and under "most recent run decides" the deciding row is the genuine 23:04/23:05 pair.
I verified the deciding binary's provenance myself rather than accepting the timing argument: the
on-disk `dynamo_runtime-8418295324029aa1` contains `/home/sandbox/workspace/wi-20260803T212805Z-12552/repo`,
**zero** `/tmp/clippy…` paths, and the new tests' `must not silently fall back to TCP` assertion
string. One imprecision worth noting for the record, immaterial to the conclusion: the validator
reported "1 occurrence of the work-item checkout path", but the string embedded is the repo root,
not the `lib/runtime` subdirectory. The substance — the binary was built from the change under
test — holds. The validator's own warning generalizes correctly and deserves to be carried
forward: the same mechanism could as easily have produced a false green for someone who was not
watching the build timings.
### Checklist discharge
- `commit-on-real-branch` — branch `fix/request-plane-invalid-value--935d8be5f4a9` (non-default),
SHA `2e8062d17ae32f6447be43703887eb8b588081d0` present in `git log` and matching `change.md`,
working tree clean, stash empty.
- `validator-blocked-or-failed-implies-request-changes` — `change-validation.md:253` reads
`## Verdict: pass`, unqualified. It is not a hedge of the MR-#9 kind: the toolchain ran, the
crate built, the tests executed, and both polarities of the regression were recorded. The one
conceded deficit (workspace clippy) is demonstrably not attributable to the change. Judged on
substance, this is an unqualified pass.
- `evidence-rows-must-show-execution` — one `validated` row, `02-rust-cargo-check`, citing
`validation/logs/2026-08-03T23-05-19.532Z-cargo-871d.log`. Read directly: the command executed,
compiled from the work-item checkout, exercised the change (five new test names present, gated
module absent), and passed at `507 passed; 0 failed`. No contradiction; no owed `failing`
disposition.
- `new-infrastructure-exercise-cited` — the change adds test infrastructure (the ungated
`mod request_plane_env_tests` and its `with_request_plane` / `ENV_LOCK` helper at
`lib/runtime/src/distributed.rs:848-926`). It was exercised, not merely registered: five named
tests appear with `ok` verdicts in the deciding log, and the same helper is what produced the
2-of-5 failure split against old semantics in the discrimination runs. I separately confirmed the
helper's safety premise in the vendored `temp-env-0.3.6` source — `with_vars` / `with_vars_unset`
serialize on a crate-internal `static SERIAL_TEST: ReentrantMutex<()>` and restore via `Drop`, so
the `#[should_panic]` test cannot leak `DYN_REQUEST_PLANE=nat` into whatever runs next in the
shared `--lib` binary. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 6 remain after this review. Walkthrough
ChangesRequest plane validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Invalid non-empty request-plane values now fail configuration instead of silently selecting TCP, while valid, absent, and empty values retain their established behavior. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 1 files. (1 skipped: 1 unsupported.)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/runtime/src/distributed.rs`:
- Around line 823-835: Update RequestPlaneMode::from_env to match
std::env::VarError::NotPresent explicitly for the TCP default, while propagating
VarError::NotUnicode through the existing Result error path. Add a Unix-only
regression test that sets DYN_REQUEST_PLANE to a non-Unicode value and verifies
the error is returned rather than defaulting to TCP.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3086c70c-c637-40d9-9ac6-1c995956ab87
📒 Files selected for processing (1)
lib/runtime/src/distributed.rs
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
Contributor
Author
Automated CI record — result: passedCI reached a terminal state on this pull request: passed. Head: 23 checks passed, 2 were skipped, and none failed.
Note: Merge authority rests with a human reviewer; this record states CI status only. |
`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 <svc-glamr@nvidia.com>
glamr-agent
temporarily deployed
to
external_collaborator
August 4, 2026 00:26 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
github-actions
Bot
added
the
documentation
Contributor
Author
|
Automated repair round 1 — pushed CI on the previous head was already terminal-success (23 pass, 4 skipping, 0 fail); this round addresses the two unresolved review discussions, one of which was a real correctness gap. Addressed
Checks actually run (all on the pushed tree)
Regression-discrimination control. The new test is not merely green — I temporarily restored the old Validation status: complete for this round. CI on the new head is still pending at the time of writing. |
MatejKosec
temporarily deployed
to
external_collaborator
August 12, 2026 02:28 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
/ok to test 2de0548 |
|
🎯 Code Coverage (details) 🔗 Commit SHA: 3e0df08 | Docs | Datadog PR Page | Give us feedback! |
Contributor
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR metadata ==='
gh pr view 12612 --repo ai-dynamo/dynamo --json number,title,headRefOid,baseRefOid,files,mergeStateStatus \
--jq '{number,title,headRefOid,baseRefOid,mergeStateStatus,files:[.files[].path]}'
echo '=== Changed-file diff summary ==='
git diff --stat HEAD^ HEAD
git diff --check HEAD^ HEAD
echo '=== Changed paths ==='
git diff --name-only HEAD^ HEAD
echo '=== distributed.rs outline ==='
ast-grep outline lib/runtime/src/distributed.rs --items all --match 'RequestPlaneMode|DistributedConfig|request_plane_env_tests' --view expanded || true
echo '=== Relevant distributed.rs changes and context ==='
git diff -U80 HEAD^ HEAD -- lib/runtime/src/distributed.rs
echo '=== Relevant documentation changes and context ==='
git diff -U50 HEAD^ HEAD -- docs/fern/pages/developer-guide/knowledge-base/concepts/communication-planes/request-plane.md
echo '=== References to DYN_REQUEST_PLANE with context ==='
rg -n -C 3 'DYN_REQUEST_PLANE|RequestPlaneMode::from_env' \
lib/runtime/src/distributed.rs \
docs/fern/pages/developer-guide/knowledge-base/concepts/communication-planes/request-plane.mdLength of output: 14543 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='d0f4808049916bcf5f349fa196296c98a80ef10a'
head='181e99ec86fe62339804ff5e98a1596c795e2c53'
echo '=== Complete PR diff summary ==='
git diff --stat "$base" "$head"
git diff --check "$base" "$head"
git diff --name-status "$base" "$head"
echo '=== Complete Rust change ==='
git diff -U100 "$base" "$head" -- lib/runtime/src/distributed.rs
echo '=== Complete documentation change ==='
git diff -U80 "$base" "$head" -- docs/fern/pages/developer-guide/knowledge-base/concepts/system-architecture/architecture.md
echo '=== Final request-plane implementation and tests ==='
sed -n '680,955p' lib/runtime/src/distributed.rs
echo '=== Final documentation references ==='
rg -n -i -C 4 'DYN_REQUEST_PLANE|request plane|invalid value|defaults to `tcp`' \
docs/fern/pages/developer-guide/knowledge-base/concepts/system-architecture/architecture.md \
docs/fern/pages/developer-guide/knowledge-base/concepts
echo '=== PR checks and unresolved review threads ==='
gh pr view 12612 --repo ai-dynamo/dynamo --json statusCheckRollup,reviews,reviewThreads \
--jq '{checks:[.statusCheckRollup[]|{name,conclusion,status}],reviews:[.reviews[]|{author:.author.login,state,commit:.commit.oid}],threads:[.reviewThreads[]|select(.isResolved|not)|{path,line,isResolved,comments:[.comments[].body]}]}'Length of output: 49551 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Request-plane parsing and configuration call sites ==='
rg -n -C 4 -P '\b(from_env|from_env_result|from_settings|for_cli)\s*\(|\brequest_plane\s*\.\s*parse\s*\(|DYN_REQUEST_PLANE' \
lib/runtime lib/bindings
echo '=== Explicit request-plane validation paths ==='
rg -n -C 5 -P '\brequest_plane\b.*\bparse\s*\(|\.parse::<[^>]*RequestPlaneMode|RequestPlaneMode' \
lib/bindings lib/runtime
echo '=== Final implementation line numbers ==='
nl -ba lib/runtime/src/distributed.rs | sed -n '810,930p'
echo '=== Pull request status checks ==='
gh api repos/ai-dynamo/dynamo/pulls/12612 \
--jq '{mergeable,mergeable_state,head:.head.sha,base:.base.sha}'
gh api repos/ai-dynamo/dynamo/commits/181e99ec86fe62339804ff5e98a1596c795e2c53/check-runs \
-H 'Accept: application/vnd.github+json' \
--paginate \
--jq '.check_runs[] | {name,status,conclusion,details_url}'
echo '=== Unresolved review threads ==='
pr_id=$(gh api repos/ai-dynamo/dynamo/pulls/12612 --jq '.node_id')
gh api graphql -F prId="$pr_id" -f query='
query($prId: ID!) {
node(id: $prId) {
... on PullRequest {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
comments(first: 10) {
nodes { author { login } body }
}
}
}
}
}
}' --jq '.data.node.reviewThreads.nodes[] | select(.isResolved == false) | {path,line,comments:[.comments.nodes[]|{author:.author.login,body}]}'Length of output: 87019 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Pull request state ==='
gh api repos/ai-dynamo/dynamo/pulls/12612 \
--jq '{mergeable,mergeable_state,head:.head.sha,base:.base.sha,state,draft}'
echo '=== Check-run outcomes for requested head ==='
gh api repos/ai-dynamo/dynamo/commits/181e99ec86fe62339804ff5e98a1596c795e2c53/check-runs \
-H 'Accept: application/vnd.github+json' \
--paginate \
--jq '.check_runs[] | [.name, .status, .conclusion] | `@tsv`'
echo '=== Unresolved review threads ==='
pr_id=$(gh api repos/ai-dynamo/dynamo/pulls/12612 --jq '.node_id')
gh api graphql -F prId="$pr_id" -f query='
query($prId: ID!) {
node(id: $prId) {
... on PullRequest {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
comments(first: 10) {
nodes { author { login } body }
}
}
}
}
}
}' --jq '
.data.node.reviewThreads.nodes[]
| select(.isResolved == false)
| {path, line, comments: [.comments.nodes[] | {author: .author.login, body}]}
'Length of output: 3019
I found no new correctness findings. The change now has the required behavior:
The documentation matches the environment path and the explicit Python binding path. I also used the existing non-Unicode handling guidance during this review. The current head is mergeable. Several checks have passed. The remaining Rust, Fern, documentation, and pre-commit checks are still in progress. One review thread remains open for a helper-comment removal. The current head already removes that comment. Resolve that thread when the reviewer confirms the update. 🧠 Learnings used✅ Action performedFull review finished. |
dagil-nvidia
approved these changes
Sep 2, 2026
dagil-nvidia
left a comment
dagil-nvidia
left a comment
Collaborator
There was a problem hiding this comment.
Re-approving the latest head. Since the prior approval the change moved its tests off process-wide env mutation onto a from_env_result seam that takes a synthesized VarError, which removes the cross-test race. Coverage reads well: absent, empty, valid including mixed case, misspelled, and non-Unicode. The panic! at both from_settings call sites is the right call given the non-Result signature, and it matches the documented behavior of stopping startup instead of silently selecting tcp.
Signed-off-by: Matej Kosec <mkosec@nvidia.com>
MatejKosec
temporarily deployed
to
external_collaborator
September 8, 2026 21:42 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
/ok to test bf20cee |
Signed-off-by: Matej Kosec <mkosec@nvidia.com>
MatejKosec
temporarily deployed
to
external_collaborator
September 8, 2026 22:31 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
/ok to test f8c5ffd |
Signed-off-by: Matej Kosec <mkosec@nvidia.com>
MatejKosec
temporarily deployed
to
external_collaborator
September 8, 2026 22:50 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
/ok to test 8ab3ca6 |
Signed-off-by: Matej Kosec <mkosec@nvidia.com>
Contributor
|
/ok to test af264e5 |
Signed-off-by: GLAMR <svc-glamr@nvidia.com>
Contributor
Author
|
nursery: @dynamo-ops please run full CI for 49974f4 |
Contributor
|
/ok to test 49974f4 |
rmccorm4
approved these changes
Sep 17, 2026
rmccorm4
enabled auto-merge (squash)
September 17, 2026 19:39
Contributor
Author
|
nursery: Maintenance handoff for Full CI run https://github.com/ai-dynamo/dynamo/actions/runs/35263893927 completed successfully. Final checks: 71 passed, 45 skipped, zero failed or pending; all five request-plane regression tests ran and passed remotely. All nine review threads are resolved, and GitHub records the PR merged by |
aung-san-i
added a commit
to aung-san-i/dynamo
that referenced
this pull request
Sep 28, 2026
* feat: KV DC Relay file based source mode (ai-dynamo#14807) Add live-reloaded file sources for KV DC Relay namespace selection and expose readiness and source revisions through /engine/state. Preserve applied membership on invalid updates, coalesce discovery refreshes, and isolate native integration tests in forked processes. Signed-off-by: Nikita Sukharev <kaonael@gmail.com> * feat(sglang): expose cross-encoder reranking through /v1/rerank (ai-dynamo#14032) Signed-off-by: xianlubird <xianlubird@gmail.com> * fix(profiler): explain inaccessible model paths during trust checks (ai-dynamo#14860) Signed-off-by: hongkuanz <hongkuanz@nvidia.com> * fix(sglang): sync discovery from native pause state (ai-dynamo#13951) Signed-off-by: William Arnold <7565007+Aphoh@users.noreply.github.com> Co-authored-by: Zero Rains <57100978+zeroRains@users.noreply.github.com> * feat(recipes): add Solar Open2 250B NVFP4 aggregated and disaggregated recipes for B200 (ai-dynamo#14376) Signed-off-by: Sandhya Rani Narravula <snarravula@nvidia.com> * refactor(agents): session_id reader from AgentContext + forward to vLLM (ai-dynamo#14428) Signed-off-by: Karen Chung <karenc@nvidia.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(discovery): allow served aliases for the same model source (ai-dynamo#14857) Signed-off-by: jthomson04 <jwillthomson19@gmail.com> * fix(router): reject unknown explicit worker targets (ai-dynamo#14858) Signed-off-by: jthomson04 <jwillthomson19@gmail.com> * fix(xpu): stabilize XPU test workers (ai-dynamo#14539) Signed-off-by: Wenxin Zhang <wenxin.zhang@intel.com> Signed-off-by: VincyZhang <wenxin.zhang@intel.com> * feat(mm-routing): add Nemotron 3 Nano Omni video routing (ai-dynamo#14653) Signed-off-by: krishung5 <krish@nvidia.com> * fix(sglang): validate diffusion input_reference and bound media fetches (ai-dynamo#14435) The sglang image-diffusion and video-generation handlers passed the client-supplied input_reference through to the generator's image_path after only a non-empty check. Validate it first, and for remote references materialize it locally before the generator sees it, so the generator is always handed a trusted local path. This brings the sglang diffusion path in line with the vLLM/omni and trtllm backends, which already validate the same field. Behavior change: local I2I/I2V references now require DYN_MM_LOCAL_PATH to be set to the allowed directory; previously any path was accepted. common/http: - validate_media_reference() returns a plain filesystem path for local references; local_media_reference() is an async context manager that fetches a remote one through fetch_bytes(policy=...), which revalidates every redirect hop, into a temp file removed on exit. data: is rejected -- a URI is not a path. - fetch_bytes() gained max_bytes, streaming through collect_capped at an explicit read granularity so the cap is an allocation bound and not only a rejection: a 128 MiB-decoded gzip body against the 64 MiB cap peaks at 68,032,217 bytes rather than the whole decompressed body. Content-Length is caller-controlled and absent when chunked, and aiohttp's read(n) returns at most n bytes, so neither a header check nor a single capped read suffices. Defaults to None, leaving existing callers unchanged. - DYN_MM_MAX_FILE_SIZE_MB makes that cap operator-tunable, in megabytes, as the SGLang arg it replaces was. Read per call; empty, unparseable or non-positive falls back to 64 with a warning, so a malformed value neither takes the worker down nor reads as unlimited. - Messages built from caller input are bounded via describe_media_source, moved from multimodal/media_source.py (it pulls in torch) into url_validator.py and re-exported from its old home; a no-op below 120 characters. - HttpStatusError bounds its .message attribute, not only the rendered string: errors.rs::extract_http_like_error reads .status and .message off this class by name and forwards .message on a 4xx without calling str(). Backend exception text is bounded head-and-tail, since aiohttp renders the host before the errno. - validate_local_path uses exc.strerror rather than the raw OSError, whose text repeats the filename, and now catches the ValueError that Path.resolve() raises on an embedded NUL so callers keep their 4xx-vs-5xx decision. Rebased onto ai-dynamo#14563 (single aiohttp backend); the httpx-side half of the max_bytes plumbing went with that backend. Signed-off-by: nnshah1 <neelays@nvidia.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(deps): upgrade fastokens to 0.3.2 (ai-dynamo#14798) Signed-off-by: jthomson04 <jwillthomson19@gmail.com> * fix(vllm): ship codec-free OpenCV for image inputs (ai-dynamo#14361) Signed-off-by: svc-glamr@nvidia.com <svc-glamr@nvidia.com> Signed-off-by: GLAMR <svc-glamr@nvidia.com> Co-authored-by: Anant Sharma <anants@nvidia.com> Co-authored-by: yunzhoul-nv <232973175+yunzhoul-nv@users.noreply.github.com> * docs: refresh community events Automated refresh from the public Dynamo Google Calendar. Generated by .github/workflows/community-events-refresh.yml. Signed-off-by: dynamo-ops <170655669+dynamo-ops@users.noreply.github.com> * ci: refresh the compliance baseline in auto-upgrade pipeline (ai-dynamo#14206) Signed-off-by: Anant Sharma <anants@nvidia.com> * feat(triton): honor KServe classification on tensor outputs (ai-dynamo#14783) Signed-off-by: Yingge He <yinggeh@nvidia.com> * docs(rl): stop the verl guide sending readers to a vLLM version it cannot run on (ai-dynamo#14571) Signed-off-by: svc-glamr@nvidia.com <svc-glamr@nvidia.com> * feat(mocker): publish native KV events from the vLLM gRPC server (ai-dynamo#14737) Signed-off-by: jthomson04 <jwillthomson19@gmail.com> * fix(kv-router): release unowned radix branches after eviction (ai-dynamo#14878) Signed-off-by: jthomson04 <jwillthomson19@gmail.com> * fix: show correct backend versions in the install selectors (ai-dynamo#13599) Signed-off-by: Anant Sharma <anants@nvidia.com> * build(vllm): prepare v0.29.0 bump (ai-dynamo#14543) Signed-off-by: Julien Darve <jdarve@NVIDIA.com> * ci(xpu): validation PR for the re-applied XPU workflows and Dockerfile Throwaway PR to prove the CI merged in #22 actually runs end to end on XPU hardware. Adds only a comment to container/templates/vllm_runtime.Dockerfile, which matches the `vllm` path filter (container/templates/vllm_*) and so makes changed-files set vllm=true, which is what gates build-xpu and the heterog-test-px-dn / heterog-test-pn-dx jobs. What this exercises: - .github/workflows/pr-xpu.yaml (push to pull-request/[0-9]+, needs the xpu label) - .github/workflows/pr-xpu-heterogeneous.yaml (push; its guard deliberately skips the label gate) - .github/workflows/epd-test-template.yml (workflow_call, from the heterog jobs) - .github/scripts/test-filters.js (the brace fix from #22) - container/templates/vllm_runtime.Dockerfile rendered and built for device=xpu Not exercised: .github/workflows/xpu-heterogeneous-dispatch.yaml is workflow_dispatch only and has to be run by hand from the Actions tab. The marker comment must be removed before this branch is ever merged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(triton): Update Triton Base Image to 26.08 (ai-dynamo#14854) Signed-off-by: J Wyman <jwyman@nvidia.com> Co-authored-by: Rini Gupta <rinig@nvidia.com> * fix(operator): normalize equivalent worker hash inputs (ai-dynamo#14721) Signed-off-by: bzsuni <bingzhe.sun@daocloud.io> * test(sglang): exercise NIXL in embedding cache E/PD test (ai-dynamo#14795) Signed-off-by: Sai Kiran Polisetty <spolisetty@nvidia.com> * fix(sglang): stop the elastic-EP scale-up worker crash-looping at startup (ai-dynamo#14568) Signed-off-by: svc-glamr@nvidia.com <svc-glamr@nvidia.com> Co-authored-by: yunzhoul-nv <232973175+yunzhoul-nv@users.noreply.github.com> * fix(responses): honor tool_choice when parsing tool calls from text (ai-dynamo#14843) Signed-off-by: xianlubird <xianlubird@gmail.com> * ci: accept trusted full-CI request comments (ai-dynamo#14868) Signed-off-by: Matej Kosec <mkosec@nvidia.com> * docs: clarify EPP mode boundary and single-replica Dynamo mode fixes [DYN-4310] (ai-dynamo#14756) Signed-off-by: Anna Tchernych <atchernych@nvidia.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci(docs): move the generated-tables determinism gate out of link checking (ai-dynamo#14135) Signed-off-by: Dan Gil <dagil@nvidia.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * ci(docs): generate the Kubernetes API reference at publish time (ai-dynamo#14122) Signed-off-by: Dan Gil <dagil@nvidia.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * fix(operator): discover pull secrets for init containers (ai-dynamo#14922) Signed-off-by: bojiang-li <327132355+bojiang-li@users.noreply.github.com> * fix(sglang): stop an unusable mooncake backend crashing workers after model load (ai-dynamo#14461) Signed-off-by: svc-glamr@nvidia.com <svc-glamr@nvidia.com> Signed-off-by: glamr-agent <glamr-agent@users.noreply.github.com> Signed-off-by: GLAMR <svc-glamr@nvidia.com> * fix(sglang): emit prefill handoff before completion in sidecar (ai-dynamo#14260) Signed-off-by: jain-ria <riajain@NVIDIA.com> Co-authored-by: jain-ria <riajain@NVIDIA.com> Co-authored-by: Connor Carpenter <connorc@nvidia.com> Co-authored-by: ishandhanani <82981111+ishandhanani@users.noreply.github.com> * test(trtllm): enable fault tolerance coverage (ai-dynamo#14609) Signed-off-by: tanmayv25 <tanmay2592@gmail.com> * fix(frontend): evict async tokenizer executors when the tokenizer is retired (ai-dynamo#13368) Signed-off-by: Peter Pan <Peter.Pan@daocloud.io> * fix(llm): report KServe datatypes by their wire names, not protobuf variants (ai-dynamo#14957) `ModelMetadata` reported each Triton-registered tensor's `datatype` using `inference::DataType::as_str_name()`, which returns the `model_config.proto` variant name (`TYPE_FP32`, `TYPE_STRING`, ...) instead of the KServe v2 wire names (`FP32`, `BYTES`, ...). Every datatype was wrong, so spec-conforming clients cannot parse any tensor the RPC describes. Adds `oip_name()` next to `tensor::DataType::to_kserve` covering all fifteen proto variants (incl. FP16 and BF16) and mapping `TYPE_STRING → BYTES`. Original PR by @ayaangazali: ai-dynamo#14770. Reissued under a signed commit to unblock the copy-pr-bot signature gate; diff is byte-identical. Closes ai-dynamo#14520. Signed-off-by: ayaangazali <ayaangazali@users.noreply.github.com> Signed-off-by: ayaangazali <ayaangazali.work@gmail.com> Signed-off-by: Vinya Kestur <vinyak@nvidia.com> Co-authored-by: ayaangazali <ayaangazali.work@gmail.com> * docs(mm-routing): document video KV routing (ai-dynamo#14958) Signed-off-by: krishung5 <krish@nvidia.com> * fix(sidecar): honor worker namespace suffix (ai-dynamo#14955) Signed-off-by: Biswa Panda <biswa.panda@gmail.com> * fix(bindings): drain bridge tasks before interpreter finalization (ai-dynamo#14813) Signed-off-by: svc-glamr@nvidia.com <svc-glamr@nvidia.com> Signed-off-by: GLAMR <svc-glamr@nvidia.com> Co-authored-by: Tushar Sharma <tusharma@nvidia.com> * fix(discovery): stop a Qwen3-VL worker from serving video with another worker's contract (ai-dynamo#14624) Signed-off-by: svc-glamr@nvidia.com <svc-glamr@nvidia.com> Signed-off-by: GLAMR <svc-glamr@nvidia.com> * fix(gms): honor configured timeout during initial weights admission (ai-dynamo#14877) Signed-off-by: svc-glamr@nvidia.com <svc-glamr@nvidia.com> Signed-off-by: Schwinn Saereesitthipitak <schwinns@nvidia.com> Signed-off-by: GLAMR <svc-glamr@nvidia.com> Co-authored-by: Schwinn Saereesitthipitak <schwinns@nvidia.com> * feat(kv-router): add construction-time indexer delegates (ai-dynamo#14945) * fix(sglang): support min_tokens on tokenizer-free decode workers (ai-dynamo#14276) Signed-off-by: svc-glamr@nvidia.com <svc-glamr@nvidia.com> Signed-off-by: GLAMR <svc-glamr@nvidia.com> Signed-off-by: jain-ria <riajain@NVIDIA.com> Co-authored-by: jain-ria <riajain@NVIDIA.com> Co-authored-by: MatejKosec <mkosec@nvidia.com> * feat(router): add SessionPrefixIndexer for session-block lineage (ai-dynamo#13807) Signed-off-by: svc-glamr@nvidia.com <svc-glamr@nvidia.com> Signed-off-by: Karen Chung <karenc@nvidia.com> Signed-off-by: Matej Kosec <mkosec@nvidia.com> Co-authored-by: svc-glamr@nvidia.com <svc-glamr@nvidia.com> Co-authored-by: Matej Kosec <mkosec@nvidia.com> * fix(vllm): settle kvwarm stages through a per-step round on every attention-DP rank (ai-dynamo#14728) Signed-off-by: Yiming Liu <yimingl@nvidia.com> * feat(vllm): benchmark hybrid caches with random KDA state (ai-dynamo#14900) Signed-off-by: hongkuanz <hongkuanz@nvidia.com> * fix(runtime): fix QUIC reassembly and reduce response stalls (ai-dynamo#14876) Signed-off-by: jthomson04 <jwillthomson19@gmail.com> * feat(router): unify frontend and standalone selection core (ai-dynamo#14570) Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com> Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Thomas Montfort <tjmontfort12@gmail.com> * fix(planner): keep control APIs responsive during Prometheus collection (ai-dynamo#14377) Signed-off-by: xianlubird <xianlubird@gmail.com> Co-authored-by: Hongkuan Zhou <tedzhouhk@gmail.com> * fix(router): record SGLang prefill completion after stream ends (ai-dynamo#14968) Signed-off-by: jain-ria <riajain@NVIDIA.com> * fix(frontend): send inline media once on the TCP request plane (ai-dynamo#14801) Signed-off-by: Sumit Mishra <sah299610@gmail.com> Co-authored-by: Indrajit Bhosale <iamindrajitb@gmail.com> * docs: refresh community events Automated refresh from the public Dynamo Google Calendar. Generated by .github/workflows/community-events-refresh.yml. Signed-off-by: dynamo-ops <170655669+dynamo-ops@users.noreply.github.com> * fix(vllm): initialize synchronizer in KV warmup capacity test (ai-dynamo#14984) Signed-off-by: Alec Flowers <aflowers@nvidia.com> * fix(recipes): make the Solar Open2 250B benchmark and docs link usable (ai-dynamo#14956) Signed-off-by: Sandhya Rani Narravula <snarravula@nvidia.com> * feat(recipes): add K-EXAONE 2.0 750B-A37B NVFP4 vLLM recipes for B200 (ai-dynamo#14822) Signed-off-by: Cheng Wang <chengwa@nvidia.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat: KVCR Resiliency Deployment Example (ai-dynamo#14695) Add two-node DynamoGraphDeployment examples for process-local KVCR and the KVCR memory service. Run one vLLM worker per GPU node, use stable Grove ordinals for cache-owner slots, and request GPU-local RDMA resources for engines and Guard services. Provide a deployment helper for rendering and selecting either variant. Run the KV state agent alongside vLLM for process-local host memory. In memory-service mode, keep KVCR and the state agent in a separate container so its Guard and shared-memory pool survive engine restarts. Document that restarting the services sidecar invalidates the MVP recovery contract and requires deployment-level replacement. Add manifest coverage and an opt-in two-host lifecycle test. Kill the source EngineCore, hold it offline, and verify that the promoted Guard serves its preserved cache to the surviving target. Correlate response equality and KVCR transfer metrics with transmit and receive counters from the selected active HCA to prove RDMA transport. Pin compatible KVCR and vLLM revisions and document the runtime, discovery, compatibility-digest, and recovery prerequisites. Signed-off-by: Adit Ranadive <aranadive@nvidia.com> * feat(omni): add Nemotron Audex speech synthesis to /v1/audio/speech (ai-dynamo#12788) Signed-off-by: Thanaji Rao Thakkalapelli <thanaji.rao.thakkalapelli@intel.com> * ci: allow glamr-agent to request CI on its own unsigned PRs (ai-dynamo#14964) Signed-off-by: Matej Kosec <mkosec@nvidia.com> * fix(vllm): isolate multimodal worker ports (ai-dynamo#14751) Signed-off-by: Keiven Chang <keivenchang@users.noreply.github.com> Co-authored-by: Keiven Chang <keivenchang@users.noreply.github.com> * fix(runtime): reject invalid DYN_REQUEST_PLANE values (ai-dynamo#12612) Signed-off-by: svc-glamr@nvidia.com <svc-glamr@nvidia.com> Signed-off-by: Matej Kosec <mkosec@nvidia.com> Signed-off-by: Coding Agent <svc-glamr@nvidia.com> Signed-off-by: GLAMR <svc-glamr@nvidia.com> Co-authored-by: MatejKosec <mkosec@nvidia.com> * fix(responses): preserve text instead of inferring tool calls (ai-dynamo#14846) Signed-off-by: xianlubird <xianlubird@gmail.com> Co-authored-by: Ryan McCormick <rmccormick@nvidia.com> * chore: temporarily increase frontend build time limit 45 --> 90 min (ai-dynamo#15019) Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com> * test(operator): cover scoped CA injection ownership (ai-dynamo#14961) Signed-off-by: Julien Mancuso <jmancuso@nvidia.com> * feat(frontend): map semantic errors to HTTP responses (ai-dynamo#14396) Signed-off-by: Biswa Panda <biswa.panda@gmail.com> * docs: correct fault-tolerance architecture details (ai-dynamo#14880) Signed-off-by: Elizabeth Thomas <email2eliza@gmail.com> * build(deps): bump nats-server to v2.14.7 (ai-dynamo#14919) Signed-off-by: Dan Gil <dagil@nvidia.com> * build(deps): bump AISimulate to 0.12.0 (ai-dynamo#15012) Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com> * remove oneAPI env for XPU detection * feat(backends): expose native LoRA capacity in model registration (ai-dynamo#14754) Signed-off-by: Julien Darve <jdarve@NVIDIA.com> Signed-off-by: bzsuni <bingzhe.sun@daocloud.io> Co-authored-by: bzsuni <86399306+bzsuni@users.noreply.github.com> * fix(planner): handle pending decisions in virtual connector wait (ai-dynamo#14841) Signed-off-by: bzsuni <bingzhe.sun@daocloud.io> Co-authored-by: Hongkuan Zhou <tedzhouhk@gmail.com> * feat(vllm): add sidecar LoRA lifecycle (ai-dynamo#13068) Signed-off-by: Julien Darve <jdarve@NVIDIA.com> Signed-off-by: bzsuni <bingzhe.sun@daocloud.io> Co-authored-by: Julien Darve <jdarve@NVIDIA.com> Co-authored-by: bzsuni <86399306+bzsuni@users.noreply.github.com> * fix(vllm/omni): pass response_format into video EngineInputs (ai-dynamo#14667) (ai-dynamo#14844) * chore: bump version to 1.6.0 post 1.5.0 branch cut (ai-dynamo#15009) Signed-off-by: pvijayakrish <pvijayakrish@nvidia.com> Signed-off-by: Pavithra Vijayakrishnan <160681768+pvijayakrish@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(ci): Use `pytest --ignore` to Skip Tests Based on Framework (ai-dynamo#14815) Signed-off-by: J Wyman <jwyman@nvidia.com> * feat(sidecar): add e2e CI testing for sidecar launch scripts (ai-dynamo#14508) Signed-off-by: tanmayv25 <tanmay2592@gmail.com> Signed-off-by: Julien Darve <jdarve@NVIDIA.com> Co-authored-by: Julien Darve <jdarve@NVIDIA.com> * chore(xpu): upgrade vllm and omni to 0.29.0 Signed-off-by: wenxin.zhang <wenxin.zhang@intel.com> * docs(operator): document the DGDR workload-creation trust boundary (ai-dynamo#14429) Signed-off-by: nnshah1 <neelays@nvidia.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * fix(xpu): use released vllm-omni prerelease Signed-off-by: Wenxin Zhang <wenxin.zhang@intel.com> * test(efa): add the EFA disaggregated deploy test for sglang (ai-dynamo#13893) Signed-off-by: Jie Hao <jihao@nvidia.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(runtime): support IPv6-only IP resolution (ai-dynamo#13126) Signed-off-by: jthomson04 <jwillthomson19@gmail.com> * docs(fault-tolerance): clarify migration after shutdown grace expires (ai-dynamo#14872) Signed-off-by: Jacky <18255193+kthui@users.noreply.github.com> * feat(vllm-omni): preserve generated video audio (ai-dynamo#13707) Signed-off-by: Guan Luo <gluo@nvidia.com> Co-authored-by: Guan Luo <gluo@nvidia.com> * feat(vllm-omni): pass model-specific video parameters (ai-dynamo#13708) Signed-off-by: Guan Luo <gluo@nvidia.com> Co-authored-by: Guan Luo <gluo@nvidia.com> * feat(vllm-omni): qualify MiniMax-H3 T2VA on B200 (ai-dynamo#13589) Signed-off-by: Guan Luo <gluo@nvidia.com> Signed-off-by: GuanLuo <41310872+GuanLuo@users.noreply.github.com> Co-authored-by: Guan Luo <gluo@nvidia.com> Co-authored-by: GuanLuo <41310872+GuanLuo@users.noreply.github.com> Co-authored-by: Ryan McCormick <rmccormick@nvidia.com> * fix(vllm): remove obsolete Omni compatibility guard Signed-off-by: Wenxin Zhang <wenxin.zhang@intel.com> * fix(vllm): retain Omni compatibility guard Signed-off-by: Wenxin Zhang <wenxin.zhang@intel.com> * .github/workflows/pr-xpu-heterogeneous.yaml; pin GPU_TAG to latest * .github/workflows/; add post-merge and nightly XPU heterogeneous CI Extract the XPU heterogeneous P/D pipeline out of pr-xpu-heterogeneous.yaml into xpu-heterogeneous-run.yml, a workflow_call reusable workflow, and call it from three thin trigger workflows so all three merge phases run the identical pipeline instead of drifting copies. xpu-heterogeneous-run.yml new, reusable. guard, changed-files, build-xpu, build-nvidia, resolve-images and both heterog tests, unchanged, plus 7 inputs. pr-xpu-heterogeneous.yaml reduced to the pre-merge trigger, the slash-command gate and the reaction. post-merge-xpu-heterogeneous.yaml new. push to main. nightly-xpu-heterogeneous.yaml new file, but the cron is MOVED, not added: it is the 0 23 * * * schedule that was already in pr-xpu-heterogeneous.yaml. No behaviour change per phase. force_all_tests replaces the old github.event_name == 'schedule' || github.event_name == 'issue_comment' expression with the same truth table: pre-merge passes github.event_name == 'issue_comment', nightly passes true. Post-merge also passes true, because a push to main has no PR base for .github/actions/changed-files to diff against, and post-merge exists to catch what per-PR gating missed. xpu-status-check stays a TOP-LEVEL job in each caller rather than moving into the reusable workflow. A job contributed by a reusable workflow reports to the Checks API as "run / xpu-status-check", so hosting it there would rename the context and leave any branch protection rule requiring xpu-status-check waiting forever on a check that no longer reports. The concurrency mapping stays byte-identical across all four workflows that touch this hardware, now including xpu-heterogeneous-dispatch.yaml. Three files do NOT get three slots: the cluster, the dynamo-system namespace and the onexpu-/onenvidia-rdma-kueue ResourceClaimTemplates are one global resource. The reusable workflow deliberately carries no concurrency block of its own, which would deadlock against the slot the caller's run already holds. Parameterised gpu_tag, model, tensor_parallel and runner as inputs so the callers can diverge; all default to the previously hardcoded values. Added workflow_dispatch to the nightly, without which a schedule-only workflow cannot be exercised before it reaches the default branch. Verified: all files parse; the four concurrency mappings are byte-identical; the reusable workflow declares no concurrency; every input each caller passes exists and every required input is supplied; nesting is depth 3 of the 4 GitHub allows. actionlint was not available to run, and will report queue:max as an unknown key in all four files, a known false positive. --------- Signed-off-by: Nikita Sukharev <kaonael@gmail.com> Signed-off-by: xianlubird <xianlubird@gmail.com> Signed-off-by: hongkuanz <hongkuanz@nvidia.com> Signed-off-by: William Arnold <7565007+Aphoh@users.noreply.github.com> Signed-off-by: Sandhya Rani Narravula <snarravula@nvidia.com> Signed-off-by: Karen Chung <karenc@nvidia.com> Signed-off-by: jthomson04 <jwillthomson19@gmail.com> Signed-off-by: Wenxin Zhang <wenxin.zhang@intel.com> Signed-off-by: VincyZhang <wenxin.zhang@intel.com> Signed-off-by: krishung5 <krish@nvidia.com> Signed-off-by: nnshah1 <neelays@nvidia.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com> Signed-off-by: svc-glamr@nvidia.com <svc-glamr@nvidia.com> Signed-off-by: GLAMR <svc-glamr@nvidia.com> Signed-off-by: dynamo-ops <170655669+dynamo-ops@users.noreply.github.com> Signed-off-by: Anant Sharma <anants@nvidia.com> Signed-off-by: Yingge He <yinggeh@nvidia.com> Signed-off-by: Julien Darve <jdarve@NVIDIA.com> Signed-off-by: J Wyman <jwyman@nvidia.com> Signed-off-by: bzsuni <bingzhe.sun@daocloud.io> Signed-off-by: Sai Kiran Polisetty <spolisetty@nvidia.com> Signed-off-by: Matej Kosec <mkosec@nvidia.com> Signed-off-by: Anna Tchernych <atchernych@nvidia.com> Signed-off-by: Dan Gil <dagil@nvidia.com> Signed-off-by: bojiang-li <327132355+bojiang-li@users.noreply.github.com> Signed-off-by: glamr-agent <glamr-agent@users.noreply.github.com> Signed-off-by: jain-ria <riajain@NVIDIA.com> Signed-off-by: tanmayv25 <tanmay2592@gmail.com> Signed-off-by: Peter Pan <Peter.Pan@daocloud.io> Signed-off-by: ayaangazali <ayaangazali@users.noreply.github.com> Signed-off-by: ayaangazali <ayaangazali.work@gmail.com> Signed-off-by: Vinya Kestur <vinyak@nvidia.com> Signed-off-by: Biswa Panda <biswa.panda@gmail.com> Signed-off-by: Schwinn Saereesitthipitak <schwinns@nvidia.com> Signed-off-by: Yiming Liu <yimingl@nvidia.com> Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com> Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com> Signed-off-by: Sumit Mishra <sah299610@gmail.com> Signed-off-by: Alec Flowers <aflowers@nvidia.com> Signed-off-by: Cheng Wang <chengwa@nvidia.com> Signed-off-by: Adit Ranadive <aranadive@nvidia.com> Signed-off-by: Thanaji Rao Thakkalapelli <thanaji.rao.thakkalapelli@intel.com> Signed-off-by: Keiven Chang <keivenchang@users.noreply.github.com> Signed-off-by: Coding Agent <svc-glamr@nvidia.com> Signed-off-by: Julien Mancuso <jmancuso@nvidia.com> Signed-off-by: Elizabeth Thomas <email2eliza@gmail.com> Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com> Signed-off-by: pvijayakrish <pvijayakrish@nvidia.com> Signed-off-by: Pavithra Vijayakrishnan <160681768+pvijayakrish@users.noreply.github.com> Signed-off-by: wenxin.zhang <wenxin.zhang@intel.com> Signed-off-by: Jie Hao <jihao@nvidia.com> Signed-off-by: Jacky <18255193+kthui@users.noreply.github.com> Signed-off-by: Guan Luo <gluo@nvidia.com> Signed-off-by: GuanLuo <41310872+GuanLuo@users.noreply.github.com> Co-authored-by: Nikita Sukharev <kaonael@gmail.com> Co-authored-by: Xianlu Bird <xianlubird@gmail.com> Co-authored-by: Hongkuan Zhou <tedzhouhk@gmail.com> Co-authored-by: William Arnold <7565007+Aphoh@users.noreply.github.com> Co-authored-by: Zero Rains <57100978+zeroRains@users.noreply.github.com> Co-authored-by: snarravula-dl <snarravula@nvidia.com> Co-authored-by: Karen Chung <karenc@nvidia.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: jthomson04 <jwillthomson19@gmail.com> Co-authored-by: VincyZhang <wenxin.zhang@intel.com> Co-authored-by: Kris Hung <krish@nvidia.com> Co-authored-by: Neelay Shah <neelays@nvidia.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: GLAMR <svc-glamr@nvidia.com> Co-authored-by: Anant Sharma <anants@nvidia.com> Co-authored-by: yunzhoul-nv <232973175+yunzhoul-nv@users.noreply.github.com> Co-authored-by: dynamo-ops <170655669+dynamo-ops@users.noreply.github.com> Co-authored-by: Yingge He <157551214+yinggeh@users.noreply.github.com> Co-authored-by: JulienDarve <86800349+JulienDarve@users.noreply.github.com> Co-authored-by: J Wyman <jwyman@nvidia.com> Co-authored-by: Rini Gupta <rinig@nvidia.com> Co-authored-by: bzsuni <86399306+bzsuni@users.noreply.github.com> Co-authored-by: Sai Kiran Polisetty <spolisetty@nvidia.com> Co-authored-by: MatejKosec <mkosec@nvidia.com> Co-authored-by: atchernych <atchernych@nvidia.com> Co-authored-by: Dan Gil <dagil@nvidia.com> Co-authored-by: Bojiang Li <327132355+bojiang-li@users.noreply.github.com> Co-authored-by: Connor Carpenter <connorcarpenter15@gmail.com> Co-authored-by: jain-ria <riajain@NVIDIA.com> Co-authored-by: Connor Carpenter <connorc@nvidia.com> Co-authored-by: ishandhanani <82981111+ishandhanani@users.noreply.github.com> Co-authored-by: Tanmay Verma <tanmayv@nvidia.com> Co-authored-by: Peter Pan <peter.pan@daocloud.io> Co-authored-by: Vinya Kestur Tumakuru Arun Kumar <vinyak@nvidia.com> Co-authored-by: ayaangazali <ayaangazali.work@gmail.com> Co-authored-by: Biswa Panda <biswa.panda@gmail.com> Co-authored-by: Tushar Sharma <tusharma@nvidia.com> Co-authored-by: Schwinn Saereesitthipitak <schwinns@nvidia.com> Co-authored-by: Ryan Olson <ryanolson@users.noreply.github.com> Co-authored-by: Yimingl_Nvidia <yimingl@nvidia.com> Co-authored-by: Thomas Montfort <tjmontfort12@gmail.com> Co-authored-by: Sumit884-byte <sah299610@gmail.com> Co-authored-by: Indrajit Bhosale <iamindrajitb@gmail.com> Co-authored-by: Alec <35311602+alec-flowers@users.noreply.github.com> Co-authored-by: chw001 <chengwa@nvidia.com> Co-authored-by: Adit Ranadive <aranadive@nvidia.com> Co-authored-by: Thanaji Rao Thakkalapelli <thanaji.rao.thakkalapelli@intel.com> Co-authored-by: Keiven C <213854356+keivenchang@users.noreply.github.com> Co-authored-by: Keiven Chang <keivenchang@users.noreply.github.com> Co-authored-by: Ryan McCormick <rmccormick@nvidia.com> Co-authored-by: Dmitry Tokarev <dtokarev@nvidia.com> Co-authored-by: Julien Mancuso <161955438+julienmancuso@users.noreply.github.com> Co-authored-by: Elizabeth Thomas <email2eliza@gmail.com> Co-authored-by: Harrison Saturley-Hall <hsaturleyhal@nvidia.com> Co-authored-by: Julien Darve <jdarve@NVIDIA.com> Co-authored-by: Jasim Kareem <mj9034812@gmail.com> Co-authored-by: Pavithra Vijayakrishnan <160681768+pvijayakrish@users.noreply.github.com> Co-authored-by: Jie Hao <jihao@nvidia.com> Co-authored-by: Jacky <18255193+kthui@users.noreply.github.com> Co-authored-by: Qi Wang <qiwa@nvidia.com> Co-authored-by: Guan Luo <gluo@nvidia.com> Co-authored-by: GuanLuo <41310872+GuanLuo@users.noreply.github.com>
This branch was successfully deployed
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
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
DYN_REQUEST_PLANEnow defaults to TCP only when the variable is absent. Every present Unicode value, including an empty value, must parse case-insensitively astcpornats; invalid and non-Unicode values no longer silently select TCP.DistributedRuntime::from_settingsand new fallible configuration constructors return these values through the runtime startup error path, including the C initializer’s existing error result. The establishedDistributedConfig::from_settingsandfor_clisignatures remain source-compatible and fail fast. The architecture documentation records the environment contract, and parser tests use injected values rather than process-global environment mutation.This closes #12552 and implements DGH-1254.
Validation: On merge head
49974f440229bf4ac8c323fd8f5e8067814516db, Rust 1.96.1rustfmt --edition 2024 --check lib/runtime/src/distributed.rsandgit diff --cached --checkpassed. The merge preserves per-worker override precedence and propagates invalid environment values through the fallible configuration path. Remote CI is pending; no local tests, builds, or benchmarks were run during this maintenance pass. Validation recorded before this merge applies to earlier heads only.