fix(conformance): runner_unreachable as a verdict is not a liveness failure - #400
Merged
Merged
Conversation
…ailure #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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a regression I introduced in #392, found by the conformance leg on
main— run 36233876045.
What broke
#392 waited out any reply whose
reasonwasrunner_unreachable. Twoscenarios assert exactly that reason as the correct outcome:
crash_before_spawn_is_unknown_and_never_retriedcrash_after_spawn_before_ack_is_unknown_and_runs_onceSo the helper waited 60s for a verdict that was never going to change, then
failed with my own message:
Both passed before #392. I applied the helper to all four crash scenarios
because they shared a shape; only one of them shared the problem.
The two answers are distinguishable
state,startedAt,finishedAtUnknownbecause the runner was unreachable{"error": …, "reason": …}, nostate#373's original failure was the second:
{"error":"the sandbox runner did not answer","reason":"runner_unreachable"}.The ones above are the first.
reasonalone cannot tell them apart, and #392keyed on
reasonalone.Now keyed on the presence of
state.This is the class #399 documents
Merged the same day: the right construct — wait for the runner — pointing at
the wrong referent.
reasonlooked like the thing that identified a livenessfailure and was not.
It is also the case for the rule in #399 about conformance: nothing in
cargo testruns these scenarios, and a unit test over the helper would have usedwhichever body shape I already believed in.
Verification
clippy --all-targets --all-features -D warningsclean,fmtclean. Thescenarios themselves only run on the conformance leg, so this needs
ci:conformanceon the PR — which is the lever #399 says to pull and nobodyhas.