From 8f67bcafaccb802bdb6ce069e7a2454d01e8f35e Mon Sep 17 00:00:00 2001 From: Juan Leni Date: Sat, 26 Sep 2026 13:30:52 +0200 Subject: [PATCH] fix(conformance): runner_unreachable as a verdict is not a liveness failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #392 waited out any reply whose `reason` was `runner_unreachable`. Two scenarios assert exactly that reason as the correct outcome — `crash_before_spawn_is_unknown_and_never_retried` and `crash_after_spawn_before_ack_is_unknown_and_runs_once` — so the helper waited 60s for a verdict that was never going to change, then failed with "the runner never answered again after the injected crash cleared". Both passed before #392. Caught by the conformance leg on main, which is the only thing that runs these. The two answers are distinguishable and #392 did not look: - **Execution record** — carries `state`, `startedAt`, `finishedAt`. The execution is `Unknown` *because* the runner was unreachable. That is the verdict. - **Error envelope** — `{"error": …, "reason": …}`, no `state`. The runner has not finished coming back from the injected crash and was asked before there was anything to record. That is the liveness window #373 flaked on, and the only thing that should be waited out. Keyed on the presence of `state` now, which is what separates them. This is the class documented in #399 the same day: the right construct — wait for the runner — pointing at the wrong referent, because `reason` alone does not say which of the two answers arrived. --- tests/sandbox_conformance.rs | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/tests/sandbox_conformance.rs b/tests/sandbox_conformance.rs index 52d6b26..72d93b7 100644 --- a/tests/sandbox_conformance.rs +++ b/tests/sandbox_conformance.rs @@ -600,19 +600,26 @@ impl LeasedSandbox { /// Exec, waiting out a runner that has not finished recovering. /// - /// `clear_execution_crash` clears the *injection*, not the runner. The - /// process the crash interrupted may still be coming back, and an exec - /// landing in that window answers `runner_unreachable` — which is - /// liveness, not the fail-closed behaviour these scenarios assert. So it - /// is waited out rather than asserted on. + /// # `runner_unreachable` is two different answers /// - /// Everything else is returned untouched, success or not, so the caller - /// still judges the exact `Unknown` and its reason. Weakening that is - /// what this must not do: the point of each scenario is that a crash in - /// its window produces exactly one reason and never runs the payload. + /// As an **execution record** — carrying `state`, `startedAt`, + /// `finishedAt` — it is the verdict: this execution is `Unknown` *because* + /// the runner was unreachable. `crash_before_spawn` and + /// `crash_after_spawn_before_ack` assert exactly that, so waiting for it + /// to change waits for something that never will. + /// + /// As an **error envelope** — `{"error": …, "reason": …}`, no `state` — + /// it is liveness: the runner has not finished coming back from the + /// injected crash, and an exec that lands in that window is answered + /// before there is anything to record. That is what #373 flaked on, and + /// the only thing waited out here. /// - /// Without this the retry races the runner and fails on the liveness - /// check, several assertions before the one under test (#373). + /// The presence of `state` is the distinction, and it is the whole reason + /// this helper can tell them apart. An earlier version keyed on `reason` + /// alone and broke both scenarios above by waiting out their verdict. + /// + /// Everything else is returned untouched, success or not, so the caller + /// still judges the exact `Unknown` and its reason. async fn exec_once_the_runner_answers( &self, argv: &[&str], @@ -622,7 +629,9 @@ impl LeasedSandbox { let deadline = Instant::now() + within; loop { let (status, body) = self.exec(argv, key).await?; - if body["reason"] != "runner_unreachable" { + let liveness_failure = + body["reason"] == "runner_unreachable" && body.get("state").is_none(); + if !liveness_failure { return Ok((status, body)); } anyhow::ensure!(