Twenty-four checks across eight suites share a ledger key with another check, because a readable-log convention collides with a keyed record

[OffgridwithJD]

Six checks in 400-a-check-result-must-be-machine share three names, so the ledger holds three rows for them. One of the six going red would mark its namesake observed-red too, for a claim nothing attacked.

@jdatcmd spotted this in pgc_ledger.py merge's own output while regenerating for #978 — the tool says it out loud and we had both scrolled past it:

duplicate check name in one run, so one ledger row covers 2:
  harness_selftest  400-a-check-result-must-be-machine  and its human line is unchanged
  harness_selftest  400-a-check-result-must-be-machine  and its verdict is SKIP

Measured

On a harness_selftest run at main f69ed1e3 plus #981's eleven arms:

RESULT records in the log:             827
distinct (suite, part, name) keys:     824
keys carrying MORE THAN ONE check:       3
records lost to collapsing:              3
collapsed keys whose checks disagree:     0

The three keys, each covering two checks:

400-a-check-result-must-be-machine   'and its verdict is SKIP'               PASS, PASS
400-a-check-result-must-be-machine   'and its human line is unchanged'       PASS, PASS
400-a-check-result-must-be-machine   'and is counted as a pass, not a skip'  PASS, PASS

Why they collide

Two hand-written blocks testing different helpers with identical names (400-...:386-411):

check "and its human line is unchanged" \
	"$(_tmh 1 check_timing "a timing check" 1 1)" \
	"SKIP  a timing check (PGC_SKIP_TIMING: wall-clock measurement)"

check "and its human line is unchanged" \
	"$(_tmh 1 check_ratio_needs_quiet_machine "a ratio check" 1 1 2)" \
	"SKIP  a ratio check (PGC_SKIP_TIMING: wall-clock ratio)"

One covers check_timing, the other check_ratio_needs_quiet_machine. Both are real, both are wanted, and the names do not say which is which.

Why it matters, and it is the same class as a fix already landed

pgc_record derives the part from BASH_SOURCE precisely because "a key of (suite, name) is not a key of checks, it is a key of check NAMES, and they differ by however many parts share a boilerplate premise" — and one going red "would then mark every sharer as observed red, a claim about a check nothing attacked."

That fixed collisions across parts. This is the same failure within one part, which the part dimension cannot separate.

No mixed verdicts today, so nothing is currently mis-recorded. The hazard is conditional and precise: if check_ratio_needs_quiet_machine's arm ever goes red, the row records ever red, and check_timing's arm inherits a red observation it never earned. checks_never_observed_red then falls by one for a check nobody attacked — and that census is the thing #925 and #918 exist to make trustworthy.

Why it matters for #432 specifically

It puts a wedge between two quantities the seeding design will need to keep straight:

checks that ran            827
rows the ledger can hold   824

Three today, in one part of one suite, found only because the merge tool printed it. The seeding design for 240 suites will be counting exactly this, and "the ledger has seen N checks" is not the same number as "N checks ran" whenever any two share a name inside one part. Nobody has measured the gap across the other 250 suites, because the ledger does not cover them yet — which means seeding is the moment it becomes visible, and a seed built on the larger number would be wrong.

The fix, and why it is not a one-line patch

Name them for the helper they exercise — and check_timing's human line is unchanged, and check_ratio's .... Six lines in one file.

It is not free: it renames three check names, so the ledger gains three rows and loses three, and the census must be re-derived. That is the regeneration tax again, and there are already PRs queued on that one line. Worth doing deliberately rather than wedged into something else.

Worth adding at the same time

A guard. pgc_ledger.py merge already detects this and prints it as a note; nothing fails on it. An arm asserting that no (suite, part, name) key covers more than one check in a single run would make the class impossible rather than merely visible — and it is a count over the RESULT records of a run the harness already produces.

That arm is the more valuable half: this instance is three benign rows, and the mechanism is what stops the next one being three rows where one hides a red.

[OffgridwithJD]

Swept the whole tree before writing the guard, and this is eight suites and twenty-four records, not six checks in one. The cause is a deliberate readability convention rather than carelessness, which changes what the fix should be.

I measured the blast radius first because a guard that refuses (suite, part, name) collisions is only shippable once I know how many suites it reddens today. Over all 252 saved suite logs from the #432 coverage run:

logs                                      252
total RESULT records                    6,331
suites with at least one collision          8
records lost to collapsing                 24
collisions with MIXED verdicts              0

suite	records	keys	colliding keys	records lost
harness_selftest	800	797	3	3
hilbert_cluster	181	176	3	5
objstore_module	30	24	3	6
alter_am_cleanup	45	43	2	2
sorted_pathkeys	113	110	2	3
vector_agg_tlist_shape	68	65	1	3
eager_ordering_record	31	30	1	1
objstore_userinfo	7	6	1	1

Two keys cover four checks each:

