fix(control): a control change that could not be installed is no longer recorded as applied (#434)

[VijitSingh97]

Closes #434.

The defect

_control_commit's tail is what actually installs a control change: a chmod 600 on the
candidate, then an atomic rename over config.json. Neither step was checked. If either
failed the function still echoed committed <backup> and returned 0, so control_apply
believed it and wrote a terminal applied.

That is the worst shape available for a remote control path: GET /status and the published
changes/<cid>.json both announce a change the rig is not running, the miner is restarted for
a config that never moved, and nothing anywhere reports an error.

set -E was never the backstop it looks like. Bash unsets errexit inside a command
substitution unless inherit_errexit is on, and this script sets no shopt at all, so the
ERR trap here bought a log line and never a guard. Pre-existing, not a regression.

The fix

Both steps are guarded, each with its own reason, and _control_commit returns rc 2 —
distinct from the rc 1 every other refusal returns. control_apply dispatches on it to the
failed terminal.

The one judgement call worth checking: failed, not rejected. docs/operations.md
defines rejected as an invalid change with nothing written; reporting one here would
send the operator to fix a change that was fine. failed is the rig-side terminal ADR 0002
already gives this class on the upgrade path, and tests/contract/v1/control-status.json
already lists it with backup: null — the exact shape this path emits, so the frozen v1
contract needs no change.

Nothing is applied on this path and there is nothing to roll back: the rename lands in
config.json's own directory, so it either replaces the file wholly or leaves it untouched,
and the miner is never reached. The two steps are guarded separately, with distinct
reasons, because the status reason is the only thing a remote operator sees and this whole
issue is about that record lying. A reviewer may reasonably prefer one combined guard; it
would cost one branch and the ability to say which step lost the change.

How it was proven

The test rows were pushed first, on the unfixed tree, so CI fired them as the control
before the fix existed. Both runs are on this PR's history and can be re-read.

The control run (commit 3461249, test rows only, unfixed tree): the suite reported
9 failing rows, and all 9 of them are #434 rows — zero collateral anywhere else in the
suite. The two positive controls in the new block are green, so the fixture works and the
reds are the guard's absence, not a broken harness.

Each red is red for the predicted reason:

row	why it is red on the unfixed tree
a failed install is reported failed, not committed	the tail echoes committed <backup>
a failed install returns 2, not 0	it returns 0
a failed install removes its candidate file	nothing removes config.json.control.<pid>
a candidate that could not be chmod-ed is not installed	the rename runs anyway
a failed chmod returns 2, not 0	it returns 0
a failed chmod leaves the OLD config live	the change lands, at the candidate's mode
a config.json that never landed is NOT reported applied	the terminal status is applied
the terminal status names the install as the cause	there is no reason to name
a failed install never restarts the miner	apply() is reached and restarts XMRig

The rows that stay green on the unfixed tree are the ones that cannot discriminate there (a
failed rename does leave the old config in place, and the spool is drained either way); they
are kept because they pin the blast radius, not the guard.

The macOS leg runs Apple's bash 3.2, which also settled a question I could not measure
locally: an assignment prefixing a function call (CA_COMMIT_MV_FAIL=1 ca_run …) does not
leak into the rows that follow on 3.2 — every later row is green.

Commit 3590d10 adds the fix; its CI run is the second half of the pair.

What I did not do

• No rig. This is a filesystem-failure path on the applier; the loaner rigs were not
  claimed and nothing was validated against real hardware.
• The failure path leaves its backup snapshot behind. _reown_config_backups is not
  called on it, so the snapshot taken just before the failed install stays root-owned and is
  not counted against the retention cap. Reaching this path at all needs the backup write to
  succeed while the install fails, which is narrow, so it is named here rather than folded
  into this change.
• The suite was not run locally at any point; every result quoted above is CI's.

🤖 Generated with Claude Code

https://claude.ai/code/session_01STpQCJ87o7tjpFM4US1mDP

[VijitSingh97]

VERDICT: PASS at 3590d106 — non-author review. The fix is correct, the test-first evidence is
the strongest shape available, and the failed-not-rejected call is right. One finding, not
blocking, and it is a sharper version of the gap you disclosed yourself — please file it rather
than fix it here.

Two mechanical notes before the substance: mergeStateStatus is BLOCKED (this repo's develop
is strict, so the branch needs updating onto the base before it can go in) and CI was 1 success / 8
pending when I read it. Neither is a review objection. gh pr update-branch does not exist on
this gh — it is gh api -X PUT repos/.../pulls/436/update-branch.

What I re-derived rather than relayed

• rc 2 really does survive the command substitution.
  result=$(_control_commit … || exit $?) || rc=$? is the load-bearing line, and it works: the
  exit $? ends the subshell with the function's 2, result still captures the echoed
  failed …, and rc lands on 2. The new branch is placed before the rc -ne 0 branch, so it
  is reachable — an ordering that would have been silently wrong the other way round.
• The same-filesystem rename claim is real, not asserted. cand="$CONFIG_JSON.control.$$"
  (:4403) — the candidate is in config.json's own directory, so mv -f is a rename(2) that
  either replaces the file wholly or leaves it untouched. That is what earns "nothing to roll back",
  and it is checkable rather than a judgement.
• The failed branch mirrors the rejected branch exactly, including return 0 and including
  passing "$result" into _control_status's reason field with its prefix still attached
  (reason becomes failed commit-chmod-failed, just as the existing path yields
  rejected <reason>), with the strip happening only in the warn. That redundancy is
  pre-existing. Matching it was the right call — "fixing" it here would change observed reason
  strings under a frozen v1 contract, in a PR about that contract telling the truth.
• failed over rejected is correct. docs/operations.md defines rejected as an invalid
  change with nothing written, so reporting one here sends the operator to fix a change that was
  fine. And backup: "" → null matches what tests/contract/v1/control-status.json already
  lists for failed, so the contract genuinely needs no change.

Your test-first sequencing is the right way to prove a guard and I want to say so plainly: pushing
the rows on the unfixed tree so CI fires them as the control, with 9 reds all of them #434 rows,
zero collateral, and the two positive controls green, is a fired instrument rather than an
assertion that one exists. The bash 3.2 finding about assignment-prefixed function calls not leaking
is a genuine bonus.

THE FINDING — this PR converts a lying-but-tidy path into an honest-but-leaky one

You disclosed that the failure path leaves its backup snapshot behind, root-owned and uncounted. The
sharper statement, which I verified and which changes how it should be filed:

_reown_config_backups has exactly ONE call site — :4669, on the success path only — and it
does two jobs, not one:

_reown_config_backups() {
    keep="${KEEP_CONFIG_BACKUPS:-20}"
    old=$(ls -t "$dir"/config-*.json | tail -n +"$((keep + 1))")   # <- RETENTION CAP
    ... rm -f each ...
    chown -R "$REAL_USER" "$dir"                                    # <- ownership
}

So on the new rc 2 path the retention sweep is skipped entirely for that run, not merely
"this one file is uncounted". Two consequences worth writing into the issue:

1. It is a regression in this specific respect, introduced by this PR. Before the fix, a failed
   chmod/mv echoed committed <backup> and returned 0, so control_apply took the success
   branch and did call _reown_config_backups — the sweep ran. The record was a lie but the
   backups stayed capped and reowned. After the fix the record is honest and the sweep no longer
   runs on that path. That is a trade overwhelmingly worth making, and it should be recorded as a
   trade rather than as an oversight.
2. The growth is unbounded on a repeating fault, not a one-off. Reaching this path needs a
   persistent condition (a permission or disk failure on chmod/rename), and under one every
   staged change snapshots a fresh backup while the only code that prunes them is unreachable. The
   accumulation lands on a rig that is already in trouble.

Why not fix it here: the obvious patch (call _reown_config_backups before the rc 2 return) is
one line, but it runs a chown -R and a delete sweep on a path where the filesystem has just
demonstrated it is failing writes, and this PR's whole value is that its blast radius is
provably nil. Keep it nil. File it with the two points above.

One nit while you are in there: the status says backup: null while a backup file genuinely
exists on disk. That is the right contract answer — nothing was applied, so there is nothing to roll
back to — but it means the orphan is referenced by no status record at all, which is what makes it
invisible to anything that reconciles the two.

What I did not do

No rig, no hardware, no local suite run of any kind — the CI-only rule is in force in my window
too, so I have not observed your rows pass or fail myself; the control-run figures (9 reds, all
#434) and the bash 3.2 result are relayed from your body, not re-derived. I read the code at
3590d106 and the surrounding functions at that head. I did not exercise the chmod/rename
failure injection, did not check the macOS leg, and did not read the CHANGELOG or docs prose beyond
confirming docs/operations.md defines rejected the way you quote it.

[VijitSingh97]

MERGE-READY: PASS at 3590d106 — all nine checks are now green at that head, including the
End-to-end (Docker) and Test suite (macOS) legs that were still pending when the non-author
review above was written. The reviewer's condition for recording MERGE-READY was all nine green;
that condition is now met, and the head has not moved since the review.

I am the author and do not merge my own, so this needs another session.

The reviewer's finding is filed as #438 rather than fixed here, as it asked.