fix(core,ci): bound local-service HTTP reads; raise strix-ubuntu E2E job timeout

[juhovainio]

Summary

Three local-service HTTP call sites read responses with read_tcp_stream_to_string, which loops TcpStream::read() until EOF with no overall deadline — a peer that trickles data or keeps the connection open can stall the read forever, even though each individual read() respects its socket timeout.

This was the cause of a ~400s stall we couldn't previously explain in the Strix Halo Ubuntu E2E lane: scenario teardown calls an unload request outside any visible cucumber step, so a hung request against a stuck lemonade server looked like an unattributed gap in the logs.

While fixing this I also found a latent truncation bug in the response-completeness check.

Fixes

• Bound the reads: switched all three call sites to the existing read_http_response_bounded helper (made pub), and deleted the now-dead read_tcp_stream_to_string.
• Stop waiting on keep-alive peers: read_http_response_bounded returns as soon as the response is complete by its own framing, instead of waiting for socket close — fixes readiness probes falsely timing out against servers that answer correctly but don't close the connection. This flips wait_for_service_http_ready's verdict from Unreachable to Serving against such a peer, which is user-observable via rocm services list; covered by @id:serve-readiness-contract (serve-12, @requires-gpu @merge-queue — exercised on the merge-queue run of the GPU lanes) and @id:networking-native-http-chat-round-trip (networking-02, mock lane, every PR).
• Fixed a truncation bug in http_response_is_complete: it compared decoded (lossy-UTF-8) body length against Content-Length, which could under-count a body ending mid multi-byte character and report completeness one read early. Now compares raw byte lengths.

Not in this PR: review turned up a per-runner timeout disparity on e2e-gpu-strix-ubuntu (strix-halo-ubuntu-2 hits its 35min cap far more often than strix-halo-ubuntu) that I'd originally folded in as a straight 35→90min raise. Per review, that's a separate problem best addressed once #355 (which targets that runner's slowness directly) lands and the lane can be re-measured — dropped from this PR to keep scope to the HTTP-read fix. e2e-gpu-strix-windows's matching 35min cap is the same follow-up, not addressed here either.

Test plan

• cargo check -p rocm-core -p rocm
• cargo clippy --workspace --all-targets -- -D warnings
• cargo test -p rocm-core -p rocm --all-targets (rocm-core has no separate bin-test target from --lib, so --all-targets is the command that actually exercises both rocm-core's 325 lib tests and rocm's 553 bin tests — -p rocm --lib alone silently no-ops since rocm is a bin-only crate)
• New regression tests:
  • lemonade_stop_unload_is_bounded_by_the_request_timeout
  • serve_readiness_ready_verdict_does_not_wait_for_the_peer_to_close
  • http_response_is_complete_does_not_miscount_a_split_multibyte_char
• Confirmed on strix-halo-ubuntu-2 (the original stall site) via a temporary runner pin, reverted before merge

[tomastola]

Review — head 06e5d7b

The fix is correct and the root-cause claim holds up — I tried to falsify it and couldn't. The gaps are around it: no regression test (which AGENTS.md makes non-optional), a dead helper left exported, and an undisclosed second behaviour change.

What I verified

Checked out the branch and ran the gate:

• cargo check -p rocm-core -p rocm --all-targets — clean
• cargo clippy -p rocm-core -p rocm --all-targets -- -D warnings — clean

I was initially skeptical of the root-cause story, because unload_lemonade_service_model (apps/rocm/src/main.rs:15340) passes Duration::from_secs(5), and read_to_string does not retry WouldBlock/TimedOut — so a silent peer was already bounded at 5s and could never produce a ~400s gap. To settle it I stood up a throwaway harness driving both readers against three peer behaviours:

peer behaviour	old read_tcp_stream_to_string	new read_http_response_bounded
accepts, never writes, holds socket	Err after 5.41s	Err after 5.13s
complete Content-Length response, then holds socket	Err after 5.09s (answer discarded)	Ok after 11ms
dribbles 1 byte / 500ms, never frames, never closes	Ok after 100.5s	Err after 5.00s

The dribble row confirms the thesis and refutes my objection: the old reader ran 20× past its 5s budget and then returned Ok with 200 bytes of garbage. Scale the dribble out and you get the teardown gap as described, followed by a confusing downstream parse failure rather than a timeout. Root cause accepted.

