From 71d516f8d440b645f3866fde41f3cfa77343a882 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Fri, 11 Sep 2026 12:50:57 +0000 Subject: [PATCH 1/2] test/pytest: build_once must observe the prefix, not just the source (#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". It was read as "does the prefix hold that build". Those are different claims 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 a pre-#945 library into /usr/local/pg18a and the corpus then reported 10 failures in test_join_runtime_filter.py, on a PR branch and on plain main, with the code entirely innocent. @jdatcmd reproduced the same shape on PG 17 deliberately: 44 checks, 25 passed and 19 FAILED against the stale library, 44 passed after restoring it. The source had not changed in either case, so the old key matched. The installed library is now part of the key, so a prefix someone else wrote is rebuilt rather than certified. Verified end to end against the real harness by calling build_once directly: prefix correct -> already-built third party installs another -> built, and the correct library is restored nothing touched -> already-built So the skip is preserved; this does not rebuild on every run. A library that is absent counts as changed rather than as fresh. Where the prefix genuinely cannot be read the comparison is skipped -- the only option that leaves three existing arms meaning what they say, since they pass a pg_config that cannot be queried -- and the marker records `unobserved`, so a degraded decision is readable instead of inferred from an absence. That path has its own test rather than being a fallback nothing exercises. 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 commit a8702031 with nothing but the build directory differing. "This source should produce digest X" is therefore false as soon as anyone builds elsewhere, which is every worktree and every devloop arm. 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 at stake. ONE DIGEST, NOT TWO. `so_md5()` already fingerprinted the installed library with md5sum, so `installed_library()` shares that path and `so_md5` delegates to it. The first attempt reached for hashlib and `test_this_module_keeps_no_private_fingerprint` refused it -- correctly, because the twin source-fingerprint implementations produced four defects in one day (#907), and a second way to digest one artifact is that defect in miniature. The library's filename is also named once now rather than in two places. Red first: three tests, each run against the unfixed tool and failing for the stated reason -- `got 'already-built' want 'built'`. 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. No shell twin: the subject is `build_once()` in test/pytest/pgc_cluster.py, so a shell part driving it would be the coupling selftest 350 and 360 were cut down to remove. Gated on this tree: docs_style 9/0, harness_selftest 776/0 (unchanged, as a pytest-only change should leave it), pytest 315, pytest -n 4 315 -- run because build_once is the xdist serialisation point -- driver-free 217, shellcheck -S error -s bash clean. No ledger row and no census move, so this composes with the two open PRs on CHANGELOG and TESTS.md only. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- CHANGELOG.md | 28 ++++++++ test/pytest/TESTS.md | 39 +++++++++++ test/pytest/pgc_cluster.py | 82 +++++++++++++++++++++-- test/pytest/test_build_refusal.py | 108 ++++++++++++++++++++++++++++++ 4 files changed, 250 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 327e0219..c9129058 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1259,6 +1259,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 genuinely cannot be read the comparison is skipped, which is the only + option that leaves the existing arms meaning what they say, and the marker records + `unobserved` so the degraded decision is readable rather than inferred. + + 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. + ## [1.0-alpha3] - 2026-09-02 ### Added diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index d555a60e..17ccd0ae 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -815,6 +815,45 @@ 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_is_recorded_as_unobserved` | the degraded path is exercised, and says so in the marker | + +**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 is tested rather than assumed.** `pg_config` may not be answerable; +three 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 can see the decision was degraded instead of inferring it from an +absence. + +**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 diff --git a/test/pytest/pgc_cluster.py b/test/pytest/pgc_cluster.py index c2c2ba85..18228de3 100644 --- a/test/pytest/pgc_cluster.py +++ b/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,36 @@ 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) + if fp: + # 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. + pathlib.Path(marker).write_text(key(installed_library(pg_config))) return "built" finally: fcntl.flock(lf, fcntl.LOCK_UN) diff --git a/test/pytest/test_build_refusal.py b/test/pytest/test_build_refusal.py index febe8605..2b0c0bc4 100644 --- a/test/pytest/test_build_refusal.py +++ b/test/pytest/test_build_refusal.py @@ -216,6 +216,114 @@ def counting(argv): expect.num(len(calls), 2, "an edited source builds again") +def _pg_config_answering(tmp_path, libdir): + """A real executable that answers --pkglibdir, so the test drives the + production path rather than a seam added for the test.""" + pgc = tmp_path / "pg_config" + pgc.write_text("#!/bin/sh\n" + "case \"$1\" in\n" + " --pkglibdir) echo %s ;;\n" + " *) echo unsupported >&2; exit 1 ;;\n" + "esac\n" % libdir) + pgc.chmod(0o755) + return pgc + + +def test_build_once_rebuilds_when_another_process_replaced_the_library(tmp_path, expect): + """#956. The marker answers "did this layer build this source", and it is read + as "does the prefix hold that build". Those are different claims. + + Measured twice 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 never changed, so + the source fingerprint matched and the build was skipped. + """ + tree = _tree_with_source(tmp_path, "replaced", "int a = 1;\n") + libdir = tmp_path / "libdir" + libdir.mkdir() + so = libdir / "pgcolumnar.so" + pgc = _pg_config_answering(tmp_path, libdir) + lock = str(tmp_path / "lock956") + + calls = [] + + def installing(argv): + # An install WRITES the library, which is what makes this fixture faithful: + # the thing the marker should be describing is a file on disk. + calls.append(argv) + so.write_bytes(b"build-%d" % len(calls)) + return _Proc(0) + + # PREMISE: the fake pg_config really answers, or the arm below is vacuous. + probe = subprocess.run([str(pgc), "--pkglibdir"], capture_output=True, text=True) + expect.num(probe.returncode, 0, "premise: the fake pg_config answers --pkglibdir") + expect.text(probe.stdout.strip(), str(libdir), "and it names the library directory") + + expect.text(build_once(tree, str(pgc), "18", lock_path=lock, runner=installing), + "built", "the first caller builds") + expect.text(build_once(tree, str(pgc), "18", lock_path=lock, runner=installing), + "already-built", "an untouched prefix still skips, as before") + expect.num(len(calls), 1, "so the build ran once") + + # A THIRD PARTY overwrites the installed library. Source unchanged, prefix + # unchanged, major unchanged -- only the artifact differs. + so.write_bytes(b"someone-elses-build") + expect.text(build_once(tree, str(pgc), "18", lock_path=lock, runner=installing), + "built", "a replaced library rebuilds rather than certifying") + expect.num(len(calls), 2, "and the build actually ran, rather than the verdict alone changing") + + +def test_build_once_rebuilds_when_the_library_was_deleted(tmp_path, expect): + """Absent is not fresh. A prefix someone cleaned must not read as built.""" + tree = _tree_with_source(tmp_path, "deleted", "int a = 1;\n") + libdir = tmp_path / "libdir2" + libdir.mkdir() + so = libdir / "pgcolumnar.so" + pgc = _pg_config_answering(tmp_path, libdir) + lock = str(tmp_path / "lock956b") + calls = [] + + def installing(argv): + calls.append(argv) + so.write_bytes(b"build") + return _Proc(0) + + build_once(tree, str(pgc), "18", lock_path=lock, runner=installing) + expect.num(len(calls), 1, "the first caller builds") + so.unlink() + expect.text(build_once(tree, str(pgc), "18", lock_path=lock, runner=installing), + "built", "a deleted library rebuilds") + expect.num(len(calls), 2, "and the build ran") + + +def test_a_prefix_that_cannot_be_observed_is_recorded_as_unobserved(tmp_path, expect): + """The degraded path, tested rather than left as a fallback nobody exercises. + + `pg_config` may not be answerable -- three tests in this file pass one that is + not. Skipping the comparison then is the only option that does not break them, + and it fails OPEN, which is this issue's own defect class. So it is allowed but + written into the marker, making the degraded state readable instead of inferred. + """ + tree = _tree_with_source(tmp_path, "unobservable", "int a = 1;\n") + lock = str(tmp_path / "lock956c") + calls = [] + + def counting(argv): + calls.append(argv) + return _Proc(0) + + expect.text(build_once(tree, "/nonexistent/pg_config", "18", + lock_path=lock, runner=counting), + "built", "an unanswerable pg_config still builds") + expect.text(build_once(tree, "/nonexistent/pg_config", "18", + lock_path=lock, runner=counting), + "already-built", "and still skips, so the existing arms keep their meaning") + expect.num(len(calls), 1, "the build ran once") + marker = pathlib.Path(lock + ".done").read_text() + expect.num(marker.count("unobserved"), 1, + "and the marker says the library was not observed, rather than implying it was") + + def test_the_fingerprint_reads_content_not_mtime(tmp_path, expect): """A checkout or a branch switch rewrites mtimes without changing what compiles, and `git stash` does the reverse. Content is the honest input.""" From 72bd43d96a1c8fdc1025b79e34f45753f99d601c Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Fri, 11 Sep 2026 13:09:40 +0000 Subject: [PATCH 2/2] test/pytest: the unobservable prefix fails closed too (#956 review) @jdatcmd constructed the hole rather than arguing about it: writing `unobserved` into the marker made two consecutive unobservable calls match each other and skip, which is a fail-open inside a change about a fail-open. before call 1 -> built call 2 -> already-built builds run: 1 after call 1 -> built call 2 -> built builds run: 2, no marker Their reachability argument was correct and is why this was not urgent: the marker is written only after `build_and_install` returns, and that shells `make PG_CONFIG=` and raises when it fails, so persisting `unobserved` needed an injected stub runner. But it depends on `build_and_install` staying unable to succeed without a usable pg_config, and nothing enforces that. One condition removes the argument. AND THE STEP THAT MAKES THE CLOSURE SAFE, which the review could not have known and I only found by asking what the arms actually depend on. Four arms passed `/bin/pg_config`. It resolves in this container, and `installed_library` returns a real digest for it -- so they skipped. It is not guaranteed on a CI runner, and the pytest job installs no PostgreSQL. With the closure and an unanswerable pg_config, every arm asserting a skip would have built twice and gone red in CI while passing locally. So the three arms that depend on the skip now use `_answerable()`, a pg_config that really answers `--pkglibdir`, and their assertions are unchanged. The fourth, `test_an_unfingerprintable_tree_always_rebuilds`, needs no change: with no fingerprint the marker is never read or written. Verified in a mount namespace with `/bin/pg_config` bound to `/dev/null`, premise asserted -- `installed_library` returns None there -- and the module is 39 passed with no usable pg_config at all. So whether the skip happens no longer depends on what the machine running the tests has installed. Re-gated: docs_style 9/0, harness_selftest 776/0, pytest 315, pytest -n 4 315, driver-free 217, shellcheck clean. Still no ledger row and no census move. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- CHANGELOG.md | 7 ++-- test/pytest/TESTS.md | 26 +++++++++++---- test/pytest/pgc_cluster.py | 21 +++++++++--- test/pytest/test_build_refusal.py | 55 ++++++++++++++++++++----------- 4 files changed, 75 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c9129058..c967f840 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1273,9 +1273,10 @@ true until the next version shipped. 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 genuinely cannot be read the comparison is skipped, which is the only - option that leaves the existing arms meaning what they say, and the marker records - `unobserved` so the degraded decision is readable rather than inferred. + 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 diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 17ccd0ae..04deeb7f 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -833,7 +833,7 @@ fix; noticing is. | --- | --- | | `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_is_recorded_as_unobserved` | the degraded path is exercised, and says so in the marker | +| `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 @@ -842,11 +842,25 @@ source: the build path is compiled in. @jdatcmd measured `2c9559d087b0` and 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 is tested rather than assumed.** `pg_config` may not be answerable; -three 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 can see the decision was degraded instead of inferring it from an -absence. +**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=` 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 diff --git a/test/pytest/pgc_cluster.py b/test/pytest/pgc_cluster.py index 18228de3..edf5aa56 100644 --- a/test/pytest/pgc_cluster.py +++ b/test/pytest/pgc_cluster.py @@ -532,11 +532,22 @@ def key(lib): except OSError: pass build_and_install(srcdir, pg_config, major, runner=runner) - if fp: - # 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. - pathlib.Path(marker).write_text(key(installed_library(pg_config))) + 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=` 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) diff --git a/test/pytest/test_build_refusal.py b/test/pytest/test_build_refusal.py index 2b0c0bc4..f46487bf 100644 --- a/test/pytest/test_build_refusal.py +++ b/test/pytest/test_build_refusal.py @@ -147,12 +147,26 @@ def test_a_missing_lib_sh_is_a_refusal_not_a_pass(tmp_path, expect): "an unsourceable lib.sh refuses") +def _answerable(tmp_path, name="cfg"): + """A pg_config that ANSWERS, plus the libdir it names. + + Three arms below used to pass `/bin/pg_config`, whose answer depends on the + machine: it resolves here and may not on a CI runner. Once `build_once` observes + the prefix, "does the skip happen" would depend on that, and a guard must not. + The fixture supplies the answer so the arms mean the same thing everywhere. + """ + libdir = tmp_path / (name + "_lib") + libdir.mkdir(exist_ok=True) + return _pg_config_answering(tmp_path, libdir), libdir + + def test_build_once_builds_once_and_then_skips(tmp_path, expect): """The xdist workers share one prefix, so the install is serialised rather than skipped.""" # A fingerprintable tree: the marker is keyed on source content, so a tree # with nothing to compile deliberately never certifies and always rebuilds. tree = _tree_with_source(tmp_path, "ok2", "int a = 1;\n") + pgc, libdir = _answerable(tmp_path) lock = str(tmp_path / "lock") calls = [] @@ -160,8 +174,8 @@ def counting(argv): calls.append(argv) return _Proc(0) - first = build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) - second = build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) + first = build_once(tree, str(pgc), "18", lock_path=lock, runner=counting) + second = build_once(tree, str(pgc), "18", lock_path=lock, runner=counting) expect.text(first, "built", "the first caller builds") expect.text(second, "already-built", "the second finds the marker") expect.num(len(calls), 1, "and the build ran exactly once") @@ -200,6 +214,7 @@ def test_editing_the_source_rebuilds(tmp_path, expect): optimisation meant to make the guard cheap. """ tree = _tree_with_source(tmp_path, "edited", "int a = 1;\n") + pgc, libdir = _answerable(tmp_path) lock = str(tmp_path / "lock3") calls = [] @@ -207,12 +222,12 @@ def counting(argv): calls.append(argv) return _Proc(0) - build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) - build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) + build_once(tree, str(pgc), "18", lock_path=lock, runner=counting) + build_once(tree, str(pgc), "18", lock_path=lock, runner=counting) expect.num(len(calls), 1, "unchanged source builds once") (tree / "src" / "columnar.c").write_text("int a = 2;\n") - build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) + build_once(tree, str(pgc), "18", lock_path=lock, runner=counting) expect.num(len(calls), 2, "an edited source builds again") @@ -296,13 +311,13 @@ def installing(argv): expect.num(len(calls), 2, "and the build ran") -def test_a_prefix_that_cannot_be_observed_is_recorded_as_unobserved(tmp_path, expect): - """The degraded path, tested rather than left as a fallback nobody exercises. +def test_a_prefix_that_cannot_be_observed_never_certifies(tmp_path, expect): + """The degraded path FAILS CLOSED (#956 review, @jdatcmd). - `pg_config` may not be answerable -- three tests in this file pass one that is - not. Skipping the comparison then is the only option that does not break them, - and it fails OPEN, which is this issue's own defect class. So it is allowed but - written into the marker, making the degraded state readable instead of inferred. + Writing `unobserved` into the marker made two consecutive unobservable calls + match and skip -- a fail-open inside a change about a fail-open. @jdatcmd + constructed it rather than arguing it. Writing no marker at all costs one + condition and removes the need to reason about reachability. """ tree = _tree_with_source(tmp_path, "unobservable", "int a = 1;\n") lock = str(tmp_path / "lock956c") @@ -314,14 +329,13 @@ def counting(argv): expect.text(build_once(tree, "/nonexistent/pg_config", "18", lock_path=lock, runner=counting), - "built", "an unanswerable pg_config still builds") + "built", "an unobservable prefix builds") expect.text(build_once(tree, "/nonexistent/pg_config", "18", lock_path=lock, runner=counting), - "already-built", "and still skips, so the existing arms keep their meaning") - expect.num(len(calls), 1, "the build ran once") - marker = pathlib.Path(lock + ".done").read_text() - expect.num(marker.count("unobserved"), 1, - "and the marker says the library was not observed, rather than implying it was") + "built", "and builds AGAIN rather than certifying a prefix it cannot see") + expect.num(len(calls), 2, "so the build really ran both times") + expect.num(1 if pathlib.Path(lock + ".done").exists() else 0, 0, + "and no marker was written, so there is nothing to match against") def test_the_fingerprint_reads_content_not_mtime(tmp_path, expect): @@ -456,6 +470,7 @@ def test_the_fingerprint_covers_a_separately_built_module(tmp_path, expect): def test_an_objstore_edit_forces_a_second_build(tmp_path, expect): """The consequence, end to end, which is what the finding was about.""" tree = _tree_with_objstore(tmp_path, "fp2") + pgc, _libdir = _answerable(tmp_path, "objstore") lock = str(tmp_path / "lock_obj") calls = [] @@ -463,12 +478,12 @@ def counting(argv): calls.append(argv) return _Proc(0) - build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) - build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) + build_once(tree, str(pgc), "18", lock_path=lock, runner=counting) + build_once(tree, str(pgc), "18", lock_path=lock, runner=counting) expect.num(len(calls), 1, "premise: an unchanged tree builds once") (tree / "objstore" / "module.c").write_text("int b = 2;\n") - build_once(tree, "/bin/pg_config", "18", lock_path=lock, runner=counting) + build_once(tree, str(pgc), "18", lock_path=lock, runner=counting) expect.num(len(calls), 2, "an edited objstore module builds again")