test(e2e): retry the Observe tab key until the dashboard acts on it - #263
Conversation
Every dashboard scenario that opens Observe sends the `4` tab key straight after launching the TUI, with no assertion in between. A key written into the pseudo-terminal before the dashboard is reading input can be consumed by whatever holds the terminal at that moment, and nothing ever retries it — so the dashboard stays on Home and the scenario fails 30s later in an assertion about a view it never left. Send the key until the Observe chip is actually marked active, so the step depends on the dashboard having acted on the key rather than on it having been ready when the key was written. Reproduced by pointing ROCM_CLI_BINARY at a wrapper that drains the terminal before exec'ing the real binary: the scenario failed with exactly the CI symptom before this change and passes after it. Split out of #165, where it was unrelated to that PR's subject. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
|
CI status: all 16 required checks are green, including the blocking mock
The dashboard scenarios this PR is about all passed on that GPU runner, including every step that goes through the changed code path: The two Strix Halo lanes are still queued behind other branches' runs on the shared self-hosted runners; I'll follow up here if either reports anything attributable to this change. |
Signed-off-by: Michael Roy <michael.roy@amd.com>
…-049b Signed-off-by: Michael Roy <michael.roy@amd.com>
Signed-off-by: Michael Roy <michael.roy@amd.com>
Signed-off-by: Michael Roy <michael.roy@amd.com>
michaelroy-amd
left a comment
There was a problem hiding this comment.
- Review focus: Please review the bounded idempotent resend loop and atomic reader-failure handoff so startup retries cannot hide terminal failures or send after the session is dead.
- Problem / impact: TUI E2E sent
4before the application necessarily read from the PTY; a swallowed key left the dashboard on Home and produced a late, misleading Observe assertion failure. Reader/parser failures near a retry boundary could also be consumed, then replaced by a generic timeout. - State: #263, head
bf5cf1dd89126087f7b8f09aea30e62aec6f27bd;OPEN, non-draft,MERGEABLE,REVIEW_REQUIRED(BLOCKED); 5 files, +481/-22. - Changed areas / review hotspots:
dash_steps.rs: Observe succeeds only when the active-chip marker● Observeappears.src/send_until.rs: at-least-once send, 500 ms resend interval, overall deadline, and explicitRunning/Stopped/Failedterminal state.src/reader_failure.rs: failure flag and one-time diagnostic observed atomically under one lock; failure remains terminal after the message is consumed.tui_driver.rs: PTY reader panic capture, child-exit polling, final screen diagnostics, and the wait-to-retry handoff.- Deterministic regressions for failure publication while the reader is still finishing and at both former race boundaries.
- Invariants to verify: Only idempotent absolute-tab input uses resend;
4is sent at least once but never after a terminal state is observed; success requires visible application state, not a successful PTY write; reader panic/child exit wins over marker timeout; one observer consumes the diagnostic without clearing terminal failure; the final error retains the underlying cause and screen; the deadline remains bounded. - Verification evidence: Required: all required hosted checks green, including E2E/report, builds/tests, clippy, signature+DCO, hygiene, and CodeQL. Advisory: Strix Ubuntu and the self-hosted consolidated report pass. Red: GPU has one unexpected
serve-hf-checkpoint-inferencefailure from the missing Lemonade runtime directory; Strix Windows has the same five unexpected IDs as #141/#262 after the llama.cpp backend download fails six times withSSL connect error (CURL code: 35). - Recommended sequence: Observe step/marker → generic resend state machine → atomic reader failure state → TUI integration → boundary-race tests.
michaelroy-amd
left a comment
There was a problem hiding this comment.
Approved at bf5cf1dd89126087f7b8f09aea30e62aec6f27bd. The focused review above covers the changed contracts and invariants. All required checks pass; the current self-hosted advisory failures are documented there and are not merge-required.
Summary
Five dashboard e2e scenarios (
dash-loading-service-status,dash-managed-service-metrics,dash-managed-service-visible, and botheai-7960-*) can fail ~30s after they start, in an assertion about the Observe view — while the captured screen shows the dashboard still sitting on Home,● Homeactive and the Observe chip untouched.Cause. Each of those scenarios launches the TUI and comes straight to "the user opens the Observe view", with no assertion in between to prove the dashboard is reading input yet.
TuiSession::sendwrites into the pseudo-terminal regardless, so a4written before the crossterm event loop is reading can be consumed by whatever holds the terminal at that moment. Nothing retries it, the dashboard never leaves Home, and the failure only surfaces one step later when the nextwait_for_screenburns its full 30s timeout on a view the scenario never navigated to.Fix. Adds
TuiSession::send_until(bytes, marker, timeout), which re-sends the key every 500ms until the screen shows the marker, and switches the Observe step tosend_until("4", "● Observe", DEFAULT_TIMEOUT). The step now depends on the dashboard having acted on the key rather than on the key having been written at a moment the dashboard happened to be ready.Why re-sending is safe here, and only here. The helper is documented as safe only for idempotent keys — a tab jump, not a toggle.
4maps unconditionally to "switch to Observe", so pressing it again while already there is a no-op; a?(help toggle) or:(palette) would flip state on every repeat and must not use this helper. The doc states that constraint, including that copies of the key may still be queued when the call returns.Terminal failures are not retried: if the child exits or the PTY reader thread dies,
send_untilreturns that error immediately.wait_for_screenconsumes the reader's panic message, so retrying past it would destroy the diagnostic and then report a generic timeout instead of the cause it already had.Verification
Reproduced deterministically by pointing
ROCM_CLI_BINARYat a wrapper that holds the terminal in non-canonical mode for two seconds, swallowing everything written to it, before exec'ing the real binary:4.dash-managed-service-metricsfails 32s later withTTFT metrics did not appear: timed out after 30s waiting for "50ms", and the captured screen shows the dashboard still on Home.4s during its window; the first resend after exec lands, and the scenario passes.Also run locally:
dash.featurescenarios green under normal conditions (no wrapper)cargo fmt --all --checkcargo clippy --locked --workspace --all-targets -- -D warningscargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings— thee2etarget setstest = false, so--all-targetsskips it and it must be linted (and compiled) explicitlyCaveat, stated plainly: the reproduction is synthetic. It proves the failure mode is real and that the change removes it, but it does not prove this is the mechanism behind the CI flake — that remains unconfirmed. The change is defensive.
Scope
Split out of #165, where it was unrelated to that PR's subject (atomic downloads).
Two sibling steps have the same structural exposure and are deliberately left alone here:
open_dashboard_help(?) andopen_command_palette(:) also send a key straight after launch. Both keys are toggles, sosend_untilcannot be used for them — if they start flaking, the right fix there is a readiness wait before the key, not a resend.Risk: low. Test harness only, no product code.
tests/e2e-cucumber/expectations.tomlfor the fixed ticket ID and removed/narrowed any now-stale xfail rows. — no rows reference these scenarios.