---

1. No regression test — AGENTS.md requires one, and the harness is already sitting there

AGENTS.md §3 is unambiguous: "Every bug fix ships with a regression test in the same change: test fails before fix and passes after fix." This PR adds none.

This isn't a case where testing is awkward. apps/rocm/src/main.rs:20190 already contains a socket test for the exact function in the reported stall — it binds TcpListener::bind(("127.0.0.1", 0)), spawns a server thread, and calls unload_lemonade_service_model(&record). Turning that into a regression test is a copy with a dribbling server and an elapsed assertion, modelled on the one already at crates/rocm-core/src/lib.rs:8417:

// server thread: read the request, then write one byte every 200ms, never framing, never closing
let started = Instant::now();
assert!(unload_lemonade_service_model(&record).is_err());
assert!(
    started.elapsed() < Duration::from_secs(8),
    "the unload must honor its 5s budget, not a multiple of it: took {:?}",
    started.elapsed()
);

On main that fails (~100s in my harness); on this branch it passes at ~5s. Please add it for at least http_post_local_service_json; the other two call sites can share the pattern.

2. read_tcp_stream_to_string now has zero callers and is still pub

$ grep -rn 'read_tcp_stream_to_string' --include='*.rs' .
crates/rocm-core/src/lib.rs:1012:pub fn read_tcp_stream_to_string(...)

This PR removed the last three call sites. The function that caused the bug is now dead code that survives clippy -D warnings purely because it's pub in a lib crate (confirmed — clippy passes). Leaving it exported means the next person writing a local HTTP call reaches for the obvious-sounding name and reintroduces this exact stall, with no lint to stop them. Suggest deleting it here. If something out-of-tree depends on it, say so and add a #[deprecated] pointing at read_http_response_bounded instead.

3. The description undersells a second, user-observable behaviour change

The PR frames this purely as bounding a timeout. Row 2 of the table shows it is also a correctness fix in the other direction: against a peer that answers in full but holds the socket open, the old reader burned the whole budget and threw the answer away; the new one returns in 11ms.

That matters because http_get_local_service is the readiness probe behind wait_for_service_http_ready_with_progress (apps/rocm/src/main.rs:18815, 750ms per attempt). Against a keep-alive peer this flips the verdict from Unreachable → Listing/Serving, which is what rocm services list prints. AGENTS.md §3 asks for a Gherkin scenario or a named existing @id: when user-observable behaviour changes; tests/e2e-cucumber/features/model_serving.feature is the natural home, and there is currently no unload/stop scenario at all.

To be fair about the trigger: this only fires against a server that ignores Connection: close. Whether lemonade or vLLM actually do that, I did not verify — but the helper's own doc comment (crates/rocm-core/src/lib.rs:1022-1026) says an intervening proxy can, and that's the stated reason the helper exists. Either cover it or state in the PR text why it's unobservable in practice.

4. The temporary CI pin in 06e5d7b is a merge blocker

06e5d7b narrows the Ubuntu E2E lane to runs-on: [self-hosted, linux, strix-halo, native, ubuntu-26.04]. The commit message and the inline comment both say "Revert before merge" — good — but nothing enforces it, and if it lands the lane silently stops covering the other native runner. Worth either dropping it before review sign-off or converting it into a checklist item on the PR so it can't be missed. (Minor: that commit message calls it "the connect-timeout stall" while the PR body describes an unbounded-read stall; the fix here is the latter.)

5. Latent: the framing check measures a lossy string, not bytes

http_response_is_complete (crates/rocm-core/src/lib.rs:1071) runs String::from_utf8_lossy over the partial buffer and compares body.len() against Content-Length. Each incomplete trailing multi-byte sequence becomes a 3-byte U+FFFD, so a partial body can measure longer than it is and satisfy the check early — returning a silently truncated response:

true body len on wire = 10
received  8/10 bytes -> lossy len 10 -> complete? true   <-- stops 2 bytes short
received  9/10 bytes -> lossy len 10 -> complete? true

Reachability, honestly: inflation is at most +2 bytes, so this only fires when a multi-byte character sits within ~1 byte of the end of the body. JSON bodies end in ASCII (}, "}]}), which puts it out of reach for the payloads at all five call sites today. So it's latent, not live, and predates this PR.

