From 3701a483af00ae29b8c443d4c5883799ff74d2d2 Mon Sep 17 00:00:00 2001 From: rob thijssen Date: Fri, 7 Aug 2026 11:57:44 +0300 Subject: [PATCH] fix: take the build verdict from COPR, not from watch-build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `copr-cli watch-build` holds a long-lived connection to COPR and can stop responding while the build carries on and finishes normally. The script treated its exit status as the build's outcome, so a watcher that lost its connection was reported as a failed build. Seen in monsoon run 48: COPR build 10835047 succeeded at 08:30:30, the watcher went silent after 08:09:11, and the runner killed the step at 08:35:50. The job was marked failed even though the RPM built and published fine, and that false failure also blocked the dependent version-bump job from running at all. Keep watch-build for its progress stream, but move it to the background, bound it with a timeout, and ignore its exit code. Poll `copr-cli status` for the authoritative state and derive the exit code from that. Polling also means the script returns as soon as the build settles instead of waiting on the watcher to notice. succeeded and skipped (already built) pass; failed and canceled fail; and never reaching a terminal state within COPR_BUILD_TIMEOUT is reported as a timeout rather than silently as a build failure. Adds tests/test-copr-build.sh, which drives the script against a stub copr-cli. The regression case — hung watcher, successful build — fails against the previous script by hanging until killed, which is the production symptom. --- CLAUDE.md | 18 +++++++ README.md | 13 ++++- scripts/copr-build.sh | 87 +++++++++++++++++++++++++++++---- tests/test-copr-build.sh | 101 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 210 insertions(+), 9 deletions(-) create mode 100755 tests/test-copr-build.sh diff --git a/CLAUDE.md b/CLAUDE.md index c668bdf..4ecb533 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -15,6 +15,24 @@ from a consumer repo (primarily `helexa/cortex`). When debugging, fetch job logs via the `gitea-mcp` tools against the consumer repo, not this one — e.g. `mcp__gitea-mcp__actions_run_read` with `owner=helexa`, `repo=cortex`. +`tests/test-copr-build.sh` covers the parts that don't need a real COPR: it +puts a stub `copr-cli` on `PATH` and drives `scripts/copr-build.sh` through +succeeded / failed / canceled / skipped / never-settles, plus the case where +`watch-build` hangs. Run it directly (`./tests/test-copr-build.sh`); it needs +nothing but bash and takes a few seconds. + +## Why the verdict does not come from `watch-build` + +`copr-cli watch-build` holds a long-lived connection and can stop responding +while the build carries on and finishes normally. Observed in `monsoon` run 48: +COPR build 10835047 succeeded at 08:30:30, the watcher went silent after +08:09:11, and the runner killed the step at 08:35:50 — reported as a failed +build, which also blocked the dependent version-bump job. + +So the script polls `copr-cli status` for the authoritative state and treats +`watch-build` purely as a progress stream whose exit code is ignored. If you +are tempted to simplify this back into `if copr-cli watch-build; then`, don't. + ## Tagging & release workflow We use a **floating major tag** alongside specific semver tags: diff --git a/README.md b/README.md index 5a50e6e..4947855 100644 --- a/README.md +++ b/README.md @@ -13,10 +13,21 @@ This action: - Submits with `--nowait` and captures the build ID. - Prints a clickable `https://copr.fedorainfracloud.org/coprs/build/...` link so you can follow live. -- Watches the build to completion (blocks, propagates exit status). +- Streams status transitions from `copr-cli watch-build` while polling COPR for + the build's actual state, so the job's verdict comes from the build rather + than from the health of a long-lived connection. - On completion, fetches each chroot's `builder-live.log` via `copr-cli download-build` and emits them as `::group::` blocks. - Fails CI if the build fails, but always dumps logs first. +- Gives up after `COPR_BUILD_TIMEOUT` (default 7200s) and says so, rather than + hanging until the runner kills the job. + +### Environment overrides + +| Variable | Default | Purpose | +|---|---|---| +| `COPR_BUILD_TIMEOUT` | `7200` | Seconds to wait for a build to reach a terminal state. | +| `COPR_POLL_INTERVAL` | `30` | Seconds between build-state polls. | ## Requirements diff --git a/scripts/copr-build.sh b/scripts/copr-build.sh index 0c36171..7d15fea 100755 --- a/scripts/copr-build.sh +++ b/scripts/copr-build.sh @@ -9,6 +9,11 @@ set -o pipefail +# How long to wait for a build to reach a terminal state before giving up, and +# how often to ask COPR where it has got to. +BUILD_TIMEOUT="${COPR_BUILD_TIMEOUT:-7200}" +POLL_INTERVAL="${COPR_POLL_INTERVAL:-30}" + PROJECT="$1" shift @@ -17,6 +22,37 @@ if [ -z "$PROJECT" ] || [ "$#" -eq 0 ]; then exit 2 fi +# COPR states that mean the build has finished, one way or another. Anything +# else (pending, starting, running, importing, waiting) is still in progress. +is_terminal_state() { + case "$1" in + succeeded | failed | canceled | skipped) return 0 ;; + *) return 1 ;; + esac +} + +build_state() { + copr-cli status "$1" 2>/dev/null | tail -n1 | tr -d '[:space:]' +} + +# Poll COPR until the build settles, or until BUILD_TIMEOUT elapses. Echoes the +# terminal state, or "unknown" if we ran out of patience. +wait_for_terminal_state() { + local build_id="$1" + local deadline=$(($(date +%s) + BUILD_TIMEOUT)) + local state + state="$(build_state "$build_id")" + while ! is_terminal_state "$state"; do + if [ "$(date +%s)" -ge "$deadline" ]; then + echo "unknown" + return + fi + sleep "$POLL_INTERVAL" + state="$(build_state "$build_id")" + done + echo "$state" +} + # Submit without waiting; capture the build ID from stdout. SUBMIT_OUT=$(copr-cli build --nowait "$PROJECT" "$@") echo "$SUBMIT_OUT" @@ -32,14 +68,49 @@ echo "Build $BUILD_ID submitted to $PROJECT" echo "Follow live: https://copr.fedorainfracloud.org/coprs/build/$BUILD_ID" echo -# Watch the build; captures status transitions to stdout. Exit non-zero -# on build failure, but defer propagating that until after we've fetched -# logs so the CI output contains diagnostics either way. -if copr-cli watch-build "$BUILD_ID"; then - STATUS=0 -else - STATUS=$? -fi +# Stream status transitions for the operator's benefit, in the background and +# bounded so it cannot hang the job. +# +# Its exit status is deliberately ignored, and it is NOT what decides the +# verdict. `watch-build` holds a long-lived connection to COPR, and it has been +# seen to stop responding while the build carries on and finishes normally: in +# one case the build succeeded at 08:30:30 while the watcher sat silent until +# the runner killed the step at 08:35:50, which the job then reported as a +# build failure. Conflating "the watcher lost its connection" with "the build +# failed" turns a green release into a red one and blocks any dependent job. +timeout "$BUILD_TIMEOUT" copr-cli watch-build "$BUILD_ID" & +WATCH_PID=$! + +# The verdict comes from COPR itself. Polling also means we stop as soon as the +# build settles, rather than waiting on the watcher to notice. +BUILD_STATE="$(wait_for_terminal_state "$BUILD_ID")" + +kill "$WATCH_PID" 2>/dev/null || true +wait "$WATCH_PID" 2>/dev/null || true + +echo +echo "COPR build $BUILD_ID final state: $BUILD_STATE" + +case "$BUILD_STATE" in + succeeded) + STATUS=0 + ;; + skipped) + # COPR already had this exact build; nothing was rebuilt, but nothing is + # wrong either. + echo "note: build $BUILD_ID was skipped (already built)" + STATUS=0 + ;; + failed | canceled) + echo "error: COPR build $BUILD_ID finished with state '$BUILD_STATE'" >&2 + STATUS=1 + ;; + *) + echo "error: gave up after ${BUILD_TIMEOUT}s waiting for COPR build $BUILD_ID to finish" >&2 + echo " check https://copr.fedorainfracloud.org/coprs/build/$BUILD_ID" >&2 + STATUS=1 + ;; +esac # Fetch per-chroot results (logs + rpms). Anonymous download — no auth needed. LOG_DIR="$(mktemp -d -t copr-logs.XXXXXX)" diff --git a/tests/test-copr-build.sh b/tests/test-copr-build.sh new file mode 100755 index 0000000..a364fd5 --- /dev/null +++ b/tests/test-copr-build.sh @@ -0,0 +1,101 @@ +#!/bin/bash +# Tests for scripts/copr-build.sh, driven by a stub `copr-cli` on PATH. +# +# The case that matters most is a watcher that hangs while the build succeeds +# anyway: that is what made a green COPR release report as a failed CI job. +# +# Usage: tests/test-copr-build.sh + +set -o pipefail + +HERE="$(cd "$(dirname "$0")" && pwd)" +SCRIPT="$HERE/../scripts/copr-build.sh" +PASS=0 +FAIL=0 + +# Build a stub copr-cli whose behaviour is driven by env vars, and put it first +# on PATH. STUB_STATES is a space-separated list of states returned by +# successive `status` calls; the last one repeats. +make_stub() { + local dir="$1" + mkdir -p "$dir" + cat > "$dir/copr-cli" <<'STUB' +#!/bin/bash +case "$1" in + build) + echo "Uploading package test.src.rpm" + echo "Created builds: 12345" + ;; + watch-build) + if [ "${STUB_WATCH_HANGS:-0}" = "1" ]; then + # Mimic a watcher that streams a little then stops responding. + echo " Build 12345: running" + sleep 3600 + else + echo " Build 12345: succeeded" + fi + ;; + status) + # Pop the next state; the final one repeats forever. + read -r -a states <<< "${STUB_STATES:-succeeded}" + idx_file="${STUB_IDX_FILE:-/tmp/stub_idx}" + idx=$(cat "$idx_file" 2>/dev/null || echo 0) + if [ "$idx" -ge "${#states[@]}" ]; then idx=$(( ${#states[@]} - 1 )); fi + echo "${states[$idx]}" + echo $(( idx + 1 )) > "$idx_file" + ;; + download-build) + exit 0 + ;; +esac +STUB + chmod +x "$dir/copr-cli" +} + +run_case() { + local name="$1" expected_rc="$2" expected_text="$3" + shift 3 + local tmp out rc + tmp="$(mktemp -d)" + make_stub "$tmp/bin" + out="$(env "$@" STUB_IDX_FILE="$tmp/idx" PATH="$tmp/bin:$PATH" \ + COPR_POLL_INTERVAL=1 COPR_BUILD_TIMEOUT="${CASE_TIMEOUT:-20}" \ + bash "$SCRIPT" owner/project test.src.rpm 2>&1)" + rc=$? + if [ "$rc" -eq "$expected_rc" ] && grep -q "$expected_text" <<< "$out"; then + echo "ok - $name" + PASS=$((PASS + 1)) + else + echo "FAIL - $name (rc=$rc want $expected_rc; wanted text: $expected_text)" + sed 's/^/ | /' <<< "$out" + FAIL=$((FAIL + 1)) + fi + rm -rf "$tmp" +} + +# The regression: watcher stops responding, build succeeded anyway. +run_case "hung watcher + succeeded build exits 0" \ + 0 "final state: succeeded" STUB_WATCH_HANGS=1 STUB_STATES="running succeeded" + +# A genuine failure must still fail, hung watcher or not. +run_case "hung watcher + failed build exits 1" \ + 1 "finished with state 'failed'" STUB_WATCH_HANGS=1 STUB_STATES="running failed" + +# Normal path. +run_case "healthy watcher + succeeded build exits 0" \ + 0 "final state: succeeded" STUB_WATCH_HANGS=0 STUB_STATES="succeeded" + +run_case "canceled build exits 1" \ + 1 "finished with state 'canceled'" STUB_WATCH_HANGS=0 STUB_STATES="canceled" + +# Already-built is not an error. +run_case "skipped build exits 0" \ + 0 "was skipped" STUB_WATCH_HANGS=0 STUB_STATES="skipped" + +# Never settles: bounded, and reported as a timeout rather than a build failure. +CASE_TIMEOUT=3 run_case "build that never settles times out" \ + 1 "gave up after" STUB_WATCH_HANGS=1 STUB_STATES="running" + +echo +echo "passed: $PASS failed: $FAIL" +[ "$FAIL" -eq 0 ]