Read the suite list the way the runner reads it, and sort it (#469) - #473
Conversation
…er line (commandprompt#469) Two problems, and the second was the serious one. The suite list was a single backslash-continued line, so every pull request that added a suite edited that line and any two conflicted by construction. commandprompt#469 counted four in one day; four more happened the night commandprompt#446, commandprompt#468 and commandprompt#444 landed. It is now one name per line, so two such branches touch two different lines. The dangerous part was never the conflict, it was the resolution. Appending the new name after the closing paren is valid shell that `bash -n` accepts. I had recorded it as "leaves a stray command that fails at run time"; that is wrong, and measuring it while writing this test is what corrected it: SUITES=(alpha beta gamma) stray_name -> stray_name: command not found -> ${#SUITES[@]} is 0 `NAME=value cmd` scopes the assignment to that one command and an array literal is no exception, so the array is left UNSET and the matrix runs no suites at all. The runner's "NO SUITES RAN" guard is the backstop. And the check that should have caught it could not. harness_selftest derived the list with an awk range plus sed plus grep, which is a reimplementation of bash's array parsing, and the two disagree on precisely this mistake: bash sees no array, awk sees a full list plus the stray name as a member. So "every suite is registered" passed while the array was destroyed. Measured: against a runner carrying the mistake, the awk parser reports 130 names where bash reports 0. The runner now answers `--list-suites`, handled before the run lock because harness_selftest calls it from inside a running matrix. The gate asks the runner instead of parsing it, so there is one parser, bash's, and no way for the two to drift. Verified by removal: restoring the awk parser turns the new checks red. The reformat holds the list byte-identical and in order, compared through `--list-suites` before and after rather than by reading the diff, which is the one thing this file's history says never to trust. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L2DvnWDM7g27ubDCQdXhky
…ts (commandprompt#469) One name per line was not enough, and measuring it is what showed that. Branching twice off a base, adding one suite on each branch, and merging: one line, both additions on the same line CONFLICT one per line, both appended at the end CONFLICT one per line + sorted, names far apart clean one per line + sorted, names that sort adjacently CONFLICT Everyone appends at the end. That is the shape all four of commandprompt#469's conflicts had and all four of this session's, so one-per-line on its own would have left every one of them still conflicting. Sorting is what gives a new suite an insertion point decided by its name, so two unrelated additions land in different places. Stated honestly: this is a large reduction, not a cure. Two suites whose names sort next to each other still collide. What has gone for good is the destructive resolution, because the closing paren now sits on its own line, and harness_selftest catches that mistake whatever the layout. The sortedness check is what keeps the property. Without it the order decays the first time somebody appends by hand and the reduction quietly disappears. Run order changes: the array is walked in order, so the summary now prints alphabetically and ports are assigned in that order. Neither is a correctness property. The parallel batch runs six at a time with no ordering guarantee already, and runs_alone still selects the sequential ones. Verified that the SET of suites is unchanged, through --list-suites rather than by reading the diff: 129 before, 129 after, identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L2DvnWDM7g27ubDCQdXhky
…andprompt#469) The five-major matrix failed harness_selftest on PG16 and PG17 while PG15, PG18 and PG19 passed: FAIL every suite is registered in run_all_versions.sh: got [unregistered: differential parallel_copy parallel_vector_agg planner_choice_quality] want [none] Those four are registered. Different names on each major, and only under the matrix, which is the signature of load rather than of a missing entry. listed_suites forked `bash run_all_versions.sh --list-suites` on every call, and the two checks below it call it once per test file: about 250 forks, inside a six-way parallel matrix. A transient failure to fork returns an empty list, and an empty list reads as "this suite is unregistered", naming whichever files happened to be in hand at the time. So ask once, for the real runner, and cache it. The fixture path still takes an explicit argument, because that one has to parse a different file. An empty answer is now a named premise rather than a silent cause of an intermittent red against innocent suites. That is the failure mode this whole change exists to remove, so leaving my own version of it in place would have been a poor joke. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L2DvnWDM7g27ubDCQdXhky
Matrix: ALL VERSIONS PASSEDZero Confirmed the tree under test actually carried the change rather than trusting the green: the build tree has the sorted array, The first matrix caught a real defect in this PRWorth recording, because it is the failure class this PR exists to remove and I had reintroduced it. The first run failed on PG16 and PG17 only, while PG15/18/19 passed: All four are registered, and the names were different on each major. That is load, not a missing entry.
So an intermittent red, against innocent suites, caused by the gate rather than the code. Fixed by asking once and caching, with an empty answer now a named premise instead of a silent cause. The fixture path still takes an explicit argument, since it parses a different file. Two things this depended on: keeping the whole matrix log instead of piping it through Leak checkThe matrix left 0 orphaned postmasters, which is #446 and #471 holding. |
…#469) harness_selftest reported "unregistered: parquet_nested_import" on #476's PG18 CI, in a run whose own summary listed that suite as having PASSED. The two are not in conflict; the check was wrong. listed_suites | grep -qx "$name" || unregistered="$unregistered $name" This file runs under `set -o pipefail`. `grep -q` returns the moment it matches, which closes the pipe while printf is still writing; printf takes EPIPE and exits non-zero, and pipefail reports the PIPELINE as failed even though grep matched. A registered suite is then recorded as unregistered. The CI log carries both halves: "printf: write error: Broken pipe" from line 209, and the false red. The note above the cache blamed transient fork failures for this symptom in the #473 matrix -- innocent suites, different names each run, some majors only. That diagnosis is wrong, or at least incomplete: caching removed the forks and the symptom survived. A race between grep exiting and printf finishing explains every observed property, including why it never reproduces locally. Measured rather than argued: 4,000 names, matching the first, 200 attempts. With pipefail, 18 false negatives; without it, 0. At the real list size of 132 short names it does NOT reproduce locally -- the whole list fits the pipe buffer, so printf's single write completes before grep can exit -- which is consistent with it having been seen once, under CI load, on a name near the end of the list. The fix is to stop piping. Membership is now a case over the cached string, with newlines on both sides for the whole-line match grep -x provided. No reader, no EPIPE, no race at any list size. Three controls come with it, because a matcher that always matched would also have made the red go away while making the check meaningless: a registered name must be found, an absent one must not be, and a prefix of a registered name must not count as registered. Proven by removal: deleting analyze_differential from SUITES fails the check with that name. 43 checks green on PG18 and PG19. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013sY1RQR5MjZNUixA2um31r
Takes the item #469 asked for first: "Once #444, #446 and #468 land, I would like to do this first, before anything else goes in." All three landed.
It did not end where I expected. Two findings changed the shape of the fix, and both came from measuring rather than reasoning.
Finding 1: the bad resolution does not leave a stray command, it destroys the array
I had this recorded as "leaves a stray command that fails at run time with
foo: command not found". That is wrong:NAME=value cmdscopes the assignment to that one command, and an array literal is no exception. So the name after the paren does not join the array, it leavesSUITESunset and the matrix runs no suites at all. The runner's existing "NO SUITES RAN" guard is the backstop that stops that being a silent green.Finding 2: the check that should have caught it was blind by construction
harness_selftestderived the list withawk '/^SUITES=\(/,/\)/' | tr | sed | grep, which is a reimplementation of bash's array parser. Against a runner carrying the mistake:So "every suite is registered in run_all_versions.sh" passed while the array was destroyed.
The runner now answers
--list-suites, handled before the run lock becauseharness_selftestcalls it from inside a running matrix and taking the lock there would refuse to answer. The gate asks the runner instead of parsing it, so there is one parser and no way for the two to drift. Verified by removal: restoring the awk parser turns the new checks red.Finding 3: one name per line does not fix the conflicts
This is the part I would have shipped wrong. Measured by branching twice, adding a suite on each branch, and merging:
Everyone appends at the end. That is the shape all four of the conflicts in #469 had, and all four of mine the night #446/#468/#444 landed, so one-per-line alone would have left every one of them conflicting. Sorting is what gives a new suite an insertion point decided by its name.
The real-world shape, measured before and after:
Stated plainly: this is a large reduction, not a cure. Two suites whose names sort next to each other still collide. What has gone for good is the destructive resolution, because the closing paren is now on its own line and the gate catches that mistake whatever the layout.
A sortedness check keeps the property. Without it the order decays the first time somebody appends by hand and the reduction quietly disappears.
What changes for readers
Run order. The array is walked in order, so the summary prints alphabetically and ports are assigned in that order. Neither is a correctness property: the parallel batch already runs six at a time with no ordering guarantee, and
runs_alonestill selects the sequential ones. The set of suites is unchanged, verified through--list-suitesbefore and after rather than by reading the diff, which is the one thing this file's history says never to trust: 129 before, 129 after, identical.Not taken
Generating the list from
test/*.shwith an exclude list, the other option #469 offered. It would end the conflict completely, but it converts "a stray.shintest/turns the gate red until you decide what it is" into "a stray.shis silently gated". Recorded so the choice is visible rather than silently taken.Gate
Full five-major matrix, because this changes how every suite is scheduled. Results to follow in a comment.