fix(image-refresh): re-pin the digest when a helm re-render reverts the workload to :tag

[saqlainsyed007]

DRAFT for discussion — fix A of A+B for the image-drift class found during the RFC-0003 D6 egress rollout. Owns the image-refresh side. The companion B (helm re-render never reverts the digest) is described at the bottom; happy to open it as a second draft.

Refs tracebloc/client-runtime#199

The bug (the script already documents it)

image-refresh re-pins a control-plane image only when the registry digest changes (recorded != latest). But the header's own note (case 1, "HELM RE-RENDER") acknowledges that helm upgrade --reset-then-reuse-values — the hourly auto-upgrade — re-renders the Deployment back to repo:tag and discards the live set image repo@digest pin, and that the script "will NOT re-pin: the annotation still records that digest, so recorded == latest… converges … on the next digest change."

That "next digest change" can be never. In the meantime the workload sits on the bare :tag (IfNotPresent per tracebloc.controlPlanePullPolicy), and on a node whose :tag layer is stale, it silently runs an old image. Concretely: a pre-#416 jobs-manager ran under the already-sealed egress netpol on the stg/prod fleets (HF-offline flags silently not injected) until I hard-pinned the digest in values. dev didn't hit it (its node pulled fresh), so it's cache-dependent and non-deterministic.

The fix (this PR — "A")

In the recorded == latest branch, don't unconditionally no-op: read the live workload image (workload_image_for_repo) and re-pin when it isn't repo@latest. So the tick after any helm re-render restores the digest, instead of waiting for the next registry publish. Bounds the exposure to ≤ one interval (~15 min).

