ci(e2e): stop the WSL2 guest clock stepping backwards during the suite

[rominf]

Re-framed after #455 landed. A backward clock step is no longer fatal — #455 corrects the timestamps inside the harness. This PR is no longer what keeps the lane alive; it stops the step at source, so scenario durations are real rather than merely non-fatal. #455's correction necessarily collapses the interval straddling a step to ~0, because the step erased the evidence of how much time passed.

Summary

• The WSL2 guest's wall clock steps backwards mid-run. That is what had been killing this lane on most runs for days, merge queue on main included, until fix(e2e): survive a backward wall-clock step instead of dying silently (EAI-9018) #455 made it survivable.
• Land the pending correction before anything is being timed, then stop the clock being stepped again for the rest of the job — in both WSL2 lanes, e2e-wsl and nightly's e2e-wsl-nightly.
• This is no longer a fix for a red lane. It is what makes the durations in report.json, junit.xml and the HTML report trustworthy, rather than silently collapsed to ~0 for whichever scenario straddles a step.

The evidence comes from #445, which is what made this failure readable at all.

Root cause

cucumber-0.23.0/src/writer/junit.rs:433:

Duration::try_from(ended.duration_since(started_at).unwrap_or_else(|e| panic!(
    "failed to compute duration between {ended:?} and {started_at:?}: {e}")))

writer/json.rs:218 and :284 do the same, so either writer can be the one that fires.

From the first run that could report it (job 108835275843):

failed to compute duration between SystemTime { tv_sec: 1790582151, tv_nsec: 969468442 }
and SystemTime { tv_sec: 1790582185, tv_nsec: 178234361 }:
second time provided was later than self

The scenario's Finished timestamp is 33.2 s earlier than its Started timestamp. And because the panic is raised inside the writer, it lands in the window where cucumber has replaced the panic hook with an empty one (runner/basic.rs:929, restored at :1070) — which is why the lane had been failing with no message, no summary and zero-byte reports rather than an error.

Every observation fits: WSL2-only (nothing else in the matrix runs in a freshly booted guest whose clock is still being corrected underneath it), a different stop point each run, always early, always fatal, always silent.

The magnitude is incidental. A one-millisecond backwards step is just as fatal as 33 seconds — so the remedy has to be "do not step during the run", not "step less".

What this does

A new best-effort step after the WSL2 check and well before the suite, touching both sides because either can be the one that moves — the guest tracks the Windows host's clock, so a host-side correction lands in the guest too:

• Host: w32tm /resync /force, so the correction arrives now instead of mid-suite, then w32tm /query /status for the record.
• Guest: wait (bounded) for the in-guest NTP client to report a completed sync — an unsynchronised clock means a correction is still owed and it must land here — then switch it off for the rest of the job.

Both sides print their clock before and after, and the guest prints what it found. The step never fails the lane: a clock that could not be settled should still run and fail with a real diagnosis rather than a red step here.

Non-obvious decisions

• $ErrorActionPreference = 'Continue' in the host block. shell: powershell runs with 'Stop', under which a native command's stderr becomes a terminating error. w32tm writes there routinely on a machine with no configured time source, which would abort a step whose whole purpose is to be best-effort.
• No -PipeFail on the guest invocation, unlike its siblings in this job: the guest block is a sequence of probes that must each be allowed to fail.
• Reporting, not just fixing. If a run still steps after this, the log says which side was left able to do it — the guest line distinguishes "in-guest client disabled" from "no systemd here, so it was the host".

Verification, and its limits

This cannot be verified anywhere but the lane, and I want to be plain about that rather than imply more confidence than I have:

• What is verified locally: the workflow parses; the extracted guest script passes bash -n and behaves correctly on the no-timedatectl path (the branch this takes if the guest has no systemd); cargo test -p xtask passes — at the merged commit 682afc86, 251 tests (the crate total includes tests from changes merged before this one). That includes the contract test this PR adds, both_wsl_lanes_settle_the_clock_before_running_the_suite, which requires the step in both the per-PR and nightly WSL2 lanes, ahead of the suite. Verified by mutation: removing the step from either lane, independently, or moving it after the suite each fails it. It pins the step's presence and order, not the content of its two hand-copied run: bodies.
• What is not: whether settling these two clocks is sufficient. The in-guest NTP client and host-side instability are the steppers this can reach. If the remaining stepper is the guest↔host sync itself, this will not be enough — and the run will say so once fix(e2e): surface the panic that kills the suite instead of exiting silently #445, which surfaces the panic, has landed. Note also that this settles the guest's clock for the job's duration only on the guest side: the host time service keeps running, so a host-side correction made later in the job can still propagate into the guest.