Raising it here because this PR triples the helper's blast radius and newly routes chat-completion bodies (apps/rocm/src/providers.rs:611) through it — the payload most likely to carry non-ASCII. The check measures the wrong quantity on principle and is one payload-shape change from silent truncation. Fix is small: do the header/body split and the length comparison on &[u8] (find b"\r\n\r\n", compare response.len() - header_end), reserving the lossy conversion for the final return. Fine to defer to a follow-up — but please don't leave it undocumented.

6. Minor notes

• Deadline spans the connect. let deadline = Instant::now() + timeout sits before connect_tcp_stream(..., timeout) in all three sites, so a slow connect eats the read budget and can yield an immediate timed out reading HTTP response. This matches http_get_text_with_auth (crates/rocm-core/src/lib.rs:540) and the direction set by fix(therock): cap http_get's connect phase at the request timeout #338, so I'd keep it — just noting the readiness probe's budget is only 750ms, so it tightens most where there's least slack. The outer poll loop retries, so it self-heals.
• http_response_is_complete is O(n) per chunk → O(n²) overall, re-allocating and re-scanning the whole accumulated buffer every 4096 bytes. Irrelevant for probe responses; worth a thought only because non-streaming chat completions are now in scope. Cheap mitigation: only re-check once the header terminator has been seen, and track the body offset.
• Design: making an internal helper pub to serve three near-duplicates is the minimal move, and defensible for a bug fix. But http_get_local_service is close to a copy of http_get_text_with_auth, and http_post_local_service_json of http_post_json_with_auth, differing mainly in host/port vs URL and return shape. The reason this bug existed in three places is that the request/response logic lives in three places. Worth a follow-up to collapse them onto the rocm-core helpers so the next transport fix lands once.

---

Checked and found fine

• Deadline arithmetic in the loop: saturating_duration_since plus the zero-check before each read is correct; no underflow, no busy-spin.
• EINTR handling is preserved and well documented (crates/rocm-core/src/lib.rs:1052-1059); the retry can't spin unbounded because the deadline still governs.
• The streaming sibling stream_json_from_local_endpoint (apps/rocm/src/providers.rs:623) is deliberately not converted. Right call — a total deadline would kill long generations, and per-read timeouts are the correct semantic for SSE.
• read_to_string's old UTF-8 validation is replaced by lossy conversion. A behaviour change, but strictly an improvement for these call sites.

Suggested path to approval

1. Add the regression test from finding 1 (required by AGENTS.md).
2. Delete read_tcp_stream_to_string (finding 2).
3. Add a scenario for the readiness-verdict change, or say why none is needed (finding 3).
4. Revert the CI pin before merge (finding 4).
5. Fix finding 5 here or file it — either is fine, but decide explicitly.
6. Update the PR body to describe the keep-alive win alongside the timeout bound; it's the more user-visible half of the change.

[tomastola]

Re-reviewed at 70cc7bd (previous review was against 06e5d7b).

All five items from my last pass are addressed, and I verified each rather than reading the diff. Short version: the code is correct and the tests are real. Everything I have left is PR text and policy, not correctness.

Verified fixed

The three new tests are genuine regression tests. This is the thing worth checking and it holds up. I reverted the production change on top of your branch — re-added read_tcp_stream_to_string, pointed the two main.rs call sites back at it, restored the lossy http_response_is_complete — kept your tests, and ran them:

http_response_is_complete_does_not_miscount_a_split_multibyte_char ... FAILED
  assertion failed: !http_response_is_complete(truncated)

serve_readiness_ready_verdict_does_not_wait_for_the_peer_to_close ... FAILED
  assertion `left == right` failed
    left: Unreachable
   right: Serving

lemonade_stop_unload_is_bounded_by_the_request_timeout ... FAILED
  assertion failed: unload_lemonade_service_model(&record).is_err()

All three pass on 70cc7bd. Fails-before/passes-after is satisfied for each.

The Unreachable → Serving flip is also the clearest statement of the second behavior change: pre-fix, a keep-alive engine that answered correctly was reported unreachable.

Timing stability. The two socket tests are wall-clock assertions, so I ran them three times: 5.01s / 5.00s / 5.00s. Deadline-dominated, not race-dominated, with ~3s of headroom against the 8s assert. Fine for a loaded runner.

