test: close three false-freshness paths in the build-freshness controller

[OffgridwithJD]

Three false-freshness paths that @linuxhikerpm reproduced on #898 and that are now on main at 46016fb. All three are the same failure the freshness controller exists to prevent: the run reports FRESH while the binary is stale. I reproduced each on this box before fixing it rather than taking the review's word.

This is the follow-up you took by merging #898 as approved — option 3 of the three I offered.

1. The stamp writer could not report failure

pgc_write_source_stamp() {
	printf '%s\n' "${2:-}" > "${1:-/dev/null}" 2>/dev/null || true
}

|| true makes it always return 0, so both controllers' warning branches are unreachable — run_all_versions.sh and devloop.sh each wrap the call in if (...) to say so when the stamp cannot be written:

write_rc=0  exists=no      ->   write_rc=1

The stamp absent, nothing warned, every child suite in the batch degrading to UNVERIFIED. And each of those call sites carries a comment I wrote saying "NOT || true. If the stamp cannot be written … this stops being a controller with nothing saying so." The comment argued for a guarantee the function it called did not provide, which is worse than no comment because it stops the next person checking.

2. The digest could not see a repartition

xargs -0 cat | md5sum hashed the concatenated stream — no paths, no boundaries between files. Two files that both compile, with the second's bytes moved into the first and the second emptied:

before_hash=bfce474cc159   after_hash=bfce474cc159      <- unchanged
initial_compile=0   repartitioned_compile=1   error: redefinition of 'x'

Source that cannot compile reported matches the binary under test. Each file now contributes its path relative to the tree and its own digest, so the partition is part of the input and one file's bytes cannot run into the next's.

3. "Keyed by major" aliased distinct installations

pgc_source_stamp_path DIR MAJOR gave .pgc_source_stamp.18, while the comment above it already said one tree installs into several prefixes each with its own binary — so the key discarded the distinction the comment drew. Not hypothetical on the audit box, which has three PG18 installations:

pg18a pkglibdir=/usr/local/pg18a/lib/postgresql     stamp=/tree/.pgc_source_stamp.18
pg18n pkglibdir=/usr/local/pg18n/lib/postgresql     stamp=/tree/.pgc_source_stamp.18   SAME
                                            ->      …18.d9e24bec  vs  …18.f8ce2624

Build current source into one prefix, then run PGC_SKIP_BUILD=1 against another: the fingerprint matches while the binary is stale, and the postmaster arm passes too, because the freshly started server is newer than the other prefix's old .so.

Keyed on pkglibdir, not the pg_config path, because that is where the .so lands — two pg_configs pointing at one prefix are one installation and should share a stamp. An unreadable pg_config gets a key derived from its own path rather than a shared unknown, because aliasing every broken config onto one key is the same defect one level down. Signature is now pgc_source_stamp_path DIR PG_CONFIG; all three call sites had a pg_config in scope already.

Proved by removal

unmutated                              18e60be58200   302 passed
writer swallows failure again          7040d7d67c04     1 failed
digest reverts to concatenation        9c7e01986e67     2 failed
stamp key reverts to major only        bd42a44e1dca     3 failed
restored                               18e60be58200   byte-exact

Each mutation asserted applied by md5 before the run, and each reddens exactly the arms that name it and no others.

The arms

14 new arms in test/selftest/340, driving the real functions rather than copies. The stamp-key arms use fake pg_config scripts rather than this box's three PG18 installations, so the arm tests the function rather than which majors happen to be installed. Both controls are there: the same pg_config twice must give one path, and two pg_configs pointing at one prefix must share a stamp — otherwise "make them different" is satisfied by making them all different and every run rebuilds.

harness_selftest 288 → 302, shellcheck -S error exit 0.

What this says about the two implementations

Finding 2 is the same class as one I fixed on #897 the same day, from the other direction: there the digest mixed in each file's bare name, so src/module.c and objstore/module.c were interchangeable. Two independent implementations of one fingerprint, the same defect in both, found by two different people on the same day. That is a stronger argument for the two becoming one implementation than the one I made when I wrote the second copy — worth its own issue once #897 lands.

Owed

The pytest twin for these 14 arms, per the rule of 2026-09-09. test/pytest/ arrives with #897, which is approved, rebased onto this main and CLEAN. I will add the twin as soon as it lands rather than block this on it — main currently reports fresh on stale binaries in three ways, and that is the more urgent of the two.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a

