Skip to content

The skipped status must be one the harness owns (#447) - #455

Merged
jdatcmd merged 1 commit into
commandprompt:mainfrom
ChronicallyJD:fix/skipped-exit-collision
Aug 6, 2026
Merged

The skipped status must be one the harness owns (#447)#455
jdatcmd merged 1 commit into
commandprompt:mainfrom
ChronicallyJD:fix/skipped-exit-collision

Conversation

@ChronicallyJD

Copy link
Copy Markdown
Collaborator

#448 used exit 2 for "ran no checks". That was wrong, and it is my defect.

2 is a status suites already produce for unrelated reasons. bash exits 2 on a parse error in
the suite file, and smoke, phase2-phase6 and audit run under set -euo pipefail, so they
abort with whatever status the failing command returned. A dead postmaster or a typo therefore
became "ran no checks", every runner printed SKIP, and the major reported PASS.

That is the lie #447 was opened to remove, relocated one layer down and shipped by the fix for it.

BEFORE  broken suite exits 2 -> main classifies rc=2 as skip -> SKIP   <- the bug
AFTER   broken suite exits 2 -> only rc=66 + marker is a skip -> FAIL  <- correct
AFTER   real skip: native_repack on PG18, rc=66, marker present -> SKIP

Now 66, which bash (1, 2, 126, 127, 128+n), psql (1, 2, 3) and make do not produce. It cannot
be made collision-proof, because set -e propagates any status an aborting command returns, so
the runners also require the SKIPPED (ran no checks) line in the log. Two independent
signals, because one was not enough.

Four more from the same review, all in merged code

  • native_parquet_streaming's sparse-file gate still called bare pgc_summary. The pyarrow
    gate thirty lines above it was converted and this one was missed, so a low-space or non-sparse
    filesystem took the 1GB-palloc ceiling and the 1600MB-hole guard out of the run while the major
    reported PASS. The dead exit 0 after it was the tell.
  • run_san's skip branch was justified by the pyarrow suites, which cannot reach it. A missing
    dependency FAILS, so pgc_skip exits 1. The comment described a case the code could not catch.
  • run_san printed SANITIZER GATE PASSED with 22 of 23 suites skipped, because the guard
    fired only when all skipped. Any skip now makes the gate incomplete.
  • run_coverage got the skip bucket but not the ran-nothing guard, so its only verdict was
    "nothing failed" — which a box where every suite aborts satisfies perfectly. It now also sets
    PGC_SKIP_TIMING=1, because a --coverage build's wall clock means nothing and this runner
    discovers every test/*.sh.
  • pg19_vacuum_options and native_repack read an unanswerable version probe as "not PG19"
    via ${srv:-0}. A dead cluster now fails. The failure is raised only when it occurs: a check
    that always ran would put PGC_CHECKS at 1 and destroy the very skip it guards on older majors.

Found by a high-effort review of test/, then verified on the bench.

)

commandprompt#448 used exit 2 for "ran no checks". That was wrong. 2 is a status suites
already produce for unrelated reasons: bash exits 2 on a parse error in the suite
file, and smoke, phase2 through phase6 and audit run under set -euo pipefail, so
they abort with whatever status the failing command returned. A dead postmaster
or a typo therefore became "ran no checks", every runner printed SKIP, and the
major reported PASS. That is the lie commandprompt#447 was opened to remove, relocated one
layer down and shipped by the fix for it.

Now 66, which bash (1, 2, 126, 127, 128+n), psql (1, 2, 3) and make do not
produce. That cannot be made collision-proof, because set -e propagates any
status, so the runners require the SKIPPED line in the log as well. Two
independent signals.

Also from the same review:

- native_parquet_streaming's sparse-file gate still called bare pgc_summary. The
  pyarrow gate above it was converted and this one was missed, so a low-space
  filesystem took the 1GB-palloc ceiling and the 1600MB-hole guard out of the run
  while the major passed.
- run_san's rc branch was justified by the pyarrow suites, which cannot reach it:
  a missing dependency FAILS, so pgc_skip exits 1. Comment corrected to describe
  what the branch actually catches.
- run_san reported PASSED with 22 of 23 suites skipped, because the guard fired
  only when ALL skipped. Any skip now makes the gate incomplete.
- run_coverage got the skip bucket and not the ran-nothing guard, so its only
  verdict was 'nothing failed' -- which a box where every suite aborts satisfies.
- pg19_vacuum_options and native_repack read an unanswerable version probe as
  'not PG19' via ${srv:-0}. A dead cluster now fails. The failure is raised only
  when it occurs, because a check that always ran would put PGC_CHECKS at 1 and
  destroy the very skip it guards on older majors.

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. I verified the central claim rather than taking it, because I approved #450 and this is a defect in code I passed.

On origin/main, right now:

run_all_versions.sh:459   elif [ "$_rc" = 2 ]; then      <- classified as skip
run_all_versions.sh:609   elif [ "$_exrc" = 2 ]; then    <- same, extended runner
$ printf 'if [ 1\n' > bad.sh && bash bad.sh; echo $?
2

psql exits 2 on a bad connection, and smoke, phase2-phase6 and audit run under set -euo pipefail. So a suite with a syntax error, or one that aborts because the postmaster is gone, is reported as SKIP and the major reports PASS. That is #447's lie relocated one layer down by the fix for it, exactly as you describe.

The two-signal design is the part I would have got wrong. 66 is unreachable for bash (1, 2, 126, 127, 128+n), psql (1, 2, 3) and make, but set -e propagates whatever an aborting command returns, so no single status can be made collision-proof by choosing a rarer number. Requiring the SKIPPED (ran no checks) marker in the log alongside it is what closes that, and saying so in the PR rather than claiming 66 is safe is the right framing.

The five follow-ons are the same shape as the thing they fix, and native_parquet_streaming's bare pgc_summary is the worst of them: a low-space or non-sparse filesystem silently removed the 1GB-palloc ceiling and the 1600MB-hole guard while the major reported PASS. The dead exit 0 being the tell is a good detail to have written down.

run_san printing SANITIZER GATE PASSED with 22 of 23 suites skipped is the one I would have shipped myself — a guard that fires only when all skipped reads as correct until you write out what "any" means.

Two notes, neither blocking:

  • run_coverage setting PGC_SKIP_TIMING=1 is right, and it now depends on check_ratio_timing from #444 being present to actually drop ratio checks rather than fail them. Worth checking the merge order between these two.
  • ${srv:-0} on the version probe: agreed that raising only when it occurs is what preserves the skip on older majors. That reasoning deserves to stay in the comment, because the obvious "simplification" is a check that always runs, and it would destroy the skip it guards.

@jdatcmd
jdatcmd merged commit 7d122e7 into commandprompt:main Aug 6, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants