Stop two cleanup handlers from turning success into failure - #38
Merged
Conversation
A cleanup function whose last command is a false test returns 1, and bash
applies that to the script's exit status from an EXIT trap. A run that
did everything right then reports failure, and an explicit `exit 0` does
not save it:
cleanup() { [[ -n "$D" && -d "$D" ]] && rm -rf "$D"; }
trap cleanup EXIT
exit 0 # exits 1 when $D is empty
Both trap handlers here had that shape -- oidc-login-test.sh and
vault-upgrade.sh. Probed directly, main's versions exit 1 on an early
success path and the fixed ones exit 0.
Latent rather than live: nothing currently exits 0 before the temporary
directory is created, so every path that reaches cleanup with the
variable empty has already failed for its own reasons. It becomes live
the moment someone adds "already at the target version, nothing to do" --
a natural thing to add to an upgrade script, and one nobody would connect
to a cleanup function.
Worth being precise about where this bites, because the same shape is
idiomatic and harmless nearly everywhere. Under set -e, a trailing `&&`
whose test is false does *not* exit at the top level, and does not exit
as the last statement of an if/else branch. It is specifically the
function-return path -- and the trap that consumes that return value.
I asserted the if/else case was a bug earlier in this work and it is not;
testing all three shapes is what settled it.
tests/lint is a new home for invariants shellcheck has no opinion about,
wired into CI. Its first check rejects this pattern in any function
registered with `trap`, and reports the offending line with the
if-block form to use instead. Run against main it names both handlers.
Also fixes two `grep -c ... || echo 0` in the integration suite. grep -c
prints 0 *and* exits 1 when it matches nothing, so the fallback appends a
second zero and the variable holds two lines -- which errors the
arithmetic comparison rather than reporting an empty CA bundle. `|| true`
is the correct form, since grep has already printed the count.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A cleanup function whose last command is a false test returns 1, and bash
applies that to the script's exit status from an EXIT trap. A run that
did everything right reports failure, and an explicit
exit 0does notsave it.
Both trap handlers in this repository had that shape. Probed directly:
vault-upgrade.sh, early success pathoidc-login-test.sh, early success pathLatent, not live — and why that still matters
Nothing currently exits 0 before the temporary directory is created, so
every path reaching cleanup with an empty variable has already failed for
its own reasons.
It becomes live the moment someone adds "already at the target version,
nothing to do" — a natural addition to an upgrade script, and one nobody
would connect to a cleanup function three screens away. The failure would
surface as a red CI job or a broken automation step with no error
message.
Where this actually bites
Worth being precise, because the same shape is idiomatic and harmless
nearly everywhere. Under
set -e, a trailing&&whose test is false:It is specifically the function-return path, and the trap that consumes
that return value.
I claimed the if/else case was a bug earlier in this work. It is not —
testing all three shapes is what settled it, and my first test of the
if/else case was itself wrong because the branch never executed.
Preventing recurrence
tests/lintis a new home for invariants shellcheck has no opinionabout, wired into CI as a 24th check. Its first rule rejects this pattern
in any function registered with
trap, and prints the offending lineplus the form to use. Run against main it names both handlers:
It only inspects functions actually named in a
trap, so the harmlesstop-level and if/else uses elsewhere are left alone.
Also
Two
grep -c ... || echo 0in the integration suite.grep -cprints0and exits 1 when it matches nothing, so the fallback appends a second
zero and the variable holds two lines — which errors the arithmetic
comparison rather than reporting an empty CA bundle.
|| trueis correct,since grep has already printed the count.