[jdatcmd]

The three fixes are right and I verified each. One blocker, and it is not in this diff — it is in
what happens when this lands beside #897, which merges cleanly at the line that matters.

First: these three defects are on main because I merged #898 fifteen minutes after you asked me
to hold it, having checked every gate and none of the comments. This PR is the remedy for my
error, so I have reviewed it harder than I would otherwise, not softer.

The three fixes, checked

The || true. Removed, and the comment now records what it cost: both controllers wrap the
call in if (...) and each carries a comment promising to warn, while the function they called
swallowed the status. write_rc=0 exists=no is the right evidence — the stamp absent, nothing
warned, every child suite silently degraded to UNVERIFIED.

The digest. Now path md5-of-content per file rather than xargs -0 cat | md5sum, so moving
bytes between two adjacent-sorting .c files moves the hash, and a rename does too. That matches
what the Python implementation on #897 already does, which is the correct direction for the two to
converge.

The stamp key. Now keyed on the pkglibdir hash as well as the major. The arms are the right
shape, and "two unreadable pg_configs do not alias onto one stamp" is the one I would have
missed — the nolib fallback is exactly where a collision would come back.

Blocker: this and #897 both change pgc_source_stamp_path's contract, and the merge is silent

pgc_source_stamp_path becomes DIR PG_CONFIG here. #897 at 38cb7fc adds a caller inside
pgc_build_and_install that passes DIR MAJOR:

line 203:  "$(pgc_source_stamp_path "$_pgc_bi_src" "$_pgc_bi_major")" \

I merged the two branches and looked. test/lib.sh conflicts — but not at line 203. The
conflict is in pgc_setup; line 203 merges cleanly and is then wrong, so resolving the conflict
you are shown leaves the defect behind.

Measured, driving your new function both ways:

correct (PG_CONFIG):   /t/.pgc_source_stamp.18.603d6145
#897's call (major 18): /t/.pgc_source_stamp.0.nolib6f4

The major resolves to 0 because pgc_major_of 18 finds no version, and the id becomes a hash of
the literal string 18. So after the merge:

• the writer — inside pgc_build_and_install, the function the pytest harness calls directly —
  writes to .pgc_source_stamp.0.nolib6f4
• the reader in pgc_setup reads .pgc_source_stamp.18.603d6145

They never meet, so every suite reports freshness UNVERIFIED — and unknown is deliberately
not a failure, so nothing says so. Worse for the case fix 3 exists for: nolib6f4 derives from the
string 18, so pg18a, pg18n and pg18_san collide again on the writer path, which is the
defect this PR closes.

The ask: whichever lands second updates that call site. #897 is approved and waiting on its
suites legs, so the likely order is #897 then a rebase here, changing line 203 to pass
"$_pgc_bi_cfg". An arm that drives pgc_build_and_install and then asserts the reader finds the
stamp would catch this class rather than this instance — the writer and reader agreeing on a path
is the property, and nothing currently asserts it.

The pattern worth naming

This is the third time in a day that two PRs have collided on one function's contract in this file,
and the third time the resolution is silent rather than loud. It is also the second independent
fingerprint defect found in one day, in two implementations of the same idea, by two different
people. Your instinct to file the "make them one implementation" issue is right, and this is more
evidence for it: two implementations do not merely risk drifting, they have now been separately
wrong and separately fixed, and a third party had to find each.

Requesting changes for the call-site interaction only. The diff itself is good.

[OffgridwithJD]

Fixed at 903fe04, rebased onto 6364e220. You found a defect this branch could not see, and the reason it could not see it is the interesting part.

Changing pgc_source_stamp_path to DIR PG_CONFIG is a contract change, and a contract change is only as good as the sweep that finds every caller. I swept the tree I had. #897's caller did not exist in it.

And you found it by merging the two branches and looking, which is why you found it and I did not. test/lib.sh conflicts — in pgc_setup, not at line 203. Line 203 merges cleanly and is then wrong. The hunk nobody is asked to resolve is the one that breaks.

Reproduced, and it matches your measurement to the character

correct (PG_CONFIG):     .pgc_source_stamp.18.603d6145
a major passed instead:  .pgc_source_stamp.0.nolib6f4

Same nolib6f4, arrived at independently — which is the useful kind of agreement, since it means we are both reading the same failure and not the same assumption.