hilbert_cluster          'premise: the plan being digested here is the columnar custom scan too'   x4
vector_agg_tlist_shape   'and it still answers correctly'                                          x4

The cause is a convention, and a good one

I expected copy-paste. It is not. The suites use a headline-plus-continuation shape:

check "REFUSE: FILTER inside an expression over aggregates" \
	"$([ "$(vec 'SELECT avg(a) FILTER (WHERE b < 50) + 1 FROM c')" -gt 0 ] && echo yes || echo no)" "no"
ansq "and it still answers correctly" 'SELECT avg(a) FILTER (WHERE b < 50) + 1 FROM %T'

The follow-up is named and it still answers correctly on purpose — it is short because it reads as a continuation of the distinct line above it, and the log is better for it:

PASS  REFUSE: FILTER inside an expression over aggregates
PASS  and it still answers correctly

Four such pairs in that file, four identical continuation names. hilbert_cluster's four are the same shape: a shared premise sentence deliberately phrased to be reusable, which is exactly what pgc_record's own comment praises about harness_selftest's premises — "phrased to be COPIED".

So this is a readable-log convention meeting a keyed-record requirement, not a defect in anyone's care. That reframes the fix: the original title said "six checks share three names", as though someone had been sloppy in one file.

What the fix has to be

A continuation check must carry enough of its antecedent to key uniquely, without losing the readability the convention buys:

and it still answers correctly (FILTER inside an expression)
premise: the plan being digested here is the columnar custom scan too (after the recluster)

About twenty call sites across eight files, each needing a phrase that says which case — which is judgement per site, not substitution. The same shape of work as converting the ten suites in #965: mechanical in form, not in content.

Why the guard still comes with it, and why it cannot come first

The guard — no (suite, part, name) key may cover more than one check in one run — is what makes the convention enforceable instead of advisory. It cannot land before the renames, because it would redden eight suites on the first run.

Today pgc_ledger.py merge prints a note and nothing fails. That note has been printing for an unknown number of runs and two people scrolled past it, which makes this a reading failure rather than a detection failure — and the harness had already done its part. That is the argument for a refusal rather than a better note.

What it means for #432, corrected upward

I told that issue the gap was 827 records against 824 keys in one part of one suite. Tree-wide it is 6,331 records against 6,307 keys.

Only harness_selftest is currently in the ledger, so 3 of the 24 affect the census today. The other 21 become lost rows the moment the 240 are seeded — and a seed sized from the check count rather than the key count would be over by exactly that, silently, with every number still reconciling.

Scope note

The 252 logs are from one PG18 run. Per-major divergence means another major could collide differently — audit's gated arm is the precedent. I have not measured a second major and am not claiming 24 is the number for all of them; it is the number for that run, which is the run the seeding design would be built from.

[OffgridwithJD]

Design note before anyone builds the guard: the debt should be bounded by a committed LIST of colliding keys, not a committed count. And the work is 6 mechanical sites plus 18 that need a phrase chosen by a person.

Posting this rather than building it, because it changes a gate's contract and should be reviewed like one.

The ordering question, settled the other way round

I said the guard cannot land before the renames, because a refusing guard would redden eight suites on its first run. @jdatcmd pointed out the repository already has the shape for a known, bounded, not-yet-paid-down debt, and it is in the same file:

suites_not_covered 250    A CEILING, monotone, may only fall, the gate refuses an increase

So the order inverts usefully: bound it now, pay it down one file at a time, with each PR's diff showing the debt falling. That is better than make-it-possible-then-require-it, and it needs no new concept.

But the bound should be a list, and this is why

A count computed from a run is loop- and environment-dependent. objstore_module's three collisions all come from one loop:

for url in "s3://bucket/key.parquet" "gs://bucket/key.parquet" "https://host/key.parquet"; do
    check     "a $(cut -d: -f1 <<<"$url") URL reports an object-storage error, not a missing file"
    check_num "and does NOT report it as a missing file"          # identical 3 times
done

The headline interpolates the scheme. The continuation does not. So:

• add a fourth scheme and the committed ceiling must be edited for a reason that is neither a defect nor a paydown;
• and that suite carries an environment gate at objstore_module.sh:175 (check_skip "cannot move $MOD"), so a box where the module cannot be moved yields a different count again.

That is the census's major-blindness reappearing in a new budget line — the precise thing #432 is blocked on.

A committed list of known-colliding keys has neither problem. An entry not exercised on some major or some box simply does not fire; a collision not in the list is refused on every box. Monotone by removal, same mechanism as suites_not_covered, and keyed on the thing the ledger already keys on.

It is also the principle @jdatcmd established earlier today for version-gated arms, applied to the budget: commit the names and let the run carry the environment. About sixteen entries, which is barely longer than a number and does not need editing when a loop grows.

If a list is judged too heavy for that file, a count with a comment saying it moves for non-defect reasons is still better than nothing — but the list is the correct artifact.

The work, sized

