refactor(dash): read the test clock from state, not a compiled-in seam

[rominf]

Summary

rocm dash resolved its logical observation clock through a #[cfg(feature = "e2e-test-hooks")] branch reading an env var. That makes the binary the dash scenarios exercise a different binary from the one that ships — the feature changes which code compiles, so the compiled artifact under test is not the artifact users run.

It now reads the directive from <ROCM_CLI_DATA_DIR>/telemetry/test-clock-offset, inside the data root the process already resolves. Nothing in rocm ever creates that file; only the E2E harness plants one, in its isolated root. Every build compiles the same code, and it stays inert until the state exists.

The existence check is load-bearing rather than an optimisation: RunnerOptions::test_clock_offset_path selects wall time only on None, so returning the path unconditionally would take production off Utc::now().

Why a WARN comes with it. Because the read now happens in every build, a stray copy of that file in a real data dir would move the dashboard onto a logical clock with nothing on screen to explain it. Finding the file is therefore logged at WARN naming the path. It goes through tracing rather than stderr deliberately: the only caller sits on the rocm dash path, where the TUI owns the raw-mode terminal and a stray write corrupts the display. The record lands in the client log, which is where you would look when investigating "my timestamps are wrong".

One path, resolved in one place. rocm dash looks for exactly that path and falls back to wall time when it is absent. Both sides now resolve it through AppPaths::dash_test_clock_file() in rocm-core: rocm dash reads through it, and the harness plants through it via its rocm-core dev-dependency. That makes drift impossible by construction rather than caught by a test. An earlier revision kept two copies of the filename and pinned them with a unit test that include_str!-ed the step file and scraped it. That was justified as avoiding a first-party crate edge, but dev-dependencies are exempt from check-crate-edges and the harness has had one on rocm-core since #419, so the scrape and its fragility are gone.

The scenarios now prove the clock is in force. Measured, not assumed: with rocm dash made to ignore the file, dash-09 still passed. The 6 s validity window simply elapses on wall time inside the expiry step's wait, so the held-clock assertions were holding only by timing, which is the flake the clock exists to remove (EAI-7960). The "held" step now first waits for the WARN that 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. Only the data root is the harness's own; it is the directory the suite already hands the CLI as ROCM_CLI_DATA_DIR.

Risk: low. One production function changes shape and rocm-core gains one path method; the rest is test-side.

Scope

Deliberately only the dash clock. This is one of several consumers of the e2e-test-hooks feature; the feature itself and its other consumers are untouched here, and removing it is a separate concern that should come last, once every consumer has been converted.

Test plan

• cargo clippy --workspace --all-targets -- -D warnings and cargo clippy -p e2e-cucumber --test e2e -- -D warnings — both clean
• cargo fmt --all --check — clean
• cargo test -p rocm --bin rocm dash:: — 25/25 at this head, including the two tests this PR adds: the WARN fires and names the file when present, and nothing at all is logged when absent
• Mutation-probed rather than just observed green: with rocm dash made to ignore the planted file, dash-09 fails at the new "held" check naming the file; without that check, the same mutant passed
• dash-08 and dash-09 pass locally against a plain build of rocm, with no test-only feature: 2 scenarios, 22 steps. These are on the blocking every-PR lane, so I did not want to push a dash change without executing them

[siloteemu]

