Skip to content

Refuse to read package metadata as empty when it exists but cannot be opened - #323

Open
ryanrhughes wants to merge 1 commit into
masterfrom
unreadable-metadata-fails-closed
Open

Refuse to read package metadata as empty when it exists but cannot be opened#323
ryanrhughes wants to merge 1 commit into
masterfrom
unreadable-metadata-fails-closed

Conversation

@ryanrhughes

Copy link
Copy Markdown
Collaborator

Follow-up to #320. rc and stable ran a release for dropbox-cli every five minutes tonight, built nothing, and requeued it on the next tick: 34 "nothing to publish" reports in two hours.

Cause

On the release host, 42 pkgbuilds/*/.omarchy/package.json files were mode 0600 root:root (all dated Aug 13; git tracks them as 100644, a fresh clone gets 0644, so this was local damage on the host). The two halves of the pipeline read that file as different users:

Step Runs as release_ring seen Decision
check-versions root fast rc/stable are behind edge (2.1 vs 2): queue it
build container uid 1000 unreadable, so empty not fast-ring: skip

package_metadata_value saw the file exist, jq failed to open it, and the function returned its default. Nothing distinguished "unreadable" from "no ring". The release finished with nothing to publish, auto-release treated that as success and cleared the queue, and the next version check refilled it. 11 of the 42 files belonged to fast-ring packages, so the same loop would recur for any of them once edge moved ahead.

The host is repaired (chmod 644; both channels published dropbox-cli 2.1 on the next tick). This PR stops the tooling from ever hiding that class of failure again.

Changes

  • package_require_readable_metadata in helpers/package-metadata.sh: metadata that exists but cannot be read aborts with status 2 and a message naming the file and the fix. Every reader in the helper goes through it, including package_is_fast_ring at the call site, since its ring lookup runs inside a command substitution where an abort would only end the subshell.
  • The package enumerators (packages_for_unscoped_build, packages_for_mirror, packages_for_aur_sync, packages_for_upstream_sync, packages_for_rebuild_sync) and package_dirs materialize their input into a variable instead of producer | while, so the abort propagates instead of yielding a silently shorter list.
  • check-versions exits without touching any queue when enumeration fails; build/build.sh refuses to build a partial list. Both print the file that could not be read.
  • sync-rebuilds --self-test covers the unreadable case: ring lookup, mirror decision, and enumeration all abort with status 2, the message names the file, and a readable file reads normally again. It skips itself when run as root, where every file is readable.

Verification

  • All four self-tests pass.
  • Sandbox check-versions against today's edge database: readable metadata behaves as before; a 0000 dropbox-cli metadata file aborts with the message and writes no queue files.
  • Sandbox build/build.sh --dry-run for rc: the unreadable file aborts with "refusing to build a partial list"; readable proceeds to dropbox-cli.

…not be opened

The version check runs as root; the build container runs as an
unprivileged user. Forty-two .omarchy/package.json files on the release
host were mode 0600, so root read release_ring=fast and queued dropbox-cli
for rc and stable while the container, unable to open the same file,
read no ring and skipped it. Each release ran with nothing to publish,
auto-release counted that as success and cleared the queue, and the next
version check wrote it straight back — 34 empty runs in two hours, one
chat report per run.

package_require_readable_metadata aborts with status 2 and a message
naming the file whenever metadata exists but is unreadable, and every
reader in package-metadata.sh goes through it. The package enumerators
materialize package_dirs into a variable instead of piping it into a
loop, so that abort reaches the caller rather than ending a subshell and
leaving a silently shorter list. check-versions exits without touching
the queue, and build.sh refuses to build a partial list, when
enumeration fails. A self-test covers the unreadable case end to end.
@omarchybot

Copy link
Copy Markdown
Collaborator

Reviewed at head 4cee5d9. The diagnosis is right and the two call sites you fixed are fixed: check-versions and build/build.sh now abort instead of acting on a short list. What is not right yet is that four other consumers of the same enumerators were left on the old pattern, and for one of them this PR makes the failure quieter than master.

Verified on a worker VM as uid 1000, so the guard is live, against a three-package tree with the middle package's package.json at mode 000:

call site master this PR
bin/sync-rebuilds:523mapfile -t packages < <(package_dirs) 3 packages, check_metadata_readable catches it, FAILED=1, exit 1 1 package, the damaged one and the one after it never seen, FAILED=0, "Rebuild trigger sync complete!", exit 0
bin/sync-aur:418done < <(packages_for_aur_sync) 2 packages synced 0 packages synced, "Sync complete!", exit 0
bin/sync-upstream:996done < <(packages_for_upstream_sync) same shape same shape: zero work, success message, exit 0
bin/list-packages:215done < <(package_dirs) 3 rows 1 row, exit 0
bin/check-versions:195 status 0, 3 candidates status 2, no queue written

sync-rebuilds is the one that goes backwards. Master's check_metadata_readable (bin/sync-rebuilds:329) counts an unreadable package as FAILED and makes the run exit non-zero; package_dirs aborting inside mapfile now pre-empts that check entirely, so the damaged package and every package sorted after it vanish and the run reports success. packages_for_aur_sync and its siblings do propagate status 2 correctly — it is done < <(...) at the consumer that throws it away, so the enumerator emits nothing and the script reads an empty list as "no work". The guard's message is on stderr in every case, so a person reading the log sees it; automation sees exit 0.

Two conditions the guard does not reach, both the same silently-wrong class the PR is about. A zero-byte or whitespace-only package.json passes [[ -r ]], and jq -r '.release_ring // $default' on empty input prints nothing at all and exits 0 — so package_release_ring returns empty, the package reads as not-fast-ring, and you have the dropbox-cli divergence again by a different route. validate_package_metadata does not catch that one either, because jq empty also exits 0 on a zero-byte file. And if .omarchy/ itself is unreadable rather than the file inside it, [[ -e "$metadata" ]] is false, the guard returns 0, and the package silently disappears from the queue: mode-000 .omarchy gives the identical two-package output on master and on this branch.

The new self-test does not run in CI. .github/workflows/test.yml runs the suite as docker run archlinux:base-devel with no --user, so id -u is 0 and the whole block at bin/sync-rebuilds:784 takes the skip: running as root branch. It passes when run as a normal user — all four self-tests are green on a worker at uid 1000, including the five new assertions — but nothing in CI would catch a regression in it. A runuser -u nobody -- around that one invocation would close it.

What I checked and found clean: all 131 package directories have a PKGBUILD and a readable 0644 package.json, and the enumerators return identical lists and status 0 on master and on this branch for every channel and both architectures (edge 127/97, rc 43/31, stable 43/31; aur-sync 69, upstream-sync 24, rebuild-sync 5). Nothing in the repository newly aborts. Metadata that is absent, whitespace-only, a directory, or a dangling symlink behaves exactly as it does today. The while ... done <<<"$dirs" loops return 0 on a healthy tree, including an empty list and a last package that fails its filter, so neither check-versions nor build.sh aborts spuriously. And the failure you were aiming at is loud: build/build.sh exiting 1 reaches bin/release:143 notify_error "Release failed: Build step failed", and bin/auto-release then records the failure, retains the queue and backs off, rather than sending another of the 34 "nothing to publish" notices.

Reviewed by Claude Opus 5 in Claude Code and independently by Codex at xhigh reasoning. Codex reached the same four consumer findings and the same severity ordering; its independence is not currently guaranteed, so read that as agreement rather than confirmation. What it contributed that I had not reasoned about is the unreadable-.omarchy-directory case above, which I then verified on the worker.

Waiting on you for the four unfixed call sites. maintainerCanModify is off on this pull request, so I could not push a fix to the branch. None of this makes the fix you did write wrong on the path it covers.

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