Repository navigation
Stop stale receivers when control replacement fails - #640
Conversation
|
Operator: hold lifted so the runner's reviewer takes this draft. It fixes a confirmed finding from the operator's RigForge audit; bugs go into the next patch release (v1.18.1, cut from develop). It merges only after the agent review passes and the owner approves at the exact head. |
VijitSingh97
left a comment
There was a problem hiding this comment.
Reviewed by Hermes agent (operator's delegate) against origin/develop b82758b: full diff read, make test-suite run per PR, body claims reproduced. Not a substitute for the owner's approval; merge stays with the operator.
Verdict: RETURN (mandatory rebase onto b82758b; design otherwise sound)
Security design verified: receiver stopped before units/credentials change, errors propagate, EXIT trap re-stops the receiver, firewall reconciliation completes before surfacing failure, Python receiver signals readiness only after loading credentials and binding. All three body-claimed pre-fix failures reproduced against develop; full suite SUITE_EXIT=0 here (the author's 10 Linux startup-timing failures did not reproduce).
Rebase required — b82758b rewrote the same apply() tail. Two content conflicts, both in rigforge.sh (tests/run.sh auto-merges):
- ~:4207 — HEAD
local _wd_rc=0 _api_rc=0 _control_rc=0ordering vs develop's local-before-restart reorder; keep develop's order, add_control_rcto the declaration. - ~:4232 — keep both: the restart-reason
:=first-failure-wins idiom, the control reason block, and a final gate over_wd_rc/_api_rc/_control_rc/RIGFORGE_RESTART_FAILED.
After rebase re-run the suite and b82758b's new restart-rejection case intests/test-sister-reconcile.sh; add one assertion that restart+control failure keeps the firstRIGFORGE_APPLY_FAIL_REASON.
Notes: error inside the install_control subshell (rigforge.sh:1451,1467) is now non-fatal to the outer command (fail-closed but silently softened; no test covers it); READY=1 send unguarded against stale NOTIFY_SOCKET (util/control-server.py:392-397); make test-suite now needs python3 unguarded (Makefile:24) — fine for the pinned CI image.
Part of #626. Implemented by an operator-run gpt-6-astra session at the owner's request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
44ccc4d to
3d5546e
Compare
|
Rebased onto develop b82758b and resolved both apply() conflicts per the review recipe: develop's local-before-restart order kept with |
|
Operator review of the rebased head (not an approval): Lint fails on the file budget ( |
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
FAIL What I investigatedFAIL. The design fixes the reported hole: the stale receiver is stopped before replacement, failures propagate, the firewall step still runs, and readiness uses
Reviewed head: What are the gaps
|
The control-disabled teardown named the remote-upgrade path unit even when control_upgrade was off, so systemctl disable failed and the teardown returned before removing the unit files. Cover the absent-upgrade and the failing-disable cases in the stub suite, and enable then disable control on real systemd in the stock-home job. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
PASS What I investigatedRound-2 review of PR #640 (resolves #626) at head 164a1b5, covering the delta from 83fdd91 (cef9b66, bebd497, 609c568, 164a1b5). Every round-1 finding is fixed or answered with evidence that holds. The new tests fail without the fix and pass at the head. CI at the head is all green, including a real-systemd stage that enables control and then tears it down. Bench tier4-e2e job 3519 on 609c568 passed the control phase, and the only later change is one line of a test the bench does not run.
Reviewed head: |
Owner brief for
|
Co-Authored-By: Claude Code <noreply@anthropic.com>
|
PASS What I investigatedRe-review of #640 (closes #626, stale control receiver kept accepting a revoked bearer) at head 20990de. Reviewed the worker's commits since 83fdd91 (cef9b66, bebd497, 609c568, 164a1b5) and the develop merge. Both round-1 return findings are fixed. The disabled-teardown and leftover-upgrade-service fixes each go red with the old hunk and green at the head. Real systemd covers the enable-then-disable control stage at the head, and the tier4-e2e control job 3519 covers 609c568, whose code outside the tests is the same as the head's.
Reviewed head: |
Owner brief for
|
# Conflicts: # Makefile
|
PASS What I investigatedRound 4 re-review of PR #640 (closes #626) at head 6cb1915. 83fdd91 is in the clone. The worker's own commits since that review are cef9b66, bebd497, 609c568 and 164a1b5; the other commits in the range came in through two develop merges (20990de, 6cb1915). Both round-1 returns are fixed. The disabled-control teardown now names only units that exist, and each new teardown hunk has a test that fails with that hunk reverted and passes at head. Real-systemd proof is in place: the Stock home (current) check exercises the control stage at head, and tier4-e2e job 3519 covers connect, control and access-tokens. The PR's own product hunks are byte-identical between 609c568 and head. All 10 GitHub checks are green at head, and the topology scan is clean.
Reviewed head: |
Owner brief for
|
What's the problem
Closes #626
When a named bearer was revoked and the control receiver could not be replaced, the old receiver process kept the token it had loaded at startup. It went on accepting payout writes that the root consumer applied.
install_controlsuppressed stop, enable and restart failures, andapplydiscarded control reconciliation failures.How I fixed it
_stop_control_receiverstops the receiver before any unit or credential change. If systemd rejects the stop, it kills the exact unit and stops it again. A missing unit is benign only when it is also inactive. If both stops fail, it warns that revocation is not confirmed and returns failure.install_controlruns in a subshell with an EXIT trap that stops the receiver whenever the function fails. Every render, reload, enable and restart step returns failure.error()(for example "needs python3") now ends onlyinstall_control. The caller records_control_rc=1, setup goes on, and the trap stops the receiver.apply, this also means control-apply and remote upgrade roll back when the receiver cannot be reconciled.Type=notifywithNotifyAccess=mainandTimeoutStartSec=15.util/control-server.pysendsREADY=1only after it has loaded credentials and bound its listener.control_upgradewas off, so real systemd failed the disable and the unit files survived. A leftover upgrade service without its path unit is also removed without naming the absent unit.docs/pithead-integration.mdstates where the revocation boundary sits:install_service's unguardedsystemctl daemon-reload._apply_runtime,applyand the control server's__main__stay removed.rigforge.sh(6303) andutil/control-server.py(400) are exactly at their file-budget ceilings.Here is the proof
Red, then green. Each check below was run on this Mac with the named old code swapped in, then restored:
python3 tests/control-revocation.py "$PWD"with develop'srigforge.shexits 1 atassert stale is None: the stale receiver accepted the revoked bearer's write. The stub rejects the firstsystemctl stop(the issue's trigger) and makeskillSIGKILL the receiver. The control is a fresh receiver that returns 401 for the revoked bearer and 202 for the master. At head it exits 0.bash tests/test-control-revocation.shwith cef9b66'selifline fails with "a leftover upgrade service without its path unit failed the control reconcile".Local, at head:
bash tests/test-control-revocation.sh,python3 tests/test-control-readiness.py,bash tests/test-sister-reconcile.sh,FILE_BUDGET_REQUIRE_BASE=1 bash scripts/lint-file-budget.sh(file budget OK), andshellcheck --severity=warningon every shell file passed. Formatting is proved by the pinned CI job: "Lint (shellcheck + shfmt)" is green on each head (the local shfmt here is older than CI's 3.14.1 pin and only disagrees on lines byte-identical todevelop). Not run locally:make test, the full suite (GitHub's Test suite job runs it), and the container e2e (this Mac runs no Docker).Real systemd:
tests/test-stock-home.sh(GitHub "Stock home setup (current)", ubuntu-24.04) now runsapplywith control enabled and remote upgrade off. It asserts that the receiver is active asType=notifyand answers its token on 8082. It then disables control, asserts the "Writable control path disabled" teardown, and checks that the receiver is inactive and its unit files are gone. On head 164a1b5 the "Stock home setup (current)" job passed with the enable-then-disable control stage. It asserts the teardown by state: receiver inactive, unit files removed. An earlier head asserted apply's log line, whichapplyhides, and that failed; the assertion is removed.Base merge at 6cb1915: this branch has merged
developtwice to resolve the base conflict, at 20990de and again at 6cb1915 to take in #641 (the worker-proof harness). Both times the only conflict was the Makefiletest-suitelist, where both sides had added test entries; each resolution keeps both. The resolved entriespython3 tests/test-control-readiness.py,python3 tests/test-worker-proof.pyandbash tests/test-worker-proof.shwere each run on the merged head and pass, andFILE_BUDGET_REQUIRE_BASE=1 bash scripts/lint-file-budget.shis OK. The control-teardown code this issue changes is byte-identical to 609c568 below; the merges add only develop-side code (upgrade throttle state, thermal watchdog retries, HugePages ceiling, worker-proof harness) that develop already proved. All ten GitHub checks pass on 6cb1915: Lint (shellcheck + shfmt), Lint (markdownlint), Lint (yamllint), Test suite, End-to-end (Docker), Coverage (kcov + diff-cover), Stock home setup (current and historical), Secret scan (gitleaks), Workflow audit (zizmor).Bench, tier4-e2e
connect,control,access-tokens, job 3519 on 609c568. The head moved to 164a1b5 afterwards, and the only change since wastests/test-stock-home.sh, which tier4 does not run; the base merges add no change to the code this job proves:bench-ci job 3519@609c568fbd6f2e38ffee5d5ed25d61bd734e7518 · rigforge · tier4-e2e
Generated by
bench-ci evidence 3519@609c568fbd6f2e38ffee5d5ed25d61bd734e7518; re-run it to check.Ponytail pass: I cut the no-systemd shortcut I had added; it bought no supported host and could hide an orphaned receiver. The stale-spool and abort-before-control windows are documented, not closed, because they are outside the replacement-failure path this issue names.
🤖 Generated with Claude Code