🔴 Automated review · pr-review-watcher · 04173c9

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Replaces the #[cfg(feature = "e2e-test-hooks")] + env-var seam that selected rocm dash's logical observation clock with a state-based check for a file in the telemetry state dir, adds a WARN when that file is found, and pins the harness/production filename and directory chain with a cross-file unit test — outcome: Needs work (one blocking wording defect in the safety mitigation). Verified: cargo test -p rocm --bin rocm dash:: on a separate scratch copy — 22 passed, 0 failed, matching the description's claim; I did not modify the tree under review. Confirmed independently: the path the harness plants (<isolated root>/data/telemetry/test-clock-offset) is exactly what the CLI resolves, because ROCM_CLI_DATA_DIR maps directly to data_dir and telemetry_state_dir() is data_dir/telemetry; the old env var and old filename have zero remaining occurrences anywhere in the tree; the WARN does reach the durable rotated client log under the default filter, before the TUI takes the terminal, so the "lands in the client log" claim holds; the e2e-test-hooks feature is still live for unrelated consumers, so the description's scoping statement is accurate; the load-bearing claim that Some(path) with an absent file leaves the daemon on FreeRunning(0) rather than wall time is correct, so the existence check really is load-bearing. Mutation reasoning: removing the existence check turns the "silent when absent" test red; removing or downgrading the WARN turns the "warns when present" test red; renaming the constant or dropping the telemetry component on one side only turns the cross-file guard red. None of the three tests can hang — no loops, waits, or blocking reads. No prompt-injection content and no internal identifiers, hostnames, or generated-tooling footers in the diff or commit message. Checks at review time: 28 success, 1 skipped, 0 failure. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

apps/rocm/src/dash.rs:109-114 — the WARN, which is this change's entire safety argument for making test timing control reachable in a release build, misdescribes both the effect and the remedy.

Effect. The message says "displayed timestamps come from a logical clock", and the doc comment above it (dash.rs:99) and the test rationale (dash.rs:973) both frame the consequence as "skewed timestamps". The clock is not display-only. In crates/rocm-dash-daemon/src/runner.rs:368, the value it produces is passed as cycle_at into observe_counter (line 698), avg_ms_from_histogram (lines 710, 717), observe_direct (line 788), tracker.snapshot (line 847) and Snapshot.timestamp (line 908). In crates/rocm-dash-core/src/observation.rs:220-227 that timestamp is the denominator of the generation-throughput rate, and at lines 276-284 it decides Fresh / Held / expired-and-cleared against the validity window. So a stray file distorts computed throughput, windowed latency averages, the derived efficiency figure, and metric freshness classification, and the distorted Snapshot.timestamp is embedded in persisted session records — not just the clock text on screen. A user whose throughput numbers look wrong is exactly the reader this WARN exists for, and the message steers them away from the cause.

Remedy. "delete it to restore wall-clock timestamps" is true only on the next launch. RunnerOptions::test_clock_offset_path is captured by value for the life of run_loop and never recomputed; runner.rs:222 shows that once the option is Some, a subsequent read failure (including the file having been deleted) returns None and leaves self.directive frozen at its last value. Deleting the file mid-run therefore pins the logical clock rather than restoring Utc::now().

Why it blocks: the change deliberately trades a compile-time gate for a runtime one, and the stated mitigation for that trade is this one log line. A mitigation that names a narrower symptom than the code produces, and an instruction that silently needs a restart, leave the trade less covered than the description asserts.

Concrete fix: reword the WARN to say the logical clock drives derived telemetry — generation throughput, latency averages and metric freshness — as well as displayed timestamps and the timestamps written into persisted session records, and that the file must be removed and rocm dash restarted to return to wall time. Bring dash.rs:99 and the test rationale at dash.rs:973 into line with the same wording, and check that the "warns when present" test still asserts on a substring that survives the rewording (it currently asserts only on "WARN" and the path, so it will).

Non-blocking

