fix(e2e): serve loopback install downloads with a real HTTP server

[rominf]

Fixes #173.

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (No rows referenced this scenario.)

Symptom

lifecycle-windows-http-install failed intermittently on the Windows job: the
installer downloaded the archive from the test's local HTTP server, then failed
on the immediately following .sha256 request to the same server with An error occurred while sending the request — a bare transport error, with no HTTP
status. Re-running passed. It hit several unrelated PRs and reproduces on main.

Root cause

The server the scenario points the installer at was hand-rolled on a raw
TcpListener, so it also owned HTTP framing, socket-mode handling and
path-traversal defence. Two of those were wrong:

• The listener is non-blocking so the accept loop can poll a stop flag. On
  Windows, accept() returns a socket that inherits the listener's non-blocking
  mode (Linux does not, which is why this was Windows-only), so if the request
  bytes had not already landed, the first read() returned WouldBlock; the
  handler propagated that as an error, wrote no response, and dropped the
  socket, resetting the connection.
• The request head was taken from a single read(), so a request split across
  TCP segments parsed as GET / and 404'd a legitimate download.

Both make service depend on when a request arrives rather than whether it
arrives — matching the observed pattern, where the first request has process
setup ahead of it and the .sha256 request, issued the instant the archive
download completes, is far more likely to lose the race.

Change

Rather than patch the hand-rolled server, replace it with tower-http's
ServeDir on axum — the stack the sibling mock server already uses, and one
that owns all three concerns. The server runs on its own runtime thread, since
the scenario step that starts it then blocks on the installer subprocess.

Two cleanups the rewrite made available:

