[rominf]

Fixes #661.

What broke. Since #546 (ee3131be), the required E2E tests job fails chat-06 (@id:chat-tool-definitions-accepted), on main and on every PR. The error is GET …/v1/models failed after 17.1s … client timeout, against a 10s budget.

Why.

• cucumber polls every in-flight scenario from a single task, and most steps run rocm synchronously. While one scenario's command runs, no other scenario is polled.
• The chat round trip was timed on that thread. Its timeout kept running while nothing read the mock's reply, so a reply sent in milliseconds became a client timeout. That is why the failure reports more time than the budget.
• fix(comfyui): reinstall replaces only ComfyUI's code, never the user's files #546's nine reinstall scenarios made these stalls much longer. Each one's first rocm call runs the CLI's startup update check against the planted runtime: a real uv download, a managed-Python setup and a package-index query, 10–15s each.
• On a 4-CPU local run, the longest stall went from ~22s at f0d4c17b to ~106s at ee3131be. In CI's report.json, a pure-assertion step took 17.4s during such a stall.

Changes.

• The chat exchange runs as its own task on the runtime's worker threads (e2e_cucumber::chat_request), so its budget measures the server again. A step failure still carries the exchange's own diagnostics.
  • a_blocked_scenario_thread_does_not_time_out_a_request_the_server_answers polls the request and a blocking step from one task, the way cucumber polls scenarios.
  • Awaiting the exchange inline instead of spawning it fails the test with the same message CI shows: budget 0.5s … failed after 2.0s … client timeout.
• The ComfyUI reinstall scenarios set ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK=1. They don't test updates, and the check made them reach the network. This cuts the longest local stall to ~35s.
  • Without it, automations-04 failed in one of three runs once chat-06 was fixed: its Given plants a 30s autostart claim, which expired during a 37s stall.

Verification.

• Full mock suite (cargo xtask e2e, pinned to 4 CPUs like the hosted runner):
  • ee3131be: 1 of 1 runs failed, on chat-06.
  • With this change: 3 of 3 runs passed, 191 scenarios each, with 0 unexpected failures (the serve-01 and serve-02 xfails are expected).
  • The local runs are WSL, and the docker-backed scenarios did not run.
• Gates:
  • fmt;
  • -p e2e-cucumber --lib (143), --test feature_naming, cargo check --test e2e;
  • clippy -D warnings (workspace all-targets, and -p e2e-cucumber --test e2e);
  • Windows-target clippy for e2e-cucumber.

User-observable behaviour: none. This changes only the test harness. The scenario it repairs is chat-06.

Not fixed here. Synchronous run_rocm* calls inside async steps can still stall the whole suite for up to ~35s, and any step with a time window is exposed to that. Making those helpers async would remove this class of failure; it is a larger change. Other scenarios that plant a runtime also still run the real-network update check, which predates #546.

[jussielo-amd]

🔴 Automated review · pr-review-watcher · 4de2b7b

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — no blocking findings

Full review of the whole change (both commits on fix/e2e-chat06-mock-lane, 3ea98361 + 4de2b7be, against base ee3131be).
Fixes #661 (chat-06 intermittent timeout on the mock lane) — the diff matches the issue's described failure mode and root cause exactly; issue remains open pending merge, as expected.

Blocking

None

Non-blocking

None

Decisions for the author

• Fix moves contention rather than eliminating it (tradeoff) — request_chat_completion now runs the chat exchange via tokio::spawn onto the shared multi-thread runtime's worker pool (confirmed: plain tokio::spawn, not spawn_blocking, and this is the right primitive here since nothing blocks an OS thread). But other scenario steps still run rocm as a synchronous subprocess call on worker threads (per the PR's own "Not fixed here" section), so on a small CPU-pinned runner (CI pins 4 cores) a worker-thread-starved moment could in principle still delay the spawned chat task's scheduling, not just its old polling-thread read. The PR discloses this adjacent class of risk directly ("Synchronous run_rocm* calls inside async steps can still stall the whole suite... any step with a time window is exposed to that") and reports 3/3 clean CI mock-suite runs under the fix. Nothing indicates this PR's own fix is unreliable; flagging only because the disclosed residual risk and this fix's mechanism are the same shape of problem, scoped out deliberately rather than overlooked.
• Two commits under one PR, same investigation (tradeoff) — commit 1 fixes the actual chat-06 timing bug; commit 2 removes a second, independently-discovered stall source (ComfyUI scenarios' startup update check) found while reproducing the same flake. Defensible as one PR since both trace to the same root investigation of #661, though a stricter split-by-mechanism reading could argue for two PRs. Not a defect either way.

Positive signals

• describe_request_error — including its comment explaining it exists because an ambiguous reqwest::Error::Display once cost "a full log archaeology" — was moved verbatim into the new module rather than dropped or rewritten, preserving that institutional knowledge through the refactor.
• The new regression test (a_blocked_scenario_thread_does_not_time_out_a_request_the_server_answers) was verified to actually exercise the failure mechanism (a blocking sibling step racing the chat exchange on a 2-worker multi-thread runtime, timeout well inside the blocking window) rather than passing vacuously.
• ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK is applied per-scenario via Command::env(), not process-global std::env::set_var, so it can't leak into concurrently-running sibling scenarios — the right call in a harness that explicitly runs scenarios concurrently.

Deployment notes

None — test-harness-only change, no production code, no user-observable behavior, confirmed by all three independent passes.

What this covered

Full fan-out: three independent passes — (1) code-correctness, reading chat_request.rs, lib.rs, e2e.rs (full 1369 lines), comfyui_steps.rs, plus callers (chat_steps.rs, mock_server.rs, therock.rs, main.rs, event_loop.rs); (2) AGENTS.md compliance, covering leak scan, DCO/signing, regression-test requirement, Gherkin-scenario applicability, docs sync, architecture guardrails, CI status, and scope; (3) history, tracing the predecessor commit, checking for dropped rationale/comments, overlapping in-flight PRs, and harness-wide async invariants. All three completed normally. Diffed ee3131be...4de2b7be (PR head not checked out).

Design review (Step 7) folded into the history pass's findings rather than a separate round, given the strength of what that pass already surfaced (the disclosed residual-risk overlap) and the empirical CI evidence (3/3 clean mock-suite runs, all CI lanes green). CI checked live via gh pr view --json statusCheckRollup: every check passing except E2E tests (MI300X) which was still QUEUED (self-hosted GPU hardware availability, not a failure) at review time.

Did not run: commit-signature cryptographic trust verification locally (no allowed_signers file available) — relied on GitHub's own Commit signatures + sign-off CI gate, which passed. Not reconciled against any PR discussion or review thread — none was read, per this review's blind-findings method.

[rominf]

Thanks. On the two decisions:

• Residual contention: agreed, it is the same kind of problem. The chat task can now run on any free worker. Before, it could only make progress when the one blocked polling thread came back. A starved worker pool is a much narrower case, and making run_rocm* async would close it. That is the follow-up the description names; it is too large to fold into this fix.
• Two commits: keeping them together. Commit 1 alone was not enough: in one of three runs automations-04 failed instead, from the same stall. So both are needed to turn the lane green.

[rominf]

The merge queue ejected this on engine-shell-01 (merge-group run 38531541718). That is the prompt race #663 fixes, not this change: chat-06 passed in that run. Re-queued behind #663, so its merge group includes that fix.