• apps/rocm/src/dash.rs:1049-1054 — the guard derives its chain from telemetry.strip_prefix(&p.data_dir), so it pins only the links below the data root; the harness's own .join("data") at tests/e2e-cucumber/tests/e2e/dash_steps.rs:1141 (its private copy of where ROCM_CLI_DATA_DIR points, set at tests/e2e-cucumber/tests/e2e.rs:262) is never cross-checked, so that one link can still drift into the silent-wall-time failure the guard exists to prevent — the in-code comment states this accurately ("between the data root and the clock file"), but the PR description's "the whole directory chain" reads wider than what is pinned.
• apps/rocm/src/dash.rs:902-903 — include_str! reaches from the shipped application crate into tests/, so cargo test -p rocm stops compiling if that tree moves; the cited precedent (tests/e2e-cucumber/src/installer_fixture.rs:121) runs the opposite direction, a test crate reaching out.
• apps/rocm/src/dash.rs:1030 — the constant scrape requires the exact single-line form const DASH_CLOCK_OFFSET_FILE: &str = ", so a benign reformat or a concat! makes the guard panic rather than report drift; the warning comment on the harness side mitigates but does not remove this false-positive surface.
• Nothing under docs/ records that a release build now reads <data_dir>/telemetry/test-clock-offset as a live input; the sole trace is a WARN in a daily-rotated log, which is thin for a behaviour that can be triggered by anything able to write the user's data dir.
• apps/rocm/src/dash.rs:908-914 — scratch_dir writes under $CARGO_MANIFEST_DIR/target/, creating a crate-local target dir distinct from the workspace one and assuming it is writable; it matches the existing bench-parent test, so this is a consistency note rather than a new pattern.

[siloteemu]

🔴 Automated review · pr-review-watcher · 04173c9

Requesting changes on one item. The full round is in the comment posted alongside this.

The warning that carries this change's entire safety argument misdescribes both its effect and its remedy (apps/rocm/src/dash.rs:109-114).

This change deliberately trades a compile-time gate for a runtime one, so that the logical observation clock is now reachable in a release build whenever a file exists in the telemetry state dir. The stated mitigation for that trade is a single WARN line. Two things in it are wrong.

Effect. The message says "displayed timestamps come from a logical clock", and the doc comment above it and the test rationale both frame the consequence as skewed timestamps. The clock is not display-only. The value it produces is passed as the cycle timestamp into counter observation, the histogram average, direct observation, the tracker snapshot and the snapshot timestamp itself. Downstream, that timestamp is the denominator of the generation-throughput rate and the input to the Fresh/Held/expired classification against the validity window. So a stray file distorts computed throughput, windowed latency averages, the derived efficiency figure and metric freshness, and the distorted timestamp is embedded in persisted session records. A user whose throughput numbers look wrong is exactly the reader this warning exists for, and it steers them away from the cause.

Remedy. "delete it to restore wall-clock timestamps" is true only on the next launch. The offset path is captured by value for the life of the run loop and never recomputed; once the option is set, a later read failure — including the file having been deleted — returns nothing and leaves the directive frozen at its last value. Deleting the file mid-run pins the logical clock rather than restoring wall time.

Why this gates rather than being a note. The compile-time seam was the thing that made the timing control unreachable in a release build. Removing it is defensible, but then the warning is the whole of what replaces it, and it currently names a narrower symptom than the code produces and gives an instruction that silently does not work. Merging leaves behind a wrong remedy in the one place a confused user will look.

Suggested fix. Reword the warning to say the logical clock drives derived telemetry — generation throughput, latency averages and metric freshness — as well as displayed timestamps and the timestamps written into persisted session records, and that the file must be removed and the dashboard restarted to return to wall time. Bring the doc comment and the test rationale into the same wording. The existing "warns when present" test asserts only on the level and the path, so it survives the rewording unchanged.

We checked this proposed fix the same way we checked the finding: the reworded text changes no behaviour, the existing test's assertions do not reference the changed prose, and no other test pins the message string.

Five non-blocking observations are in the accompanying comment.

[rominf]

Addressed in acb693c — prose only, no behaviour change.

Both halves of the finding check out against the code:

