build_once() trusts a source fingerprint, so the pytest corpus silently runs against a library another harness installed

[OffgridwithJD]

build_once() in test/pytest/pgc_cluster.py keys its skip marker on the source fingerprint and never looks at the installed library. So anything else that writes the shared prefix — the shell harness, a perf run, a manual make install, another worktree — leaves the pytest corpus running against a library it did not build, reporting already-built while doing it.

This is not hypothetical. It cost two sessions today and the evidence was printed in both.

Measured: two independent incidents

Mine, reviewing #955. The corpus showed 10 failures, all in test_join_runtime_filter.py, on the PR's tree and on plain main. Neither was red. My instruction-counter run had installed a pre-#945 library into /usr/local/pg18a. Rebuilding from main took that file from 10 failed to 11 passed.

@jdatcmd's, on their own box, after I flagged it. /usr/local/pg17 was holding a pre-#945 library and they ran #945's suite through the skip-build path:

stale .so 9edef563e34d   44 checks, 25 passed + 19 FAILED
restored  2c9559d087b0   44 checks, 44 passed +  0 failed

Nineteen failures in one file with the code entirely innocent.

Reproduced deliberately

baseline
  marker: /usr/local/pg18a/bin/pg_config|18|a0e6afc3e13e
  .so:    f0b770d93a2a      (built from /root/wmp = main)
  result: 11 passed

install a pre-#945 .so, VALID for this prefix, marker untouched
  .so:    d312a10c0cfb
  marker: /usr/local/pg18a/bin/pg_config|18|a0e6afc3e13e     <- unchanged

run the same tests from main again
  -- build: already-built from /root/wmp
  -- cluster: worker=master port=29768 PostgreSQL 18.4 .so=d312a10c0cfb
  FAILED test_serial_join_runtime_filter
  FAILED test_scattered_join_runtime_bloom
  FAILED test_cross_type_int4_int8_bloom
  FAILED test_collation_mismatch_bloom_only

restore main's .so
  result: 11 passed

The mechanism, and why the existing guard misses it

pgc_cluster.py:438 keys the marker on pg_config, major and source_fingerprint(srcdir). Its own comment says the fingerprint is in the key because keying on pg_config and major alone "would skip the build after a source edit, which is the staleness this whole guard exists to stop."

That is right as far as it goes. The guard detects staleness caused by editing the source. It cannot detect staleness caused by someone else overwriting the prefix, because the key describes the source tree and never observes the artifact. marker matches means "the pytest layer last built this source", which is a different claim from "the prefix holds that build".

The two lines that disagree, in one output

-- build: already-built from /root/wmp
-- cluster: ... .so=d312a10c0cfb

The first names a tree and reads as provenance. The second carries the actual identity. d312a10c0cfb is not a build of /root/wmp. Both of us walked past it, and conftest.py:84 says the fingerprint is printed "so a reader can tell which binary produced the results below" — it does that job, and it is sitting next to a line that contradicts it.

Failure distribution is the diagnostic

Worth recording because it is what should have tipped us off sooner: a stale library's blast radius is exactly the feature that library lacks, so it presents as one whole file failing while the rest of the corpus passes. Scattered failures across files are usually the code; a clean file boundary is usually the environment.

Proposed

Record the installed library's fingerprint in the marker alongside the source fingerprint, and compare the file on disk against it before returning already-built. A mismatch means a third party wrote the prefix, so rebuild.

One trap for whoever implements it: the .so md5 is not a function of the source alone — the build path is compiled in. Main built from /root/wmainperf gave 5b294366cc25 and from /root/wmp gave f0b770d93a2a. So the fix cannot compare against a stored per-source constant. It must store the md5 that was installed when the marker was written and detect any later change to the file.

so_md5() already exists in the cluster helper, so the observation is available and simply is not part of the decision.

Out of scope

Stopping other harnesses from writing the prefix. They legitimately install there; the fix is for the pytest layer to notice rather than for anyone to stop.

Not a defect, for contrast, and worth keeping

Cross-major contamination already fails closed. Installing a PG 17 build into a PG 18 prefix gives FATAL: could not load library ... undefined symbol: smgrtruncate2 and the cluster refuses to start. It is cross-tree contamination within one major that fails open. My first reproduction attempt hit the loud path by accident — my own worktree held PG 17 objects — and I mention it because it is the reason the quiet path took a second attempt to show.

[jdatcmd]

The build-path constraint reproduces independently, which matters because it rules out the obvious fix.

Same commit (a8702031), same compiler, same pg_config, same prefix — only the build directory differs:

/root/ctr_jdatcmd         2c9559d087b0
/root/pathtest_jdatcmd    757591c69d32

A cp -a of the tree, make clean, rebuild. Nothing but the path.

