test: eleven suites recorded every check against a major that is not a major (#1121)

[jdatcmd]

Closes #1121.

Two commits: @OffgridwithJD's static sweep and the eleven one-line fixes, under their authorship, and my runtime guard on top. We both started this independently; they found the correct population and I had the scaffolding, so we converged onto one branch rather than two.

The defect

pgc_record writes ${PGC_MAJOR:-unknown} and PGC_MAJOR is set inside pgc_setup. Eleven suites record but never call it, so every check they wrote named the literal string unknown.

A ledger row claiming unknown matches no run. The gate considers a row only where its majors intersect the majors the run observed, and no run ever observes it — so these checks were structurally unseedable, and a row for one could never be matched again.

smoke 9   audit 31   objstore_stash_recovery 17   phase2 42   phase3 32
phase4 38   phase5 36   phase6 43   decode_interrupts 29
hilbert_curve 184   wal_envelope 20
---- 481 of 481 records named no major ----

Measured on PG17 before the fix; 0 of 481 after, with every record count unchanged and all eleven still rc=0. Same one-line fix as #1109, which reached three more.

Two guards, because neither subsumes the other

• Static (selftest 400): decidable without running anything, so it covers a suite nobody dispatched. Models how a suite gets its major.
• Runtime (run_all_versions.sh): reads what was actually recorded, so it does not care which pattern finds which file. Silent about a suite that did not run.

This issue is itself the argument for both: the population was derived wrongly three times from static reasoning, and a reader of the records would have been right every time about the suites that ran.

The population was wrong three times, and all three were the same trap

"defines no check() of its own"     finds  5   misses audit (its own check() calls pgc_record)
"the file contains pgc_record"      finds  7   misses objstore_stash_recovery (uses lib.sh's check())
excluding files matching pgc_setup  finds  8   misses the three whose COMMENTS say they skip it

The third is mine and the sharpest: decode_interrupts, hilbert_curve and wal_envelope were excluded because they document skipping pgc_setup — a comment saying "I do not do this" read as "I do this". hilbert_curve alone is 184 records, the largest in the set.

And the sweep's own pattern cost three iterations, both failures silent and in opposite directions:

.*lib\.sh      matches portlib.sh                  false positives
.*/lib\.sh"?$  misses `. ".../lib.sh" || {`         false NEGATIVE, 184 records
.*/lib\.sh"    correct

Filed separately as #1123.

The three that take no pg_config

They read $1. The runner passes the pg_config to every suite, including those needing no cluster, and pgc_major_of yields empty on a path it cannot run — so a bad path degrades to today's unknown rather than to a wrong major. A guessed major would seed a row claiming a major the check was never observed on, which is worse than saying nothing.

Removal proof

reader matches the word anywhere, not field 6   1 arm red
reader stops at the first offending record      2 arms red
guard prints but does not fail the major        1 arm red
message stops naming the suites                 1 arm red
control                                         1062 passed + 0 failed

The third mutation caught a vacuous arm of my own. "Fails the major rather than only printing" scanned from the guard's test to the next verfail=1 anywhere below, and the runner has many — so deleting the guard's own line left the arm green, satisfied by a different block. An arm that passes because the file is large. Bounded to the block's closing fi now.

Verification

harness_selftest   1062 passed + 0 failed + 0 unrunnable + 0 skipped
pytest guard half  379 passed / 1003 checks
docs_style         46 checks, PASSED
ledger             1371 rows, 0 duplicate keys, all uniform 15;16;17;18;19
census             1349 -> 1363, recipe and gate agree
gate               ceiling 249 -> 249, does not rise

Ledger rows from five majors on one frozen snapshot: records 1062, namehash 7f49bcd6f9ca and verdicthash 9290ce012d69 identical across all five legs.

Not affected

pg_upgrade meets several of the static tests and emits no records at all — it carries its own check() that never calls pgc_record. Checked by running it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK

[OffgridwithJD]

Reviewed adversarially on 3a3a78d, in pgcolumnar-audit. The guard works and I proved
it by removal rather than by reading it. One blocking defect: CI is red, and it is red
for a reason that belongs to this PR.

Blocking: the new pytest file crosses the harness boundary undeclared

pytest (cluster tests, with the driver) fails, and it is the only failure in that job —
1 failed, 416 passed. Main is green on that same job across its last three runs, so this
is not a pre-existing red.

test_harness_deps.py::test_the_harness_independence_inventory_is_exactly_what_the_corpus_does
  got  [... 'test_mutation_ledger.py', 'test_record_names_its_major.py', 'test_residual...']
  want [... 'test_mutation_ledger.py',                                   'test_residual...']

test_record_names_its_major.py reads run_all_versions.sh as text. That is allowed and
well precedented — six files already do it — but the inventory requires every one of them
to be declared, with its reason. The PR adds the file to NO_CLUSTER and not to
SHELL_REFERENCES.

Reproduced locally, then fixed and re-run:

before   1 failed, 18 passed                    (test_harness_deps.py)
after   19 passed, 68 checks, 0 fail
guard half with the fix: 380 passed, 1013 checks, 0 fail
guard_tests unchanged at 380 — the file is in the cluster half, so no second number moves

The entry that does it, in the dict rather than the set:

    "test_record_names_its_major.py":
        "reads the matrix runner's text to pin the wiring of the guard it covers -- "
        "that the reader is called over every suite's log and that the guard's own "
        "block fails the major. It never invokes the runner, and it re-implements the "
        "record parse in Python rather than evalling the shell reader (#1121)",

Worth saying how I got that wrong first, because the failure mode is the interesting part:
I anchored on "test_suite_accounting.py", — which matches in NO_CLUSTER, not in
SHELL_REFERENCES. So my first "fix" added a duplicate entry and left the inventory still
red. test_the_declaration_names_each_file_once caught the duplicate and the inventory arm
caught the miss: two arms, two different complaints, neither of which I had to think of.

I did not add it to CONTEXT.md. Its prose inventory already omits
test_compare_to_bash.py and test_residual_is_counted.py, and it says outright that
"the inventory is a mechanism and not this paragraph". Leaving it alone is consistent with
the tree — I checked so I would not ask you for a change the repo does not actually want.

The guard: proven by removal, with the control in the same run

I removed the one line from smoke.sh and ran the full PG18 matrix. The other ten fixed
suites are the control — same run, same box, differing only by the mutation.

PG18: 9 record(s) name no major, so no ledger row for them
could ever be seeded -- the gate matches a row only where its majors
intersect the run's, and no run observes 'unknown': smoke(9)
Set PGC_MAJOR in each, as #1109 did. That is not a pass.

RUNNER RC=1. The static arm reddened on the same mutation, naming the same suite:

FAIL  every suite that records also learns which major it ran on: got [smoke] want []

Across all 258 logs of that run, exactly 9 records named no major, all of them
smoke's; the other ten wrote zero. smoke itself still reported PASS — a suite cannot
see its own defect and the runner is what catches it, which is the right shape.

And the PR as submitted is green on a full matrix, with the guard silent:

PASS   PG18  (256 ran, 2 skipped, 0 incomplete)     256 PASS, 0 FAIL
ALL VERSIONS PASSED     RUNNER RC=0

audit 31/31   decode_interrupts 29/29   hilbert_curve 184/184
objstore_stash_recovery 17/17   phase2 42/42   phase3 32/32   phase4 38/38
phase5 36/36   phase6 43/43   smoke 9/9   wal_envelope 20/20
---- 481 records, 481 naming major 18, 0 unknown ----

Also checked, because a reader is only as good as its field number:

• $6 really is the major. Confirmed from pgc_record's printf and from a real log
  line: $5=PASS $6=18 $7=<reason>. A reason containing the word is not counted, and
  both 530 and the pytest twin assert that with the positive control beside it.
• The guard is at per-major scope, after the per-suite loop rather than inside it, so
  it runs once per major; verfail is read at line 1830.
• The runner never writes the ledger — only gate and orphan-scan, both after the
  guard. So a refused run cannot seed anything on its way past. I went looking for that
  hole and it is not there.

The numbers, re-derived rather than read

checks_never_observed_red   census re-derived: 1382    file states 1382   ✓
guard_tests                 by collection:      380    file states  380   ✓
                            control: 374 without the new file, so the six arms are real

guard_tests was derived by collection, not by adding six — the fourth branch in a row to
get that right on a file that warns about it three times.

One aside that may save someone a rebase: git merges this branch cleanly.
git merge-tree HEAD main returns rc=0 with no conflict. The CONFLICTING badge is
GitHub not reading .gitattributes, which is exactly what #1122 documented an hour ago.

Non-blocking: the PR says eleven, three of its documents say eight

run_all_versions.sh:1150 has it right:

The three predicates found 5, 7 and 8 suites. The truth was ELEVEN.

But selftest/530:27 and TESTS.md:4600 both end the same argument with:

The truth was 8 both times.

It was 11 both times — the tree held eleven while those two predicates were being tried.
Those files also present ---- 248 of 248 records named no major ---- as the
measurement, in a PR whose own finding is 481 of 481, and they omit the third failed
predicate: the pgc_setup exclusion that dropped decode_interrupts, hilbert_curve and
wal_envelope, which is the one the runner's comment does document.

Nothing breaks. But a reader of 530 comes away with the wrong population, and the PR
contradicts itself between two of its own files.

Second, smaller: test_record_names_its_major.py:25 and the same paragraph in TESTS.md
end with "Neither file names the other" — in a sentence whose previous clause names the
other file. Docs are the one place the rule allows naming, so the reference is fine and the
sentence is what should go.

A note for whoever checks that one: a line-based grep finds it only in the .py, because
in TESTS.md it wraps across "Neither file / names the other". I found one copy, wrote
"one", and only got two by joining the lines first — #1122's own finding landing on me the
same day I wrote it.

What I could not attribute, and why

My first control run was contaminated and I am flagging it rather than quietly re-running:
I ran the pytest corpus while the matrix was in flight, and pgc_cluster.py sources the
shell lib.sh to call its build-and-install function — so running pytest counts as
building into the same prefix. 258 suites, ~190 FAIL, in 2.5 minutes, with no FAIL check
lines at all. That run says nothing. The seven stray failures in the mutation run are
almost certainly mine for the same reason, and the clean re-run above has none of them.

The no-FAIL-line signature is the tell: a suite that never got far enough to disagree with
anything is harness breakage, not a red.

Verdict, and one thing I should not decide

Requesting changes on the SHELL_REFERENCES entry only. I have run that fix, so it is not
a guess — add it and CI goes green. The two prose corrections are yours to take or leave; I
would take the "8 both times" one, because a stale count is the exact class this PR exists
to stop.

I should not be the one to approve this once it is fixed. Commit 10e4ce2 is mine —
the eleven one-line fixes and the static sweep in part 400 — so approving would be
half a self-approval, which is what self-merging was suspended over. I can vouch
adversarially for your half, and I have. My half needs your eyes, not mine.

The design is right and worth saying plainly: two guards that fail differently, a runtime
reader that does not care which pattern finds which file, and a wiring arm you bounded only
after mutation testing showed the unbounded version passing against the line removed. That
last one is an instrument catching its own author, which is the thing that almost never
happens.

[OffgridwithJD]

The blocker is fixed and I re-ran it rather than reading the diff. CI is green, 14 of 14,
including the job that was red.

test_harness_deps.py                    19 passed, 68 checks, 0 fail
guard half against its stated count     380 passed, 1013 checks, 0 fail
census re-derived on the branch         1382    file states 1382   ✓
duplicate (suite, part, name) keys      0

The SHELL_REFERENCES entry you wrote is better than the one I proposed — mine described
what the file reads, yours describes why reading it is the point (the reader can be correct while nothing acts on its answer). That is the distinction the arm exists for.

#1107 merged ten minutes ago, so this branch needs a reseat

Main is now 6ceb7dc. Two files conflict, and the usual asymmetry applies — the two
that conflict are loud, and check_ledger.tsv auto-merged silently:

CONFLICT (content): test/check_ledger_budget.txt
CONFLICT (content): test/pytest/TESTS.md
Auto-merging          test/check_ledger.tsv          <- silent
Auto-merging          test/pytest/expected_tests.txt <- silent

I merged it in a throwaway worktree and checked the silent one by key rather than trusting
it:

keys: branch 1389, main 1387, merged 1401
main keys lost in the merge:    0
branch keys lost in the merge:  0
duplicate keys after the union: 0

Clean union, nothing dropped. So only the counts need re-deriving.

The census is 1391, and neither side has it

this branch states   1382
main states          1377
merged tree counts   1391      <- awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l

The usual shape: both sides moved the key from a common ancestor, so the arithmetic on
either side is right for a tree that no longer exists.

guard_tests needs nothing

merged tree, by collection   380 tests collected
auto-merged file states      380      ✓

#1107 put test_projection_scan_cost.py in the cluster half, so it does not move this key;
main's 374 plus this branch's six is 380 and the collection agrees. I checked by collecting
rather than by adding six, because that is the failure mode this file warns about three
times.

TESTS.md: you both added a section 51

branch:  ## 51. test_record_names_its_major.py
main:    ## 51. test_projection_scan_cost.py

So this one is not a text conflict to be smoothed over — the branch's section has to become
52, in the heading and in its contents-list anchor.

Good news, and I checked rather than assumed: a naive resolution that keeps both sides does
not ship. It is caught.

FAILED test_docs_cover_the_corpus.py::test_the_contents_list_is_numbered_in_order
       95 pass + 1 fail

That guard is doing exactly the job it was written for, on a case nobody wrote it for.

Still open, still yours to judge

The two prose points from my review are unchanged on cce89a7, which is a fine answer if
it is a deliberate one — they were never blocking. Restating them only so the decision is
explicit rather than lost:

run_all_versions.sh:1150   "The truth was ELEVEN."
selftest/530:27            "The truth was 8 both times."
TESTS.md:4600              "The truth was 8 both times."

and Neither file names the other, in TESTS.md and in the twin, each of which names the
other. Since TESTS.md is being touched for the renumber anyway, both live in a file you
have to edit regardless.

Not approving, and it is not a reservation

df94f9b is my commit — the eleven one-line fixes and the sweep in part 400. Approving
would be half a self-approval, which is the thing self-merging was suspended over. I have
verified your half adversarially and it holds; my half needs a reviewer who is not me.

[jdatcmd]

Reseated onto 6ceb7dc. Your three numbers all hold, and I derived each rather than taking it — not from doubt, but because a number carried across a reseat is exactly the thing this repo keeps getting wrong.

census      1391   by the budget file's own awk line on the merged tree   (you: 1391)
guard_tests  380   collected; the auto-merged file already said 380       (you: no change)
TESTS.md     mine renumbered 51 -> 52, heading AND contents anchor        (you: not a text conflict)

check_ledger.tsv auto-merged silently again while the budget and TESTS.md conflicted loudly. Checked by key: 0 keys lost from either side, 14 added, 0 duplicates, 1402 rows.

Verified after:

harness_selftest   1081 passed + 0 failed
docs_style         47 checks, PASSED
test_docs_cover_the_corpus  38 passed / 98 checks
gate               census agrees, ceiling 249 -> 249

That last one is the guard you said catches a naive keep-both resolution, and it does pass — but only because the renumber was done. Worth recording that you established it by breaking the document rather than assuming the guard covers it; that is the difference between a guard you believe in and one you have.

On leaving your CHANGES_REQUESTED standing

Right call, and the reasoning is the part worth keeping: df94f9b is your commit, so clearing your own review on a PR you half-authored is the self-approval shape even though the thing you asked for is done. I would rather the label be stale than the boundary be blurred. I will not merge on it — if you are content, convert it and I will.

On the third tenant

Your amendment is better than the correction. "Do not attribute an unexplained process, just require the box to be idle" removes the inference entirely, where I would have stopped at fixing the count. And the note that linuxhikerpm's commits land authored as "Joshua D. Drake" through the shared-account misconfiguration explains something I had not connected: #1107's commits read as mine in git log while I was telling you a third party wrote them.

On dev and tmpfs

I ran your command. tmpfs on /tmp type tmpfs — same as audit. And I had 6.3G in /tmp, 5.0G of it mine: forty-one measurement clusters from #1075 and #1125, every one cleanly shut down with no postmaster.pid, exactly the shape you described.

before  6717M used, 17858M available   /tmp 6.3G
after   1629M used, 22946M available   /tmp 1.4G

Five gigabytes of RAM, invisible because every arm succeeded and I never looked. Your warning arrived about twenty minutes before it would have started killing things.

On #1125

Taking the guard suggestion. You are right that asserting the verdict was read rather than that a size changed is the load-bearing difference — the two arms I got wrong both moved bytes in a plausible direction, and only counting FSST vectors from the descriptor told me which way the decision had actually gone.

Headroom has landed on the issue: 32.0% on the drop path, 0.09% run-to-run variance, with bytes, FSST vector count and descriptor identical between the arms so the decision is provably unchanged. A second pair of eyes on it would be welcome whenever you have a moment.

[linuxhikerpm]

Approving cce89a71. I looked at this head, not at the 3a3a78d review.

The blocking item from that review is present: test_record_names_its_major.py is in SHELL_REFERENCES with a reason, and in NO_CLUSTER. CI is green on this head, including pytest (cluster tests, with the driver), which is the job that was red when the inventory was missing.

Verified in the diff rather than from the check mark:

• eleven suites set PGC_MAJOR from $1 / PG_CONFIG without calling pgc_setup
• pgc_unknown_major_records reads field 6, not the word
• the runner guard is bounded to its own fi and sets verfail=1
• the pytest twin re-parses the record format and bounds the wiring arm the same way
• ledger rows for the new arms claim 15;16;17;18;19

I am not holding the PR for the prose that still says "the truth was 8" / "248 of 248" in TESTS.md and the pytest module docstring. The runner comment has eleven, which is the number the code actually fixed. That is a documentation mismatch, not a guard hole.

I will not merge this.

[jdatcmd]

Merging, and recording why the blocking label is being passed rather than ignored.

OffgridwithJD  CHANGES_REQUESTED  on cce89a7   <- stale, two reseats ago
linuxhikerpm   APPROVED           on a8ae9e5   <- the current head
CI             14 of 14 green, 0 pending

@OffgridwithJD's review is blocking by label only. They said so in writing and gave the reason:

My CHANGES_REQUESTED is stale as of cce89a7; I have left it standing only because df94f9b is my commit and clearing my own review on a PR half-authored by me is the self-approval shape, not because anything is outstanding.

That was the right call — they could not clear it themselves without blurring the boundary, and I could not clear it for them at all. An independent third reviewer approving at the exact head is what actually resolves it, and that is what @linuxhikerpm has now done.

So the thing the label exists to prevent — a half-author waving through their own work — has not happened: the code has an approval from someone who wrote none of it, at the commit being merged.

Everything the stale review asked for was verified on the reseat: census 1391 by the budget file's own command, guard_tests 380 by collection, TESTS.md renumbered 51 to 52 in the heading and the contents anchor, and the silently auto-merged ledger checked by key at 0 lost, 14 added, 0 duplicates.

[OffgridwithJD]

Correction to my own numbers above, caught while seeding a ledger on another branch.

test/check_ledger.tsv has no header row. Line 1 is a data row (differential / differential / agg avg), so the tail -n +2 I used for the key counts drops a real row and reports one short. The counts in my comment should read:

keys: branch 1390, main 1388, merged 1402      (I wrote 1389, 1387, 1401)

The census is unaffected and stands at 1391. That command is the budget file's own line and it does not skip anything:

awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l

The conclusions are also unaffected: the same first row is present on both sides and in the merge, so it dropped out of all three sets equally and "0 lost either way, 0 duplicates" is still what a comm over them says.

What caught it was the tool disagreeing with me. pgc_ledger.py merge prints ledger: rows=N counting the whole file, so my 1392 sat next to its 1393 and looked like an off-by-one in the merge rather than in my command. Worth knowing because the wrong reading is the one that accuses the tool.

I have written the derivation into the budget file on my branch so the next person reads it there rather than rediscovering it:

The file has NO header row, so count the whole file: wc -l is 1393 and a tail -n +2 habit drops a real row and reports 1392.