diff --git a/CHANGELOG.md b/CHANGELOG.md index a309698b1..f49d40fe0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -320,6 +320,13 @@ true until the next version shipped. ### Fixed +- `test/native_upgrade_converge.sh` no longer leaves its PostgreSQL server + running and its workdir in `/tmp` after every run (#1321). It armed its own + `trap cleanup EXIT` after `pgc_setup`, which replaced the harness's + `pgc_on_exit`; its cleanup now calls `pgc_on_exit` first. Selftest part 620 + fails any suite that calls `pgc_setup` and arms an EXIT trap reaching neither + `pgc_on_exit` nor `pgc_teardown`. Reported by @OffgridwithJD. + - An `ALTER TABLE` on a partitioned columnar table that does not recurse no longer opens the other partitions without a lock. `DETACH PARTITION`, `DETACH PARTITION ... CONCURRENTLY`, `ATTACH PARTITION` and `ALTER TABLE ONLY` diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index 345ee9186..5e307f09a 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -1642,6 +1642,11 @@ harness_selftest 610-a-declined-capability-is-not-silent the verdict: names lost harness_selftest 610-a-declined-capability-is-not-silent the verdict: names lost beside a skip naming the capability are declined 15;16;17;18;19 2026-10-12 pgc_capability_verdict has no declined outcome, so a lost name beside a skip reads silent harness_selftest 610-a-declined-capability-is-not-silent the verdict: names lost with neither a failure nor a skip are silent 15;16;17;18;19 2026-10-12 pgc_capability_verdict answers declined for every input harness_selftest 610-a-declined-capability-is-not-silent the verdict: no name lost is preserved 15;16;17;18;19 2026-10-12 pgc_capability_verdict answers declined for every input +harness_selftest 620-a-suite-s-own-trap-must-stop and is not satisfied by the name in a comment or in the next function 15;16;17;18;19 2026-10-13 the trap detector never reports a suite;the trap detector reads a one-line handler on into the next function;the trap detector reads comments as code +harness_selftest 620-a-suite-s-own-trap-must-stop and passes a handler that chains either, and a suite without pgc_setup 15;16;17;18;19 2026-10-13 the trap detector accepts only pgc_on_exit, not pgc_teardown +harness_selftest 620-a-suite-s-own-trap-must-stop no suite's own EXIT trap leaves its cluster running 15;16;17;18;19 2026-10-13 native_upgrade_converge.sh's cleanup without pgc_on_exit, so its EXIT trap replaces lib.sh's;the trap detector accepts only pgc_on_exit, not pgc_teardown +harness_selftest 620-a-suite-s-own-trap-must-stop premise: pgc_setup still arms the trap this part protects 15;16;17;18;19 never - +harness_selftest 620-a-suite-s-own-trap-must-stop the detector flags a handler that reaches neither pgc_on_exit nor pgc_teardown 15;16;17;18;19 2026-10-13 the trap detector never reports a suite;the trap detector sees EXIT only at the end of a trap line index_am_support index_am_support btree answers its operator with the same rows the scan returns 15;16;17;18;19 2026-09-23 pgcolumnar_index_build_range_scan drops two rows in three, so the index misses entries index_am_support index_am_support btree builds an index over a columnar table 15;16;17;18;19 2026-09-23 pgcolumnar_index_build_range_scan errors, so CREATE INDEX fails index_am_support index_am_support gist answers its operator with the same rows the scan returns 15;16;17;18;19 2026-09-23 pgcolumnar_index_build_range_scan drops two rows in three, so the index misses entries diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index 663fb4c2c..ae04a0431 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -1443,4 +1443,18 @@ suites_not_covered 247 # never 1698 + 2 = 1700 # dated 361 + 8 = 369 # 1700 + 369 == 2069 -checks_never_observed_red 1700 +# +# #1321, a suite's own EXIT trap must still stop its cluster (OffgridwithJD): +# 1700 -> 1701. Selftest part 620 enters with 5 checks from a five-major +# harness_selftest run (1228/1228 on each of 15-19). Four arms are dated: the +# population arm by the run before the fix (native_upgrade_converge.sh's cleanup +# without pgc_on_exit), the three detector arms by five mutations of the +# detector. The fifth, 'premise: pgc_setup still arms the trap this part +# protects', is a fact about lib.sh, the fixture this part reads, and arrives +# never. +# +# ledger rows 2069 + 5 = 2074 +# never 1700 + 1 = 1701 +# dated 369 + 4 = 373 +# 1701 + 373 == 2074 +checks_never_observed_red 1701 diff --git a/test/native_upgrade_converge.sh b/test/native_upgrade_converge.sh index 7cbd3a091..0249d5655 100755 --- a/test/native_upgrade_converge.sh +++ b/test/native_upgrade_converge.sh @@ -137,7 +137,12 @@ for v in "${_NUC_FIXTURES[@]}"; do cp "$src" "$dst" STAGED+=("$dst") done +# CHAINED, not replacing: pgc_setup armed `trap pgc_on_exit EXIT`, and bash keeps +# one EXIT trap. Without this line the trap below replaced it, and every run left +# its server listening and its workdir in /tmp (selftest part 620). First, so it +# reads the suite's exit status before anything here overwrites `$?`. cleanup() { + pgc_on_exit for f in "${STAGED[@]:-}"; do [ -n "$f" ] || continue if [ -f "$f.pgcbak" ]; then diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index b8c389afd..20b3672de 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -144,6 +144,7 @@ behaviour, the source of that number is named. - [96. test_autovacuum_expire.py: the daemon expires retention tables only when told to](#96-test_autovacuum_expirepy-the-daemon-expires-retention-tables-only-when-told-to) - [97. test_rollup.py: incremental rollups fold appended row groups, and equal a full recompute](#97-test_rolluppy-incremental-rollups-fold-appended-row-groups-and-equal-a-full-recompute) - [98. test_timeseries_recipes.py: the gap-fill, LOCF and interpolation recipes answer what they claim](#98-test_timeseries_recipespy-the-gap-fill-locf-and-interpolation-recipes-answer-what-they-claim) +- [99. test_suite_exit_trap.py: a suite's own EXIT trap must still stop its cluster](#99-test_suite_exit_trappy-a-suites-own-exit-trap-must-still-stop-its-cluster) ## 1. How to read a test in here @@ -6639,3 +6640,20 @@ Red first: on main the how-to has no recipes, so the premise reads 0 and the com | --- | --- | | `test_the_recipes_are_in_the_how_to` | each of the three headings has a `sql` block naming `readings` | | `test_each_recipe_matches_the_reference` | the table is columnar; the gap fill, LOCF and interpolation match the Python reference for all 24 hours; the gap fill gives the same buckets in an Asia/Kolkata session; its aggregate runs as the vectorized grouped node when that is on | + +## 99. test_suite_exit_trap.py: a suite's own EXIT trap must still stop its cluster + +The pytest twin of selftest part 620 (#1321). `pgc_setup` arms `trap pgc_on_exit EXIT`, which stops the server, removes the workdir and gives a suite that died its accounting (#1233). Bash keeps one EXIT trap, so a suite that arms its own after `pgc_setup` must call back into `pgc_on_exit` or `pgc_teardown`. `native_upgrade_converge.sh` did not, and every run left its server listening and its workdir in `/tmp`. + +Independent of the part: it reads the suites and `lib.sh` as text but never sources or runs them, and its reader is Python's own. Trap lines are split with `shlex`. Comments are removed by the shell's rule (a `#` that starts a word, outside quotes, with backslash escapes) rather than by cutting everything after `#`. A one-line handler is read as that line, and any other up to its closing brace. The check names match the part's, five of them. It is not graded by `compare_to_bash`, which pairs `test/.sh` with `test_.py` and so cannot see a selftest part; `test_capability_decline.py` (parts 600 and 610) is the precedent. + +Written red first: on the tree before the fix, the population arm fails naming `native_upgrade_converge.sh` and nothing else. Five mutations of this file's reader each redden the same arms as the matching mutation of the part: it never reports, it keeps comments, it reads past a one-line handler, it accepts only `pgc_on_exit`, and it sees `EXIT` only last on the line. + +### Every test + +| test | what it holds | +| --- | --- | +| `test_the_detector_flags_a_handler_that_reaches_neither` | two suites the test writes, one trapping `EXIT` and one `EXIT INT TERM`, are both reported | +| `test_the_detector_is_not_satisfied_by_a_comment_or_the_next_function` | the teardown named only in a comment, or only in the function after a one-line handler, does not count | +| `test_the_detector_passes_a_handler_that_chains` | a handler calling `pgc_on_exit`, one calling `pgc_teardown`, a quoted handler, and a suite without `pgc_setup` are not reported | +| `test_no_suites_own_exit_trap_leaves_its_cluster_running` | `pgc_setup` still arms `trap pgc_on_exit EXIT` (premise), and no suite in `test/` is reported | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 2c3009b81..f855f4e52 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -269,7 +269,9 @@ # (#1286 review), collected over the NO_CLUSTER files on this tree. # guard_tests 413 -> 414 for #1305's test_an_unguarded_import_is_refused_under_xdist_too, by collection. # guard_tests 414 -> 415 for test_docs_how_to_sections.py (the lost pg_partman heading), by collection. -guard_tests 415 +# guard_tests 415 -> 419 for #1321's test_suite_exit_trap.py, four tests, the twin of +# selftest part 620, collected over the NO_CLUSTER files on this tree. +guard_tests 419 # The complement: tests that need the driver and a throwaway cluster. Until #1016 these ran # in no CI job at all -- a quarter of the corpus, green when somebody ran them by hand and diff --git a/test/pytest/test_harness_deps.py b/test/pytest/test_harness_deps.py index d7505b7d2..ef529100f 100644 --- a/test/pytest/test_harness_deps.py +++ b/test/pytest/test_harness_deps.py @@ -77,6 +77,9 @@ # #1215: inner pytest runs of expect.declines and require_module. Needs no # cluster and no driver; it never reaches into the shell harness. "test_capability_decline.py", + # #1321: reads the suites and lib.sh as text, with its own Python reader. Needs + # no cluster and no driver, and never runs the shell harness. + "test_suite_exit_trap.py", # The classifier's own controls, split out of this file so the job whose file # list IS this list actually runs them. This file cannot be in the list: it # hands cluster-bound file names to pytest, so it needs what they need. @@ -1199,6 +1202,11 @@ def test_the_job_installs_no_database_driver(expect): "matches on cannot become a hand-maintained copy that rots apart from it " "(#1040, where five of eight helpers had been unreadable since the pattern " "shipped)", + "test_suite_exit_trap.py": + "reads every suite's text and the helper library's setup function as DATA, " + "with its own Python reader, to find an EXIT trap that replaces the one setup " + "arms (#1321). It never sources or executes either: the rule is about what " + "the shell text says, so the text is the only place to read it", } _SHELL_NAMES = re.compile( diff --git a/test/pytest/test_suite_exit_trap.py b/test/pytest/test_suite_exit_trap.py new file mode 100644 index 000000000..4c2981091 --- /dev/null +++ b/test/pytest/test_suite_exit_trap.py @@ -0,0 +1,144 @@ +"""A suite's own EXIT trap must still stop its cluster (#1321). + +The pytest half of selftest part 620, with its own reader. It does not source, call +or import lib.sh, the part, or any shell helper: it reads the suites as text, the +way the part does, but through Python's shlex rather than sed and awk, so the twins +share the rule and not the parser. + +pgc_setup arms `trap pgc_on_exit EXIT`; pgc_on_exit stops the server and removes the +workdir (pgc_teardown) and gives a suite that died its accounting (#1233). Bash keeps +one EXIT trap, so a suite that arms its own after pgc_setup must call back into one +of the two. native_upgrade_converge.sh did not, and every run left its server +listening and its workdir in /tmp. + +Needs no cluster and no driver. +""" + +import pathlib +import re +import shlex + +HERE = pathlib.Path(__file__).resolve().parent +TESTDIR = HERE.parent +CHAIN = ("pgc_on_exit", "pgc_teardown") + + +def _code(text): + """The text with shell comments removed: a `#` that starts a word, outside quotes. + + Not "everything after #": `$#` and `${x#y}` are not comments. A backslash escapes + the next character except inside single quotes, as in the shell. + """ + out = [] + for line in text.splitlines(): + cut, quote, i = None, None, 0 + while i < len(line): + ch = line[i] + if ch == "\\" and quote != "'": + i += 2 + continue + if quote: + if ch == quote: + quote = None + elif ch in "'\"": + quote = ch + elif ch == "#" and (i == 0 or line[i - 1] in " \t;"): + cut = i + break + i += 1 + out.append(line if cut is None else line[:cut]) + return out + + +def _handlers(lines): + """The handler of every `trap HANDLER ... EXIT ...` line.""" + found = [] + for line in lines: + try: + words = shlex.split(line) + except ValueError: + continue + if len(words) >= 3 and words[0] == "trap" and "EXIT" in words[2:]: + found.append(words[1]) + return found + + +def _body(lines, name): + """The function's own lines: one for `name() { ...; }`, else up to its `}`.""" + for i, line in enumerate(lines): + if not line.startswith(name + "()"): + continue + if re.search(r"\{.*\}\s*$", line): + return [line] + body = [line] + for nxt in lines[i + 1:]: + body.append(nxt) + if nxt.startswith("}"): + break + return body + return [] + + +def _offenders(paths): + bad = [] + for path in paths: + lines = _code(pathlib.Path(path).read_text()) + if not any(re.match(r"\s*pgc_setup(\s|$)", ln) for ln in lines): + continue + for h in _handlers(lines): + if any(c in h for c in CHAIN): + continue + if any(c in ln for ln in _body(lines, h) for c in CHAIN): + continue + bad.append(pathlib.Path(path).name) + break + return bad + + +SUITES = { + "replaces": "pgc_setup x\ncleanup() {\n\trm -f y\n}\ntrap cleanup EXIT\n", + "replaces_on_signals": "pgc_setup x\ncleanup() {\n\trm -f y\n}\ntrap cleanup EXIT INT TERM\n", + "names_it_in_a_comment": "pgc_setup x\ncleanup() {\n\trm -f y # then pgc_teardown\n}\ntrap cleanup EXIT\n", + "next_function_has_it": "pgc_setup x\nt() { a; }\nother() {\n\tpgc_teardown\n}\ntrap t EXIT\n", + "chains_the_wrapper": "pgc_setup x\ncleanup() {\n\tpgc_on_exit\n\trm -f y\n}\ntrap cleanup EXIT\n", + "chains_the_teardown": "pgc_setup x\nt() { a; pgc_teardown; }\ntrap t EXIT INT TERM\n", + "chains_in_quotes": "pgc_setup x\ntrap 'restore; pgc_on_exit' EXIT\n", + "no_setup": "cleanup() {\n\trm -f y\n}\ntrap cleanup EXIT\n", +} + + +def _built(tmp_path, *names): + paths = [] + for n in names: + p = tmp_path / f"{n}.sh" + p.write_text(SUITES[n]) + paths.append(p) + return paths + + +def test_the_detector_flags_a_handler_that_reaches_neither(tmp_path, expect): + expect.text("\n".join(_offenders(_built(tmp_path, "replaces", "replaces_on_signals"))), + "replaces.sh\nreplaces_on_signals.sh", + "the detector flags a handler that reaches neither pgc_on_exit nor pgc_teardown") + + +def test_the_detector_is_not_satisfied_by_a_comment_or_the_next_function(tmp_path, expect): + expect.text("\n".join(_offenders(_built(tmp_path, "names_it_in_a_comment", "next_function_has_it"))), + "names_it_in_a_comment.sh\nnext_function_has_it.sh", + "and is not satisfied by the name in a comment or in the next function") + + +def test_the_detector_passes_a_handler_that_chains(tmp_path, expect): + got = _offenders(_built(tmp_path, "chains_the_wrapper", "chains_the_teardown", + "chains_in_quotes", "no_setup")) + expect.text(f"[{' '.join(got)}]", "[]", + "and passes a handler that chains either, and a suite without pgc_setup") + + +def test_no_suites_own_exit_trap_leaves_its_cluster_running(expect): + lib = _code((TESTDIR / "lib.sh").read_text()) + setup = _body(lib, "pgc_setup") + expect.num(sum(1 for ln in setup if re.search(r"\btrap pgc_on_exit EXIT\b", ln)), 1, + "premise: pgc_setup still arms the trap this part protects") + got = _offenders(sorted(TESTDIR.glob("*.sh"))) + expect.text(f"[{' '.join(got)}]", "[]", "no suite's own EXIT trap leaves its cluster running") diff --git a/test/selftest/620-a-suite-s-own-trap-must-stop.sh b/test/selftest/620-a-suite-s-own-trap-must-stop.sh new file mode 100644 index 000000000..e3b916b82 --- /dev/null +++ b/test/selftest/620-a-suite-s-own-trap-must-stop.sh @@ -0,0 +1,68 @@ +# ---- a suite's own EXIT trap must still stop its cluster --------------------- +# +# pgc_setup arms `trap pgc_on_exit EXIT`, and pgc_on_exit is what stops the +# server and removes the workdir (pgc_teardown) and gives a suite that died its +# accounting (#1233). Bash keeps ONE EXIT trap, so a suite that arms its own +# after pgc_setup replaces all of that unless its handler calls back into it. +# objstore_module.sh found this first and chains; native_upgrade_converge.sh did +# not: its `trap cleanup EXIT` restored the staged fixtures and nothing else, so +# every run left a PostgreSQL server listening and its workdir in /tmp. Measured +# on PG17: each of five converge runs left exactly one server, started within a +# second of the run's last log line. +# +# READ FROM THE SUITES' OWN TEXT, the one place the rule can be checked for all +# of them at once; driving each suite to see whether its server outlives it would +# take every suite's full run. The detector is driven below on suites written for +# it, so a detector that flags nothing cannot pass the population arm. Captured +# text reaches grep and awk as here-strings, never through a pipe (part 080). + +_te_offenders() { + local f code h fn + for f in "$@"; do + code="$(sed 's/#.*//' "$f")" + grep -qE '^[[:space:]]*pgc_setup( |$)' <<<"$code" || continue + while IFS= read -r h; do + case "$h" in *pgc_on_exit*|*pgc_teardown*) continue ;; esac + # The handler's own body: one line for a one-line function, else up to + # its closing brace -- never on into the next function's. + fn="$(awk -v h="$h" ' + !on && index($0, h "()") == 1 && $0 ~ /\{.*\}[[:space:]]*$/ { print; exit } + !on && index($0, h "()") == 1 { on = 1 } + on { print } + on && /^}/ { exit }' <<<"$code")" + grep -qE 'pgc_on_exit|pgc_teardown' <<<"$fn" && continue + basename "$f" + break + done < <(sed -nE "s/^[[:space:]]*trap[[:space:]]+(['\"]([^'\"]*)['\"]|([A-Za-z_][A-Za-z_0-9]*))[[:space:]]+(.*[[:space:]])?EXIT([[:space:]].*)?$/\2\3/p" <<<"$code") + done +} + +check_num "premise: pgc_setup still arms the trap this part protects" \ + "$(sed -n '/^pgc_setup()/,/^}/p' "$PGC_TESTDIR/lib.sh" | sed 's/#.*//' | grep -c 'trap pgc_on_exit EXIT')" "1" + +_te_tmp="$(mktemp -d)" +_te_mk() { printf '%b\n' "$2" > "$_te_tmp/$1.sh"; } +_te_mk replaces 'pgc_setup x\ncleanup() {\n\trm -f y\n}\ntrap cleanup EXIT' +_te_mk replaces_on_signals 'pgc_setup x\ncleanup() {\n\trm -f y\n}\ntrap cleanup EXIT INT TERM' +_te_mk names_it_in_a_comment 'pgc_setup x\ncleanup() {\n\trm -f y # then pgc_teardown\n}\ntrap cleanup EXIT' +_te_mk next_function_has_it 'pgc_setup x\nt() { a; }\nother() {\n\tpgc_teardown\n}\ntrap t EXIT' +_te_mk chains_the_wrapper 'pgc_setup x\ncleanup() {\n\tpgc_on_exit\n\trm -f y\n}\ntrap cleanup EXIT' +_te_mk chains_the_teardown 'pgc_setup x\nt() { a; pgc_teardown; }\ntrap t EXIT INT TERM' +_te_mk chains_in_quotes "pgc_setup x\ntrap 'restore; pgc_on_exit' EXIT" +_te_mk no_setup 'cleanup() {\n\trm -f y\n}\ntrap cleanup EXIT' + +check_text "the detector flags a handler that reaches neither pgc_on_exit nor pgc_teardown" \ + "$(_te_offenders "$_te_tmp/replaces.sh" "$_te_tmp/replaces_on_signals.sh")" \ + "$(printf 'replaces.sh\nreplaces_on_signals.sh')" +check_text "and is not satisfied by the name in a comment or in the next function" \ + "$(_te_offenders "$_te_tmp/names_it_in_a_comment.sh" "$_te_tmp/next_function_has_it.sh")" \ + "$(printf 'names_it_in_a_comment.sh\nnext_function_has_it.sh')" +check_text "and passes a handler that chains either, and a suite without pgc_setup" \ + "[$(_te_offenders "$_te_tmp/chains_the_wrapper.sh" "$_te_tmp/chains_the_teardown.sh" \ + "$_te_tmp/chains_in_quotes.sh" "$_te_tmp/no_setup.sh")]" "[]" +rm -rf "$_te_tmp" + +check_text "no suite's own EXIT trap leaves its cluster running" \ + "[$(_te_offenders "$PGC_TESTDIR"/*.sh | tr '\n' ' ')]" "[]" + +unset -f _te_offenders _te_mk diff --git a/test/selftest/parts.manifest b/test/selftest/parts.manifest index e1964777a..2326f5778 100644 --- a/test/selftest/parts.manifest +++ b/test/selftest/parts.manifest @@ -58,3 +58,4 @@ 590-a-declared-suite-must-account.sh 600-a-missing-pyarrow-declines.sh 610-a-declined-capability-is-not-silent.sh +620-a-suite-s-own-trap-must-stop.sh