read_tcp_stream_to_string is gone, not just unused — only two comment references remain. And I checked no equivalent unbounded socket read survives elsewhere: the read_to_string hits in engines/vllm and engines/lemonade are stdin in read_request, and therock.rs reads through ureq, which has its own timeouts and had its connect phase capped in #338.

The lossy fix is correct. Finding the header terminator on raw bytes and keeping body as &[u8] fixes it at the root rather than special-casing. Decoding only the header slice is sound — even if a server sent non-ASCII header bytes, U+FFFD inflation can't affect a line/colon split. This is a better fix than the one I suggested.

Incidentally it's also faster: the old version allocated a lossy String over the whole buffer on every loop iteration. The O(n²) rescan I mentioned last time is now a cheap windows(4) scan with no allocation, so that note is discharged.

The runner pin is fully reverted. Net CI effect versus base is the timeout and its comment, nothing else.

Clean: cargo clippy --workspace --all-targets -- -D warnings passes; rocm-core 325 pass, rocm --bins 553 pass; DCO sign-off present on all four commits.

Please fix before merge

1. The test plan's command doesn't run the tests it claims. cargo test -p rocm --lib errors outright:

error: no library targets found in package `rocm`

apps/rocm is a bin-only crate. In the combined form you listed, cargo test -p rocm-core -p rocm --lib, cargo doesn't error — it just silently selects the one package that has a lib target and runs rocm-core's 325 tests. Your two new main.rs regression tests never execute under that command, so the "553 passing in rocm" line can't have come from it.

They do pass — I confirmed under --bins — so this is a PR-text fix, not a code problem. But it's worth correcting, because as written the test plan reads as verification of exactly the two tests it skips. --all-targets is what AGENTS.md §12 asks for anyway.

2. Name the Gherkin scenario, or justify its absence (AGENTS.md §3). The readiness verdict flipping from unreachable to serving is user-observable in services list, and §3 is explicit that "a unit test asserting the internal helper does NOT discharge this."

I don't think you need to write a new scenario — coverage looks like it already exists:

• @id:serve-readiness-contract (serve-12, "A service reported ready can immediately serve inference") is precisely this contract
• @id:networking-02 covers the chat round-trip over a local endpoint, which is the providers.rs call site you changed

§3 asks you to name the @id and say the change makes it pass, rather than rely on it silently. Both are @requires-gpu-class lanes, so per the same section, name the lane that will exercise them.

Worth settling, your call

3. e2e-gpu-strix-windows has the identical bug you just fixed. It still sits at timeout-minutes: 35 carrying the same comment, word for word:

# 35min: see e2e-gpu — one collapsed job runs all serves + per-scenario
# install sdk; the cap must exceed the run so the job writes platform.json.

Same collapsed-suite design, same per-scenario install sdk. Your diagnosis — this lane copied e2e-gpu's design but not its cap — applies verbatim. Every other GPU lane is at 90. Either raise it too, or add a line saying why Windows won't hit a cold-cache download of the same size.

4. Scope. AGENTS.md §11 asks that each PR stay one logical change, and the PR body describes the CI timeout as "a separate CI timeout ... a false-positive flake" with an unrelated root cause. By your own framing that's a second change riding along. It's small, well-explained, and shares an investigation, so I'd accept it — but if the maintainer wants it split, the body makes the case for them.

Minor

• drop(server) at the end of the readiness test reads like cleanup but is a no-op: dropping a JoinHandle detaches. The listener thread loops on accept() and never exits. Harmless in a test binary, but the line suggests a teardown that isn't happening — worth deleting or replacing with a comment.
• If connect_tcp_stream consumes the entire timeout, remaining.is_zero() trips immediately and the caller gets "timed out reading HTTP response" without a read ever being attempted. Bounding the whole call by timeout is the right semantic and it's consistent across all three sites — just a slightly misleading message on that edge.

On the CI cap

I can't verify the SSH observation of the multi-GB therock-dist pull, so I'm taking that on trust; the reasoning and the comment are consistent and the "matches e2e-gpu" claim checks out. One tradeoff worth naming: the run step has no inner timeout, so 90min is the only bound and a genuine hang now burns 90 minutes of a single self-hosted Strix runner instead of 35. This PR removes the hang that motivated the change, so the risk is lower than it was — and e2e-gpu already runs this way — but a step-level timeout-minutes on "Run E2E tests" would let a real hang fail fast without capping legitimate long runs.

