fix(tests): widen _wait_pidfile's window and name what it saw (#595) - #797
Open
fujibee wants to merge 4 commits into
Open
fix(tests): widen _wait_pidfile's window and name what it saw (#595)#797fujibee wants to merge 4 commits into
fujibee wants to merge 4 commits into
Conversation
… what the successor is doing
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.
Declared reviewers: 1
Refs #595, #758.
Landing on
integration/remote. Headcfc642428c6ce1111f087fa9698343e396ed5b18.This PR does not fix the failing assertion. It was opened as a candidate fix, CI refuted that, and what it is now is the instrument that produced the first real information about the failure in three days. Read the next two sections before the rest.
The window hypothesis is dead, and this PR is what killed it
Commit 1 is #758's fix, ported here:
_wait_pidfile's poll window widened from ~3s to ~10s.It ran on an idle queue — every other run was frozen, so this was the only thing on CI — and
bats (ubuntu-latest 4/4)failed anyway, on the same assertion, in 7m27s:So: not the window, and the "loaded runner" account is weaker too, since this was the least-loaded state all night. Every other check on that run was green (14 success, 1 failure) and the shard's only failure was this one line.
Reading it the other way — that a widened window proves the machine was not slow — would be reading more than the log carries, and the same log is why (below).
The number is #595, not #769
Both issues are about the same test, and they are different assertions:
#769 is
:501— its body quotes that line. #595 is:495, and names the assertion outright: "failing at_wait_pidfile "$pf" "$w2"". What fails on CI tonight is:495. An earlier version of this body cited #769 for a property measured about the other assertion; it does not, and the reference is removed.What the failure now says, and what it still does not
last saw: '<missing>'is the last poll only. It cannot tell "the file never appeared" from "it appeared and then went away", because each poll overwrites the previous value — raised in review, and correct. What the log supports is exactly this:Commit 2 closes that gap. Every distinct value is kept with the poll it first appeared at, and the timeout also reports whether the wanted pid is still alive — which separates a successor that died before writing from a successor that is running while something else removes its record:
(c) is the case
last sawcould not express, and it is the one that would point at ownership rather than timing.All three were forced locally and printed — including the wanted-pid liveness line in both directions. A first draft got (a) wrong:
lastwas seeded with"", so a file missing from the very first poll produced an empty trail — the one case that matters most, silently dropped. It is seeded with a value no read can produce now, and (a) prints.The directory listing is dumped too, so "the file is gone" can be told from "something else is in there".
Commit 2 is beyond #758
Commit 1 is byte-identical to #758's added lines — verified with
diff, not by reading:Commit 2 is not in #758. It answers the same ask #595 made — a failure message that distinguishes what happened — with what the CI run showed to be necessary. If #758 lands on
mainlater, commit 1 is the same change from the other direction and commit 2 is additive.Why land a change that fixes nothing
Because the next occurrence answers the question instead of restating it. Tonight's log could not distinguish three causes; the next one can. Against that:
scripts/is untouched. Nothing here can change what any shipped code does.Measurements, at this head
That green proves only that nothing regressed. The assertion under investigation passes locally on this machine every time — #769 records the same thing for its own assertion — so a local pass is what both hypotheses predict and it discriminates nothing. The CI run is the evidence.
No mutation table: this changes a test helper's timing and its diagnostics, not a behaviour. There is no property to break. The forced (a)/(b)/(c) cases above are what stands in for one — each print was made to appear rather than assumed.
Drift
From the repo root, immediately before the push, against
origin/integration/remote:Re-measured immediately before landing.
Boundaries
macos-latest 4/4; tonight's runs areubuntu-latest 4/4. Same helper, same assertion, different shard and OS.aa8077e3was given before commit 2 existed and does not carry to this head.