fix(tools): fail closed where errexit is off

.github/workflows/update.yml

@@ -48,9 +48,16 @@ jobs:
   #
   # First, so a branch that went green since the last run is on `main` before
   # `check-updates.sh` compares anything against `main`, and before the publish job
-  # below asks whether `main` has been published. `!cancelled()` rather than
-  # `success()`: a failure here is a branch that did not land, and it must not stop
-  # the scheduled check from finding new releases.
+  # below asks whether `main` has been published.
+  #
+  # This job goes red when a step failed on any branch -- a `gh` or `git` call that
+  # could not run -- after it has tried every other branch. The jobs below carry
+  # `!cancelled()` rather than `success()`, which is what makes red here safe: a
+  # branch that did not land must not stop the scheduled check from finding new
+  # releases, and `main` may still need publishing for a commit that landed earlier
+  # in this run or by a human merge. Red is kept rather than swallowed with
+  # `continue-on-error`, because that would show a green run for an hour in which
+  # nothing could be landed.
   land:
     runs-on: ubuntu-latest
     permissions:

tools/check-origin.sh

@@ -42,9 +42,19 @@ trap 'rm -f "$rows" "$missing" "$named"' EXIT
 # package's own pkginfo, so `.url` is what `apk info` prints. A noarch package
 # appears in every architecture's index and is wanted once, which is what the
 # sort -u below is for.
-find "$OUT" -name index.json -exec \
+#
+# No `2>/dev/null || true` on either read. That pair hid a jq or awk that failed on
+# one index, and every package in it went unchecked while the rest passed -- a check
+# that cannot run counts as failed. `find` matching nothing still exits 0; `find`
+# exits non-zero when an `-exec ... {} +` invocation does.
+if ! find "$OUT" -name index.json -exec \
 	jq -r '.packages[]? | ["apk", .name, .version, (.url // "")] | @tsv' {} + \
-	>> "$rows" 2>/dev/null || true
+	>> "$rows"; then
+	echo "tools/check-origin.sh: could not read every index.json under $OUT" >&2
+	echo "  failed: find $OUT -name index.json -exec jq -r '.packages[]? | ...' {} +" >&2
+	echo "  jq's own error is above; rebuild the index (owfeed index) and re-run" >&2
+	exit 1
+fi
 
 # opkg, from the text index. Both spellings are read because both appear in
 # practice: a package built by OpenWrt's SDK carries the repository in `URL:` and
@@ -54,7 +64,7 @@ find "$OUT" -name index.json -exec \
 #
 # Continuation lines cannot be mistaken for fields here: opkg indents them with a
 # space, and `Description:` -- the only multi-line field -- is written last.