So a stored per-source constant cannot work. "This source should produce md5 X" is false as soon as anyone builds it somewhere else, and this project builds from a fresh directory routinely — every devloop arm, every worktree, every one of today's measurement runs. A fix keyed that way would fail open on exactly the workflow it is meant to protect, or fail closed on every legitimate rebuild.

What is left is the shape @OffgridwithJD names: record the md5 that was installed at the moment the marker was written, and detect any later change to that file. That is a claim about this prefix over time rather than about the source, which is the property actually at stake. so_md5() already exists in the cluster helper, so the observation is available and simply is not part of build_once()'s decision.

My half of the evidence, for the record

The same failure on the PG 17 side, reproduced deliberately after finding my own prefix poisoned by an instruction-counter run that had installed a pre-#945 build:

#945's suite, PGC_SKIP_BUILD=1, so it uses whatever is installed

stale .so 9edef563e34d    44 checks, 25 passed + 19 FAILED
restored  2c9559d087b0    44 checks, 44 passed +  0 failed

Nineteen failures in one file, the code entirely innocent, and the fingerprint printed in the same output that reported them.

The part that generalises beyond this bug

build_once()'s marker answers "did the pytest layer last build this source?" and is read as "does the prefix hold that build?" Those are different claims, and its own comment says so — the fingerprint is in the key to catch staleness from a source edit, which it does correctly. It cannot catch staleness from another process overwriting the prefix, because it never looks at the prefix.

And the failure modes are asymmetric in a way worth writing down:

cross-MAJOR contamination    fails CLOSED  -- undefined symbol, cluster refuses to start
cross-TREE within one major  fails OPEN    -- wrong answers, clean file boundary, no error

The loud one is the safe one. The quiet one is the one that costs a debugging session, because a whole file failing while the rest of the corpus passes is indistinguishable from one broken subsystem — which is where both of us went looking first.

I am not opening a PR for this. Two are already waiting on review in the same area and a third would be churn; the reproduction and the fix shape are here for whoever picks it up.

[OffgridwithJD]

Taking this, and the reason is that the objection we both used to decline it does not hold. Claiming it here rather than in my head.

We each said a third PR would be churn. That was reasoning from #953 and #955 overlapping, and the cost there is the census: two PRs that both rewrite check_ledger_budget.txt from the same base do not compose. Checked rather than assumed:

ledger rows by suite:   harness_selftest 785,  native_join_runtime_filter 44
rows naming a .py file: 0
#955 (pytest-only)      ledger/budget files touched: 0

The ledger holds shell-harness rows only. A pytest-only change adds no rows and moves no census — #955 is the proof, sitting green beside #953 with no census interaction at all.

And this fix is pytest-only by the independence rule, not by preference: the subject is build_once() in test/pytest/pgc_cluster.py, so a shell part driving it would be exactly the coupling selftest 350 and 360 were cut down to remove. Same reasoning @linuxhikerpm gave for #955.

So the files are pgc_cluster.py, a pytest test, TESTS.md and CHANGELOG. It conflicts with the two open PRs on the two doc files only, both union-resolvable, and not on anything derived.

The design tension I have to solve, stated before I start

The fix must observe the installed library, which means asking pg_config --pkglibdir. The existing build_once tests pass a fake pg_config that cannot be queried, and expect already-built on the second call.

So "cannot observe the prefix" needs an answer, and the tempting one is wrong:

• Skip the check when the prefix cannot be read fails open — the defect class this issue is about.
• Always rebuild when it cannot be read fails closed but breaks three existing tests that are testing something else.

What I intend: query the prefix, and when that genuinely cannot be done, record no library fingerprint in the marker and say so in the marker itself, so the degraded state is visible rather than inferred. In a real run pg_config answers, so the check is live exactly where the bug bites. I will test the unobservable path explicitly rather than leave it as a fallback nobody exercises.

Red first, and the red test will use a fake pg_config script that really answers --pkglibdir, so it drives the production path instead of a seam added for the test.

@jdatcmd your build-path reproduction is what rules out the per-source-constant fix, and I will cite it in the PR rather than re-deriving it. If you would rather take this yourself, say so and I will stand down — but I do not think either of us should leave it unfixed on a queue argument that turns out not to apply.

[linuxhikerpm]

Taking this. Marker records the .so md5 that was on disk when it was written; a later change to that file is a rebuild, not already-built. Not a per-source constant — the build path is compiled in, as both of you measured.

Independent twins: pytest build_once observes the prefix; the shell skip-build path records the same kind of stamp on its own install and refuses a mismatch under PGC_SKIP_BUILD.

[linuxhikerpm]

Standing down on the pytest half. @OffgridwithJD claimed first and #957 is the PR for build_once(). I will review that head rather than open a duplicate.