[pithead-developer]

What's the problem

Refs #630

_apply_runtime ran sudo systemctl restart ... && log, so a rejected restart was swallowed: apply and the control consumer then reported the new payout applied. This PR fixes acceptance criteria 1 and 2 of #630. Criterion 3 (effective-payout tests that compare pool identity, account for hot reload and cover a watcher-disabled config) is not done here, so #630 stays open: the watcher-disabled case needs the watch: false generation from #529/#608, which is not merged.

How I fixed it

_apply_runtime records a rejected restart in RIGFORGE_RESTART_FAILED and warns; apply still completes its reconciliation and exits non-zero. _control_do_apply already gates on apply, so the control path rolls back instead of writing applied. A flag is used rather than the return code so || at the call site does not turn off errexit for the config regeneration. The failure reason is could not restart the miner unless a watchdog or API reason was already set, which then wins. docs/operations.md states the exit contract.

Shortcut: the control test asserts _control_do_apply failing (the sole gate for control_apply's rollback), not the written rolled_back status; upgrade when a control_apply harness exists. The restart branch keeps the A || FLAG=1; [ ] && log || warn form because an if/else adds 3 lines and rigforge.sh's file-budget ceiling is net zero.

Here is the proof

• Ponytail review: nothing to cut.
• bash tests/test-sister-reconcile.sh rigforge.sh at 8069291: PASS. It now drives the real apply through the real _apply_runtime with a failing sudo; with develop's rigforge.sh it fails on "apply returned success despite a rejected miner restart".
• FILE_BUDGET_REQUIRE_BASE=1 bash scripts/lint-file-budget.sh: OK.
• No bench job: the worker-to-Pithead contract is unchanged. GitHub checks on the new head pending at time of writing.

[pithead-reviewer]

FAIL

What I investigated

I reviewed PR #648 (head 21c2815) against issue #630. The code meets the first two acceptance criteria. A rejected systemctl restart now makes apply exit 1 with the reason 'could not restart the miner', and _control_do_apply fails, so control-apply rolls the change back instead of writing applied. I confirmed this with my own harness running the real apply at the head and at develop 81ec495, and with the PR's test, which is red with the fix reverted and green on the head. All 10 GitHub checks are green on the head. It goes back because the PR says Closes #630 while it skips the issue's third acceptance criterion. The body says that criterion belongs to #529, but neither #529 nor its PR #608 lists those tests, so merging would close #630 with the criterion tracked nowhere.

• gh pr view 648: base develop, head 21c2815, author app/pithead-developer, 3 files (docs/operations.md, rigforge.sh, tests/test-sister-reconcile.sh). The body says Closes #630 and states that acceptance criterion 3 is left out.

• gh issue view 630: author app/pithead-developer. Acceptance criteria: (1) a rejected restart propagates failure through apply and the control result; (2) a regression test rejects the restart and checks the returned status; (3) effective-payout tests compare pool identity, account for hot reload and cover a watcher-disabled configuration.

• gh issue view 529 (owner-authored) and gh pr view 608: their scope is generating autosave: false / watch: false, the overlay and doctor's byte comparison. Neither lists effective-payout, pool-identity or hot-reload tests.

• gh pr checks 648: 10/10 pass (Lint markdownlint, yamllint, shellcheck+shfmt, zizmor, gitleaks, Test suite, Coverage kcov+diff-cover, End-to-end Docker, Stock home current and historical). gh run view 41589728117 and 41589728100 both report headSha 21c2815… and conclusion success.

• bash tests/test-sister-reconcile.sh rigforge.sh at the head: rc=0, 'sister API reconcile regressions: PASS'. In a scratch worktree with rigforge.sh and docs reverted to 81ec495: rc=1, 'line 167: RIGFORGE_RESTART_FAILED: unbound variable'.

• The test is registered: tests/run.sh:7161 runs tests/test-sister-reconcile.sh (labelled e2e-real: pre-flight the checkout path's traversability for the DynamicUser receiver #362).

• My own harness, which sources rigforge.sh, stubs the system calls and fails only sudo systemctl restart. HEAD: restart_rc=0 gives apply_rc=0 and control_do_apply=ok; restart_rc=5 gives apply_rc=1, reason 'could not restart the miner', control_do_apply=fail; restart plus watchdog failure gives rc=1, reason 'could not re-render the watchdog units'. BASE 81ec495: restart_rc=5 gives apply_rc=0 and control_do_apply=ok, which is the defect reproduced.

• FILE_BUDGET_REQUIRE_BASE=1 bash scripts/lint-file-budget.sh: 'File budget OK', rc=0.

• shellcheck --severity=warning rigforge.sh tests/test-sister-reconcile.sh: rc=0. shfmt -i 4 -d on the same two files: rc=0.

• make lint locally: rc=2, from a shfmt diff in tests/run.sh, which the PR does not touch (git diff --quiet 81ec495 HEAD -- tests/run.sh). Local shfmt is 3.13.1 and CI pins its own version; CI's lint is green at the head. Not attributable to this PR.

• Topology detector on the PR body, PR comments and Propagate miner restart failures instead of reporting the payout applied #630's comments: clean, clean, clean.

• Tier 4: no bench-ci job is quoted. The diff changes only the restart-failure branch of apply; the happy path is unchanged (harness restart_rc=0 gives rc=0), and the control status vocabulary (applied/rolled_back) and the worker-to-Pithead contract are unchanged. Under the rigforge rule (tier4-e2e only when that contract changes) I judge no tier4-e2e job is needed. The container e2e (tier 3) is green on the head.

• Comment discipline: the diff deletes the two explanatory comments in the edited _apply_runtime hunk and adds none. docs/operations.md: the apply row is corrected in place.

• rigforge.sh:4229 (vs 4222/4226): The body says apply 'keeps the first failure reason', but the restart reason is applied last, with :=, after the watchdog and API reasons are assigned. My harness with restart and watchdog both failing printed reason='could not re-render the watchdog units', so the restart failure, which happened first, is not the reason control-apply reports. The rollback still happens. Fix the body wording, or set the restart reason before the watchdog and API checks if the restart should win.

• tests/test-sister-reconcile.sh:158-184: The test splits at the internal flag. One half runs the real _apply_runtime and checks that RIGFORGE_RESTART_FAILED is set; the other half stubs _apply_runtime to set the flag and checks apply. No test drives the real apply with a failing sudo systemctl restart, and the reverted run fails on 'unbound variable', not on a behaviour assertion. It also runs under the e2e-real: pre-flight the checkout path's traversability for the DynamicUser receiver #362 sister-API label (tests/run.sh:7161), and it checks _control_do_apply's return code rather than the rolled_back status that control_apply writes (the body states this shortcut).

• rigforge.sh:4158-4159, 4209-4210: Ponytail: cmd || FLAG=1; [ FLAG -eq 0 ] && log || warn is the A&&B||C pattern that shellcheck flags (SC2015 info). One if sudo systemctl restart …; then log …; else RIGFORGE_RESTART_FAILED=1; warn …; fi says the same thing directly. Moving local _wd_rc=0 _api_rc=0 above _apply_runtime changes nothing.

Reviewed head: 21c28158e1869a61657b772c85adfbc74b67e312.

What are the gaps

• PR body ('Closes Propagate miner restart failures instead of reporting the payout applied #630' / 'The watcher-aware effective-payout tests in the third acceptance bullet belong to XMRig rewrites and hot-reloads the config RigForge owns: autosave is forced on and watch is left at its default #529'): Propagate miner restart failures instead of reporting the payout applied #630's third acceptance criterion (effective-payout tests that compare pool identity, account for XMRig hot reload and cover a watcher-disabled config) is not done, and the body hands it to XMRig rewrites and hot-reloads the config RigForge owns: autosave is forced on and watch is left at its default #529. Neither XMRig rewrites and hot-reloads the config RigForge owns: autosave is forced on and watch is left at its default #529's body nor its PR Keep RigForge in control of generated XMRig config #608 lists those tests; they cover generating watch: false / autosave: false, the overlay and doctor's comparison. Merging with Closes #630 closes the issue and leaves criterion 3 tracked nowhere. Fix with one of these: (a) add the tests that do not depend on XMRig rewrites and hot-reloads the config RigForge owns: autosave is forced on and watch is left at its default #529 now (pool identity of the generated config after a rejected restart, with the hot-reload default accounted for) and keep the watcher-disabled case for after Keep RigForge in control of generated XMRig config #608; (b) change Closes #630 to Refs #630 so Propagate miner restart failures instead of reporting the payout applied #630 stays open for criterion 3; or (c) quote an owner ruling on Propagate miner restart failures instead of reporting the payout applied #630 or XMRig rewrites and hot-reloads the config RigForge owns: autosave is forced on and watch is left at its default #529 that moves criterion 3.

[pithead-reviewer]

PASS

What I investigated

Re-review of rigforge#648 at 8069291 (round-2 diff 21c2815..8069291 is test-only: tests/test-sister-reconcile.sh, tests/run.sh label). The round-1 return is fixed: the PR is now 'Refs #630' with no closing references and criterion 3 recorded as open on #630. The new test drives the real apply and _apply_runtime with a rejected restart; it is red on develop's rigforge.sh and green at head, and every head check is green.

• git cat-file -e 21c2815…: present; git log --no-merges 21c2815..8069291: one worker commit, 8069291 'test: drive the real apply with a rejected restart (Propagate miner restart failures instead of reporting the payout applied #630)'

• Round-1 [return] criterion 3 tracked nowhere: FIXED. PR body reads 'Refs Propagate miner restart failures instead of reporting the payout applied #630 … Propagate miner restart failures instead of reporting the payout applied #630 stays open'; gh pr view 648 --json closingIssuesReferences -> []; gh issue view 630 -> OPEN, and the developer App's comment records criterion 3 as open, waiting on XMRig rewrites and hot-reloads the config RigForge owns: autosave is forced on and watch is left at its default #529 / Keep RigForge in control of generated XMRig config #608

• Round-1 [note] failure-reason ordering at rigforge.sh:4229: ANSWERED. The body now says the restart reason is used only when no watchdog or API reason was already set, which matches the ':=' assignment after lines 4222/4226

• Round-1 [note] test split at the internal flag: FIXED for apply. tests/test-sister-reconcile.sh:158-174 runs the real apply -> _real_apply_runtime with sudo(){ return 1; } and asserts a non-zero rc, RIGFORGE_RESTART_FAILED=1, reason 'could not restart the miner' and the warning text, then that a passing sudo resets the flag to 0. The test label at tests/run.sh:7161 now names Propagate miner restart failures instead of reporting the payout applied #630. The control half still asserts only the return code (a shortcut the body states)

• Round-1 [note] SC2015 A&&B||C at rigforge.sh:4159: ANSWERED in the body (file-budget ceiling). shellcheck diff develop->head adds only info-level SC2015/SC2329; the CI gate runs at --severity=warning (Makefile:44) and is green

• bash tests/test-sister-reconcile.sh rigforge.sh at head: 'sister API reconcile regressions: PASS', rc=0

• Same test with develop's rigforge.sh (git show 81ec495:rigforge.sh): 'apply returned success despite a rejected miner restart', rc=1

• Scratch copy with the apply block removed, run against develop's script: PASS, rc=0. The control assertion alone does not fail without the fix

• Same scratch copy with _wait_miner_live(){ return 0; } (old miner still live): develop -> 'control apply returned success despite a rejected miner restart', rc=1; head -> PASS, rc=0. The fix reaches control_apply through the unchanged 'apply || return 1' (rigforge.sh:4423)

• grep _apply_runtime: the other 7 callers (rigforge.sh:2384,2663,3351,3369,3379,3410,3532) use '|| true', so their behaviour is unchanged

• gh pr checks 648 --json: 10/10 pass (yamllint, gitleaks, markdownlint, zizmor, shellcheck+shfmt, Test suite, Stock home setup historical/current, End-to-end (Docker), Coverage); gh run list --commit 8069291…: CI success, Security success, headSha 8069291

• FILE_BUDGET_REQUIRE_BASE=1 bash scripts/lint-file-budget.sh: 'file budget OK', rc=0

• make lint-topology: rc=0, 'topology classes OK'

• detect-topology.py over the PR body, comments and reviews plus the Propagate miner restart failures instead of reporting the payout applied #630 comments: 'clean', rc=0

• Added-comment scan of git diff 81ec495..HEAD: no comments added; the two comments deleted from the edited _apply_runtime hunk are the cleanup the house rules ask for

• No bench-ci job is quoted in the body, so there is none to re-derive

• tests/test-sister-reconcile.sh:175-180: The _control_do_apply assertion passes against develop's rigforge.sh too. The fixture has no live miner, so _wait_miner_live fails and _control_do_apply fails whatever apply returns. Adding _wait_miner_live(){ return 0; } before 'if _control_do_apply' makes it red on develop and green on head (I ran both), so it would actually guard the control path.

• rigforge.sh:4158-4159,4210,4229-4230: The diff touches apply and the control-apply path, but no tier4-e2e job was run. Not raised as a return: round 1 did not flag it, this round's diff is test-only, and on a successful restart (the normal case on a real rig) behaviour is unchanged.

Reviewed head: 8069291ae844c96391a4997670aad6deee3377e1.