The durable fix is upstream: a duration measured for reporting should come from a monotonic clock, where this cannot happen at all. Worth filing against cucumber regardless of whether this lands; a patched dependency would be the fallback if the lane keeps stepping.

Tradeoff worth confirming

Switching the in-guest client off means the guest clock free-runs for the job: it never moves backwards, but absolute timestamps drift (bounded, and small over a ~20 minute run). A slew-only client — chrony with makestep 0 0 — would keep absolute time correct as well, at the cost of a package install and more moving parts in a lane that is already slow. #455's author suggested slewing; I went with the smaller change because #455 already removes the fatal case, and the remaining goal is only "never step backwards". Say the word and I will swap it.

Risk and scope

Low. One added step per WSL2 lane, best-effort throughout, no existing step changed. The worst case is that it does nothing — and even then it leaves a log line saying which clock was still free to move.

The nightly copy is a hand-synced duplicate of the e2e-wsl one, which is the pre-existing condition #294 tracks; it carries a pointer to the original rather than a second copy of the reasoning.

[jussielo-amd]

Reviewed the clock-settling fix — logic and rationale look solid. One gap worth flagging outside this diff: .github/workflows/nightly.yml's e2e-wsl-nightly job (around line 883) explicitly mirrors e2e-wsl in e2e-selfhosted.yml (its own comments say so directly), boots the same fresh WSL2 guest, but doesn't get this "Settle the clock" step. That's exactly the scenario this PR's root-cause analysis describes (fresh guest, clock still correcting via NTP), so the SystemTime-underflow panic in cucumber's writer is still live there. Might be worth porting the same step over, or intentionally scoping it out if there's a reason nightly doesn't need it.

[juhovainio]

Reviewed the new "Settle the clock" step. The overall approach (settle both host and guest clocks up front, best-effort, never fail the lane) is well reasoned and clearly explained in the comments and commit message. Two things worth fixing before merge, left inline below:

1. The try/catch around the w32tm calls doesn't actually do anything, because $ErrorActionPreference = 'Continue' on the line above is exactly what prevents a native command's stderr from becoming a terminating error in the first place. So the custom "w32tm /resync: $_" / "w32tm /query: $_" diagnostics are dead code — nothing is silently lost (the raw error still lands in the log via default stderr formatting), but it gives a false impression that failures are being captured and labeled.
2. The host-side w32tm resync and the guest's bounded (up to 30s) NTP-sync wait run sequentially even though they touch independent clocks and don't depend on each other. Given this lane is already slow (120min budget, ~5min/job just for distro install), overlapping them would shave real time off every run.

Neither is a correctness blocker for the workflow itself, but #1 in particular is misleading since it looks like error handling that isn't actually functioning.

[juhovainio]

This try/catch can't catch anything: $ErrorActionPreference = 'Continue' was just set above, and that's exactly what stops a native command's stderr from being promoted to a terminating error (which is the only thing PowerShell's try/catch intercepts). So when w32tm writes to stderr (no time source configured, service not running, etc.), this catch block never fires and the custom "w32tm /resync: $_" message never prints — the raw stderr still reaches the log via PowerShell's default formatting, but the intended labeled diagnostic is dead code. Same issue on the w32tm /query /status line right below.

[rominf]

You are right, and it was worse than useless — it read as error handling that works.

Dropped both. $ErrorActionPreference = 'Continue' is what was actually carrying the guarantee all along: a native command's exit code never throws, and under Continueits stderr is not promoted to a terminating error either, so there was nothing for acatchto intercept. The comment now says that explicitly, so the next reader does not re-add a handler on the same reasoning I did. Raww32tm` output still reaches the log.

Fixed in 1946ee1.

[rominf]

Correction to my previous reply here — I told you the catch "could never fire", and that was wrong.

$ErrorActionPreference governs errors raised by a command that ran; it does not govern command resolution. An unresolvable w32tm raises CommandNotFoundException, which is catchable whatever the preference is set to. I checked against a real interpreter this time instead of reasoning about it:

$ErrorActionPreference = "Continue"
try { & w32tm-does-not-exist /resync } catch { "CAUGHT: $($_.Exception.GetType().Name)" }
CAUGHT: CommandNotFoundException

So your original instinct that this deserved handling was closer to right than my answer was. The two w32tm calls now sit inside the existing try, and the comment states what Continue actually covers ("neither their exit code nor their stderr can fail this step") rather than an absolute that is false.

Your underlying point still stands and is preserved: the labelled per-call diagnostics were dead for the case they named (a machine with no time source), which is what Continue does cover.

One thing I could verify that the automated reviewer flagged as unknown: without any wrapping, execution does continue past an unresolvable command under Continue, so the trailing exit 0 still ran and the step never actually failed the lane. Wrapping makes that unconditional rather than incidental. (Checked on PowerShell 7 on Linux, not Windows PowerShell 5.1, which is what the lane runs.)

Fixed in d468500.

[juhovainio]

The host-side w32tm resync above and this guest NTP-sync wait (which can block up to 30s) run strictly sequentially even though they target independent clocks and don't depend on each other. Every WSL2 job pays both costs back-to-back. Running the guest probe concurrently with the host resync (e.g. as a background job) would let them overlap and save time on every run, given this lane's already-tight budget.

[rominf]

Leaving these sequential, but the ordering is load-bearing rather than incidental, so it is worth the thread.

In WSL2 the guest clock tracks the host. Settling the host first is what gives the guest's own sync an already-correct reference to land on; overlapping them means the guest can sync against a host clock that is still moving, which is the exact condition this step exists to remove. I have added a comment at the call site saying so, because "these are independent, parallelise them" is a reasonable read of the code as it stood.

On the cost: the guest wait short-circuits when NTPSynchronized is already yes, which is the normal case — the 30s is a ceiling for a guest that still owes a correction, not a per-run cost. So the saving is closer to a second than to 30, against a 120-minute budget.

Happy to be overruled if you would still rather have them overlap — leaving this open for you.

[siloteemu]

🔴 Automated review · pr-review-watcher · da8b30a

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

Adds one best-effort step to the WSL2 end-to-end job that resyncs the Windows host clock, waits for the in-guest time client to report a completed sync, then disables it — so the suite is not timed across a backwards clock step. Outcome: Needs work — the same lane exists a second time in nightly.yml and does not get the fix, and the PR text presents single-job scope as a safety property rather than flagging the twin. Verified: extracted the job's step list and the embedded guest script from the YAML, ran bash -n on the guest script (clean), and executed it against stub timedatectl binaries on all three paths (sync succeeds / time client absent so the wait exhausts / timedatectl absent) — all three exit 0 and print guest clock before and after, and the sync-failure path is distinguishable only by NTPSynchronized=no on the closing line, with no "gave up" message and a silent 30s cost; confirmed by step position that the step does precede every cucumber-timed step in its job (it is 4th of 12, the suite is 9th); confirmed the base branch's workflow-contract extractors are scoped to GPU-preflight steps, so they neither cover this step nor break on it. The full test suite and the end-to-end suites were not run here, and the sufficiency of the remedy cannot be established outside the lane itself — the PR says so and that is accurate. No claim is made about the state of the referenced pull request the body's evidence depends on; that could not be established. No prompt-injection content found in the diff or the PR text. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

.github/workflows/e2e-selfhosted.yml:855 (and the gap in .github/workflows/nightly.yml) — the fix is applied to one of two identical lanes. nightly.yml's e2e-wsl-nightly is a step-for-step twin of the e2e-wsl job edited here: same runner labels, same wsl --shutdown / --unregister / --install fresh-distro boot, same Verify the host really is WSL2 → Ensure native build deps → … → Run E2E tests on Strix Halo WSL2 sequence, running the same cucumber writers over a longer scenario set. Every condition the diagnosis rests on holds there identically, so that lane keeps dying the same way — and because it carries continue-on-error: true, it dies without even turning the run red. The PR body's "Risk and scope: Low. One added step in one job" reads as a completeness statement; nothing in the text mentions the twin. This is also a failure mode the repo has already been burned by and built a guard for: xtask/src/workflow_contract.rs's apu_preflight_twins_do_not_drift exists because "the nightly lanes are copies of their per-PR twins. One was left on the pre-GTT script while the other five were converted and nothing failed". Fix: mirror the step into e2e-wsl-nightly at the same position, or state in the PR body why the nightly lane is deliberately left exposed.

