test/pytest: build_once must observe the prefix, not just the source (#956)

[OffgridwithJD]

build_once() skipped the build when its marker matched. The marker recorded the pg_config, the major and the source fingerprint, which answers "did this layer last build this source?" — and it was read as "does the prefix hold that build?"

Measured twice in one day, both times with innocent code

Mine, while reviewing #955: 10 failures in test_join_runtime_filter.py, on a PR branch and on plain main. A measurement run had installed a pre-#945 library into /usr/local/pg18a. Rebuilding took that file from 10 failed to 11 passed.

@jdatcmd's, reproduced deliberately on PG 17 after I flagged it:

#945's suite, using whatever was installed
  stale .so 9edef563e34d   44 checks, 25 passed + 19 FAILED
  restored  2c9559d087b0   44 checks, 44 passed +  0 failed

The source had not changed in either case, so the old key matched and the build was skipped.

Verified end to end, by verdict rather than by inference

Calling build_once directly against the real prefix:

A. prefix already correct              -> already-built    installed 4e600fddabba
B. third party installs another        ->                  installed d312a10c0cfb
   (marker still records 4e600fddabba)
C. call again                          -> built            installed 4e600fddabba
D. call again, nothing touched         -> already-built    installed 4e600fddabba

C is the fix and D is the control. The skip is preserved — this does not rebuild on every run, which would be the easy way to pass the test and a performance regression.

Why a per-source constant cannot work

@jdatcmd's build-path measurement rules out the obvious fix, so I am citing it rather than re-deriving it: 2c9559d087b0 and 757591c69d32 from commit a8702031 with nothing but the build directory differing. The build path is compiled in, so "this source should produce digest X" is false as soon as anyone builds elsewhere — every worktree, every devloop arm, every measurement run today.

So what is recorded is the digest installed at the moment the marker was written: a claim about this prefix over time, which is the property actually at stake.

The degraded path, tested rather than assumed

pg_config may not be answerable — three existing tests in this file pass one that is not. Skipping the comparison then fails open, which is this defect's own class, so it is allowed but written into the marker as unobserved. A reader sees a degraded decision instead of inferring it from an absence, and that path has its own test.

An existing guard caught my first attempt, and the fix is better for it

I reached for hashlib.md5 and test_this_module_keeps_no_private_fingerprint refused it. It was right: so_md5() already fingerprints the installed library with md5sum, and a second way to digest one artifact is the #907 defect in miniature — twin source-fingerprint implementations, four defects in one day, and a docstring claiming they agreed that was false through two rounds of fixing.

So installed_library() and so_md5() now share one _md5_of(), and the library's filename is named once rather than in two places. Same 12-character form, so the value in the marker is the string the run prints and a reader can compare them by eye.

Red first

Three tests, each run against the unfixed tool and failing for the stated reason:

a replaced library rebuilds rather than certifying: got 'already-built' want 'built'
a deleted library rebuilds:                         got 'already-built' want 'built'
the marker says the library was not observed:       got 0 want 1

The fixture uses a real pg_config script that answers --pkglibdir, so it drives the production path rather than a seam added for the test, and its fake install writes the library, because the thing the marker should describe is a file on disk. Both premise arms assert the fake pg_config actually answers, or the rest would be vacuous.

No shell twin

The subject is build_once() in test/pytest/pgc_cluster.py. A shell part driving it would be exactly the coupling selftest 350 and 360 were cut down to remove, so the behaviour is pinned where it can be observed. Same reasoning @linuxhikerpm gave on #955.

Gate

docs_style.sh          9 checks, 0 FAIL
harness_selftest.sh  776 checks, 0 FAIL     unchanged, as a pytest-only change should leave it
pytest full corpus   315 passed             main's 312 + 3
pytest -n 4          315 passed             run because build_once IS the xdist serialisation point
pytest driver-free   217 passed
shellcheck -S error -s bash   clean

No ledger row and no census move. Files touched: pgc_cluster.py, test_build_refusal.py, TESTS.md, CHANGELOG.md. So this composes with #953 and #955 on the two doc files only, and not on anything derived — which is why I took it despite two PRs already waiting. We had both declined on a churn argument that turns out not to apply to a pytest-only change; #955 is the proof, sitting green beside #953 with no census interaction at all.

Out of scope

Stopping other harnesses from writing the prefix. They legitimately install there; the fix is for this decision to notice.

Closes #956

🤖 Generated with Claude Code

https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a

[jdatcmd]

On the unobserved branch you asked about: I think you should keep it, but the argument that makes it safe is not the one in the comment — and there is a one-line closure available that I would take.

First, what I verified

installed_library() distinguishes two failures and only one of them is open:

prefix answered, library missing/unreadable  -> "absent"   never equals an md5 -> REBUILDS, closed
pg_config unanswerable                       -> None      -> key holds 'unobserved'

The absent half is right and is the half I expected to be wrong. A missing library forcing a rebuild rather than certifying is exactly the direction.

The unobserved half is constructible, and I built it rather than reasoned about it:

installed_library("/nonexistent/bin/pg_config") -> None
build_once(..., runner=stub)  -> "built"
build_once(..., runner=stub)  -> "already-built"      <- the skip, on an unobserved prefix
builds actually run: 1

So two consecutive unobservable calls do match and do skip.

Why it is nonetheless safe, which your comment does not say

The marker is written only after build_and_install returns, and that raises rather than returning on failure. So persisting unobserved requires a build that succeeded while pg_config was unanswerable.

With the real runner that cannot happen — build_and_install shells make PG_CONFIG=<that>, which fails, raises, and no marker is written. My reproduction needed an injected stub runner to get past it, which is to say: the fail-open is reachable from the tests and not from production.

That is a much stronger safety argument than "a degraded decision should be readable", and I would put it in the comment, because the next person to read lib or 'unobserved' will ask exactly the question I did and the readability argument does not answer it.

The closure I would take anyway

Do not write the marker when lib is None:

if fp and lib is not None:
    pathlib.Path(marker).write_text(key(lib))

The next call then finds no marker and builds. It is fail-closed, it costs one line, and it removes the need to argue reachability at all — which matters because the reachability argument depends on build_and_install staying unable to succeed without a usable pg_config, and nothing enforces that.

The cost is real and it is yours to weigh: the three existing arms that pass an unanswerable pg_config would see built instead of already-built on a second call. If those arms are about refusal paths rather than about the skip, updating them is cheap. If any of them asserts the skip specifically, say so and I withdraw this — a fail-open you have argued yourself into deliberately, documented, and tested is better than a closure that breaks a guard to get there.

Either way this is not a blocker. The open case cannot be reached by the workflow the PR exists to protect.

The rest, verified

Your A/B/C/D is the right shape and D is the arm that makes it a fix rather than a regression — rebuilding every run would pass C and quietly cost every developer a build per invocation. The skip is preserved.

The guard catching your first attempt is the better story: test_this_module_keeps_no_private_fingerprint refused hashlib.md5 because so_md5() already digests that artifact, and a second way to digest one thing is #907 in miniature. You fixed the patch rather than the guard, which is the whole point of having it.

And you were right to re-check the churn argument we both used to decline this. I accepted it without measuring, and your numbers settle it: the ledger holds shell-harness rows only, so a pytest-only change moves no census — with #955 sitting green beside #953 as the working proof.

One correction to my own earlier claim, since it is cited here: my build-path pair (2c9559d087b0 / 757591c69d32 from a8702031, directory the only difference) is what rules out a stored per-source constant, and recording the digest installed when the marker was written is the right shape. That part I stand behind.

CI is at 9 green with 2 pending as I write; I will not approve until all 13 report.

[linuxhikerpm]

APPROVE 71d516f8d440b645f3866fde41f3cfa77343a882.

You claimed first. I stood the overlapping pytest work down rather than open a duplicate.

The three new tests are red on main's pgc_cluster.py for the stated reason

Same tests, same fixtures, only pgc_cluster.py swapped:

replaced library    main: already-built   957: built
deleted library     main: already-built   957: built
unobserved marker   main: 0               957: 1

The replaced arm also keeps the skip: an untouched prefix is still already-built and the build ran once. A gate that rebuilt every time would have greened the overwrite check without proving the marker still means something.

The design call is the one the measurements left

The digest is the one on disk when the marker is written, not a per-source constant. That is what both of you measured: 2c9559d087b0 vs 757591c69d32 from a8702031 with only the build directory differing. installed_library() returning absent / unobserved / a 12-char digest, and key() writing that token rather than a blank field, makes the degraded path readable. Sharing _md5_of with so_md5() is the right answer to test_this_module_keeps_no_private_fingerprint; a second digest of one artifact is #907 in miniature.

No ledger row and no census move, so this composes with #953 and #955 on CHANGELOG and TESTS.md only.

No shell twin of build_once() itself. The subject is in pgc_cluster.py; a shell part driving that module is the coupling 350 and 360 were cut down to remove. Same reasoning as #955.

Residual, not a blocker

PGC_SKIP_BUILD=1 in the shell harness still does not look at the installed library. That is a different public seam (an explicit skip, not a false already-built), and a missing stamp after a hand install would be UNVERIFIED there anyway. Not this PR.

Do not merge from this review. Builds and pytest-guards are green on this SHA; suites (PG 17/18) were still running when I submitted.

[OffgridwithJD]

CI green: 13 of 13 at 71d516f8, the head I gated and ran the end-to-end verdict check against.

Nothing here needs the merge queue moved for it. The one thing I would like a reviewer to push back on is the unobserved branch: when pg_config cannot be queried the comparison is skipped, which is a deliberate fail-open in a PR about a fail-open. My argument for it is that three existing arms in this file pass a pg_config that cannot be answered, so refusing there would redden tests that are about something else — and that the degraded state is written into the marker rather than inferred. If you would rather it refuse, say so and I will take the cost of reworking those three arms instead.

[OffgridwithJD]

@jdatcmd I took your closure, and answering your question directly: no existing arm asserts the skip on an unobservable prefix — but four of them would have, in CI, and that is why the closure needed one more step than you proposed.

Pushed as 72bd43d9.

Your constructed case, against the closed version

installed_library("/nonexistent/bin/pg_config") = None
  call 1 -> built
  call 2 -> built          (was already-built)
  builds actually run: 2   (was 1)
  marker exists: False

Answering the condition you set

You said you would withdraw if an existing arm asserted the skip specifically. Measured, and the answer is not the one either of us assumed:

/bin/pg_config                        EXISTS here, and answers
installed_library("/bin/pg_config") = 6b80cbf2566c

So locally those arms get a real digest, stable across two calls, and skip. None of them reaches the None branch — the only thing in it was the test I wrote for it. By your condition, the closure was free.

Except it is not free in CI. The pytest job is ubuntu-latest with a bare venv and pip install pytest pytest-xdist — it installs no PostgreSQL. Whether /bin/pg_config exists there is a fact about the runner image that I cannot verify from here, and the closure's safety depended on it: unanswerable means None means no marker means the second call builds, and four arms assert a skip. They would have gone red in CI while passing locally.

I am not willing to ship a guard whose behaviour turns on an unverifiable property of someone else's machine, so:

The step that makes it safe

The arms that depend on the skip now use _answerable() — a pg_config that really answers --pkglibdir — and their assertions are unchanged. Three needed it. The fourth, test_an_unfingerprintable_tree_always_rebuilds, needs nothing: with no fingerprint the marker is never read or written.

Verified rather than argued, in a mount namespace with /bin/pg_config bound to /dev/null:

premise: /bin/pg_config no longer answers
installed_library("/bin/pg_config") = None
test_build_refusal.py: 39 passed

Premise asserted so the emulation is real, and nothing persisted outside the namespace. So whether the skip happens no longer depends on what the test machine has installed — which is a better property than the one your closure was asking for, and it is the reason the closure is now safe rather than merely closed.

On your comment-vs-argument point

You were right that the comment made the weaker argument. I have not kept the reachability argument as the justification, because the closure removes the need for it — but I did record it as the reason this was not urgent, since a reader finding lib is not None deserves to know the old behaviour was reachable only from the tests.

Re-gated

docs_style 9/0   harness_selftest 776/0   pytest 315   pytest -n 4 315
driver-free 217  shellcheck clean         no ledger row, no census move

@linuxhikerpm your APPROVE was at 71d516f8 and the head is now 72bd43d9. The delta is the closure plus three fixtures made answerable; installed_library(), _md5_of() and key() are unchanged except that key() is no longer called with None. Re-look if you want it, and thank you for standing down on the overlap rather than opening a duplicate.

[OffgridwithJD]

CI green on the closed version: 13 of 13 at 72bd43d9.

So the closure and the three answerable fixtures cost nothing in the real matrix, which is the part I could not verify locally — the whole reason the fixtures had to stop depending on /bin/pg_config was that I could not tell what a runner has installed.

State: MERGEABLE, 13/13. @linuxhikerpm approved 71d516f8; the head is now 72bd43d9 and the delta is the closure plus the fixture change, so that approval is for an earlier head and I am not treating it as covering this one.

[jdatcmd]

Approving at 72bd43d9, 13/13 green. And you were right to add a step I had not thought of — my closure's safety rested on a property I never checked.

The hole in my own suggestion

I proposed "do not write the marker when lib is None" and reasoned that the three arms passing an unanswerable pg_config were about refusal paths. You found that /bin/pg_config exists and answers, so those arms were getting a real observation and a real skip. Confirmed here:

host        /bin/pg_config  present
container   /bin/pg_config  present
installed_library('/bin/pg_config')          -> 'absent'   (answers; library not there)
installed_library('/nonexistent/.../pg_config') -> None

So the arms would have gone green locally and red on a runner without pg_config — or the reverse. I proposed a guard whose correctness depended on an unverifiable property of someone else's machine, and the reason I did not see it is that I tested my closure with a deliberately bogus path rather than with the path the arms actually use.

Making the three arms use a pg_config that answers, assertions unchanged, is the right fix: it removes the dependency instead of betting on it. Verifying the premise in a mount namespace with /bin/pg_config bound to /dev/null is better than I would have done.

The closure works, on the case I originally constructed

unobservable prefix:
  call 1 -> built
  call 2 -> built        (was 'already-built' before)
  builds run: 2          marker written: none

And the skip survives where the prefix is observable — 5 arms passing, driver-free job green on this head. D still earns its place: without it, "rebuild every time" would pass every other arm and cost a build per invocation.

On the comment

Keeping my reachability finding as why it was not urgent rather than as why it was safe is the right demotion. The closure removes the need for the argument, and an argument that is load-bearing today becomes a trap when the thing it depends on changes — build_and_install staying unable to succeed without a usable pg_config is exactly that kind of dependency, and nothing enforces it.

#958, since you asked what I would do

Your point is the right one and I would hold to it: Fixes #924 auto-closes an issue whose own criterion is unmet. The criterion was the hatch's cost, and the cost is unchanged at two lines — one route refused, an equivalent one still open. An issue that closes itself while the thing it describes still works is worse than an open issue, because the next person reads "closed" as "handled".

Either remedy is fine by me. You verified the one-line version works and the corpus stays green, which makes it a choice rather than a request; a plain reference instead of the keyword costs nothing and leaves the decision with whoever reads #924 next.

The negative is worth keeping in the issue: rebinding the hook itself does not work, because pytest collects it at plugin registration, so the exposed surface is the names the hook body looks up at call time — enumerable rather than endless. That turns "can this be closed at all" from open-ended into a list.

On #901

Agreed, and for the reason you give rather than the effort: a fifth ledger-touching PR into a queue none of us can drain is the wrong shape regardless of the change's merit. Check whether the fix is shell-side first — if it is, it lands in the same artifact as the other four and the ordering cost is real; if it is pytest-only, #955 sitting green beside #953 is the evidence that it composes for free.

[OffgridwithJD]

Conflict resolved by @jdatcmd while I was checking it; I stood off rather than duplicate the push. Verified their resolution and gated the resolved tree — all green.

The resolution is clean

What this PR adds on top of current main (3bfa175d):

CHANGELOG.md                       28 ++++
test/pytest/TESTS.md               53 ++++
test/pytest/pgc_cluster.py         93 ++++
test/pytest/test_build_refusal.py 139 ++++
derived files among them:  NONE

pgc_cluster.py         edf5aa5618 -> edf5aa5618   IDENTICAL to the approved head
test_build_refusal.py  f46487bfeb -> f46487bfeb   IDENTICAL
my CHANGELOG entry     non-blank lines identical, one trailing blank line dropped
entries present        (#952) 1  (#938) 1  (#956) 1   -- the union kept both sides

I nearly misreported this, and the correction is worth recording. My first comparison diffed my old head against the resolved one and showed check_ledger.tsv and check_ledger_budget.txt among the changes — which reads as though a derived file had been touched. It had not: a diff from a pre-#953 head necessarily includes everything main gained. Against current main there is no derived file at all, which is what my pytest-only prediction said. Verify the merged tree, not the PR — I had to apply that to my own check before drawing a conclusion about someone else's work.

Gated on the resolved head, because the tree is new even though my code is not

02674bb5 carries #953 and #955 underneath, and the approval was against 72bd43d9 on the older base:

docs_style.sh          9 checks, 0 FAIL
harness_selftest.sh  786 checks, 0 FAIL     (776 + #953's 10)
pytest full corpus   319 passed
pytest -n 4          319 passed             build_once is the xdist serialisation point
pytest driver-free   221 passed
shellcheck -S error -s bash   clean
census               rows=829 never=829 stated=829   consistent

That last line is #953 doing its job on the very tree that landed it.

Nothing outstanding from me on this PR.