[rominf]

I reviewed this at 70cc7bd as well. Tomas has already covered the code correctness thoroughly and independently — I re-ran the framing predicate as a differential fuzz against main (3M HTTP-shaped inputs plus an exhaustive prefix sweep) and found 26 fixes, zero regressions, so I'll second that conclusion rather than repeat it. The core fix is right, and it discharges debt that was raised against these exact call sites back in #150 and never actioned.

I have one thing that I don't think has been said yet, and it changes what I'd do with the CI half of this PR.

The 90min raise is treating a per-runner problem as a suite-sizing problem

I pulled every E2E tests (Strix Halo, Ubuntu) job from the last 100 workflow runs (81 with usable timings, 2026-09-02 → 09-08) and split by runner_name:

runner	jobs	hit the 35min cap	median (non-cap)
strix-halo-ubuntu	44	4 (9%)	25.4 min
strix-halo-ubuntu-2	37	33 (89%)	34.2 min

Same suite, same labels, same commit range. One machine finishes in ~25 min with 10 min of headroom; the other essentially never finishes. That's not a cap that was sized wrong at introduction — the healthy runner has never needed more than 34.3 min.

Two things follow that I think matter for the merge decision:

The verification run cited in the PR body doesn't show what it's claimed to show. The body says this was "Confirmed on strix-halo-ubuntu-2 (the original stall site) via a temporary runner pin." That pinned run is 34115456538 at 06e5d7bd — and it was itself cancelled at the cap, 35m19s, on strix-halo-ubuntu-2. Meanwhile the green Strix Ubuntu check on the current head ran on strix-halo-ubuntu (25m19s), i.e. the runner that was never failing. So the lane is green here, but it hasn't actually been exercised on the machine this change is aimed at.

There's decent evidence the real cause is the bug #355 fixes. strix-halo-ubuntu-2 has exactly one successful run in this window: 34156110331. That's on eai-8572-share-therock-cache, at 34a4d78d — the first commit on that branch after b8f9a5e0 ("stop wiping the shared runtime tree on every e2e scenario"). Its two earlier runs on the same branch, at 17e71c91 and b8f9a5e0, both hit the cap. One data point, so I won't oversell it, but the ordering lines up: the per-scenario re-download of the ~3.3 GB lemonade backend is a much better fit for "one runner is 6+ min slower and blows the cap" than a one-off cold cache is.

Given you own both PRs, my suggestion is to land #355 first and re-check whether this lane still needs 90 min on ubuntu-2. If it does, raise the cap with the per-runner numbers as the rationale — that's a defensible "tolerate a degraded runner" argument. What I'd avoid is the current comment, because it records a benign cause ("a cold-cache download can push it past the cap") for what the data says is a machine-specific regression, and that comment is exactly the kind of thing that gets copy-pasted onto the next lane — which is how the 35 it replaces got here in the first place.

Concretely, 90 min converts a fast red into a slow red on a two-machine pool: continue-on-error: true means the lane gates nothing, so the extra 55 min of occupancy buys no signal. Tomas's suggestion of a step-level timeout-minutes on "Run E2E tests" is a good one and would address that independently.

Smaller

The other two are inline. On the points already raised: I agree the Gherkin justification is the one open AGENTS.md §3 item, and if you go the naming route, note @id:serve-readiness-contract is @requires-gpu @merge-queue, so §3 also wants the lane named.

Checked and found fine