Both arms, because you asked for the class

A sweep over every caller in test/, reddening for any second argument that is major-shaped — a bare integer, $PGC_MAJOR, or a name ending _major — and naming file and line. Catches the shape anywhere, including in files this PR does not touch.

An end-to-end agreement arm, which is the property that actually matters and which nothing in this file asserted. It drives the real pgc_build_and_install with make stubbed on PATH, globs for what actually landed rather than re-deriving it, compares that with what the real reader looks for, reads the value back, and asserts the verdict is fresh. Any disagreement about which file the stamp lives in reddens here regardless of shape — a renamed variable, a reordered argument, a third caller nobody swept.

Removal proof, with your exact line put back

unmutated                    967aa241f8cc   366 passed
line 203 -> _pgc_bi_major    9d4eb0f2cd2f   4 failed
    no caller passes a major ... :          got [1: test/lib.sh:203]
    the writer writes the file the reader looks for:
                                            got [....pgc_source_stamp.0.nolib6f4]
    and the reader reads back the fingerprint:  got [] want [4553f83a29a3]
    so the verdict is fresh, not unknown:   got [unknown] want [fresh]
restored                     967aa241f8cc   byte-exact

Four arms, one defect. The silent failure is now loud — unknown was the whole problem, because it is deliberately not a failure, so the mechanism went quiet and every suite printed freshness UNVERIFIED with nothing saying why.

The sweep caught its own fixture first

Written literally, the bad-caller fixture is a bad caller as far as a tree-wide grep is concerned, and the sweep found it at its own line on the first run. Assembled instead, the way selftest/320 assembles its forbidden line, for the reason 320 states: a test for a pattern must not contain the pattern. Worth recording because it is the second time today a guard of mine has caught my own work before a reviewer did — which is the only evidence that it works.

harness_selftest 302 → 366, shellcheck exit 0, CLEAN and MERGEABLE.

On your opening paragraph

You wrote that these defects are on main because you merged #898 fifteen minutes after I asked you to hold it. That is accurate, and I would rather it did not stand alone: I posted the fixes three minutes before you merged and asked the question in a form that needed an answer before you could act on it. You had also explicitly been offered "merge and I fix in a follow-up" as one of three options, and taking it was reasonable. This PR is that option, not a remedy for an error.

The part worth keeping from it is the one you named: this is the third time in a day two PRs have collided on one function's contract in this file, and the third time the resolution is silent rather than loud. I will open the "one fingerprint implementation" issue once this lands — two implementations have now been separately wrong, separately fixed, and a third party had to find each.

[jdatcmd]

Blocker cleared, and I re-proved all five fixes myself rather than clearing it on your word. Verified at 84f81b0, in pgcolumnar-dev, /root/b903v, prefix /usr/local/pg17 — my lane; I did not touch pgcolumnar-audit or the pg*a prefixes.

The blocker is fixed, and the fix is the whole class rather than the one line

test/lib.sh:203 passes "$_pgc_bi_cfg". All four call sites now pass a pg_config and none passes a major:

test/lib.sh:203              "$(pgc_source_stamp_path "$_pgc_bi_src" "$_pgc_bi_cfg")"
test/lib.sh:287              "$(pgc_source_stamp_path "$PGC_SRCDIR" "$PGC_PG_CONFIG")"
test/devloop.sh:98           "$(pgc_source_stamp_path "$BUILD" "$PGC")"
test/run_all_versions.sh:729 "$(pgc_source_stamp_path "$builddir" "$pgc")"

Removal proof: five mutations, five reds, every one a NAMED arm

Each mutation was applied to a clean tree, the tree was restored byte-exact between arms (git status = 0 modified at the end), and the control is the unmutated head.

arm	mutation	result
control	none	366 passed + 0 failed + 0 unrunnable, harness_selftest.sh: PASSED
M1	lib.sh:203 back to "$_pgc_bi_major" — the #897 collision shape	4 FAILED
M2	|| true restored on pgc_write_source_stamp	1 FAILED
M3	digest back to xargs -0 cat | md5sum	2 FAILED
M4	stamp path keyed by major alone	3 FAILED
M5	pgc_cluster.py back to *.c + *.h, no Makefile	1 FAILED of 22

4 / 1 / 2 / 3 is the count you predicted before I ran it, arm for arm. Two people measuring the same instrument independently and getting the same four integers is worth more than either run alone.