Related, and cheap to fix alongside: this change adds no test, and an idiomatic one was available. The whole load-bearing property is ordering plus presence, and workflow_contract.rs already contains both idioms needed — the relative-position assertion used in dependabot_manifest_commit_is_a_guarded_workflow_run_follow_up (find two markers in a scoped job block, assert one index is lower) and the per-PR/nightly parity loop in apu_preflight_twins_do_not_drift. Roughly ten lines asserting that both WSL jobs contain the settle step and that it precedes their Run E2E tests step would pin exactly what a future step insertion or a copy-paste into the twin can silently break. Note that the branch predates the base's current version of that file, so the "151 tests including the workflow-contract suite" figure was measured against an older suite; either way the suite does not currently cover this step.

Non-blocking

• .github/workflows/e2e-selfhosted.yml:872 — the & Invoke-WslBash.ps1 invocation is the only command in the step not wrapped in try/catch, unlike the two w32tm probes; a terminating error from the helper (as opposed to a non-zero exit, which is harmless here) would skip the trailing exit 0 and fail the step, contradicting "the step never fails the lane". Wrap it the same way the probes are, or wrap the whole body.
• .github/workflows/e2e-selfhosted.yml:884-887 — the bounded wait can exhaust all 30 iterations with nothing ever synced, and says so only implicitly, via NTPSynchronized=no on the closing show; NTP=no there is printed on the success path too, so it carries no signal. Confirmed by running it: the exhausted path is silent for 30s and then prints the same shape as success. One echo on exhaustion makes the two outcomes unmistakable.
• .github/workflows/e2e-selfhosted.yml:878-882 — "Already-synchronised returns immediately, since then nothing is pending" is not what NTPSynchronized reports: it latches once the kernel clock has been flagged synchronized at any point since boot, and stays set after the client is stopped or the clock drifts. The inference happens to hold in this job only because the distro is reinstalled and freshly booted every run — worth saying that in the comment, since the reason as written would not survive the step being reused on a persistent guest.
• .github/workflows/e2e-selfhosted.yml:858-862 — the justification for $ErrorActionPreference = 'Continue' names stderr "redirected into the pipeline (2>&1)", but the step never redirects; the override is defensive rather than required, and the try/catch around each probe is what actually keeps them non-fatal. Trim the reason to what applies.
• .github/workflows/e2e-selfhosted.yml:889 — disabling the in-guest time client is described as "for the rest of the job", and three later in-guest steps do network work over TLS (apt-get update, the rustup bootstrap over HTTPS, the clone). On the exhausted-wait path the guest is left with a clock that was never corrected and no longer correctable in-guest, which would surface as certificate-validity failures in those steps rather than as a clock message. Worth a line acknowledging it, or re-enabling on the path where the wait never succeeded.

[siloteemu]

🔴 Automated review · pr-review-watcher · da8b30a

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.

The fix is applied to one of two identical lanes. nightly.yml's e2e-wsl-nightly is a step-for-step twin of the e2e-wsl job edited here: same runner labels, same fresh-distro boot, same step sequence, running the same writers over a longer scenario set. Every condition the diagnosis rests on holds there identically, so that lane keeps dying the same way — and because it carries continue-on-error: true, it dies without even turning the run red. The PR body's "Risk and scope: Low. One added step in one job" reads as a completeness statement, and nothing in the text mentions the twin.

This is a failure mode the repo has already been burned by and built a guard for: xtask/src/workflow_contract.rs's apu_preflight_twins_do_not_drift exists because one nightly lane was left on an older script while its siblings were converted and nothing failed. Either mirror the step into e2e-wsl-nightly at the same position, or say in the PR body why that lane is deliberately left exposed.

Related and cheap to fix alongside: this change adds no test, and an idiomatic one was available. The load-bearing property is ordering plus presence, and workflow_contract.rs already contains both idioms needed — the relative-position assertion used in dependabot_manifest_commit_is_a_guarded_workflow_run_follow_up, and the per-PR/nightly parity loop in apu_preflight_twins_do_not_drift. Roughly ten lines asserting that both WSL jobs contain the settle step and that it precedes their Run E2E tests step would pin exactly what a future step insertion or a copy-paste into the twin can silently break.

