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

CHANGELOG.md

@@ -1268,6 +1268,34 @@ true until the next version shipped.
   now re-records. It names the two cases that remain: a declaration that no longer
   resolves, and the implicit base projection, which is not readable by name at all.
 
+- The pytest harness no longer reports results against a library another process
+  installed (#956).
+
+  `build_once()` skipped the build when its marker matched, and the marker recorded
+  the pg_config, the major and the source fingerprint. That answers "did this layer
+  last build this source", and it was read as "does the prefix hold that build". The
+  two differ whenever anything else writes the shared prefix: the bash harness, a
+  timing run, a manual install, another worktree. Measured twice in one day, a
+  measurement run installed an older library and the corpus then reported ten
+  failures in one file on one machine and nineteen on another, with the code under
+  test entirely innocent.
+
+  The installed library is now part of the marker, so a prefix someone else wrote is
+  rebuilt rather than certified. A library that is absent counts as changed. Where
+  the prefix cannot be observed at all, no marker is written, so the next call builds
+  rather than matching another unobservable run. The arms that exercise the marker
+  supply a `pg_config` that answers, so whether the skip happens no longer depends on
+  whether the machine running the tests has one.
+
+  The digest cannot be predicted from the source, because the build path is compiled
+  in: one commit built in two directories produces two different libraries. So what
+  is recorded is the digest installed at the moment the marker was written, which is
+  a statement about that prefix over time.
+
+  Blast radius worth knowing, since it is what made this hard to spot: a stale
+  library fails exactly the tests of the feature it lacks, so it presents as one
+  whole file failing while the rest of the suite passes. Scattered failures are
+  usually the code; a clean file boundary is usually the environment.
 - `pgc_ledger.py gate` no longer certifies a census that contradicts its own
   ledger (#952).
 

test/pytest/TESTS.md

@@ -815,6 +815,59 @@ Three of the four the scan found before this change were hooks:
 and `pytest_sessionfinish(exitstatus)`. Only `_sh` was a defect, so the budget was 1 and
 is now 0.
 
+### The marker answered a different question from the one it was read as (#956)
+
+`build_once()` skips the build when a marker matches. The marker used to record
+`pg_config`, the major and the **source** fingerprint, so it answered *"did this layer
+last build this source?"* — and it was read as *"does the prefix hold that build?"*
+Those are different claims, and the gap is not hypothetical: it cost two debugging
+sessions in one day. A measurement run installed a pre-#945 library into the shared
+prefix, and the corpus then reported **10 failures** here and **19** on @jdatcmd's box,
+with the code entirely innocent. The source had not changed, so the old key matched.
+
+The installed library is now part of the key. Anything may write that prefix — the shell
+harness, a perf run, a manual install, another worktree — and stopping them is not the
+fix; noticing is.
+
+| test | asserts |
+| --- | --- |
+| `test_build_once_rebuilds_when_another_process_replaced_the_library` | a third party overwriting the library rebuilds instead of certifying, and the build actually runs |
+| `test_build_once_rebuilds_when_the_library_was_deleted` | absent is not fresh |
+| `test_a_prefix_that_cannot_be_observed_never_certifies` | a prefix that cannot be seen is never certified: no marker, so the next call builds |
+
+**Why a per-source constant cannot work.** The library's digest is not a function of the
+source: the build path is compiled in. @jdatcmd measured `2c9559d087b0` and
+`757591c69d32` from one commit with nothing but the build directory differing. So
+"this source should produce digest X" is false as soon as anyone builds elsewhere, which
+is every worktree and every `devloop` arm. What is recorded instead is **the digest that
+was installed when the marker was written** — a claim about this prefix over time.
+
+**The degraded path fails CLOSED**, which it did not in the first version of this change.
+When the prefix cannot be observed, **no marker is written at all**, so the next call finds
+nothing to match and builds.
+
+The first version recorded `unobserved` in the marker instead, reasoning that a degraded
+decision should be readable. @jdatcmd constructed the hole rather than arguing about it:
+two consecutive unobservable calls match each other and skip, which is a fail-open inside
+a change about a fail-open. It was only reachable with an injected stub runner, because
+`build_and_install` shells `make PG_CONFIG=<that>` and raises when it fails — but that
+argument depends on `build_and_install` staying unable to succeed without a usable
+`pg_config`, and nothing enforces it. One condition removes the argument.
+
+**And the fixtures now answer, which is what makes the closure safe.** Three arms passed
+`/bin/pg_config`, whose answer depends on the machine: it resolves in this container and
+a CI runner may not have it. Once the prefix is observed, "does the skip happen" would
+depend on that, and a guard must not. `_answerable()` supplies a `pg_config` that really
+answers `--pkglibdir`, so those arms assert exactly what they asserted before, everywhere.
+Verified by running the module in a mount namespace with `/bin/pg_config` bound to
+`/dev/null`: premise asserted (`installed_library` returns `None` there), **39 passed**.
+
+**The digest is computed once.** `so_md5()` already fingerprinted the installed library
+with `md5sum`, so `installed_library()` shares that path rather than adding a second way
+to digest one artifact. `test_this_module_keeps_no_private_fingerprint` caught the first
+attempt, which reached for `hashlib` — the guard was right, and the fix is better for it.
+
+
 ## 6. test_docs_cover_the_corpus.py: this document, checked
 
 **THE SWEEP GOES BOTH WAYS NOW (#908).** `undocumented()` computes tests on disk

test/pytest/pgc_cluster.py

@@ -226,7 +226,7 @@ def dsn(self, dbname="postgres"):
 
     @property
     def so_path(self):
-        return os.path.join(self.libdir, "pgcolumnar.so")
+        return os.path.join(self.libdir, _SO_NAME)
 
     def so_md5(self):
         """Fingerprint the library under test.
@@ -235,8 +235,7 @@ def so_md5(self):
         against a previously installed library. The Python harness keeps it for the
         same reason.
         """
-        out = _run(["md5sum", self.so_path])
-        return out.split()[0][:12]
+        return _md5_of(self.so_path)
 
     # -- lifecycle, the only place a binary is invoked ---------------------
     def initdb(self):
@@ -435,6 +434,57 @@ def source_manifest(srcdir):
     return _fp.manifest(srcdir)
 
 
+# The library's filename, named ONCE. `so_path` and `installed_library` both need
+# it, and this module's own comments argue against twin implementations of "is the
+# thing under test the thing in this tree" -- that pair produced four defects in one
+# day (#907).
+_SO_NAME = "pgcolumnar.so"
+
+
+def _md5_of(path):
+    """Digest a FILE with md5sum, the way `so_md5` has always done it.
+
+    Deliberately not `hashlib`: `test_this_module_keeps_no_private_fingerprint`
+    forbids a private digest in this module, because the twin source-fingerprint
+    implementations produced four defects in one day and the docstring claiming they
+    agreed was false through two rounds of fixing (#907). That argument is about a
+    SECOND WAY TO COMPUTE ONE THING, which is what a separate artifact digest would
+    be, so `so_md5` and `installed_library` share this.
+
+    Truncated to 12 like `so_md5`, so the value written into the build marker is the
+    same string the run prints, and a reader can compare them by eye.
+    """
+    return _run(["md5sum", str(path)]).split()[0][:12]
+
+
+def installed_library(pg_config):
+    """What the prefix holds RIGHT NOW: an md5, "absent", or None if unreadable.
+
+    This is a claim about THIS PREFIX OVER TIME, which is the property actually at
+    stake, and deliberately not a claim about the source. The source cannot predict
+    the artifact: the build path is compiled in, so one commit built in two
+    directories produces two different libraries -- @jdatcmd measured 2c9559d087b0
+    and 757591c69d32 from a8702031 with nothing but the directory differing. A
+    stored per-source constant would therefore fail open on every legitimate
+    rebuild-elsewhere, and this project builds from a fresh directory routinely.
+
+    `None` is NOT "unchanged". The caller writes it into the marker as `unobserved`,
+    so a degraded decision is readable rather than inferred from an absence.
+    """
+    try:
+        libdir = _pg_config(pg_config, "--pkglibdir")
+    except Exception:
+        return None
+    try:
+        return _md5_of(pathlib.Path(libdir) / _SO_NAME)
+    except Exception:
+        # The prefix answered and the library could not be digested -- missing, or
+        # unreadable. That is an observation, not an absence of one, and it must
+        # rebuild rather than certify. Broad on purpose: every way of failing to
+        # read the artifact means the same thing here, and the direction is closed.
+        return "absent"
+
+
 def build_once(srcdir, pg_config, major, lock_path=None, runner=None):
     """build_and_install, but at most once across xdist workers.
 
@@ -457,18 +507,47 @@ def build_once(srcdir, pg_config, major, lock_path=None, runner=None):
     # make the guard cheap. A tree we cannot fingerprint gets a key that never
     # matches, so it always rebuilds.
     fp = source_fingerprint(srcdir)
-    want = f"{pg_config}\n{major}\n{fp}\n" if fp else None
+
+    def key(lib):
+        # THE INSTALLED LIBRARY IS PART OF THE KEY (#956). The source fingerprint
+        # answers "did this layer last build this source". It was read as "does the
+        # prefix hold that build", and those are different claims. The gap cost two
+        # debugging sessions in one day: a perf run installed a pre-#945 library
+        # into the shared prefix, and the corpus then reported ten failures here and
+        # nineteen on @jdatcmd's box with the code entirely innocent. The source had
+        # not changed, so the old key matched and the build was skipped.
+        #
+        # Anything may write this prefix -- the shell harness, a measurement run, a
+        # manual install, another worktree -- and stopping them is not the fix. The
+        # fix is for this decision to notice.
+        return f"{pg_config}\n{major}\n{fp}\n{lib or 'unobserved'}\n"
+
     with open(lock_path, "w") as lf:
         fcntl.flock(lf, fcntl.LOCK_EX)
         try:
             try:
-                if want is not None and pathlib.Path(marker).read_text() == want:
+                if fp and pathlib.Path(marker).read_text() == key(
+                        installed_library(pg_config)):
                     return "already-built"
             except OSError:
                 pass
             build_and_install(srcdir, pg_config, major, runner=runner)
-            if want is not None:
-                pathlib.Path(marker).write_text(want)
+            lib = installed_library(pg_config)
+            # AFTER the install, because the install is what writes the library: a
+            # fingerprint taken before it would record the previous one and certify
+            # exactly the state this guard exists to refuse.
+            #
+            # AND NO MARKER AT ALL WHEN THE PREFIX COULD NOT BE OBSERVED (#956
+            # review, @jdatcmd). Writing `unobserved` made two consecutive
+            # unobservable calls match each other and skip -- a fail-open inside a
+            # change about a fail-open. It was only reachable with an injected stub
+            # runner, because `build_and_install` shells `make PG_CONFIG=<that>` and
+            # raises when it fails, but that argument depends on build_and_install
+            # staying unable to succeed without a usable pg_config and nothing
+            # enforces it. Writing nothing costs one condition and removes the
+            # argument: the next call finds no marker and builds.
+            if fp and lib is not None:
+                pathlib.Path(marker).write_text(key(lib))
             return "built"
         finally:
             fcntl.flock(lf, fcntl.LOCK_UN)