[jussielo-amd]

Strix Halo Ubuntu managed serves return success without exposing the model, with logs showing argument rejection, backend-ready/router-unavailable, or download failure. Existing xfails attribute all three to an immediate shutdown.

• Diagnostics: Capture runtime version, configured pins, candidate backend paths, SHA-256 fingerprints, and version/help output alongside archived service logs. The report reads files first and executes a backend second: the manifest, pins, candidate paths, fingerprints and version.txt are read before the service is stopped, and the llama-server --version and --help probes run after the stop returns, with their own 20 second budget. Both steps and the stop sit in one function, backend_evidence_around_stop, which runs the stop exactly once for every engine. The runtime manifest is read through AppPaths::engine_install_manifest_path, the same helper the Lemonade installer writes it with, so the two cannot drift. Candidates are searched under rocm-stable, rocm-nightly, rocm and vulkan down to three directory levels (the cpu backend is deliberately left out). The search is bounded as a whole: the inventory's 20 second deadline starts at entry and is checked at every directory and entry, at most 128 directories and 2048 entries are read across all backend directories, and the walk ends as soon as it has eight candidates, in sorted order. Symlinks are never followed (a link cycle ends, and a link out of the backend directory is not read; the report counts them), and a search cut short by the clock or a limit says which and never emits the completed-empty-search text, which is reserved for a search that finished and found nothing. The manifest and pin reads, the search and each fingerprint run on blocking threads with that in-traversal deadline. An empty search lists the locations it covered and what bin/llamacpp holds (16 names out of at most 64 entries read, with the omitted count), and does not guess at a cause. Bound probe duration and retained output; kill and reap timed-out children. The engine is read from both --engine X and --engine=X, falls back to the printed serve plan, and the failure report says so when no engine can be determined. A value that starts with -- after --engine (for example serve m --engine --managed) counts as no engine named, so the report still prints the engine unknown line instead of silently skipping the inventory. Executables up to 512 MiB are fingerprinted (streamed 64 KiB at a time under the inventory's time budget); a larger one is still listed and probed, and its entry says sha256 unavailable: executable exceeds 512 MiB hash limit.
• Expectations: Replace shutdown-only attribution with the observed failure signatures and cover serve-40's shared serve precondition (main now carries the serve-40 row itself, so this PR keeps that single row and only the other rows' attribution changes). Retain intermittent-failure handling and existing platform scope. The header keeps both the rule that flaky is set per row and is not derived from bug, and the caveat to revisit before deleting any of these rows until a failed run on the lane has carried the new backend evidence. The regression tests include Linux hosts with no GPU, with a gfx target but no usable GPU, and with a GPU but no gfx target, so removing either the therock_family or the has_amd_gpu guard from the bench and chat rows turns a test red; the earlier mock-capability case could not, because its os_family is other and the os = "linux" condition excluded it first.
• Scope: Diagnostic coverage, not a confirmed backend fix. Preserve the runtime pin and asynchronous managed-launch contract; distinguish configured pins and candidate binaries from proof of what actually ran. The only change outside the e2e harness is the small AppPaths::engine_install_manifest_path helper in rocm-core, which the Lemonade engine's own manifest_path now calls; it returns the same path as before.
• Scenario coverage: This changes the e2e harness's failure reporting and xfail expectations only, not anything the CLI prints or does, so there is no new user-observable behavior to cover with a scenario. The existing serving scenarios keep running; their xfail attribution is what changes.