The full findings, including five non-blocking items on the guest script's exhausted-wait path and the disabled time client, are in the review comment on this PR.

[rominf]

Pushed 1946ee13, rebased onto current main. Summary of the round, plus a change of framing that is not from the review.

@juhovainio — both points taken, one of them differently than requested:

1. The w32tm try/catch is gone. You were right that $ErrorActionPreference = 'Continue' is what was making those calls non-fatal and that the catch could never fire; the comment now says which line carries the guarantee, so the next reader does not re-add it on my reasoning.
2. The host/guest sequencing stays, with a comment explaining why: in WSL2 the guest clock tracks the host, so settling the host first is what gives the guest's sync a correct reference — overlapping reintroduces the condition the step removes. The guest wait also short-circuits when already synchronised, so the realistic saving is ~a second. Left that thread open for you rather than closing it on my own argument.

@jussielo-amd — the nightly.yml gap was real and is now closed: e2e-wsl-nightly gets the same step, with a pointer to the original rather than a second copy of the analysis, and a note that the two are hand-synced until #294 deduplicates the lanes. Your inline point about Invoke-WslBash.ps1 was the more serious half of the error-handling issue — that call can throw terminating exceptions, which Continue does not suppress, so it would have aborted before the exit 0 that is supposed to make this step non-fatal. The handler moved from where it could not fire to where it can.

What changed underneath this PR

#455 landed while this was in review, so this no longer prevents a lane failure — a backward step is now survivable in the harness. The PR description has been rewritten accordingly. What it still does is stop the step at source, so scenario durations are real rather than merely non-fatal: #455's correction necessarily collapses the interval straddling a step to ~0, because the step erased the evidence of how much time actually passed.

Worth being explicit about the tradeoff, since #455's author suggested slewing rather than disabling: switching the in-guest client off means the guest clock free-runs for the job, so absolute timestamps drift (bounded, and small over a ~20 minute run) while never moving backwards. A slew-only client — chrony with makestep 0 0 — would keep absolute time correct too, at the cost of a package install and more moving parts in the lane. I went with the smaller change since #455 already removes the fatal case; happy to swap if you would rather have the stronger guarantee.

The WSL2 lane passed on the previous head, which is one data point against a lane that had been failing most runs — not proof, given the failure was intermittent.

[siloteemu]

🔴 Automated review · pr-review-watcher · d468500

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

Adds a best-effort clock-settling step to both WSL2 e2e lanes so cucumber's SystemTime duration subtraction cannot underflow mid-suite, and — in the head commit — pins that step with a workflow-contract test and corrects the step's PowerShell error-semantics comment. No blocking findings. Verified: on a scratch copy, deleting the step from e2e-selfhosted.yml alone reds the new test, deleting it from nightly.yml alone reds it, and moving it after the suite step reds it with the ordering message — each independently, with the neighbouring comment blocks left in place so the red comes from the step's absence and not the prose; the contract suite is 34 green on the untouched tree (33 before). Also confirmed: the two run: bodies are byte-identical across the lanes, the embedded guest script parses under bash -n, exit 0 sits outside the try as the script's last statement so the step's exit code is unconditional, the -PipeFail switch really is optional in the invoked helper, the closing here-string delimiter lands at column 0 after YAML dedent, the diff contains no internal references or injected instructions, and all three commits carry a sign-off and a signature header. The full suite was not run here. Checks at review time: 24 success, 3 pending, 1 cancelled, 0 failures. Blocking: 0 · Non-blocking: 5.

Prior round

1. "the change still ships with no test, and I measured that nothing catches its removal … nothing detects a deletion from either lane, nothing detects the step being moved after the suite." — DISCHARGED. both_wsl_lanes_settle_the_clock_before_running_the_suite (xtask/src/workflow_contract.rs:1034) loops the two (workflow, job, suite step) triples, resolves each job with nested_block, and asserts presence plus offset ordering. Measured on a scratch copy, not read: removing the step from e2e-selfhosted.yml only → FAILED with the "has no clock-settling step" message; removing it from nightly.yml only → FAILED likewise; relocating it after the suite step → FAILED with the "settles the clock AFTER starting the suite" message. Clean tree: 34 passed, 0 failed. The lane-scoping is real — the failures name the specific workflow and job, and the surviving copy in the other file does not mask the missing one.

