test: make the midpoint budget case event-driven and drop a duplicate suite - #6
Merged
Conversation
… suite CI failed on 37b53fd with two suites red, neither a product defect. The same content had passed as PR #5's check minutes earlier. t14_midpoint_warning_continues_same_job asked for a job to complete inside a window of (ceiling/2, ceiling) and then spent seven serialized poll cycles crossing it, each spawning a node process, against an 8-second ceiling. Every unit of runner slowness ate the margin until the loop cancelled at its deadline — behaving exactly as designed — and the case read that as failure. It had already failed this way on c2f2ea9 nine days earlier. Drive completion from the event instead of from the clock. The fixture's MAESTRO_TEST_JOB_TERMINAL_FLAG, which tests/job-lock.sh already uses, reports the job as running until a named file exists. The case now waits for the halfway warning, then creates that file, so completion is causally after the warning and one poll cycle away rather than seven. The ceiling lives in a single variable the assertion pattern derives from, so the two can no longer drift apart. All four assertions are unchanged. tests/shared-git-dir.sh re-ran the entire lease suite that tests/run.sh had already run and passed in the same job, piping it through `tail -1` so a failure inside it could not be diagnosed at all. It added no coverage, cost ~55s, and doubled the chance that any timing-sensitive lease case failed the build. Its other checks stay; the re-run goes. 42.87s to 2.07s. The remaining changes widen nine event waits from 5s to 30s across liveness, lease, and provenance-edge. These are preventive, not fixes for anything observed: each loop breaks as soon as its event arrives, so a larger bound costs nothing when the host is healthy and removes a false negative when it is not. A genuinely absent event still fails, only later. t19 is deliberately unchanged. It fails in the Codex sandbox because ps reports no process start identity there, so the contender fails closed instead of waiting; it passes on the GitHub runner and on developer hosts. Verified with the full suite under a fabricated empty HOME and /bin/bash 3.2, which is what the runner actually provides: 18 passed, 0 failed.
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 the CI failure on
mainat 37b53fd (run 31552000770), where two suites went red.Neither was a product defect — the identical content had passed as PR #5's check minutes
earlier.
liveness.sh—t14_midpoint_warning_continues_same_jobThe case required the job to complete inside a window of
(ceiling/2, ceiling), and thenspent seven serialized poll cycles crossing it — each spawning a node process — against
an 8-second ceiling. On a loaded runner the accumulated poll cost passed the ceiling, the
loop cancelled at its deadline exactly as designed, and the case reported
rc=11 want 0.This is intermittent, not new: the same case failed on c2f2ea9 nine days ago and passed on
b3adebc. Enlarging the numbers cannot fix it — the window stays proportionally
(ceiling/2, ceiling)under any values.Fix: make completion causally follow the warning instead of racing it. The fixture
already supports
MAESTRO_TEST_JOB_TERMINAL_FLAG(tests/job-lock.shuses it), whichreports the job as running until a named file exists. The case now waits for the halfway
warning, then creates the file, so completion is one poll cycle after the warning rather
than seven. The ceiling is a single variable that the assertion pattern derives from, so
the two cannot drift. All four assertions are unchanged.
Counterexample, made deterministic with the fixture's
MAESTRO_TEST_STATUS_HANG(delayseach status call, i.e. "a loaded runner" on demand):
MAESTRO_TEST_STATUS_HANG=1rc=11 want 0— the exact CI symptomshared-git-dir.sh— duplicate lease suiteIt re-ran the entire lease suite that
tests/run.shhad already run and passed in thesame job, piping it through
tail -1so a failure inside it was undiagnosable — which iswhy the original CI log could not say which case broke. No added coverage, ~55s of CI time,
and a doubled chance that any timing-sensitive lease case fails the build. Every other check
in the file stays.
42.87s → 2.07s.
Wait bounds (preventive, labelled as such)
Nine event waits go from 5s to 30s across
liveness.sh,lease.sh, andprovenance-edge.sh. These fix nothing observed — they are hardening. Each loop breaks themoment its event arrives, so a larger bound costs nothing on a healthy host and removes a
false negative on a loaded one; a genuinely absent event still fails, just later. Only loops
that break on success and report "never happened" were widened; no settling delay before a
negative assertion was found.
t19— deliberately unchangedt19fails only in the Codex sandbox, withMAESTRO_LOCK: process start identity unavailable for pid=.... Thereps -o lstart=returnsnothing, so the contender fails closed instead of entering the wait and never prints the
string the case polls for. No bound can fix that. It passes on the GitHub runner
(
PASS waiting contender TERM never cancels the active ownerin the very run being fixed)and on developer hosts.
Verification
Full suite under a fabricated empty
HOMEwith/bin/bash3.2 — what the runner actuallyprovides, and the environment in which
bash tests/lease.shalone would have given a falsegreen: