fix: a suite must not decide a verdict from a pipeline's exit status (#486)

[jdatcmd]

Closes #486.

echo "$s" | grep -q PATTERN under set -o pipefail answers "not found" whenever the WRITER fails, whatever the string contained. The && arm never runs and the helper reports absence.

It reddened #484's PG18 CI:

native_agg.sh: line 41: echo: write error: Broken pipe
FAIL  count(*) uses the metadata agg node: got [no] want [yes]
PASS  sum/min/max uses the metadata agg node

The failing check and the passing one below it call the same helper on plan text from the same node. The same job re-run on the identical commit passed.

The failure direction is why this is a rule and not a one-line fix: it always reports the thing you were looking for as absent, which reads as a real regression in whatever area the check covers. That one read as a planner regression in the area #133 and #140 live in.

Measured before writing anything

form	300 KB string, match on line 1
echo "$s" | grep -q	no, which is wrong
case "$s" in *pat*)	yes
grep -q pat <<<"$s"	yes

What the mechanism is not. The issue first said grep's early exit causes this, and at a kilobyte it cannot: an EXPLAIN plan fits entirely in the 64 KB pipe buffer, so the writer's write() completes before grep can have matched anything. Something else must have killed the reader; under a six-way parallel matrix, a grep killed under memory pressure is plausible. Not proved, not claimed. What is proved is that the helper's answer depends on the reader surviving, and pipefail turns anything that happens to it into a wrong answer. Corrected on the issue rather than left standing.

The change

60 rewrites across 19 files, as herestrings. case is what #473 used for the fixed-string membership test and is still right there, but most of these carry regex, alternation and -i, which a glob cannot express; a herestring keeps the pattern and flags exactly and removes the pipeline, so the status is grep's alone. Verified equivalent on the positive, negative, regex, whole-line and unterminated-last-line cases.

The rule lives in harness_selftest.sh with its control above it, because a rule with no demonstrated failure is a style preference and this one is not.

Scope is deliberate and stated in the comment: a reader whose EXIT STATUS is the answer. | head -1 inside a diagnostic string stays, because losing that pipeline's status changes a message and no verdict. analyze_stats.sh has several; they are left alone on purpose rather than missed. An earlier draft of the rule flagged them, which would have made it a rule people argue with rather than keep.

The script was wrong once

Matching grep instead of grep -q rewrote echo "$err" | grep -oiE ... | head -1 into grep -oiE ... | head -1 <<<"$err" — redirecting head's stdin and leaving grep reading the script's, which would have printed the first line of $err unfiltered. That grep's output is used, not its status, so it was never in scope. Caught by reading the diff rather than trusting bash -n, which accepted it happily. The script now matches only -q and refuses any line where a further unquoted | means grep is not the last stage.

Verification

• Proved by removal: re-arming one instance fails the rule naming the exact file:line; restoring it passes.
• All 19 affected suites re-run: 19 PASSED, 0 FAILED.
• bash -n on every changed file, and the complete diff read by eye.
• Not covered by the above, and checked separately rather than counted as covered: fuzz_arrow and fuzz_parquet run 4 checks on a clean run, and their 14 rewrites are crash and sanitizer classification branches a clean fuzz run never enters. Both forms of all three conditions were compared over nine inputs including an ASAN report, a UBSAN misaligned-load report, a signal-11 log and a connection refusal: 27 comparisons, all agreeing, with a sanity line proving the classifications are not all "miss" (which would make agreement vacuous).

🤖 Generated with Claude Code

[ChronicallyJD]

Approving — this is strictly better than what is on main, the removal proof is the right way to land a rule, and catching your own script's head-redirect bug by reading the diff rather than trusting bash -n is the part I would have missed.

Two things: a correction of mine, and one finding that I think matters for whether this closes #486.

First, my own comment on the issue was reasoning from the wrong model

I ranked the instances by "how exposed the writer is", putting psql | grep -q above the echo forms because psql "has more left to write". Your measurement kills that: below the 64 KB pipe buffer the write completes before the reader can exit, so the writer's nature does not come into it. I reproduced yours and it is writer-agnostic:

bytes	builtin writer	external writer
3,008	yes	yes
300,008	no	no

Size decides, not the writer. My ranking was wrong and the reason I gave for lib.sh mattering most was wrong with it.

The finding: the fix covers the mechanism the control demonstrates, not the one your analysis favours

Your correction says the early-exit route cannot explain the CI failure, because an EXPLAIN plan fits in the pipe buffer, and posits a reader killed by something else under the six-way matrix. I think that is right. But a herestring only removes the writer from the pipeline — the answer is still a process's exit status, so a killed reader still produces the wrong answer.

Reader finds the match and is then killed (stub exiting 137, plan-sized input, string contains the pattern, correct answer is yes in every row):

form	answer
echo "$s" | grep -q (today)	no
grep -q <<<"$s" (this PR)	no
case "$s" in *pat*)	yes

And under the 300 KB early-exit route your control uses:

form	answer
echo "$s" | grep -q	no
grep -q <<<"$s"	yes
case "$s" in *pat*)	yes

So the herestring fixes the 300 KB case and not the kilobyte-plus-dead-reader case. The control in harness_selftest.sh demonstrates the one it fixes, which is why it passes.

I want to be careful about how far this goes: which mechanism actually fired in #484's CI is unproved, and you say so. If it was a large write, this PR closes it. If it was a killed reader — the explanation your own analysis prefers — the rewritten helpers will answer no on the next occurrence exactly as they did on the last one, and the rule will report compliance while they do.

What I would change, and it is small

Draw the rule's boundary at "is the answer a process's exit status?" rather than at what is on the left of the pipe. That is the property that produced the wrong verdict, and it is the one that survives both mechanisms.

The reason given for herestrings over case is that most of these carry regex, alternation and -i that a glob cannot express. Bash covers all three without a process — [[ ]] is a keyword and ${var,,} is a parameter expansion, so neither can be killed, take EPIPE, or have a status to corrupt:

[[ $s =~ Gather( Merge)? ]]        # ERE, alternation, optional group
[[ $s =~ Index.*Scan ]]            # ERE
[[ ${s,,} =~ canceling\ statement ]]   # the -i cases
case "$s" in *'Index Scan'*|*'Bitmap Scan'*) ;; esac   # fixed-string alternation

Verified each of those against the pattern kinds this PR rewrites.

That would also let the rule's regex stop caring about echo|printf, which is what currently leaves test/lib.sh:521 (pgc_is_columnar_scan, called by 8 suites), native_groupagg.sh:50 and arrow_import.sh:172 outside it. I am no longer arguing those are more exposed than the ones you fixed — that was my error above. I am arguing they are exposed identically, and that a rule scoped to the writer cannot say so.

None of this blocks the merge. The change is an improvement either way, and if you would rather land it and treat the [[ ]]/case conversion as a follow-on, I am happy to write that one — say whether you want lib.sh in it or separate, since the shared helper changes 8 suites' behaviour and the rest are local.

Smaller notes

• The vacuity guard on the scan (_epipe_scanned -ge 20) is the right instinct and I would not have thought to add it.
• The fuzz_arrow/fuzz_parquet treatment — 27 comparisons over nine inputs, with a sanity line proving the classifications are not uniformly "miss" — is the answer to "a clean run never enters these branches". That is the check that stops the other 26 from being vacuous, and it is worth keeping as a pattern.
• Leaving analyze_stats.sh's | head -1 diagnostics alone, and saying in the comment that they are deliberate rather than missed, is what will stop someone re-flagging them in six months.