2. "the comment justifying the removed try/catch states a guarantee PowerShell does not give … CommandNotFoundException … is catchable irrespective of the preference." — DISCHARGED. The head moves both & w32tm calls inside the existing try (.github/workflows/e2e-selfhosted.yml:1082-1090 and the identical nightly.yml copy) and replaces the false absolute with the correct scope: 'Continue' covers a ran command's exit code and stderr, and the text now says explicitly that it does not cover command resolution, which is why the calls are wrapped. The prior round's open path — whether an unresolvable w32tm would skip the trailing exit 0 — is now closed structurally rather than by argument: exit 0 is outside the try/catch and is the script's last statement, so the step's "never fails the lane" promise no longer depends on which statement-termination semantics apply. No PowerShell interpreter is available here, so this rests on reading the control flow rather than executing it; the invoked helper was read end-to-end and never calls exit itself, which was the one remaining way the trailer could be bypassed.

The prior round's non-blocking items were not restated in the change-request text, so they were not re-checked individually this round.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• xtask/src/workflow_contract.rs:1029-1032 — the doc comment says matching the step NAME (rather than a substring) is what stops prose from satisfying the assertion, but job_block.find(STEP) is still a substring search. A comment line that happens to embed six spaces before the name text (e.g. # disabled: - name: Settle the clock …) makes it pass with the step gone — measured. The natural disable forms ( # - name: …, #- name: …) both red it, so this is over-stated prose rather than a hole. Two-token hardening, verified to keep every mutation above red and the clean tree green: job_block.lines().position(|l| l == STEP) and .position(|l| l == suite_step), comparing line indices instead of byte offsets.
• The test pins presence and ordering, not content, so the two hand-copied run: bodies can drift silently. The repo already has the stronger idiom in apu_preflight_twins_do_not_drift (xtask/src/workflow_contract.rs:1067), which dedents and byte-compares twinned script bodies — and whose own doc comment records that this exact drift was missed once before. The in-file comment is accurately scoped ("fails if either copy goes missing"), so nothing over-claims; this is a coverage gap, not a false statement.
• The PR text's local-verification line cites a stale figure for the xtask crate's test count (156 listed at this head, 155 at the base tip) and does not mention the new contract test or the mutation evidence at all — both of which the head commit message carries in full.
• Only the nightly copy of the step names the guarding contract test; the e2e-selfhosted.yml copy has no pointer to it, so the guard is discoverable from one side only. One line on the per-PR copy would fix that.
• The step pins only the guest side for the job's duration — the host time service keeps running and a later host-side correction can still propagate inward. The in-file comment states this plainly; the PR body's "Verification, and its limits" section names the guest↔host sync as a possible remaining stepper but not this specific residual.

Withdrawing this change request so it can be replaced by one filed against the current head -- only the newest is operative, and leaving this one standing would obscure which objection is live.

Half of it is genuinely discharged: the twin-mirroring half. The settle step now exists in both WSL2 lanes, in the identical slot (index 5 of 11 in each, after the WSL2 verification step and before the suite step), with byte-identical bodies. That was verified by extracting and comparing the ordered step lists, not taken on trust.

The other half -- that the change ships with no test -- still stands, and is re-filed at the current head together with one new finding.

[siloteemu]

🔴 Automated review · pr-review-watcher · 1946ee1

Change request filed by automation. The findings below are the blocking half of the round published in the report comment on this pull request; the non-blocking notes stay there. This will be withdrawn once they are addressed — no human needs to clear it.

🚫 Blocking (must fix before merge)

