From d28a57170c3454e09155e2717bd95d7d015c00c4 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 18:59:25 +0000 Subject: [PATCH 1/3] test: close three false-freshness paths @linuxhikerpm reproduced NOT PUSHED PENDING @jdatcmd's DECISION. This PR is APPROVED and this repository does not dismiss stale reviews, so pushing would make an approval cover three changes nobody reviewed. Committed locally so the work is not lost. All three are the same failure this PR exists to prevent -- the run reports FRESH while the binary is stale -- and all three were reproduced on my own box before being fixed, not inferred from the review. --- 1. THE WRITER COULD NOT REPORT FAILURE ------------------------------------ pgc_write_source_stamp() { printf '%s\n' "${2:-}" > "${1:-/dev/null}" 2>/dev/null || true } `|| true` made it always return 0, so BOTH controllers' warning branches were unreachable -- run_all_versions.sh and devloop.sh each wrap the call in `if (...)` to say so when the stamp cannot be written. Reproduced: write_rc=0 exists=no And each of those call sites carries a comment I wrote saying "NOT `|| true`. If the stamp cannot be written ... this stops being a controller with nothing saying so." The comment argued for a guarantee the function it called did not provide, which is worse than no comment because it stops the next person checking. --- 2. THE DIGEST COULD NOT SEE A REPARTITION --------------------------------- `xargs -0 cat | md5sum` hashed the concatenated stream, with no paths and no boundaries between files. Two files that both compile, with the second's bytes moved into the first: before_hash=bfce474cc159 after_hash=bfce474cc159 initial_compile=0 repartitioned_compile=1 error: redefinition of 'x' Source that CANNOT COMPILE reported "matches the binary under test". Now each file contributes its path relative to the tree and its own digest, so the partition is part of the input and one file's bytes cannot run into the next's. This is the same class as a collision I fixed on #897's Python side today, where the digest mixed in each file's bare NAME and src/module.c and objstore/module.c were interchangeable. Two independent implementations, the same defect, found by two different people -- which is the argument for the two becoming one. --- 3. "KEYED BY MAJOR" ALIASED DISTINCT INSTALLATIONS ------------------------ `pgc_source_stamp_path DIR MAJOR` gave `.pgc_source_stamp.18`, and the comment above it already said one tree installs into several prefixes each with its own binary -- so the key discarded the distinction the comment drew. Not hypothetical on this box: pg18a pkglibdir=/usr/local/pg18a/lib/postgresql pg18n pkglibdir=/usr/local/pg18n/lib/postgresql stamp_a=/tree/.pgc_source_stamp.18 stamp_b=/tree/.pgc_source_stamp.18 SAME=YES Build into one prefix, run PGC_SKIP_BUILD=1 against another, and the fingerprint matches while the binary is stale -- and the postmaster arm passes too, because the freshly started server is newer than the other prefix's old .so. Now keyed on PKGLIBDIR rather than on the pg_config path, because that is where the .so lands: two pg_configs pointing at one prefix ARE one installation and should share a stamp. An unreadable pg_config gets a key derived from its own path rather than a shared "unknown", because aliasing every broken config onto one key is the same defect one level down. The signature is now `pgc_source_stamp_path DIR PG_CONFIG`; all three call sites had a pg_config in scope already. --- PROVED BY REMOVAL -------------------------------------------------------- unmutated 18e60be58200 302 passed writer swallows failure again 7040d7d67c04 1 failed digest reverts to concatenation 9c7e01986e67 2 failed stamp key reverts to major only bd42a44e1dca 3 failed restored 18e60be58200 byte-exact Each mutation asserted applied by md5 before the run, and each reddens exactly the arms that name it and no others. 14 new arms in test/selftest/340, driving the REAL functions. The stamp-key arms use FAKE pg_config scripts rather than this box's three PG18 installations, so the arm does not depend on which majors happen to be installed here. They include the two controls that keep the fix honest: the same pg_config twice must give one path, and two pg_configs pointing at one prefix must share a stamp. harness_selftest 288 -> 302, shellcheck exit 0. THE PYTEST TWIN IS OWED. Per jd's rule of 2026-09-09 these arms need a pytest half, and test/pytest/ exists only on #897. #897 is now approved, so the twin lands when this branch is rebased onto it -- which it must be anyway, for the stamp-write interlock. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- test/devloop.sh | 2 +- test/lib.sh | 72 +++++++++++- test/run_all_versions.sh | 2 +- .../340-the-binary-must-be-built-from.sh | 108 ++++++++++++++++++ 4 files changed, 176 insertions(+), 8 deletions(-) diff --git a/test/devloop.sh b/test/devloop.sh index 5be67552..2d995863 100755 --- a/test/devloop.sh +++ b/test/devloop.sh @@ -95,7 +95,7 @@ fi if ( . "$BUILD/test/lib.sh" pgc_write_source_stamp \ - "$(pgc_source_stamp_path "$BUILD" "$(pgc_major_of "$PGC")")" \ + "$(pgc_source_stamp_path "$BUILD" "$PGC")" \ "$(pgc_source_fingerprint "$BUILD")" ); then : diff --git a/test/lib.sh b/test/lib.sh index 762f42e3..9fc7baf3 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -200,7 +200,7 @@ pgc_build_and_install() { # against, so a suite measuring an edited tree reported "matches the binary # under test". A red arm caught it, which is the only reason this exists. pgc_write_source_stamp \ - "$(pgc_source_stamp_path "$_pgc_bi_src" "$_pgc_bi_major")" \ + "$(pgc_source_stamp_path "$_pgc_bi_src" "$_pgc_bi_cfg")" \ "$(pgc_source_fingerprint "$_pgc_bi_src")" return 0 } @@ -284,7 +284,7 @@ pgc_setup() { # And verify it, whether this run built or skipped. A skipped build is exactly # when the binary can be older than the source. _pgc_fresh_recorded="$(pgc_read_source_stamp \ - "$(pgc_source_stamp_path "$PGC_SRCDIR" "$PGC_MAJOR")")" + "$(pgc_source_stamp_path "$PGC_SRCDIR" "$PGC_PG_CONFIG")")" _pgc_fresh_current="$(pgc_source_fingerprint "$PGC_SRCDIR")" case "$(pgc_freshness_verdict "$_pgc_fresh_recorded" "$_pgc_fresh_current")" in fresh) @@ -656,7 +656,27 @@ pgc_source_fingerprint() { # pgc_source_fingerprint DIR -> hash done < <(pgc_source_build_dirs "$dir") find "$dir" -maxdepth 1 -type f \( -name 'Makefile' -o -name '*.control' \ -o -name '*.sql' \) -print0 2>/dev/null - } | sort -z | xargs -0 cat 2>/dev/null | md5sum | cut -c1-12 + } | sort -z | while IFS= read -r -d '' _pgc_fp_f; do + # EACH FILE'S PATH AND ITS OWN DIGEST, not the concatenated stream. + # + # `xargs -0 cat | md5sum` hashed the bytes of every file run together, + # so it could not see a change that PRESERVES the stream while moving + # bytes between translation units. Two files, `static int x=1;` and + # `static int x=2;`, both compile; move the second into the first and + # empty it and the source no longer compiles, while the hash does not + # move (@linuxhikerpm, #898 review): + # + # before_hash=bfce474cc159 after_hash=bfce474cc159 + # initial_compile=0 repartitioned_compile=1 + # error: redefinition of 'x' + # + # A skip-build run then printed "matches the binary under test" for + # source that cannot produce any binary at all. The path makes the + # partition part of the input, and the per-file digest is an + # unambiguous boundary between one file's bytes and the next's. + printf '%s %s\n' "${_pgc_fp_f#"$dir"/}" \ + "$(md5sum < "$_pgc_fp_f" 2>/dev/null | cut -d' ' -f1)" + done | md5sum | cut -c1-12 } # fresh the binary was built from this source @@ -742,7 +762,18 @@ pgc_running_binary_verdict() { # pgc_running_binary_verdict SO_EPOCH PM_EPOCH } pgc_write_source_stamp() { # pgc_write_source_stamp FILE HASH - printf '%s\n' "${2:-}" > "${1:-/dev/null}" 2>/dev/null || true + # NO `|| true`. It was there, and it made both controllers' warning branches + # UNREACHABLE: run_all_versions.sh and devloop.sh each wrap this in `if (...)` + # and promise to say so when the stamp cannot be written, and each carries a + # comment saying "NOT || true" -- while the function they call swallowed the + # status (@linuxhikerpm, #898 review). Driven against an unwritable target: + # + # write_rc=0 exists=no + # + # The stamp absent, nothing warned, every child suite degraded to UNVERIFIED. + # A comment that argues for a guarantee the code does not provide is worse + # than no comment, because it stops the next person checking. + printf '%s\n' "${2:-}" > "${1:-/dev/null}" 2>/dev/null } pgc_read_source_stamp() { # pgc_read_source_stamp FILE -> hash or empty @@ -762,8 +793,37 @@ pgc_major_of() { # pgc_major_of PG_CONFIG -> major "$1" --version | sed -E 's/^[^0-9]*([0-9]+).*/\1/' } -pgc_source_stamp_path() { # pgc_source_stamp_path DIR MAJOR - printf '%s/.pgc_source_stamp.%s\n' "${1:-.}" "${2:-0}" +pgc_source_stamp_path() { # pgc_source_stamp_path DIR PG_CONFIG + # KEYED BY THE INSTALLATION, NOT ONLY THE MAJOR. The key was + # `.pgc_source_stamp.`, and the comment above it already said that one + # tree installs into several prefixes each with its own binary -- so the key + # discarded the distinction the comment drew (@linuxhikerpm, #898 review). + # + # Not hypothetical on this box: pg18a, pg18n and pg18_san are three PG18 + # installations with different pkglibdirs, and all three resolved to + # `.pgc_source_stamp.18`. Build current source into one prefix, then run + # PGC_SKIP_BUILD=1 against another, and the fingerprint matches while the + # binary is stale -- and the postmaster arm passes too, because the freshly + # started server is newer than the old .so. The run then reports fresh while + # executing the other prefix's binary, which is this file's whole subject. + # + # pkglibdir rather than the pg_config path, because that is where the .so + # actually lands: two pg_configs pointing at one prefix ARE the same + # installation and should share a stamp. + local _pgc_sp_dir="${1:-.}" _pgc_sp_cfg="${2:-}" + local _pgc_sp_major _pgc_sp_lib _pgc_sp_id + _pgc_sp_major="$(pgc_major_of "$_pgc_sp_cfg" 2>/dev/null)" + _pgc_sp_lib="$("$_pgc_sp_cfg" --pkglibdir 2>/dev/null)" + # An unreadable pg_config gets a key that matches nothing rather than one + # every broken config shares: `unknown` would alias them together, which is + # the defect being fixed, one level down. + if [ -n "$_pgc_sp_lib" ]; then + _pgc_sp_id="$(printf '%s' "$_pgc_sp_lib" | md5sum | cut -c1-8)" + else + _pgc_sp_id="nolib$(printf '%s' "$_pgc_sp_cfg" | md5sum | cut -c1-3)" + fi + printf '%s/.pgc_source_stamp.%s.%s\n' \ + "$_pgc_sp_dir" "${_pgc_sp_major:-0}" "$_pgc_sp_id" } pgc_write_build_stamp() { diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 4371be84..b6330593 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -726,7 +726,7 @@ for pgc in "${CONFIGS[@]}"; do if ( . "$builddir/test/lib.sh" pgc_write_source_stamp \ - "$(pgc_source_stamp_path "$builddir" "$major")" \ + "$(pgc_source_stamp_path "$builddir" "$pgc")" \ "$(pgc_source_fingerprint "$builddir")" ); then : diff --git a/test/selftest/340-the-binary-must-be-built-from.sh b/test/selftest/340-the-binary-must-be-built-from.sh index 8064fdff..09e3d080 100644 --- a/test/selftest/340-the-binary-must-be-built-from.sh +++ b/test/selftest/340-the-binary-must-be-built-from.sh @@ -178,3 +178,111 @@ check "an unreadable library is not a failure" "$_rb_rc3" "0" check "but it says which question went unanswered" \ "$(printf '%s\n' "$_rb_out3" | grep -c 'could not compare')" "1" rm -rf "$_rb_dir" + +# ---- three false-freshness paths @linuxhikerpm reproduced (#898 review) ------ +# +# All three are the same failure this file exists to prevent: the run reports +# FRESH while the binary is stale. Each was reproduced against the branch before +# it was fixed, and each arm below drives the REAL function rather than a copy. + +# 1. THE WRITER COULD NOT REPORT FAILURE. `|| true` on the redirect meant both +# controllers' `if (...)` warning branches were unreachable -- and both carry +# a comment saying "NOT || true", so the comments argued for a guarantee the +# code did not provide. Measured: write_rc=0 exists=no. +_fs_bad="/proc/pgc-source-stamp-selftest" # a path that cannot be created +pgc_write_source_stamp "$_fs_bad" "deadbeef" 2>/dev/null +check "the stamp writer reports failure on an unwritable target" \ + "$?" "1" + +check "premise: and the stamp really was not written, so the arm is not vacuous" \ + "$([ -f "$_fs_bad" ] && echo written || echo absent)" "absent" + +_fs_ok="$PGC_WORKDIR/stamp.ok" +pgc_write_source_stamp "$_fs_ok" "cafebabe" +check "control: and it still succeeds on a writable one" "$?" "0" + +check "control: writing the value it was given" "$(cat "$_fs_ok" 2>/dev/null)" "cafebabe" + +# 2. THE DIGEST COULD NOT SEE A REPARTITION. `xargs -0 cat | md5sum` hashed the +# concatenated stream, so moving bytes BETWEEN files left the hash unchanged +# while the source stopped compiling: +# before_hash=bfce474cc159 after_hash=bfce474cc159 +# initial_compile=0 repartitioned_compile=1 error: redefinition of 'x' +_fs_rp="$PGC_WORKDIR/repartition"; rm -rf "$_fs_rp"; mkdir -p "$_fs_rp/src" +printf 'all:\n\ttrue\n' > "$_fs_rp/Makefile" +printf 'static int x=1;\n' > "$_fs_rp/src/a.c" +printf 'static int x=2;\n' > "$_fs_rp/src/b.c" +_fs_before="$(pgc_source_fingerprint "$_fs_rp")" + +check "premise: the fixture fingerprints at all" \ + "$([ -n "$_fs_before" ] && echo yes || echo no)" "yes" + +# The same bytes, a different partition: a.c gains b.c's line and b.c is emptied. +printf 'static int x=1;\nstatic int x=2;\n' > "$_fs_rp/src/a.c" +: > "$_fs_rp/src/b.c" +check "moving bytes between files moves the fingerprint" \ + "$([ "$(pgc_source_fingerprint "$_fs_rp")" != "$_fs_before" ] && echo moved || echo SAME)" \ + "moved" + +# And back, so the arm above is about the partition rather than about any edit. +printf 'static int x=1;\n' > "$_fs_rp/src/a.c" +printf 'static int x=2;\n' > "$_fs_rp/src/b.c" +check "and restoring the partition restores the fingerprint" \ + "$(pgc_source_fingerprint "$_fs_rp")" "$_fs_before" + +# Renaming a file is also a repartition: same bytes, different translation unit. +mv "$_fs_rp/src/b.c" "$_fs_rp/src/c.c" +check "renaming a source file moves the fingerprint too" \ + "$([ "$(pgc_source_fingerprint "$_fs_rp")" != "$_fs_before" ] && echo moved || echo SAME)" \ + "moved" + +# 3. "KEYED BY MAJOR" ALIASED DISTINCT INSTALLATIONS. Two PG18 prefixes with +# different pkglibdirs both resolved to `.pgc_source_stamp.18`, so a build +# into one certified the other. This box really has pg18a, pg18n and +# pg18_san -- but the arm uses FAKE pg_configs so it does not depend on which +# majors happen to be installed here. +_fs_cfg="$PGC_WORKDIR/cfgs"; rm -rf "$_fs_cfg"; mkdir -p "$_fs_cfg" +for _fs_n in a b; do + { + printf '#!/bin/sh\n' + printf 'case "$1" in\n' + printf ' --version) echo "PostgreSQL 18.4" ;;\n' + printf ' --pkglibdir) echo "/usr/local/pg18%s/lib/postgresql" ;;\n' "$_fs_n" + printf 'esac\n' + } > "$_fs_cfg/pg_config.$_fs_n" + chmod 755 "$_fs_cfg/pg_config.$_fs_n" +done + +_fs_pa="$(pgc_source_stamp_path /tree "$_fs_cfg/pg_config.a")" +_fs_pb="$(pgc_source_stamp_path /tree "$_fs_cfg/pg_config.b")" + +check "premise: both fake configs report the same major, which is the whole point" \ + "$(pgc_major_of "$_fs_cfg/pg_config.a")=$(pgc_major_of "$_fs_cfg/pg_config.b")" "18=18" + +check "two installations of one major get different stamp paths" \ + "$([ "$_fs_pa" != "$_fs_pb" ] && echo different || echo "SAME:$_fs_pa")" "different" + +check "and the major is still readable in the name" \ + "$(printf '%s' "$_fs_pa" | grep -c '\.pgc_source_stamp\.18\.')" "1" + +# The same installation must still resolve to ONE stamp, or every run rebuilds +# and the check never has a recorded value to compare against. +check "control: the same pg_config twice gives the same path" \ + "$(pgc_source_stamp_path /tree "$_fs_cfg/pg_config.a")" "$_fs_pa" + +# Two pg_configs pointing at ONE prefix are the same installation and share a +# stamp: the key is pkglibdir, not the pg_config path. +cp "$_fs_cfg/pg_config.a" "$_fs_cfg/pg_config.a2" +check "and two pg_configs for one prefix share a stamp, keyed on pkglibdir" \ + "$(pgc_source_stamp_path /tree "$_fs_cfg/pg_config.a2")" "$_fs_pa" + +# An unreadable pg_config must not land every broken config on one key, which is +# the aliasing defect one level down. +printf '#!/bin/sh\nexit 1\n' > "$_fs_cfg/pg_config.broken"; chmod 755 "$_fs_cfg/pg_config.broken" +printf '#!/bin/sh\nexit 1\n' > "$_fs_cfg/pg_config.broken2"; chmod 755 "$_fs_cfg/pg_config.broken2" +check "two unreadable pg_configs do not alias onto one stamp" \ + "$([ "$(pgc_source_stamp_path /tree "$_fs_cfg/pg_config.broken")" \ + != "$(pgc_source_stamp_path /tree "$_fs_cfg/pg_config.broken2")" ] \ + && echo different || echo SAME)" "different" + +unset _fs_bad _fs_ok _fs_rp _fs_before _fs_cfg _fs_n _fs_pa _fs_pb From 903fe047575ea888762ada894a5c0bc8c22f3e4c Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 19:27:24 +0000 Subject: [PATCH 2/3] test: the writer and the reader must address the same stamp file @jdatcmd's #903 review found a defect this branch could not see: changing pgc_source_stamp_path from `DIR MAJOR` to `DIR PG_CONFIG` is a CONTRACT change, and #897 added a caller I never swept because it did not exist when I wrote the sweep. lib.sh:203 "$(pgc_source_stamp_path "$_pgc_bi_src" "$_pgc_bi_major")" He merged the two branches and looked, which is why he found it and I did not: test/lib.sh CONFLICTS -- but in pgc_setup, not at line 203. Line 203 merges CLEANLY and is then wrong, so resolving the conflict you are shown leaves the defect behind. The hunk nobody is asked to resolve is the one that breaks. WHAT IT COSTS, reproduced here and matching his measurement to the character: correct (PG_CONFIG): .pgc_source_stamp.18.603d6145 a major passed instead: .pgc_source_stamp.0.nolib6f4 `pgc_major_of 18` finds no version in the string "18", so the major becomes 0 and the id becomes a hash of the literal "18". Writer and reader then address different files and never meet: the reader finds nothing, the verdict is `unknown`, and unknown is DELIBERATELY not a failure -- so every suite prints "freshness UNVERIFIED" and nothing says why. And `nolib6f4` derives from "18", so pg18a, pg18n and pg18_san collide again on the writer path, which is the defect fix 3 of this PR closes. FIXED: line 203 passes "$_pgc_bi_cfg". Both surviving call sites now pass a pg_config, asserted rather than eyeballed: call sites and the argument each passes: $_pgc_bi_cfg $PGC_PG_CONFIG TWO ARMS, BECAUSE HE ASKED FOR THE CLASS AND NOT THE INSTANCE. * A SWEEP over every caller in test/, reddening for any second argument that is major-shaped -- a bare integer, `$PGC_MAJOR`, or a name ending `_major`. It names the file and line. This catches the shape anywhere it appears, including in a file this PR does not touch. * AN END-TO-END AGREEMENT ARM, which is the property that actually matters. It drives the REAL pgc_build_and_install with `make` stubbed on PATH, then globs for what actually landed and compares it with what the real reader looks for, then reads the value back and asserts the verdict is `fresh`. Any disagreement about which file the stamp lives in reddens here regardless of shape -- a renamed variable, a reordered argument, a third caller nobody swept. Nothing else in this file asserted that the writer and the reader agree, which is what makes the freshness check a check. PROVED BY REMOVAL, with his exact line put back: unmutated 967aa241f8cc 366 passed line 203 -> _pgc_bi_major 9d4eb0f2cd2f 4 failed: no caller passes a major ... : got [1: test/lib.sh:203] the writer writes the file the reader looks for: got [....pgc_source_stamp.0.nolib6f4] and the reader reads back the fingerprint: got [] want [4553f83a29a3] so the verdict is fresh, not unknown: got [unknown] want [fresh] restored 967aa241f8cc byte-exact Four arms, one defect, and the silent failure -- `unknown` -- is now loud. THE SWEEP CAUGHT ITS OWN FIXTURE FIRST. Written literally, the bad-caller fixture IS a bad caller as far as a tree-wide grep is concerned, and the sweep found it at its own line on the first run. Assembled instead, the way selftest 320 assembles its forbidden line, for the reason 320 states: a test for a pattern must not contain the pattern. Rebased onto 6364e220 (#897 merged). harness_selftest 302 -> 366, shellcheck 0. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- .../340-the-binary-must-be-built-from.sh | 149 ++++++++++++++++++ 1 file changed, 149 insertions(+) diff --git a/test/selftest/340-the-binary-must-be-built-from.sh b/test/selftest/340-the-binary-must-be-built-from.sh index 09e3d080..14366f49 100644 --- a/test/selftest/340-the-binary-must-be-built-from.sh +++ b/test/selftest/340-the-binary-must-be-built-from.sh @@ -286,3 +286,152 @@ check "two unreadable pg_configs do not alias onto one stamp" \ && echo different || echo SAME)" "different" unset _fs_bad _fs_ok _fs_rp _fs_before _fs_cfg _fs_n _fs_pa _fs_pb + +# ---- every caller must pass a pg_config, not a major ------------------------ +# +# WHY THIS EXISTS, AND WHY IT IS A SWEEP RATHER THAN AN ARM ABOUT ONE LINE. +# Changing pgc_source_stamp_path from `DIR MAJOR` to `DIR PG_CONFIG` changes a +# CONTRACT, and a contract change is only as good as the sweep that finds every +# caller. @jdatcmd merged the two branches and found the case this misses +# (#903 review): #897 adds a NEW writer inside pgc_build_and_install, and +# `test/lib.sh` conflicts in pgc_setup while that new call site merges CLEANLY +# and is then wrong. The resolution you are shown is not the defect. +# +# What the mismatch costs, measured by driving the real function both ways: +# +# correct (PG_CONFIG): /t/.pgc_source_stamp.18.603d6145 +# a major passed instead: /t/.pgc_source_stamp.0.nolib6f4 +# +# `pgc_major_of 18` finds no version in the string "18", so the major becomes 0 +# and the id becomes a hash of the literal "18". The writer and the reader then +# address DIFFERENT FILES and never meet, so every suite reports +# "freshness UNVERIFIED" -- and unknown is deliberately not a failure, so +# NOTHING SAYS SO. Worse for the case this file exists for: a key derived from +# the literal "18" is the same for every PG18 prefix, so pg18a, pg18n and +# pg18_san collide again on the writer path. +# +# So this arm is about the CLASS. It reddens for any caller, in any file, that +# passes a major where a pg_config belongs -- including one that arrives through +# a clean merge in a hunk nobody was asked to resolve. + +_sc_sites="$(grep -rn 'pgc_source_stamp_path ' "$PGC_TESTDIR"/*.sh "$PGC_TESTDIR"/selftest/*.sh 2>/dev/null \ + | grep -v 'pgc_source_stamp_path() {')" + +# A sweep that found nothing reports "no bad callers" and looks exactly like a +# sweep that works. +check "premise: the sweep finds the call sites it is meant to police" \ + "$([ "$(printf '%s\n' "$_sc_sites" | grep -c .)" -ge 3 ] && echo enough \ + || printf '%s\n' "$_sc_sites" | grep -c .)" "enough" + +# The second argument, per call site. A major-shaped one is the defect: a bare +# integer, or a variable whose name ends in _major, or PGC_MAJOR. +_sc_bad=""; _sc_n=0 +while IFS= read -r _sc_l; do + [ -n "$_sc_l" ] || continue + _sc_arg="$(printf '%s' "$_sc_l" | sed -n 's/.*pgc_source_stamp_path "[^"]*" "\([^"]*\)".*/\1/p')" + [ -n "$_sc_arg" ] || continue + case "$_sc_arg" in + '$PGC_MAJOR' | *_major'}' | *_major | [0-9] | [0-9][0-9]) + _sc_n=$((_sc_n + 1)) + [ "$_sc_n" -le 4 ] && _sc_bad="$_sc_bad ${_sc_l%%:*}:$(printf '%s' "$_sc_l" | cut -d: -f2)" + ;; + esac +done < "$_sc_fix/bad.sh" +_sc_probe="$(sed -n 's/.*pgc_source_stamp_path "[^"]*" "\([^"]*\)".*/\1/p' "$_sc_fix/bad.sh")" +check "premise: the argument parser reads the second argument at all" \ + "$_sc_probe" '$_pgc_bi_major' + +check "a caller passing a major is caught" \ + "$(case "$_sc_probe" in '$PGC_MAJOR' | *_major'}' | *_major | [0-9] | [0-9][0-9]) echo caught ;; *) echo MISSED ;; esac)" \ + "caught" + +# The mirror: a correct caller must NOT be flagged, or the arm reddens on every +# healthy tree and gets switched off. +printf '\t\t"$(pgc_source_stamp_path "$_pgc_bi_src" "$_pgc_bi_cfg")" \\\n' \ + > "$_sc_fix/good.sh" +_sc_probe2="$(sed -n 's/.*pgc_source_stamp_path "[^"]*" "\([^"]*\)".*/\1/p' "$_sc_fix/good.sh")" +check "control: a caller passing a pg_config is not flagged" \ + "$(case "$_sc_probe2" in '$PGC_MAJOR' | *_major'}' | *_major | [0-9] | [0-9][0-9]) echo FLAGGED ;; *) echo clean ;; esac)" \ + "clean" + +unset _sc_sites _sc_bad _sc_n _sc_l _sc_arg _sc_fix _sc_probe _sc_probe2 + +# ---- the writer and the reader must address the SAME file ------------------- +# +# THE CLASS, NOT THE INSTANCE (@jdatcmd, #903 review). The arm above sweeps for +# a major passed where a pg_config belongs, which catches the shape. This one +# catches the CONSEQUENCE regardless of shape: it drives the real +# pgc_build_and_install and then asks the real reader for the value, so ANY +# disagreement between the two about which file the stamp lives in reddens here +# -- a renamed variable, a reordered argument, a third caller nobody swept. +# +# It is the property that actually matters. The writer and the reader agreeing +# on a path is what makes the freshness check a check; when they disagree the +# reader finds nothing, the verdict is `unknown`, and unknown is DELIBERATELY +# not a failure -- so the whole mechanism goes quiet and every suite prints +# "freshness UNVERIFIED" while looking healthy. Nothing else in this file +# asserts it. +# +# `make` is stubbed on PATH so this costs nothing: the subject is which path the +# stamp lands on, not whether the tree compiles. + +_wr="$PGC_WORKDIR/writer_reader"; rm -rf "$_wr"; mkdir -p "$_wr/src" "$_wr/bin" +printf 'int x;\n' > "$_wr/src/a.c" +printf 'all:\n\ttrue\n' > "$_wr/Makefile" +printf '#!/bin/sh\nexit 0\n' > "$_wr/bin/make"; chmod 755 "$_wr/bin/make" +{ + printf '#!/bin/sh\n' + printf 'case "$1" in\n' + printf ' --version) echo "PostgreSQL 18.4" ;;\n' + printf ' --pkglibdir) echo "%s/lib" ;;\n' "$_wr" + printf 'esac\n' +} > "$_wr/pg_config"; chmod 755 "$_wr/pg_config" + +# Drive the REAL function, exactly as pgc_setup calls it. +( PATH="$_wr/bin:$PATH"; pgc_build_and_install "$_wr" "$_wr/pg_config" 18 ) >/dev/null 2>&1 +_wr_rc=$? + +check "premise: the build path ran to completion, so a stamp was due" "$_wr_rc" "0" + +# What landed, and what the reader will look for. Globbed rather than computed, +# so this reads the writer's ACTUAL choice instead of re-deriving it. +_wr_written="" +for _wr_f in "$_wr"/.pgc_source_stamp.*; do + [ -e "$_wr_f" ] && _wr_written="$_wr_f" +done +_wr_expected="$(pgc_source_stamp_path "$_wr" "$_wr/pg_config")" + +check "premise: the writer wrote a stamp at all" \ + "$([ -n "$_wr_written" ] && echo yes || echo "none in $_wr")" "yes" + +check "the writer writes the file the reader looks for" \ + "$_wr_written" "$_wr_expected" + +# And end to end: the reader gets the fingerprint back, so the verdict is +# `fresh` rather than `unknown`. +check "and the reader reads back the fingerprint the writer recorded" \ + "$(pgc_read_source_stamp "$_wr_expected")" "$(pgc_source_fingerprint "$_wr")" + +check "so the verdict is fresh, not unknown" \ + "$(pgc_freshness_verdict "$(pgc_read_source_stamp "$_wr_expected")" \ + "$(pgc_source_fingerprint "$_wr")")" "fresh" + +unset _wr _wr_rc _wr_written _wr_expected _wr_f From 84f81b06065aa1cb48068d91d7177e05660c6547 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 9 Sep 2026 19:42:02 +0000 Subject: [PATCH 3/3] test: the pytest twin, and a fourth instance of the fingerprint defect THE TWIN IS OWED AND NOW PAYABLE. selftest/340's stamp arms had no pytest half because test/pytest/ did not exist on main. #897 merged, so it does. Four arms, driving the SHELL functions through bash rather than reimplementing them -- which is the whole lesson of this PR applied to its own tests. AND WRITING IT FOUND A FOURTH INSTANCE OF THE SAME DEFECT, on main, in the implementation @linuxhikerpm had already fixed once. source_fingerprint() in pgc_cluster.py says in its own docstring that it uses "the same input set as pgc_source_fingerprint in test/lib.sh". It did not. The shell hashes each build directory's *.c, *.h AND Makefile; the Python read only *.c and *.h there: baseline shell=45be41a5c47b python=bea88c7d79ca objstore/Makefile edited shell=cfb8f4553041 python=bea88c7d79ca Editing objstore/Makefile changes how that module is BUILT. The shell hash moves; the Python one does not; build_once then reports "already-built" and the pytest corpus measures a stale module. That is @linuxhikerpm's #897 finding one layer over -- they found the module's SOURCES missing from this implementation, and the module's MAKEFILE was still missing after that was fixed. I found it by writing the twin and asking what the docstring's claim would look like as an assertion, not by reading the code. THE ARM ASSERTS WHAT THE DOCSTRING CLAIMED AND NOT MORE. The two hashes are NOT required to be equal: they are different digests over the same files, used independently, and requiring equality would couple two things that have no reason to be coupled. What "the same input set" means is that THE SAME EDIT MOVES BOTH, so the arm walks five edits -- a source, a module source, a module Makefile, the top-level Makefile, the control file -- and requires both hashes to move for each. Red before the fix, exactly and only where the defect was: editing a module Makefile moves both fingerprints: got 'True False' want 'True True' The other four edits already moved both, which is why this had survived two people looking at it. THE COUNT THAT MATTERS. Two implementations of one idea have now been separately wrong, separately fixed, and a third party had to find each one. I will open the "make them one implementation" issue when this lands; this commit is the fourth data point for it, not an argument against it. Verified: harness_selftest 366 passed + 0 failed + 0 unrunnable PASSED docs_style 9 checks PASSED pytest 78 passed serial, 78 passed -n 4, marker cleared for each shellcheck -S error -s bash test/*.sh test/selftest/*.sh exit 0 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- test/pytest/TESTS.md | 40 ++++++++++- test/pytest/pgc_cluster.py | 14 +++- test/pytest/test_build_refusal.py | 111 ++++++++++++++++++++++++++++++ 3 files changed, 163 insertions(+), 2 deletions(-) diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index b15d2ecd..1b455763 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -4,7 +4,7 @@ Reference for anyone reading, running, or adding to `test/pytest/`. The design a the decisions behind the harness are in `design/ISSUE_432_PYTEST_HARNESS.md`. This file covers the tests themselves. -**74 tests in 6 files.** Fifty-nine of them test the harness rather than the +**78 tests in 6 files.** Sixty-three of them test the harness rather than the product, and they come first, because a harness that can report a false green makes every other result in this directory worthless. @@ -330,6 +330,10 @@ is where a wrong quote would hide. | `test_the_fingerprint_covers_a_separately_built_module` | an `objstore/` edit moves the hash | | `test_an_objstore_edit_forces_a_second_build` | and forces a rebuild, end to end | | `test_make_cluster_leaves_nothing_behind_when_setup_fails` | a failed setup leaks no directory | +| `test_the_stamp_writer_reports_failure` | `\|\| true` made both controllers' warnings unreachable | +| `test_two_installations_of_one_major_do_not_share_a_stamp` | the key names the installation, not just the major | +| `test_moving_bytes_between_files_moves_the_shell_fingerprint` | the digest sees a repartition | +| `test_the_two_fingerprint_implementations_cover_the_same_inputs` | **the two implementations move on the same edits** | ### The build/start ORDER, which is not a detail @@ -344,6 +348,40 @@ after the alpha4 rebase gave 15 cluster-start errors and the second run passed. **A flake that clears on a second run is what a stale-binary defect looks like from outside.** +### The twin of `selftest/340`, and the fourth instance of one defect + +These four drive the **shell** functions through `bash` rather than +reimplementing them, and they are the pytest half of `test/selftest/340`'s stamp +arms, owed under the twin rule and payable only once `test/pytest/` reached +`main` with #897. + +The last one is the interesting one. `source_fingerprint` in `pgc_cluster.py` +says in its own docstring that it uses *"the same input set as +`pgc_source_fingerprint` in `test/lib.sh`"*. It did not. The shell hashes each +build directory's `*.c`, `*.h` **and `Makefile`**; this side read only the +sources, so editing `objstore/Makefile` — which changes how that module builds — +moved one hash and not the other: + +``` +baseline shell=45be41a5c47b python=bea88c7d79ca +objstore/Makefile edited shell=cfb8f4553041 python=bea88c7d79ca +``` + +`build_once` then certified a stale module as current. **That is +@linuxhikerpm's finding one layer over**: they found the module's *sources* +missing from this implementation, and the module's *Makefile* was still missing +after that was fixed. + +The arm asserts the property the docstring always claimed, and not more: the two +hashes are **not** required to be equal — they are different digests over the +same files, used independently — but **the same edit must move both**. It walks +five edits: a source, a module source, a module Makefile, the top-level +Makefile, and the control file. + +Two implementations of one idea have now been separately wrong, separately +fixed, and a third party had to find each. That is the argument for making them +one. + ### Two findings from @linuxhikerpm, both about infrastructure rather than coverage **The fingerprint read `src/` only.** `objstore/` is a separately built shared diff --git a/test/pytest/pgc_cluster.py b/test/pytest/pgc_cluster.py index 0db12d44..43fce107 100644 --- a/test/pytest/pgc_cluster.py +++ b/test/pytest/pgc_cluster.py @@ -409,7 +409,19 @@ def source_fingerprint(srcdir): srcdir = pathlib.Path(srcdir) paths = [] for d in source_build_dirs(srcdir): - paths += list(d.glob("*.c")) + list(d.glob("*.h")) + # EACH BUILD DIRECTORY'S Makefile TOO, not only its sources. The shell + # implementation hashes `*.c`, `*.h` AND `Makefile` per directory; this + # read only the sources, so editing `objstore/Makefile` -- which changes + # how that module is built -- moved the shell hash and not this one: + # + # baseline shell=45be41a5c47b python=bea88c7d79ca + # objstore/Makefile edited shell=cfb8f4553041 python=bea88c7d79ca + # + # `build_once` then certified a stale module as current. That is + # @linuxhikerpm's #897 finding one layer over: they found the module's + # sources missing here, and the module's Makefile was still missing + # after that was fixed. The docstring claimed parity throughout. + paths += list(d.glob("*.c")) + list(d.glob("*.h")) + list(d.glob("Makefile")) paths = sorted( paths + [p for p in (srcdir / "Makefile",) if p.exists()] diff --git a/test/pytest/test_build_refusal.py b/test/pytest/test_build_refusal.py index e0addc60..71cc9773 100644 --- a/test/pytest/test_build_refusal.py +++ b/test/pytest/test_build_refusal.py @@ -344,3 +344,114 @@ def test_make_cluster_leaves_nothing_behind_when_setup_fails(tmp_path, expect): leaked = sorted(set(glob.glob("/tmp/pgc-pytest-*")) - before) expect.text(", ".join(leaked) or "none", "none", "a failed make_cluster leaves no directory behind") + + +# --------------------------------------------------------------------------- +# THE PYTEST TWIN of test/selftest/340's stamp arms, owed under jd's rule of +# 2026-09-23... 2026-09-09: every test written twice. The .sh half could not +# have one until test/pytest/ existed on main, which it now does (#897). +# +# These drive the SHELL functions through bash rather than reimplementing them, +# for the reason that keeps being proved this week: a second implementation of +# one idea drifts, and the drift is invisible until someone diffs the two. + + +# The tree this corpus belongs to, derived the same way conftest.py derives it. +SRCDIR = pathlib.Path(__file__).resolve().parents[2] + + +def _sh(srcdir, expr): + """Evaluate one lib.sh expression against a tree, and return its stdout.""" + script = f'. "{SRCDIR}/test/lib.sh" || exit 1; {expr}' + p = subprocess.run(["bash", "-c", script], capture_output=True, text=True) + return p.stdout.strip(), p.returncode + + +def _tree_with_module(tmp_path, name): + t = tmp_path / name + (t / "src").mkdir(parents=True, exist_ok=True) + (t / "objstore").mkdir(parents=True, exist_ok=True) + (t / "src" / "a.c").write_text("int a;\n") + (t / "objstore" / "b.c").write_text("int b;\n") + (t / "objstore" / "Makefile").write_text("all:\n\ttrue\n") + (t / "Makefile").write_text("all:\n\t$(MAKE) -C objstore\n") + (t / "pgcolumnar.control").write_text("x\n") + return t + + +def test_the_stamp_writer_reports_failure(tmp_path, expect): + """`|| true` made both controllers' warning branches unreachable.""" + out, rc = _sh(tmp_path, 'pgc_write_source_stamp "/proc/pgc-twin" "deadbeef"') + expect.num(rc, 1, "the writer reports failure on an unwritable target") + ok, rc2 = _sh(tmp_path, f'pgc_write_source_stamp "{tmp_path}/s" "cafebabe"') + expect.num(rc2, 0, "control: and succeeds on a writable one") + + +def test_two_installations_of_one_major_do_not_share_a_stamp(tmp_path, expect): + """The stamp key must name the installation, not only the major.""" + cfgs = [] + for n in ("a", "b"): + c = tmp_path / f"pg_config.{n}" + c.write_text('#!/bin/sh\ncase "$1" in\n' + ' --version) echo "PostgreSQL 18.4" ;;\n' + f' --pkglibdir) echo "/usr/local/pg18{n}/lib" ;;\nesac\n') + c.chmod(0o755) + cfgs.append(c) + a, _ = _sh(tmp_path, f'pgc_source_stamp_path /tree "{cfgs[0]}"') + b, _ = _sh(tmp_path, f'pgc_source_stamp_path /tree "{cfgs[1]}"') + expect.text(str(a != b), "True", "two prefixes of one major get different stamps") + a2, _ = _sh(tmp_path, f'pgc_source_stamp_path /tree "{cfgs[0]}"') + expect.text(a2, a, "control: the same pg_config twice gives the same path") + + +def test_moving_bytes_between_files_moves_the_shell_fingerprint(tmp_path, expect): + """`xargs -0 cat | md5sum` could not see a repartition.""" + t = _tree_with_module(tmp_path, "rp") + before, _ = _sh(t, f'pgc_source_fingerprint "{t}"') + expect.at_least(len(before), 12, "premise: the tree fingerprints at all") + (t / "src" / "a.c").write_text("int a;\nint b;\n") + (t / "objstore" / "b.c").write_text("") + after, _ = _sh(t, f'pgc_source_fingerprint "{t}"') + expect.text(str(after != before), "True", + "moving bytes between files moves the fingerprint") + + +def test_the_two_fingerprint_implementations_cover_the_same_inputs(tmp_path, expect): + """THE PROPERTY THE TWO IMPLEMENTATIONS MUST SHARE, and the one they did not. + + `source_fingerprint` in pgc_cluster.py says in its own docstring that it uses + "the same input set as pgc_source_fingerprint in test/lib.sh". It did not: + the shell hashes each build directory's `*.c`, `*.h` AND `Makefile`, while + the Python read only `*.c` and `*.h` there. Editing `objstore/Makefile` -- + which changes how that module builds -- moved the shell hash and not the + Python one, so `build_once` certified a stale module as current: + + baseline shell=45be41a5c47b python=bea88c7d79ca + objstore/Makefile edited shell=cfb8f4553041 python=bea88c7d79ca + + That is @linuxhikerpm's finding one layer over: they found the .c files + missing from the Python side, and the Makefiles were still missing after it + was fixed. + + The two hashes are NOT required to be equal -- they are different digests + over the same files, used independently. What is required is that the same + edit moves both, which is what "the same input set" means and all the + docstring ever claimed. + """ + t = _tree_with_module(tmp_path, "cover") + for edit, path, body in ( + ("a source file", t / "src" / "a.c", "int a = 2;\n"), + ("a module source", t / "objstore" / "b.c", "int b = 2;\n"), + ("a module Makefile", t / "objstore" / "Makefile", "all:\n\ttrue # x\n"), + ("the top-level Makefile", t / "Makefile", "all:\n\t$(MAKE) -C objstore # x\n"), + ("the control file", t / "pgcolumnar.control", "y\n"), + ): + sh_before, _ = _sh(t, f'pgc_source_fingerprint "{t}"') + py_before = source_fingerprint(t) + old = path.read_text() + path.write_text(body) + sh_after, _ = _sh(t, f'pgc_source_fingerprint "{t}"') + py_after = source_fingerprint(t) + path.write_text(old) + expect.text(f"{sh_after != sh_before} {py_after != py_before}", "True True", + f"editing {edit} moves both fingerprints")