diff --git a/CHANGELOG.md b/CHANGELOG.md index 03fd15e..cc5db03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ All notable changes to RigForge are documented here. The format is based on ## [Unreleased] +### Added + +- **Control API: a restart-free fast path for `watchdog_interval_min` and `max_temp_c` (#381, from + #344 item 1).** `control-apply` used to re-run the entire `apply` pipeline for every accepted + change — regenerate XMRig's config, re-render its unit, restart the service, then poll for a live + pool connection — even for a change that never touches XMRig at all. A change whose keys are + *only* `watchdog_interval_min` and/or `max_temp_c` now reconciles just the watchdog timer and + leaves XMRig running, closing the ~62s gap the original walkthrough measured for a single-key + change. The allowlist is closed and checked as a subset match (a key not on it, including any + future addition to the control-writable set, still takes the full path), and a fast-path failure + falls back to the same full-pipeline rollback a failed restart already uses. + ## [1.15.2] - 2026-08-21 ### Added diff --git a/docs/adr/0001-writable-worker-config-control-path.md b/docs/adr/0001-writable-worker-config-control-path.md index 25d8296..5331174 100644 --- a/docs/adr/0001-writable-worker-config-control-path.md +++ b/docs/adr/0001-writable-worker-config-control-path.md @@ -96,6 +96,12 @@ D7 stamps `source: "control"` on a control apply. #254 extends the provenance to The accepted→poll contract (D2) has a race: a concurrent change between a caller's `POST` and its poll makes the no-arg `GET /status` report the *newer* change, so the caller can't confirm its own. #255 adds `GET /status?change_id=<16hex>` returning that change's recorded outcome (or `404`) — the applier already writes each outcome, so it additionally indexes them under `changes/.json` (last ~20; the id is server-generated 16-hex and re-validated before it becomes a path component). The no-arg form stays most-recent for compatibility; auth is unchanged. Chosen over a `/changes` ring-buffer endpoint (issue Option B) as the smaller, direct change. +### D12. A restart-free fast path for a closed subset of the allowlist (#381, from #344 item 1) + +D6's `apply` re-run is correct as a baseline but expensive: a live walkthrough measured a single `watchdog_interval_min` change taking ~62s round-trip through `POST /apply`, because the applier re-runs the *entire* `apply` pipeline — regenerate XMRig's config, re-render its unit, restart the service, then poll for a live pool connection — for every change, even one that never touches XMRig at all. + +The #344 discussion set one constraint before this could land: the fast path must not fork a second apply implementation that can drift from the real one. Decision: a closed allowlist (`watchdog_interval_min`, `max_temp_c`), checked as a **subset** match — every changed key must be on it, never a "not otherwise restart-requiring" complement — so a future addition to the D3 allowlist that nobody has re-proven restart-free here takes the full path by construction. Both current members are proven restart-free from the applier's own code, not asserted: `watchdog_interval_min` bakes into *only* `rigforge-watchdog.timer`'s cadence, and `max_temp_c` is never rendered into a unit at all — the watchdog verb re-reads `config.json` on every scheduled run. Neither reaches XMRig's generated config or its unit template. The fast path reuses `install_watchdog` — the same call the full `apply` pipeline already makes on every run — rather than re-implementing unit rendering, so the two paths cannot drift on what "restart-free" renders; it still stamps `config_meta` provenance (D10) the same way `apply` does, so a consumer cannot tell which path served a change from the feed alone. Success is a *run-state* comparison, not a bare is-active snapshot: the applier records whether the miner service was active *before* the fast apply and again *after*, and only a transition from active to inactive counts as failure. A rig can be legitimately stopped when a restart-free change lands — a watchdog thermal hold (D8/#257), or an operator's manual stop — and since the fast path never touches the XMRig unit or service, a stopped rig staying stopped (or even coming back on its own) is not this change's doing and must not be read as a fast-path failure; the new value still takes effect on the watchdog's next scheduled tick. An adversarial review of the first version of this decision found it gated on is-active alone, which would have discarded the operator's change and force-restarted a rig that was deliberately offline — exactly the thermal-hold-plus-`max_temp_c`-edit case this fast path exists for. A failure (active before, inactive after) falls through to the *same* full-pipeline rollback D6 already defines, never a bespoke recovery path. `pools`, `DONATION`, `autotune`, and the `watchdog` enable/disable flag stay on the full path: the first two are XMRig's own served config, and the latter two touch `install_*` behaviour this issue did not audit for restart-freedom. + ## Alternatives considered - **Write verbs on the sister API.** Rejected: violates the read-only invariant, and the `DynamicUser` read process cannot persist or apply. diff --git a/docs/configuration.md b/docs/configuration.md index b021d3a..0770f2b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -55,7 +55,7 @@ not read by `parse_config` today, so it isn't in the table below.) | `api` | `"disabled"` | `"enabled"` serves the sister API: a second **read-only** port with XMRig's `/1/summary`+`/2/summary` passed through verbatim plus a namespaced `rigforge` object (tune state, RAPL watts, firmware/health probes, pinned versions), and `/health` + `/tune` endpoints. One tiny persistent stdlib server; a systemd timer refreshes its data every 15 s, so requests never touch the miner (see [operations › sister API](operations.md)). Gated by the same `ACCESS_TOKEN`; Linux-only. | | `api_port` | `8081` | Sister API port (8080 is rejected — that's XMRig's own API). | | `api_bind` | `"0.0.0.0"` | Sister API listen address. | -| `control` | `"disabled"` | `"enabled"` serves the **writable** control path (#236): a *separate* authenticated port that lets a Pithead stack apply config changes through RigForge, so `config.json` stays the source of truth (the producer for pithead Worker Inspect). **Fail-closed:** enabling it requires *both* `ACCESS_TOKEN` and `api_allow_from` — a writable API with no token or no pinned source is refused with a hard error. Only `pools`, `DONATION`, `autotune`, `watchdog`(+`watchdog_interval_min`), and `max_temp_c` are writable through it; anything else is rejected. The **remote** path additionally refuses to disable `watchdog` or to unset / out-of-band `max_temp_c` — a rig's thermal protection can only be *removed* by a local `rigforge.sh apply` on the box (#257). Each change is validated, the old config is snapshotted to `config-backups/` first, and a change that doesn't come back live is rolled back. The receiver holds no privilege and stages off the request path, so writes never touch mining. Linux-only. See [Operations › Control path](operations.md#writable-control-path-opt-in). | +| `control` | `"disabled"` | `"enabled"` serves the **writable** control path (#236): a *separate* authenticated port that lets a Pithead stack apply config changes through RigForge, so `config.json` stays the source of truth (the producer for pithead Worker Inspect). **Fail-closed:** enabling it requires *both* `ACCESS_TOKEN` and `api_allow_from` — a writable API with no token or no pinned source is refused with a hard error. Only `pools`, `DONATION`, `autotune`, `watchdog`(+`watchdog_interval_min`), and `max_temp_c` are writable through it; anything else is rejected. A change touching only `watchdog_interval_min` and/or `max_temp_c` applies without restarting XMRig (#381) — every other key restarts it. The **remote** path additionally refuses to disable `watchdog` or to unset / out-of-band `max_temp_c` — a rig's thermal protection can only be *removed* by a local `rigforge.sh apply` on the box (#257). Each change is validated, the old config is snapshotted to `config-backups/` first, and a change that doesn't come back live is rolled back. The receiver holds no privilege and stages off the request path, so writes never touch mining. Linux-only. See [Operations › Control path](operations.md#writable-control-path-opt-in). | | `control_port` | `8082` | Control path port (rejects 8080 and the `api_port`). | | `control_bind` | `"0.0.0.0"` | Control path listen address. Pair with `api_allow_from` (required) to pin who may write. | | `miner_user` | `""` *(root)* | Run the miner as this dedicated non-root system user (created at setup, nologin). RigForge applies the CPU's MSR preset root-side before start; on families without a known preset the ~10-15% MSR boost is skipped — which is why this ships opt-in. Lowering privilege changes nothing else: HugePages come from the boot reservation, tune/doctor/apply all keep working. | diff --git a/docs/operations.md b/docs/operations.md index dec6e4c..734156c 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -590,10 +590,16 @@ How a change flows: 2. A path-triggered root oneshot picks up the staged change, snapshots the current `config.json` to `config-backups/config-.json`, merges only the allowlisted keys, and re-validates the result with the same rules `apply` uses. An invalid change is rejected and **nothing is written**. -3. A valid change is written durably (temp file, `fsync`, atomic rename) and applied through the - normal `apply` path. If the miner does not come back to a live hashrate, the snapshot is restored - and re-applied, and the outcome is recorded as `rolled_back` (or `failed`, if the rollback snapshot - itself could not be read back). +3. A valid change is written durably (temp file, `fsync`, atomic rename) and applied. Changing only + `watchdog_interval_min` and/or `max_temp_c` takes a **restart-free fast path** (#381): neither key + ever reaches XMRig's generated config or its unit, so only the watchdog timer is reconciled and + XMRig itself is left running — round-trip in about a second instead of the ~60s a full restart + costs on a big-page host. Any other key (alone or mixed with those two) applies through the normal + `apply` path, which restarts XMRig. Either way, if the miner does not come back to a live hashrate + (full path) or the miner service is not found still running (fast path — a signal something else + was already wrong, since neither key can cause that), the snapshot is restored and re-applied + through the full path, and the outcome is recorded as `rolled_back` (or `failed`, if the rollback + snapshot itself could not be read back). 4. `GET :8082/status` returns the last change's outcome (`applied` / `rejected` / `rolled_back` / `failed`, with `source: "control"`, the changed keys, the backup path, and a `warnings[]` for any change that touched thermal protection). Every response also carries a derived `age_seconds` next to its own diff --git a/docs/pithead-integration.md b/docs/pithead-integration.md index b893a12..c7258c4 100644 --- a/docs/pithead-integration.md +++ b/docs/pithead-integration.md @@ -191,7 +191,11 @@ apply config changes through RigForge — the RigForge-side producer for pithead (pithead #185). It is deliberately independent of the read API: a `POST :8082/apply` of an allowlisted change (`pools`, `DONATION`, `autotune`, `watchdog`, `watchdog_interval_min`, `max_temp_c`) returns `202 Accepted`; RigForge validates, snapshots the old config, applies it, and -rolls back anything that doesn't come back live. The stack reads the new effective config back from +rolls back anything that doesn't come back live. Changing only `watchdog_interval_min` and/or +`max_temp_c` never restarts XMRig (#381) — those two are proven not to reach XMRig's config or unit, +so RigForge reconciles just the watchdog timer instead of running the full apply pipeline, landing in +about a second instead of the ~60s a restart costs. Any other key, alone or mixed with those two, +takes the full, XMRig-restarting path. The stack reads the new effective config back from `:8081/2/summary` and polls `:8082/status` for the outcome. The write path is pinned to the stack host by `api_allow_from` (mandatory) — the miner never accepts a config from anywhere else. Full mechanics and the security model: [Operations › Writable control path](operations.md#writable-control-path-opt-in) diff --git a/rigforge.sh b/rigforge.sh index bbb3b43..a64c611 100755 --- a/rigforge.sh +++ b/rigforge.sh @@ -4134,6 +4134,89 @@ _control_do_apply() { _wait_miner_live "${CONTROL_LIVE_TRIES:-20}" } +# #381 (from #344 item 1): a live walkthrough measured a single restart-free key +# (watchdog_interval_min) taking ~62s round-trip through POST /apply, because _control_do_apply +# above re-runs the ENTIRE apply() pipeline — regenerate xmrig's config, re-render its unit, restart +# the service, then wait out _wait_miner_live's pool-liveness retries — for every change, even one +# that never touches xmrig at all. +# +# CONTROL_FAST_PATH_KEYS is a CLOSED allowlist, checked as a SUBSET match (never a "not on the slow +# list" complement) by _control_fast_path_eligible below: a changed-keys set takes the fast path only +# when EVERY key in it is in this list, so an unrecognised key — including any future addition to +# CONTROL_WRITABLE_KEYS that nobody has re-proven restart-free here — falls through to the full, +# restart-safe path by construction. Evidence per key (grep rigforge.sh for both to re-check): +# +# watchdog_interval_min — bakes into ONLY rigforge-watchdog.timer's OnUnitActiveSec, rendered by +# install_watchdog. It never reaches generate_xmrig_config or the xmrig unit template, so xmrig +# itself has nothing to reload; install_watchdog (the SAME function the full path already calls +# unconditionally on every apply, see apply() above) is sufficient — it re-renders the timer and +# daemon-reloads it. +# max_temp_c — never rendered into ANY unit. install_watchdog's own comment records why: "only the +# cadence is baked into the units — the verb re-reads config.json every run, so an apply after a +# max_temp_c ... edit needs no unit rewrite". The watchdog verb picks it up on its next scheduled +# run; nothing beyond the config.json write _control_commit already did is needed here. +# +# pools/DONATION change xmrig's OWN generated config — restarting it is the only way xmrig serves the +# new values — and autotune/watchdog (the enable/disable flag, not the interval) govern install_* +# paths this issue has not audited for restart-freedom. All four stay on the full path. Never widen +# this list without the same kind of evidence trail (a grep proving the key never reaches +# generate_xmrig_config or a unit template); the closed-set-subset design means an unaudited key's +# safe default is already "restart", not "skip". +CONTROL_FAST_PATH_KEYS="watchdog_interval_min max_temp_c" + +# True (rc 0) iff is non-empty and every key in it is in CONTROL_FAST_PATH_KEYS. A single +# key outside the list — or an empty/malformed keys-csv (e.g. control_apply's "?" sentinel when jq +# couldn't read the staged file's keys) — fails CLOSED to "not eligible", so the caller takes the +# full apply() path. Subset match, never complement: this can only be talked into "eligible" by a key +# that is actually named on the allowlist above, so the allowlist growing is the only way this +# function's answer changes for a given input. +_control_fast_path_eligible() { # + local keys="$1" k found=0 + [ -n "$keys" ] || return 1 + local IFS=',' + for k in $keys; do + [ -n "$k" ] || continue + found=1 + case " $CONTROL_FAST_PATH_KEYS " in + *" $k "*) ;; + *) return 1 ;; + esac + done + [ "$found" -eq 1 ] +} + +# The restart-free counterpart to _control_do_apply (#381). Only reached when +# _control_fast_path_eligible says every changed key is provably restart-free (see +# CONTROL_FAST_PATH_KEYS above), so this DELIBERATELY skips apply()/_apply_runtime entirely: no +# generate_xmrig_config, no xmrig unit re-render, no `systemctl restart` of the miner, no +# _wait_miner_live retry loop, no _apply_pool_check. It reuses install_watchdog verbatim — the same +# call the full apply() makes unconditionally on every run — rather than re-implementing unit +# rendering, so the fast and full paths cannot drift on what "restart-free" actually renders. It +# still stamps provenance exactly like apply() does, via the same RIGFORGE_CONFIG_SOURCE / +# RIGFORGE_CONFIG_CHANGE_ID dynamic-scope contract, so config_meta on the read feed does not depend on +# which path served the change. +# +# Success is "the run-state did not DEGRADE", not "is currently active" (security review finding on +# the original version of this function, which gated on is-active alone): a rig can be LEGITIMATELY +# stopped before this change lands — a watchdog thermal hold, or an operator's manual stop — and by +# construction the fast path never touches the xmrig unit or service, so a stopped rig staying +# stopped (or even coming back up on its own) is correct, not a failure; the new value takes effect +# on its own schedule (the watchdog's next tick). Only a transition from active to inactive is a real +# regression worth the caller's rollback. No thermal-hold-marker special-casing needed: the +# before/after comparison covers every "was already down" case with less machinery. +_control_do_apply_fast() { + local was_active is_active + was_active=1 + systemctl is-active --quiet "$SERVICE_NAME" 2>/dev/null && was_active=0 + parse_config + install_watchdog >/dev/null 2>&1 || true + _stamp_config_meta "${RIGFORGE_CONFIG_SOURCE:-local}" "${RIGFORGE_CONFIG_CHANGE_ID:-}" + is_active=1 + systemctl is-active --quiet "$SERVICE_NAME" 2>/dev/null && is_active=0 + [ "$was_active" -eq 1 ] && return 0 + [ "$is_active" -eq 0 ] +} + # Record a status record for the receiver's GET /status (mode 644 so the DynamicUser server reads it # back) — a terminal outcome, or control_upgrade's non-terminal `started` marker (#320). _control_status() { # @@ -4198,13 +4281,29 @@ control_apply() { fi backup="${result#committed }" _reown_config_backups "$backups" - log "control-apply: committed change $cid (keys: $change_keys); applying..." # #254: attribute this (and the rollback re-apply) to the control path with its change_id — the # nested apply()'s _stamp_config_meta reads these via dynamic scope. local RIGFORGE_CONFIG_SOURCE=control RIGFORGE_CONFIG_CHANGE_ID="$cid" - if _control_do_apply; then + # #381: dispatch on the closed fast-path allowlist. Only the CLASSIFICATION differs between the + # two branches below — a success writes the same "applied" status either way, and a failure of + # EITHER path falls through to the same full-pipeline rollback, so a wrong "eligible" verdict (or + # the fast path failing for an unrelated reason) still ends up restart-safe, never silently stuck. + local fast=0 apply_ok=0 + if _control_fast_path_eligible "$change_keys"; then + fast=1 + log "control-apply: committed change $cid (keys: $change_keys); applying (fast path — xmrig untouched)..." + _control_do_apply_fast && apply_ok=1 + else + log "control-apply: committed change $cid (keys: $change_keys); applying..." + _control_do_apply && apply_ok=1 + fi + if [ "$apply_ok" -eq 1 ]; then _control_status "$status" applied "$cid" "$change_keys" "" "$backup" - log "control-apply: change $cid applied." + if [ "$fast" -eq 1 ]; then + log "control-apply: change $cid applied (fast path)." + else + log "control-apply: change $cid applied." + fi else warn "control-apply: change $cid did not come back live — rolling back to $backup." # #276: the backup must be readable to restore it — guard the cp explicitly (not just -e/ERR) diff --git a/tests/run.sh b/tests/run.sh index 2785bf8..ec6c0cc 100644 --- a/tests/run.sh +++ b/tests/run.sh @@ -7041,6 +7041,160 @@ bkf_out="$( ( assert_contains "commit: unwritable backup dir -> rejected" "$bkf_out" "rejected backup-failed" assert_eq "commit: backup failure leaves config.json untouched (donation 1)" "$(jq -r .DONATION "$bkfail/config.json")" "1" +echo "== unit: _control_fast_path_eligible — closed allowlist classification (#381) ==" +fpe() { # -> eligible|not + ( + source "$SCRIPT" + set +e + if _control_fast_path_eligible "$1"; then echo eligible; else echo not; fi + ) +} +assert_eq "single restart-free key: watchdog_interval_min -> eligible" "$(fpe "watchdog_interval_min")" "eligible" +assert_eq "single restart-free key: max_temp_c -> eligible" "$(fpe "max_temp_c")" "eligible" +assert_eq "both restart-free keys together -> eligible" "$(fpe "max_temp_c,watchdog_interval_min")" "eligible" +assert_eq "pools alone (own xmrig config) -> not eligible" "$(fpe "pools")" "not" +assert_eq "DONATION alone (own xmrig config) -> not eligible" "$(fpe "DONATION")" "not" +assert_eq "autotune alone (unaudited install_* path) -> not eligible" "$(fpe "autotune")" "not" +assert_eq "watchdog alone (unaudited install_* path) -> not eligible" "$(fpe "watchdog")" "not" +# Mutation this catches: the ALL-keys-must-qualify subset check loosened to an ANY-key check — a +# restart-free key riding alongside DONATION would wrongly clear the whole change for the fast path. +assert_eq "mixed restart-free + non-restart-free -> not eligible" "$(fpe "DONATION,max_temp_c")" "not" +# Mutation this catches: the closed-allowlist subset check inverted into a "not on the slow list" +# complement — an unrecognised/future CONTROL_WRITABLE_KEYS addition would then wrongly pass. +assert_eq "unrecognised/future key alone -> not eligible (closed set, not a complement)" "$(fpe "some_future_key")" "not" +assert_eq "empty keys-csv -> not eligible (fail closed)" "$(fpe "")" "not" +assert_eq "control_apply's jq-failure sentinel '?' -> not eligible (fail closed)" "$(fpe "?")" "not" + +# Pin the allowlist's exact membership (mirrors the CONTROL_WRITABLE_KEYS drift guard further below). +# Mutation this catches: the allowlist silently growing (or shrinking) without a matching +# evidence-trail/test update. +fp_keys="$(grep -oE 'CONTROL_FAST_PATH_KEYS="[^"]*"' "$SCRIPT" | head -1 | sed 's/.*="//; s/"//' | tr ' ' '\n' | sort | tr '\n' ' ')" +assert_eq "fast-path allowlist is EXACTLY {max_temp_c, watchdog_interval_min} (#381)" "$fp_keys" "max_temp_c watchdog_interval_min " +# Drift guard: the fast-path allowlist must stay a SUBSET of the control-writable allowlist — a +# fast-path key control_apply couldn't even accept as writable in the first place would be dead code. +ck_keys="$(grep -oE 'CONTROL_WRITABLE_KEYS="[^"]*"' "$SCRIPT" | head -1 | sed 's/.*="//; s/"//' | tr ' ' '\n' | sort | tr '\n' ' ')" +fp_subset_ok=y +for k in $fp_keys; do + case " $ck_keys " in *" $k "*) ;; *) fp_subset_ok=n ;; esac +done +assert_eq "fast-path allowlist is a subset of the control-writable allowlist (#381)" "$fp_subset_ok" "y" + +echo "== unit: _control_do_apply_fast — reuses install_watchdog, skips apply()/xmrig restart (#381) ==" +FPA="$(mktemp -d "$SANDBOX/fpa.XXXXXX")" +mkdir -p "$FPA/systemd" +cp "$ROOT/systemd/rigforge-watchdog.service.template" "$ROOT/systemd/rigforge-watchdog.timer.template" "$FPA/systemd/" +fpa_run() { # -> "rc="; side effects: $FPA/apply-called, $FPA/meta.json + rm -f "$FPA/apply-called" "$FPA/meta.json" + ( + source "$SCRIPT" + OS_TYPE=Linux + SCRIPT_DIR="$FPA" + SYSTEMD_DIR="$FPA/systemd" + REAL_USER=rfop + SERVICE_NAME=xmrig + CONFIG_JSON="$FPA/config.json" + CONFIG_META_FILE="$FPA/meta.json" + WATCHDOG_MODE=enabled + WATCHDOG_INTERVAL_MIN="$1" + # Isolate: parse_config would normally derive the globals above from CONFIG_JSON; stub it so + # this test pins _control_do_apply_fast's OWN behaviour, not parse_config's (covered elsewhere). + parse_config() { :; } + apply() { + echo called >"$FPA/apply-called" 2>/dev/null + return 0 + } + RIGFORGE_CONFIG_SOURCE=control + RIGFORGE_CONFIG_CHANGE_ID=fedcba9876543210 + set +e + PATH="$STUBS:$PATH" _control_do_apply_fast + echo "rc=$?" + ) +} +printf '{"pools":[{"url":"h:3333"}],"watchdog":"enabled","watchdog_interval_min":9}\n' >"$FPA/config.json" +out="$(fpa_run 9)" +assert_contains "_control_do_apply_fast returns 0 when the miner service is active" "$out" "rc=0" +assert_eq "_control_do_apply_fast NEVER calls apply() — xmrig is not restarted (#381)" "$([ -f "$FPA/apply-called" ] && echo called || echo not-called)" "not-called" +assert_contains "install_watchdog re-renders the timer with the NEW interval (#381)" "$(cat "$FPA/systemd/rigforge-watchdog.timer")" "OnUnitActiveSec=9min" +assert_eq "config_meta stamped source=control, parity with apply()'s own _stamp_config_meta call (#381)" "$(jq -r .source "$FPA/meta.json" 2>/dev/null)" "control" +assert_eq "config_meta records the change_id, same parity (#381)" "$(jq -r .last_change_id "$FPA/meta.json" 2>/dev/null)" "fedcba9876543210" + +echo "== unit: control_apply + REAL _control_do_apply_fast — run-state criterion, not is-active alone (#381 security review) ==" +# The generic systemctl stub always exits 0, so the fpa_run tests above only ever exercise the +# active-before/active-after case. A rig can be LEGITIMATELY stopped when a restart-free change +# lands — a watchdog thermal hold, or an operator's manual stop — and the fast path must not read +# that pre-existing stop as its own failure and roll the change back. This exercises control_apply +# end to end with the REAL _control_do_apply_fast (unlike the marker-stubbed one in ca_exec below) and +# a STATEFUL systemctl stub that answers `is-active` differently across the two calls the function +# makes (before the watchdog reconcile, and after), so the run-state comparison is genuinely tested. +CAF="$(mktemp -d "$SANDBOX/caf.XXXXXX")" +caf_systemctl_stub() { # -> writes $CAF/bin/systemctl + mkdir -p "$CAF/bin" + cat >"$CAF/bin/systemctl" <> "\${CALL_LOG:-/dev/null}" +case "\$*" in +*"is-active"*) + n=\$(( \$(cat "$CAF/systemctl-calls" 2>/dev/null || echo 0) + 1 )) + echo "\$n" >"$CAF/systemctl-calls" + if [ "\$n" -eq 1 ]; then exit $1; else exit $2; fi + ;; +*) exit 0 ;; +esac +EOF + chmod +x "$CAF/bin/systemctl" +} +caf_exec() { + ( + source "$SCRIPT" + parse_config() { :; } # the live config is already valid; don't re-validate it (matches ca_exec) + # apply()/_wait_miner_live only matter for the ROLLBACK leg here — already covered in depth by + # the #236/#276 tests below — so they stay simple stubs; the marker proves whether a rollback + # (i.e. a restart attempt) was ever reached, which is exactly what a wrongly-tripped fast-path + # failure would cause. + apply() { + echo called >"$CAF/full-apply-called" 2>/dev/null || true + return 0 + } + _wait_miner_live() { return 0; } + OS_TYPE=Linux + SCRIPT_DIR="$CAF" + SYSTEMD_DIR="$CAF/systemd" + CONFIG_JSON="$CAF/config.json" + REAL_USER=rfop + SERVICE_NAME=xmrig + WATCHDOG_MODE=enabled + RIGFORGE_CONTROL_STATE="$CAF/state" + set +e + PATH="$CAF/bin:$STUBS:$PATH" control_apply >/dev/null 2>&1 + ) +} +caf_run() { # + rm -rf "$CAF" + mkdir -p "$CAF/systemd" "$CAF/state/spool" + cp "$ROOT/systemd/rigforge-watchdog.service.template" "$ROOT/systemd/rigforge-watchdog.timer.template" "$CAF/systemd/" + printf '%s\n' "$CFG_236" >"$CAF/config.json" + printf '%s' "$1" >"$CAF/state/spool/pending-abc123.json" + caf_systemctl_stub "$2" "$3" + caf_exec +} +cfst() { jq -r ".$1" "$CAF/state/status.json" 2>/dev/null; } + +# (a) inactive-before, inactive-after (rc 1, 1): a rig thermally held or manually stopped before the +# change. Mutation this catches: reverting the run-state comparison to a naive "is it active NOW" +# check — that mutant reports rc=1 here (not active) and would wrongly roll the change back. +caf_run '{"max_temp_c":90}' 1 1 +assert_eq "inactive-before/inactive-after -> status applied, not rolled back (#381)" "$(cfst status)" "applied" +assert_eq "inactive-before/inactive-after -> the new value lands in config.json (#381)" "$(jq -r .max_temp_c "$CAF/config.json")" "90" +assert_eq "inactive-before/inactive-after -> no restart/rollback ever attempted (#381)" "$([ -f "$CAF/full-apply-called" ] && echo called || echo not-called)" "not-called" + +# (b) active-before, inactive-after (rc 0, 1): the miner really did go down across this change. +# Guards against the run-state criterion being dropped entirely (e.g. _control_do_apply_fast reverted +# to always returning 0) — a real regression here must still trip the existing rollback. +caf_run '{"max_temp_c":90}' 0 1 +assert_eq "active-before/inactive-after -> status rolled_back (#381)" "$(cfst status)" "rolled_back" +assert_eq "active-before/inactive-after -> config restored, max_temp_c unset again (#381)" "$(jq -r .max_temp_c "$CAF/config.json")" "null" +assert_eq "active-before/inactive-after -> rollback re-apply invoked (#381)" "$([ -f "$CAF/full-apply-called" ] && echo called || echo not-called)" "called" + echo "== unit: control_apply orchestration + rollback (#236) ==" CA="$(mktemp -d "$SANDBOX/ca.XXXXXX")" ca_exec() { @@ -7048,8 +7202,21 @@ ca_exec() { source "$SCRIPT" parse_config() { :; } # the live config is already valid; don't re-validate it # Stub apply + liveness (not _control_do_apply itself) so its real body runs: apply is a - # no-op, CA_APPLY_OK drives whether the miner "comes back" (0 -> the rollback path). - apply() { return 0; } + # no-op, CA_APPLY_OK drives whether the miner "comes back" (0 -> the rollback path). The + # marker file lets an assertion OUTSIDE this subshell prove apply() — the pipeline that + # regenerates xmrig's config and restarts it — was (or, #381, was deliberately NOT) reached. + apply() { + echo called >"$CA/full-apply-called" 2>/dev/null || true + return 0 + } + # #381: the fast-path counterpart, stubbed the same way — not _control_fast_path_eligible + # (its real body runs, since the routing decision IS what these tests exercise). + # CA_FAST_APPLY_OK drives whether it "succeeds" (default 1); on failure control_apply must + # fall through to the SAME full-pipeline rollback a failed full apply already takes. + _control_do_apply_fast() { + echo called >"$CA/fast-apply-called" 2>/dev/null || true + [ "${CA_FAST_APPLY_OK:-1}" = 1 ] + } # _wait_miner_live is called once for the initial apply and (on the rollback path) again for the # rollback re-apply. CA_APPLY_OK drives the 1st call; CA_ROLLBACK_OK drives the 2nd, defaulting to # CA_APPLY_OK so every pre-#276 test (which only ever sets CA_APPLY_OK) is unaffected — #276 pins @@ -7141,6 +7308,45 @@ printf '%s' '{"status":"pending","change_id":"'"$CID344"'","accepted_at":"2020-0 CA_APPLY_OK=1 ca_exec assert_eq "control_apply writes the terminal changes/.json (#344)" "$([ -f "$CA/state/changes/$CID344.json" ] && echo y || echo n)" "y" assert_eq "control_apply clears the now-superseded pending/.json (#344)" "$([ -f "$CA/state/pending/$CID344.json" ] && echo y || echo n)" "n" + +# #381 (from #344 item 1): control_apply must route a change through _control_do_apply_fast instead +# of the full, xmrig-restarting apply() IFF every changed key is on the closed CONTROL_FAST_PATH_KEYS +# allowlist — proven here via the full-apply-called/fast-apply-called markers ca_exec's stubs write, +# not just by the reported status (which is "applied" either way and so can't tell the paths apart). +ca_run "$CFG_236" '{"watchdog_interval_min":9}' 1 +assert_eq "watchdog_interval_min-only change -> status applied (#381)" "$(cst status)" "applied" +assert_eq "watchdog_interval_min-only change -> config committed" "$(jq -r .watchdog_interval_min "$CA/config.json")" "9" +assert_eq "watchdog_interval_min-only change takes the fast path" "$([ -f "$CA/fast-apply-called" ] && echo called || echo not-called)" "called" +assert_eq "watchdog_interval_min-only change NEVER calls apply() (xmrig untouched, #381)" "$([ -f "$CA/full-apply-called" ] && echo called || echo not-called)" "not-called" +ca_run "$CFG_236" '{"max_temp_c":90}' 1 +assert_eq "max_temp_c-only change also takes the fast path (#381)" "$([ -f "$CA/fast-apply-called" ] && echo called || echo not-called)" "called" +assert_eq "max_temp_c-only change never calls apply() (#381)" "$([ -f "$CA/full-apply-called" ] && echo called || echo not-called)" "not-called" +ca_run "$CFG_236" '{"watchdog_interval_min":9,"max_temp_c":90}' 1 +assert_eq "both restart-free keys together still take the fast path (#381)" "$([ -f "$CA/fast-apply-called" ] && echo called || echo not-called)" "called" +assert_eq "both-keys change reports applied (#381)" "$(cst status)" "applied" + +# Mutation this catches: if the ALL-keys-must-qualify subset check were loosened to an ANY-key (or a +# "not explicitly on the slow list") check, this mixed change would wrongly take the fast path and +# skip the restart DONATION needs to actually reach xmrig. +ca_run "$CFG_236" '{"DONATION":5,"max_temp_c":90}' 1 +assert_eq "mixed restart-free + full-path key -> takes the FULL path (#381)" "$([ -f "$CA/full-apply-called" ] && echo called || echo not-called)" "called" +assert_eq "mixed change never takes the fast path (#381)" "$([ -f "$CA/fast-apply-called" ] && echo called || echo not-called)" "not-called" +assert_eq "mixed change still reports applied" "$(cst status)" "applied" +# A pure DONATION change (already covered under #236 above) stays on the full path too — restated +# here under #381 naming so the fast/full boundary is asserted with the marker files in one place. +ca_run "$CFG_236" '{"DONATION":6}' 1 +assert_eq "DONATION-only change takes the full path, not fast (#381)" "$([ -f "$CA/full-apply-called" ] && echo called || echo not-called)" "called" +assert_eq "DONATION-only change never takes the fast path (#381)" "$([ -f "$CA/fast-apply-called" ] && echo called || echo not-called)" "not-called" + +# A failed fast-path apply must fall back to the SAME full, restart-safe rollback a failed full apply +# already takes — never report "applied" on a fast-path failure, and never leave the change stuck. +# Mutation this catches: the fast branch skipping the rollback on failure, or reporting "applied" +# regardless of _control_do_apply_fast's return code. +CA_FAST_APPLY_OK=0 ca_run "$CFG_236" '{"max_temp_c":90}' 1 +assert_eq "failed fast-path apply -> status rolled_back, not applied (#381)" "$(cst status)" "rolled_back" +assert_eq "failed fast-path apply -> config restored to pre-change (max_temp_c unset again)" "$(jq -r .max_temp_c "$CA/config.json")" "null" +assert_eq "the rollback re-apply after a fast-path failure uses the FULL path (#381)" "$([ -f "$CA/full-apply-called" ] && echo called || echo not-called)" "called" + # prune: KEEP_CONFIG_BACKUPS caps the history PB="$(mktemp -d "$SANDBOX/pb.XXXXXX")" mkdir -p "$PB/bk"