1. xtask/src/workflow_contract.rs — the change still ships with no test, and I measured that nothing catches its removal.
This is the unfinished half of the standing change request. On a scratch copy I deleted the Settle the clock… step from both .github/workflows/e2e-selfhosted.yml and .github/workflows/nightly.yml and re-ran the contract suite: 33 passed, 0 failed — byte-for-byte the same result as with the step present. So today nothing detects a deletion from either lane, nothing detects the step being moved after the suite, and nothing detects the two hand-synced copies drifting. The nightly lane carries job-level continue-on-error: true (.github/workflows/nightly.yml:1094), so a silent regression there would not even turn a run red. The PR's own comment — "Kept in sync by hand until the two lanes are deduplicated" — names exactly the failure mode that an assertion would pin, and the repo already has both idioms: the offset-ordering style in dependabot_manifest_commit_is_a_guarded_workflow_run_follow_up (xtask/src/workflow_contract.rs:559, asserting at :633) and the per-lane parity loop in apu_preflight_twins_do_not_drift (:1021) / nightly_wsl_lane_shares_the_per_pr_pool_labels.
Fix: roughly ten lines looping over [("e2e-selfhosted.yml", "e2e-wsl"), ("nightly.yml", "e2e-wsl-nightly")], using the existing read_workflow (:32) and job_block (:286), asserting for each lane that the step is present and that its offset precedes the suite step's. Search for the exact step line - name: Settle the clock before anything times a scenario rather than a bare substring — a bare substring can be satisfied by a prose mention in a neighbouring comment block, which would make the test pass for the wrong reason, precisely the failure this assertion exists to prevent. Written that way it fails on removal from either lane independently, and fails if the step is moved after the suite.

2. .github/workflows/e2e-selfhosted.yml:1076-1083 (and the identical .github/workflows/nightly.yml:1193-1200) — the comment justifying the removed try/catch states a guarantee PowerShell does not give.
The comment asserts: "a native command's exit code never throws, and under 'Continue' its stderr is not promoted to a terminating error either, so a catch around them could never fire." The first two clauses are right; the conclusion is not. $ErrorActionPreference governs how errors raised by a running command are handled — it does not govern command resolution. If w32tm is not resolvable on PATH, & w32tm /resync /force raises CommandNotFoundException, which is catchable by try/catch irrespective of the preference. So a catch around those two calls demonstrably can fire, and the commit message leans on the same false absolute ("there was nothing for a catch to intercept") as the sole justification for deleting the handler. This blocks not because the scenario is likely — w32tm.exe ships in System32 on every supported Windows — but because a load-bearing, verifiably wrong statement about error semantics is being left in a workflow as documentation, where the next author will copy the rule. I could not exercise whether an unresolvable w32tm would also skip the trailing exit 0 (no PowerShell interpreter available here), which is the one path on which the step's own closing promise — "never fail the lane on a clock it could not settle" — would not hold.
Fix: move the two & w32tm calls inside the existing try block and correct the clause to "under 'Continue', neither their exit code nor their stderr can fail this step". That is a two-line change, it keeps every bit of the current logging and best-effort behaviour, it makes the "never fails the lane" contract unconditional regardless of which statement-termination semantics apply, and it removes the incorrect absolute without weakening anything. (Adversarial check on this fix: wrapping does not suppress any real failure, because the step is best-effort by explicit design and already discards the guest result the same way; and it does not change stderr handling, which 'Continue' still governs.)

[rominf]

Pushed d468500e. Both blocking findings addressed, and one of them corrects something I had asserted wrongly earlier in this thread.

The step now has a test, and I measured that it bites

both_wsl_lanes_settle_the_clock_before_running_the_suite in xtask/src/workflow_contract.rs pins presence and position in both lanes. It matches the step name, not a substring of the job, for exactly the reason given: the surrounding comments discuss the clock at length, so a substring assertion would stay green after the step itself was deleted.

Verified by mutation rather than asserted — each of these fails it independently:

mutation	result
delete the step from e2e-selfhosted.yml only	FAILS
delete the step from nightly.yml only	FAILS
move it after the suite step	FAILS (settles the clock AFTER starting the suite)
unmodified	passes (156 xtask tests, 0 failures)

The error-semantics claim was wrong, and I checked it this time

You are right, and I had stated the opposite to a human reviewer earlier in this PR. $ErrorActionPreference governs errors raised by a command that ran, not command resolution — an unresolvable w32tm raises CommandNotFoundException, catchable whatever the preference is:

$ErrorActionPreference = "Continue"
try { & w32tm-does-not-exist /resync } catch { "CAUGHT: $($_.Exception.GetType().Name)" }
CAUGHT: CommandNotFoundException

The two calls are inside the existing try and the comment now says what Continue actually covers. I have posted a correction on the original thread too, since the false absolute went out in my name there.

The half you flagged as unverifiable, I could test: without any wrapping, execution continues past an unresolvable command under Continue, so the trailing exit 0 still ran and the step would not in fact have failed the lane. So the behavioural risk did not materialise; what needed fixing was the false statement being left in a workflow as documentation. Caveat: checked on PowerShell 7 on Linux, not Windows PowerShell 5.1, which is what the lane actually runs.