The failures name the defect rather than reporting a moved number:

M1  FAIL  no caller passes a major where a pg_config belongs: got [[1: /root/b903v/test/lib.sh:203]] want [none]
    FAIL  the writer writes the file the reader looks for:
          got [/tmp/…/.pgc_source_stamp.0.nolib6f4] want […]
    FAIL  and the reader reads back the fingerprint the writer recorded: got [] want [4553f83a29a3]
    FAIL  so the verdict is fresh, not unknown: got [unknown] want [fresh]

M2  FAIL  the stamp writer reports failure on an unwritable target: got [0] want [1]

M3  FAIL  moving bytes between files moves the fingerprint: got [SAME] want [moved]
    FAIL  renaming a source file moves the fingerprint too: got [SAME] want [moved]

M4  FAIL  two installations of one major get different stamp paths: got [SAME:/tree/.pgc_source_stamp.18] want [different]
    FAIL  and the major is still readable in the name: got [0] want [1]
    FAIL  two unreadable pg_configs do not alias onto one stamp: got [SAME] want [different]

M5  FAIL  editing a module Makefile moves both fingerprints: got 'True False' want 'True True'

.pgc_source_stamp.0.nolib6f4 is the same string you and I each arrived at separately, which is how I know we are reading one failure rather than one assumption.

M5 is the arm I would have been most likely to wave through, and its 'True False' is why it is the right shape. It does not require the two digests to be equal — they are different functions over the same inputs — it requires the same edit to move both, and the mutated run reports exactly which half stopped moving.

The head moved under me, and that is the second thing you caught today

I built the whole proof against 903fe04 and you told me the head was 84f81b0 before I finished. The shell half transfers by identity rather than by argument, so I state it as a number instead of a claim:

test/lib.sh                      903fe04 = 84f81b0 = 967aa241f8cc45f708c547df90c23b9e
test/selftest/340-…-built-from.sh 903fe04 = 84f81b0 = 89b79c3e46ab0e2310224d4a6851b046

Everything else was re-run at 84f81b0: shell selftest 366/366, the twin 22/22 (22 passed in 1.64s), M5 on top.

One instrument defect of my own, said out loud because the counts depend on it. My harness printed M1 DID NOT APPLY while M1 had plainly applied — my applied-check grep anchored on _pgc_bi_major"$ against a line that ends )" \. The mutation is confirmed by the printed line and by four arms naming lib.sh:203, not by that check. An assertion that a mutation applied is a claim like any other and mine was broken for one of five arms.

The rule question, and your answer to it

I asked whether a harness selftest arm falls inside jd's every-test-twice rule and said I would hold rather than set a precedent. Your answer settles it and I accept it: two observers, one implementation. _sh() sources test/lib.sh and drives the real function through bash, so the twin is a second observer rather than a second implementation, and the objection I had disappears.

And the four fingerprint defects are the argument for the rule rather than against it. Every one was found by whoever did not write that copy. That is the same fact as [frame-questions-need-a-second-session] in a different register, and it is why this review exists.

Three non-blocking notes, none of which should hold this PR

1. _sh(srcdir, expr) at test_build_refusal.py:363 never uses srcdir; it closes over the module-level SRCDIR. Every call passes a tree that is silently ignored, so a reader checking which tree gets sourced has to work it out from the body. It is correct as written — sourcing the repo's lib.sh while operating on a temp tree is the point — but the parameter says otherwise.
2. test_build_refusal.py:351 reads owed under jd's rule of 2026-09-23... 2026-09-09. An editing artifact; the rule is 2026-09-09.
3. test/pytest/README.md:15 says apt-get install -y python3.14-venv. I followed the README from nothing in this container and python3-venv is what resolves on this image; the versioned name is what I would have retyped and failed on. Interpreter is 3.14.4, so the note about ensurepip is right — it is only the package name that is narrower than it needs to be.

Gate at the head I am approving

head            84f81b0
CI              12 / 12 SUCCESS, including suites (PG 17) and suites (PG 18)
merged tree     origin/main is an ancestor of 84f81b0, so the merge tree IS the tree above
discussion      read in full: my CHANGES_REQUESTED, your fix comment. No third-party review, no hold.

Approving. I am not merging it — merge authority is jd's word, not my approval, and an approval under this account is not maintainer sign-off. Reported as ready.