Skip to content

test(e2e): don't count harness stalls against the quit timeout - #719

Merged
rominf merged 4 commits into
mainfrom
fix/e2e-dash-quit-on-ubuntu-26-04
Oct 23, 2026
Merged

rominf merged 4 commits into
mainfrom
fix/e2e-dash-quit-on-ubuntu-26-04

Conversation

@rominf

@rominf rominf commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Main's CI is red. The hosted E2E tests job fails on two scenarios, dash-gen-tps-expiry-boundary (dash-09) and dash-esc-menu-quit-row-confirms-when-serving (dash-34). The failing step is "When the user quits the dashboard":

timed out after 30s waiting for the TUI to exit after repeating "q".

Every merge-queue run is ejected for the same reason. #713 did not fix this.

Cause

The runner image is not the cause, and neither is the dashboard. The quit driver stops waiting before it answers the prompt.

Fix

The retry loops that have to act on what they read, quit_and_wait and send_until, now share a PolledDeadline (new tests/e2e-cucumber/src/polled_deadline.rs). It measures the gaps between polls of each wait. Each wait re-checks its condition every poll interval (20 ms in the driver), and its timer may fire up to TIMER_SLACK (50 ms) late, as a sleep on Windows ends on a 15.6 ms timer tick. stall_in treats whatever a gap runs past poll_interval + TIMER_SLACK as a stall and adds gap - (poll_interval + TIMER_SLACK) to the deadline. At the driver's 20 ms poll interval, gaps up to 70 ms count as polling: they move nothing and are not reported as a stall. So the timeout is the time the loop's waits were actually polled, whether a stall lands inside a wait's budget or runs past it.

  • A TUI that never exits still times out, after timeout of polled time.
  • A wall-clock cap of 4x the timeout (WALL_CLOCK_CAP_FACTOR) bounds how far stalls can push the deadline, and each wait's budget is cut to whichever of the deadline and the cap comes first. A 30 s step can therefore take up to 120 s of wall time (360 s with E2E_TUI_TIMEOUT_SECS=90), but only when the host stalls it. A pathological stall therefore fails one step with its own message, instead of running into the CI job timeout and losing every scenario's report.
  • The timeout message names the time the harness left the step unpolled, so the next harness stall is easy to recognise in the log:
    • timed out after 30s of polling, not counting 72s the harness left this step unpolled waiting for ...
    • timed out after the 120s wall-clock cap (4x the 30s timeout); the harness left this step unpolled for 100s of it waiting for ...
    • with no stall, the message is unchanged (timed out after 30s waiting for ...).

send_until had the same defect. It exists to resend a key that did not take effect, and when that key was lost and a stall pushed the wait past the deadline, it returned before the resend. Sharing the deadline also removes the duplicated deadline logic between the two loops.

Other waits (wait_for_screen, wait_for_screen_where, ...) still spend their timeout in wall-clock time. They only re-check a condition and do not act on it, so a stall delays them but cannot make them give up before acting.

This is the minimal harness fix. The deeper cause is that synchronous subprocess calls in async steps freeze every concurrent scenario. Making run_rocm and its siblings async fixes that at the root, and is tracked separately in #720.