-find "$OUT" -name Packages -type f -exec awk '
+if ! find "$OUT" -name Packages -type f -exec awk '
 	function flush() {
 		if (name != "") {
 			origin = (url != "") ? url : source
@@ -73,7 +83,12 @@ find "$OUT" -name Packages -type f -exec awk '
 	/^URL: /        { url     = substr($0, 6)  }
 	/^Source: /     { source  = substr($0, 9)  }
 	END             { flush() }
-' {} + >> "$rows" 2>/dev/null || true
+' {} + >> "$rows"; then
+	echo "tools/check-origin.sh: could not read every Packages index under $OUT" >&2
+	echo "  failed: find $OUT -name Packages -type f -exec awk ... {} +" >&2
+	echo "  awk's own error is above; rebuild the index (owfeed index) and re-run" >&2
+	exit 1
+fi
 
 [ -s "$rows" ] || { echo "tools/check-origin.sh: no package found in any index under $OUT" >&2; exit 1; }
 

tools/check-updates.sh

@@ -103,7 +103,17 @@ may_automerge() {
 	# Only this job's own commits count. Counting every commit that touched the file
 	# counts the one that added the package, and every hand edit to it since --
 	# which in a young repository is enough to refuse the first real update.
-	_recent="$(git log --since='24 hours ago' --author='owfeed-bot' --oneline -- "$_up" | wc -l | tr -d ' ')"
+	#
+	# Read first and counted second. This function runs as an `if` condition, where
+	# errexit is off, and in `git log | wc -l` a failed git still counts to 0 --
+	# under the ceiling, so a failure read as permission. Every failed read in here
+	# returns 1: the cost is a pull request, never an unreviewed merge.
+	if ! _log="$(git log --since='24 hours ago' --author='owfeed-bot' --oneline -- "$_up")"; then
+		echo "  could not count recent automatic updates: 'git log --since=\"24 hours ago\" --author=owfeed-bot -- $_up' failed"
+		return 1
+	fi
+	_recent=0
+	[ -z "$_log" ] || _recent="$(printf '%s\n' "$_log" | wc -l | tr -d ' ')"
 	if [ "$_recent" -ge 2 ]; then
 		echo "  $_recent automatic updates to $_name in the last day: the next one wants a person"
 		return 1
@@ -122,7 +132,15 @@ may_automerge() {
 	# changed line anywhere else means either a bug here or an upstream.sh that was
 	# edited between the checkout and now -- and SIG_KEY_ID moving would be the
 	# whole verification quietly relaxing itself.
-	_bad="$(git diff -U0 -- "$_up" \
+	#
+	# The diff is captured on its own for the same reason as the log above: piped
+	# straight into the filters, a failed `git diff` is an empty list of changed
+	# lines, and the `|| true` the filters need turns that into "only pins moved".
+	if ! _diff="$(git diff -U0 -- "$_up")"; then
+		echo "  could not read what changed: 'git diff -U0 -- $_up' failed, so it is not merging itself"
+		return 1
+	fi
+	_bad="$(printf '%s\n' "$_diff" \
 		| grep -E '^[+-][^+-]' \
 		| grep -vE '^[+-](VERSION|TAG|ARTIFACT|ARTIFACT_IPK|SHA256|SHA256_IPK)=' \
 		| grep -vE '^[+-][a-zA-Z0-9_.-]+ +[0-9a-f]{64} +' || true)"
@@ -207,15 +225,40 @@ for up in packages/*/upstream.sh; do
 		# fetch.sh's, so a package that pins no tag is still compared against
 		# exactly the tag fetch.sh would download for it.
 		current_tag="${TAG:-v${VERSION%-r*}}"
-		# `|| true` is load-bearing under `set -e`: an assignment takes the exit
-		# status of its command substitution, so a repository with no releases at
-		# all -- or a `gh` that could not reach GitHub -- would kill this subshell
-		# here, before the line below can say so. It used to survive that by
-		# accident: the old command ended in `| sed`, and a pipeline reports its
-		# last command.
-		latest_tag="$(gh release view --repo "$REPO" --json tagName -q .tagName 2>/dev/null || true)"
+		# Scratch space for this package, removed however the subshell ends. Made
+		# before the first `gh` call, whose stderr is read below.
+		tmp="$(mktemp -d)"
+		trap 'rm -rf "$tmp"' EXIT
 
-		[ -n "$latest_tag" ] || { echo "$name: upstream has no releases"; exit 0; }
+		# "Upstream has no releases" and "could not ask" are different answers, and
+		# only the first is green. This was `2>/dev/null || true`, which read a 401,
+		# a 502 or a dropped connection as "no releases" and left the run green with
+		# the package never checked -- a check that cannot run counts as failed.
+		#
+		# gh does not tell them apart by exit code. Measured with gh 2.99.0: a
+		# repository with no releases (octocat/Hello-World), a repository that does
+		# not exist, a bad token and an unreachable proxy all exit 1. The first two
+		# print exactly `release not found` -- both are a 404 on /releases/latest --
+		# and the others print the HTTP or transport error. So `release not found` is
+		# confirmed with a question an existing repository answers and a missing one
+		# fails, the release list; a renamed or deleted upstream is a REPO to fix,
+		# not a quiet hour. Every other failure stops this package through the
+		# handler after the subshell.
+		if ! latest_tag="$(gh release view --repo "$REPO" --json tagName -q .tagName 2>"$tmp/view.err")"; then
+			if [ "$(cat "$tmp/view.err")" = "release not found" ] &&
+				gh api "repos/$REPO/releases?per_page=1" -q length >/dev/null 2>"$tmp/list.err"; then
+				echo "$name: upstream has no releases"
+				exit 0
+			fi
+			{
+				echo "$name: could not read the latest release of $REPO"
+				cat "$tmp/view.err" "$tmp/list.err" 2>/dev/null | sed 's/^/  /'
+				echo "  failed: gh release view --repo $REPO --json tagName, then gh api repos/$REPO/releases"
+				echo "  a renamed or deleted repository needs REPO fixed in $up; an outage clears on a later run"
+			} >&2
+			exit 1
+		fi
+		[ -n "$latest_tag" ] || { echo "$name: gh release view answered an empty tag for $REPO" >&2; exit 1; }
 
 		# Versions, for what reads a version rather than a tag: the major-bump
 		# refusal, an `apk` shape's artifact names, and anyone reading the branch.
@@ -277,9 +320,6 @@ for up in packages/*/upstream.sh; do
 		fi
 		echo "$name: $current -> $latest"
 
-		tmp="$(mktemp -d)"
-		trap 'rm -rf "$tmp"' EXIT
-
 		# Only what this shape needs to recompute its pins. A manifest package pins
 		# no checksums at all -- they are in the manifest, under the author's
 		# signature -- so downloading its ninety-odd assets on every run to look at
@@ -314,8 +354,12 @@ for up in packages/*/upstream.sh; do
 		apk)
 			file="$(echo "$ARTIFACT" | sed "s/${current}/${latest}/g")"
 			[ -f "$tmp/$file" ] || { echo "$name: $latest_tag publishes no $file" >&2; exit 1; }
+			# The sum is taken into a variable before it goes near `sed`. Inside the
+			# sed argument a failed `sha256sum` is invisible -- the command's status
+			# is sed's -- and the pin is committed as an empty checksum.
+			sum="$(sha256sum "$tmp/$file")"
 			sed -i "s|^ARTIFACT=.*|ARTIFACT=\"${file}\"|" "$up"
-			sed -i "s|^SHA256=.*|SHA256=\"$(sha256sum "$tmp/$file" | cut -d' ' -f1)\"|" "$up"
+			sed -i "s|^SHA256=.*|SHA256=\"${sum%% *}\"|" "$up"
 
 			# The 24.10 container, when upstream ships one. Leaving it pinned to the
 			# previous version does not fail here -- it fails later, when fetch.sh asks
@@ -324,8 +368,9 @@ for up in packages/*/upstream.sh; do
 			if [ -n "${ARTIFACT_IPK:-}" ]; then
 				file_ipk="$(echo "$ARTIFACT_IPK" | sed "s/${current}/${latest}/g")"
 				[ -f "$tmp/$file_ipk" ] || { echo "$name: $latest_tag publishes no $file_ipk" >&2; exit 1; }
+				sum="$(sha256sum "$tmp/$file_ipk")"
 				sed -i "s|^ARTIFACT_IPK=.*|ARTIFACT_IPK=\"${file_ipk}\"|" "$up"
-				sed -i "s|^SHA256_IPK=.*|SHA256_IPK=\"$(sha256sum "$tmp/$file_ipk" | cut -d' ' -f1)\"|" "$up"
+				sed -i "s|^SHA256_IPK=.*|SHA256_IPK=\"${sum%% *}\"|" "$up"
 			fi
 			;;
 		binaries)
@@ -335,13 +380,24 @@ for up in packages/*/upstream.sh; do
 			echo "$ARTIFACTS" | while read -r artifact _ arches; do
 				[ -n "$artifact" ] || continue
 				[ -f "$tmp/$artifact" ] || { echo "$name: $latest_tag publishes no $artifact" >&2; exit 1; }
-				printf '%s  %s  %s\n' "$artifact" "$(sha256sum "$tmp/$artifact" | cut -d' ' -f1)" "$arches"
+				sum="$(sha256sum "$tmp/$artifact")"
+				printf '%s  %s  %s\n' "$artifact" "${sum%% *}" "$arches"
 			done > "$tmp/table"
+			# `|| exit 1`, not `&& mv`. The left side of `&&` is exempt from errexit,
+			# so a failing awk -- BSD awk refuses the newlines in this `-v` -- left
+			# the package running with VERSION and TAG already rewritten above and
+			# the old checksums still in place, and that half-pin was committed,
+			# pushed and sent to the checks.
 			awk -v table="$(cat "$tmp/table")" '
 				/^ARTIFACTS="/ { print; print table; inside = 1; next }
 				inside && /^"/ { print; inside = 0; next }
 				!inside        { print }
-			' "$up" > "$tmp/new" && mv "$tmp/new" "$up"
+			' "$up" > "$tmp/new" || {
+				echo "$name: could not rewrite the checksum table in $up; nothing is committed" >&2
+				echo "  failed: awk -v table=... $up (its own error is above)" >&2
+				exit 1
+			}
+			mv "$tmp/new" "$up"
 			;;
 		esac
 

tools/land-updates.sh

@@ -100,7 +100,14 @@ superseded() {
 
 	_base="$(git merge-base "refs/remotes/origin/main" "$_sha" 2>/dev/null || true)"
 	[ -n "$_base" ] || return 0
-	_own="$(git diff --name-only "$_base..$_sha")"
+	# Checked by hand, because errexit is off in here: this runs inside `if` and
+	# inside `land || echo`. A failed diff prints nothing, and nothing is the answer
+	# one line down that deletes the branch. Said on stderr so stdout stays empty,
+	# which the caller reads as "keep".
+	if ! _own="$(git diff --name-only "$_base..$_sha")"; then
+		echo "cannot tell what $_sha changes: 'git diff --name-only $_base..$_sha' failed; keeping it" >&2
+		return 0
+	fi
 	[ -n "$_own" ] || { echo "it adds nothing on top of where it left main"; return 0; }
 
 	# One `git diff` per path, not one call with the whole list: an unquoted list
@@ -136,10 +143,11 @@ superseded() {
 	echo "it proposes $_theirs, behind the $_ours main publishes"
 }
 
-# Land one branch, or say why not and return. Never fails the run: one branch that
-# cannot land must not stop the next one, and none of the reasons below is an error
-# in the first place -- a check still running, a `main` that moved, a human's branch
-# that is none of this script's business.
+# Land one branch, or say why not and return. Returns 0 for every ordinary reason not
+# to land -- a check still running, a `main` that moved, a pull request waiting for a
+# person, a branch already spent -- because none of those is an error. Returns 1 only
+# when a step could not run; the loop at the bottom carries on to the next branch and
+# makes the run red at the end.
 land() {
 	branch="$1"
 	sha="$2"
@@ -152,7 +160,18 @@ land() {
 	# Captured, never piped into a test: a pipeline reports its last command, so a
 	# failing `gh pr list` reaching `grep` reads as "no pull request is open" --
 	# the one answer that makes this script push.
-	pr="$(gh pr list -R "$SELF" --head "$branch" --state open --json number -q '.[0].number')"
+	#
+	# And its status checked right here, because capturing is not enough on its
+	# own. `land` is the left side of `|| echo` below, and POSIX turns errexit off
+	# for every command in it: a failed call left `pr` empty and the function
+	# carried on to push a branch whose pull request was waiting for a person.
+	# Reproduced with a stub `gh` that exits 1 -- tools/test-land-updates.sh.
+	if ! pr="$(gh pr list -R "$SELF" --head "$branch" --state open --json number -q '.[0].number')"; then
+		echo "$branch: could not read its pull requests, so it is not landed this run"
+		echo "  failed: gh pr list -R $SELF --head $branch --state open"
+		echo "  a branch waiting for a person looks like any other until this answers; the next run asks again"
+		return 1
+	fi
 	if [ -n "$pr" ]; then
 		echo "$branch: pull request #$pr is open; a person merges that one"
 		return 0
@@ -175,8 +194,15 @@ land() {
 	# What the required contexts say about THIS commit. Check runs bind to a
 	# commit rather than to an event, so the run `check-updates.sh` dispatched on
 	# the branch reports against the same sha that is about to be pushed.
-	checks="$(gh api "repos/$SELF/commits/$sha/check-runs?per_page=100" \
-		-q '.check_runs[] | "\(.status)/\(.conclusion // "pending")\t\(.name)"')"
+	# A failed read already falls the safe way -- no runs seen is "no run" -- but it
+	# would say so as "not green yet", which sends whoever reads the log to the
+	# checks instead of to the API call that failed.
+	if ! checks="$(gh api "repos/$SELF/commits/$sha/check-runs?per_page=100" \
+		-q '.check_runs[] | "\(.status)/\(.conclusion // "pending")\t\(.name)"')"; then
+		echo "$branch: could not read its check runs, so it is not landed this run"
+		echo "  failed: gh api repos/$SELF/commits/$sha/check-runs"
+		return 1
+	fi
 
 	# Every run of a required name has to be completed and successful, not just
 	# the newest one. A cancelled run stays on the commit and GitHub has been
@@ -210,8 +236,21 @@ land() {
 	# `main` is read per branch, not once per run: an earlier branch in this same
 	# loop may already have landed, and the fast-forward test below has to be
 	# against where `main` is now rather than where it was when the job started.
-	git fetch -q origin "+refs/heads/main:refs/remotes/origin/main"
-	git fetch -q origin "+refs/heads/$branch:refs/remotes/origin/$branch"
+	#
+	# Each fetch checked by hand (errexit is off in here, see `gh pr list` above). A
+	# failed fetch leaves the refs where the last one put them, and every test below
+	# -- the branch did not move, `main` is its ancestor, the paths it adds -- would
+	# then be answered about a repository that may no longer exist.
+	if ! git fetch -q origin "+refs/heads/main:refs/remotes/origin/main"; then
+		echo "$branch: could not fetch main, so it is not landed this run"
+		echo "  failed: git fetch origin +refs/heads/main:refs/remotes/origin/main"
+		return 1
+	fi
+	if ! git fetch -q origin "+refs/heads/$branch:refs/remotes/origin/$branch"; then
+		echo "$branch: could not fetch the branch, so it is not landed this run"
+		echo "  failed: git fetch origin +refs/heads/$branch:refs/remotes/origin/$branch"
+		return 1
+	fi
 	head="$(git rev-parse "refs/remotes/origin/$branch")"
 	if [ "$head" != "$sha" ]; then
 		# The branch moved between the listing and the fetch. The green contexts
@@ -256,7 +295,11 @@ land() {
 	# deletes the branch. Kept as a guard rather than as a branch of logic: an
 	# empty `files` would make the pattern check below vacuously true, and "no
 	# paths to object to" must never read as "allowed".
-	files="$(git diff --name-only "refs/remotes/origin/main..$sha")"
+	if ! files="$(git diff --name-only "refs/remotes/origin/main..$sha")"; then
+		echo "$branch: could not list the paths it changes, so it is not landed this run"
+		echo "  failed: git diff --name-only refs/remotes/origin/main..$sha"
+		return 1
+	fi
 	if [ -z "$files" ]; then
 		echo "$branch: nothing to land against main"
 		return 0
@@ -296,12 +339,38 @@ if [ -z "$refs" ]; then
 	exit 0
 fi
 
-# `|| echo` on the call, not `set +e` around the loop: an unexpected failure inside
+# `if ! land` on the call, not `set +e` around the loop: an unexpected failure inside
 # `land` -- a `gh` outage, a git object that is not there -- must cost that one
 # branch and not every branch after it. Under plain `set -eu` the first one would
 # take the job down with the rest unread, which is the failure mode this repository
 # keeps re-learning (see the `checkout -` note in check-updates.sh).
-printf '%s\n' "$refs" | while read -r sha ref; do
+#
+# The price: POSIX turns errexit off for everything `land` runs when it is an `if`
+# condition, so nothing inside it stops on its own. Every step there whose failure
+# would read as a harmless answer checks its own status and returns 1 -- keep it
+# that way when adding one.
+#
+# A here-document rather than `printf | while`, so the loop runs in this shell and
+# `failed` survives it; a pipeline would run the loop in a subshell and lose it.
+# `land` reads nothing from stdin, and `</dev/null` keeps any `gh` or `git` in it
+# from swallowing the rest of the branch list.
+failed=""
+while read -r sha ref; do
 	branch="${ref#refs/heads/}"
-	land "$branch" "$sha" || echo "$branch: not landed this run (the step above failed)"
-done
+	if ! land "$branch" "$sha" </dev/null; then
+		echo "$branch: not landed this run (the step above failed)"
+		failed="$failed $branch"
+	fi
+done <<EOF
+$refs
+EOF
+
+# Red when a step failed, after every branch has been read -- the same shape as the
+# end of check-updates.sh. Green here used to mean "nothing failed" and "a `gh`
+# outage stopped every branch" alike, and a scheduled run nobody watches only gets
+# looked at when it is red.
+if [ -n "$failed" ]; then
+	echo "not landed because a step failed:$failed" >&2
+	echo "  every other branch was still read; the lines above name the command that failed" >&2
+	exit 1
+fi