diff --git a/CHANGELOG.md b/CHANGELOG.md index 707e43c..d7a8d77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,32 @@ All notable changes to RigForge are documented here. The format is based on ## [Unreleased] +### Fixed + +- **An unreachable worker API no longer aborts the run, or cries abort while succeeding (#364).** + The API readers had a "propagate" mode that let curl's exit escape, so an unreachable miner would + "surface upstream". Nothing upstream ever read it — every caller branches on an empty body — and + letting it escape broke two different ways depending on the bash running the script: + + - **Any caller that was not guarded aborted outright.** Measured on a rig (Linux, bash 5.2): with + the API refusing connections, `tune`/`autotune`'s sampling loop and a bare `_status_api_summary` + both died with `[ERROR] rigforge aborted ... (exit 7)`. An API that went away mid-sweep — the + miner restarting under you — took the sweep with it. This is the failure #210 first hit on + miner-0 and papered over with a `|| true` at one call site. + - **On bash 3.2 (macOS) even the guarded callers printed the abort banner.** `status` wraps its + read in `( ... ) || true` and still emitted two `[ERROR] rigforge aborted while running 'status'` + lines to stderr before printing its correct "worker API not reachable" line and exiting 0 — the + reported shape. `set -E` inherits the ERR trap into the `$( )` the caller reads through, and 3.2 + does not carry the caller's suppressed-errexit context into that child, so the trap fires there + once per frame the failure unwinds through. Bash 5.2 does carry it and stays quiet, which is why + this showed up on dev machines and not on the rigs. Spending the line operators are taught to + read as "something broke" on a routine, handled, exit-0 path is what made #341's real abort easy + to miss. + + The mode is gone. Both readers now always return 0 with an empty body when the API is unreachable + — the contract every caller already assumed — so neither failure shape is reachable, and #210's + guard-inside-the-`$( )` idiom is no longer needed for these readers. + ## [1.15.0] - 2026-08-15 ### Added diff --git a/rigforge.sh b/rigforge.sh index 46f17f4..04c0f40 100755 --- a/rigforge.sh +++ b/rigforge.sh @@ -3368,41 +3368,41 @@ autotune() { # (evaluated as-is — a raw hashrate for _read_api_hashrate's own check below, a full JSON body for # every other caller). Shared by tune/autotune/status and the sister API's stats superset — was two # near-identical readers, _read_api_summary + _xmrig_summary_json (#353). -# : "propagate" lets a curl failure raise under set -e — what tune/autotune/status relied on so -# an unreachable API surfaces upstream. Default "swallow" always returns 0 — what the sister API -# refresh loop and the pool-connection probe need, since an unreachable miner there is routine, not a -# crash. The API is open (read-only) with no token by default; only send a Bearer when ACCESS_TOKEN is -# set (XMRig 401s a token it never asked for, and curl -f's exit 22 would then hit the mode check -# below either way). Branches on the Bearer header rather than an empty-array curl arg, and on mode -# rather than appending to a built-up arg list — both trip set -u on bash 3.2 (macOS). -_read_api_summary() { # [propagate] +# ALWAYS returns 0 with an empty body when the API is unreachable — curl's exit never escapes. There +# was a "propagate" mode that let it, so an unreachable miner would "surface upstream"; nothing +# upstream ever read that status (every caller branches on an empty body) and it broke two ways under +# the ERR trap (#364). Unguarded callers aborted outright: on a rig (bash 5.2) an API refusing +# connections killed tune/autotune's sampling loop with exit 7 mid-sweep — the failure #210 hit on +# miner-0. And on bash 3.2 (macOS) even GUARDED callers printed the banner: set -E inherits the trap +# into the $( ) each caller reads through, and 3.2 does not carry the caller's suppressed-errexit +# context (`( ... ) || true`) into that child, so the trap fires there once per frame the failure +# unwinds — `status` emitted "[ERROR] rigforge aborted while running 'status'" TWICE for a stopped +# miner while still exiting 0. Swallowing here kills both shapes for every reader at once. The API is +# open (read-only) with no token by default; only send a Bearer when ACCESS_TOKEN is set (XMRig 401s a +# token it never asked for). Branches on the Bearer header rather than an empty-array curl arg — an +# empty array trips set -u on bash 3.2 (macOS). +_read_api_summary() { local url="http://127.0.0.1:8080/2/summary" if [ -n "${API_CMD:-}" ]; then - eval "$API_CMD" + eval "$API_CMD" || true # the override stands in for the API — a failing one means "unreachable", not "crash" return fi command -v curl >/dev/null 2>&1 || return 0 - if [ "${1:-}" = propagate ]; then - if [ -n "${ACCESS_TOKEN:-}" ]; then - curl -fsS --max-time 5 -H "Authorization: Bearer $ACCESS_TOKEN" "$url" 2>/dev/null - else - curl -fsS --max-time 5 "$url" 2>/dev/null - fi + if [ -n "${ACCESS_TOKEN:-}" ]; then + curl -fsS --max-time 5 -H "Authorization: Bearer $ACCESS_TOKEN" "$url" 2>/dev/null || true else - if [ -n "${ACCESS_TOKEN:-}" ]; then - curl -fsS --max-time 5 -H "Authorization: Bearer $ACCESS_TOKEN" "$url" 2>/dev/null || true - else - curl -fsS --max-time 5 "$url" 2>/dev/null || true - fi + curl -fsS --max-time 5 "$url" 2>/dev/null || true fi } +# Same never-fails contract as its reader: `|| true` covers the jq leg too, so a malformed body (or a +# missing jq) can't ride pipefail out and trip the ERR trap in a caller's $( ) either (#364). _read_api_hashrate() { if [ -n "${API_CMD:-}" ]; then - eval "$API_CMD" + eval "$API_CMD" || true return fi - _read_api_summary propagate | jq -r '.hashrate.total[0] // empty' 2>/dev/null + _read_api_summary | jq -r '.hashrate.total[0] // empty' 2>/dev/null || true } # Median of N live API hashrate samples, seconds apart. Smooths the jittery live reading so a @@ -3760,7 +3760,7 @@ mac_disable() { # markers (that's doctor's job); plain aligned lines stay grep-friendly. Never sudo, never prompts. _status_api_summary() { local body hs pool up acc rej hp - body=$(_read_api_summary propagate) + body=$(_read_api_summary) if [ -z "$body" ]; then echo "RigForge: worker API not reachable at 127.0.0.1:8080 (miner stopped or still starting)." return 0 @@ -4412,14 +4412,16 @@ watchdog() { warn "watchdog: temp ${t}°C is above max_temp_c=${MAX_TEMP_C}°C — miner stopped (starts again below $((MAX_TEMP_C - 5))°C)." return 0 fi - # Wedge check: the API probe returns empty (unreachable) or the live hashrate (a float). - # `|| true` INSIDE the substitution (#210): curl's nonzero exit (refused/timeout) rides the - # pipeline out of the probe via pipefail; unguarded, the assignment errexits the whole check, - # and a guard OUTSIDE the $() still lets the ERR trap fire in the subshell and spam "aborted - # while" into the journal every tick. An unreachable API is a STRIKE, not a crash. - hr=$(_read_api_hashrate || true) + # Wedge check: the API probe returns empty (unreachable) or the live hashrate (a float). An + # unreachable API is a STRIKE, not a crash — _read_api_hashrate swallows curl's refused/timeout + # exit itself (#364), so this unguarded assignment can't errexit the check (verified on a rig: + # the old reader aborted here with exit 7, the current one returns empty). #210 needed a + # `|| true` here for exactly that; the reader now owns the guarantee, so the guard is gone. + hr=$(_read_api_hashrate) if [ -z "$hr" ] || awk -v h="$hr" 'BEGIN { exit !(h == 0) }'; then - f=$(cat "$fails_f" 2>/dev/null || true) # guard inside the $() — see the probe above (#210) + # Guard INSIDE the $( ): on bash 3.2 a caller-side `|| true` does NOT stop the ERR trap + # firing in the child, because 3.2 doesn't carry suppressed errexit into it (#210/#364). + f=$(cat "$fails_f" 2>/dev/null || true) [[ "$f" =~ ^[0-9]+$ ]] || f=0 f=$((f + 1)) if [ "$f" -ge 2 ]; then diff --git a/tests/run.sh b/tests/run.sh index a946e91..4ad23a6 100644 --- a/tests/run.sh +++ b/tests/run.sh @@ -2484,20 +2484,30 @@ bash "$SCRIPT" completion >/dev/null 2>&1 || comp_rc=$? assert_rc "completion without a shell errors with usage (#145)" "$comp_rc" "1" # #353 (6): _read_api_summary + _xmrig_summary_json were near-identical readers, merged into one -# function with a mode argument. Prove the two modes actually differ: "propagate" (tune/autotune/ -# status's shape) surfaces a curl failure as a nonzero exit; the default "swallow" (the sister API's -# shape) always returns 0. Both must still yield an EMPTY body either way — only the exit code differs. -echo "== unit: _read_api_summary propagate vs swallow (#353) ==" +# function with a mode argument. #364 then deleted that mode: "propagate" let curl's exit escape so an +# unreachable API would "surface upstream", but no caller ever read the status (every one branches on +# an empty body) and letting it escape fired the inherited ERR trap inside the $( ) each caller reads +# through. Both readers now ALWAYS return 0 with an empty body — the contract the callers assume. +echo "== unit: the API readers never let a curl failure escape (#353/#364) ==" rap_out="$( ( source "$SCRIPT" unset API_CMD curl() { return 7; } # simulate an unreachable worker API set +e - printf 'default:[%s] rc=%s\n' "$(_read_api_summary)" "$?" - printf 'propagate:[%s] rc=%s\n' "$(_read_api_summary propagate)" "$?" + printf 'summary:[%s] rc=%s\n' "$(_read_api_summary)" "$?" + printf 'hashrate:[%s] rc=%s\n' "$(_read_api_hashrate)" "$?" + # NB: no apostrophes in comments inside this $( ) — bash 3.2 (macOS) opens a quote on one and + # swallows the rest of the file. The override is how the suite stands in for the worker API, and + # a FAILING one is how the watchdog strike-2 case simulates "unreachable". It must honour the + # same contract as the curl path, or the never-fails guarantee callers lean on has a hole there. + API_CMD=false + printf 'summary-cmd:[%s] rc=%s\n' "$(_read_api_summary)" "$?" + printf 'hashrate-cmd:[%s] rc=%s\n' "$(_read_api_hashrate)" "$?" ) 2>&1)" -assert_contains "default mode swallows a curl failure (rc 0, empty body)" "$rap_out" "default:[] rc=0" -assert_contains "propagate mode surfaces a curl failure (nonzero rc, empty body)" "$rap_out" "propagate:[] rc=7" +assert_contains "_read_api_summary swallows a curl failure (rc 0, empty body)" "$rap_out" "summary:[] rc=0" +assert_contains "_read_api_hashrate swallows a curl failure (rc 0, empty body)" "$rap_out" "hashrate:[] rc=0" +assert_contains "_read_api_summary swallows a failing API_CMD (rc 0, empty body)" "$rap_out" "summary-cmd:[] rc=0" +assert_contains "_read_api_hashrate swallows a failing API_CMD (rc 0, empty body)" "$rap_out" "hashrate-cmd:[] rc=0" # #143: `status` prepends a one-glance live summary from ONE /2/summary fetch — facts, no ✓/! markers, # never sudo. Unreachable API (miner stopped / http off) degrades to a single explanatory line and the @@ -2531,6 +2541,58 @@ assert_contains "status: platform block still follows (#143)" "$(cat "$ST/calls. out="$(run_status fail)" assert_contains "status: unreachable API -> one explanatory line (#143)" "$out" "worker API not reachable at 127.0.0.1:8080" assert_contains "status: platform block untouched when API is down (#143)" "$(cat "$ST/calls.log")" "[systemctl] status xmrig" + +# #364: the same unreachable API through the REAL dispatch, where errexit and the ERR trap are live. +# `run_status fail` above cannot catch this — shadowing curl inside a `set +e` subshell disarms the +# very path that produced the bug, which is how it shipped. Here curl is a real failing BINARY on +# PATH, so its exit rides out of _read_api_summary exactly as it does against a stopped miner. It +# used to print "[ERROR] rigforge aborted while running 'status'" TWICE: set -E inherits the trap +# into the $( ) _status_api_summary reads through, and bash does NOT carry svc_status's suppressed +# errexit (`( ... ) || true`) into that child, so the trap fired once per frame the failure unwound. +echo "== black-box: unreachable worker API is quiet, not an abort (#364) ==" +STE="$(mktemp -d "$SANDBOX/statuserr.XXXXXX")" +mkdir -p "$STE/bin" "$STE/home/worker" +cat >"$STE/config.json" <"$STE/bin/curl" +chmod +x "$STE/bin/curl" +out="$(cd "$STE" && PATH="$STE/bin:$STUBS:$PATH" STUB_UNAME_S=Linux CALL_LOG="$STE/calls.log" RIGFORGE_HOME="$PWD" bash "$SCRIPT" status &1)" +rc=$? +assert_rc "status: unreachable API exits 0 through real dispatch (#364)" "$rc" "0" +assert_absent "status: no ERR-trap abort on a real curl failure (#364)" "$out" "aborted while" +assert_eq "status: the unreachable line prints exactly once (#364)" \ + "$(printf '%s\n' "$out" | grep -c "worker API not reachable")" "1" +assert_contains "status: platform block still follows (#364)" "$(cat "$STE/calls.log")" "[systemctl] status xmrig" + +# The same root cause on the tune/autotune side, which reads through _read_api_hashrate — and the +# shape that bites on a RIG, not just a dev laptop: an UNGUARDED read under errexit. Verified against +# both bashes with the API refusing connections (miner-0, Linux 5.2 / this box, macOS 3.2): the old +# reader killed the run outright and printed four abort lines, the current one returns empty and the +# sampling loop runs to the end. So an API that went away mid-sweep — the miner restarting under you +# — used to take the sweep with it. +# +# Driven as a SEPARATE bash process on purpose. Running it in a subshell here would inherit this +# suite's own errexit context, and bash 5.2 carries a suppressed context into a $( ) (3.2 does not), +# which silently disarms the very failure under test on one platform or the other — a green that +# means nothing. A child process starts from a clean top-level context on both. +echo "== black-box: an unreachable API never aborts a tune sampling read (#364) ==" +cat >"$STE/drive.sh" </dev/null || true # returns 1 once it gives up; that is not a failure +echo "LOOP-DONE" +DRV +wml_out="$(cd "$STE" && PATH="$STE/bin:$STUBS:$PATH" RIGFORGE_HOME="$PWD" bash "$STE/drive.sh" 2>&1 || true)" +assert_contains "tune: an unreachable API never aborts the reader (#364)" "$wml_out" "SURVIVED hr=[]" +assert_contains "tune: the sampling loop runs to the end (#364)" "$wml_out" "LOOP-DONE" +assert_absent "tune: no ERR-trap noise while the API is down (#364)" "$wml_out" "aborted while" : >"$ST/calls.log" out="$( (