• No new RBAC — reads only what the SA already reads (get_annotation already gets the Deployment in the release ns; the node-agents Role is */*/* for the DaemonSet). Respects the backend#2469 "NO NEW READS" bootstrap-lockout rule.
• shellcheck -S warning -x clean on the rendered script; helm-unittest guard added (38/38 pass); Chart.yaml bumped 1.9.107 → 1.9.108.

Design decision to confirm (in the code comment too)

This also converges a fresh install to a digest pin on the first tick where the workload is still on :tag — one extra rollout ~one interval post-install, a deliberate widening of the header's "first-tick contract." That contract avoided install churn, but by staying on :tag it left exactly the steady-state stale-:tag exposure this closes. If the install-churn cost is unwanted, gate the re-pin on a per-image "have we ever applied a digest here?" marker so it fires only on genuine reverts — I can switch to that; it trades immediacy on already-pinned fleets for a clean install.

Not this PR — the companion "B" (fully closes the window)

A alone bounds the window to one tick; it doesn't eliminate it. B makes helm upgrade never revert in the first place — e.g. the Deployment/DaemonSet templates seed the image digest from the tracebloc.io/last-refreshed-*-digest annotation via helm lookup, so a re-render renders repo@<current-digest> (empty on fresh install/CI → bare tag, fine). Bigger blast radius (lookup is empty during helm template/diff), so it's worth its own review — say the word and I'll draft it.

Explicitly NOT changing the pull policy

Always-when-unpinned would re-break #569/#705 (offline Docker/WSL restart → ImagePullBackOff). The fix is coordinating image-refresh's digest with the helm re-render, not the pull policy.

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes edge control-plane rollout behavior (jobs-manager Recreate, resource-monitor DaemonSet flap path) on fresh installs and after chart bumps; incorrect live-image logic could cause extra rollouts or miss stale tags on cached nodes.

Overview
Fixes image-refresh so control-plane workloads are not left on a bare :tag after fleet helm auto-upgrade re-renders deployments and drops an earlier kubectl set image digest pin (backend#199 / client-runtime#199).

When the recorded registry digest matches the latest, the cron script now reads live deployment/daemonset image refs (workload_image_for_repo, requests_proxy_image) and re-pins if the spec is not on that digest—bounded to about one cron interval (~15m). Digest comparison uses the @sha256 suffix so registry-prefix mirrors are not treated as reverts. requests-proxy is checked alongside jobs-manager api so partial re-pins cannot look “unchanged” forever. ATTEMPT_KEY handling retries unfinished rollouts when the spec already shows the digest but rollout never settled, without forcing retries once flap lockout has latched.

Docs/comments and values.yaml describe the widened first-install path: record on first tick, pin on the next when still on :tag. Chart 1.9.109; helm-unittest regex guards plus new bats (image-refresh-repin-on-revert.bats) assert behavior, not just script text.

^{Reviewed by Cursor Bugbot for commit ea89c76. Bugbot is set up for automated code reviews on this repo. Configure here.}

[saqlainsyed007]

bugbot run

[cursor]

Stale Bugbot comment from a previous run.

[saqlainsyed007]

bugbot run

[cursor]

Stale Bugbot comment from a previous run.

[LukasWodka]

Read the whole script around the change, the auto-upgrade script, the RBAC, the templates' container names, and kubectl's set image no-change path. The mechanism is correct and I'd ship A without waiting for B. Two asks before I approve, one design answer since you asked for one, and one question.

Design decision: keep the fresh-install widening. The second-tick pin is not only churn — it also fixes a reinstall onto a node whose :tag layer is already stale: under IfNotPresent that install runs the old image until the next registry publish, which is the same class this PR closes. The "ever-pinned marker" alternative would leave that case open. One extra rollout ~15 min post-install (jobs-manager is Recreate, and the resource-monitor DaemonSet rolls every node) is a fair price for an edge that is reproducible from t+15m — but it must be written down as the new contract (ask 2), because today the header promises the opposite.

Thrash check (for the record): no hourly loop. auto-upgrade only runs helm upgrade when a new chart version is published (current == latest → exit 0), so the revert happens once per chart release, and A adds one rollout ≤15 min after it. Steady state is a true no-op: kubectl set image with an unchanged ref produces an empty patch and no rollout. Worth a sentence in the body — "the hourly auto-upgrade" reads as an hourly revert.

Question — persistent mismatch. The comparison is exact string equality against ${IMAGE_REGISTRY}/${repo}@${latest}. On a cluster with a mutating admission webhook that rewrites image refs to an internal mirror (not unusual behind hospital proxies), have never equals want: every tick re-pins, the webhook rewrites again, the generation bumps, and after three ticks the #563 flap lockout freezes refresh for all images — where before this PR the same cluster re-imaged only on a real digest change. Do we know no customer cluster does this? If not, either normalise the registry prefix before comparing, or treat "still != want on the tick after a re-pin" as "environment rewrites refs, stop" rather than as another attempt.

Ask 1 — a behavioural test, not a text-presence guard. The new helm-unittest asserts check that workload_image_for_repo(), have="$(…, proxy_off_digest and the no-op log line appear in the script. Invert the comparison and they all still pass. This repo already has the harness for exactly this script — scripts/tests/image-refresh-stale-pin.bats / image-refresh-skip-streak.bats extract the rendered branch and stub get_annotation/kubectl. Please add the same for this branch: (live :tag, recorded == latest) → falls through with restart_needed=1; (api and proxy both @latest) → "no-op"/continue; (api @latest, proxy :tag) → falls through with the proxy line; (unreadable) → whatever you decide below. || return 1-hardened like the others.

Ask 2 — the header now contradicts the code. "TWO KNOWN, BOUNDED LIMITATIONS → 1. HELM RE-RENDER" still says "This tick will NOT re-pin … the loop no-ops", and the "First-tick contract" paragraph still says a fresh edge "runs repo:tag until the first real digest change"; values.yaml's "exits without touching anything" bullet says the same. All are false after this PR, and the header is what the next reader trusts. Rewrite (1) as bounded to one tick, the first-tick paragraph as "pins on the tick after first observation", and drop the "DESIGN NOTE (for review) … see PR discussion" block from the shipped ConfigMap once the decision above is taken — a customer operator cannot follow a PR thread.

Two small ones, no need to re-request:

• The helper comment says "fail-safe, same stance as get_annotation" — get_annotation is fail-closed (non-zero → do not assume absent, skip). This helper is fail-open (empty → re-assert). Fail-open is acceptable here because the re-assert is idempotent and rolls nothing when already pinned, but a read failure then logs "a helm re-render reverted the pin" and burns an attempt on a healthy edge; skipping the image for that tick with a "could not read live image" line (the stance the SKIP_KEY read already takes) would be cleaner. Say which you chose.
• "-- a helm re-render reverted the pin onto :${IMAGE_TAG}" will print on every fresh install's second tick, where nothing reverted. Neutral wording: "workload is not on the pinned digest (fresh install, or a helm re-render reverted the pin)".

Verified along the way: container names match the templates; RBAC already grants get on the deployment and the resource-monitor DaemonSet; requests-proxy is rendered unconditionally so the proxy read can't hit NotFound; the Bugbot partial-re-pin fix is correct. No manifest or copy-catalog regen applies to this template.

[LukasWodka]

Two more things surfaced after I posted, both worth folding into the revision:

1. A failed re-pin is one-shot and leaves the attempt counter stuck. set image mutates spec.template before rollout status confirms. If the DaemonSet rollout times out on one slow node, set -e exits before the ATTEMPT_KEY-/FLAP_KEY- reset — and on the next tick have already reads repo@sha, so the loop logs "already on the pinned digest; no-op" while the rollout is still stuck and ATTEMPT_KEY=1 persists until the next real digest change (which then gets 2 attempts, not 3). The digest-change path never had this because recorded != latest kept retrying. Cheapest fix: on the re-pin path, treat have == want as "pinned" only when the workload's rollout is also settled (the top-of-tick settled guard checks jobs-manager only), or don't count a re-pin toward ATTEMPT_KEY at all.

2. Helm 4 server-side apply conflict on installer-run edges. The installer pins Helm 4 (scripts/lib/common.sh), which applies server-side; kubectl set image records field manager kubectl-set on each container .image. Post-PR every edge is kubectl-set-owned on all four images within ~30 min of install and re-owned ≤15 min after every revert. An operator who then re-runs install-k8s.sh --force or any helm upgrade from the host's Helm 4 hits conflict occurred while applying object … conflict with "kubectl-set" … containers[name="api"].image — the exact shape docs/MIGRATIONS.md §"limits.cpu" already documents, and its "the durable fix is to stop the out-of-band patch" line now describes image-refresh itself. Pre-PR a fresh edge stayed unpinned for days and any pin self-cleared at the next Helm-3 revert, so this re-run was clean. Please confirm whether the stored apply method survives the Helm-3 auto-upgrade; if it does, either --force-conflicts becomes mandatory on every operator upgrade (document it) or the pin needs a shared field manager. This is the strongest argument for companion B (stop the revert at the render) over repairing it afterwards.

Neither changes my verdict — both are yours to weigh in the revision.

[saqlainsyed007]

Thanks @LukasWodka — all four addressed in 461eeaa.

Ask 1 — behavioural test, not text-presence. Added scripts/tests/image-refresh-repin-on-revert.bats, modelled on image-refresh-stale-pin.bats: it extracts the shipped recorded == latest branch from the rendered chart and drives it with the registry HEAD and both live-workload reads stubbed. Cases: :tag revert → re-pins (restart_needed=1); api+proxy both on digest → no-op; api on digest, proxy on :tag → re-pins the proxy line; unreadable api / unreadable proxy → skip this tick; registry-prefix rewrite → not a revert. Inverting the comparison reddens these. 6/6 green; the helm-unittest presence-check now points at the bats file and pins proxy_on_digest instead of the renamed proxy_off_digest.

Question — persistent mismatch under a mutating image webhook. Taken the robust option: compare on the @sha256 digest (${have##*@} vs latest), not the full ref. A webhook that rewrites the registry prefix keeps the digest, so a prefix-only rewrite no longer reads as a revert — which is what would have re-pinned every tick and tripped the #563 flap lockout for all images after three ticks. A genuine :tag revert has no @sha256 and still re-pins. Covered by the mutating webhook bats case.

Nit (fail-open vs fail-closed). Chose skip-on-unreadable: an unreadable live image now skips the re-pin this tick and retries, rather than re-asserting (which burned a flap attempt on a healthy edge and logged a revert that may not have happened). Both helper comments now say fail-closed, matching get_annotation/the settled guard. The revert log is neutral wording (fresh install, or a helm re-render reverted the pin) so it no longer misreports a fresh install's second tick.

Ask 2 — header ↔ code. Rewrote the HELM RE-RENDER limitation as handled, bounded to one tick; the first-tick contract as "pins on the tick after first observation"; and the two matching values.yaml bullets. Dropped the DESIGN NOTE (for review) block from the shipped ConfigMap.

Verified: helm template renders, helm unittest 38/38, the three image-refresh bats suites 36/36, shellcheck -S warning -x clean, sh -n OK.

[saqlainsyed007]

bugbot run

[cursor]

Stale Bugbot comment from a previous run.

[saqlainsyed007]

bugbot run

[cursor]

Stale Bugbot comment from a previous run.

[LukasWodka]

Re-reviewed at 74f1a14. Ran the new bats file locally (8/8), then mutated the script six ways — inverted the api digest comparison, dropped the pending-attempt retry, dropped the proxy digest check, removed both unreadable-skips, and compared the whole ref instead of the digest — and each mutation reddened exactly the test written for it. helm unittest 696/696, the two sibling image-refresh suites 30/30, shellcheck on the rendered script clean. No installer script under scripts/ changed, so no manifest regen applies.

Closed from my last pass:

• Behavioural test — yes. Extracted from the rendered chart, stubbed live reads, || return 1 throughout. Exactly the shape I asked for.
• Header/values.yaml now match the code; the review-only design note is out of the shipped ConfigMap.
• Digest-only comparison (${have##*@}) handles the mirror-prefix rewrite case; bats case 8 pins it.
• The stuck ATTEMPT_KEY case is handled by re-entering the rollout when the counter is raised; bats cases 3/4 pin both sides.

Still open — the one thing between this and approve:

• Helm 4 server-side-apply field ownership. No change or reply on this. The installer pins Helm v4.2.3, whose --force-conflicts is opt-in, and after this PR every edge has kubectl-set owning all four .image fields within ~30 min of install and again ≤15 min after every re-render. Please confirm whether the stored apply method survives the in-cluster Helm 3 auto-upgrade; if it does, either document --force-conflicts as mandatory on operator-run helm upgrade/--force reinstalls (a line in docs/MIGRATIONS.md next to the existing limits.cpu case is enough), or move the pin to a shared field manager. Not asking for companion B here — just the confirmation and the doc line.

Nits, no re-request needed:

• The new comments, values.yaml text, bats header and the three latest commit subjects cite issue 199 under a different repo than the PR body's Refs line, the original commit and the pre-existing comment near workload_image_for_repo. One of them is wrong — pick one.
• Worth one header sentence: with the pending-attempt retry, a DaemonSet rollout that can never settle (a permanently NotReady node) reaches the flap lockout ~45 min after install rather than at the next digest change. Same class the digest-change path already had, just earlier — I think that is the right trade, but say so.
• The pending_attempt read and its numeric sanitisation sit above the loop, so the harness injects the value rather than exercising the read; fine for now.
• PR body still says "DRAFT for discussion" and "1.9.107 → 1.9.108"; it is 1.9.109 now.

Four checks were still pending when I read the gate (Cursor Bugbot, bugbot / review, Prereqs — rockylinux:9, mutation-check). Approval follows once they are green and the field-manager question has an answer.

[saadqbal]

Mechanism's right and I'd ship this independent of #1013. Traced it end to end: set image writes the pin, the auto-upgrade re-render drops it back to :tag, and the recorded == latest branch now reads the live spec and falls through to the re-image path — one tick of exposure instead of "until the next publish". I extracted the shipped branch from this head and drove it for pods-monitor and resource-monitor as well as jobs-manager, and all three re-pin correctly, so the class is covered rather than one workload short. The bats file is the real thing too — it drives the rendered branch with the reads stubbed, so an inverted comparison reddens.

One thing I'd want closed before I approve, and it's Bugbot's still-open thread rather than something new. The fresh-install widening routes every new edge through the shared #563 flap path, and the resource-monitor rollout status needs every node Ready. One permanently-NotReady node times out three consecutive ticks, FLAP_KEY latches, and refresh stops for all control-plane images with no auto-resume — the exact hazard the activeDeadlineSeconds comment further down already calls out as "looks healthy from outside". Pre-PR that edge just sat on :tag until a real publish, so this is a new way to wedge an install on day one. I think the trade is defensible, but it needs the header sentence Lukas asked for plus an operator-facing line on clearing tracebloc.io/refresh-attempt, because right now that remediation exists only inside a log message on an edge that has already stopped refreshing. Not counting a re-pin toward ATTEMPT_KEY would also close it.

Smaller, same root: with the digest stable, restart_needed is now 1 on a flap-latched edge, so the tick exits at the flap branch before the annotate_args write — the stale-pin-* bookkeeping quietly stops being written or cleared. Cosmetic next to the above, but silent.

And #1013 bumps Chart.yaml too, so whichever lands second conflicts there. Worth picking the order now rather than at merge.

[shujaatTracebloc]

Reviewed at 74f1a14. The mechanism is right and I agree with @LukasWodka and @saadqbal that A is worth shipping independently of #1013 — recorded == latest proving only that the REGISTRY moved, never that the workload runs it, is a real class of bug and reconciling from the live spec is the right shape. The bats file is the genuine article: it extracts the branch from the RENDERED chart and stubs the live reads, so an inverted comparison reddens, and every standalone assertion carries || return 1. Comparing on ${have##*@} closes the mirror-webhook thrash cleanly. I am requesting changes on three things in the new code, one of which I do not think anyone has raised yet.

Verified before writing this: chart guard satisfied (version AND appVersion both 1.9.109, in lockstep; both gate checks green). The single non-green check is Helm unit tests CANCELLED on run 34341907087 — superseded 20s later by run 34341939853 on the SAME head sha, which passed. gh pr checks (deduped) shows it as pass. That is rollup noise, not a red; it is not blocking from my side. Same artifact on #1013, whose rollup also reads FAILURE with zero real failures.

BLOCKING 1 — the forced retry is a no-op exactly when it claims to act. (New.) The pending_attempt > 0 branch logs "re-running the rollout to resolve it or surface it via the flap guard" (line 670), but once ATTEMPT_KEY >= MAX_REFRESH_ATTEMPTS the guard at line 754 annotates FLAP_KEY and exit 0s BEFORE any set image / rollout status runs. So in the latched state it neither resolves the unfinished rollout nor surfaces anything new — it only forces restart_needed=1 and suppresses the rest of the tick. Please gate it as [ "$pending_attempt" -gt 0 ] && [ "$pending_attempt" -lt "$MAX_REFRESH_ATTEMPTS" ]. That single change also fixes blocking 2.

BLOCKING 2 — a latched flap now permanently suppresses the annotation write. @saadqbal called this "cosmetic but silent"; I read it as worse than cosmetic. pending_attempt is read once at line 480 and never reset in the loop, so while a flap is latched every tick takes the forced branch, sets restart_needed=1 at line 690, and exits at line 754 — which is BEFORE the if [ -n "$annotate_args" ] write at line 836. Dropped every tick, forever: the first-observation last-refreshed-<image>-digest record (so an image on first observation re-does first observation every 15 min) and every tracebloc.io/stale-pin-* clear or write (so an operator who removes a stale values pin never sees the finding clear). FLAP_KEY is also re-annotated ~96x/day. Pre-PR these ticks had restart_needed=0 and the annotate block ran, so this is a regression this PR introduces, not a pre-existing one. Either take the fix in blocking 1, or move the annotate_args write above the restart block.

BLOCKING 3 — the fresh-install widening, with the numbers. This is the open Bugbot thread and @saadqbal's point, and I land on the same side, but the PR body's "one extra rollout ~one interval post-install" understates it and should be corrected before merge. I checked the two templates: resource-monitor-daemonset.yaml is RollingUpdate / maxUnavailable 10% with tolerations: [{operator: Exists}], so desiredNumberScheduled counts every node including NotReady and cordoned ones and rollout status can never settle on a fleet with one dead node — three ticks (~45 min after install, default 15m schedule) latch the SHARED MAX_REFRESH_ATTEMPTS=3 lockout and stop refresh for ALL control-plane images, with no auto-resume, while the CronJob stays green. And jobs-manager-deployment.yaml is strategy: Recreate with a 60s grace period, so the extra rollout is full jobs-manager downtime plus wait-for-mysql, for byte-identical content, on every fresh install and again after every chart-version bump. Combined with blocking 2, a brand-new edge can wedge on day one and then silently stop recording anything. I am not asking you to reverse the design decision — Lukas's argument about the reinstall-onto-stale-:tag case is good and I agree the widening earns its keep. I am asking for either the "have we ever applied a digest here?" gate you offered, or: the header sentence Lukas asked for, an operator-facing line on clearing tracebloc.io/refresh-attempt (that remediation currently exists only inside a log message on an edge that has already stopped refreshing), and not counting a re-pin toward ATTEMPT_KEY.

Minor — workload_image_for_repo has no *) arm. An unmatched case returns empty with status 0, and the new caller reads empty as "unreadable" and continues while logging "API read error / container absent". So a fourth repo added to the set -- list, or a container renamed in a template, silently kills the re-pin for that image on every tick while the log blames the API. The comment says the names are "contractual ... keep in sync" but nothing enforces it. Suggest *) log "BUG: no live-image reader for $1"; return 1 ;;.

Nits, no re-request needed:

• Lines 310 and 601 both say "the hourly auto-upgrade", but line 67 of this same file says "not an hourly revert" and auto-upgrade-cronjob.yaml:232 exits 0 at "already at latest" behind the chart-version gate. A reader taking the new comments at face value would size the blast radius as two Recreate rollouts an hour. Same wording issue Lukas flagged in the body.
• Still backend#199 in five new places (:57, :102, :466, values.yaml:1723, the bats header) and in the last three commit subjects, while the body and the pre-existing comments say client-runtime#199. Lukas raised this; still open. The closing-ref gate itself is fine — the body ref is owner-qualified and uses "Refs" with no closing verb, and set-status / closing-ref is green.
• PR body still says "DRAFT for discussion" and "1.9.107 -> 1.9.108"; it is 1.9.109.

One question, and I want to be upfront that I could NOT verify it here — I have no cluster and no k3d in this pass, so this is a mechanism argument, not an observation. The mirror gate (IMAGE_REGISTRY != docker.io -> exit 0) correctly makes the reconcile inert on mirrored and airgapped installs, so those never get pinned — good, and the right fail-closed call. And a node that genuinely pulled from docker.io by tag HAS that digest in containerd, so repo@digest under IfNotPresent is satisfiable. The case I am unsure about is a SIDE-LOADED image (a locally built image seeded with k3d image import / ctr images import) on a cluster left at global.imageRegistry=docker.io: the store holds a local manifest under a tag alias, not docker.io's digest, so the pinned ref is not resolvable and IfNotPresent falls through to a pull — ImagePullBackOff if the edge is offline. Pre-PR that was reachable only when docker.io actually published; post-PR the second tick makes it deterministic ~15 min after every fresh install. Scope-limiting fact in your favour: k3d image import appears nowhere in this repo, so it is not a documented install path and this may be purely a local-dev / local-e2e concern rather than a customer one. Can you confirm that no supported install seeds control-plane images out-of-band without setting global.imageRegistry? If the local dev loop does, one sentence in the header is enough.

On #1013: they compose rather than contradict, and I would land A first. B renders the digest from the last-refreshed-* annotation, and in A's recorded == latest branch the annotation equals latest, so A reads on-digest and no-ops — no fight. Two notes for ordering. First, the Chart.yaml numbers are already sequenced (this PR 1.9.109 from 1.9.108, #1013 1.9.110 from 1.9.108), so there is no version clash, just a textual conflict on that one line for whichever lands second — trivial rebase, not the problem @saadqbal's note implies. Second, and more substantive: Lukas's still-open Helm 4 server-side-apply field-ownership question gets strictly WORSE with B landed, because helm then actively renders a value for the same .image field that kubectl-set writes, instead of leaving it at :tag. Worth answering that before B rather than after.

Not blocking on my side: the superseded-run CI noise, and the pre-existing-skew follow-up.

[saqlainsyed007]

Addressed all three in ea89c76.

Blocking 1 & 2 — fixed with the one gate you named. The forced-retry is now [ "$pending_attempt" -gt 0 ] && [ "$pending_attempt" -lt "$MAX_REFRESH_ATTEMPTS" ]. Once ATTEMPT_KEY latches (>= MAX), the branch falls to the no-op path instead of forcing restart_needed=1 → the tick no longer hits the flap-guard exit 0 before the annotation write, so first-observation records and stale-pin clears land every tick again (not dropped forever). New bats case on-digest with a LATCHED attempt (>= MAX) is a no-op, not a forced re-run pins it (feeds pending=3, asserts no-op + RESTART:0 + no "unfinished re-image"); test 3's UNFINISHED case (pending=2 < MAX) still re-runs. 9/9. I also had to define MAX_REFRESH_ATTEMPTS=3 in the bats harness — it lives at the top of the script, outside the extracted slice, so the new < MAX reference needed it.

Blocking 3 — took the header route you offered. The first-tick-contract header now states the cost honestly: the re-pin enters the shared #563 flap path, rollout status on the resource-monitor DaemonSet counts every node (tolerations Exists) so it can't settle on a fleet with one NotReady/cordoned node, three ticks (~45 min) latch the shared lockout for ALL control-plane images, and jobs-manager is Recreate (full downtime for byte-identical content). It also names the "have we ever applied a digest here?" gate as the follow-up that would skip the flap path on a genuine fresh install — happy to do that as its own PR if you'd rather have the fix than the documented cost; I kept it out of this one to avoid a new "applied" marker + logic on customer infra without your and Lukas's eyes on the design.

On the Helm unit tests CANCELLED: agreed, rollup noise (superseded run on the same sha passed). shellcheck -S warning -x clean, bash -n clean, chart 1.9.109 (> develop 1.9.108), gate checks green. Re-requesting.

[saqlainsyed007]

bugbot run

[shujaatTracebloc]

Withdrawing one thing I said in my review above, having now reviewed B (#1013) properly. My change-request on this PR stands unchanged — findings 1 and 2 are unaffected.

What I got wrong: I said Lukas's Helm 4 server-side-apply field-ownership question "gets strictly WORSE with B landed, because helm then actively renders a value for the same .image field that kubectl-set writes, instead of leaving it at :tag."

The premise is false. repo:tag is a rendered value for .image — helm has always declared and owned that field, and that ownership is exactly why the client-runtime#199 revert happens in the first place. B changes the value helm writes, never the ownership set; it adds no manager. Concretely, on the next helm upgrade after a kubectl set image, helm reclaims the field and writes what the chart rendered — it does not error and does not preserve the digest. Pre-B it reclaims and writes the wrong value (:tag, i.e. this bug); post-B it reclaims and writes the right one. So B is neutral on ownership and better on outcome, not worse.

One fact that defuses the question further, for @LukasWodka: the fleet path is Helm 3 today — autoUpgrade pins alpine/helm 3.16.4 (values.yaml:1624-1626) and CI's render job pins v3.15.4 — so server-side-apply ownership does not reach the fleet at all until someone bumps that tag to 4.x.

And one thing I said too weakly, which reviewing B sharpened. I wrote that A and B "compose rather than contradict" and that I "would land A first". The composition claim holds in the steady state, but "A first" is stronger than a preference — it is a correctness precondition for B. B's authority is the last-refreshed-* annotation, and that annotation lags the live spec in two reachable states: a rollout that times out (set image has moved the spec, set -e exits before the annotate) and helm rollback (which re-applies a stored manifest rather than re-rendering). In both, B renders the older digest and pulls the workload backwards. What heals it is precisely A's live-spec reconcile, within one tick. Without A, recorded == latest no-ops and the stale digest sticks until the next upstream publish.

So the ordering in my review was right, but the reason matters more than I gave it credit for — and it is an argument for getting A's two blockers closed rather than parking A behind B.

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit ea89c76. Configure here.}

[shujaatTracebloc]

Re-reviewed at ea89c76. All three blockers are closed — thanks for the fast turnaround, and for taking the gate rather than the bigger refactor. Verified locally rather than read off the diff: 9/9 bats in a throwaway worktree, helm unittest 38/38, shellcheck -S warning -x clean on the rendered script.

Blocking 1 — CLOSED. Line 672 now reads [ "$pending_attempt" -gt 0 ] && [ "$pending_attempt" -lt "$MAX_REFRESH_ATTEMPTS" ], so a latched image falls to the no-op arm and continues. The log at 693 now fires only when there is budget left, i.e. only when the restart block genuinely reaches set image + rollout status — text and behaviour agree. The new bats case is mutation-proof, not text-matching: I dropped the < MAX bound, and separately weakened -lt to -le, and case 5 reddens on each while the other eight stay green. It enforces the bound, not its presence.

Blocking 2 — CLOSED as filed. With the gate, a latched tick keeps restart_needed=0, the restart block at 749 is skipped, and control reaches the annotate_args write at 859. I traced both artefacts by hand: a first-observation record (607) and a stale-pin clear (535/563/587) accumulated by another image both survive a latched tick. The permanent suppression is gone.

One correction to my own review: I offered "or move the annotate_args write above the restart block" as an alternative. That was wrong, and you were right not to take it — the comment at 745 says why: annotating the digest before a successful rollout status would let a failed rollout freeze the workload on the old image, because the next tick then reads recorded == latest. Withdrawn.

The residual, for the record and not for this PR: the flap guard's exit 0 at 790 still precedes the write at 859, so a tick that is BOTH off-digest and latched still drops that tick's Pass 0 annotations. The mechanism is pre-existing — on develop, any digest-change tick under a latched flap does the same — and the real fix is to split annotate_args: the stale-pin clears can move above the restart block, the key=latest digest records cannot. Its own issue, not a blocker here.

Blocking 3 — CLOSED on the documentation option. The header at 106-119 states the cost with the numbers: the shared MAX_REFRESH_ATTEMPTS lockout, desiredNumberScheduled counting every node under tolerations: Exists, three ticks / ~45 min at the 15m schedule, refresh stopped for ALL control-plane images while the CronJob stays green, and jobs-manager's strategy: Recreate downtime for byte-identical content. values.yaml 1718-1730 carries the operator-facing version. The remediation line is reachable and names the key verbatim (786: clear the tracebloc.io/refresh-attempt annotation to re-arm refresh).

The third sub-item I asked for — a re-pin not counting toward ATTEMPT_KEY — is not in, and I accept the deferral. @LukasWodka's point on PRRT_kwDOMzdex86gnkwj holds: the same counter and the same DaemonSet wait were already reachable at the first upstream digest publish, so this PR moves when the lockout becomes reachable, not whether. Please file the "have we ever applied a digest here?" gate as a real issue rather than leaving it a promise in a comment — it is the thing that removes the fresh-install roll outright.

One thing the gate itself introduced — the last item, and it is one line.

restart_needed is the only path into the flap guard at 749, and 788 is the only writer of FLAP_KEY. So on a tick where pending_attempt >= MAX_REFRESH_ATTEMPTS, every image now falls to the no-op arm, restart_needed stays 0, the guard never runs, and:

• FLAP_KEY is never written;
• the "MANUAL ATTENTION NEEDED" WARN at 786 never prints;
• 695 logs "digest unchanged and workload already on the pinned digest; no-op" on the exact tick the script knows an earlier re-image never settled.

Traced on your own fresh-install-with-one-cordoned-node case: tick 2 re-pins and the DaemonSet rollout status times out (ATTEMPT_KEY=1); ticks 3 and 4 force the retry to 2 then 3; from tick 5 on, refresh is dead for every control-plane image, FLAP_KEY is absent, and the CronJob is green and quiet — until the next upstream digest publish, which is the "can be never" this PR exists to fix. Before this push that state was loud (the forced branch set restart_needed=1, so 786/788 fired every tick) at the cost of dropping the annotation write; the gate fixes the drop and takes the noise with it. The comment you added at 676-681 asserts the opposite — "advancing it to the #563 flap lockout, which SURFACES the stuck rollout rather than hiding it" — which is true of the bounded arm and no longer true of the latched one. And it is exactly what #1964 wrote down as the rule for this file: "images did not update" must never be inferable only from the Job's colour.

Suggested, in the latched arm before the continue:

if [ "$pending_attempt" -ge "$MAX_REFRESH_ATTEMPTS" ]; then
  log "  WARN: workload is on the pinned digest but ${ATTEMPT_KEY}=${pending_attempt} (>= MAX_REFRESH_ATTEMPTS=${MAX_REFRESH_ATTEMPTS}) -- FLAP LATCHED: image refresh is STOPPED for ALL control-plane images and does not auto-resume. MANUAL ATTENTION NEEDED: clear the ${ATTEMPT_KEY} annotation on deployment/${DEPLOYMENT_NAME} to re-arm refresh."
  kubectl annotate deployment -n "$RELEASE_NAMESPACE" "$DEPLOYMENT_NAME" \
    "${FLAP_KEY}=${pending_attempt}" --overwrite --request-timeout=15s
fi
log "  digest unchanged and workload already on the pinned digest; no-op"
continue

A bats case asserting the WARN at pending=3 would pin it; the harness already carries MAX_REFRESH_ATTEMPTS=3.

Still open from my last pass, all non-blocking:

• workload_image_for_repo (338) still has no *) arm. Unreachable today — the set -- list holds exactly those three repos — but an unmatched case returns empty with status 0, which the caller at 657 reads as "unreadable" and continues while logging "API read error / container absent". It pairs with the other half: the two new unreadable skips at 658/662 have no counter and no ceiling, unlike every other cannot-read path in this file (SKIP_KEY/MAX_SKIP_TICKS at 395-423, or exit 1 at 396/430), so a persistently-empty read disables the re-pin forever and green. *) log "BUG: no live-image reader for $1"; return 1 ;; plus a streak annotation mirroring SKIP_KEY closes both.
• 323 and 613-614 still say "the hourly auto-upgrade", against 67 of this same file ("not an hourly revert") and auto-upgrade-cronjob.yaml 231-233, which exit 0s when LATEST = CURRENT. It overstates the exposure the header asks reviewers to price.
• backend#199 should be client-runtime#199, unchanged since my last pass, in five places: image-refresh-cronjob.yaml 57, 102, 479; values.yaml 1723; image-refresh-repin-on-revert.bats 5. Plus this commit's subject. A bare backend#199 points at an unrelated issue in another repo.
• image-refresh-repin-on-revert.bats 13 names proxy_off_digest; the script and the helm-unittest guard both use proxy_on_digest.
• The PR body still opens "DRAFT for discussion" and says "Chart.yaml bumped 1.9.107 -> 1.9.108" while Chart.yaml is 1.9.109. This becomes the squash-merge message.

CI: fully green at ea89c76 — 53 pass, 4 skipped, nothing failing or pending, and all three Bugbot threads resolved. For the record, bugbot / review went red earlier on this head and it was an artifact of its own wait window, not a finding: the gate gave up at 900s with Bugbot still IN_PROGRESS (its log records p50 164s / max 635s over 40 runs), Bugbot then finished at 13:19 with "no new issues", and the re-run passed in 6s.

So the only thing between this and my approval is that one WARN/FLAP_KEY line. Push it and I approve on sight — nothing else is outstanding from me, and I am not asking for the non-blocking list above to be a condition (though the PR-body sweep is worth doing before the squash).

[LukasWodka]

Re-reviewed at ea89c76. Ran image-refresh-repin-on-revert.bats 9/9 plus the two sibling image-refresh suites (16/16, 14/14), helm unittest 696/696, shellcheck on the rendered script clean. Mutated the new gate two ways — dropped the < MAX bound, and weakened -lt to -le — and case 5 reddened on each while the other eight stayed green, so the test enforces the bound rather than its presence. The round-2 mutations still bite (inverted digest comparison → 7/9 red; forced-retry branch unreachable → case 3 red). No installer script under scripts/ changed, so no manifest regen applies.

Closed from my last pass:

• Helm 4 server-side-apply field ownership — closed, no change needed. Two facts settle it. The in-cluster auto-upgrade runs alpine/helm:3.16.4 (values.yaml:1624-1626), so server-side-apply ownership does not reach the fleet path at all until that tag moves to 4.x. And the operator-run Helm 4 path is already documented: docs/MIGRATIONS.md § "helm upgrade aborts with a server-side apply conflict" names image-refresh's kubectl-set pins in the conflict list, says --force-conflicts reverts them and image-refresh re-pins on its next tick, and mandates --server-side=true because a rollback (including the auto-upgrade's automatic one) reverts the stored apply method to client-side. That is the line I was asking for; it was already there. Thanks @shujaatTracebloc for the Helm 3 pointer.
• Header sentence on the fresh-install trade — closed. Lines 106-119 state it with the numbers: the shared flap path, desiredNumberScheduled counting every node under tolerations: Exists, three ticks / ~45 min at the 15m schedule, refresh stopped for all control-plane images while the CronJob stays green, and jobs-manager's Recreate downtime for byte-identical content. That is the trade I accepted on the Bugbot thread, now written down. Please file the "have we ever applied a digest here?" gate as a real issue rather than a promise in a comment.
• < MAX gate — correct. The forced retry runs for 1..MAX-1 and stops exactly where the guard at line 777 would have exit 0'd, so a latched tick keeps restart_needed=0, skips the restart block and reaches the annotate_args write. First-observation records and stale-pin clears land again. Case 5 pins it.

Endorsing @shujaatTracebloc's one open item — this is the last thing before I approve too. With the gate, the latched on-digest state is now silent: FLAP_KEY is only written at line 788 and the MANUAL ATTENTION NEEDED WARN only prints at 786, both inside the restart_needed -eq 1 block that a latched tick never enters. Traced on the header's own case: tick 2 re-pins and the DaemonSet rollout times out (refresh-attempt=1), ticks 3-4 force the retry to 2 then 3, and from tick 5 every image logs "digest unchanged and workload already on the pinned digest; no-op", refresh is dead for all control-plane images, no annotation says so, and the CronJob is green — until the next upstream publish, which is the "can be never" this PR exists to fix. On develop that state is unreachable (a fresh install never re-pins), so it is new here. The comment at 676-681 ("advances it to the flap lockout, which surfaces it") is now true only of the bounded arm. This is exactly the rule #1964 wrote for this file: "images did not update" must never be inferable only from the Job's colour. The fix is the one-liner Shujaat posted — in the else arm, when pending_attempt >= MAX_REFRESH_ATTEMPTS, print the WARN naming the refresh-attempt clear and annotate FLAP_KEY=${pending_attempt} before the no-op continue — plus a bats case asserting the WARN at pending=3 (the harness already carries MAX_REFRESH_ATTEMPTS=3). Approve on sight once that lands.

Nits, unchanged since last pass — worth sweeping before the squash, not a re-request:

• The issue-199 citation is still split between two repos: image-refresh-cronjob.yaml 57/102/479, values.yaml 1723, the bats header line 5 and the four latest commit subjects cite one repo; the PR body Refs line, the first commit, cronjob 331/618, values.yaml 476/587 and bats line 9 cite client-runtime#199. This repo is public — the wrong one points readers at an unrelated issue in a repo they cannot see. Pick client-runtime#199.
• image-refresh-repin-on-revert.bats:13 names proxy_off_digest; the script and the helm-unittest guard use proxy_on_digest.
• Lines 323 and 614 say "the hourly auto-upgrade"; line 67 of the same file says "not an hourly revert", and the auto-upgrade exits 0 at latest. Overstates the exposure the header asks a reader to price.
• PR body still opens "DRAFT for discussion" and says "1.9.107 → 1.9.108"; Chart.yaml is 1.9.109. This becomes the squash-merge message.
• values.yaml 1718-1730 describes the pin-on-next-tick contract without the flap cost the header now states; one clause would keep the operator-facing text honest too.

CI fully green at ea89c76 (53 pass, 4 path-filtered skips), all three Bugbot threads resolved. Holding at comment only because Shujaat's change-request already carries the block above; nothing else is outstanding from me.