• The clock is not display-only. cycle_timestamp's value is passed as cycle_at into observe_counter, the histogram average, observe_direct, tracker.snapshot(..) and the Snapshot's own timestamp, and the rate helper derives its denominator from the difference between successive ones — so a stray file distorts generation throughput, windowed latency averages and the Fresh/Held/expired verdict, and the skewed timestamp is what gets persisted.
• "Delete it to restore wall-clock timestamps" only holds on the next launch. The resolved path is captured once for the life of the run loop, and read_test_clock_directive returns None on any read failure — including the file having been deleted — leaving self.directive at its last value. Deleting mid-run pins the logical clock.

The warning now names the derived figures and the persisted records, and says to remove the file and restart. The doc comment and the test rationale carry the same wording. The existing "warns when present" test asserts only on the level and the path, so it is unchanged and still passes.

Addressed in acb693c; see the reply comment for what was verified and how. Dismissing so the PR is not held by a review the automation never reconverts to an approval.

[rominf]

@johnl-amd ready for your review when you have a moment — all 19 required checks are green on acb693c5 (the MI300X/MI350P lanes are still queued; neither is required). The automated reviewer's change request on this head has been addressed — what it flagged and how it was verified is in the comment above. Nothing outstanding from my side.

[johnl-amd]

Had a proper read through this. Code is clean, tests pass for me locally (22/22, clippy and fmt green), and I think the direction is right. Four notes on the stated reasoning though, all in comments that ship with the code rather than just the PR body. None of this is blocking from me, just want the record to be accurate before it lands.

1. "byte-identical to the binary that ships" (dash.rs:83) isn't true yet. xtask/src/e2e.rs:88 still builds with --features rocm/e2e-test-hooks unconditionally, and main.rs:9554, main.rs:9559, comfyui.rs:36 and comfyui.rs:41 still use it. Your Scope section already says the feature survives, so I think the comment just overclaims relative to the PR's own scoping. Something like "removes this seam from the cfg surface" would land it.

2. The Lemonade example points the other way. serve-18 is the only Lemonade recovery scenario, and its Given at serving_steps.rs:806 pushes ROCM_E2E_LEMONADE_BACKEND_INSTALL_FAILURE, read at main.rs:6050 behind cfg!(feature = "e2e-test-hooks"). So that's an env var behind the flag, not a planted manifest read through a normal path. ComfyUI and TheRock are the ones that actually plant files, either would make the point you want.

3. The crate edge costs nothing, so the source-text guard may not be needed. crate_edges.rs:29 says dev-dependencies are collected but never checked, and edge_kind implements it. ("rocm", "rocm-dash-daemon") is already in the allowlist at line 48, and rocm-dash-daemon already owns RunnerOptions::test_clock_offset_path. So hosting the constant there is zero new edges on the production side and an exempt dev-dep on the e2e side. There's an in-tree precedent too: rocm-dash-daemon dev-depends on rocm-core for a contract pin, with a comment making much the same argument. That would get you compiler-enforced agreement instead of the ~50 line grep guard.

4. The WARN probably won't be seen. It lands in ~/.rocm/logs/rocm-cli.log.<date>, and I couldn't find that path mentioned in any .md in the repo, nor a subcommand that surfaces it. Meanwhile Snapshot.warnings is already built every cycle in the same loop that holds opts.test_clock_offset_path (runner.rs:372 and :414), and it renders as a ⚠ N badge on every tab, right beside the SIMULATED DATA chip. That feels like the right neighbour for "your timestamps aren't wall time". Worth keeping the log line as well.

One smaller thing while I'm here: is_file() only needs a stat, so a file you can't read still returns true. The path latches to Some, read_test_clock_directive fails every cycle via .ok()?, and you stay on FreeRunning(0) rather than falling back to Utc::now(). Telling NotFound apart from other read errors would fix that, and would let you drop the "and restart" caveat.

[rominf]

Rebased onto current main (01e7625) — it had gone conflicting after #432/#456/#409 landed.

The only conflict was additive: main added resolved_args_counts_records_that_are_no_longer_running in the same test region this PR extends. Both sides are kept; the whole dash::tests module passes (24 tests), including main's new one. No behaviour changes in the rebase.