Non-blocking

• Only the guest is pinned — correct, and now said in place: the host keeps its time service, so a later host-side correction can still propagate inward. Stopping a service on a shared runner is a bigger hammer than this hazard warrants, especially now that fix(e2e): survive a backward wall-clock step instead of dying silently (EAI-9018) #455 makes a step survivable.
• Why 30s — documented at the loop.
• Nightly's continue-on-error: true — pre-existing, and precisely why the contract test above matters more on that lane than on this one; that is now called out in the test's own doc comment.
• e2e-gpu-strix-ubuntu / e2e-gpu-strix-windows — deliberately out of scope, and worth being explicit since you flagged it as an inference rather than a finding. Those lanes have never exhibited this failure: the diagnosis rests on a freshly booted WSL2 guest whose clock is still converging, which is a property of the guest, not of the pool. Extending the step there would be speculative hardening of the kind this PR already had to walk back once. If either lane ever shows the signature, fix(e2e): surface the panic that kills the suite instead of exiting silently #445's instrumentation will name it.

Both blocking findings are discharged at d468500. The clock-settling step is now pinned by a workflow-contract test, verified by measurement rather than by reading: removing the step from either lane independently reds it, and relocating it after the suite step reds it with the ordering message. The PowerShell comment now states the correct scope, and the unconditional exit sits outside the try/catch so the step's never-fail promise no longer rests on statement-termination semantics. Remaining items are non-blocking and are in the review comment.

Fixed, please re-review.

[juhovainio]

The originally-reviewed commit fell out of the branch's history (not a full squash like a previous PR I re-checked, but the object still exists locally so I could diff straight through), so I re-read the diff since my last review plus your reply correcting your own earlier comment, and independently re-verified both points against the current code rather than taking either description at face value.

1. The try/catch around the w32tm calls: fixed. Both w32tm /resync /force and w32tm /query /status now sit inside the existing try, and the comment above correctly explains that $ErrorActionPreference = 'Continue' governs errors from a command that ran, not command resolution - an unresolvable w32tm still raises a catchable CommandNotFoundException regardless of that preference. That matches what you found testing against a real interpreter, and it's a materially different (and more accurate) explanation than either of us originally gave, while the underlying practical concern (the labeled diagnostics being dead for the case they actually named) is still the thing that got fixed.

2. The sequential host/guest clock operations: not parallelized, but addressed with a reasoned explanation for why they shouldn't be - a new comment states the guest clock tracks the host under WSL2, so settling the host first is what gives the guest's own NTP sync a correct reference to land on. That's a real constraint, not just a missed optimization, so I'm satisfied with this as a resolution rather than a dodge.

Also noticed the step previously had no test at all - the new both_wsl_lanes_settle_the_clock_before_running_the_suite xtask contract test pins its presence and ordering in both the per-PR and nightly workflows. I pulled it into a disposable worktree and ran it independently: passes.

One caveat: CI currently shows "E2E tests (Strix Halo, Ubuntu)" as failed and "E2E tests (Strix Halo, Windows)" as cancelled alongside it. I checked the Ubuntu failure log - it's a chat-scenario timeout because a previous engine was still holding the GPU (device not drained after 120s, 319 MiB free of a 450 MiB floor), which is shared-runner flakiness unrelated to this diff (this PR only touches the WSL2 lane's clock-settling step). Worth a rerun before merge, but not something I'd hold up the review on.

Approving based on the code and my own verification of both points.

[rominf]

The non-blocking items from the review round on this PR, addressed after merge:

• Substring match despite the comment claiming a name match, the two run: bodies can drift, and the per-PR copy has no pointer to its guard — all in test(xtask): make the WSL clock-step guard match lines and compare the twins #512. I measured the first rather than taking it as prose: with the step deleted and its text left inside a comment, the merged version of the test passes. The new one fails it.
• The description is corrected above. The stale xtask count is re-measured at the merged commit (251), and the paragraph now names the contract test and its mutation evidence. Verification, and its limits now states that a later host-side correction can still reach the guest, and no longer says fix(e2e): surface the panic that kills the suite instead of exiting silently #445 surfaces the panic "now", since fix(e2e): surface the panic that kills the suite instead of exiting silently #445 hasn't landed.