Verification. The search bounds are pinned by unit tests that go red when each guard is removed: the clock read during the walk (driven by an injected clock, no sleeping), the whole-search entry and directory limits, the cap ending the walk when reached, symlink handling on Unix with a real link cycle, the bounded listing, and the scan running off the async executor. The ordering is pinned by unit tests in lemonade_diagnostics.rs: the stop step deletes the candidate, so a fingerprint in the report shows the file was read before the stop, and a recording probe shows the probes ran after it; moving either step to the other side of the stop turns the test red. An engine test writes a manifest and asserts it lands at the shared path with the field names the report reads, and it goes red when the engine builds a different path. The probe's timeout, kill and reap path now runs on every platform: the unit tests re-run the test binary itself as the child (an ignored helper test selected by an environment variable), let the probe time out, and require the child to be gone afterwards (run with the child on one test thread and on several, because libtest labels a single-threaded child's output), and leaking the child instead of killing it turns that test red. The shell-script llama-server stand-ins used by the other probe tests remain unix-only. Not covered anywhere is the kill/reap failed branch, which needs an operating system that refuses to kill a child it just started. I could not run the Windows lane locally; clippy for the Windows target passes for both the library and the e2e test target, and the Windows lane on the push is what runs the new child-process tests there. The e2e scenarios themselves were not run locally (no GPU).

Lane evidence. At the previous head, the Strix Halo Ubuntu nightly and release jobs both printed the new report five times each (E2E tests (Strix Halo, Ubuntu, nightly, current) and E2E tests (Strix Halo, Ubuntu, release, current) in the E2E self-hosted run for that head): the manifest was read from the data dir, the rocm-stable b12109 llama-server was found one directory below its backend directory and fingerprinted under the old 32 MiB cap, and --version and --help exited 0. That run used the old probe order (probes before the stop), so it shows the report is populated and the manifest path is right, not that the new order changes anything. The same two jobs on this push are what confirm the new order: the report's probes, run after the service stop was attempted section should follow its stop section. The W7900 and MI300X lane jobs had not reported at the time of writing.

Known trade-offs, stated so they are not read as more than they are. serve-40 (serve-stop-leaves-no-background-helper) is xfail and flaky on lemonade/linux because it needs the same real serve, so until #897 closes its "no background helper" assertion carries no signal on that lane: a pass there does not show the scenario's behaviour is healthy and a failure is not attributed to it. The guard itself stays covered by the unit test background_helper_not_spawned_when_no_automation_is_enabled in apps/rocm/src/service_launch.rs (which checks the gate function, not the serve call site) and by scenario automations-11 on the mock lane (the helper's own lifecycle). Whether a llama-server --version or --help run initialises a GPU backend has not been observed on a lane, which is why the probes now run after the stop and not next to the router; a stop that fails or finds no record can still leave the router running, and the report's stop section says so. The 512 MiB hash cap is an assumption about builds larger than the one observed (a statically linked multi-target build), not a measured size.

Supersedes #899, which had the same content under a bot author; this is the same change started from main at the time, plus one commit per review fix and merges of main (the branch is merged rather than rebased so the existing review context keeps matching).

• 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.
• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md (the clap declaration and dispatch wiring staying in main.rs/lib.rs is expected, not a violation).
• Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state — not only the wording — per AGENTS.md §3. Covers claims of an outcome, remediation advice naming a command, and promises that something will not happen.

Refs #897

[Copilot]

🟡 Changes recommended

The regression test uses a non-Linux mock capability, so it skips the relevant xfail guards and needs Linux/no-GPU cases, including one with a reported gfx target, to protect MockServer-backed chat scenarios.

1 open issue

• Add Linux mock chat cases to protect xfail scope · New

What changed in this PR

This PR improves E2E diagnostics for Lemonade managed-serve failures tracked in #897 without changing runtime pins or the CLI’s asynchronous launch behavior.

Changes:

• Captures candidate backend identities, configured pins, and bounded version/help output before service cleanup.
• Updates expected-failure attribution to cover observed failure signatures and serve-40’s shared precondition.
• Adds diagnostic and expectation tests, plus documentation of evidence limits.