• Both test servers now share one http_server::ServerHandle for the
  bind/serve/graceful-shutdown lifecycle, with two entry points (spawn on the
  caller's runtime, or on a dedicated thread). Each server is left with only
  its routes.
• Test URLs are built from a typed url::Url with join instead of format!,
  so separators and escaping are not each call site's problem. base_url()
  stays a String for the installer, which appends its own /<file> to
  ROCM_CLI_DOWNLOAD_BASE; a unit test pins that it has no trailing slash.

tower-http 0.6 is the version reqwest already pulls in and url is already
in the tree, so the only new crate is http-range-header.

Verification

The hand-rolled parser's unit tests are gone with the parser. In their place the
tests drive the real server over real loopback sockets: a byte-for-byte 256 KiB
body, the install sequence (bundle then sidecars), 404, and traversal. These are
library unit tests, so cargo test --workspace runs them natively on the
Windows job, where the fault lived.

What those tests do and do not establish, since it is easy to over-read them:

• The traversal test sends un-normalised targets (/../secret.txt,
  /..%2Fsecret.txt, /%2e%2e/secret.txt, /..\secret.txt,
  /served/../secret.txt) over a raw socket, because any URL type applies RFC
  3986 dot-segment removal before the request goes out — a client can only ever
  ask for /secret.txt, which 404s whether or not the server defends anything.
  Confirmed by probe that all five reach the router verbatim, so the 404s are
  refusals; a companion raw request for a served file returns 200, so a 404
  cannot be a request the server failed to parse.
• The back-to-back download test injects no timing pressure, so it would not
  have caught the Windows flake — it checks the sequence is served, nothing
  more. What rules out that bug class is the move to async I/O, which never
  surfaces WouldBlock to application code at all.

Ten consecutive green Windows E2E runs of all 13 Windows - install-lifecycle
scenarios on the self-hosted Strix Halo runner: five against the first fix, five
against this rewrite. Given the flake rate, that is the evidence a single green
run cannot provide.

Locally on Linux: cargo test -p e2e-cucumber --lib (57 passed), cargo clippy -p e2e-cucumber --all-targets -- -D warnings, cargo fmt --all --check,
cargo xtask tpn --check, cargo xtask manifest --check.

Risk

Low, and confined to test infrastructure — no shipped code changes. The blast
radius beyond the download server is the mock inference server, which every
scenario uses: its public API is unchanged and its lifecycle semantics are
preserved (it still shuts down on drop), with the full E2E suite covering it.

[volen-silo]

Reviewed the full diff plus the branch history. No blocking defects — the root cause is correctly diagnosed and genuinely eliminated by construction, and the dependency/manifest/licence bookkeeping is right. Three items below are all about claims that are stronger than what the code actually establishes, which matters more than usual in a public repo.

Verified locally in a clean worktree at 718c2c1: cargo fmt --all --check, cargo test -p e2e-cucumber --lib (56 passed, including the 4 new loopback_http and 3 new http_server tests), cargo clippy -p e2e-cucumber --all-targets -- -D warnings, cargo xtask manifest --check — all pass. cargo xtask tpn --check could not run here (cargo-about not installed); the http-range-header 0.4.2 notice was checked by inspection instead and matches the vendored crate.

---

1. The traversal test cannot fail — Url::join strips the .. before the request is sent

tests/e2e-cucumber/src/loopback_http.rs:133-147

get() builds the request as server.url().join(path) (:64-73). Per RFC 3986 dot-segment removal, joining "../secret.txt" onto a base whose path is / yields /secret.txt — the traversal never leaves the process. The server only ever receives GET /secret.txt, which 404s because that file isn't in the served directory. The test is therefore equivalent in effect to missing_file_is_a_404 at :132-138, and would pass identically against a server with zero traversal defence.

The assertion compounds it: assert_ne!(body, "secret") never checks the status, so an empty 404 body passes.

To be clear, ServeDir's defence is real — un-normalised raw targets (/../secret.txt, /..%2Fsecret.txt, /%2e%2e/secret.txt, /..\secret.txt, /served/../secret.txt) all 404 against a live axum + ServeDir server. The property holds; the test just can't observe it. Either send the request over a raw socket with an un-normalised target and assert NOT_FOUND, or drop the test and rely on tower-http's own suite.

2. serves_back_to_back_downloads is labelled a regression test but wouldn't have caught the regression

tests/e2e-cucumber/src/loopback_http.rs:99-122, comment at :101-105

Rebuilding the pre-fix hand-rolled server and running this exact 4-request sequence against it: 50/50 pass on Linux naturally (Linux accept() doesn't inherit O_NONBLOCK, so the precondition never occurs), and only ~0.7% failures (4/600) even with the accepted socket forced non-blocking to emulate Windows. The test injects no timing pressure, so on Windows it would have passed the overwhelming majority of runs — the same way the original flake did.

Worth noting that 6c5e4be had exactly the right test for this — serves_back_to_back_requests_on_a_non_blocking_accepted_socket, which called set_nonblocking(true) on the accepted socket to force the condition rather than hope for it, plus retries_past_would_block_instead_of_failing_the_request and gives_up_on_a_socket_that_never_becomes_ready. 7ebf428 removed all three (grep -rnE 'WouldBlock|set_nonblocking' tests/e2e-cucumber/ returns nothing at HEAD).

The stronger argument is the one the PR already has: async I/O never surfaces WouldBlock to application code, so the bug class is eliminated by construction — a better guarantee than any test. Suggest leaning on that and softening the comment to describe what the test actually is (a behavioural check of the download sequence).

3. The module doc asserts a path-traversal defect that never existed

tests/e2e-cucumber/src/loopback_http.rs:11-18 — "Each of those was a real defect here."

Framing and socket-mode were real defects (fixed in ad12ac73 and 6c5e4be). Traversal wasn't: the removed safe_join (root.join → canonicalize() both sides → starts_with) is byte-identical from d17fc0c through 6c5e4be — it was never touched for a bug fix. Same overstatement in 7ebf428's commit message.

Related tradeoff worth a line while you're in there: ServeDir rejects .., drive prefixes and root components but never canonicalises, so unlike safe_join it follows symlinks out of the served root (confirmed: a symlink inside the root returns the outside file with 200). Irrelevant in practice — the root is a test-created TempDir with no attacker-controlled symlinks — but it means ServeDir isn't strictly superior on the axis the doc singles out.

---

Checked and clean

• Root cause. Confirmed against the removed code: listener set non-blocking, accepted stream's mode never cleared, read_request_head's reader.read(...)? propagated WouldBlock as Err, and serve's let _ = handle_conn(...) dropped the socket without writing a response. The fix removes the mechanism rather than moving the race. Uncredited bonus: the old loop ran handle_conn inline, so a slow client blocked every subsequent accept() — also fixed.
• Shutdown. The old MockServer had no Drop impl, but the oneshot::Sender field's drop glue calls complete(), waking the receiver exactly as an explicit send(()) does — "it still shuts down on drop" holds.
• No nested-runtime hazard. The harness is #[tokio::main] multi-thread; spawn_on_own_thread's new_current_thread runtime is independent, so blocking a caller worker on addr_rx.recv() can't deadlock.
• URL changes are byte-identical. join("v1") matches the old format!, and base_url() still has no trailing slash. Traced ROCM_CLI_DOWNLOAD_BASE through install.sh:339 and install.ps1:599-600 — no separator change. The installers issue plain GETs with no Range/If-None-Match/HEAD, so ServeDir's extra capabilities are never exercised.
• Dependencies. Cargo.lock adds exactly one package (http-range-header 0.4.2); tower-http 0.6.11 and url 2.5.8 were already in the tree at those versions and already in MANIFEST.md. Alphabetical placement correct, licence MIT confirmed from the vendored crate, dev-dep doesn't leak into shipped binaries.

Minor, take or leave: lifecycle_steps.rs:1222-1228's pub mod http shim has a single consumer (:770) and could be a direct use; LoopbackServer::url() (loopback_http.rs:54-57) is only called from that file's own #[cfg(test)] module.

Couldn't verify: Windows behaviour directly (no Windows host — the accept() inheritance mechanism is reasoned from the removed code plus a forced-condition emulation on Linux), and the ten green Windows E2E runs.

[rominf]

Thanks — all three land, and each was a claim stronger than the code. Addressed in ea293a5, rebased onto main.

1. Traversal test couldn't fail. Confirmed, and worse than "equivalent to the 404 test": assert_ne! on the body meant an empty 404 passed regardless. It now sends /../secret.txt, /..%2Fsecret.txt, /%2e%2e/secret.txt, /..\secret.txt and /served/../secret.txt over a raw socket and asserts 404. I probed that all five reach the router verbatim (echoing uri.path() back from a fallback handler), so the 404s are refusals rather than an unparsed request — and added a raw request for a served file returning 200 as the control, since a raw-socket test that silently stopped reaching the server would otherwise look like a pass.

2. Back-to-back test isn't a regression test. Agreed, and your 4/600 number makes the case better than the code did. Comment now says what it is — a check that the install sequence is served — and points at the real guarantee: async I/O never surfaces WouldBlock to application code, so the class is gone by construction rather than watched for by a test.

3. Module doc asserted a traversal defect that never existed. Correct — safe_join was byte-identical across that range and never touched for a bug. The doc now claims only framing and socket-mode as real defects here, keeps traversal as "a third thing a test server shouldn't be maintaining", and states the tradeoff you flagged: ServeDir doesn't canonicalise, so unlike safe_join it follows symlinks out of the root — irrelevant for a test-created TempDir, but not strictly superior on that axis.

Minors both taken: the http shim is gone in favour of a direct use, and LoopbackServer::url() is gone (tests go through the handle).

The PR description's Verification section overstated the same way, so it now spells out what these tests do and don't establish.

On the red E2E tests (Strix Halo, Ubuntu): unrelated to this PR — 0 unexpected failure(s), failing on 3 stale EAI-7423 XPASS rows. #182 already handles that on main, so the rebase picks it up.

Not verified here: Windows behaviour directly, same as your review — no Windows host on this end either.