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..2966cdff2 100644 --- a/apps/rocm/src/dash.rs +++ b/apps/rocm/src/dash.rs @@ -69,18 +69,50 @@ 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()`. +/// +/// 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); + 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/ @@ -862,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/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..d32db3f31 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -19,7 +19,23 @@ 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"; +/// +/// `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: /// its histogram pins time-to-first-token at exactly 50 ms @@ -1102,23 +1118,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 +1148,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/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 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