File	Description
tests/​e2e-cucumber/​tests/​e2e/​serving_steps.rs	Collects backend evidence before stopping failed services.
tests/​e2e-cucumber/​src/​serve_log.rs	Adds engine detection and backend evidence to reports; bounds reads.
tests/​e2e-cucumber/​src/​lib.rs	Exposes the diagnostics module.
tests/​e2e-cucumber/​src/​lemonade_diagnostics.rs	Implements backend inventory, fingerprints, bounded probes, and tests.
tests/​e2e-cucumber/​src/​expectation.rs	Tests updated failure attribution and platform scope.
tests/​e2e-cucumber/​features/​model_serving.feature	Updates a comment’s xfail reference to #897.
tests/​e2e-cucumber/​expectations.toml	Revises failure descriptions and adds serve-40 coverage.
tests/​e2e-cucumber/​Cargo.toml	Adds hashing and asynchronous process/I/O dependencies.
docs/​testing.md	Documents collected evidence, limitations, and focused tests.
Cargo.lock	Records the E2E crate’s hashing dependency.

_{Review effort: Balanced}

[jussielo-amd]

Fixed in 2649611: a new test covers Linux hosts with no GPU, with a gfx target only, and with a GPU only, and still expects the xfail for the full GPU host. Removing has_amd_gpu from bench-load-real-serve turns only the new test red, and so does removing therock_family from chat-end-to-end-local-model; the old test stays green in both cases, which confirms your point.

[juhovainio]

I went through the whole diff. I found no correctness problems in the new diagnostics; the hash worker's own deadline inside spawn_blocking, the kill-and-reap on the probes, and the boundary tests around the size cap all look right. Two small things are left as inline comments, and I'm requesting changes so they get a look before merge.

Questions for the author:

• serve-40 is now xfail+flaky on lemonade/linux. The comment in the PR admits its assertion carries no signal on that lane, so a real background-helper regression there stays hidden until #897 closes. Is that the intended trade-off?
• The llama-server --version/--help probes run while the failed service is still alive. On a shared-memory APU, could a probe that initialises the GPU backend contend with the still-running router? I couldn't verify this either way.
• A llama-server binary larger than the 32 MiB hash cap gets no fingerprint. That is probably fine for current builds, but I haven't confirmed it.

The branch is also CONFLICTING with main (Cargo.lock, tests/e2e-cucumber/Cargo.toml, tests/e2e-cucumber/src/lib.rs). It needs a rebase, so the green CI so far is on the pre-rebase head.

[juhovainio]

--engine followed by another flag (e.g. serve m --engine --managed) returns --managed as the engine here. The report then neither captures backend evidence nor prints the "engine unknown" line, which breaks the promise that a skipped inventory is visible. Treating a value that starts with -- as absent would fix it; a test case for it would be good too.

[jussielo-amd]

Fixed in f475592: a value starting with -- after --engine (and --engine=--managed) now counts as no engine named, so the report prints the "engine unknown" line. Both forms are in the test table, and with the guard removed the test fails with left: Some("--managed").

[juhovainio]

The header this replaces also said that flaky is per-row and not derived from bug, so a new row has to set it explicitly. That rule is gone now. Worth keeping that sentence for the next person adding a row.

[jussielo-amd]

Restored in f237d2d: the header says again that flaky is per-row and not derived from bug, so a new #897 row has to set flaky = true explicitly.

[jussielo-amd]

Pushed 5f37981 with fixes for the inline notes (see the replies on each) and a merge of main instead of a rebase, so your review context stays valid. The only conflict was Cargo.lock in the e2e-cucumber dependency list: I kept both sha2 from this branch and shlex from main, in sorted order, and cargo metadata --locked accepts it; tests/e2e-cucumber/Cargo.toml and src/lib.rs merged cleanly. The PR body is updated, including dropping the stale "squashed to one commit" claim.

On your three questions:

1. serve-40 is xfail and flaky on lemonade/linux on purpose: until E2E: lemonade serving fails on the Strix Halo Ubuntu lanes, with a different log signature depending on the run #897 closes the assertion carries no signal on that lane, and the row is to be removed when it does (the expectations.toml comment says so, and the PR body now does too). What covers the guard elsewhere, verified in the code: the unit test background_helper_not_spawned_when_no_automation_is_enabled in service_launch.rs covers the gate function and scenario automations-11 covers the helper's lifecycle on the mock lane. The gap is that neither covers the serve call site itself.
2. Probe contention: every call site in serving_steps.rs runs the evidence collection only after a non-zero exit or a timed-out readiness poll, and only after the log has been read and archived, so the failing outcome is already decided. The router can still be alive during the probes, which run one at a time (5 s each, 20 s total) in the harness environment, not the engine's. What I cannot verify here (no GPU, no llama-server) is whether --version or --help initialises a GPU backend, so that part is unchecked, and docs/testing.md now says so.
3. A binary over the 32 MiB cap is still listed and probed, and the report prints sha256 unavailable: executable exceeds 32 MiB hash limit; no partial hash is ever labelled SHA-256. 25e11c5 adds a report-level test for it, which goes red when that line is suppressed.

cargo test -p e2e-cucumber passes (226 lib tests plus the extra targets), and clippy, fmt and cargo xtask manifest --check are clean. Could you take another look?

[Copilot]

🔵 Needs a closer look

The new llama-server probes run before a potentially live router is stopped, and their GPU initialization and contention risks remain unvalidated on Strix Halo hardware.

1 open issue

• Add Linux mock chat cases to protect xfail scope

_{Review effort: Balanced}

[jussielo-amd]

The Windows lane found a bug in a test I added in the last push, and c05b62c fixes it. oversized_executable_reports_why_it_has_no_fingerprint built its expected candidate path with / separators and compared printed text, while the report on Windows prints the same path with mixed separators (...\bin/llamacpp\rocm-stable\llama-server.exe); the report itself was correct. The test now parses the candidate lines back into paths and compares them component-wise. Auditing the same class in this PR found one more instance: evidence_truncates_candidates_without_hashing_or_probing_the_rest asserted that the skipped candidate's printed path was absent, which could never match on Windows and so passed vacuously. It now asserts the exact set of listed candidates (the count, the absent skipped one and the present first one), and a mutation that lists the wrong eight turns it red. The other new tests use no paths. One gap remains and is not new: five tests in lemonade_diagnostics.rs are unix-only because they use shell-script stand-ins, so the probe timeout and kill path does not run on the Windows lane. I could not run a Windows check locally (the C toolchain for the cross build is missing), so the Windows lane on this push is the confirmation.

[jussielo-amd]

Merged main into the branch again (5463b83, a plain merge commit) because main moved, including #900, which adds its own serve-stop-leaves-no-background-helper row to expectations.toml. The only conflict was in that file: the merge left two identical rows for that scenario (same lemonade/linux condition, #897, 90 s timeout, flaky = true), so I kept main's row with its fuller comment (it already says that while the row is in place the scenario's own helper check is not exercised, and that the row has to be deleted by hand when #897 closes) and dropped this branch's copy, so the scenario is listed once. The sentence saying flaky is per-row and not derived from bug is kept in the header, runtime-path-not-nested, which main removed, stays removed, and the regression test for the mock scope still passes against the merged row. cargo test -p e2e-cucumber --lib passes (238), clippy and fmt are clean, and the PR body is updated to match.

fixed

[juhovainio]

