fix(api): an unreachable worker API must not abort the run, or cry abort while succeeding (#364) - #366
Merged
Merged
Conversation
…ort 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 ways depending on the bash running the script: * Any caller that was NOT guarded aborted outright. Measured on miner-0 and the HP appliance (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 GUARDED callers printed the banner. `status` wraps its read in `( ... ) || true` and still emitted two "[ERROR] rigforge aborted while running 'status'" lines before printing its correct "worker API not reachable" line and exiting 0 — the shape reported in #364. 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. 5.2 does carry it and stays quiet, which is why this showed on dev machines and not on the rigs. 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. The watchdog's `|| true` goes with it: the reader owns the guarantee now (verified unguarded on both rigs). `set +e` inside svc_status's subshell was considered and rejected: bash's ERR trap fires independently of errexit, so it silences nothing. Coverage: 7 assertions, each verified to FAIL on the pre-fix script. The tune-side test is driven as a separate bash process on purpose — run in a subshell it inherits the suite's own errexit context, which disarms the failure under test on one bash or the other and passes vacuously. Closes #364. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
VijitSingh97
force-pushed
the
fix/status-err-trap-noise
branch
from
August 15, 2026 22:36
f6de4f7 to
07e9d7e
Compare
This was referenced Aug 15, 2026
Merged
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
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.
Closes #364.
Not just noise — it aborted runs on the rigs
#364 is filed as spurious stderr on an exit-0 path, and that is real. But tracing it turned up a
worse sibling on the same root cause, and that one bites on hardware rather than a dev laptop:
an unreachable worker API aborted
tune/autotune's sampling loop outright. A miner restartingunder a sweep took the sweep with it. Please don't read this as cosmetic.
Root cause
The API readers had a
propagatemode that let curl's exit escape, so an unreachable miner would"surface upstream". Nothing upstream ever read that status — every caller branches on an empty
body. Letting it escape then broke two ways depending on the bash running the script:
tune/autotunesampling)( ... ) || true(status)set -Einherits the ERR trap into the$( )each caller reads through. Bash 3.2 does not carrythe caller's suppressed-errexit context into that child, so the trap fires there once per frame the
failure unwinds through — hence two lines from
statuswhile it still printed its correct fallbackand exited 0. 5.2 does carry it, which is why this showed up on dev machines and not on the rigs.
The trap fires once per stack frame, not twice for one command: once for the failing
curlinside_read_api_summary, then again as_read_api_summaryreturns 7 (curl is its last command) one frameup in
_status_api_summary.Fix
propagateis gone. Both readers always return 0 with an empty body when the API is unreachable —the contract every caller already assumed — so neither failure shape is reachable. This fixes
status,tune,autotuneand the control channel's post-apply liveness wait in one place insteadof copying #210's guard to each call site; the watchdog's
|| trueis removed because the reader nowowns the guarantee.
Two alternatives considered and rejected:
set +einsidesvc_status's subshell — tested, silences nothing: bash's ERR trap firesindependently of
errexit.one place all of them route through.
Verification
Old-vs-new, API refusing connections, ERR trap live:
statusverbbash tests/run.sh: 1699 passed, 0 failed.make lint+make lint-mdclean.vacuous coverage. All 10 existing watchdog: crashes (exit 28) on an unreachable API instead of counting a strike — wedge recovery never fires in v1.5.0 #210 assertions still pass, which is what backs removing the
watchdog guard.
dirs removed, miner-0's xmrig untouched.
Note for the reviewer
The tune-side test is driven as a separate
bashprocess deliberately. Run inside a subshell itinherits the suite's own errexit context, and since 5.2 and 3.2 disagree about whether that context
crosses into a
$( ), it silently disarms the failure under test on one platform or the other. Twoearlier drafts of this test passed against the broken script for exactly that reason. Please don't
fold it back into a subshell.