test: refuse to measure a binary that was not built from this source - #898
Open
OffgridwithJD wants to merge 3 commits into
Open
test: refuse to measure a binary that was not built from this source#898OffgridwithJD wants to merge 3 commits into
OffgridwithJD wants to merge 3 commits into
Conversation
…r runs it
jd's design: one controller arm per batch of tests, so a stale .so can never be
measured and nothing has to rebuild per test.
TWO QUESTIONS THE HARNESS COULD NOT ANSWER.
selftest 110 compares the INSTALLED .so against the one built in this tree, so a
missed install and a foreign overwrite were already caught. Nothing derived anything
from the SOURCE TEXT, so both copies could agree with each other while both were
stale against edited source. PGC_SKIP_BUILD opens that hole widest, because not
rebuilding is its whole purpose.
And a cp is not enough. shared_preload_libraries maps the library at postmaster
start, so make install over a running instance changes the file and nothing else:
every backend keeps executing the code it already mapped. A binary can match the
source exactly while the server runs something older.
THE SHAPE. Whoever builds records a fingerprint of the build inputs -- src/*.c,
src/*.h, the Makefile, the control file, the shipped SQL. Every suite in the batch
recomputes it and compares, which is one build per batch and one hash per suite. Then
once the cluster is up, the suite compares the binary's mtime against
pg_postmaster_start_time().
-- source: 28b66bd0ac0c matches the binary under test
-- server: started after the binary was installed
A stale source fingerprint and a server predating the binary are both FATAL, because
every check that followed would be about code that is not running. A missing stamp is
UNVERIFIED and said plainly rather than failed: a person who ran make install by hand
has no stamp, and refusing would break a documented workflow.
MEASURED, three arms:
normal build, install, run source matches, server fresh, PASSED
source edited + PGC_SKIP_BUILD=1 FATAL: not built from this source, exit 1
binary newer than the postmaster FATAL: server already running, exit 1
The two verdict functions are pure and take their inputs as arguments, for the same
reason pgc_build_needs_clean does, so selftest 340 exercises fresh, stale, unknown,
predates and the non-numeric cases without a build. It also requires that the
fingerprint MOVES when a build input moves, STAYS when nothing does, and ignores a
file that is not a build input -- a fingerprint that never changes reports fresh
forever, which is this file's own failure mode one level down.
WHAT WROTE THE STAMP IN THE WRONG PLACE, AND WHAT CAUGHT IT. The first revision wrote
it in the skip-build branch, so every run recorded the source it was about to compare
against and a suite measuring an edited tree reported "matches the binary under test".
The arm that requires `stale` is what caught it. The stamp is now written only where
the install succeeded, and the comment there says why.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
…lt from jd's ask: a controller arm that verifies the build is fresh against the batch of tests being run, so we never measure a stale .so but also do not rebuild for every test. run_all_versions.sh is that controller. It builds and installs once per major and then runs every suite with PGC_SKIP_BUILD=1, so the suites have no way of their own to tell whether the binary they measure came from this tree. It now records the fingerprint of the build inputs after a successful install, and lib.sh checks it in every suite whether that suite built or skipped. In a subshell sourcing lib.sh rather than recomputing the hash inline: two implementations of one fingerprint drift, and the suites compare against exactly what this writes. Not `|| true`. If the stamp cannot be written, every suite in the batch reports "freshness UNVERIFIED" and the controller silently stops being a controller -- the batch degrades to the state this exists to prevent, with nothing saying so. A failure now prints what it means for the run below it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
pgc_setup writes .pgc_source_stamp.<major> beside the source it fingerprints, so running any suite from a checkout left an untracked file in `git status`. This project gates on a clean tree, so a harness artifact that dirties one is a recurring false alarm rather than a cosmetic issue. Placed next to .pgc_built_for_major, which is the same kind of file written for the same reason. Verified the new rule is the one that matches -- the file was NOT ignored before, and `git check-ignore -v` now names .gitignore:10 -- and that the existing rule still matches its own file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
Collaborator
Author
|
🤖 Generated with Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The matrix builds once per major and then runs every suite with
PGC_SKIP_BUILD=1. Nothing checked that the binary those suites measured came from the tree under test. This adds that check, at the controller, so a stale.socannot be measured and nothing has to rebuild per test.Two failure modes, both of which have bitten this project before, and both now FATAL rather than advisory.
1. The binary was not built from this source
The controller records a fingerprint of the build inputs after a successful install. Every suite compares it, whether that suite built or skipped.
Proved by removal, with controls on both sides:
The red arm reports:
It names both fingerprints, because "stale" without the two values leaves the reader unable to tell a real drift from a broken fingerprint.
2. The server predates the binary
A
make installdoes not reload anything.shared_preload_librariesmaps the library at postmaster start, so a reinstall under a running server leaves the backends executing the old code while the file on disk is new. The check compares the.somtime againstpg_postmaster_start_time():and on the bad verdict says what to do rather than only what is wrong:
unknownis not a failure, deliberatelySomeone who ran
make installby hand has no stamp. Refusing would break a documented workflow, so that case printsThe point is that it says which question was not answered, rather than printing nothing and letting a reader assume the check passed. A silent third state is how a verdict becomes lossy.
The verdict functions are pure, and tested as such
pgc_freshness_verdictandpgc_running_binary_verdicttake strings and returnfresh/stale/unknownandfresh/predates/unknown. They touch no filesystem, sotest/selftest/340-the-binary-must-be-built-from.shtests them directly — 16 arms including both empty inputs, a non-numeric epoch, equal timestamps on the exact boundary, and fingerprint sensitivity to each input class.harness_selftestgoes from 261 checks to 277.Two defects this found in itself
The stamp was written in the wrong branch first. My initial version wrote it in the skip-build path, which made the check tautological: it recomputed the fingerprint of the source it had just read and reported "matches" on edited source. My own red arm caught it. The stamp is now written only after a successful install; the verify runs always.
The controller swallowed its own failure. The stamp write was
... || true. If it failed, every suite in the batch would reportfreshness UNVERIFIEDand the controller arm would silently stop being a controller arm — the exact state this exists to prevent, with nothing saying so. It now prints what a failure means for the run below it.Verification
Full matrix on both majors at
0332527:A full matrix is the right bar here and there was no shortcut available: this changes
test/lib.sh, which every suite reads.The check was observed working inside that matrix, not only in isolation. Sampling the live PG19 per-suite logs three times during the run:
The five that report nothing are accounted for rather than assumed:
audit,bench_guards,concurrencyanddocs_stylenever callpgc_setup, anddecode_interruptsis a static source-analysis suite whose only occurrence ofpgc_setupis the comment "No cluster needed; pgc_setup is skipped deliberately." — my firstgrep -ccounted that comment as a call..gitignoregains.pgc_source_stamp.*beside.pgc_built_for_major, which is the same kind of file for the same reason. Verified the new rule is the one matching — the file was not ignored before, andgit check-ignore -vnow names.gitignore:10— rather than assuming my rule was what caught it.