feat(ci): make an orphaned GHCR deploy fence diagnosable and recoverable - #3034
Draft
devantler wants to merge 5 commits into
Draft
feat(ci): make an orphaned GHCR deploy fence diagnosable and recoverable#3034devantler wants to merge 5 commits into
devantler wants to merge 5 commits into
Conversation
…elease The `reassert_flux_ghcr_auth` step ran under `always()`, which also fires after a job is cancelled. It acquires the `ghcr-auth-refresh` Lease, whose design deliberately has no automatic expiry takeover: Talos machine-config writes expose no fencing token, so a non-empty holder always requires explicit human recovery. A cancelled job is force-killed once the runner's post-cancellation grace expires, so the EXIT trap's release never lands and the Lease keeps a dead holder — after which every later deploy fails to acquire it. Merge-queue evictions cancel this job routinely, so the state is reachable in normal operation. Run the step on failure but not on cancellation; the next deploy's staging step performs the same reassertion with a full time budget. Adding a signal trap is not an alternative: a trapped TERM makes bash defer the handler until the in-flight child returns, which under a kill window is strictly worse than the current immediate EXIT-trap path.
The refusal messages told an operator to "prove the prior process is dead before explicitly recovering", but nothing shipped to do either half. The holder identity was `<revision>-<pid>-<random>`, and that PID belongs to a runner that no longer exists, so liveness was not decidable from it; recovery meant hand-writing a CAS-guarded patch against prod. Record the GitHub run and attempt in the identity. Both policy fences reuse the lease holder, so one change makes every fence resolvable against the API instead of by correlating timestamps across workflow runs. Add `--fences`: a read-only report naming every fence still held, its holder, its liveness evidence, and the exact CAS-guarded release command. It runs before any credential work, because an operator reaches for it precisely when a deploy will not start. It deliberately performs no mutation. Releasing a fence stays the operator's explicit step: a Talos machine-config write exposes no fencing token, which is the same reason automatic expiry takeover is refused. The gap being closed is the improvisation, not the human decision. Point both refusals at the report and at a new runbook scenario.
Contributor
✅
|
Contributor
Author
@coderabbitai review |
Contributor
|
|
Contributor
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
CodeRabbit reported @codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2d74c078f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…in for Codex found two P1 defects in the fence report, both confirmed against the source. The parent Kustomization never carries `reconcile: disabled` — only the child handoff does, because pause_flux_policy_parent writes the owner annotation and spec.suspend and nothing else. The generated release command emitted that test operation for both fences, so the parent's patch failed its own precondition and could never release the root Kustomization: the documented recovery was unusable for the more damaging of the two fences. Each branch now mirrors its resume_* function exactly. The node sweep keyed on the drain-recovery journal alone, but the ordinary per-node path claims cordon ownership with an empty journal, so a node killed there carries the owner annotation and no record. The report would answer "no fence is held" while that node stayed cordoned and the next run refused its owner — a false all-clear, the worst failure mode for a tool whose only job is to say what is held. It now sweeps the owner annotation independently and reports the cordon state with it. The regression test compares the operation set of each generated command against the release function it mirrors, rather than asserting on substrings: the defect was a mismatch between the two, so a mismatch is what has to be caught.
Contributor
Author
Both P1s from the previous round were confirmed against the source and fixed: the parent-fence release command no longer emits the @coderabbitai review |
Contributor
|
|
Contributor
Author
Both P1 threads are fixed and resolved at this head. CodeRabbit reports @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ace64a3f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Four findings from the second review round, all confirmed against the source. The report printed an unconditional `uncordon` for any unschedulable held node. A node can already be cordoned for maintenance or ill health before the bridge ever claimed it — which is exactly why the journal records the pre-claim state — so that command could reverse an intent this script never owned. It now uncordons only what the journal records this transaction as having cordoned, states plainly when a node was already cordoned, and says the state is unrecorded rather than guessing when there is no journal. The jq test uses `has` rather than `//`, because the alternative operator treats `false` as empty and would misreport the single state that may safely uncordon. Node fence owners were still minted from a revision plus PID, so the liveness check reported "no run reference" for precisely the fences the report exists to make decidable. Both node owners now carry the run reference, like the lease holder. The `--fences` failure path exited through the EXIT trap: under errexit a bare call to the report never reached the trap-disable, so a failed cluster read produced the same secondary cleanup errors the success path was already fixed to avoid — in the one situation an operator is reading this output. It runs as an `if` condition now, where errexit is suspended. A run blocked on the parent fence stops before the child-handoff refusal, so that refusal never reached the operator. It now carries the same recovery pointer.
Contributor
Author
All four findings from the last round were confirmed against the source and fixed; every thread is resolved. New head, so re-reviewing. @coderabbitai review |
Contributor
|
|
Code Coverage OverviewLanguages: Go Go / code-coverage/goThe overall coverage in commit 1600aa6 in the Show a code coverage summary of the most covered files.
Updated |
Contributor
Author
CodeRabbit reports All four findings from the previous round are fixed and their threads resolved: the parent-fence release command, the cordon-owner sweep, the errexit failure path, and the parent-fence refusal pointer. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cc2bdef75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Four more findings, all confirmed against the source. The cordon branch compared `wasCordoned` against the booleans `false` and `true`, but the journal serializes it with --argjson and validates it as numeric `== 0 or == 1`. Nothing ever matched, so every real journal fell through to UNRECORDED and the safe uncordon could never print — the previous fix was inert. Its test passed because the fixture used a `false` shape the code does not produce; the test now pins the numeric branches and asserts the schema they must agree with. A journal in `active` or `retain` is not releasable by removing annotations: reconcile_bootstrap_recovery refuses both, because one may hold an interrupted pre-reboot mutation and the other crossed the reboot edge with no release-ready proof. The report printed the removals anyway, which would discard the only durable record of that state and invite restoring an unverified node. Those phases now stop before the release block and point at bootstrap recovery. The Lease is the global exclusion fence and was reported first. Released before the fences it guards, it lets a queued or newly dispatched deploy start against a half-recovered cluster. It is now reported last, mirroring cleanup_refresh_work's own order, and the report states that ordering. A rerun reuses the run id, so the printed `gh run view` needs --attempt or it inspects the newest attempt: an orphan from a finished attempt reads as live while a later one runs.
This was referenced Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
When a deploy is killed part-way it leaves its fences held, and every later deploy then refuses to start. Today that happened twice in one afternoon and took two rounds of log archaeology to clear.
The refusal told the operator to "prove the prior process is dead before explicitly recovering" — but nothing shipped to do either half. The holder was
<revision>-<pid>-<random>, and that PID belongs to a runner that no longer exists, so liveness could not be judged from it. Recovery meant hand-writing a CAS-guarded patch against a prod resource, under pressure.It is also easy to fix the wrong thing: clearing the Lease unblocked the deploy only as far as the next fence, because the same dead process had also left
infrastructuresuspended — silently stopping GitOps for that layer.What
--fencesreport lists every fence still held, its holder, its liveness evidence, and the exact CAS-guarded release command. It is read-only and runs before any credential work, because an operator reaches for it exactly when a deploy will not start.It performs no mutation, on purpose. Releasing a fence stays the operator's explicit step — a Talos machine-config write exposes no fencing token, which is the same reason automatic expiry takeover is refused. What is being removed is the improvisation, not the human decision.
Fixes #3033
🤖 Generated with Claude Code