From 518e261cc0237fdcc732206db62acf71a111ac25 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 11 Sep 2026 20:09:03 -0600 Subject: [PATCH 1/2] test: a skipped arm records under the name it would have used (#994) Four check_skip calls stood in for 19 named arms under a name none of those arms has. When the condition failed, those 19 produced no record at all: a reader could not tell WHICH arms did not run, and the ledger could not tell a skipped arm from a deleted one, because a skipped arm's row has no matching record exactly as a removed check's would. The convention was already in the tree, 30 lines below one of the offenders: skip under each arm's own name, in a loop. 340-the-binary-must-be-built-from.sh 3 arms native_parquet_flba.sh 6 native_parquet_pushdown.sh 3 sorted_pathkeys.sh 7 Each site's skip-loop names verified equal, as a set, to the names its sibling branch emits. THE ISSUE COUNTED 17. IT IS 19. Two arms in sorted_pathkeys.sh go through `ansp`, which calls check_text with its first argument, and a sweep looking for `check` did not see them -- the same hand-written helper list that cost me a wrong answer reviewing #992 an hour earlier. ONE NAME WAS ALREADY UNSTABLE. `premise: C and $ALTCOLL really disagree on this data` interpolates the variable whose emptiness CAUSES the skip: box with a collation premise: C and en_US.utf8 really disagree ... the skip branch premise: C and really disagree ... so skipping under it would record a key no real run emits, and it is a different key on every box. The name is now stable and the collation moves into the display, which is not the key. AND A GUARD, BECAUSE THE FIX DUPLICATES THE ARM NAMES. A rename in the sibling branch desynchronises the loop silently, and the skip then records under a name nothing emits -- the failure this change removes, reintroduced by an edit nobody thinks is risky. Writing this I put a name from another open PR's rename into the loop; the set comparison caught it before it shipped, and #982's renames are still landing. Part 470 sweeps every `for VAR in ... check_skip "$VAR"` loop in the corpus -- derived, so a fifth site is covered the day it is written -- and asserts the loop's names equal the sibling branch's arms. The population is a premise, because a sweep that matched nothing reports the same zero mismatches as a corpus in agreement. Removal proof: rename an arm and leave the loop stale -- exactly the drift made by accident above. 1 FAIL in the whole suite, and it names the file: every skip loop names exactly the arms its sibling branch would emit got [sorted_pathkeys.sh] want [] This is the precondition for arming #983's orphan guard: until a skipped arm records under its own name, absence cannot mean removal. Verified: harness_selftest 862/862 on PG16, shellcheck clean at CI's own flags, docs_style 9/9. Ledger 905 -> 908, census DERIVED, ceiling 250 unchanged, nothing orphaned. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw --- CHANGELOG.md | 31 ++++++++ test/check_ledger.tsv | 3 + test/check_ledger_budget.txt | 2 +- test/native_parquet_flba.sh | 14 +++- test/native_parquet_pushdown.sh | 9 ++- .../340-the-binary-must-be-built-from.sh | 13 ++- ...-skipped-arm-records-under-its-own-name.sh | 79 +++++++++++++++++++ test/selftest/parts.manifest | 1 + test/sorted_pathkeys.sh | 19 ++++- 9 files changed, 165 insertions(+), 6 deletions(-) create mode 100644 test/selftest/470-a-skipped-arm-records-under-its-own-name.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 14c81059..596d9bcb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -483,6 +483,37 @@ true until the next version shipped. ### Fixed +- A skipped arm records under the name it would have used, so a skipped arm and + a deleted one are no longer indistinguishable (#994). + + Four `check_skip` calls stood in for 19 named arms under a name none of those + arms has. When the condition failed, those 19 produced no record at all: a + reader could not tell which arms did not run, and the ledger could not tell a + skipped arm from a deleted one, because a skipped arm's row has no matching + record exactly as a removed check's would. + + The convention was already in the tree, 30 lines below one of the offenders: + skip under each arm's own name, in a loop. + + The issue counted 17. It is 19. Two arms in `sorted_pathkeys.sh` go through + `ansp`, which records under its first argument, and a sweep that looked for + `check` did not see them. + + One arm's name interpolated the very variable whose emptiness causes its own + skip, so the skip would have recorded a key no real run emits. That name is now + stable and the collation it names moves into the display, which is not the key. + + A new selftest part asserts every skip loop names exactly the arms its sibling + branch would emit. The loop duplicates those names, so a rename desynchronises + them silently and the skip records under a name nothing emits, which is the + failure this change exists to remove. That is not hypothetical: writing this, + a name from another open PR's rename went into the loop, and the comparison is + what caught it. + + This is the precondition for arming the orphan guard in #983. Until a skipped + arm records under its own name, absence cannot mean removal. + + - A conftest can no longer switch a vacuity rule off by rebinding a name the layer reads (#924). diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index 29b068c0..d62af8de 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -859,6 +859,9 @@ harness_selftest 460-the-controller-must-record-the-binary premise: the mutation harness_selftest 460-the-controller-must-record-the-binary premise: this part was given an executable pg_config to read the prefix from never - harness_selftest 460-the-controller-must-record-the-binary so a child suite under PGC_SKIP_BUILD reaches verified, not source-only (#961) never - harness_selftest 460-the-controller-must-record-the-binary the controller's stamp carries BOTH fields (#961) never - +harness_selftest 470-a-skipped-arm-records-under-its-own-name every skip loop names exactly the arms its sibling branch would emit (#994) never - +harness_selftest 470-a-skipped-arm-records-under-its-own-name premise: at least four loops had arms to be compared against never - +harness_selftest 470-a-skipped-arm-records-under-its-own-name premise: the sweep found skip loops to compare never - native_join_runtime_filter native_join_runtime_filter 3-table answer equals filter-off never - native_join_runtime_filter native_join_runtime_filter 3-table join order matches filter-off never - native_join_runtime_filter native_join_runtime_filter 3-table plan has coordinator never - diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index 7c63eb23..1d44692e 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -34,4 +34,4 @@ suites_not_covered 250 # Without that it is a hand-maintained count that drifts, which is the failure # this repository has spent a day proving. It is not a ceiling; it is a # measurement that must be true. -checks_never_observed_red 905 +checks_never_observed_red 908 diff --git a/test/native_parquet_flba.sh b/test/native_parquet_flba.sh index 3e5719d3..740f2952 100755 --- a/test/native_parquet_flba.sh +++ b/test/native_parquet_flba.sh @@ -208,7 +208,19 @@ if phys != {"i32": "INT32", "i64": "INT64"}: sys.exit("unexpected physical types: %s" % phys) PYINT if [ $? -ne 0 ]; then - check_skip "the integer-backed decimal case" "SKIP this pyarrow does not store decimals as integers as expected" "this pyarrow does not store decimals as integers" + # ONE SKIP PER ARM, UNDER THE ARM'S OWN NAME (#994): a single skip under a + # name none of the six arms has leaves all six with no record, so a skipped + # arm and a deleted one are indistinguishable to a reader and to the ledger. + for _fl_n in "INT32-backed DECIMAL reads" \ + "INT64-backed DECIMAL reads" \ + "INT32-backed DECIMAL keeps its null" \ + "parquet_schema advises numeric for an INT32 DECIMAL" \ + "parquet_schema advises numeric for an INT64 DECIMAL" \ + "an INT64 DECIMAL still binds to bigint as the unscaled integer"; do + check_skip "$_fl_n" \ + "SKIP $_fl_n (this pyarrow does not store decimals as integers as expected)" \ + "this pyarrow does not store decimals as integers" + done else check "INT32-backed DECIMAL reads" \ "$(q "SELECT string_agg(d::text, ',' ORDER BY d) FROM pgcolumnar.read_parquet('$W/dec_i32.parquet') AS t(d numeric);")" \ diff --git a/test/native_parquet_pushdown.sh b/test/native_parquet_pushdown.sh index 7ab4ab15..ce019c82 100755 --- a/test/native_parquet_pushdown.sh +++ b/test/native_parquet_pushdown.sh @@ -211,7 +211,14 @@ if f.metadata.num_row_groups != 4: sys.exit("expected 4 row groups") PYDEC if [ $? -ne 0 ]; then - check_skip "the integer-DECIMAL pushdown case" "SKIP could not build the integer-DECIMAL pushdown file" "could not build the fixture file" + # ONE SKIP PER ARM, UNDER THE ARM'S OWN NAME (#994). + for _pd_n in "INT64-backed DECIMAL: predicate skips 3 of 4 groups" \ + "INT64-backed DECIMAL: skipping did not drop rows" \ + "INT64-backed DECIMAL: unfiltered scan skips nothing"; do + check_skip "$_pd_n" \ + "SKIP $_pd_n (could not build the integer-DECIMAL pushdown file)" \ + "could not build the fixture file" + done else psql_run "CREATE FOREIGN TABLE ftdec (d numeric) SERVER pq OPTIONS (path '$PGC_WORKDIR/dec_push.parquet');" 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 49df9815..6bc2f9a9 100644 --- a/test/selftest/340-the-binary-must-be-built-from.sh +++ b/test/selftest/340-the-binary-must-be-built-from.sh @@ -674,7 +674,18 @@ _fp_as() { # _fp_as EXPR -> stdout, or `harness-unreadable` } if [ -z "$_fp_user" ]; then - check_skip "the unreadable-source refusal" "SKIP no non-root user to read as; root ignores chmod 000" "no non-root user to read as" + # ONE SKIP PER ARM, UNDER THE ARM'S OWN NAME (#994). A single skip named + # "the unreadable-source refusal" -- a name no arm below has -- leaves the three + # names with no record at all, so a reader cannot tell WHICH arms did not run and + # the ledger cannot tell a skipped arm from a deleted one. That is the same shape + # the loop 30 lines below already avoids. + for _fp_n in "premise: the unprivileged reader can source the staged harness" \ + "premise: the tree fingerprints to something when it is readable" \ + "premise: the unprivileged read agrees while everything is readable"; do + check_skip "$_fp_n" \ + "SKIP $_fp_n (no non-root user to read as; root ignores chmod 000)" \ + "no non-root user to read as" + done else # Premise nought: the reader can source the copy at all. Stated separately # from the fingerprint premise so a reachability failure and a fingerprint diff --git a/test/selftest/470-a-skipped-arm-records-under-its-own-name.sh b/test/selftest/470-a-skipped-arm-records-under-its-own-name.sh new file mode 100644 index 00000000..1ec9b3f8 --- /dev/null +++ b/test/selftest/470-a-skipped-arm-records-under-its-own-name.sh @@ -0,0 +1,79 @@ +# ---- a skipped arm must record under the name it would have used ------------ +# +# #994. Four sites skipped a whole block under ONE name that none of the arms +# inside it had. When the condition failed, those arms produced no record at all: +# a reader could not tell WHICH did not run, and the ledger could not tell a +# skipped arm from a deleted one, because a skipped arm's row has no matching +# record -- exactly as a removed check's would. +# +# The convention that fixes it was already in the tree: skip under each arm's own +# name, in a loop. This part asserts the loop stays in agreement with the arms. +# +# WHY A GUARD AND NOT JUST THE FIX. The loop DUPLICATES the arm names, so a rename +# in the sibling branch silently desynchronises them and the skip records under a +# name nothing emits -- which is the exact failure the fix exists to remove, +# reintroduced by an edit nobody thought was risky. +# +# That is not hypothetical. Writing #994 I put a name in the loop that a DIFFERENT +# open PR renames, and the set comparison below is what caught it before it +# shipped. #982's renames are still landing, so this drifts again the moment one +# of them touches these files. +# +# DERIVED, NOT LISTED. The sweep finds every `for VAR in ... check_skip "$VAR"` +# loop in the corpus, so a fifth site added later is covered on the day it is +# written rather than when somebody remembers this file. + +_sk_sweep() { # _sk_sweep -> "file:var" per skip loop, derived + grep -rlE 'check_skip "\$[a-z_]+"' "$TESTDIR"/*.sh "$TESTDIR"/selftest/*.sh 2>/dev/null | + while read -r _f; do + grep -oE 'check_skip "\$[a-z_]+"' "$_f" | sed 's/.*"\$\(.*\)"/\1/' | sort -u | + while read -r _v; do + grep -qE "for $_v in" "$_f" && printf '%s:%s\n' "$_f" "$_v" + done + done +} + +_sk_loop_names() { # _sk_loop_names FILE VAR -> the names the loop skips under + awk -v v="$2" '$0 ~ ("for " v " in") {i=1} i {print} i && /; do$/ {exit}' "$1" | + grep -oE '"[^"]*"' | tr -d '"' | grep -v '^$' | sort -u +} + +_sk_arm_names() { # _sk_arm_names FILE VAR -> the names the sibling branch emits + awk -v v="$2" '$0 ~ ("for " v " in") {f=1} + f && /^[[:space:]]*else/{i=1;f=0;next} + i && /^[[:space:]]*fi[[:space:]]*$/{exit} i' "$1" | + grep -oE '^[[:space:]]*(check|check_num|check_text|check_ratio|ansp|ansq?|diff_query|diff_query_ordered)[[:space:]]+"[^"]*"' | + sed 's/.*"\(.*\)"/\1/' | sort -u +} + +_sk_sites="$(_sk_sweep)" +_sk_n=$(printf '%s\n' "$_sk_sites" | grep -c . || true) + +# THE POPULATION IS THE PREMISE. A sweep that matched nothing reports the same +# zero mismatches as a corpus in perfect agreement, and only one of those is news. +check "premise: the sweep found skip loops to compare" \ + "$([ "${_sk_n:-0}" -ge 4 ] && echo yes || echo "only $_sk_n")" "yes" + +_sk_bad="" +_sk_pairs=0 +for _sk_site in $_sk_sites; do + _sk_f="${_sk_site%:*}"; _sk_v="${_sk_site##*:}" + _sk_l="$(_sk_loop_names "$_sk_f" "$_sk_v")" + _sk_a="$(_sk_arm_names "$_sk_f" "$_sk_v")" + # A loop guarding a block with no named arms is fine -- the skip IS the record + # and nothing else claimed to run. Only a loop with arms to agree with is compared. + [ -z "$_sk_a" ] && continue + _sk_pairs=$((_sk_pairs + 1)) + if [ "$_sk_l" != "$_sk_a" ]; then + _sk_bad="$_sk_bad $(basename "$_sk_f")" + fi +done + +check "premise: at least four loops had arms to be compared against" \ + "$([ "$_sk_pairs" -ge 4 ] && echo yes || echo "only $_sk_pairs")" "yes" + +check "every skip loop names exactly the arms its sibling branch would emit (#994)" \ + "$(printf '%s' "$_sk_bad" | sed 's/^ //')" "" + +unset _sk_sites _sk_n _sk_bad _sk_pairs _sk_site _sk_f _sk_v _sk_l _sk_a +unset -f _sk_sweep _sk_loop_names _sk_arm_names diff --git a/test/selftest/parts.manifest b/test/selftest/parts.manifest index 26090671..b3a184ad 100644 --- a/test/selftest/parts.manifest +++ b/test/selftest/parts.manifest @@ -43,3 +43,4 @@ 440-a-count-grep-never-produced.sh 450-a-red-nightly-must-be-findable.sh 460-the-controller-must-record-the-binary.sh +470-a-skipped-arm-records-under-its-own-name.sh diff --git a/test/sorted_pathkeys.sh b/test/sorted_pathkeys.sh index 85b477e0..f829c88b 100755 --- a/test/sorted_pathkeys.sh +++ b/test/sorted_pathkeys.sh @@ -275,11 +275,26 @@ ansp "and it answers in C order, matching heap" colh colc \ ALTCOLL="$(q "SELECT collname FROM pg_collation WHERE collname IN ('en_US.utf8','en_US.UTF-8','en_US','und-x-icu') ORDER BY 1 LIMIT 1;")" if [ -z "$ALTCOLL" ] || \ [ "$(q "SELECT (min(k) COLLATE \"C\") = (SELECT min(k COLLATE \"$ALTCOLL\") FROM colh) FROM colh;" 2>/dev/null)" != "f" ]; then - check_skip "the collation-change demonstration" "SKIP the collation-change demonstration: this server has no collation that" "this server has no suitable collation" + # ONE SKIP PER ARM, UNDER THE ARM'S OWN NAME (#994). Seven arms sat behind one + # skip named for none of them, so a reader could not tell which seven did not + # run. Two of them go through `ansp`, which records under its first argument -- + # they are arms like any other and were missed by a sweep that looked only for + # `check`. + for _sp_n in "premise: C and the alternate collation really disagree on this data" \ + "premise: the collation ALTER rewrote nothing (same storage id)" \ + "premise: so the run is still recorded as lexicographic" \ + "premise: and the column's collation really did change" \ + "REFUSE: the order the rows are in is no longer the order the column asks for" \ + "and ORDER BY k LIMIT returns the new collation's first rows, matching heap" \ + "and the whole ordered result matches heap"; do + check_skip "$_sp_n" \ + "SKIP $_sp_n (this server has no collation that disagrees with C on ASCII)" \ + "this server has no suitable collation" + done echo " disagrees with C on ASCII, so the arm could not fail and is not run." echo " The refusal it demonstrates is asserted above on COLLATE \"C\"." else - check "premise: C and $ALTCOLL really disagree on this data" \ + check "premise: C and the alternate collation really disagree on this data" \ "$([ "$(q "SELECT k FROM colh ORDER BY k COLLATE \"C\" LIMIT 1;")" \ != "$(q "SELECT k FROM colh ORDER BY k COLLATE \"$ALTCOLL\" LIMIT 1;")" ] && echo yes || echo no)" "yes" From 01c6e8da982733a55a334beb2a729c30160cff91 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 11 Sep 2026 20:50:29 -0600 Subject: [PATCH 2/2] test: six sites, and the guard checks every loop (#994, from review) @OffgridwithJD's review of #998 found three things, all real, all verified here before acting on them. FIVE ARMS VANISHED IN THE FILE THIS PR IS MOST ABOUT. 340 skipped five arms behind `if [ -z "$_fp_user" ]; then : # already skipped above`. "Above" skipped the three PREMISES, not these five. Worse, #998 made that comment more convincing rather than less: a reader auditing the file now sees a loop above and moves on. The repair needs `_fp_base=""` before the block -- it is assigned only in block 1's `else`, and under `set -uo pipefail` letting the `elif` fire without it kills the suite, which is presumably why the `:` was there. SIX SITES, NOT FOUR. The two outer pyarrow gates hold their arms in the `then` branch with the skip in the `else`. A classifier looking only FORWARD from a skip reads those as armless -- which is how the issue came to say four, and how my own tool then repeated the same mistake. THE GUARD COMPARED ONE LOOP PER (FILE, VARIABLE), AND 340 HAS THREE. The one it compared was the loop #998 had just written both sides of, so it agreed by construction and examined nothing. Rewritten as a script that walks every loop. Two further faults of my own surfaced fixing that: a `check_skip "$VAR"` line counted as an ARM, which silently dropped four sites from comparison python inside a HEREDOC parsed as shell `if`, unbalancing the walk so a ten-arm branch reported six -- the tool that finds silently-lost arms was silently losing arms Both fixed; structure is now read from a heredoc-blanked view while names come from the real lines. AND THE GAP IS STATED RATHER THAN HIDDEN. 340's five-arm loop -- the one this change repairs -- is NOT compared, because its sibling arm is generated by a loop of its own and a literal comparison would be wrong in both directions. Driven both ways: dropping a name from a comparable loop is caught, dropping one from that loop is not. The part prints `interpolated 1` so a total of zero mismatches cannot read as agreement. loops 8, compared 6, interpolated 1, armless 1, mismatches 0 Verified: harness_selftest 863/863 on PG16, shellcheck clean at CI's own flags, docs_style 9/9, ledger 905 -> 909 with the census DERIVED and the ceiling unchanged. KNOWN COLLISION: this PR's sorted_pathkeys loop lists `and the whole ordered result matches heap`, which #992 renames. Whichever lands second makes part 470 red until the list is updated. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw --- .github/scripts/skip-loop-arms.py | 220 ++++++++++++++++++ CHANGELOG.md | 14 ++ test/check_ledger.tsv | 7 +- test/check_ledger_budget.txt | 2 +- test/native_parquet_flba.sh | 19 +- test/native_parquet_pushdown.sh | 11 +- .../340-the-binary-must-be-built-from.sh | 15 +- ...-skipped-arm-records-under-its-own-name.sh | 104 ++++----- 8 files changed, 324 insertions(+), 68 deletions(-) create mode 100755 .github/scripts/skip-loop-arms.py diff --git a/.github/scripts/skip-loop-arms.py b/.github/scripts/skip-loop-arms.py new file mode 100755 index 00000000..2fb6ab0f --- /dev/null +++ b/.github/scripts/skip-loop-arms.py @@ -0,0 +1,220 @@ +#!/usr/bin/env python3 +"""Compare every skip loop's names against the arms its sibling branch emits. + +#994. A `for VAR in ; do check_skip "$VAR" ...; done` exists so that a +skipped arm records under the name it would have used. The loop DUPLICATES those +names, so a rename in the sibling branch desynchronises them silently and the +skip starts recording under a name nothing emits. + +WHY A SCRIPT AND NOT awk IN THE PART. The first version used two awk extractors +that each `exit` on first match, so a file with three loops over one variable +compared only the first -- and the one it compared was the one whose both sides +had just been written together, so it agreed by construction. + +WHAT IS AND IS NOT COMPARED, reported rather than assumed: + + loops every `for VAR in ... check_skip "$VAR"` found + armless the sibling branch names no arms; the skip IS the record + interpolated either side contains a shell expansion, so a literal set + comparison would be wrong in both directions + compared the rest, where a mismatch is a real finding + +`interpolated` is printed because a comparison nobody makes and a comparison that +passes are indistinguishable in a total of mismatches. +""" + +import pathlib +import re +import sys + +# Derived, not listed: any function defined in the corpus whose body reaches +# check/pgc_record emits a record under its first argument. +_HELPER_DEF = re.compile(r"^([a-z_][a-z0-9_]*)\(\)\s*\{", re.M) +_LOOP = re.compile(r"^\s*for\s+([a-z_][a-z0-9_]*)\s+in\b") + + +_HEREDOC = re.compile(r"<<-?\s*'?\"?([A-Za-z_][A-Za-z0-9_]*)'?\"?\s*$") + + +def blank_heredocs(lines): + """Return lines with heredoc BODIES blanked out. + + Shell-shaped tokens inside a heredoc are not shell. `native_parquet_flba.sh` + embeds python whose `if phys != {...}:` a naive depth count reads as a shell + `if`, which unbalanced the walk and made a ten-arm branch report six. The + tool that finds silently-lost arms was silently losing arms. + """ + out, term = [], None + for l in lines: + if term is None: + m = _HEREDOC.search(l) + out.append(l) + if m: + term = m.group(1) + else: + out.append("") + if l.strip() == term: + term = None + return out + + +def emitters(text): + out = {"check", "check_num", "check_skip", "check_text", "check_timing"} + for m in _HELPER_DEF.finditer(text): + name, start = m.group(1), m.end() + end = text.find("\n}", start) + body = text[start:end if end > 0 else len(text)] + if re.search(r"\bcheck\w*\b|\bpgc_record\b", body): + out.add(name) + return out + + +def names_in(lines, emit): + pat = re.compile(r"^\s*(?:%s)\s+\"([^\"]*)\"" % "|".join(sorted(emit, key=len, reverse=True))) + out = set() + for l in lines: + m = pat.match(l) + if not m: + continue + n = m.group(1) + # A BARE VARIABLE REFERENCE IS THE SKIP MECHANISM, NOT AN ARM. A sibling + # branch often contains its own `check_skip "$VAR"` loop; counting that as + # an arm name made four comparable sites read as INTERPOLATED and therefore + # uncompared -- a silent loss of coverage in the tool that reports coverage. + if re.fullmatch(r"\$\{?[a-z_][a-z0-9_]*\}?", n): + continue + out.add(n) + return out + + +def loop_blocks(lines, var): + """Every `for in` header block and the line index it ends on.""" + out = [] + for i, l in enumerate(lines): + m = _LOOP.match(l) + if not m or m.group(1) != var: + continue + j = i + while j < len(lines) and not lines[j].rstrip().endswith("; do"): + j += 1 + out.append((i, j)) + return out + + +def _sibling_before(lines, start): + """If the loop sits in an `else` branch, return the `then` branch above it.""" + depth = 0 + j = start - 1 + while j >= 0: + t = lines[j].strip() + if t == "fi" or t.startswith("fi "): + depth += 1 + elif t.startswith("if ") or t.startswith("if["): + if depth == 0: + return [] # reached our own `if` without meeting an `else` + depth -= 1 + elif (t == "else" or t.startswith("elif ")) and depth == 0: + # collect from here back to the matching `if` + k, d2, idx = j - 1, 0, [] + while k >= 0: + u = lines[k].strip() + if u == "fi" or u.startswith("fi "): + d2 += 1 + elif u.startswith("if ") or u.startswith("if["): + if d2 == 0: + return idx + d2 -= 1 + idx.append(k) + k -= 1 + return [] # ran off the top without a balanced `if`: unparsed + j -= 1 + return [] + + +def sibling_lines(real, struct, start): + """Walk the heredoc-free view, return the corresponding real lines.""" + idx = sibling(struct, start, want_index=True) + return [real[i] for i in idx] + + +def sibling(lines, start, want_index=False): + """The branch the loop is the alternative to, looking BOTH ways. + + A loop in the `then` branch has its arms after the `else`. A loop in the + `else` branch has them BEFORE it, after the `if`. The first version looked + only forward, so the two outer pyarrow gates -- whose thirteen arms sit in + the `then` branch -- read as ARMLESS: a false negative of exactly the kind + this tool exists to find, in the tool. @OffgridwithJD made the same mistake + classifying the sites in #994, which is why there were six rather than four. + """ + back = _sibling_before(lines, start) + if back: + return back + depth, j = 0, start + while j < len(lines): + s = lines[j].strip() + if s.startswith("if ") or s.startswith("if["): + depth += 1 + elif s == "fi" or s.startswith("fi "): + if depth == 0: + return [] + depth -= 1 + elif (s == "else" or s.startswith("elif ")) and depth == 0: + k, d2, idx = j + 1, 0, [] + while k < len(lines): + t = lines[k].strip() + if t.startswith("if "): + d2 += 1 + elif t == "fi": + if d2 == 0: + return idx + d2 -= 1 + idx.append(k) + k += 1 + return idx + j += 1 + return [] + + +def main(testdir): + root = pathlib.Path(testdir) + files = sorted(list(root.glob("*.sh")) + list((root / "selftest").glob("*.sh"))) + lib = (root / "lib.sh").read_text(errors="replace") if (root / "lib.sh").exists() else "" + + loops = compared = interpolated = armless = 0 + bad = [] + for f in files: + text = f.read_text(errors="replace") + if "check_skip \"$" not in text: + continue + lines = text.splitlines() + # STRUCTURE IS READ FROM A HEREDOC-FREE VIEW; names still come from the + # real lines, because a heredoc never contains a check. + struct = blank_heredocs(lines) + emit = emitters(text) | emitters(lib) + for var in sorted(set(re.findall(r'check_skip\s+"\$([a-z_][a-z0-9_]*)"', text))): + for start, hdr_end in loop_blocks(struct, var): + loops += 1 + loop_names = {s for s in re.findall(r'"([^"]*)"', "\n".join(lines[start:hdr_end + 1])) + if s and not s.startswith("$")} + arms = names_in(sibling_lines(lines, struct, hdr_end), emit) + if not arms: + armless += 1 + continue + if any("$" in n for n in loop_names | arms): + interpolated += 1 + continue + compared += 1 + if loop_names != arms: + bad.append(f"{f.name}:{start + 1}") + print(f"loops {loops}") + print(f"compared {compared}") + print(f"interpolated {interpolated}") + print(f"armless {armless}") + for b in bad: + print(f"MISMATCH {b}") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1] if len(sys.argv) > 1 else "test")) diff --git a/CHANGELOG.md b/CHANGELOG.md index 596d9bcb..26b5968e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -503,6 +503,15 @@ true until the next version shipped. skip, so the skip would have recorded a key no real run emits. That name is now stable and the collation it names moves into the display, which is not the key. + Six sites, not four, and 19 arms. Two arms go through `ansp`, which records + under its first argument. Two sites hold their arms in the `then` branch with + the skip in the `else`, which a classifier looking only forward reads as having + no arms at all. + + `340` also skipped five arms behind a branch whose comment said they had + already been skipped above. Above had skipped the three premises, not these + five, so on a box with no non-root user five arms produced no record. + A new selftest part asserts every skip loop names exactly the arms its sibling branch would emit. The loop duplicates those names, so a rename desynchronises them silently and the skip records under a name nothing emits, which is the @@ -510,6 +519,11 @@ true until the next version shipped. a name from another open PR's rename went into the loop, and the comparison is what caught it. + The part reports what it did not compare. One loop's sibling arm is generated + by a loop of its own, so a literal comparison would be wrong in both + directions. That loop is the one this change repairs, and it is not covered. + A total of zero mismatches would otherwise read as a corpus in agreement. + This is the precondition for arming the orphan guard in #983. Until a skipped arm records under its own name, absence cannot mean removal. diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index d62af8de..755ed3ed 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -859,9 +859,10 @@ harness_selftest 460-the-controller-must-record-the-binary premise: the mutation harness_selftest 460-the-controller-must-record-the-binary premise: this part was given an executable pg_config to read the prefix from never - harness_selftest 460-the-controller-must-record-the-binary so a child suite under PGC_SKIP_BUILD reaches verified, not source-only (#961) never - harness_selftest 460-the-controller-must-record-the-binary the controller's stamp carries BOTH fields (#961) never - -harness_selftest 470-a-skipped-arm-records-under-its-own-name every skip loop names exactly the arms its sibling branch would emit (#994) never - -harness_selftest 470-a-skipped-arm-records-under-its-own-name premise: at least four loops had arms to be compared against never - -harness_selftest 470-a-skipped-arm-records-under-its-own-name premise: the sweep found skip loops to compare never - +harness_selftest 470-a-skipped-arm-records-under-its-own-name every comparable skip loop names exactly the arms its sibling would emit (#994) never - +harness_selftest 470-a-skipped-arm-records-under-its-own-name premise: and it actually compared some of them never - +harness_selftest 470-a-skipped-arm-records-under-its-own-name premise: the sweep found skip loops at all never - +harness_selftest 470-a-skipped-arm-records-under-its-own-name premise: the sweep tool is present never - native_join_runtime_filter native_join_runtime_filter 3-table answer equals filter-off never - native_join_runtime_filter native_join_runtime_filter 3-table join order matches filter-off never - native_join_runtime_filter native_join_runtime_filter 3-table plan has coordinator never - diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index 1d44692e..05cd37ff 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -34,4 +34,4 @@ suites_not_covered 250 # Without that it is a hand-maintained count that drifts, which is the failure # this repository has spent a day proving. It is not a ceiling; it is a # measurement that must be true. -checks_never_observed_red 908 +checks_never_observed_red 909 diff --git a/test/native_parquet_flba.sh b/test/native_parquet_flba.sh index 740f2952..bd70df90 100755 --- a/test/native_parquet_flba.sh +++ b/test/native_parquet_flba.sh @@ -243,7 +243,24 @@ PYINT "-3500000,0,1250000" fi else - check_skip "the foreign-producer FLBA cases" "SKIP pyarrow not available; foreign-producer FLBA cases skipped" "pyarrow not available" + # ONE SKIP PER ARM, UNDER THE ARM'S OWN NAME (#994). This is the OUTER gate: + # without pyarrow none of the ten arms in the `then` branch runs, and a single + # skip named for none of them left all ten with no record. Only one branch ever + # fires, so naming the six that the inner gate also names duplicates nothing. + for _fl_o in "pyarrow uuid reads as uuid" \ + "pyarrow decimal128 values are exact" \ + "crafted out-of-range scale is rejected, not decoded" \ + "backend survived the crafted scale" \ + "INT32-backed DECIMAL reads" \ + "INT64-backed DECIMAL reads" \ + "INT32-backed DECIMAL keeps its null" \ + "parquet_schema advises numeric for an INT32 DECIMAL" \ + "parquet_schema advises numeric for an INT64 DECIMAL" \ + "an INT64 DECIMAL still binds to bigint as the unscaled integer"; do + check_skip "$_fl_o" \ + "SKIP $_fl_o (pyarrow not available)" \ + "pyarrow not available" + done fi pgc_summary diff --git a/test/native_parquet_pushdown.sh b/test/native_parquet_pushdown.sh index ce019c82..0c31eaf7 100755 --- a/test/native_parquet_pushdown.sh +++ b/test/native_parquet_pushdown.sh @@ -232,7 +232,16 @@ PYDEC "$(skipped_for_t ftdec 'd >= 0')" "0" fi else - check_skip "the integer-DECIMAL pushdown case" "SKIP pyarrow not available; integer-DECIMAL pushdown case skipped" "pyarrow not available" + # ONE SKIP PER ARM, UNDER THE ARM'S OWN NAME (#994). The OUTER gate: without + # pyarrow none of the three arms runs, and a single skip named for none of them + # left all three with no record. + for _pd_o in "INT64-backed DECIMAL: predicate skips 3 of 4 groups" \ + "INT64-backed DECIMAL: skipping did not drop rows" \ + "INT64-backed DECIMAL: unfiltered scan skips nothing"; do + check_skip "$_pd_o" \ + "SKIP $_pd_o (pyarrow not available)" \ + "pyarrow not available" + done fi pgc_summary 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 6bc2f9a9..ae49258f 100644 --- a/test/selftest/340-the-binary-must-be-built-from.sh +++ b/test/selftest/340-the-binary-must-be-built-from.sh @@ -635,6 +635,11 @@ cp "$PGC_TESTDIR/lib.sh" "$PGC_TESTDIR/portlib.sh" "$PGC_TESTDIR/pgc_fingerprint "$_fp_harness/" 2>/dev/null chmod -R a+rX "$_fp" +# INITIALISED BEFORE BLOCK 1, because the arms block below reads it in an `elif` +# and `_fp_base` is assigned only inside block 1's `else`. Under `set -uo pipefail` +# an unassigned read kills the suite, which is why the branch below used to be a +# bare `:` -- and that `:` is what silently dropped five arms (#994 review). +_fp_base="" _fp_user="" if [ "$(id -u)" -ne 0 ]; then _fp_user="-" # already unprivileged; read in this shell @@ -708,9 +713,13 @@ fi # loudly -- never running them against a reader that is broken for a reason they # do not describe. The premises stay FAILED in that case, so the suite is still # red and still says why. -if [ -z "$_fp_user" ]; then - : # already skipped above -elif [ -z "$_fp_base" ] || [ "$_fp_base" = harness-unreadable ] \ +# NO `: # already skipped above` BRANCH. There was one, and it was wrong: "above" +# skipped the three PREMISES, not these five arms, so on a box with no non-root +# user five arms produced no record at all -- indistinguishable from five deleted +# checks, which is the whole of #994. With `_fp_base` initialised empty the `elif` +# below fires and skips all five under their own names, and its list already names +# them, so nothing is duplicated. +if [ -z "$_fp_base" ] || [ "$_fp_base" = harness-unreadable ] \ || [ "$_fp_base" != "$(pgc_source_fingerprint "$_fp/tree")" ]; then for _fp_n in "an unreadable b.c yields no fingerprint, not a wrong one" \ "an unreadable c.c yields no fingerprint, not a wrong one" \ diff --git a/test/selftest/470-a-skipped-arm-records-under-its-own-name.sh b/test/selftest/470-a-skipped-arm-records-under-its-own-name.sh index 1ec9b3f8..8bd7b004 100644 --- a/test/selftest/470-a-skipped-arm-records-under-its-own-name.sh +++ b/test/selftest/470-a-skipped-arm-records-under-its-own-name.sh @@ -1,8 +1,8 @@ # ---- a skipped arm must record under the name it would have used ------------ # -# #994. Four sites skipped a whole block under ONE name that none of the arms -# inside it had. When the condition failed, those arms produced no record at all: -# a reader could not tell WHICH did not run, and the ledger could not tell a +# #994. Sites skipped a whole block under ONE name that none of the arms inside +# it had. When the condition failed, those arms produced no record at all: a +# reader could not tell WHICH did not run, and the ledger could not tell a # skipped arm from a deleted one, because a skipped arm's row has no matching # record -- exactly as a removed check's would. # @@ -11,69 +11,55 @@ # # WHY A GUARD AND NOT JUST THE FIX. The loop DUPLICATES the arm names, so a rename # in the sibling branch silently desynchronises them and the skip records under a -# name nothing emits -- which is the exact failure the fix exists to remove, -# reintroduced by an edit nobody thought was risky. +# name nothing emits -- the exact failure the fix exists to remove, reintroduced by +# an edit nobody thought was risky. Writing #994 I put into a loop a name that a +# DIFFERENT open PR renames, and this comparison is what caught it. # -# That is not hypothetical. Writing #994 I put a name in the loop that a DIFFERENT -# open PR renames, and the set comparison below is what caught it before it -# shipped. #982's renames are still landing, so this drifts again the moment one -# of them touches these files. +# EVERY LOOP, NOT THE FIRST ONE PER VARIABLE. The first version of this part +# extracted with an `exit` on first match, so a file with three loops over the same +# variable compared only the first -- and 340 has three (@OffgridwithJD). Worse, +# the one it compared was the one #994 had just written BOTH sides of, so it agreed +# by construction and examined nothing. # -# DERIVED, NOT LISTED. The sweep finds every `for VAR in ... check_skip "$VAR"` -# loop in the corpus, so a fifth site added later is covered on the day it is -# written rather than when somebody remembers this file. - -_sk_sweep() { # _sk_sweep -> "file:var" per skip loop, derived - grep -rlE 'check_skip "\$[a-z_]+"' "$TESTDIR"/*.sh "$TESTDIR"/selftest/*.sh 2>/dev/null | - while read -r _f; do - grep -oE 'check_skip "\$[a-z_]+"' "$_f" | sed 's/.*"\$\(.*\)"/\1/' | sort -u | - while read -r _v; do - grep -qE "for $_v in" "$_f" && printf '%s:%s\n' "$_f" "$_v" - done - done -} +# THE GAP, STATED BECAUSE IT COVERS THE LOOP THIS CHANGE REPAIRS. `340`'s +# five-arm loop is NOT compared: its sibling arm is +# `check "an unreadable $_fp_n yields ..."`, generated by a loop over b.c and c.c, +# so a literal set comparison would report a false mismatch on a correct site. +# Driven: dropping a name from that loop is NOT caught, while dropping one from a +# comparable loop is. The count below is what makes that visible -- `interpolated 1` +# is a loop nobody checked, and a total of zero mismatches would otherwise read +# as a corpus in agreement. +# +# AND A COMPARISON IS ONLY MADE WHERE BOTH SIDES ARE LITERAL. `340`'s second loop +# has an INTERPOLATED sibling -- `check "an unreadable $_fp_n yields ..."` -- which +# is correct at runtime and would read as a mismatch to a literal set comparison. +# Those are counted and reported rather than silently dropped, because a comparison +# nobody makes and a comparison that passes look identical in a total. -_sk_loop_names() { # _sk_loop_names FILE VAR -> the names the loop skips under - awk -v v="$2" '$0 ~ ("for " v " in") {i=1} i {print} i && /; do$/ {exit}' "$1" | - grep -oE '"[^"]*"' | tr -d '"' | grep -v '^$' | sort -u -} +_sk_py="$TESTDIR/../.github/scripts/skip-loop-arms.py" -_sk_arm_names() { # _sk_arm_names FILE VAR -> the names the sibling branch emits - awk -v v="$2" '$0 ~ ("for " v " in") {f=1} - f && /^[[:space:]]*else/{i=1;f=0;next} - i && /^[[:space:]]*fi[[:space:]]*$/{exit} i' "$1" | - grep -oE '^[[:space:]]*(check|check_num|check_text|check_ratio|ansp|ansq?|diff_query|diff_query_ordered)[[:space:]]+"[^"]*"' | - sed 's/.*"\(.*\)"/\1/' | sort -u -} +check "premise: the sweep tool is present" \ + "$([ -r "$_sk_py" ] && echo yes || echo no)" "yes" -_sk_sites="$(_sk_sweep)" -_sk_n=$(printf '%s\n' "$_sk_sites" | grep -c . || true) +_sk_out="$(python3 "$_sk_py" "$TESTDIR" 2>&1)" || _sk_out="TOOL FAILED: $_sk_out" -# THE POPULATION IS THE PREMISE. A sweep that matched nothing reports the same -# zero mismatches as a corpus in perfect agreement, and only one of those is news. -check "premise: the sweep found skip loops to compare" \ - "$([ "${_sk_n:-0}" -ge 4 ] && echo yes || echo "only $_sk_n")" "yes" +_sk_loops=$(printf '%s\n' "$_sk_out" | sed -n 's/^loops \([0-9]*\)$/\1/p') +_sk_cmp=$(printf '%s\n' "$_sk_out" | sed -n 's/^compared \([0-9]*\)$/\1/p') +_sk_skip=$(printf '%s\n' "$_sk_out" | sed -n 's/^interpolated \([0-9]*\)$/\1/p') +_sk_armless=$(printf '%s\n' "$_sk_out"| sed -n 's/^armless \([0-9]*\)$/\1/p') +_sk_bad="$(printf '%s\n' "$_sk_out" | sed -n 's/^MISMATCH //p' | tr '\n' ' ' | sed 's/ $//')" -_sk_bad="" -_sk_pairs=0 -for _sk_site in $_sk_sites; do - _sk_f="${_sk_site%:*}"; _sk_v="${_sk_site##*:}" - _sk_l="$(_sk_loop_names "$_sk_f" "$_sk_v")" - _sk_a="$(_sk_arm_names "$_sk_f" "$_sk_v")" - # A loop guarding a block with no named arms is fine -- the skip IS the record - # and nothing else claimed to run. Only a loop with arms to agree with is compared. - [ -z "$_sk_a" ] && continue - _sk_pairs=$((_sk_pairs + 1)) - if [ "$_sk_l" != "$_sk_a" ]; then - _sk_bad="$_sk_bad $(basename "$_sk_f")" - fi -done +echo "-- skip loops: $_sk_loops found, $_sk_cmp compared, $_sk_skip interpolated, $_sk_armless armless" -check "premise: at least four loops had arms to be compared against" \ - "$([ "$_sk_pairs" -ge 4 ] && echo yes || echo "only $_sk_pairs")" "yes" +# THE POPULATION IS THE PREMISE, twice over. A sweep that matched nothing reports +# the same zero mismatches as a corpus in agreement; and a sweep that found loops +# but compared none of them reports the same thing again. +check "premise: the sweep found skip loops at all" \ + "$([ "${_sk_loops:-0}" -ge 6 ] && echo yes || echo "only ${_sk_loops:-none}")" "yes" +check "premise: and it actually compared some of them" \ + "$([ "${_sk_cmp:-0}" -ge 4 ] && echo yes || echo "only ${_sk_cmp:-none}")" "yes" -check "every skip loop names exactly the arms its sibling branch would emit (#994)" \ - "$(printf '%s' "$_sk_bad" | sed 's/^ //')" "" +check "every comparable skip loop names exactly the arms its sibling would emit (#994)" \ + "$_sk_bad" "" -unset _sk_sites _sk_n _sk_bad _sk_pairs _sk_site _sk_f _sk_v _sk_l _sk_a -unset -f _sk_sweep _sk_loop_names _sk_arm_names +unset _sk_py _sk_out _sk_loops _sk_cmp _sk_skip _sk_armless _sk_bad