Repository navigation
fix(runtime): rebind system status listener after accept failures - #12059
Merged
MatejKosec merged 33 commits intoOct 6, 2026
Merged
MatejKosec merged 33 commits into
MatejKosec merged 33 commits into
Conversation
glamr-agent
temporarily deployed
to
external_collaborator
July 23, 2026 03:09 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
glamr-agent
temporarily deployed
to
external_collaborator
July 23, 2026 03:09 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
👋 Hi glamr-agent! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
the
external-contribution
Contributor
Author
change.md# Change — materialization of ai-dynamo/dynamo#12059 (no new authorship)
## This is somebody else's work
**Nothing in this diff was authored by this run.** The single commit on the
working branch is a verbatim materialization of the head diff of
[ai-dynamo/dynamo#12059](https://github.com/ai-dynamo/dynamo/pull/12059) —
*"fix(runtime): rebind system health server after CRIU listener loss"* — placed
on a local branch so that this work item's validator can compile and execute it
here. The plan's `Route: review` carries no `Review action: reproduce` line and
the caller asked only to *watch and review*, so per the `existing-request`
template's Printer focus I materialized the supplied head diff and wrote no fix,
no refactor, no test, and no drive-by cleanup of my own.
Attribution, taken from the PR's own commit list (`gh api .../pulls/12059/commits`):
| Role | Identity |
|---|---|
| PR author (opened the request) | `glamr-agent` |
| Original fix commit `b4d60c16d` (2026-07-28) | `svc-glamr@nvidia.com <svc-glamr@nvidia.com>` |
| Commits `32d582037`, `8cfdbe62b`, `bf2125f01` (merges from `main`) | Matej Kosec `<mkosec@nvidia.com>` |
| Commit `87a366567` (libc dev-dependency) | Matej Kosec `<mkosec@nvidia.com>` |
| Head commit `8325b4329` (socket2 instead of libc, 2026-08-12T17:22Z) | Matej Kosec `<mkosec@nvidia.com>` |
So the change is joint work: the original rebinding fix by `glamr-agent`, plus
later corrections by Matej Kosec. Both are recorded as `Co-authored-by:` trailers
on the local commit, and the commit body opens with `NOT AUTHORED HERE.` and
states explicitly that it "is not a fix, not a competing implementation, and must
not be published as one."
One naming trap worth flagging for the reviewer and publisher: this sandbox's own
git identity is also `svc-glamr@nvidia.com`, which happens to equal the author of
the *original* upstream commit `b4d60c16d`. The identity match is a coincidence of
the factory service account, **not** evidence that this run wrote the code. The
`Signed-off-by:` trailer attests only that I materialized and pass on the work.
## Commit
| | |
|---|---|
| Branch | `chore/review-pr-12059--cab24ee14044` |
| Branch point | `bb2dcc126ed833cf66997bc2545fd18c21f11d89` (`main`, unmoved) |
| Commit SHA | `41c6c3a3141de63ad64941d6fcf87c5846f01e07` |
| Files changed | 1 |
| Lines | +250 / −35 |
| Working tree after commit | clean |
`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 DCO trailer match, and there is exactly one `Signed-off-by:` line (the
repo's `prepare-commit-msg` hook is idempotent against `--signoff`, as
`learnings/commits-dco.md` describes).
## Per-file summary
**`lib/runtime/src/system_status_server.rs`** (+250/−35) — the only file touched.
The upstream change extracts `build_system_status_router` (:174) and
`bind_system_status_listener` (:270) out of `spawn_system_status_server`; adds a
private `struct RebindingTcpListener` (:303) with an `impl Listener` (:322) whose
`accept` loop drops and rebinds a listening socket that has stopped accepting;
adds `is_transient_accept_error` (:372) and a one-second
`SYSTEM_STATUS_REBIND_BACKOFF` (:133); and adds three `#[tokio::test]`s at :871,
:917, and :935 (`test_rebinding_listener_serves_again_after_listener_shutdown`,
`..._reports_the_address_it_rebinds`, `..._still_shuts_down_gracefully`). That is
a description of what I received, not a review of it — judging the design is the
reviewer's job, and I deliberately did not touch a line of it.
## Was the apply clean? Yes — and byte-exact
Clean, with no conflict and no manual resolution of any kind.
- Source of the diff: `gh api -H "Accept: application/vnd.github.v3.diff"
repos/ai-dynamo/dynamo/compare/ccc9cf8d3661b1440b13b0d47f4bb7db256abe23...8325b43295571b62e53af030233ad519808aa1b3`
— pinned to the **immutable head SHA**, not to the mutable PR ref. I also ran
`gh pr diff 12059` and the two outputs were byte-identical, which additionally
confirms the head had not moved under me mid-run. Both are read-only calls.
- I re-checked the head SHA before applying: still
`8325b43295571b62e53af030233ad519808aa1b3`, state `OPEN`, 1 file, +250/−35 —
matching the plan's Discovery exactly.
- `git apply --check` passed, then `git apply` reported *"Applied patch
lib/runtime/src/system_status_server.rs cleanly."*
- The diff's pre-image blob `e85d8d649814` is what local `main` already had, as
the plan predicted.
The strongest check I ran: after applying, the working-tree file hashes to
`a7a9ac5d48cbce017e6352bfe35c10185fcb56a7` at 62431 bytes, and
`gh api ".../contents/lib/runtime/src/system_status_server.rs?ref=8325b432..."`
reports that same blob SHA and size. **The materialized file is byte-for-byte
identical to the file at the PR head.** There is no drift for the reviewer to
hunt.
`change.diff` (14527 bytes, `git diff bb2dcc126..HEAD`) re-applies cleanly to the
branch point — verified by checking out `bb2dcc126` detached and running
`git apply --check`. Compared against the upstream patch it differs on exactly
one line: `index e85d8d649..a7a9ac5d4` versus GitHub's
`index e85d8d649814..a7a9ac5d48cb`, a cosmetic `core.abbrev` width difference
over the *same* blobs. All 342 other lines are identical.
## What I ran, including what went wrong
I kept this deliberately narrow — recipes 00/02/05/07 belong to the validator and
I did not want to pre-empt or half-run them. What I did run:
- `git diff --check` — clean, no whitespace errors introduced.
- `cargo fmt -p dynamo-runtime --check` — **exit 0, passes.**
**The honest part: my first formatting check was a false alarm caused by my own
command, not by the code.** Following the per-file snippet in
`learnings/format-changed-only.md`, I ran
`cargo fmt -- --check lib/runtime/src/system_status_server.rs`, and it reported
four hunks of import-reordering "drift" (`use crate::config::HealthStatus;` and
`Router` in the `axum::{...}` group). Before touching anything, I stashed the
patch and ran the identical command against **unmodified `main`** — byte-identical
complaint, same four hunks. So it was not PR-introduced. Root cause: passing a
file path through to rustfmt bypasses cargo's edition detection, and rustfmt then
defaults to style edition 2015 while this workspace is `edition = "2024"`
(root `Cargo.toml:46`). `cargo fmt -- --check --style-edition 2024 <file>` exits
0, and the package-scoped `cargo fmt -p dynamo-runtime --check` exits 0. I
changed no code over this; per `learnings/format-changed-only.md` I would not
have "fixed" pre-existing drift anyway, but in this case there was no drift at
all. Recording it because a validator running the same per-file form would hit
the same false positive.
I did **not** run `cargo check`, `cargo clippy`, `cargo test`, or any smoke — no
compile of the materialized code has happened yet. That is recipe 02's and 07's
work and I left it whole rather than reporting a partial result.
**Environment mutation I made, disclosed:** `rustfmt` was not installed for the
active toolchain, so I ran `rustup component add rustfmt` once (succeeded). That
installed it for `1.93.1` only. Nothing else about the sandbox was changed, and
nothing outside the work-item root and the checkout was written.
## Handover notes for the validator
Two things I tripped over that will cost the validator time in recipe 02 if it
does not know them up front:
1. **The active Rust toolchain is not the one the repo pins.**
`rust-toolchain.toml` pins `1.96.1`, but `RUSTUP_TOOLCHAIN=1.93.1` is set in
the environment and overrides it (`rustup show` says *"active because:
overridden by environment variable RUSTUP_TOOLCHAIN"*). Both toolchains are
installed.
2. **`clippy` is missing from the active toolchain.** `cargo clippy --version` on
`1.93.1` returns *"'cargo-clippy' is not installed for the toolchain
'1.93.1-x86_64-unknown-linux-gnu'"*. The repo-pinned `1.96.1` already carries
both `clippy` and `rustfmt`
(`rustup component list --toolchain 1.96.1 --installed`). So recipe 02
Section 3 will not run as written until the validator either adds the
component to `1.93.1` or invokes the pinned toolchain.
I deliberately did not decide this for the validator. Both routes have a real
cost: `rustup component add clippy` keeps toolchain consistency with the
`dynamo._core` that `cli.ts` prebuilt under `1.93.1`, whereas switching to
`1.96.1` matches the repo's own pin and upstream CI but will recompile the
workspace from cold and may invalidate the prebuilt bindings that recipe 07's
worker loads. Whichever it picks, `change-validation.md` should say which
toolchain produced the numbers, because a clippy result from a different rustc
than the pinned one is weaker evidence than it looks.
Also carrying forward, unchanged, from the plan since it bears on recipe 07:
`lib/runtime/src/config.rs:21` sets `DEFAULT_SYSTEM_PORT: i16 = -1` and the
server only starts when `system_port >= 0`, so the smoke must export a positive
`DYN_SYSTEM_PORT` or it will exercise none of this code. And per the plan, recipe
00 must run *after* this materialization — the commit is now in place, so that
ordering constraint is satisfiable as of `41c6c3a31`.
## Constraints observed
No push, no PR, no comment, no CI trigger — every `gh` call was read-only
(`api ... --jq` reads, `pr diff`). No `mcp__compute-session__*`, `ssh`, or `srun`.
Branched from `main` only, under the exact name
`chore/review-pr-12059--cab24ee14044`. `plan.md` untouched; the only packets
written are `change.md` and `change.diff`.
MR URL: https://github.com/ai-dynamo/dynamo/pull/12059 |
Contributor
Author
change-validation.md# Change validation — PR #12059, `fix(runtime): rebind system health server after CRIU listener loss`
Local, advisory validation of the printer-materialized head of ai-dynamo/dynamo#12059
(1 file, +250/−35, all in `lib/runtime/src/system_status_server.rs`). Checkout at
`/home/sandbox/workspace/wi-20260812T174023Z-12059/repo`, commit `41c6c3a31`
("chore(review): materialize PR #12059 head 8325b432 for local review") on top of
`bb2dcc126`. Four recipes were run, each through
`/workflow/agents/customs/evidence.ts record`: `00-dynamo-editable-install`,
`02-rust-cargo-check`, `05-code-inspection`, `07-agg-smoke`.
## Reading the registry honestly
Seventeen rows are recorded and every one carries `exit=0`. That is not seventeen
successes. My wrapper scripts capture substantive failures in echoed markers
(`workspace-check-exit=101`, `registered=0`, `HTTP 404`) and then exit 0 themselves, so
the registry's exit column is a statement about the wrapper, not about the work. Three
recipe-07 rows and one recipe-02 row are failed attempts that read green in that column.
The narrative below names each one. The latest row per recipe — 00 at 18:06:55, 02 at
18:17:09, 05 at 18:41:10, 07 at 18:37:26 — is the one that carries the evidence, and
customs scores the row verdict from the latest run, which is correct here only because I
re-ran each recipe to a genuinely green state before stopping.
## Toolchain: which compiler produced these numbers
`rust-toolchain.toml` pins `1.96.1`, but the environment sets `RUSTUP_TOOLCHAIN=1.93.1`,
which overrides the file. **Every recipe-02 number below was produced by
`rustc 1.93.1 (01f6ddf75 2026-02-11)`** — the same toolchain that built the `dynamo._core`
extension that recipe 07 then loads, which is the reason I let the override stand rather
than forcing 1.96.1 and desynchronizing the compiled artifact from the smoke test.
Clippy was initially absent for 1.93.1. That is a solvable tooling gap, not a
`missing-dep` disposition: `rustup component add clippy --toolchain 1.93.1` installed it,
and the recorded run reports `clippy 0.1.93 (01f6ddf758 2026-02-11)` followed by
`clippy-exit=0` for `cargo clippy --workspace --all-targets --no-deps -- -D warnings`.
For formatting I ran both toolchains. Under 1.93.1: `fmt-all-exit=0`, `fmt-pkg-exit=0`
(`cargo fmt -p dynamo-runtime --check`). Cross-checked under the repo-pinned toolchain:
`RUSTUP_TOOLCHAIN=1.96.1 cargo fmt --all --check` → `fmt-all-196-exit=0`. I also
reproduced the printer's reported false positive for the record: the file-path-scoped
invocation `cargo fmt -- --check lib/runtime/src/system_status_server.rs` exits 1 and
wants to move `use crate::config::HealthStatus;` down the import block, because that form
bypasses the manifest and defaults to edition 2015 import ordering. Re-running the same
argv with `--style-edition 2024` exits 0. The file is correctly formatted; only the
scoping was wrong.
## Recipe 00 — editable install, and the trap it hides
The plan warned that recipe 00 can short-circuit and leave the Python extension holding
pre-change bytes, in which case recipe 07's green result would be evidence of nothing.
That is exactly what happened on the first run: at 18:03:32 the recorded `BEFORE` probe
showed `_core.abi3.so` with mtime `Wed Aug 12 17:53:19 2026`, while the changed source
`lib/runtime/src/system_status_server.rs` has mtime `18:00:50` — the extension predated
the diff by seven minutes. The editable install did not rebuild it.
I therefore ran `maturin develop --uv` explicitly from `lib/bindings/python` through the
recorder (18:03:57). Its output names the crate under change:
```
Compiling dynamo-runtime v1.4.0 (/home/sandbox/.../repo/lib/runtime)
Compiling dynamo-py3 v1.4.0 (/home/sandbox/.../repo/lib/bindings/python)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 1m 53s
📦 Built wheel for abi3 Python ≥ 3.10 ...
🛠 Installed ai-dynamo-runtime-1.4.0
```
The 18:06:55 verification run then established three things: the extension resolves inside
the checkout (`dynamo OK at ['.../components/src/dynamo', '.../lib/bindings/python/src/dynamo']`),
its mtime is now `Wed Aug 12 18:06:00 2026` (`extension newer than changed source: True`),
and — the decisive check — a string only introduced by this PR is present in the compiled
object: `strings _core.abi3.so | grep -c 'System status listener stopped accepting'` → `1`.
No `PYTHONPATH` substitution was used anywhere.
Recipe 00 also surfaced an environment failure that recurs below: the
`utoipa-swagger-ui v9.0.2` build script tries to download Swagger UI over the network and
fails with `curl ... exit status: 52` in this egress-less sandbox. Exporting
`SWAGGER_UI_DOWNLOAD_URL="file:///opt/dynamo/assets/swagger-ui-v5.17.14.zip"` resolves it.
This is a sandbox property, not a PR defect, and the export was applied to every
subsequent cargo invocation.
## Recipe 02 — cargo check, clippy, and the tests that carry the weight
Section 1 (format) is covered above. Section 2 (`cargo check`) failed on its first
recorded attempt with `workspace-check-exit=101` and `bindings-check-exit=101` — the
swagger download. With the local-file export, the re-run (18:09:29) gives
`workspace-check-exit=0` and `bindings-check-exit=0` for `cargo check --workspace
--all-targets` and `cargo check --manifest-path lib/bindings/python/Cargo.toml
--all-targets`. Section 3 (clippy) is `clippy-exit=0` as noted.
**Section 4 is the primary evidence, and it is real.** The full suite ran in the actual
crate binary against the actual changed source:
```
cargo test -p dynamo-runtime --lib
test result: ok. 578 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 111.49s
```
The three tests the PR adds each executed and each passed. Section 4b ran them together
with `--nocapture --test-threads=1`:
```
running 3 tests
test system_status_server::tests::test_rebinding_listener_reports_the_address_it_rebinds ... ok
test system_status_server::tests::test_rebinding_listener_serves_again_after_listener_shutdown ... ok
test system_status_server::tests::test_rebinding_listener_still_shuts_down_gracefully ... ok
test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 577 filtered out; finished in 0.03s
```
`test_rebinding_listener_serves_again_after_listener_shutdown` is the only local evidence
that recovery happens at all. It stands in for CRIU by calling
`socket2::SockRef::from(&listener).shutdown(Shutdown::Both)` on the *listening* socket,
and it contains a deliberate early return for platforms that refuse that call:
`eprintln!("skipping: this platform refused shutdown on a listening socket: {error}"); return;`.
**On this host it did not take that path.** Section 4e greps the un-captured output for
that marker and reports `0`. The recovery assertions genuinely ran: the listener was
broken, the rebind fired, and the server served again.
I also ran the three tests as paired polarities, each as its own `--exact` invocation, so
the new harness is shown to distinguish the cases rather than merely to pass:
- Broken-listener polarity (4c) — `shutdown(2)` applied, rebind is required:
`test_rebinding_listener_serves_again_after_listener_shutdown ... ok`,
`test result: ok. 1 passed; 0 failed; ... 579 filtered out`.
- Healthy-listener polarity (4d) — accept works and no rebind should occur:
`test_rebinding_listener_reports_the_address_it_rebinds ... ok` and
`test_rebinding_listener_still_shuts_down_gracefully ... ok`, each
`test result: ok. 1 passed; 0 failed; ... 579 filtered out`
(`healthy-shutdown-exit=0`).
The graceful-shutdown test matters more than its name suggests: it drives
`axum::serve(listener, app).with_graceful_shutdown(cancel_token.cancelled_owned())`
against the new listener type, which is the interaction most likely to regress into a
hang, and it terminates.
## Recipe 05 — the five claims that cannot be executed here
Each claim is cited from source read in this sandbox, not from memory. The axum source is
the resolved one: root `Cargo.toml:154` pins `axum = { version = "=0.8.4", ... }`,
`Cargo.lock:922` carries `version = "0.8.4"`, and the vendored crate inspected below is
`/scratch/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/axum-0.8.4` whose own
`Cargo.toml` reports `version = "0.8.4"`.
**Claim 1 — `Listener::accept` returns no `Result`, so axum retries the same socket
forever.** `axum-0.8.4/src/serve/listener.rs:20` declares
`fn accept(&mut self) -> impl Future<Output = (Self::Io, Self::Addr)> + Send;` with the
doc "If the underlying accept call can return an error, this function must take care of
logging and retrying." axum's own `impl Listener for TcpListener` (`listener.rs:26-42`)
loops on `Self::accept(self)` and on error calls `handle_accept_error(e)`, which
(`listener.rs:140-158`) logs `error!("accept error: {e}")`, sleeps one second, and returns
— to the same file descriptor. This is precisely the failure mode the PR describes: after
CRIU restore the socket never recovers, and stock axum will spin on it for the life of the
process. The trait signature is also why the fix has to be a new `Listener` impl rather
than a wrapper around `TcpListener::accept`.
**Claim 2 — axum selects the shutdown future against accept.**
`axum-0.8.4/src/serve/mod.rs:284-291`:
```rust
let (io, remote_addr) = tokio::select! {
conn = listener.accept() => conn,
_ = signal_tx.closed() => { trace!("signal received, not accepting new connections"); break; }
};
```
The consequence is that the new `accept`'s internal `tokio::time::sleep` backoff cannot
delay shutdown: the select cancels the accept future outright. This is what makes the
1-second rebind backoff safe, and it is corroborated empirically by
`test_rebinding_listener_still_shuts_down_gracefully` and by recipe 07's SIGTERM result.
**Claim 3 — the transient `ErrorKind` set matches axum's own.**
`is_transient_accept_error` (`system_status_server.rs:371-379`) matches
`ConnectionRefused | ConnectionAborted | ConnectionReset`. axum's `is_connection_error`
(`listener.rs:160-167`) matches the identical triple. The classification is neither
narrower nor wider than upstream's, so a client that dies between SYN and accept is
retried immediately and does not trigger a spurious rebind.
**Claim 4 — drop-before-rebind avoids `EADDRINUSE`.** In the accept loop
(`system_status_server.rs:326-366`) the non-transient arm sets `self.listener = None`
*before* sleeping, and the rebind happens on the next iteration through the
`let Some(listener) = self.listener.as_ref() else { ... }` branch. The ordering is
load-bearing and the code says so: a socket that has stopped accepting is still bound, so
rebinding first would fail. The rebind targets `self.address` — the *resolved* address
from the initial bind, not the caller's request — which is what keeps a `port 0`
deployment on the same port after recovery, and is what
`test_rebinding_listener_reports_the_address_it_rebinds` asserts.
**Claim 5 — one production caller, behind a default-off gate.** Outside its own module,
`spawn_system_status_server` is called exactly once, at
`lib/runtime/src/distributed.rs:248`. It is reached only when `cancel_token` is `Some`,
which follows `system_server_enabled()` (`config.rs:339-344`, `self.system_port >= 0`),
and `config.rs:20-21` defaults `DEFAULT_SYSTEM_PORT: i16 = -1` (disabled). The blast
radius of this change is therefore one server, in deployments that have explicitly set
`DYN_SYSTEM_PORT`. It is also why recipe 07 had to set that variable to exercise anything
at all.
One out-of-scope observation, offered as context rather than as a finding against this PR.
The same raw-`TcpListener` pattern that motivated the fix is still present at nine other
`axum::serve` call sites — `lib/kv-router/src/services/{selection/service.rs:513,
selection/server.rs:360, slot_tracker/mod.rs:83, shared_cache/mod.rs:43,
indexer/mod.rs:243}`, `lib/llm/src/http/service/service_v2.rs:{877,930}`, and
`lib/llm/src/block_manager/metrics_kvbm.rs:197`. If CRIU listener loss is a real
production concern, the frontend HTTP service in particular would suffer the same way. The
tree contains exactly one custom `Listener` impl, the new one
(`system_status_server.rs:322`), so nothing else already covers those sites.
Upstream state, recorded as context only and never treated as local evidence:
PR #12059 is `OPEN`, `reviewDecision: REVIEW_REQUIRED`, 1 changed file, +250/−35, with
**0** `CHANGES_REQUESTED` reviews. One check fails (`lychee`, a link checker) and several
were pending at the time of reading.
## Recipe 07 — aggregate smoke on the in-sandbox A100
Four attempts. The first three failed for reasons that are all environmental or
recipe-staleness, and all three are recorded with `exit=0` wrappers, so I name them
explicitly.
1. **Stale flag.** The recipe's `--store-kv file` no longer exists in this checkout; the
worker exited with `unrecognized arguments: --store-kv file`. The current option is
`--discovery-backend` (`components/src/dynamo/common/configuration/groups/runtime_args.py:172`,
`components/src/dynamo/frontend/frontend_args.py:393`; env `DYN_DISCOVERY_BACKEND`,
choices `kubernetes|etcd|file|mem`). Substituted `--discovery-backend file`.
2. **Self-inflicted kill.** My own cleanup `pkill -f "dynamo.vllm"` matched the wrapper
shell whose command line contained that literal, killing the run (exit 144). Fixed by
never colocating the pattern with the launch, and by verifying a clean slate
(`ps aux | grep -c "[d]ynamo\."` = 0, GPU at 1 MiB) before relaunching.
3. **Stale gate literal, then the proxy.** The recipe waits for `"endpoint_id"` in
`/health`; this build emits `"instance_id"` inside an `"instances":[...]` array, so the
gate never fired even with a healthy worker. Re-gated on the array and added a second
gate on `/v1/models`. That exposed the real blocker: the chat completion returned
HTTP 404 with an empty model list, and the frontend log read
`Model materialization failed ... fetching http://10.200.0.2:9090/v1/metadata/dynamo/backend/generate/qwen_qwen3-0_6b/_base/config.json returned status 403 Forbidden`.
The 403 did not come from the worker. The status router
(`system_status_server.rs:174-266`) has no 403 path at all — its handlers return only
200/404/500 and its fallback logs `[fallback handler] called`, and
`grep -c "fallback handler"` on the worker log was `0`, so the request never arrived.
The sandbox routes egress through a persistent internal proxy address with
`no_proxy=127.0.0.1,localhost,::1`; the pod IP `10.200.0.2` was not exempt, so reqwest
sent an intra-pod request to the egress proxy, which refused it. Fixed by appending the
pod IP to `no_proxy`/`NO_PROXY`. This is a sandbox networking property with no bearing
on the PR.
The fourth attempt (18:37:26) is clean. The worker ran with `DYN_SYSTEM_PORT=9090`
(mandatory — without a non-negative port the gate at `config.rs:343` never starts the
server and none of the changed code would execute), model `Qwen/Qwen3-0.6B` with
`--load-format dummy`, TP 1, on the single A100. The worker log confirms the server came
up on the new listener:
```
INFO dynamo_runtime::system_status_server: [spawn_system_status_server] binding to: 0.0.0.0:9090
INFO dynamo_runtime::system_status_server: [spawn_system_status_server] system status server bound to: 0.0.0.0:9090
INFO dynamo_runtime::distributed: System status server started successfully on 0.0.0.0:9090
```
Startup: endpoints registered 60s after launch, model card published in the same window,
`/v1/models` listing `Qwen/Qwen3-0.6B`. Section 2: the chat completion through the
frontend returned **HTTP 200** with a well-formed `chatcmpl-...` body and
`finish_reason: "length"` at `max_tokens: 5` (the text is gibberish, as expected from
`--load-format dummy` weights — that is correct behavior, not a defect). Direct hits on
the changed code path: `GET :9090/health` → **HTTP 200** `{"status":"ready",...}`,
`GET :9090/live` → **HTTP 200**, and a repeat `/health` → **HTTP 200**, showing the same
listener accepting repeatedly rather than serving once.
The strongest signal is indirect. Model-card materialization in Dynamo is a *pull*: the
frontend fetches `/v1/metadata/...` from the worker's own system status server. With the
proxy exemption in place, `frontend materialization failures: 0` — meaning the
`RebindingTcpListener` was on the critical path of the deployment coming up, not merely
poked by a side curl. The error path stayed quiet, as it should have with nothing breaking
the socket: `grep -c "System status listener stopped accepting"` → `0`.
Section 3 teardown: `SIGTERM-result=EXITED worker gone after 8s (no hang in the accept
loop)`, `frontend-wait-status=0`, `worker-wait-status=0`, `No errors` in the worker log,
the status port refusing connections afterwards, and GPU memory back to `0, 1 MiB`. The
new accept loop with its 1-second backoff does not delay or block shutdown — the empirical
counterpart to Claim 2.
## What was and was not demonstrated
Demonstrated locally: the change compiles clean under check and clippy; the full
`dynamo-runtime` lib suite passes with the change in place (578 passed, 0 failed); the
three new tests execute in the real crate binary, and the one that matters —
`test_rebinding_listener_serves_again_after_listener_shutdown` — took the real path, not
its platform early return; the new listener carries a live worker's health, metadata, and
model-card traffic on a GPU; and it shuts down promptly on SIGTERM.
Not demonstrated, and honestly outside what this sandbox can do: an actual CRIU
checkpoint/restore. The test's `shutdown(2)` on a listening socket is a stand-in for it,
and a good one for the accept-side failure, but it does not reproduce the network-namespace
change that the code comment cites as a second motivation for rebinding. The claim that
rebinding "lands the server in the restored network namespace" is reasoned, not verified
here. Likewise, the failure mode has no repeated-failure test: nothing exercises a rebind
that itself fails and retries under backoff, so the `bind_system_status_listener` error arm
at `system_status_server.rs:331-338` is unexecuted by the suite.
Every recipe reached a green final state; the environment failures (swagger download, the
egress proxy) were resolved rather than worked around, and none of them are attributable to
the change.
## Verdict: pass |
Contributor
Author
Assessment: soundFindingsNo blocking findings remain. Evidence audit
|
Contributor
Author
|
CI result: Observed during bounded CI watch for #12059. Failing checks/statuses observed:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review. WalkthroughThe system status server retains its resolved address and retries failed listener binds with a one-second backoff. It classifies accept errors and serves with graceful cancellation. Tests cover listener recovery, address contention, error classification, shutdown during backoff, and descriptor exhaustion. ChangesSystem status server
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The listener retains its chosen address and can retry recovery without the previously duplicated bind errors. No identified issue blocks merging after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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
🧹 Nitpick comments (1)
lib/runtime/src/system_status_server.rs (1)
268-271: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFallback logs at
infoon every 404 — looks like a debug artifact.
[fallback handler] calledfires at info level for every unmatched request with no path context, which is noisy in production. Considerdebugand including the URI.♻️ Suggested tweak
- .fallback(|| async { - tracing::info!("[fallback handler] called"); - (StatusCode::NOT_FOUND, "Route not found").into_response() - }) + .fallback(|uri: axum::http::Uri| async move { + tracing::debug!(%uri, "system status route not found"); + (StatusCode::NOT_FOUND, "Route not found").into_response() + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/runtime/src/system_status_server.rs` around lines 268 - 271, Update the fallback handler’s tracing call to use debug-level logging instead of info, and include the unmatched request URI in the message. Preserve the existing NOT_FOUND response returned by the fallback closure.
🤖 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/system_status_server.rs`:
- Around line 294-303: Update serve_system_status_once to use a serving path
that surfaces unrecoverable TcpListener::accept failures instead of relying on
axum::serve’s retry loop. Preserve graceful shutdown via cancel_token, and
ensure listener errors propagate from this function so
supervise_rebinding_server can perform rebinding.
---
Nitpick comments:
In `@lib/runtime/src/system_status_server.rs`:
- Around line 268-271: Update the fallback handler’s tracing call to use
debug-level logging instead of info, and include the unmatched request URI in
the message. Preserve the existing NOT_FOUND response returned by the fallback
closure.
🪄 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: d39fbfb5-1814-4402-8fa6-9e9d60f6f4a3
📒 Files selected for processing (1)
lib/runtime/src/system_status_server.rs
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
|
🔄 Datadog auto-retried 8 jobs - 8 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: 0e64178 | Docs | Datadog PR Page | Give us feedback! |
glamr-agent
temporarily deployed
to
external_collaborator
July 24, 2026 20:43 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
Repair round 1 pushed commit Addressed:
Validation run:
Note: I could not fetch the previous failed GitHub Actions logs because GitHub returned |
glamr-agent
temporarily deployed
to
external_collaborator
July 24, 2026 20:53 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
Repair round 2 pushed commit Addressed:
Validation run from
|
MatejKosec
changed the title
MatejKosec
temporarily deployed
to
external_collaborator
July 24, 2026 22:39 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
MatejKosec
temporarily deployed
to
external_collaborator
July 28, 2026 21:02 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
MatejKosec
force-pushed
the
fix/system-health-server-rebind-after-criu--95e8e92a4325
branch
from
July 28, 2026 21:34
d6297d3 to
4da3e75
Compare
MatejKosec
temporarily deployed
to
external_collaborator
July 28, 2026 21:34 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
nursery: Removed both redundant helper summaries in |
Contributor
Author
|
nursery: Extended |
Contributor
Author
|
nursery: Added |
Contributor
Author
|
nursery: Changed nothing for the docstring percentage warning in CodeRabbit’s summary. The changed helpers are private. Their useful comments explain the Axum constraint and raw errno handling. Adding summaries just to raise the percentage would conflict with the specific review findings asking to remove redundant helper comments. |
Contributor
Author
|
nursery: @coderabbitai full review |
Contributor
Author
|
nursery: /devin review |
Contributor
|
✅ Action performedFull review finished. |
Contributor
Author
|
nursery: @dynamo-ops please run full CI for f11a184 |
There was a problem hiding this comment.
Previously reported defects still present:
- Original discussion: The EMFILE entry is still present in
test_resource_exhaustion_does_not_look_like_a_dead_listener. In the selected Linux Rust test stages,test_rebinding_listener_preserves_backlog_during_resource_exhaustionalready drives real EMFILE throughRebindingTcpListener::acceptand fails if EMFILE is handled as either the rebind path or the no-backoff connection-error path, so the classifier parameter remains redundant there while ENFILE and ENOBUFS retain distinct raw-errno coverage.
Signed-off-by: GLAMR <svc-glamr@nvidia.com>
Contributor
Author
|
nursery: Removed the duplicate |
Contributor
Author
|
nursery: @coderabbitai full review |
Contributor
Author
|
nursery: /devin review |
Contributor
Author
|
nursery: @dynamo-ops please run full CI for c56d94d |
Contributor
|
✅ Action performedFull review finished. |
dmitry-tokarev-nv
dismissed
their stale review
October 5, 2026 21:13
Withdrawing my approval because new commits landed after it. I will review the new commits before I approve again.
jthomson04
approved these changes
Oct 5, 2026
jthomson04
left a comment
jthomson04
left a comment
Contributor
There was a problem hiding this comment.
Reviewed c56d94d. All four prior comments are addressed, and I found no additional issues. Source review only; I did not run tests or inspect CI.
Contributor
|
/ok to test c56d94d |
MatejKosec
enabled auto-merge (squash)
October 5, 2026 23:08
Signed-off-by: GLAMR <svc-glamr@nvidia.com>
Contributor
Author
|
nursery: @dynamo-ops please run full CI for c8850ef |
Contributor
|
/ok to test c8850ef |
Contributor
External PR triageBlocker: approved and otherwise green, but BLOCKED by infrastructure. @jthomson04 approved head Next action, owned by @jthomson04 (or @dynamo-ops): re-run the failed jobs of run 37389982152. If the gates turn green, merge. Automated external-PR triage. This comment is edited in place on each run. Written by Devin |
Signed-off-by: GLAMR <svc-glamr@nvidia.com>
Contributor
Author
|
nursery: @dynamo-ops please run full CI for 9a4d2c3 |
Contributor
|
/ok to test 9a4d2c3 |
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
6 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.
Overview:
Summary
After process restore, the system status server can retain a listener descriptor that no longer accepts connections. Axum normally retries the same descriptor indefinitely, leaving status endpoints unavailable while the runtime continues. This change recovers by rebinding the listener.
Details:
Where should the reviewer start?
lib/runtime/src/system_status_server.rs:RebindingTcpListener::acceptandtest_rebinding_listener_preserves_backlog_during_resource_exhaustion.Related Issues
Refs #10665 — system health server does not rebind after CRIU restore.
Validation
git diff --checkpassed. No local tests, builds, or benchmarks were run during maintenance.EMFILE. The classifier test separately checksENFILEandENOBUFS.Summary by CodeRabbit