• read_tcp_stream_to_string deletion is clean — grepped the tree including *.md/*.toml/*.yml/*.feature; the only two hits are prose inside the new tests' comments.
• The deadline-spans-connect pattern matches http_get_text_with_auth/http_post_json_with_auth and the direction #338 set. I measured loopback connect against a saturated backlog at ~1ms worst case, so the 750ms probe budget isn't meaningfully narrowed.
• I specifically tried to show the 30s non-streaming chat path regressed, and couldn't — for a server that buffers and answers in one write (what a real engine does), the new reader is strictly better: at a 1s generation with a 3s budget, old returned Err at 4.13s, new returned Ok at 1.00s. Only a byte-dribbling peer behaves differently, and that's the bug being fixed.
• Chunked-with-trailers isn't recognized as complete, but that predicate is byte-identical to main's — pre-existing, not worsened here. Worth a follow-up only because the helper is now pub.

[rominf]

The comment attributes the overruns to the collapsed suite plus a cold-cache download, but per-runner data over the last 81 timed jobs on this lane points at the machine instead: strix-halo-ubuntu hit the 35min cap 4/44 times (median 25.4 min), strix-halo-ubuntu-2 hit it 33/37 times (89%).

The part I'd push back on is the sentence "This lane ran at 35min from its introduction despite mirroring e2e-gpu's design" — the healthy runner has never exceeded 34.3 min, so 35 was adequate for it. Also worth knowing: e2e-gpu's own 90 was raised for the large-model @nightly scenario, which this lane doesn't run per-PR, so the "matches e2e-gpu" symmetry is weaker than it reads.

If the cap stays, I'd rewrite the rationale to say what's actually being bought — headroom for a known-slower runner — and link #355/EAI-8572. See the summary for why I'd rather land #355 first and re-measure.

[juhovainio]

Dropped the 90min raise entirely rather than rewriting the rationale — per-runner data makes this a strix-halo-ubuntu-2 problem, and #355 targets that directly. Reverted the hunk back to 35min/original comment in 3af8c79. Re-measuring after #355 lands is tracked there rather than here.

[rominf]

Nit: the body is 35 bytes, not 36 ({"status":"success","message":"ok"}). The margin argument is unaffected — 35 bytes at 300ms is 10.5s against a 5s budget — just the number is off.

[juhovainio]

Fixed in bb90794 — comment now says 35 bytes.

[rominf]

The test's own comment says the unload "must now return an error at (not far past) its 5s budget," but only the upper half is asserted — any error at any time under 8s passes. I confirmed a trivially-early failure would satisfy both asserts (a refused connect returns Err in ~160µs).

That path isn't reachable today, since the listener is bound before it moves into the thread, so this is hardening rather than a live gap. But if something later makes http_post_local_service_json fail fast for an unrelated reason, this test stays green while no longer testing the deadline. A lower bound would pin it:

let elapsed = started.elapsed();
assert!(elapsed >= Duration::from_secs(4), "bounded BY the 5s deadline, not failing early: {elapsed:?}");
assert!(elapsed < Duration::from_secs(8), "{elapsed:?}");

Tomas measured 5.01/5.00/5.00s across three runs, so a 4s floor has plenty of margin.

[juhovainio]

Added the lower bound (>= 4s) in bb90794, matches the assertion you suggested.

[juhovainio]

Addressed in bb90794 and 3af8c79 (built on 70cc7bd):

@rominf — dropped the 90min raise entirely rather than rewriting the rationale. You're right that the per-runner split points at strix-halo-ubuntu-2 specifically, and #355 targets that directly, so the honest move is to let #355 land and re-measure rather than carry a timeout change here at all. Reverted .github/workflows/e2e-selfhosted.yml back to the original 35min/comment (3af8c79). Replied inline on the two test nits — both fixed as suggested.

@tomastola — from your second pass:

1. Test plan command fixed in the PR body: cargo test -p rocm-core -p rocm --all-targets (confirmed this actually runs rocm's bin tests, unlike the --lib form).
2. Gherkin scenario named in the PR body: @id:serve-readiness-contract (serve-12, @requires-gpu @merge-queue) and @id:networking-native-http-chat-round-trip (networking-02, mock lane, every PR) both cover the keep-alive verdict flip.
3. e2e-gpu-strix-windows's matching bug — since the CI timeout change is out of this PR entirely now, that's deferred alongside fix(e2e,lemonade): share the therock/tool cache and fix the shared runtime tree getting wiped per scenario #355 rather than fixed here. Noted in the PR body.
4. Scope — agreed this PR is now just the HTTP-read fix; the CI question moved out per the above.

Minor: left the other three pre-existing drop(server) no-ops alone (same pattern predates this PR) but added a clarifying comment on the one in the new test.

feedback addressed

[tomastola]

Approving. All four items from my second pass are resolved and the PR is now three files and one logical change.

Verified at bb90794: cargo clippy --workspace --all-targets -- -D warnings exits 0; cargo test -p rocm-core → 325 passed; cargo test -p rocm --bins → 553 passed with all three new tests named. The unload deadline test measures 5.01 / 5.00 / 5.00s over three runs, so the new 4s floor and 8s ceiling both have margin. The workflow file is byte-identical to base — git diff 4ed2a03..bb90794 -- .github/workflows/e2e-selfhosted.yml is empty. DCO sign-off on all six commits.

One non-blocking correction to the PR body's coverage claim, and two nits — details below.

---

Re-reviewed at bb90794 (previous passes: 06e5d7b, 70cc7bd).

All four items from my second pass are addressed. The PR is now three files and one logical change. No blockers from me — one correction to the PR body's coverage claim below, which I verified by running the scenario, and one restated minor.

Round-2 items — verified resolved

1. Test plan command. Fixed, and the parenthetical is right. Confirmed both halves on bb90794:

• cargo test -p rocm-core → 325 passed
• cargo test -p rocm --bins → 553 passed; 1 ignored, with all three new tests named in the output.

2. Gherkin scenarios named. Both @ids exist and match what the body claims — @id:serve-readiness-contract (model_serving.feature:122, serve-12, @requires-gpu @merge-queue) and @id:networking-native-http-chat-round-trip (networking.feature:25, networking-02, untagged → mock lane). The mock lane does run on this PR: heavy includes **/*.rs. See the coverage note below.