records lost from LOOPS (discriminator already in scope, mechanical)    6
records lost from HAND-WRITTEN sites (a phrase chosen per site)        18
                                                                      --
                                                                      24

LOOP   objstore_module          3 keys, 1 source occurrence each
HAND   hilbert_cluster 3,  sorted_pathkeys 2,  alter_am_cleanup 2,
       vector_agg_tlist_shape 1,  eager_ordering_record 1,
       objstore_userinfo 1,  harness_selftest 3

Three quarters needs judgement. I had implied all of it was taste; one quarter is not.

The rule, and it preserves the convention rather than replacing it

A continuation check carries the same discriminator its headline already interpolates.

before   check     "a s3 URL reports an object-storage error, not a missing file"
         check_num "and does NOT report it as a missing file"

after    check     "a s3 URL reports an object-storage error, not a missing file"
         check_num "and the s3 URL is NOT reported as a missing file"

No new convention, no lost readability, and at the loop sites the variable is already in scope. For the hand-written sites the discriminator is whatever the headline above it names, which is why those eighteen are judgement and not substitution.

Next step agreed, and deliberately small

400-a-check-result-must-be-machine first, as a pattern-setter with the rule stated — the file both of us have now measured independently, so the shape gets agreed on ground neither of us is guessing about. The #965 precedent is the argument: smoke.sh went first, review found checks run: was read by nothing, and nine files did not repeat the mistake.

The remaining seven follow once the phrasing shape is agreed, each lowering a committed bound.

Independent verification of the harness_selftest row

@jdatcmd measured it on PG16 across three separate trees while I measured PG18: three colliding keys, three records lost, zero mixed verdicts, matching exactly. And their observation that the names confirm the diagnosis rather than the count alone — every colliding name begins with and — is the evidence that this is the continuation convention and not carelessness. That is visible in the data, not inferred.

[jdatcmd]

A design note on how the debt should be committed, not a proposal to build either version tonight. This is a gate's contract and it should be reviewed like one.

First, a correction

@OffgridwithJD credited me with finding the objstore_module case. I did not — they did, while checking whether a count I proposed was stable. What I contributed was the environment-invariant-names principle, and that came from their skip-loop in #972, which they arrived at by accident and I only named.

The case matters more than the principle here, so the credit belongs where the work was.

I proposed a count. The count is wrong, and here is why

A third budget line, keys_covering_more_than_one_check 24, alongside the existing ceiling:

suites_not_covered 250     a CEILING, monotone, may only fall, the gate refuses an increase

Same mechanism, lands with no renames, makes the debt a diff a reviewer sees rather than a note nobody reads, and refuses a new collision the day it is written.

It is wrong because the number moves for reasons that are not defects. test/objstore_module.sh:104:

for url in "s3://bucket/key.parquet" "gs://bucket/key.parquet" "https://host/key.parquet"; do
	check     "a $(cut -d: -f1 <<<"$url") URL reports an object-storage error, not a missing file" ...
	check_num "and does NOT report it as a missing file" ...     # identical, three times
done

The headline interpolates the scheme; the continuation does not. So:

• add a fourth scheme and the committed ceiling needs raising — for a change that fixes nothing and breaks nothing;
• and :175 carries check_skip "the absent and broken module paths" ... "cannot move $MOD", so a box where the module cannot be moved produces a different count again.

Verified both in the source rather than taken on report.

That is the census's major-blindness reappearing in a new budget line, which is precisely the thing #432 is already stuck on.

A list has neither problem

Commit the known-colliding keys, not their number. Then:

• an entry not exercised on some major or some box simply does not fire;
• a collision not in the list is refused on every box;
• monotone by removal — the same shape as suites_not_covered, paid down per file;
• keyed on (suite, part, name), which is what the ledger already keys on.

About sixteen entries against one integer. Barely longer, and it does not lie when the environment changes.

This is the same rule the ten result-blind suites already follow: emit environment-invariant names and let the run carry the environment. Applied to the budget instead of to a suite.

The work is not uniformly a matter of taste

records lost from LOOPS         6    discriminator already in scope, mechanical
records lost from HAND-WRITTEN  18   a phrase chosen per site
                                --
                                24

So three quarters needs judgement and one quarter does not — which is a stronger argument for one-file-first than "it is all taste" was, and @OffgridwithJD corrected their own framing upward to say so.

And the loop quarter has a convention-preserving fix worth stating as the rule: the continuation carries the same discriminator its headline already interpolates —

and the s3 URL is NOT reported as a missing file

No new convention, no readability lost, and the variable is already in scope at every one of those sites.

What is being asked of a reviewer

Three things, in order of how much they cost to get wrong:

1. list or count — the argument above;
2. whether a third entry in check_ledger_budget.txt is a concept too many at all;
3. the phrasing rule for the eighteen hand-written sites, which 400-a-check-result-must-be-machine will propose as one file before it is repeated.