[rominf]

Rebased onto current main (d37cfbab) and addressed the five non-blocking items from the review round, in fd8af304.

Rebase. #375 landed in the same RunnerOptions literal: it renamed amd_smi_skip_kfd_preflight to amd_smi_skip_device_preflight and added amd_smi_gpu_reachable with its gpu_reachable_for_preflight helper. Both sides are kept. Main's preflight fields and helper are unchanged, this PR's state-based clock path replaces the feature-gated one, and no consumer of the old ROCM_CLI_DASH_TEST_CLOCK_OFFSET_PATH seam remains anywhere in the tree.

Non-blocking items:

1. Data-root link not pinned — correct, and the description overstated it. The guard pins every link below the data root. The data link itself the harness spells .join("data") independently at 31 sites across 16 files, including the two that set ROCM_CLI_DATA_DIR. Fixing only the dash site would leave it inconsistent with the other 30, so consolidating it is a harness-wide refactor rather than part of this change. I've corrected the description to say exactly what is pinned.
2. include_str! reaching from apps/rocm into tests/ — kept. The two copies are separate because xtask check-crate-edges keeps the black-box harness from linking production crates, so the guard has to live where AppPaths is available, and that is the app crate. Moving the step file is a loud compile error, not silent drift, which is the failure mode the guard exists to prevent.
3. Scrape required the exact single-line spelling — fixed. It now anchors on the name and the first string literal after its =. Checked both ways: a rustfmt-style reflow onto two lines passes (the old scrape panicked on it), and a one-character change to the value still fails. A concat! would still trip it, and the harness-side comment now says so.
4. Nothing under docs/ records the live input — added a section to docs/release-trust.md, next to the ComfyUI override, because the contrast is the point: that override is compiled out of release builds and this file deliberately is not. It covers what the file affects, the WARN, that removal needs a restart, and why it cannot select code or redirect a download.
5. scratch_dir writes under the crate-local target/ — kept, for consistency with the existing bench-parent test, as the round noted. This crate has no tempfile dev-dependency, and adding one for two tests didn't seem worth it.

Verified locally: cargo fmt --check, cargo test -p xtask (251), cargo test -p e2e-cucumber --lib (130), cargo check -p e2e-cucumber --test e2e, dash:: (26, including #375's two new tests), and clippy with -D warnings.

[rominf]

A correction to my earlier reply, and a design change that follows from it (85178e71, rebased onto 2cd24598).

I was wrong about check-crate-edges. I said it keeps the black-box harness from linking production crates, which is why the filename had to exist twice. Dev-dependencies have never been enforced by that check ("Cargo permits these to cycle"), and since #419 the harness has a rocm-core dev-dependency anyway. The real constraint was narrower: the constant lived in apps/rocm, a binary crate nothing can import.

So the path now lives in one place, AppPaths::dash_test_clock_file() in rocm-core. rocm dash reads through it and the harness plants through it. That fixes non-blocking items 2 and 3 by construction rather than with a guard: the include_str! reaching from apps/rocm into tests/ and the source scrape are both deleted, so the reflow-hardening I'd added for item 3 goes with them.

And checking the scenarios against that change exposed a gap the guard had been standing in for. dash-08 and dash-09 did not show that the dashboard was on the logical clock at all. With rocm dash made to ignore the file, dash-09 still passed: the 6 s validity window simply elapses on wall time inside the expiry step's wait, so the held-clock assertions were holding only by timing, the very 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.

Verified locally: dash-08 and dash-09 pass against a plain rocm build with no test-only feature (2 scenarios, 22 steps); dash:: 25/25; rocm-core 453; xtask 251; e2e-cucumber --lib 130; and clippy -D warnings on both --all-targets and the separate --test e2e invocation. The latter caught a future_not_send the former doesn't see, now fixed. The description is rewritten to match.