From 7a19ff84f0cdfc0660204166b6a8804df2f657da Mon Sep 17 00:00:00 2001 From: August Cayzer Date: Sat, 5 Sep 2026 21:47:19 +0100 Subject: [PATCH] fix: two inert defects, before something copies them RUNPOOL_SETTLE_SECS was defined outside the snapshot-source-restore block, so a value in the config file beat one in the environment, the opposite of every other setting and of what the documentation says. Nothing outside scheduler.sh reads it and nobody sets it twice, so this never bit. It is fixed because it is the nearest neighbour of any setting added next: designing the stuck-queue guard, the obvious move was to copy it. The health check did not consult the reconfiguration lock, so a check landing while registrations were being rewritten could call a deliberate operation an outage, at critical. Unreachable for an org pool, whose registration count spans the whole organisation, and now unreachable full stop. Both cases fail without their fix. Closes #68 Closes #69 --- bin/runpool | 2 +- lib/common.sh | 8 ++-- lib/notify.sh | 5 +++ tests/pool-settling-window.sh | 71 +++++++++++++++++++++++++++++++++++ 4 files changed, 82 insertions(+), 4 deletions(-) diff --git a/bin/runpool b/bin/runpool index d1d595f..d5d355e 100755 --- a/bin/runpool +++ b/bin/runpool @@ -43,7 +43,7 @@ export RUNPOOL_INVOKED # The released version, and the only place it is written. The Homebrew formula # builds from a git tag, so a tag without a matching bump here ships a binary # that misreports itself. -RUNPOOL_VERSION="0.12.2" +RUNPOOL_VERSION="0.12.3" # shellcheck source=lib/common.sh . "${RUNPOOL_ROOT}/lib/common.sh" diff --git a/lib/common.sh b/lib/common.sh index e8e4a53..5a82089 100644 --- a/lib/common.sh +++ b/lib/common.sh @@ -24,6 +24,7 @@ _rp_env_LOG_DIR="${RUNPOOL_LOG_DIR:-}" _rp_env_LABEL_NS="${RUNPOOL_LABEL_NS:-}" _rp_env_IDLE_SECS="${RUNPOOL_IDLE_SECS:-}" _rp_env_STUCK_WAKES="${RUNPOOL_STUCK_WAKES:-}" +_rp_env_SETTLE_SECS="${RUNPOOL_SETTLE_SECS:-}" _rp_env_LOAD_WARN="${RUNPOOL_LOAD_WARN:-}" _rp_env_NOTIFY_CMD="${RUNPOOL_NOTIFY_CMD:-}" _rp_env_JOB_HOOK="${RUNPOOL_JOB_HOOK:-}" @@ -48,6 +49,7 @@ set +a [ -n "${_rp_env_LABEL_NS}" ] && RUNPOOL_LABEL_NS="${_rp_env_LABEL_NS}" [ -n "${_rp_env_IDLE_SECS}" ] && RUNPOOL_IDLE_SECS="${_rp_env_IDLE_SECS}" [ -n "${_rp_env_STUCK_WAKES}" ] && RUNPOOL_STUCK_WAKES="${_rp_env_STUCK_WAKES}" +[ -n "${_rp_env_SETTLE_SECS}" ] && RUNPOOL_SETTLE_SECS="${_rp_env_SETTLE_SECS}" [ -n "${_rp_env_LOAD_WARN}" ] && RUNPOOL_LOAD_WARN="${_rp_env_LOAD_WARN}" [ -n "${_rp_env_NOTIFY_CMD}" ] && RUNPOOL_NOTIFY_CMD="${_rp_env_NOTIFY_CMD}" [ -n "${_rp_env_JOB_HOOK}" ] && RUNPOOL_JOB_HOOK="${_rp_env_JOB_HOOK}" @@ -55,13 +57,13 @@ set +a [ -n "${_rp_env_TELEMETRY}" ] && RUNPOOL_TELEMETRY="${_rp_env_TELEMETRY}" [ -n "${_rp_env_DRAIN_TIMEOUT}" ] && RUNPOOL_DRAIN_TIMEOUT="${_rp_env_DRAIN_TIMEOUT}" unset _rp_env_BASE _rp_env_CACHE_DIR _rp_env_POOLS_FILE _rp_env_LOG_DIR _rp_env_LABEL_NS \ - _rp_env_IDLE_SECS _rp_env_STUCK_WAKES _rp_env_LOAD_WARN _rp_env_NOTIFY_CMD _rp_env_JOB_HOOK \ + _rp_env_IDLE_SECS _rp_env_STUCK_WAKES _rp_env_SETTLE_SECS _rp_env_LOAD_WARN _rp_env_NOTIFY_CMD _rp_env_JOB_HOOK \ _rp_env_HOOK_DIR _rp_env_TELEMETRY _rp_env_DRAIN_TIMEOUT # Restored values need exporting again: the restore above is a plain assignment # and happens after 'set -a' was turned off. export RUNPOOL_BASE RUNPOOL_CACHE_DIR RUNPOOL_LOG_DIR RUNPOOL_LABEL_NS RUNPOOL_IDLE_SECS \ - RUNPOOL_STUCK_WAKES \ + RUNPOOL_STUCK_WAKES RUNPOOL_SETTLE_SECS \ RUNPOOL_LOAD_WARN RUNPOOL_NOTIFY_CMD RUNPOOL_JOB_HOOK RUNPOOL_HOOK_DIR RUNPOOL_TELEMETRY \ RUNPOOL_DRAIN_TIMEOUT \ RUNPOOL_CONFIG RUNPOOL_POOLS_FILE @@ -136,7 +138,7 @@ RUNPOOL_IDLE_SECS="${RUNPOOL_IDLE_SECS:-1200}" # launchd starts it, and until that lands the pool looks exactly like a broken # one: agents up locally, nothing online at GitHub. Judging a pool inside that # window reports every healthy start as an outage. -RUNPOOL_SETTLE_SECS="${RUNPOOL_SETTLE_SECS:-120}" +RUNPOOL_SETTLE_SECS="${RUNPOOL_SETTLE_SECS:-120}" # and in the precedence block above # How many fruitless wake cycles a queued run is given before it stops counting # as work. A run can sit `queued` forever with no jobs ever attached, and diff --git a/lib/notify.sh b/lib/notify.sh index 9e96a17..2661360 100644 --- a/lib/notify.sh +++ b/lib/notify.sh @@ -81,6 +81,11 @@ _rp_health_check() { echo "${now}" >| "${RUNPOOL_HEALTH_STATE}" for p in $(_rp_pool_names); do _rp_load_pool "${p}" || continue + # A pool mid-resize, mid-drain or mid-rename has its registrations being + # rewritten, so a check landing in that window can see none and call a + # deliberate operation an outage. Whoever holds the lock already knows. + # The same predicate autoscale and `up` use. + _rp_resize_locked_by_other "${p}" && continue gh="$(_rp_gh_runners)"; reg="${gh% *}"; online="${gh#* }" running="$(_rp_running_in "${p}" "${POOL_COUNT}")" settling=0; _rp_pool_settling "${p}" && settling=1 diff --git a/tests/pool-settling-window.sh b/tests/pool-settling-window.sh index 8832ae5..2c32a89 100755 --- a/tests/pool-settling-window.sh +++ b/tests/pool-settling-window.sh @@ -90,4 +90,75 @@ echo "not-a-number" >| "$(_rp_pool_started_flag acme)" _rp_pool_settling acme && fail "an unreadable stamp is not settling" pass=$(( pass + 1 )) +# --- a pool being reconfigured is not judged -------------------------------- +# set-count, reregister and rename all delete registrations and create them +# again. A check landing in that window can see none and report a deliberate +# operation as an outage, at critical, to whoever holds the lock and already +# knows. Asserted by call count, because the point is that GitHub is not asked +# at all rather than asked and forgiven. +mkdir -p "${RUNPOOL_BASE}/pools" "${scratch_dir}/bin" +cat >"${RUNPOOL_BASE}/pools/alpha.conf" <"${scratch_dir}/bin/gh" <> "${scratch_dir}/gh-calls" +echo "0 0" +STUB +chmod +x "${scratch_dir}/bin/gh" +export PATH="${scratch_dir}/bin:${PATH}" +gh_calls() { awk 'NF { n++ } END { print n + 0 }' "${scratch_dir}/gh-calls" 2>/dev/null || echo 0; } + +# shellcheck source=/dev/null +. "${repo_dir}/lib/notify.sh" +: >"${scratch_dir}/gh-calls" + +# A lock held by a live process that is not us, which is what +# _rp_resize_locked_by_other tests for. It has to be a real running pid: $$ +# reads as our own lock and is deliberately ignored, and a pid we cannot +# signal reads as a dead holder, which is equally deliberate. +sleep 60 & +holder=$! +mkdir -p "$(_rp_resize_lock_dir alpha)" +echo "${holder}" >"$(_rp_resize_lock_dir alpha)/pid" +rm -f "${RUNPOOL_HEALTH_STATE}" +_rp_health_check >/dev/null 2>&1 +check "a pool mid-reconfiguration is not asked about" "0" "$(gh_calls)" + +kill "${holder}" 2>/dev/null; wait "${holder}" 2>/dev/null +rm -rf "$(_rp_resize_lock_dir alpha)" +rm -f "${RUNPOOL_HEALTH_STATE}" +_rp_health_check >/dev/null 2>&1 +[ "$(gh_calls)" -gt 0 ] || fail "an unlocked pool should still be judged" +pass=$(( pass + 1 )) + +# --- where the window's own setting comes from ------------------------------ +# Precedence is environment, then config file, then default, and every setting +# has to be threaded through the snapshot-source-restore block to get it. This +# one was defined outside that block for a long time, which inverted it: a +# value in the config beat one in the environment, the opposite of every other +# setting and of what the documentation says. +# +# Each case is a subshell that sources the library fresh, because precedence is +# decided once at load time. +setting_in() { + ( export RUNPOOL_BASE RUNPOOL_CACHE_DIR RUNPOOL_POOLS_FILE RUNPOOL_LOG_DIR RUNPOOL_LOG + export RUNPOOL_CONFIG="$1" + if [ -n "${2:-}" ]; then export RUNPOOL_SETTLE_SECS="$2"; else unset RUNPOOL_SETTLE_SECS; fi + # shellcheck source=/dev/null + . "${repo_dir}/lib/common.sh" 2>/dev/null || true + echo "${RUNPOOL_SETTLE_SECS}" ) +} +conf="${scratch_dir}/precedence.conf" +printf 'RUNPOOL_SETTLE_SECS=222\n' >"${conf}" + +check "the default applies with neither set" "120" "$(setting_in /dev/null "")" +check "the config file beats the default" "222" "$(setting_in "${conf}" "")" +check "the environment beats the config file" "333" "$(setting_in "${conf}" 333)" +check "and the environment beats the default" "333" "$(setting_in /dev/null 333)" + echo "ok: ${pass} case(s)"