Overall: no blocking problems found; the scope is honest (diagnostics for #897, not a fix) and CI is green. Requesting changes on the points below.

Should address

• The llama-server --version/--help probes run before the service is stopped and may initialise a GPU backend alongside the live router, perturbing the state being diagnosed. Move the probes after the stop, or confirm on the lane that they are harmless and replace the "has not been checked" note in docs/testing.md.
• No test ties the new call site to the documented behaviour: the runtime.json manifest path matches what the CLI writes, and evidence is taken before the stop. A wrong path degrades silently to unavailable: on the real lane.

Smaller points

• backend_candidates searches only one level under four fixed backend dirs, and its empty-result message hints at a download problem.
• 32 MiB hash cap may be below real ROCm server binaries.
• The "revisit before deleting" caveat was dropped from the expectations.toml header; it is still true until the new evidence lands.
• Please confirm a repo-wide grep EAI-7423 is clean.
• The Windows timeout/kill path of the probe has no test and the PR text does not say so.
• The strix lane that would exercise the new report was not among the checks; please note which run confirms the report is populated.

[juhovainio]

Evidence, including the llama-server --version/--help probes, is collected here, before stop_scenario_services below. llama.cpp ROCm/Vulkan builds typically initialise the backend on --version, so the probes may touch the GPU next to the still-running router and perturb the state being diagnosed. Could the probes (not the file inventory/hashes) run after the stop, or has the lane shown they are harmless? docs/testing.md currently says this is unchecked.

[jussielo-amd]

Fixed in dfb844f. The probes now run after the stop: the manifest, pins, candidate paths, fingerprints and version.txt are still read here, before the stop, and the --version and --help probes execute only after stop_scenario_services returns, with their own 20 second budget. The order lives in one function, backend_evidence_around_stop, so this call site cannot reorder it, and docs/testing.md now describes what the code does instead of saying it was unchecked. I could not show on a lane whether a probe initialises a GPU backend, so I kept them off the router's side of the stop rather than call them harmless. At the previous head the Strix Ubuntu lanes ran the probes before the stop and both exited 0, which says nothing about contention either way.

[juhovainio]

Nothing asserts that this is the file the CLI actually writes, nor that this call precedes the stop. A wrong filename would only show up as unavailable: ... on the real lane, which defeats the feature. Consider sharing the filename constant with the producer or adding a test for the path.

[jussielo-amd]

Fixed in 05bcc1c and dfb844f. The filename is now one definition, AppPaths::engine_runtime_manifest_path, which the Lemonade installer's manifest_path and this call site both use, and an engine test writes a manifest and asserts it lands at that path with the field names the report reads (it goes red when the engine builds a different path). For the order, backend_evidence_around_stop takes the stop as a closure, and its test makes the stop delete the candidate: the fingerprint in the report shows the inventory came before the stop and a recording probe shows the probes came after it. Moving either step to the other side of the stop turns that test red. On the lane, the manifest was read correctly at the previous head: the Strix Ubuntu nightly and release jobs both printed the manifest, pins and a fingerprinted llama-server.

[jussielo-amd]

Correction to my earlier reply: the shared helper is AppPaths::engine_install_manifest_path, not engine_runtime_manifest_path. It was renamed in d4ca134 because the registry writer guard on main (a text match for runtime_manifest_path( in any file that also writes a file) matched the old name inside the Lemonade installer, and the new name also says what it is, the engine's own install manifest and not a runtime registry entry. The path it returns and the engine and harness call sites that share it are unchanged.

[juhovainio]

backend_candidates only looks at depth 0 and 1 under four hard-coded backend dirs, and when nothing is found the report hints at an incomplete download. Consider wording the empty case neutrally and listing the searched locations.

[jussielo-amd]

Fixed in 59d7891 (with a test-order correction in f5bff5f). The search now goes three directory levels below each of the four backend directories, bounded by an entry cap per directory, and the cpu backend is still left out on purpose. An empty result no longer hints at a download: it lists every location searched, the depth, and what bin/llamacpp actually contains (or that it cannot be read), because an interrupted download, an unsearched layout and a build that was never installed look the same. A test with literal layouts shows a build three levels down is found and one four levels down is not, and it goes red when the depth is lowered. For reference, on the Strix Ubuntu lanes the b12109 build sits one level below rocm-stable, so the old depth already found it there.

[juhovainio]

32 MiB hash cap: please confirm real ROCm llama-server builds fit under it, otherwise the fingerprint is absent on exactly the lane this is for.

[jussielo-amd]

Checked, and changed anyway as margin, in b064945 and ee08e62. The Strix Halo Ubuntu nightly and release jobs at the previous head both fingerprinted the rocm-stable b12109 llama-server under the old 32 MiB cap, so a current ROCm build fits. A statically linked multi-target build could be larger, so the cap is now 512 MiB; hashing still streams 64 KiB at a time under the inventory time budget, so memory and time stay bounded, and the over-cap line is worded from the constant. The remaining assumption, about builds larger than the observed one, is stated in docs/testing.md and the PR body.

[jussielo-amd]

Thanks, all points are addressed on this branch, and I merged main again in 11f338d (no conflicts; the conflicts from the earlier review are already resolved).

• Probes before the stop: moved after it, dfb844f. File reads stay before the stop; the probes run after it with their own budget. docs/testing.md and the expectations header now state what the code does.
• No test tied the call site to the documented behaviour: the manifest path is shared with the producer and tested against it (05bcc1c), and the order against the stop is pinned by a test that goes red if either step moves (dfb844f). A wrong path still shows as unavailable: in the report, but it can no longer differ from the one the installer writes.
• --engine followed by another flag: already fixed in f475592 (a value starting with -- counts as no engine, so the engine unknown line prints), with both forms in the test table.
• expectations.toml header: the per-row flaky sentence was restored in f237d2d, and the revisit-before-deleting caveat is back in a9bf807, reworded for what is true until a failed run on the lane has carried the new evidence.
• backend_candidates and its empty-result message: wider search and a neutral, location-listing message, 59d7891.
• 32 MiB cap: confirmed from the lane that current builds fit, cap raised as margin, b064945 and ee08e62.
• grep -rn EAI-7423 is not empty, and I left the hits: one is a bare ticket id in this diff, in an expectations.toml comment saying EAI-7423 described only the earlier symptom, and the other two come from main (the serve-40 row's comment from test(e2e): expect serve-40 to fail on lemonade Linux hosts while #897 is open #900, and a fixture in expectation.rs). Bare ticket ids are allowed by the repo's contribution rules, and no new rows use it as bug.
• Windows timeout and kill path: now tested on every platform in 465912f by re-running the test binary as the child process; leaking the child instead of killing it turns it red. The kill/reap failed branch is still untested, because it needs an OS that refuses to kill a child it just started, and the PR text and docs/testing.md say so. I could not run the Windows lane locally, only clippy for the Windows target, so the Windows job on this push is the confirmation.
• Strix lane: the Strix Halo Ubuntu nightly and release jobs in the E2E self-hosted run for 5463b83 printed the new report five times each, with manifest, pins, a fingerprinted llama-server and --version and --help exit 0; the W7900 and MI300X jobs were still pending. That run used the old order, so the same two jobs on the next push are what confirm the new one (the probes, run after the service stop was attempted section after stop).
• The CI clippy runs also needed one lint fix, 1e514a9.

Fixed: every point is answered on the PR; the probes now run after the stop (dfb844f), the manifest path is shared with its producer and tested (05bcc1c), the candidate search and its empty-result message are reworked (59d7891), the hash cap is raised (b064945), the probe kill path is tested (465912f) and the expectations header caveats are restored (a9bf807).

[Copilot]

🟡 Changes recommended

Candidate discovery runs before the deadline and applies its cap only after recursive search, allowing large or symlinked caches to exhaust time and memory and delay service shutdown, cleanup, and failure reporting.

2 open issues

• Bound candidate discovery by the deadline and traversal limits · New
• Add Linux mock chat cases to protect xfail scope

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

The timeout regression test can fail with single-threaded libtest output, and directory- or entry-limited searches can falsely report that all backend trees were searched.

3 open issues

• Suppress empty-search explanation when discovery stops at a limit · New
• Print the child PID on a fresh line for reliable parsing · New
• Add Linux mock chat cases to protect xfail scope

1 resolved since last review

• Bound candidate discovery by the deadline and traversal limits

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Human assessment is needed to evaluate a symlink-handling gap that could expose file contents from outside the backend directory in failure reports.

1 open issue

• Add Linux mock chat cases to protect xfail scope

2 resolved since last review

• Print the child PID on a fresh line for reliable parsing
• Suppress empty-search explanation when discovery stops at a limit

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

The diagnostics report can read and embed an outside file through a symlinked version.txt despite reporting the link as skipped, so the marker needs a non-symlink regular-file check and regression coverage.

2 open issues

• Version marker symlinks are followed despite being reported skipped · New
• Add Linux mock chat cases to protect xfail scope

_{Review effort: Balanced}