Tests

  • polled_deadline.rs:

    • polling_slack_is_not_a_stall: at a 20 ms poll interval, gaps up to 70 ms (poll_interval + TIMER_SLACK) are not stalls; a 71 ms gap credits 1 ms.
    • a_coarse_timer_is_not_a_stall: a wait whose 20 ms sleeps each last three 15.625 ms ticks moves nothing.
    • a_stall_inside_the_budget_moves_the_deadline: a stall that ends inside the wait's budget is still credited, exactly once.
    • a_budget_is_cut_to_the_wall_clock_cap_once_stalls_pass_it: a stall pushes the deadline to 425 ms, past the 400 ms cap; a wait that asks for 10 s gets the 20 ms left before the cap and ends at exactly 400 ms with a wall-clock expiry.
    • the_wall_clock_cap_ends_a_loop_that_keeps_stalling (under a 5 s tokio::time::timeout), the_report_names_only_time_the_step_went_unpolled.
  • quit_and_wait.rs:

    • a_stall_longer_than_the_timeout_still_answers_the_prompt and a_stall_inside_a_long_budget_still_answers_the_prompt: the prompt is answered (q, then y) whether the stall runs past the wait's budget or lands inside it.
    • a_stalled_child_that_never_exits_moves_the_deadline_by_the_stall_once: with a 200 ms timeout and a 150 ms stall, the reported stall is the credited part (150 ms - (poll_interval + TIMER_SLACK)), and the loop ends within one tick after timeout plus that credit.
    • a_wait_longer_than_the_time_left_is_cut_to_the_deadline, a_child_that_never_exits_times_out, a_stall_past_the_wall_clock_cap_fails_the_step.
    • a_wait_is_cut_to_the_wall_clock_cap_when_stalls_push_the_deadline_past_it: the same clamp as above, through the loop: after a 380 ms stall the doubled 10 s interval is cut to the cap, and the loop ends at exactly 400 ms on the cap.
    • Every test here runs under a 5 s tokio::time::timeout, so a deadline that keeps moving fails the test instead of hanging it.
  • send_until.rs: a_lost_key_is_resent_after_a_stall_longer_than_the_timeout, a_lost_key_is_resent_after_a_stall_inside_a_long_budget, a_wait_longer_than_the_time_left_is_cut_to_the_deadline (each attempt's budget is cut to the time left, mirroring quit_and_wait), and a_marker_that_never_appears_times_out_and_names_the_stall.

  • The test double ticking_wait sleeps one tick before giving up on a budget that is already spent, so a broken deadline that keeps handing out empty budgets still advances the paused clock and the outer timeout fails the test instead of spinning.

  • Mutation check. Each mutation was applied on the current head and cargo test -p e2e-cucumber --lib was run. Every mutation failed at least one test, and no run hung (the test binary finished in about 2 s each time):

    mutation failing tests
    .min(self.wall_deadline...) clamp removed from budget a_budget_is_cut_to_the_wall_clock_cap_once_stalls_pass_it, a_wait_is_cut_to_the_wall_clock_cap_when_stalls_push_the_deadline_past_it
    cap branch of expired() never fires 4, including the_wall_clock_cap_ends_a_loop_that_keeps_stalling (fails on its timeout instead of spinning)
    send_until waits the full resend_interval (no budget) send_until::a_wait_longer_than_the_time_left_is_cut_to_the_deadline
    stall not added to the deadline 10, including both prompt-answering tests and both send_until resend tests
    every stall added twice a_stall_inside_the_budget_moves_the_deadline, a_stalled_child_..._by_the_stall_once
    no TIMER_SLACK 5, including polling_slack_is_not_a_stall and a_coarse_timer_is_not_a_stall
    every gap counted as a stall 8
  • The timing tests were run 30 times pinned to one CPU (taskset -c 0), and the full --lib suite 10 times pinned to one CPU, with no failure.

  • Reproduction with the CI command (cargo xtask e2e) in containers limited to 3 CPUs, on the first revision of this PR:

    image before after
    ubuntu:26.04, full suite dash-09 and dash-34 fail (same screen as CI); another run failed dash-26 219/221 passed, run twice; the 2 failures are the expected serve-01/02 xfails, rc=0
    ubuntu:24.04, full suite fails on CI (ci: run Linux CI on both Ubuntu LTS images; fork-free reclaim decoys with liveness checks (ROCMAI-670) #718) 219/221 passed, rc=0
    dash-09/26/27/28/34 alone, both images 5/5 pass 5/5 pass
  • On 08856d68 (the later commit changes only unit tests, their test double, and a doc comment), the full mock suite (cargo xtask e2e) pinned to 3 CPUs (taskset -c 0-2) on the dev host: 219/221 passed. dash-09 and dash-34 passed, and the 2 failures are the expected serve-01/02 xfails ("0 unexpected failure(s)", exit 0). No step reported a stall.

  • Local gates: cargo fmt --all --check, cargo test -p e2e-cucumber --lib (169 passed), --test feature_naming, cargo check -p e2e-cucumber --test e2e, both clippy invocations (--workspace --all-targets and -p e2e-cucumber --test e2e, with -D warnings), and a Windows-target (x86_64-pc-windows-gnu) cargo check -p e2e-cucumber --tests and --test e2e with RUSTFLAGS=-D warnings.

There is no new Gherkin scenario. This change is internal to the test harness, and the affected scenarios already exist; the E2E lane on this PR runs dash-09 and dash-34.


  • If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (No xfail rows exist for dash-09 or dash-34.)
  • If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md (the clap declaration and dispatch wiring staying in main.rs/lib.rs is expected, not a violation). (N/A)
  • Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state — not only the wording — per AGENTS.md §3. (Only the harness timeout messages changed. the_report_names_only_time_the_step_went_unpolled pins the wording; a_stalled_child_that_never_exits_moves_the_deadline_by_the_stall_once and a_marker_that_never_appears_times_out_and_names_the_stall assert the reported stall together with the loop's timing and outcome, and polling_slack_is_not_a_stall proves ordinary polling is not reported.)

dash-09 and dash-34 fail "When the user quits the dashboard" on main's
hosted E2E lane. The TUI has opened the quit-confirm prompt and is idle
waiting for an answer, but the driver never sends "y".

cucumber polls every concurrent scenario from one task, and many steps
run `rocm` synchronously, so while one of them runs no other scenario is
polled. Instrumented on a 3-CPU container, the quit loop's first 500 ms
wait came back after 102 s, while the therock-next and comfyui steps ran
their dry-run installs back to back. By then the 30 s deadline had
passed, so the loop gave up before it read the screen again and answered
the prompt it found there. #630 added four of those therock scenarios,
which is why main went red at that commit.

Add any time a wait runs past its budget to the deadline, so the timeout
counts only time the loop was being polled. A TUI that never exits still
times out, and the timeout message now reports how long the step was
stalled.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
@rominf
rominf requested a review from a team as a code owner October 22, 2026 11:20
@rominf
rominf requested review from r0x0r and volen-silo and removed request for r0x0r October 22, 2026 11:20

@tomastola tomastola left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — needs work

Full review of the whole change (#719, 32834b29…5fd8053b).
The description says this fixes main's red hosted E2E lane (dash-09, dash-34), with the async run_rocm root fix left for later. No issue is linked.

Blocking

  • The new "never exits" test claims a check it cannot fail on — tests/e2e-cucumber/src/quit_and_wait.rs:376-408
    The assertion message says "the stall must extend the deadline once, not indefinitely", and the description repeats that claim. The test checks only elapsed < 2s, and its timeout is 60 ms. Mutations run on a scratch copy:

    • Removing deadline += overrun (:98): the test still passes.
    • A conditional second extension when overrun > 50ms: all 8 tests pass, in about 460 ms.
    • An unconditional double extension: three tests hang until the 120 s outer timeout kills them. They hang because nothing bounds them.

    The extension itself is covered only by a_stall_longer_than_the_timeout_still_answers_the_prompt.
    Confidence 100 · logic · Fix: bound the elapsed time tightly (about timeout + stall + slack, e.g. < 60 + 200 + 150 ms) and assert it is at least stall + timeout, so the test proves the deadline moved once. Wrap the call in tokio::time::timeout so an over-extension fails the test instead of hanging it.

  • The docs, the error message and the description overstate what the timeout now excludes — tests/e2e-cucumber/src/quit_and_wait.rs:34-36, :52, tests/e2e-cucumber/tests/e2e/tui_driver.rs:899-900, :932-934; the description's "The timeout now means 30 s of time the loop was actually polled".
    The code (:96-98) credits only elapsed - budget, the part of a wait that ran past its own budget. The code governs, and :60 already describes it correctly. The other claims are wrong in both directions, and a probe with the real wait's shape confirmed each:

    • Stalls inside a budget are still counted. One 300 ms stall inside a 400 ms budget, with a 1 s timeout, gave TimedOut { stalled: 9ms } after 1.009 s of wall time, so only about 700 ms was actually polled. The budgets double toward about 14.5 s at a 30 s timeout, so later stalls increasingly count against the timeout, which is the failure this PR targets.
    • Ordinary polling slack is reported as a harness stall. wait_for_exit_code_within (tui_driver.rs:963-976) sleeps POLL_INTERVAL (20 ms) past its deadline. With no stall and a 3 s timeout, the run reported stalled: 77.5ms, and the message would blame "the harness did not poll this step" for a step that was never starved.

    DEFAULT_TIMEOUT_SECS (tui_driver.rs:76) is still documented as a "wall-clock budget", which quit_and_wait no longer treats it as.
    Confidence 100 · mechanical · Fix: reword the doc comments, the driver doc and the description to say "time a wait runs past its budget is added to the deadline", and either subtract one POLL_INTERVAL of slack before reporting stalled, or word the message as "waits overran their budget by X".

Non-blocking

  • send_until keeps the defect class, and the description says it does not — tests/e2e-cucumber/src/send_until.rs:53-67
    The description says that send_until "already re-check[s] [its] condition after a stall, so a stall only delays them". That holds when the first key took effect. send_until exists to resend a key that did not take effect. If that key was lost and a stall pushes the wait past the deadline, :67 returns before the resend, which is the same "deadline checked before acting again" shape this PR fixes in quit_and_wait. It is rarer because it needs a lost key. Juho's review note 2 on #713 already flagged that these two loops duplicate the same deadline logic, and this PR makes them diverge further.
    Confidence 70 · logic · Fix: apply the same overrun credit there, or factor the deadline/overrun handling into one helper both loops use. At minimum, correct the sentence in the description.

Decisions for the author

  • Patch the symptom here, or make run_rocm async — tradeoff
    The description already says this is the minimal fix. For the record, it is at least the fifth workaround for the same root cause:

    • chat_request.rs moved the request onto its own task (tokio::spawn);
    • paced_download.rs runs tar via spawn_blocking;
    • the comfyui update-check disable (#661);
    • launch_release timing (e2e.rs:88-90);
    • this PR.

    The root fix is mechanical: a spawn_blocking-backed run_rocm future, about 186 .awaits and about 29 helpers. It needs no new tokio feature. Shipping this unblocks main now. The cost is that every new deadline loop has to remember the same credit.

  • Wall time on a starved lane is now bounded only by the CI job — tradeoff
    Each of the roughly 6-8 doubling iterations can absorb a full round of the other scenarios' blocking calls. There is no per-step or per-scenario timeout. On the 15-minute mock job, a pathological stall now ends in a job kill, which loses every scenario's report, instead of one failed step. A hung TUI still fails after timeout of polled time, so nothing real is masked. Confirm that trade is acceptable, or add an outer wall-clock cap (e.g. 4× timeout).

Positive signals

  • The overrun is measured against the budget the loop itself granted, so the existing clamp still guarantees termination, and a hung TUI still times out.
  • stalls_once isolates one stall cleanly, and a_stall_longer_than_the_timeout_still_answers_the_prompt fails without the fix (TimedOut, keys ["q"]) and passes with it.
  • Surfacing stalled in the timeout message makes the next harness stall recognisable from the log. Keep it once the wording is accurate.

Deployment notes

None. The change is internal to the test harness.

juhovainio
juhovainio previously approved these changes Oct 22, 2026

@juhovainio juhovainio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the quit_and_wait deadline-extension logic (tests/e2e-cucumber/src/quit_and_wait.rs, tests/e2e-cucumber/tests/e2e/tui_driver.rs) plus the new unit tests.

No blocking issues. The core claim checks out: a_stall_longer_than_the_timeout_still_answers_the_prompt genuinely fails without the deadline += overrun line and passes with it, and a_stalled_child_that_never_exits_still_times_out confirms a single stall doesn't extend the deadline indefinitely. saturating_sub/saturating_duration_since are used consistently, so no Duration underflow risk. Grepped for other QuitError::TimedOut consumers — none outside the two changed files, so the added stalled field doesn't break anything silently.

Two non-blocking notes:

  • overrun = started.elapsed().saturating_sub(budget) can't distinguish a genuine harness stall from ordinary polling-loop jitter (e.g. exit_poll_within's own poll interval occasionally overshooting). Harmless at the current scale (tens of ms vs a 30s timeout) and consistent with the stated design, so just worth knowing stalled is an upper bound rather than an exact measurement.
  • The deadline extension is unbounded if a wait kept overrunning on every iteration rather than once — only the single-stall case is tested. Given the PR explicitly frames this as the minimal fix ahead of the deeper async rework, that's probably fine as-is, but flagging in case it's worth a cap later.

On CI: the two scenarios this PR exists to fix, dash-09 and dash-34, pass cleanly on the Strix Halo WSL2 lane. That same job still shows overall failure from an unrelated scenario (comfyui-source-download-shows-live-progress timing out) that this diff doesn't touch — looks like the same contention-driven shape as other known flakes on that runner, but it's not one I've seen characterized before, so worth a rerun/second look rather than assuming it's safe to ignore. The other Strix Halo lanes show cancelled (not failure), consistent with known self-hosted runner contention.

…_until

The previous fix credited only the time a wait ran past its own budget.
A stall that lands inside a budget (which doubles toward ~15 s at a 30 s
timeout) still counted, and the one-tick overshoot of every wait's own
poll loop was reported as a harness stall.

Move the deadline into a PolledDeadline that measures the gaps between
polls of each wait instead: a gap longer than twice the wait's poll
interval is a stall, and all but one poll interval of it is added to the
deadline. Cap the loop at 4x the timeout of wall time, so a pathological
stall fails one step rather than the whole job and its reports.

send_until uses the same deadline: if its first key was lost, a stall
counted against the timeout would give up before the resend.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
@rominf

rominf commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

@tomastola Thanks for the review. Each point is addressed in 8e4cf243.

1. The "never exits" test could not fail on its claim (blocking). Fixed. It is now a_stalled_child_that_never_exits_moves_the_deadline_by_the_stall_once. With a 200 ms timeout and a 150 ms stall, it asserts that the loop ends at or after timeout + stall - one poll interval, and before timeout + stall + 100 ms. The "- one poll interval" comes from the new accounting in point 2. The call runs under a 5 s tokio::time::timeout, and so does every other test in the module. I ran your three mutations against the new code:

  • stall not added to the deadline: 7 tests fail, including this one;
  • a second extension when the stall is over 50 ms: this test and a_stall_inside_the_budget_moves_the_deadline fail;
  • an unconditional double extension: the same 2 fail.

None of the three runs hung; each finished in under 1 s.

2. The docs, the message and the description overstated what is excluded (blocking). I fixed the accounting rather than only the wording. Your in-budget probe was right, and budgets near 15 s make that the common case. quit_and_wait and send_until now share a PolledDeadline (new polled_deadline.rs). It measures the gaps between polls of each wait. A gap longer than twice the wait's poll interval (20 ms in the driver) is a stall, and everything past one poll interval of it is added to the deadline. So a stall is credited whether it lands inside a wait's budget or runs past it, and the timeout is the time the waits were actually polled. The contract this adds is that each wait re-checks at least every poll_interval. Every driver wait already does, and a wait that broke it would only extend the deadline, never past the cap in point 4.

  • Polling slack: the 20 ms sleep a wait runs past its own budget is a 20 ms gap, so it is not a stall. It is neither credited nor reported. polling_slack_is_not_a_stall pins this, and making every gap count fails it.
  • A stall inside a budget: a_stall_inside_the_budget_moves_the_deadline and a_stall_inside_a_long_budget_still_answers_the_prompt cover it. In the second, a 250 ms stall falls inside a 300 ms budget, and the prompt still gets answered. With the stall not credited, that test returns TimedOut after a single q; the previous revision credited only the part past the budget, which is none here.
  • The message now reads timed out after 30s of polling, not counting 72s the harness left this step unpolled. With no stall it is unchanged. The DEFAULT_TIMEOUT_SECS, default_timeout, send_until and quit_and_wait doc comments now say which waits spend the budget in polled time, and the description is updated.
  • Limitation: a gap measures any time the step was not polled. On a starved host that includes OS scheduling delays longer than two poll intervals, not only another scenario's blocking step. That is still time the loop could not act, so I credit it; the cap bounds it.

3. send_until has the same defect (non-blocking). Fixed by sharing the deadline, which also removes the duplicated deadline logic Juho noted on #713. a_lost_key_is_resent_after_a_stall_longer_than_the_timeout loses the first key and stalls the first wait past the whole timeout; the resend still happens. That test fails when the stall is not credited. I also corrected the description's sentence about send_until.

4. Wall-clock cap (decision). Added. WALL_CLOCK_CAP_FACTOR is 4, so a 30 s timeout gets 120 s of wall time. Past that, the step fails with timed out after the 120s wall-clock cap (4x the 30s timeout); the harness left this step unpolled for ..., instead of the job being killed and every report lost. a_stall_past_the_wall_clock_cap_fails_the_step and the_wall_clock_cap_ends_a_loop_that_keeps_stalling cover it, and both fail when the cap is removed.

5. Async run_rocm (decision). Agreed that this is the root cause and that this PR is one more workaround. The root fix is tracked in #720, and I kept it out of this PR so main can be unblocked first.

Verification on 8e4cf243: cargo fmt --all --check, cargo test -p e2e-cucumber --lib (164 passed), --test feature_naming, cargo check -p e2e-cucumber --test e2e, and both clippy invocations with -D warnings. I ran the timing tests 30 times pinned to one CPU, and the full --lib 10 times, with no failure. The full mock suite pinned to 3 CPUs (taskset -c 0-2 cargo xtask e2e) passed 219/221. dash-09 and dash-34 passed, and the 2 failures are the serve-01/02 xfails, with 0 unexpected failures.

@rominf
rominf dismissed stale reviews from tomastola and juhovainio October 22, 2026 13:52

Please re-review.

@juhovainio juhovainio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change fixes real CI flakiness (dash-09/dash-34) with a well-reasoned shared PolledDeadline abstraction. The gap-crediting and wall-clock-cap arithmetic matches the description's stated semantics, and I didn't find a correctness bug in the deadline logic itself (no double-counting, no lost stall credit, the wall-clock cap correctly takes precedence once it's exceeded).

Blocking

The PR's own new timing-sensitive unit tests currently fail on windows-build-and-test (confirmed on this PR's CI run), in both polled_deadline.rs::a_stall_inside_the_budget_moves_the_deadline and quit_and_wait.rs::a_stalled_child_that_never_exits_moves_the_deadline_by_the_stall_once. Windows is a supported platform, so this needs fixing (a wider tolerance, or a platform-scaled TICK) rather than merging red — see the two inline comments.

The target E2E tests job (containing dash-09/dash-34) currently passes, so the actual fix looks effective; this is specifically about the new tests' own tolerance being too tight for Windows' coarser timer granularity, not about the harness fix itself.

Non-blocking

  • send_until.rs doesn't have a "stall absorbed inside the wait's own budget" test the way quit_and_wait.rs does (a_stall_inside_a_long_budget_still_answers_the_prompt). The mechanism is covered elsewhere, so this is a minor coverage gap rather than untested behavior.
  • wait_for_screen/wait_for_screen_where/wait_for_popup_body_where in tui_driver.rs are left on wall-clock deadlines, per the PR description's reasoning (they only re-check a condition rather than acting on a stale read). Reads as a deliberate scope boundary — worth a one-line confirmation that it's intentional rather than deferred.
  • quit_and_wait.rs and send_until.rs still share a near-identical retry-loop skeleton on top of the now-deduplicated PolledDeadline. Low-signal, not asking for action here.

CI

windows-build-and-test: FAIL (see above). E2E tests: PASS. Everything else that's completed is green; several self-hosted hardware E2E lanes (Strix Halo, MI300X, W7900) were still pending/queued at review time.

.run(ticking_wait(Duration::from_millis(200), stall, || false))
.await;
assert!(
deadline.stalled >= stall.saturating_sub(TICK) && deadline.stalled < stall + 4 * TICK,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bound (stalled >= stall - TICK && stalled < stall + 4*TICK, with TICK = 5ms) is tight enough to fail on Windows' coarser timer granularity — confirmed failing right now on windows-build-and-test (stalled 181.8125ms for a 150ms stall, well outside this window). Windows is a documented supported platform, so this needs a wider tolerance (or a platform-scaled poll interval) rather than being left red.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 08856d6. I did not widen the tolerance. These tests now run on tokio's paused clock (start_paused = true), and PolledDeadline reads tokio::time::Instant, which is the host clock in the harness. Under the paused clock a sleep advances time by exactly what it asks for, so this test now asserts the exact credit, stalled == credited(stall), and that the deadline moved by exactly that amount. It no longer depends on how precisely the host's timer fires.

The 181 ms was an over-count, not only a tight bound. A gap between polls counted as a stall once it exceeded 2 * poll_interval, and these tests polled every 5 ms. On Windows each 5 ms sleep wakes on a 15.6 ms tick, so every tick crossed the 10 ms threshold and about 10 ms of it was credited as a stall. The stall rule now credits only what a gap runs past poll_interval + TIMER_SLACK, where TIMER_SLACK is 50 ms, about three Windows ticks. The new a_coarse_timer_is_not_a_stall test pins this: a 20 ms poll loop whose sleeps each last 46.9 ms is credited zero.

Linux reproduction: I set a 16 ms timer slack with prctl(PR_SET_TIMERSLACK). Under it the old head failed this test (stalled 189ms for a 150ms stall) along with three others. The new head passed 30 of 30 runs pinned to one core with that slack, and 30 of 30 more with a CPU hog on the same core.

Mutations that fail this test: the stall not added to the deadline; a conditional second extension; a double extension; every gap counted as a stall; and the old 2 * poll_interval threshold. None of them hangs, and every run finishes in under 10 ms.

Comment thread tests/e2e-cucumber/src/quit_and_wait.rs Outdated
let QuitError::TimedOut(Expiry::Polled { stalled, .. }) = error else {
panic!("expected a polled-time timeout, got {error:?}");
};
assert!(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as polled_deadline.rs's a_stall_inside_the_budget_moves_the_deadline: this bound is too tight for Windows' timer granularity. CI shows this failing with reported stall 368.6094ms for a 150ms stall, outside [stall - TICK, stall + slack). Needs widening before merge since windows-build-and-test is currently red.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 08856d6. This one was a real over-count. A 368 ms report for a 150 ms stall means about 220 ms of ordinary 5 ms polling was credited as stall, one ~10 ms overshoot per Windows timer tick. Each credit also moved the deadline, which kept the loop running longer and added more credit. On Linux, with the timer slack set to 16 ms, the old code went as far as the wall-clock cap: WallClock { stalled: 631ms } for the same 150 ms stall.

In production the harness polls every 20 ms. A Windows wake lands up to about 36 ms after the last poll, just under the old 40 ms threshold, so one extra tick on a loaded runner would have reported sleep overshoot as "the harness left this step unpolled" and extended the deadline by it. Stalls now count only past poll_interval + TIMER_SLACK (20 + 50 ms in the harness). The credit is now a slight underestimate, never an overestimate, and real stalls (a synchronous rocm run) are hundreds of milliseconds or more.

The test runs on tokio's paused clock and asserts exact values. The reported stall is credited(stall), and the loop ends in [timeout + credited(stall), timeout + credited(stall) + TICK]. Missing the credit, crediting it twice (conditionally or always), and counting every gap each fail this test, with no hang: the HANG timeout is virtual too.

…se timers

The new timing tests failed on Windows: a 150 ms stall was reported as
181 ms and 368 ms. A gap between polls counted as a stall once it passed
twice the poll interval, and the tests polled every 5 ms. A Windows sleep
ends on a 15.6 ms timer tick, so every 5 ms sleep overran that threshold
and its overshoot was credited as a stall. The harness polls every 20 ms,
whose two-tick wake (up to ~36 ms) sat just under the 40 ms threshold, so
a loaded Windows runner could report sleep overshoot as the harness
stalling, too.

Count as a stall only what a gap runs past the poll interval plus a fixed
50 ms timer slack (about three Windows ticks). Real harness stalls are
hundreds of milliseconds or more, so they still register.

Read time from tokio::time::Instant, which is the host clock in the
harness, and run the retry loops' unit tests on tokio's paused clock.
Sleeps then advance time exactly, so the tests assert exact stall credits
and deadlines instead of tolerances, and do not depend on host timers.

Add a send_until test where a stall is absorbed inside the wait's own
budget, and a test that a coarse timer is not reported as a stall.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
@rominf

rominf commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator Author

@juhovainio thanks for the review. The two blocking timing failures are answered inline. The fix is 08856d6: the tests now run on tokio's paused clock, and the stall rule allows for coarse timers. Here are the non-blocking items:

  • send_until stall inside its own budget. Added a_lost_key_is_resent_after_a_stall_inside_a_long_budget. The resend interval outlasts the 300 ms timeout, so the first wait's budget is the whole timeout. A 250 ms stall lands inside that budget while the first key is lost. The test passes only if the stall is credited and the key is resent. With deadline += stall removed, it fails.
  • wait_for_screen, wait_for_screen_where and wait_for_popup_body_where stay on wall-clock deadlines on purpose. They send nothing; they only read. The pty reader thread keeps updating the screen while the harness is stalled, and each loop checks its condition before its deadline. So the first poll after a stall still sees everything the TUI rendered during it, and a condition that became true then is still reported as success. quit_and_wait and send_until are different: they must act on what they read, and the TUI then needs time to respond to the key. That is the failure a stall caused, and it is why only those two use PolledDeadline.
  • Shared loop skeleton: not done. The parts the two loops share, the deadline, stall credit and wall-clock cap, are already in PolledDeadline. The rest differs on every axis. quit_and_wait picks its key from the screen on each pass, doubles its interval, and stops on ExitPoll::Finished. send_until resends one fixed key at a fixed interval, and it runs a three-way terminal-state check between the wait and the deadline. A shared skeleton would need callbacks for key choice, interval policy and post-wait classification, which is more surface than the two ~20-line loops it would replace.

Verification on 08856d6:

  • cargo fmt --check, cargo test -p e2e-cucumber --lib (166 passed), feature_naming, cargo check --test e2e, and both CI clippy invocations, all with -D warnings.
  • cargo check --target x86_64-pc-windows-gnu -p e2e-cucumber --tests and --test e2e, with RUSTFLAGS=-D warnings.
  • The retry tests ran 30 times pinned to one core, 30 times with a 16 ms timer slack, and 30 times with that slack plus a CPU hog on the same core. All passed. Under the same slack, the previous head failed four of them.
  • I did not run the Windows tests locally; windows-build-and-test covers them.

@rominf
rominf dismissed juhovainio’s stale review October 23, 2026 07:28

Addressed on 08856d6 (reviewed 8e4cf24). The timing tests now run on tokio's paused clock with exact assertions. The 368 ms was a real over-count from timer slack, now fixed with TIMER_SLACK and pinned by a_coarse_timer_is_not_a_stall. windows-build-and-test passes. The send_until test was added, and the inline replies cover each point.

@volen-silo volen-silo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. The shared PolledDeadline reads right to me, and moving the timing tests onto tokio's paused clock makes them exact on any host. CI is green on this head. Two small things before merge, plus a few notes.

Before merge

  1. The description does not match the code for the stall threshold. The code credits gap - (poll_interval + TIMER_SLACK) with TIMER_SLACK at 50 ms (stall_in in polled_deadline.rs), so at the driver's 20 ms poll interval up to 70 ms counts as polling. The description says gaps up to two poll intervals are not stalls and longer gaps lose one poll interval, and it never mentions TIMER_SLACK. The unit tests assert the code's behaviour, so the description is the side to correct. It also says 164 tests passed; cargo test -p e2e-cucumber --lib on this head gives 166.
  2. The .min(self.wall_deadline...) clamp in PolledDeadline::budget is not covered. I removed it locally and all 166 lib tests still passed. A test where stalls push deadline past the cap while the resend interval is large, asserting the wait ends at the cap, would pin it.

Smaller

  • the_wall_clock_cap_ends_a_loop_that_keeps_stalling loops on while deadline.expired().is_none() with no outer bound, unlike the guards in quit_and_wait.rs and send_until.rs. If the cap branch of expired() ever breaks, this test would spin instead of failing. Wrapping it in tokio::time::timeout would match the others. This also bears on the description's "no run hung" line in the mutation section.
  • In send_until.rs, deadline.budget(timing.resend_interval) has no test that each attempt is cut to the time left. quit_and_wait has one for the same logic.
  • The doc comment on TuiSession::quit_and_wait in tui_driver.rs ends with "gives the reasons for both", and "both" no longer has anything to point at.

To confirm it is deliberate

  • A 30 s step can now take up to 120 s of wall time (cap factor 4), and up to 360 s with E2E_TUI_TIMEOUT_SECS=90. A hung TUI still fails after the normal polled time, so this only costs time when the host stalls.
  • With tokio's test-util as a dev-dependency, the timing unit tests no longer exercise the real clock on Windows, which is where the slack matters. The slack constant is covered only through the virtual coarse test.

Not covered by this review: a full cargo xtask e2e run, and the reproduction claims (3-CPU containers, the 102 s stall).

Cover the `.min(wall_deadline)` clamp in `PolledDeadline::budget`: a
stall pushes the deadline past the cap, and a wait that asks for 10 s
must end at the cap, both on `PolledDeadline` directly and through
`quit_and_wait`. Add the `send_until` counterpart of `quit_and_wait`'s
test that a wait is cut to the time left.

Bound `the_wall_clock_cap_ends_a_loop_that_keeps_stalling` with
`tokio::time::timeout` like the other guards, and have `ticking_wait`
sleep one tick before giving up on an already-spent budget, so a broken
cap fails the tests instead of spinning them on the paused clock.

Fix the dangling "both" in `TuiSession::quit_and_wait`'s doc comment.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
@rominf

rominf commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator Author

Thanks for the review, @volen-silo. All points are addressed in 2bd7a0c.

Before merge

  1. I rewrote the description to match stall_in. It now says a stall is credited as gap - (poll_interval + TIMER_SLACK), that TIMER_SLACK is 50 ms, and that gaps up to 70 ms count as polling at the driver's 20 ms poll interval. The test list, the a_stalled_child_..._by_the_stall_once bounds and the gate count (now 169 with the three new tests) are updated too.

  2. I added two tests for the .min(self.wall_deadline...) clamp:

    • polled_deadline::a_budget_is_cut_to_the_wall_clock_cap_once_stalls_pass_it checks PolledDeadline directly. A 380 ms stall moves the 100 ms deadline to 425 ms, which is past the 400 ms cap. budget(10 s) must then be 20 ms, the wait must end at exactly 400 ms, and the expiry must be WallClock.
    • quit_and_wait::a_wait_is_cut_to_the_wall_clock_cap_when_stalls_push_the_deadline_past_it checks the same clamp through the loop. After the stall, the doubled 10 s interval must end at exactly 400 ms on the cap.

    With the clamp removed, both tests fail: the wait ends at 425 ms with a Polled expiry.

Smaller

  • the_wall_clock_cap_ends_a_loop_that_keeps_stalling now runs under a 5 s tokio::time::timeout. With the mutation where the cap branch of expired() never fires, it fails on that timeout instead of spinning. That mutation also showed that quit_and_wait's own cap tests could spin despite their HANG guard. Once the cap passes, budget returns zero, and ticking_wait with a zero budget and no stall returned without ever awaiting, so the paused clock never advanced and the timeout could not fire. ticking_wait now sleeps one tick before giving up when it has not slept at all. With that change, the mutation fails 4 tests in about 2 s and none hang.
  • I added send_until::a_wait_longer_than_the_time_left_is_cut_to_the_deadline, which mirrors the quit_and_wait test. With a 100 ms timeout and a 5 s resend interval, the loop must end within one tick of 100 ms. The test fails if deadline.budget(timing.resend_interval) is replaced with timing.resend_interval.
  • The doc comment now ends: "quit_and_wait explains why the gesture is resent, why the wait doubles, and why the deadline counts only polled time."

I redid the full mutation table on this head. Every mutation fails at least one test, and none hangs.

To confirm

  • The 4x cap is deliberate. A 30 s step can take up to 120 s of wall time, or 360 s with E2E_TUI_TIMEOUT_SECS=90, and the cap is what still fails a step on a pathological stall. Time is only added for gaps longer than poll_interval + TIMER_SLACK, which happen only when the harness leaves the step unpolled. A hung TUI on a host that keeps polling still fails after the normal polled timeout, as a_child_that_never_exits_times_out shows. The description now states the 120 s and 360 s bounds.
  • I did not add a real-clock test. To be worth having, it would have to assert that ordinary 20 ms polling on the real clock is never reported as a stall. That is a bound on host scheduling, and loaded CI runners break exactly that kind of bound, which is the reason this PR exists. A tolerance loose enough not to flake would no longer test the 50 ms slack. On coverage, no CI lane currently exercises PolledDeadline on Windows' real clock. Every scenario that reaches quit_and_wait or send_until is tagged @requires-os:linux; the only exception is dash-24, which needs @requires-wsl @requires-gpu and is skipped on every lane today. On Linux the real clock runs on the hosted E2E tests lane. For Windows, TIMER_SLACK is headroom for when the PTY scenarios run there. Until then, a_coarse_timer_is_not_a_stall pins it against a modelled 15.625 ms tick, three ticks per sleep, and a test cannot cover more than that until a Windows PTY lane exists.

@volen-silo

Copy link
Copy Markdown
Collaborator

Thanks, @rominf. I checked the new head (2bd7a0c) and the points hold.

  • cargo test -p e2e-cucumber --lib gives 169 passed, as the description says.
  • With the .min(self.wall_deadline...) clamp removed from PolledDeadline::budget, exactly the two new tests fail: a_budget_is_cut_to_the_wall_clock_cap_once_stalls_pass_it and a_wait_is_cut_to_the_wall_clock_cap_when_stalls_push_the_deadline_past_it.
  • With send_until using the raw timing.resend_interval, send_until::a_wait_longer_than_the_time_left_is_cut_to_the_deadline fails.
  • With the cap branch of expired() never firing, 4 tests fail in about 4 s and nothing hangs, including the_wall_clock_cap_ends_a_loop_that_keeps_stalling.
  • The description now states the 50 ms slack, the 70 ms bound and the 120 s and 360 s limits. The hosted E2E tests, E2E consolidated report, clippy and windows-build-and-test pass on this head; the self-hosted lanes were still pending when I looked.

Your reasoning on the real-clock test and the Windows coverage gap is clear and I have no further points. I did not re-run a full cargo xtask e2e, and I'm not able to approve this myself. It still needs an approval from another maintainer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants