fix(installer): fetch-plugins.sh reported success after doing nothing - #1337
Merged
Merged
Conversation
`set -e` is deliberately off in bin/fetch-plugins.sh -- the download loop
needs to tolerate curl failures and retry -- but the swap at the end of the
script was unguarded:
rm -rf "$dest"
mkdir -p "$(dirname "$dest")"
mv "$staging" "$dest"
say "Plugins at $pluginsVer"
so `say` ran whatever those three did. Hit for real on a tree left
root-owned by a previous install while verifying the FOG_PLUGINS_VERSION
bump that carried the Font Awesome 7 migration: every operation failed with
"Permission denied", the script printed "Plugins at v1.6.15" and exited 0,
and v1.6.14 was still on disk.
That is the worst available shape. downloadplugins() already tests the exit
status correctly, so the installer reported the step done, and the version
stamp still named the old release -- nothing downstream could tell either.
After the Font Awesome migration the consequence is a server running FA4
plugin icon names against a core that ships no v4 shims, with no error
anywhere naming the cause.
The swap is the only step whose failure produces a wrong answer rather than
an error, so it is the only one checked; the download loop keeps its
tolerate-and-retry behaviour. downloadplugins()'s guidance also now points
at the error log, because the fetcher fails for two unrelated reasons and
only one of them is the offline-install advice it was printing.
tests/plugin-tree-integrity.test.sh gains three gates covering a read-only
destination: non-zero exit, no success line, and the old tree still in
place. Verified by restoring the original unguarded swap, which fails two
of them.
Co-Authored-By: Claude <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
1 participant
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.
The bug
set -eis deliberately off inbin/fetch-plugins.sh— the download loop needs to tolerate curl failures and retry — but the swap at the end was unguarded:Hit for real while verifying the
FOG_PLUGINS_VERSIONbump in #1336, on a tree left root-owned by a previous install:Exit status 0.
Why it matters
downloadplugins()already tests the exit status correctly, so the installer reported the step done — the script was the only thing lying. And the version stamp still named the old release, so nothing downstream could tell either: no error, no warning, no artifact anywhere naming the cause.After #1336 the consequence is concrete. A server in this state runs the old plugin release's FA4 icon names against a core that ships no v4 shims — blank icons across every plugin menu entry, from an install that said it succeeded.
The fix
The swap is the only step whose failure produces a wrong answer rather than an error, so it is the only one checked. The download loop keeps its tolerate-and-retry behaviour untouched.
downloadplugins()'s failure guidance also now points at the error log: the fetcher fails for two unrelated reasons — could not reach a verified release, could not write the tree — and it was printing the offline-install advice for both.Verification
tests/plugin-tree-integrity.test.shgains three gates covering a read-only destination — non-zero exit, no success line, old tree still in place. Skipped as root, which ignores the mode.Mutation-tested by restoring the original unguarded swap: two of the three gates fail. A fourth defensive check (reading the stamp back after the swap) was written and then removed — no mutant could distinguish it, because with the guards in place there is no reachable path where all three commands return 0 and the stamp is still wrong.