diff --git a/apps/rocm/src/dash.rs b/apps/rocm/src/dash.rs index 1c563c18f..e3af6494d 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,55 @@ 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) -} - -#[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. 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. +fn dash_test_clock_offset_path(paths: &AppPaths) -> Option { + let path = paths.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: 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) } /// API key precedence — sourced from the environment ONLY (never TOML/CLI/source/ @@ -1032,6 +1073,117 @@ mod tests { let _ = std::fs::remove_dir_all(&root); } + /// 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 — 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"); + let p = paths_at(&root); + std::fs::create_dir_all(p.telemetry_state_dir()).unwrap(); + let clock = p.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}" + ); + } + #[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/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 a77ef4ce5..2ccc4bb41 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -17,9 +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). -const DASH_CLOCK_OFFSET_FILE: &str = "dash-clock-offset-secs"; /// 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,24 +1309,84 @@ 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. +/// +/// 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(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 @@ -1337,6 +1394,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"); @@ -1391,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"); }