3. e2e-gpu-strix-windows. Moot — no CI change left in the PR.

4. Scope. Resolved. git diff 4ed2a03..bb90794 -- .github/workflows/e2e-selfhosted.yml is empty: the workflow is byte-identical to base, not just re-set to 35. Net diff is apps/rocm/src/main.rs, apps/rocm/src/providers.rs, crates/rocm-core/src/lib.rs.

Minors. Byte count now correct — the body literal is 35 bytes, so 35 × 300ms = 10.5s against the 5s budget, and the margin argument holds. Lower bound added as suggested; measured 5.01 / 5.00 / 5.00s over three runs, so the 4s floor has ~1s of margin and the 8s ceiling ~3s. drop(server) comment reads accurately.

cargo clippy --workspace --all-targets -- -D warnings exits 0. DCO sign-off present on all six commits.

One correction to the PR body

covered by @id:serve-readiness-contract … and @id:networking-native-http-chat-round-trip

These scenarios traverse the changed code but do not discriminate fixed from unfixed, so they don't evidence the verdict flip. Measured:

$ git checkout 4ed2a03            # base, pre-fix
$ cargo xtask e2e -- -n networking-02
  Scenario: networking-02 - A chat round-trip over a local endpoint uses the native HTTP stack
   ✔ ... 1 scenario (1 passed)

$ git checkout bb90794            # PR head
  ✔ ... 1 scenario (1 passed)

Green on both sides. To be fair to the scenario, it does reach the changed code — I traced rocm chat --prompt → provider_chat → LocalProvider::chat → post_json_to_local_endpoint_body, and separately select_local_chat_service → ready_local_services → managed_service_endpoint_readiness → http_get_local_service. So the readiness verdict logic is exercised.

The reason it can't discriminate: all three call sites send Connection: close, and the mock is axum/hyper, so the peer closes after responding and the old read-to-EOF loop terminated normally. The flip only appears against a peer that ignores Connection: close and holds the socket — which is exactly what the new unit test constructs, and what a stuck lemonade server does in the field. Same reasoning applies to serve-12 against a real lemonade/vLLM server, though that one I can't run, so treat it as reasoning rather than measurement.

I'd just soften the wording — say these scenarios exercise the path rather than that they cover the flip. An E2E scenario that genuinely pins the flip would need a mock that deliberately ignores Connection: close; worth a follow-up if you think it's warranted, not something I'd hold this PR for. The unit test already pins it: reverting the production change under it flips Serving back to Unreachable, which I verified last pass.

Restated minor (not addressed, still not blocking)

If connect_tcp_stream consumes the whole timeout, the deadline is already expired when read_http_response_bounded runs, and the first loop pass bails timed out reading HTTP response — accurate about the budget, misleading about the phase, since nothing was read. Behaviour is correct either way. Given the base commit is #338 ("cap http_get's connect phase at the request timeout"), phase-distinct messaging might be worth a small follow-up.

Nit

bb90794's subject is test(core): but the change is in apps/rocm/src/main.rs, not crates/rocm-core.