diff --git a/CHANGELOG.md b/CHANGELOG.md index 6dc1113a..da78edaa 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index bd6a8d0a..b14e6d85 100644 --- a/test/pytest/TESTS.md +++ b/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=` 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 diff --git a/test/pytest/pgc_cluster.py b/test/pytest/pgc_cluster.py index c2c2ba85..edf5aa56 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,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=` 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 febe8605..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,15 +222,122 @@ 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") +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_never_certifies(tmp_path, expect): + """The degraded path FAILS CLOSED (#956 review, @jdatcmd). + + 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") + 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 unobservable prefix builds") + expect.text(build_once(tree, "/nonexistent/pg_config", "18", + lock_path=lock, runner=counting), + "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): """A checkout or a branch switch rewrites mtimes without changing what compiles, and `git stash` does the reverse. Content is the honest input.""" @@ -348,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 = [] @@ -355,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")