From 13dd7b38595984e38aabef6d79f91946e244f2e4 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Tue, 22 Sep 2026 10:01:06 +0000 Subject: [PATCH 1/2] test(e2e): drive the Lemonade recovery scenario with a planted runtime Delete the `e2e-test-hooks` cargo feature and every seam it gated. The feature cost the repo a CI-wide asymmetry: every lane running the full suite pre-built `rocm` with it, so the binary under test was not the binary released. The Linux `ci.yml` lifecycle lane packaged and installed a hook-carrying build through the real installer, and the contract tests encoded the split rather than removing it -- one lane asserted hook-free, two asserted hook-ful, and Linux was covered by neither. The principle applied throughout: what must not diverge between the tested and the shipped binary is the COMPILE. This is not "no test affordances" -- a path- or state-based affordance is present identically in every build and inert until something plants the state, whereas a `#[cfg(feature)]` makes the tested binary a different binary. Both consumers move to the former. Lemonade -------- `@id:serve-lemonade-preparation-recovery` needs Lemonade's llama.cpp backend install to fail twice so the scenario can pin the retry count and the terminal recovery guidance. It now gets that from a planted runtime instead of a compiled-in branch: one fixture binary copied to `lemonade`/`lemond` in the scenario's isolated runtime dir, plus a hand-written install manifest. The manifest's `env_id` is the load-bearing field. Before `serve` prepares an engine it calls `Detect` and skips its own install only when the reported id equals the `lemonade-embeddable-{LEMONADE_VERSION}` that `managed_engine_runtime_id` builds. A placeholder there does not fail loudly -- it silently lets `serve` run the real multi-gigabyte install and succeed, which is the opposite of the scenario's premise. The id is therefore derived from `rocm_deps::LEMONADE_VERSION` rather than copied, so a version bump cannot reintroduce that fallthrough. With the install skipped, `serve_http` resolves the planted runtime and reaches `ensure_best_llamacpp_backend`, whose `backends install` runs the fixture and fails where a genuinely broken install fails: at a non-zero child process. `resolve_runtime` accepts the manifest because it checks only that it parses and that `lemond` is a file -- no checksum, version or signature is involved on this path, so nothing here weakens a verification the product performs, and `rocm` stays byte-identical to what ships. Seam #2 waived serve's no-GPU pre-flight so the scenario could reach the install phase on a GPU-less host. Without it the pre-flight is reached first, so the scenario is retagged `@requires-gpu`. That is the real cost of this change: it moves off the blocking every-PR lane onto a gated one. Tracked in the issue this closes. Because the scenario no longer runs on a GPU-less lane, a unit test pins the coupling that lane would otherwise silently lose: the fixture's `backends` table must still parse into a `llamacpp:rocm` install attempt. Dashboard clock --------------- The logical observation clock added in #380 was the feature's other consumer. The daemon side already ships unconditionally -- `RunnerOptions::test_clock_offset_path` is a plain field documented test-only -- and only the CLI's env-var read was gated. `rocm dash` now takes the directive file from `/telemetry/ test-clock-offset`, inside the data root it already resolves, so the harness plants it exactly as it plants the Lemonade manifest. The existence check there is load-bearing rather than an optimisation: `test_clock_offset_path` selects `Utc::now()` only on `None`, and a `Some(path)` whose file is absent leaves the daemon on its default `FreeRunning(0)` logical clock. Returning the path unconditionally would take production off wall time. Workflows --------- The two prebuilt-lane contract tests are repurposed rather than deleted. "Match the feature xtask uses" becomes "match the invocation xtask uses", which is the invariant that actually prevents the duplicate release build, asserted whole-line so a trailing flag cannot satisfy it. Closes #349. Signed-off-by: Roman Inflianskas --- .github/workflows/ci.yml | 22 ++-- .github/workflows/e2e-selfhosted.yml | 43 ++++---- .github/workflows/nightly.yml | 36 +++---- Cargo.lock | 1 + apps/rocm/Cargo.toml | 3 - apps/rocm/src/dash.rs | 30 ++++-- apps/rocm/src/main.rs | 7 +- engines/lemonade/Cargo.toml | 3 - engines/lemonade/src/lib.rs | 45 ++++++-- tests/e2e-cucumber/Cargo.toml | 14 +++ .../features/install_lifecycle.feature | 8 -- .../features/model_serving.feature | 15 +-- tests/e2e-cucumber/src/bin/fake-lemonade.rs | 100 ++++++++++++++++++ tests/e2e-cucumber/tests/e2e.rs | 84 +++++++++++++++ tests/e2e-cucumber/tests/e2e/dash_steps.rs | 33 ++++-- tests/e2e-cucumber/tests/e2e/serving_steps.rs | 27 ++--- xtask/src/e2e.rs | 11 +- xtask/src/workflow_contract.rs | 68 ++++++------ 18 files changed, 384 insertions(+), 166 deletions(-) create mode 100644 tests/e2e-cucumber/src/bin/fake-lemonade.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2a1489b78..416b0ea87 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -631,20 +631,16 @@ jobs: # Windows install lifecycle (packaging + native-crypto verify + user-PATH # + loopback HTTP + isolated smoke + uninstall) as opt-in @lifecycle E2E. # - # Hand the suite the binaries the Build step already produced. Without - # this `cargo xtask e2e` builds `rocm`/`rocmd` for itself, and because it - # adds `--features rocm/e2e-test-hooks` the feature resolution differs - # from the Build step's — so cargo recompiles the whole release graph - # rather than reusing it. That second release build cost 3-4 minutes on - # every run, on the job that alone determines when CI goes green. + # Hand the suite the binaries the Build step already produced, so + # `cargo xtask e2e` skips its own build entirely. Relying on cargo to + # notice the artifacts are fresh is not enough: any divergence between + # the two invocations (profile, package set, RUSTFLAGS, target dir) + # re-resolves the graph and recompiles it. That is what happened here — + # a feature-set mismatch cost a second 3-4 minute release build on every + # run, on the job that alone determines when CI goes green. # - # Deliberately NOT built with `rocm/e2e-test-hooks`, unlike the prebuilt - # lanes in e2e-selfhosted.yml / nightly.yml. Those run the full suite, - # whose scripted failure seams are compiled out without the feature. - # `E2E_ONLY_LIFECYCLE` restricts this lane to @lifecycle scenarios, none - # of which touch a seam — and this lane packages and installs the binary - # through the real installer, so it should ship exactly what a release - # ships rather than a build carrying test hooks. + # This lane packages and installs the binary through the real installer, + # so it ships exactly what a release ships. env: E2E_INCLUDE_LIFECYCLE: "1" E2E_ONLY_LIFECYCLE: "1" diff --git a/.github/workflows/e2e-selfhosted.yml b/.github/workflows/e2e-selfhosted.yml index 0e490d7ef..c284c2fac 100644 --- a/.github/workflows/e2e-selfhosted.yml +++ b/.github/workflows/e2e-selfhosted.yml @@ -315,14 +315,11 @@ jobs: # pre-warm and suite so xtask does not rebuild. Honors # CARGO_TARGET_DIR set above. # - # `--features rocm/e2e-test-hooks` must match what `cargo xtask e2e` - # builds when it builds for itself. The suite's deterministic failure - # seams (e.g. the scripted Lemonade backend-install failure) are - # compiled out without it, so a pre-built binary that omits the feature - # leaves those scenarios unable to reach their premise — they then fail - # as regressions on whichever lane happens to select them. Every lane - # that pre-builds and exports ROCM_CLI_BINARY must pass it. - cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks + # Keep this invocation identical to the one `cargo xtask e2e` runs when + # it builds for itself. Any divergence (profile, package set, features) + # re-resolves the graph and makes xtask recompile everything instead of + # reusing what this step just built. + cargo build --release -p rocm -p rocmd export ROCM_CLI_BINARY="$CARGO_TARGET_DIR/release/rocm" export ROCM_CLI_ROCMD_BINARY="$CARGO_TARGET_DIR/release/rocmd" @@ -480,9 +477,9 @@ jobs: export E2E_SHARED_RUNTIMES_DIR="$prewarm/data/runtimes" # Build the rocm and rocmd binaries once; reuse them for pre-warm + suite. - # See the e2e-gpu lane for why the e2e-test-hooks feature must match - # what `cargo xtask e2e` would build. - cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks + # See the e2e-gpu lane for why this must match what `cargo xtask e2e` + # would build for itself. + cargo build --release -p rocm -p rocmd export ROCM_CLI_BINARY="$CARGO_TARGET_DIR/release/rocm" export ROCM_CLI_ROCMD_BINARY="$CARGO_TARGET_DIR/release/rocmd" @@ -648,9 +645,9 @@ jobs: # Build the rocm and rocmd binaries once; reuse them for pre-warm + suite. # This job does not set CARGO_TARGET_DIR, so the binaries land in # the default target\release (fall back to it when the env var is unset). - # See the e2e-gpu lane for why the e2e-test-hooks feature must match - # what `cargo xtask e2e` would build. - cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks + # See the e2e-gpu lane for why this must match what `cargo xtask e2e` + # would build for itself. + cargo build --release -p rocm -p rocmd if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } $targetDir = if ($env:CARGO_TARGET_DIR) { $env:CARGO_TARGET_DIR } else { "target" } $env:ROCM_CLI_BINARY = "$targetDir\release\rocm.exe" @@ -981,9 +978,9 @@ jobs: export E2E_SHARED_RUNTIMES_DIR="$prewarm/data/runtimes" # Build the rocm and rocmd binaries once; reuse them for pre-warm + suite. - # See the e2e-gpu lane for why the e2e-test-hooks feature must match - # what `cargo xtask e2e` would build. - cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks + # See the e2e-gpu lane for why this must match what `cargo xtask e2e` + # would build for itself. + cargo build --release -p rocm -p rocmd export ROCM_CLI_BINARY="$CARGO_TARGET_DIR/release/rocm" export ROCM_CLI_ROCMD_BINARY="$CARGO_TARGET_DIR/release/rocmd" @@ -1157,9 +1154,9 @@ jobs: prewarm="$RUNNER_WORKSPACE/e2e-prewarm-multi-arch-v2" export E2E_SHARED_RUNTIMES_DIR="$prewarm/data/runtimes" - # See the e2e-gpu lane for why the e2e-test-hooks feature must match - # what `cargo xtask e2e` would build. - cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks + # See the e2e-gpu lane for why this must match what `cargo xtask e2e` + # would build for itself. + cargo build --release -p rocm -p rocmd export ROCM_CLI_BINARY="$CARGO_TARGET_DIR/release/rocm" export ROCM_CLI_ROCMD_BINARY="$CARGO_TARGET_DIR/release/rocmd" @@ -1286,9 +1283,9 @@ jobs: prewarm="$RUNNER_WORKSPACE/e2e-prewarm-multi-arch-v2" export E2E_SHARED_RUNTIMES_DIR="$prewarm/data/runtimes" - # See the e2e-gpu lane for why the e2e-test-hooks feature must match - # what `cargo xtask e2e` would build. - cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks + # See the e2e-gpu lane for why this must match what `cargo xtask e2e` + # would build for itself. + cargo build --release -p rocm -p rocmd export ROCM_CLI_BINARY="$CARGO_TARGET_DIR/release/rocm" export ROCM_CLI_ROCMD_BINARY="$CARGO_TARGET_DIR/release/rocmd" diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 1e8bd1740..4be94e164 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -449,9 +449,9 @@ jobs: export E2E_SHARED_RUNTIMES_DIR="$prewarm/data/runtimes" # Build both binaries once; reuse them for pre-warm + suite. - # See the e2e-gpu lane in e2e-selfhosted.yml for why the - # e2e-test-hooks feature must match what `cargo xtask e2e` would build. - cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks + # See the e2e-gpu lane in e2e-selfhosted.yml for why this must match + # what `cargo xtask e2e` would build for itself. + cargo build --release -p rocm -p rocmd export ROCM_CLI_BINARY="$CARGO_TARGET_DIR/release/rocm" export ROCM_CLI_ROCMD_BINARY="$CARGO_TARGET_DIR/release/rocmd" @@ -552,9 +552,9 @@ jobs: prewarm="$RUNNER_WORKSPACE/e2e-prewarm-multi-arch-v2" export E2E_SHARED_RUNTIMES_DIR="$prewarm/data/runtimes" - # See the e2e-gpu lane in e2e-selfhosted.yml for why the - # e2e-test-hooks feature must match what `cargo xtask e2e` would build. - cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks + # See the e2e-gpu lane in e2e-selfhosted.yml for why this must match + # what `cargo xtask e2e` would build for itself. + cargo build --release -p rocm -p rocmd export ROCM_CLI_BINARY="$CARGO_TARGET_DIR/release/rocm" export ROCM_CLI_ROCMD_BINARY="$CARGO_TARGET_DIR/release/rocmd" @@ -643,9 +643,9 @@ jobs: prewarm="$RUNNER_WORKSPACE/e2e-prewarm-multi-arch-v2" export E2E_SHARED_RUNTIMES_DIR="$prewarm/data/runtimes" - # See the e2e-gpu lane in e2e-selfhosted.yml for why the - # e2e-test-hooks feature must match what `cargo xtask e2e` would build. - cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks + # See the e2e-gpu lane in e2e-selfhosted.yml for why this must match + # what `cargo xtask e2e` would build for itself. + cargo build --release -p rocm -p rocmd export ROCM_CLI_BINARY="$CARGO_TARGET_DIR/release/rocm" export ROCM_CLI_ROCMD_BINARY="$CARGO_TARGET_DIR/release/rocmd" @@ -743,9 +743,9 @@ jobs: prewarm="$RUNNER_WORKSPACE/e2e-prewarm-multi-arch-v2" export E2E_SHARED_RUNTIMES_DIR="$prewarm/data/runtimes" - # See the e2e-gpu lane in e2e-selfhosted.yml for why the - # e2e-test-hooks feature must match what `cargo xtask e2e` would build. - cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks + # See the e2e-gpu lane in e2e-selfhosted.yml for why this must match + # what `cargo xtask e2e` would build for itself. + cargo build --release -p rocm -p rocmd export ROCM_CLI_BINARY="$CARGO_TARGET_DIR/release/rocm" export ROCM_CLI_ROCMD_BINARY="$CARGO_TARGET_DIR/release/rocmd" @@ -849,9 +849,9 @@ jobs: # engine's cache lookup didn't fall back to USERPROFILE. $env:E2E_SHARED_CACHE_DIR = "$env:RUNNER_WORKSPACE\e2e-shared" - # See the e2e-gpu lane in e2e-selfhosted.yml for why the - # e2e-test-hooks feature must match what `cargo xtask e2e` would build. - cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks + # See the e2e-gpu lane in e2e-selfhosted.yml for why this must match + # what `cargo xtask e2e` would build for itself. + cargo build --release -p rocm -p rocmd if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } $targetDir = if ($env:CARGO_TARGET_DIR) { $env:CARGO_TARGET_DIR } else { "target" } $env:ROCM_CLI_BINARY = "$targetDir\release\rocm.exe" @@ -1091,9 +1091,9 @@ jobs: prewarm="/root/work/e2e-prewarm-multi-arch-v2" export E2E_SHARED_RUNTIMES_DIR="$prewarm/data/runtimes" - # See the e2e-gpu lane in e2e-selfhosted.yml for why the - # e2e-test-hooks feature must match what `cargo xtask e2e` would build. - cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks + # See the e2e-gpu lane in e2e-selfhosted.yml for why this must match + # what `cargo xtask e2e` would build for itself. + cargo build --release -p rocm -p rocmd export ROCM_CLI_BINARY="$CARGO_TARGET_DIR/release/rocm" export ROCM_CLI_ROCMD_BINARY="$CARGO_TARGET_DIR/release/rocmd" diff --git a/Cargo.lock b/Cargo.lock index 8b8bf4d6f..2cb998971 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1271,6 +1271,7 @@ dependencies = [ "e2e-report", "portable-pty", "reqwest 0.13.4", + "rocm-deps", "serde", "serde_json", "tempfile", diff --git a/apps/rocm/Cargo.toml b/apps/rocm/Cargo.toml index 3d4990869..c1de88d85 100644 --- a/apps/rocm/Cargo.toml +++ b/apps/rocm/Cargo.toml @@ -10,9 +10,6 @@ publish.workspace = true [lints] workspace = true -[features] -e2e-test-hooks = ["rocm-engine-lemonade/e2e-test-hooks"] - [dependencies] anyhow.workspace = true clap.workspace = true diff --git a/apps/rocm/src/dash.rs b/apps/rocm/src/dash.rs index e005220b4..992a7a546 100644 --- a/apps/rocm/src/dash.rs +++ b/apps/rocm/src/dash.rs @@ -69,18 +69,32 @@ pub fn runner_options( // Production always runs the real `/dev/kfd` pre-flight; only daemon // integration tests with a fake binary skip it. amd_smi_skip_kfd_preflight: false, - test_clock_offset_path: dash_test_clock_offset_path(), + test_clock_offset_path: dash_test_clock_offset_path(paths), } } -#[cfg(feature = "e2e-test-hooks")] -fn dash_test_clock_offset_path() -> Option { - std::env::var_os("ROCM_CLI_DASH_TEST_CLOCK_OFFSET_PATH").map(Into::into) -} +/// Name of the logical-clock directive file, read from the telemetry state dir. +const DASH_TEST_CLOCK_FILE: &str = "test-clock-offset"; -#[cfg(not(feature = "e2e-test-hooks"))] -const fn dash_test_clock_offset_path() -> Option { - None +/// Path of the daemon's logical observation clock, or `None` to use wall time. +/// +/// State-based, not compile-time-gated: the file simply does not exist on a +/// user's machine, and nothing in `rocm` ever creates it — only the E2E harness +/// plants one, in the isolated data root it hands this process via +/// `ROCM_CLI_DATA_DIR`. That keeps the binary under test byte-identical to the +/// binary that ships, the same way the Lemonade recovery scenario plants a +/// runtime manifest that production code reads through its normal path. A +/// `#[cfg(feature = ...)]` seam cannot make that claim: it makes the tested +/// binary a different binary. +/// +/// The existence check is load-bearing, not an optimisation. `RunnerOptions:: +/// test_clock_offset_path` selects wall time *only* on `None`; a `Some(path)` +/// whose file is absent leaves the daemon on its default +/// `TestClockDirective::FreeRunning(0)` logical clock. Returning the path +/// unconditionally would therefore take production off `Utc::now()`. +fn dash_test_clock_offset_path(paths: &AppPaths) -> Option { + let path = paths.telemetry_state_dir().join(DASH_TEST_CLOCK_FILE); + path.is_file().then_some(path) } /// API key precedence — sourced from the environment ONLY (never TOML/CLI/source/ diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 3377ee51c..782ba39ea 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -5995,13 +5995,8 @@ fn serve(args: ServeArgs) -> Result<()> { // BEFORE preparing or launching any engine (no wasted engine download, and an // actionable message instead of a late engine crash). The engine enforces the // same rule as a backstop. Skipped for cpu_only; permissive when availability - // cannot be probed on this platform (probe returns `None`). The E2E-only - // backend-failure scenario bypasses this host precondition so the black-box - // test reaches Lemonade's backend boundary without real GPU hardware. - let scripted_backend_failure = cfg!(feature = "e2e-test-hooks") - && std::env::var_os("ROCM_E2E_LEMONADE_BACKEND_INSTALL_FAILURE").is_some(); + // cannot be probed on this platform (probe returns `None`). if !cpu_only - && !scripted_backend_failure && let Some(usable) = visible_gpu_indices.as_deref() && usable.is_empty() { diff --git a/engines/lemonade/Cargo.toml b/engines/lemonade/Cargo.toml index 94e37252e..d2c28fb7b 100644 --- a/engines/lemonade/Cargo.toml +++ b/engines/lemonade/Cargo.toml @@ -10,9 +10,6 @@ publish.workspace = true [lints] workspace = true -[features] -e2e-test-hooks = [] - [[bin]] name = "rocm-engine-lemonade" path = "src/main.rs" diff --git a/engines/lemonade/src/lib.rs b/engines/lemonade/src/lib.rs index 154d90c0f..5c44aa707 100644 --- a/engines/lemonade/src/lib.rs +++ b/engines/lemonade/src/lib.rs @@ -56,8 +56,6 @@ const BACKEND_VERSIONS_RESOURCE: &str = "resources/backend_versions.json"; /// wipes it, and re-extracts on every call — destroying any llama.cpp backend /// installed into it in the process. const RUNTIME_VERSION_MARKER: &str = "runtime-version.txt"; -#[cfg(feature = "e2e-test-hooks")] -const BACKEND_INSTALL_FAILURE_TEST_ENV: &str = "ROCM_E2E_LEMONADE_BACKEND_INSTALL_FAILURE"; /// Preferred llama.cpp backends, best first. Lemonade reports per-GPU support; /// we pick the highest-priority backend it considers supported on this host. /// GPU backends only — `cpu` is intentionally excluded so the router path never @@ -444,14 +442,6 @@ fn detect_response() -> DetectResponse { fn install_response(request: InstallRequest) -> Result { let paths = AppPaths::discover()?; paths.ensure()?; - // Debug builds expose a deterministic failure seam for the black-box CLI - // scenario that pins retry count and terminal recovery guidance. Keep it at - // the backend phase: the real defect happens after the embeddable is ready, - // and exercising it must not download or alter a runtime on the test host. - #[cfg(feature = "e2e-test-hooks")] - if std::env::var_os(BACKEND_INSTALL_FAILURE_TEST_ENV).is_some() { - install_llamacpp_backend_with_retry(|| bail!("scripted Lemonade backend install failure"))?; - } eprintln!( "Preparing Lemonade embeddable {}...", rocm_deps::LEMONADE_VERSION @@ -6816,6 +6806,41 @@ vllm rocm unsupported Requires Linux ); } + /// The E2E fake runtime's `lemonade backends` table must drive this engine to + /// attempt a `llamacpp:rocm` install — that attempt is the whole premise of + /// `@id:serve-lemonade-preparation-recovery`. + /// + /// That scenario is `@requires-gpu`, so nothing on a GPU-less lane would + /// notice if a parser change stopped accepting the fixture's table: the + /// scenario would quietly stop reaching the failure it asserts, and only a + /// GPU lane would report it. Pin the coupling here, where every lane runs it. + /// Keep this table byte-identical to the one printed by + /// `tests/e2e-cucumber/src/bin/fake-lemonade.rs`. + #[test] + fn e2e_fake_runtime_table_drives_a_rocm_backend_install() { + let output = "\ +Recipe Backend Status +-------- -------- ----------- +llamacpp rocm installable +llamacpp vulkan unsupported +"; + let backends = parse_llamacpp_backend_statuses(output); + assert_eq!( + backends, + vec![ + ("rocm".to_owned(), "installable".to_owned()), + ("vulkan".to_owned(), "unsupported".to_owned()), + ] + ); + // `false` = not already installed, which is what makes the engine run the + // install the fixture then refuses. Reporting `installed` instead would + // skip the install and the scenario would never see a failure. + assert_eq!( + select_best_llamacpp_backend(&backends), + Some(("rocm".to_owned(), false)) + ); + } + #[test] fn selects_vulkan_when_rocm_unsupported() { let backends = vec![ diff --git a/tests/e2e-cucumber/Cargo.toml b/tests/e2e-cucumber/Cargo.toml index e7c3e8a8c..2b4b7889b 100644 --- a/tests/e2e-cucumber/Cargo.toml +++ b/tests/e2e-cucumber/Cargo.toml @@ -16,6 +16,16 @@ workspace = true name = "rocm-demo-env" path = "src/bin/rocm-demo-env.rs" +# Stand-in for a Lemonade runtime's `lemonade`/`lemond` executables, planted by +# the serve-preparation-recovery scenario. A bin target of this package so the +# suite can reach it through `CARGO_BIN_EXE_fake-lemonade`, which cargo sets when +# it builds the `e2e` test target — no xtask plumbing and no second copy of the +# path to keep in step. It must be a real executable, not a script: the engine +# spawns it by exact path, and Windows `CreateProcess` needs a PE image. +[[bin]] +name = "fake-lemonade" +path = "src/bin/fake-lemonade.rs" + [dependencies] axum.workspace = true cucumber = { version = "0.23", features = ["output-json", "output-junit"] } @@ -26,6 +36,10 @@ e2e-report = { path = "../../crates/e2e-report" } # only way to exercise the crossterm raw-mode event loop a piped `Command` can't. portable-pty = "0.9" reqwest = { version = "0.13", features = ["json"] } +# The pinned Lemonade version, so the planted runtime's `env_id` is derived from +# the same constant `rocm` builds its expected runtime id from rather than being +# a copy that a version bump would silently invalidate. +rocm-deps = { path = "../../crates/rocm-deps" } serde.workspace = true serde_json.workspace = true tempfile = "3" diff --git a/tests/e2e-cucumber/features/install_lifecycle.feature b/tests/e2e-cucumber/features/install_lifecycle.feature index 63f5a617b..6bcd08e9f 100644 --- a/tests/e2e-cucumber/features/install_lifecycle.feature +++ b/tests/e2e-cucumber/features/install_lifecycle.feature @@ -15,14 +15,6 @@ Feature: Release install lifecycle # signing key, and installs into its own directory rooted in the scenario's # temp dir, so ordering never affects the outcome. Cargo's release build cache # is shared naturally. - # - # CONSTRAINT: no @lifecycle scenario may depend on a scripted failure seam - # (anything gated behind the `rocm/e2e-test-hooks` feature). ci.yml's Windows - # lane runs these against binaries built WITHOUT that feature, deliberately, - # so it installs what a release installs. A scenario that reached a seam would - # fail on Windows only — the hook-carrying Linux lane would stay green — which - # is a confusing way to learn about it. Keep such scenarios in the always-on - # suite instead, where the hooks are compiled in. # ── Packaging + signature-verified install (Linux) ──────────────────── diff --git a/tests/e2e-cucumber/features/model_serving.feature b/tests/e2e-cucumber/features/model_serving.feature index 8ecd89f79..f9a1be15f 100644 --- a/tests/e2e-cucumber/features/model_serving.feature +++ b/tests/e2e-cucumber/features/model_serving.feature @@ -176,12 +176,15 @@ Feature: Model serving Then serving is refused before any engine starts And the user is told the two selectors cannot be combined - # The failure is injected at Lemonade's backend-install boundary in debug/test - # builds, after the CLI has selected Lemonade but before any runtime download or - # machine mutation. That makes the user-visible retry and final recovery command - # deterministic on the blocking no-GPU lane rather than relying on a real 3 GiB - # transfer to fail at just the right moment. - @id:serve-lemonade-preparation-recovery @requires-no-gpu + # The harness plants a Lemonade runtime whose `lemonade backends install` always + # exits non-zero, so the failure happens at the real boundary — a non-zero child + # process, after the CLI has selected Lemonade but before any runtime download or + # machine mutation — and the user-visible retry and recovery command are + # deterministic without a real multi-gigabyte transfer failing at just the right + # moment. Needs a GPU because `rocm serve` enforces its GPU-required policy + # before it resolves the runtime, so a GPU-less host stops at the pre-flight and + # never reaches preparation. + @id:serve-lemonade-preparation-recovery @requires-gpu Scenario: serve-18 - Repeated Lemonade preparation failure gives the user a recovery path Given Lemonade preparation cannot complete When the user serves a model with Lemonade diff --git a/tests/e2e-cucumber/src/bin/fake-lemonade.rs b/tests/e2e-cucumber/src/bin/fake-lemonade.rs new file mode 100644 index 000000000..7663ea370 --- /dev/null +++ b/tests/e2e-cucumber/src/bin/fake-lemonade.rs @@ -0,0 +1,100 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! A stand-in for the two executables a Lemonade runtime installs: the `lemonade` +//! CLI and the `lemond` server. +//! +//! `@id:serve-lemonade-preparation-recovery` needs Lemonade's llama.cpp backend +//! install to fail deterministically, twice, so the scenario can pin the retry +//! count and the terminal recovery guidance. That failure used to be a +//! `#[cfg(feature = "e2e-test-hooks")]` seam compiled into `rocm` itself, which +//! meant every full-suite CI lane shipped a binary that differed from a release +//! build. Driving it from here keeps `rocm` identical to what users run: the CLI +//! spawns these as real subprocesses over the same interface it uses for a real +//! runtime, and only the runtime is fake. +//! +//! The harness copies this one binary into a planted runtime directory under +//! both names, so the role is taken from `argv[0]`: +//! +//! - `lemond` — the server `rocm serve` spawns. It only has to stay alive; the +//! CLI's readiness check polls the `lemonade` CLI, not this process. +//! - `lemonade` — the CLI the engine shells out to. Three calls matter, and the +//! real binary's contract for each is what this reproduces: +//! - `--host H --port P status` → exit 0, which is how +//! `wait_for_lemonade_cli_status` decides the server came up. +//! - `backends` → the recipe/backend/status table +//! `parse_llamacpp_backend_statuses` scrapes. Reporting `llamacpp rocm` as +//! `installable` (not `installed`) is what makes the engine attempt an +//! install rather than reuse one. +//! - `--host H --port P backends install llamacpp:rocm` → non-zero, the +//! failure the scenario is about. + +use std::path::Path; +use std::process::ExitCode; +use std::time::Duration; + +fn main() -> ExitCode { + // Copied rather than symlinked (self-hosted Windows runners commonly lack + // SeCreateSymbolicLinkPrivilege), so the file name is the only role signal. + let argv0 = std::env::args_os().next().unwrap_or_default(); + let role = Path::new(&argv0) + .file_stem() + .map(|stem| stem.to_string_lossy().into_owned()) + .unwrap_or_default(); + let args: Vec = std::env::args().skip(1).collect(); + + match role.as_str() { + "lemond" => run_lemond(), + "lemonade" => run_lemonade_cli(&args), + other => { + eprintln!( + "fake-lemonade: copied to an unexpected name {other:?}; expected `lemond` or \ + `lemonade`" + ); + ExitCode::FAILURE + } + } +} + +/// Stay alive until the parent kills us. `rocm serve` spawns `lemond`, writes its +/// pid into the running state, and terminates it when the serve fails — so +/// exiting on our own would race that teardown and turn the scenario's expected +/// backend-install failure into a spurious "server exited" diagnosis. +fn run_lemond() -> ExitCode { + loop { + std::thread::sleep(Duration::from_mins(1)); + } +} + +fn run_lemonade_cli(args: &[String]) -> ExitCode { + if let Some(index) = args.iter().position(|arg| arg == "backends") { + if args.get(index + 1).map(String::as_str) == Some("install") { + // Fail loudly on stderr: the engine surfaces the child's output, and + // a scenario that somehow ran against a real runtime should be + // obvious in the log rather than look like a genuine install bug. + eprintln!( + "fake-lemonade: refusing to install a backend; this runtime was planted by the \ + E2E harness for @id:serve-lemonade-preparation-recovery" + ); + return ExitCode::FAILURE; + } + // Only the `llamacpp` rows are read, but print a plausible table rather + // than a single row so a parser change that starts depending on the + // header or on other recipes fails here instead of silently selecting + // nothing. + println!("Recipe Backend Status"); + println!("-------- -------- -----------"); + println!("llamacpp rocm installable"); + println!("llamacpp vulkan unsupported"); + return ExitCode::SUCCESS; + } + + if args.iter().any(|arg| arg == "status") { + return ExitCode::SUCCESS; + } + + // Any other subcommand: succeed quietly. The scenario never reaches model + // load, and failing here would mask the failure it is actually asserting. + ExitCode::SUCCESS +} diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index 628128a20..9447b7179 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -479,6 +479,90 @@ impl E2eWorld { self.legacy_rocm_path = Some(rocm); } + /// Plant a Lemonade runtime whose llama.cpp backend install always fails, so + /// `@id:serve-lemonade-preparation-recovery` can pin the retry count and the + /// terminal recovery guidance without downloading a real multi-gigabyte + /// runtime. + /// + /// This replaces a `#[cfg(feature = "e2e-test-hooks")]` seam that used to be + /// compiled into `rocm`. The seam made every full-suite lane test a binary + /// that differed from a release build; planting a runtime instead leaves + /// `rocm` byte-identical to what ships and moves the fake behind the + /// subprocess boundary the engine already talks to. + /// + /// Black-box, like [`Self::register_mock_service`]: the manifest is plain + /// JSON matching the engine's on-disk schema, not a typed import from the + /// engine crate. `resolve_runtime` accepts it as installed because it checks + /// only that the manifest parses and that `lemond` is a file — no checksum, + /// version, or signature is involved on this path, so nothing here weakens a + /// verification the product performs. + /// + /// `env_id` is the one field that cannot be a sentinel. Before `serve` + /// prepares an engine it calls `Detect` and skips the install only when the + /// reported `env_id` equals `lemonade-embeddable-{LEMONADE_VERSION}` — the + /// id `managed_engine_runtime_id` builds for the pinned version. A placeholder + /// there does not fail; it makes `serve` quietly run the REAL install, which + /// is why this scenario downloaded a 4.6 GB backend and then passed its serve. + /// Derive the id from `rocm_deps` rather than copying the string, so a + /// version bump cannot reintroduce that silent fallthrough. + /// + /// The runtime is planted under the scenario's own `data/engines/lemonade` + /// because this scenario does NOT call [`Self::use_shared_runtimes`]: with an + /// empty runtimes registry the CLI resolves no env root, so `lemonade_root` + /// is the isolated engine dir. Opting into the shared tree here would both + /// move that target and plant fake binaries in a tree other scenarios serve + /// from. + pub fn plant_failing_lemonade_runtime(&mut self) { + let root = self.isolated_root.as_ref().expect("no isolated root"); + let engine = root.path().join("data").join("engines").join("lemonade"); + let runtime_dir = engine.join("runtime"); + // `serve` writes running state and a startup log beside the manifest and + // does not create these itself outside the install path. + for dir in [ + &runtime_dir, + &engine.join("manifests"), + &engine.join("state"), + &engine.join("logs"), + ] { + std::fs::create_dir_all(dir) + .unwrap_or_else(|e| panic!("failed to create {}: {e}", dir.display())); + } + + let exe = |name: &str| { + runtime_dir.join(if cfg!(windows) { + format!("{name}.exe") + } else { + name.to_owned() + }) + }; + let (lemond, lemonade) = (exe("lemond"), exe("lemonade")); + // One fixture, copied under both names — it takes its role from argv[0]. + // Copied rather than linked so it works without the symlink privilege + // self-hosted Windows runners lack; `fs::copy` carries the Unix mode + // bits, so the copies stay executable. + for dest in [&lemond, &lemonade] { + std::fs::copy(env!("CARGO_BIN_EXE_fake-lemonade"), dest) + .unwrap_or_else(|e| panic!("failed to plant {}: {e}", dest.display())); + } + + let manifest = serde_json::json!({ + "env_id": format!("lemonade-embeddable-{}", rocm_deps::LEMONADE_VERSION), + "version": rocm_deps::LEMONADE_VERSION, + "runtime_dir": runtime_dir, + "lemond": lemond, + "lemonade": lemonade, + "backend_recipe": "llamacpp", + "backend_name": "rocm", + "installed_at_unix_ms": 0, + }); + let path = engine.join("manifests").join("runtime.json"); + std::fs::write( + &path, + serde_json::to_vec_pretty(&manifest).expect("manifest json"), + ) + .unwrap_or_else(|e| panic!("failed to write {}: {e}", path.display())); + } + /// Register the running mock server with the CLI by writing a managed-service /// record into the isolated services directory (`/services/`), exactly /// as `rocm serve --managed` would. This lets `rocm services list` and the diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index 6235ef8e9..687b6ec80 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -19,7 +19,13 @@ use crate::e2e::tui_driver::{TermSignal, TuiSession, default_timeout}; const MANAGED_MODEL_PROMPT: &str = "hello from the terminal"; /// File the daemon's test-only logical clock reads every cycle (see /// `rocm_dash_daemon::runner`'s `TestClockDirective` for the grammar). -const DASH_CLOCK_OFFSET_FILE: &str = "dash-clock-offset-secs"; +/// +/// Keep this path in step with `apps/rocm/src/dash.rs`'s +/// `dash_test_clock_offset_path`: `rocm dash` looks for exactly +/// `/telemetry/test-clock-offset` and falls back to wall time +/// when it is absent, so a rename on either side silently drops the whole +/// mechanism rather than failing loudly. +const DASH_CLOCK_OFFSET_FILE: &str = "test-clock-offset"; /// The Observe instances table's TTFT cell while the scripted mock is serving: /// its histogram pins time-to-first-token at exactly 50 ms @@ -1102,23 +1108,28 @@ async fn managed_model_scripted_metrics(world: &mut E2eWorld) { world.register_mock_service_with(ServiceRecordOptions::default()); } +/// Plant the clock file before the dashboard is launched. +/// +/// `rocm dash` decides once, at daemon construction, whether a logical clock is +/// in play — it takes the file's presence as the signal. So this must run before +/// the "opens the dashboard" step, which the scenarios guarantee by ordering +/// this `Given` ahead of them. #[given("dashboard observation time is deterministic")] async fn dashboard_observation_time_is_deterministic(world: &mut E2eWorld) { - let path = dash_clock_path(world); - write_dash_clock(&path, "0"); - world.command_env.push(( - "ROCM_CLI_DASH_TEST_CLOCK_OFFSET_PATH", - path.into_os_string(), - )); + write_dash_clock(&dash_clock_path(world), "0"); } -/// Path of this scenario's test-clock file, inside its isolated root. +/// Path of this scenario's test-clock file, inside the isolated data root the +/// CLI resolves from `ROCM_CLI_DATA_DIR` — no env var of its own, so the +/// binary under test carries no test-only branch. fn dash_clock_path(world: &E2eWorld) -> std::path::PathBuf { world .isolated_root .as_ref() .expect("scenario has no isolated root") .path() + .join("data") + .join("telemetry") .join(DASH_CLOCK_OFFSET_FILE) } @@ -1127,6 +1138,12 @@ fn dash_clock_path(world: &E2eWorld) -> std::path::PathBuf { /// write can be observed mid-update as an empty file; rename makes each /// directive visible all-at-once instead. fn write_dash_clock(path: &std::path::Path, directive: &str) { + // The telemetry state dir is created by `AppPaths::ensure()` on the first + // CLI run, which for these scenarios happens after this step. + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent) + .unwrap_or_else(|e| panic!("failed to create {}: {e}", parent.display())); + } let tmp = path.with_extension("tmp"); std::fs::write(&tmp, directive).expect("failed to stage the dashboard test clock"); std::fs::rename(&tmp, path).expect("failed to publish the dashboard test clock"); diff --git a/tests/e2e-cucumber/tests/e2e/serving_steps.rs b/tests/e2e-cucumber/tests/e2e/serving_steps.rs index e1515f24c..0dbacf98c 100644 --- a/tests/e2e-cucumber/tests/e2e/serving_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/serving_steps.rs @@ -792,15 +792,17 @@ async fn user_serves_vllm_capable_default(world: &mut E2eWorld) { #[given("Lemonade preparation cannot complete")] async fn lemonade_preparation_cannot_complete(world: &mut E2eWorld) { - world.command_env.push(( - "ROCM_E2E_LEMONADE_BACKEND_INSTALL_FAILURE", - "repeated".into(), - )); + // Plant a runtime whose `lemonade backends install` always exits non-zero, + // rather than compiling a failure seam into `rocm`. The CLI then fails at the + // same boundary a real broken install fails at — a non-zero child process — + // so what the scenario asserts is the product's real retry and recovery + // handling, not a test-only branch. + world.plant_failing_lemonade_runtime(); } #[when("the user serves a model with Lemonade")] async fn user_serves_with_failing_lemonade_preparation(world: &mut E2eWorld) { - let (stdout, stderr, rc) = crate::run_rocm_with_scenario_env( + let (stdout, stderr, rc) = crate::run_rocm( world, &[ "serve", @@ -1014,17 +1016,16 @@ async fn assert_lemonade_preparation_retry_is_bounded(world: &mut E2eWorld) { Some(0), "serve unexpectedly succeeded:\n{output}" ); - // The scripted failure seam also waives serve's no-GPU pre-flight, so this - // refusal can only appear when the seam is compiled out. Name that cause: - // otherwise a lane that pre-builds `rocm` without the feature reports a + // Serve enforces its GPU-required policy before it touches the runtime, so a + // host without a usable device never reaches Lemonade preparation at all. + // Name that cause: the scenario is `@requires-gpu` precisely because of this + // pre-flight, and a lane that runs it anyway would otherwise report a // baffling "no retry announcement" instead of its real misconfiguration. assert!( !output.contains("no usable AMD GPU detected"), - "serve stopped at the no-GPU pre-flight, so the binary under test was \ - built without the `rocm/e2e-test-hooks` feature and never reached \ - Lemonade preparation. E2E lanes that pre-build `rocm` and export \ - ROCM_CLI_BINARY must pass `--features rocm/e2e-test-hooks`, matching \ - what `cargo xtask e2e` builds for itself:\n{output}" + "serve stopped at the no-GPU pre-flight, so it never reached Lemonade \ + preparation. This scenario is tagged @requires-gpu and needs a host with \ + a usable AMD GPU:\n{output}" ); assert_eq!( output.matches("retrying once").count(), diff --git a/xtask/src/e2e.rs b/xtask/src/e2e.rs index 05cf29bbb..87d242a01 100644 --- a/xtask/src/e2e.rs +++ b/xtask/src/e2e.rs @@ -85,16 +85,7 @@ pub fn run(args: &[String]) -> Result<()> { if binaries.build_release { let status = Command::new(&cargo) - .args([ - "build", - "--release", - "-p", - "rocm", - "-p", - "rocmd", - "--features", - "rocm/e2e-test-hooks", - ]) + .args(["build", "--release", "-p", "rocm", "-p", "rocmd"]) .current_dir(&root) .status() .context("failed to run `cargo build --release -p rocm -p rocmd`")?; diff --git a/xtask/src/workflow_contract.rs b/xtask/src/workflow_contract.rs index 0e6a432ce..1fecc99ac 100644 --- a/xtask/src/workflow_contract.rs +++ b/xtask/src/workflow_contract.rs @@ -250,17 +250,17 @@ mod tests { } /// Every lane that pre-builds `rocm` and hands it to the suite via - /// `ROCM_CLI_BINARY` must enable the same test-hook feature `cargo xtask e2e` - /// enables when it builds for itself. + /// `ROCM_CLI_BINARY` must build it with exactly the invocation `cargo xtask + /// e2e` uses when it builds for itself. /// - /// The suite's deterministic failure seams (e.g. the scripted Lemonade - /// backend-install failure) are `#[cfg(feature = "e2e-test-hooks")]`. A lane - /// that omits the feature ships a binary in which those seams do not exist, - /// so the scenarios relying on them cannot reach their premise and fail as - /// regressions — but only on whichever lane happens to select them, which is - /// what made this divergence so hard to read the first time. Pin it here so a - /// new lane copying an existing block cannot silently reintroduce it. - fn assert_prebuilt_e2e_lanes_enable_test_hooks(workflow: &str, text: &str) { + /// Exporting the binaries only saves the second build if cargo would have + /// produced the same artifacts. Any divergence — package set, profile, + /// features — re-resolves the graph, and the lane silently pays a full + /// release rebuild on top of the one it already did. That is exactly the + /// regression #342 fixed on the Windows lane, where the two invocations had + /// drifted apart by one feature flag. Pin it here so a new lane copying an + /// existing block cannot reintroduce the drift. + fn assert_prebuilt_e2e_lanes_match_xtask_build(workflow: &str, text: &str) { let blocks: Vec<_> = multiline_run_blocks(text) .into_iter() .filter(|block| block.contains("ROCM_CLI_BINARY") && invokes_e2e(block)) @@ -270,14 +270,16 @@ mod tests { "{workflow} must contain prebuilt E2E run blocks" ); for block in blocks { + // Whole-line, not `contains`: a trailing `--features …` would + // satisfy a substring check while being exactly the drift this pins. assert!( - block.contains( - "cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks" - ), - "{workflow} prebuilt E2E lane must build with \ - `--features rocm/e2e-test-hooks`, matching what `cargo xtask e2e` \ - builds for itself; without it the suite's scripted failure seams \ - are compiled out:\n{block}" + block + .lines() + .any(|line| line.trim() == "cargo build --release -p rocm -p rocmd"), + "{workflow} prebuilt E2E lane must build exactly \ + `cargo build --release -p rocm -p rocmd`, matching what `cargo xtask \ + e2e` builds for itself; anything else re-resolves the graph and the \ + lane pays a second full release build:\n{block}" ); } } @@ -1241,15 +1243,15 @@ trigger-a-workflow#triggering-a-workflow-from-a-workflow" } #[test] - fn self_hosted_prebuilt_e2e_lanes_enable_test_hooks() { + fn self_hosted_prebuilt_e2e_lanes_match_xtask_build() { let workflow = read_workflow("e2e-selfhosted.yml"); - assert_prebuilt_e2e_lanes_enable_test_hooks("e2e-selfhosted.yml", &workflow); + assert_prebuilt_e2e_lanes_match_xtask_build("e2e-selfhosted.yml", &workflow); } #[test] - fn nightly_prebuilt_e2e_lanes_enable_test_hooks() { + fn nightly_prebuilt_e2e_lanes_match_xtask_build() { let workflow = read_workflow("nightly.yml"); - assert_prebuilt_e2e_lanes_enable_test_hooks("nightly.yml", &workflow); + assert_prebuilt_e2e_lanes_match_xtask_build("nightly.yml", &workflow); } #[test] @@ -1278,17 +1280,15 @@ trigger-a-workflow#triggering-a-workflow-from-a-workflow" /// Build step already produced. /// /// Without `ROCM_CLI_BINARY`, `cargo xtask e2e` builds `rocm`/`rocmd` for - /// itself WITH `--features rocm/e2e-test-hooks`. That is a different feature - /// resolution than the Build step's, so cargo rebuilds the entire release - /// graph instead of reusing it — a second 3-4 minute release build on the one - /// job that alone determines total CI wall clock. + /// itself. Cargo reusing the Build step's artifacts then depends on the two + /// invocations agreeing on profile, package set, features and target dir; a + /// one-flag drift cost this job a second 3-4 minute release build on every + /// run — the one job that alone determines total CI wall clock. Exporting the + /// paths removes the dependence on that agreement instead of restating it. /// - /// Note this is the mirror image of - /// [`assert_prebuilt_e2e_lanes_enable_test_hooks`]: lanes running the FULL - /// suite must build WITH the hooks, while this lifecycle-only lane must build - /// WITHOUT them. `E2E_ONLY_LIFECYCLE` keeps it to @lifecycle scenarios, none - /// of which use a scripted seam, and the lane packages and installs the - /// binary through the real installer — so it must ship what a release ships. + /// [`assert_prebuilt_e2e_lanes_match_xtask_build`] pins the same invariant + /// from the other side for the self-hosted and nightly lanes, which pre-build + /// and must therefore match `xtask`'s own invocation. #[test] fn ci_windows_lifecycle_lane_reuses_the_binaries_it_built() { let ci = read_workflow("ci.yml"); @@ -1347,12 +1347,6 @@ trigger-a-workflow#triggering-a-workflow-from-a-workflow" "the Windows lifecycle lane must fail fast, naming the missing binary, \ when the Build step did not produce it:\n{lifecycle}" ); - - assert!( - !lifecycle.contains("e2e-test-hooks"), - "the lifecycle-only lane packages and installs what a release ships, \ - so it must NOT carry the test hooks:\n{lifecycle}" - ); } // Extractor guards: prove the helpers actually parse multiline forms, so the From 7841012cd833bd82a7abc59971ab17dbec10f3c7 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Thu, 24 Sep 2026 07:34:32 +0000 Subject: [PATCH 2/2] fix(dash): warn when the test clock file steers a real dashboard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The logical-clock seam is state-based, so `dash_test_clock_offset_path` runs in every build, not only under the E2E harness. A stray copy of `telemetry/test-clock-offset` in a real data dir — synced from a dev or CI machine, or left behind by some future bug — therefore moves the dashboard off wall time with nothing to show for it, and the user has no way to explain the timestamps they are shown. Log at WARN when the file is found, naming it. It goes to `tracing` rather than stderr: the only caller runs on the `rocm dash` path where the TUI owns the terminal (see `logging.rs`), and a stray write there would corrupt the display — and would land on the PTY grid the dash scenarios assert against. Also pin the harness's copy of the file name and its `telemetry` subdirectory to production's. `apps/rocm` is a binary crate, so the suite cannot import the constant, and hosting it in a shared library would add a first-party crate edge for one string; instead a unit test reads the step file's source and fails in the every-PR lane if either side is renamed, which previously would have quietly dropped the whole mechanism. Declare the suite's `rocm-deps` dependency in the crate-edges allowlist that landed on main while this branch sat. Signed-off-by: Roman Inflianskas --- apps/rocm/src/dash.rs | 194 ++++++++++++++++++++- tests/e2e-cucumber/tests/e2e/dash_steps.rs | 20 ++- xtask/src/crate_edges.rs | 4 + 3 files changed, 212 insertions(+), 6 deletions(-) diff --git a/apps/rocm/src/dash.rs b/apps/rocm/src/dash.rs index 992a7a546..2966cdff2 100644 --- a/apps/rocm/src/dash.rs +++ b/apps/rocm/src/dash.rs @@ -92,9 +92,27 @@ const DASH_TEST_CLOCK_FILE: &str = "test-clock-offset"; /// whose file is absent leaves the daemon on its default /// `TestClockDirective::FreeRunning(0)` logical clock. Returning the path /// unconditionally would therefore take production off `Utc::now()`. +/// +/// Because this runs in every build, not just under the harness, a stray copy +/// of the file in a real data dir would otherwise move the dashboard onto a +/// logical clock with nothing to show for it. Finding the file is therefore +/// logged at WARN naming the path, so skewed timestamps are explained by the +/// log rather than having to be guessed at. The warning goes to `tracing`, not +/// stdout/stderr: the only caller is [`maybe_spawn_embedded_daemon`] on the +/// `rocm dash` path, where the TUI owns the terminal (see `logging.rs`) and a +/// stray write would corrupt the display. fn dash_test_clock_offset_path(paths: &AppPaths) -> Option { let path = paths.telemetry_state_dir().join(DASH_TEST_CLOCK_FILE); - path.is_file().then_some(path) + if !path.is_file() { + return None; + } + tracing::warn!( + test_clock_file = %path.display(), + "dashboard telemetry clock is in TEST mode, not wall time: displayed \ + timestamps come from a logical clock read from this file. Only the E2E \ + harness creates it; delete it to restore wall-clock timestamps." + ); + Some(path) } /// API key precedence — sourced from the environment ONLY (never TOML/CLI/source/ @@ -876,6 +894,180 @@ mod tests { } } + /// Source of the E2E step file that plants the clock directive, read at + /// compile time (test builds only) so + /// [`e2e_harness_plants_the_file_rocm_dash_reads`] can check its literals + /// against production's. Same cross-tree reach `e2e-cucumber`'s + /// `installer_fixture.rs` already makes for `install.ps1`. + const E2E_DASH_STEPS_SRC: &str = + include_str!("../../../tests/e2e-cucumber/tests/e2e/dash_steps.rs"); + + /// A private scratch directory under `target/`, matching the convention the + /// bench-parent test below already uses (no `tempfile` dev-dependency in + /// this crate). Named per test so parallel tests cannot collide. + fn scratch_dir(name: &str) -> PathBuf { + let dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("target") + .join(format!("{name}-{}", std::process::id())); + std::fs::remove_dir_all(&dir).ok(); + dir + } + + /// `AppPaths` rooted at a real directory, so the clock file's presence can + /// actually be varied (the shared [`paths`] fixture points at `/tmp`). + fn paths_at(root: &Path) -> AppPaths { + AppPaths { + config_dir: root.join("config"), + data_dir: root.join("data"), + cache_dir: root.join("cache"), + } + } + + /// In-memory `tracing` sink. + #[derive(Clone, Default)] + struct LogCapture(std::sync::Arc>>); + + impl LogCapture { + fn contents(&self) -> String { + String::from_utf8_lossy(&self.0.lock().unwrap()).into_owned() + } + } + + impl std::io::Write for LogCapture { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.0.lock().unwrap().extend_from_slice(buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } + } + + impl tracing_subscriber::fmt::MakeWriter<'_> for LogCapture { + type Writer = Self; + + fn make_writer(&self) -> Self::Writer { + self.clone() + } + } + + /// Run `f` under a capturing `tracing` subscriber and return what it wrote. + /// + /// `with_default` installs the subscriber for this thread only, so a + /// concurrently-running test in the same binary neither sees these records + /// nor leaks its own into them. + fn captured_logs(f: impl FnOnce()) -> String { + let capture = LogCapture::default(); + let subscriber = tracing_subscriber::fmt() + .with_writer(capture.clone()) + .with_ansi(false) + .finish(); + tracing::subscriber::with_default(subscriber, f); + capture.contents() + } + + /// The clock seam is state-based, so it is live in every build, not only + /// under the harness. A stray file in a real data dir therefore moves the + /// dashboard off wall time; finding one must say so, naming the file, or a + /// user has no way to explain the skewed timestamps they are shown. + #[test] + fn dash_test_clock_offset_path_warns_when_the_file_is_present() { + let root = scratch_dir("dash-clock-warn"); + let p = paths_at(&root); + std::fs::create_dir_all(p.telemetry_state_dir()).unwrap(); + let clock = p.telemetry_state_dir().join(DASH_TEST_CLOCK_FILE); + std::fs::write(&clock, "0").unwrap(); + + let mut resolved = None; + let logs = captured_logs(|| resolved = dash_test_clock_offset_path(&p)); + std::fs::remove_dir_all(&root).ok(); + + assert_eq!(resolved, Some(clock.clone())); + assert!(logs.contains("WARN"), "expected a WARN record, got: {logs}"); + assert!( + logs.contains(&clock.display().to_string()), + "the warning must name the offending file, got: {logs}" + ); + } + + /// The overwhelmingly common case — no file, wall time — must stay silent. + /// A warning on every launch would be noise, and would drain the signal out + /// of the one above. + #[test] + fn dash_test_clock_offset_path_is_silent_when_the_file_is_absent() { + let root = scratch_dir("dash-clock-silent"); + let p = paths_at(&root); + std::fs::create_dir_all(p.telemetry_state_dir()).unwrap(); + + let mut resolved = Some(PathBuf::new()); + let logs = captured_logs(|| resolved = dash_test_clock_offset_path(&p)); + std::fs::remove_dir_all(&root).ok(); + + assert_eq!(resolved, None, "an absent file must resolve to wall time"); + assert!( + logs.is_empty(), + "a wall-time launch must log nothing, got: {logs}" + ); + } + + /// `apps/rocm` is a binary crate, so the E2E harness cannot import + /// [`DASH_TEST_CLOCK_FILE`]; the file name and the `telemetry` + /// subdirectory are a second copy on the harness side, previously kept in + /// step by a comment alone. Renaming *or relocating* either side would not + /// fail — `rocm dash` would simply not find the planted file and quietly + /// use wall time, leaving the dash clock scenarios to time out on an + /// unrelated-looking symptom. Pin the two sides together here instead, in + /// the every-PR unit lane. + #[test] + fn e2e_harness_plants_the_file_rocm_dash_reads() { + let declared = E2E_DASH_STEPS_SRC + .split_once("const DASH_CLOCK_OFFSET_FILE: &str = \"") + .and_then(|(_, rest)| rest.split_once('"')) + .map(|(value, _)| value) + .expect("e2e dash_steps.rs no longer declares DASH_CLOCK_OFFSET_FILE as a literal"); + assert_eq!( + declared, DASH_TEST_CLOCK_FILE, + "the E2E harness plants `{declared}`, but `rocm dash` reads `{DASH_TEST_CLOCK_FILE}`" + ); + + // Check the directory chain inside the harness's own path builder, not + // across the whole step file: an unrelated `.join("telemetry")` + // elsewhere in it must not be able to satisfy this on its own. + let builder = E2E_DASH_STEPS_SRC + .split_once("fn dash_clock_path(") + .and_then(|(_, rest)| rest.split_once("\n}")) + .map(|(body, _)| body) + .expect("e2e dash_steps.rs no longer defines `fn dash_clock_path`"); + + // Every directory `rocm dash` puts between the data root and the clock + // file, in order, then the file itself. Taking the whole relative path + // rather than its last component means *relocating* the telemetry state + // dir is caught too, not only renaming its leaf. + let p = paths(); + let telemetry = p.telemetry_state_dir(); + let mut chain: Vec = telemetry + .strip_prefix(&p.data_dir) + .expect("the telemetry state dir lives under the data root") + .components() + .map(|c| format!(".join({:?})", c.as_os_str().to_string_lossy())) + .collect(); + chain.push(".join(DASH_CLOCK_OFFSET_FILE)".to_owned()); + + let mut cursor = 0; + for link in &chain { + let offset = builder[cursor..].find(link.as_str()).unwrap_or_else(|| { + panic!( + "`rocm dash` reads `{}`, but the E2E harness's `dash_clock_path` does not \ + build that path — expected `{link}` after the part already matched. \ + Harness body:\n{builder}", + telemetry.join(DASH_TEST_CLOCK_FILE).display() + ) + }); + cursor += offset + link.len(); + } + } + #[test] fn runner_options_wires_services_dir_to_registry() { let p = paths(); diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index 687b6ec80..d32db3f31 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -20,11 +20,21 @@ const MANAGED_MODEL_PROMPT: &str = "hello from the terminal"; /// File the daemon's test-only logical clock reads every cycle (see /// `rocm_dash_daemon::runner`'s `TestClockDirective` for the grammar). /// -/// Keep this path in step with `apps/rocm/src/dash.rs`'s -/// `dash_test_clock_offset_path`: `rocm dash` looks for exactly -/// `/telemetry/test-clock-offset` and falls back to wall time -/// when it is absent, so a rename on either side silently drops the whole -/// mechanism rather than failing loudly. +/// `rocm dash` looks for exactly `/telemetry/test-clock-offset` +/// and falls back to wall time when it is absent, so a rename or a move on +/// either side would drop the whole mechanism without failing: the dashboard +/// would just run on wall time and these scenarios would time out on a symptom +/// that points nowhere near the cause. +/// +/// `apps/rocm` is a binary crate, so this cannot import its +/// `DASH_TEST_CLOCK_FILE` — and hosting the constant in a library both sides +/// could depend on would add a first-party crate edge for one string (see +/// `xtask check-crate-edges`). The two copies are instead pinned to each other +/// by `dash::tests::e2e_harness_plants_the_file_rocm_dash_reads` in +/// `apps/rocm/src/dash.rs`, which reads this file's source and fails in the +/// every-PR unit lane. It needs this constant declared on one line, and needs +/// `dash_clock_path` below to keep that name and to keep building the path +/// from `.join(..)` links — otherwise the guard stops seeing this. const DASH_CLOCK_OFFSET_FILE: &str = "test-clock-offset"; /// The Observe instances table's TTFT cell while the scripted mock is serving: diff --git a/xtask/src/crate_edges.rs b/xtask/src/crate_edges.rs index 8c931ad66..dbd21fffb 100644 --- a/xtask/src/crate_edges.rs +++ b/xtask/src/crate_edges.rs @@ -67,6 +67,10 @@ const ALLOWLIST: &[(&str, &str)] = &[ ("xtask", "e2e-report"), ("xtask", "rocm-core"), ("e2e-cucumber", "e2e-report"), + // The E2E suite derives the planted Lemonade runtime's `env_id` from the + // same pinned version `rocm` builds its expected id from, rather than + // copying it — see the dependency's note in the suite's Cargo.toml. + ("e2e-cucumber", "rocm-deps"), ]; /// Subset of `cargo metadata --no-deps` output we consume: the manifest-level