From 07e045f3929512078f394f8a45bf50ab11a601d3 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Wed, 30 Sep 2026 10:14:53 +0000 Subject: [PATCH 1/4] refactor(dash): read the test clock from state, not a compiled-in seam `rocm dash` resolved its logical observation clock through a `#[cfg(feature = "e2e-test-hooks")]` branch reading an env var, so the binary the dash scenarios exercised was not the binary that ships: the feature changed which code compiled. It now reads the directive from `/telemetry/test-clock-offset`, inside the data root it already resolves. Nothing in `rocm` creates that file; only the E2E harness plants one, in its isolated root. Every build compiles the same code, inert until the state exists. Because the read happens in every build, a stray copy of the file in a real data dir would move the dashboard off wall time with nothing to show for it. Finding it is therefore logged at WARN naming the path, through `tracing` rather than stderr: the only caller sits on the `rocm dash` path, where the TUI owns the terminal and a stray write would corrupt the display. The harness's filename and its whole directory chain are pinned against production's by a unit test that reads the step file's source, so a rename or a relocation on either side fails in the every-PR unit lane instead of silently dropping the mechanism and timing the scenarios out on a symptom that points nowhere near the cause. Signed-off-by: Roman Inflianskas --- apps/rocm/src/dash.rs | 222 ++++++++++++++++++++- tests/e2e-cucumber/tests/e2e/dash_steps.rs | 43 +++- 2 files changed, 249 insertions(+), 16 deletions(-) diff --git a/apps/rocm/src/dash.rs b/apps/rocm/src/dash.rs index 1c563c18f..ab508b933 100644 --- a/apps/rocm/src/dash.rs +++ b/apps/rocm/src/dash.rs @@ -75,7 +75,7 @@ pub fn runner_options( rocm_core::is_wsl_host(), rocm_core::has_usable_amd_gpu(), ), - test_clock_offset_path: dash_test_clock_offset_path(), + test_clock_offset_path: dash_test_clock_offset_path(paths), } } @@ -90,14 +90,46 @@ const fn gpu_reachable_for_preflight(is_wsl_host: bool, has_usable_gpu: bool) -> is_wsl_host && has_usable_gpu } -#[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/ @@ -1032,6 +1064,180 @@ mod tests { let _ = std::fs::remove_dir_all(&root); } + /// 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 a77ef4ce5..52aeb89b2 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 services overlay's own panel title, drawn by `draw_services_manager` on /// the overlay's border row. It is on screen exactly while the overlay is, so @@ -1312,23 +1328,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) } @@ -1337,6 +1358,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"); From 18b537e702a8bc577e80b14151fb8ae0afb4b69a Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Thu, 1 Oct 2026 10:28:46 +0000 Subject: [PATCH 2/4] docs(dash): say what the test clock actually skews, and how to undo it The WARN that now carries this seam's whole safety argument described a narrower symptom than the code produces and gave a remedy that silently does not work. The logical clock is not display-only. Its value is the cycle timestamp for counter and direct observation, the histogram average, the tracker snapshot and the snapshot timestamp, so it is the denominator of generation throughput, the window for latency averages and the basis of the Fresh/Held/expired verdict -- and it is written into persisted session records. A user whose throughput looks wrong is exactly the reader this warning exists for, and it steered them elsewhere. "Delete it to restore wall-clock timestamps" holds only on the next launch: the resolved path is captured once for the life of the run loop, and a read that fails -- including the file having been deleted -- leaves the directive frozen at its last value. Deleting mid-run pins the logical clock instead of restoring wall time. Prose only; the doc comment and test rationale are brought into the same wording, and no test pins the message string. Signed-off-by: Roman Inflianskas --- apps/rocm/src/dash.rs | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/apps/rocm/src/dash.rs b/apps/rocm/src/dash.rs index ab508b933..ffe46530b 100644 --- a/apps/rocm/src/dash.rs +++ b/apps/rocm/src/dash.rs @@ -112,9 +112,18 @@ const DASH_TEST_CLOCK_FILE: &str = "test-clock-offset"; /// /// 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 +/// logical clock with nothing to show for it. The clock is not display-only: +/// its value is the cycle timestamp for counter and direct observation, the +/// histogram average, the tracker snapshot and the snapshot's own timestamp, so +/// it is the denominator of generation throughput, the window for latency +/// averages and the basis of the Fresh/Held/expired verdict — and it is what +/// gets written into persisted session records. Finding the file is therefore +/// logged at WARN naming the path, so a user whose throughput numbers look +/// wrong is pointed at the cause rather than having to guess. The warning says +/// *restart* as well as remove, because the resolved path is captured once for +/// the life of the run loop and a read that fails — including the file having +/// been deleted — leaves the directive frozen at its last value, pinning the +/// logical clock rather than restoring wall time. 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. @@ -125,9 +134,12 @@ fn dash_test_clock_offset_path(paths: &AppPaths) -> Option { } 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." + "dashboard telemetry clock is in TEST mode, not wall time: a logical \ + clock read from this file drives displayed timestamps, the timestamps \ + written into persisted session records, and every figure derived from \ + them — generation throughput, latency averages and metric freshness. \ + Only the E2E harness creates it; remove it and restart the dashboard to \ + return to wall time (deleting it mid-run pins the logical clock instead)." ); Some(path) } @@ -1139,8 +1151,10 @@ mod tests { /// 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. + /// dashboard off wall time — not just in the timestamps on screen but in + /// throughput, latency averages, freshness and the session records written + /// to disk. Finding one must say so, naming the file, or a user has no way + /// to explain the numbers they are shown. #[test] fn dash_test_clock_offset_path_warns_when_the_file_is_present() { let root = scratch_dir("dash-clock-warn"); From 3a9dd4e66663f7d07abccd91e49efbb6ef2c626a Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 2 Oct 2026 12:57:20 +0000 Subject: [PATCH 3/4] test(dash): let the harness-sync guard survive a reflow, and document the clock file Two follow-ups from review on the state-based test clock. The cross-file guard read the harness's filename by matching the exact single-line spelling `const DASH_CLOCK_OFFSET_FILE: &str = "`, so a rustfmt reflow moving the value onto its own line made it panic as if the two copies had drifted. Anchor on the name and the first string literal after its `=` instead. Checked both ways: the reflowed form now passes, and a one-character change to the value still fails. A release build now reads `/telemetry/test-clock-offset` as a live input, and nothing under docs/ said so; the only trace was a WARN in a rotated log. Record it in release-trust.md next to the ComfyUI override, whose contrast is the point: that override is compiled out of release builds, and this file deliberately is not. Signed-off-by: Roman Inflianskas --- apps/rocm/src/dash.rs | 7 +++++- docs/release-trust.md | 29 ++++++++++++++++++++++ tests/e2e-cucumber/tests/e2e/dash_steps.rs | 7 +++--- 3 files changed, 39 insertions(+), 4 deletions(-) diff --git a/apps/rocm/src/dash.rs b/apps/rocm/src/dash.rs index ffe46530b..3a42b830f 100644 --- a/apps/rocm/src/dash.rs +++ b/apps/rocm/src/dash.rs @@ -1205,8 +1205,13 @@ mod tests { /// the every-PR unit lane. #[test] fn e2e_harness_plants_the_file_rocm_dash_reads() { + // Anchored on the name and the first string literal after its `=`, not on + // the exact single-line spelling: a rustfmt reflow that moves the value + // to its own line is not drift and must not fail this. let declared = E2E_DASH_STEPS_SRC - .split_once("const DASH_CLOCK_OFFSET_FILE: &str = \"") + .split_once("const DASH_CLOCK_OFFSET_FILE") + .and_then(|(_, rest)| rest.split_once('=')) + .and_then(|(_, rest)| rest.split_once('"')) .and_then(|(_, rest)| rest.split_once('"')) .map(|(value, _)| value) .expect("e2e dash_steps.rs no longer declares DASH_CLOCK_OFFSET_FILE as a literal"); diff --git a/docs/release-trust.md b/docs/release-trust.md index 3fa23afce..366a77058 100644 --- a/docs/release-trust.md +++ b/docs/release-trust.md @@ -244,6 +244,35 @@ the override *logic* does not exist at all in a build without environment entirely and unconditionally returns the hardcoded default URL, so a stray environment variable can never redirect a production install. +## Dashboard Test Clock File + +`rocm dash` checks for one test-only input that, unlike the override above, is +**not** compiled out of release builds: + +```text +/telemetry/test-clock-offset +``` + +When the file exists, the embedded telemetry daemon takes its observation clock +from the directive in it instead of wall time. That is deliberate: it lets the +E2E suite drive the dashboard across metric-validity boundaries +deterministically while testing the same binary that ships, rather than one +built with a test-only feature. + +Nothing in rocm-cli creates this file; only the E2E harness plants it, inside +its own isolated data root. If one is found, `rocm dash` logs a WARN naming the +path. Its effect is not limited to displayed timestamps: the clock drives +generation throughput, latency averages and metric freshness, and the +timestamps written into persisted session records. To return to wall time, +remove the file **and restart `rocm dash`** — the path is resolved once per +launch, and deleting the file mid-run leaves the clock pinned at its last +directive. + +The directive is an integer offset in seconds, `hold`, or `hold `; it can +skew telemetry but cannot select code or redirect a download. The data directory it lives in +already holds the runtime registry and service records, so write access to it +is already trusted. + ## Remaining Owner Step The repo still needs a real project-owned public signing key and matching diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index 52aeb89b2..4ae1276a7 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -32,9 +32,10 @@ const MANAGED_MODEL_PROMPT: &str = "hello from the terminal"; /// `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. +/// every-PR unit lane. It needs this constant's value to stay a plain string +/// literal (formatting is free; a `concat!` is not), 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 services overlay's own panel title, drawn by `draw_services_manager` on From 85178e71b951fbca7b98cf1d8d89f1d24efe56ef Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 2 Oct 2026 14:15:54 +0000 Subject: [PATCH 4/4] refactor(dash): resolve the test clock file in one place, and prove it is read The clock filename was written twice, once in `rocm dash` and once in the E2E harness, and kept in step by a unit test that `include_str!`-ed the harness source and scraped the constant and the `.join(..)` chain out of it. That was only needed because `apps/rocm` is a binary crate nothing can import. The harness now has a `rocm-core` dev-dependency (#419), and dev edges are exempt from `check-crate-edges`, so put the path in one place instead: `AppPaths::dash_test_clock_file()`. `rocm dash` reads through it and the harness plants through it, so the two cannot drift. The scraping guard and its fragility (a reflow, a `concat!`, a moved step file) go with it. Checking the scenarios against this exposed a gap the guard had been standing in for. dash-08 and dash-09 do not show that the dashboard is on the logical clock: with `rocm dash` made to ignore the file, dash-09 still passes, because the 6 s validity window simply elapses on wall time inside the expiry step's wait. The held-clock assertions then hold only by timing, which is the flake the clock exists to remove. The "held" step now first waits for the WARN `rocm dash` logs only when it finds the planted file, matched on the exact path. With the same mutant, dash-09 now fails at that step and names the file the dashboard never picked up. dash-08 and dash-09 pass locally against a plain build, with no test-only feature. Signed-off-by: Roman Inflianskas --- apps/rocm/src/dash.rs | 77 +------------------ crates/rocm-core/src/lib.rs | 13 ++++ tests/e2e-cucumber/tests/e2e/dash_steps.rs | 87 +++++++++++++++------- 3 files changed, 77 insertions(+), 100 deletions(-) diff --git a/apps/rocm/src/dash.rs b/apps/rocm/src/dash.rs index 3a42b830f..e3af6494d 100644 --- a/apps/rocm/src/dash.rs +++ b/apps/rocm/src/dash.rs @@ -90,9 +90,6 @@ const fn gpu_reachable_for_preflight(is_wsl_host: bool, has_usable_gpu: bool) -> is_wsl_host && has_usable_gpu } -/// Name of the logical-clock directive file, read from the telemetry state dir. -const DASH_TEST_CLOCK_FILE: &str = "test-clock-offset"; - /// 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 @@ -128,7 +125,7 @@ const DASH_TEST_CLOCK_FILE: &str = "test-clock-offset"; /// `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); + let path = paths.dash_test_clock_file(); if !path.is_file() { return None; } @@ -1076,14 +1073,6 @@ mod tests { let _ = std::fs::remove_dir_all(&root); } - /// 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. @@ -1160,7 +1149,7 @@ mod tests { 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); + let clock = p.dash_test_clock_file(); std::fs::write(&clock, "0").unwrap(); let mut resolved = None; @@ -1195,68 +1184,6 @@ mod tests { ); } - /// `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() { - // Anchored on the name and the first string literal after its `=`, not on - // the exact single-line spelling: a rustfmt reflow that moves the value - // to its own line is not drift and must not fail this. - let declared = E2E_DASH_STEPS_SRC - .split_once("const DASH_CLOCK_OFFSET_FILE") - .and_then(|(_, rest)| rest.split_once('=')) - .and_then(|(_, rest)| rest.split_once('"')) - .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/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index c9d096f50..ff2ef533d 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -1942,6 +1942,19 @@ impl AppPaths { self.data_dir.join("telemetry") } + /// The directive file that moves `rocm dash`'s telemetry daemon off wall time + /// and onto a logical observation clock (see `docs/release-trust.md`). + /// + /// `rocm dash` reads it in every build, and nothing in rocm-cli creates it — + /// only the E2E harness plants one, in its isolated data root. Both resolve it + /// through this one method, so the path the harness plants and the path the + /// dashboard reads cannot drift apart. If they did, the dashboard would + /// silently stay on wall time, and the clock-driven scenarios would not fail + /// outright: they would keep passing, but only by timing. + pub fn dash_test_clock_file(&self) -> PathBuf { + self.telemetry_state_dir().join("test-clock-offset") + } + /// Log file for the rocm-dash telemetry daemon, under the shared logs dir. /// /// Deliberately under the canonical `AppPaths` data root diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index 4ae1276a7..2ccc4bb41 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -17,26 +17,6 @@ use crate::e2e::tui_driver::{TermSignal, TuiSession, default_timeout}; /// corresponding `Then` step (`managed_chat_request_carried_prompt`) asserts /// the mock actually received — so the two can never silently drift apart. 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). -/// -/// `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's value to stay a plain string -/// literal (formatting is free; a `concat!` is not), 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 services overlay's own panel title, drawn by `draw_services_manager` on /// the overlay's border row. It is on screen exactly while the overlay is, so @@ -1343,15 +1323,70 @@ async fn dashboard_observation_time_is_deterministic(world: &mut E2eWorld) { /// 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. +/// +/// Resolved through `rocm_core::AppPaths::dash_test_clock_file`, the same method +/// `rocm dash` reads it with, so where the harness plants the file and where the +/// dashboard looks for it cannot drift apart. Only the data root is the harness's +/// own: it is the directory this suite hands the CLI as `ROCM_CLI_DATA_DIR`. fn dash_clock_path(world: &E2eWorld) -> std::path::PathBuf { - world + isolated_app_paths(world).dash_test_clock_file() +} + +/// The `AppPaths` the CLI under test resolves for this scenario: the same roots +/// [`crate::E2eWorld::isolate_env`] hands it as `ROCM_CLI_*_DIR`. +fn isolated_app_paths(world: &E2eWorld) -> rocm_core::AppPaths { + let root = world .isolated_root .as_ref() .expect("scenario has no isolated root") - .path() - .join("data") - .join("telemetry") - .join(DASH_CLOCK_OFFSET_FILE) + .path(); + rocm_core::AppPaths { + config_dir: root.join("config"), + data_dir: root.join("data"), + cache_dir: root.join("cache"), + } +} + +/// Wait until `rocm dash`'s own client log records that it found the planted +/// clock file — the WARN it logs only when it switches off wall time. +/// +/// Without this, nothing proves the dashboard under test is on the logical +/// clock at all. Measured: with `rocm dash` made to ignore the file, dash-09 +/// still passes, because the 6 s validity window simply elapses on wall time +/// inside the expiry step's wait. The held-clock assertions then hold only by +/// timing, which is the flake the clock exists to remove. Checking the log is +/// deterministic: the line names the exact file, or it never appears. +/// +/// Polled rather than read once: the client log is written by a non-blocking +/// appender, so the line can trail the launch slightly. +/// +/// Takes the resolved paths rather than the world: a `&E2eWorld` held across the +/// `.await` below would need `E2eWorld: Sync`, which it is not. +async fn assert_dashboard_reads_the_planted_clock(paths: rocm_core::AppPaths) { + let clock = paths.dash_test_clock_file().display().to_string(); + let log_dir = paths.client_log_dir(); + let deadline = std::time::Instant::now() + default_timeout(); + loop { + let found = std::fs::read_dir(&log_dir) + .into_iter() + .flatten() + .filter_map(|entry| std::fs::read_to_string(entry.ok()?.path()).ok()) + .any(|log| { + log.lines() + .any(|line| line.contains("TEST mode") && line.contains(&clock)) + }); + if found { + return; + } + assert!( + std::time::Instant::now() < deadline, + "`rocm dash` never logged that it found the planted clock file {clock} \ + (searched {}), so it is on wall time and the held-clock assertions \ + below would pass or fail by timing alone", + log_dir.display() + ); + tokio::time::sleep(std::time::Duration::from_millis(250)).await; + } } /// Publish a clock directive atomically (write a sibling temp file, then @@ -1419,6 +1454,8 @@ async fn positive_gen_tps_displayed(world: &mut E2eWorld) { /// 6 s window with margin, and it stays there. #[when("dashboard observation time is held")] async fn dashboard_observation_time_is_held(world: &mut E2eWorld) { + // Holding a clock the dashboard never picked up would do nothing, silently. + assert_dashboard_reads_the_planted_clock(isolated_app_paths(world)).await; write_dash_clock(&dash_clock_path(world), "hold"); }