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

[saqlainsyed007]

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 #1013 and must land after this one.

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; Chart.yaml bumped to 1.9.110.

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 control-plane image reconciliation and can trigger extra rollouts (jobs-manager Recreate, resource-monitor DaemonSet) on fresh install, chart version bumps, or after Helm re-render; mitigated by fail-closed API reads, digest-only comparison, and explicit flap/latch surfacing.

Overview
Fixes client-runtime#199: when the registry digest is unchanged (recorded == latest), the image-refresh CronJob no longer treats that as an unconditional no-op. It now reads each workload’s live container image (workload_image_for_repo, plus requests_proxy_image for the separate requests-proxy deployment) and re-pins repo@digest when Helm auto-upgrade has re-rendered back to repo:tag, bounding stale-cache exposure to about one cron interval (~15m).

Matching uses the @sha256 digest only so registry-prefix mirror webhooks are not mistaken for a revert. The first-tick flow is unchanged (record only); the next tick can pin fresh installs still on :tag. Unfinished rollouts are retried via ATTEMPT_KEY until flap latch, which now warns and sets FLAP_KEY instead of staying silent.

Chart 1.9.110; helm-unittest regex guards and new bats (image-refresh-repin-on-revert.bats) lock behavior. Companion template-side fix is tracked separately (#1013).

^{Reviewed by Cursor Bugbot for commit a38563d. 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.

[saadqbal]

That's my side closed — all three, at 79b61601.

The latched arm now does both halves: the WARN names the latch, says refresh is stopped for all control-plane images and doesn't auto-resume, and gives the remediation inline; then it annotates FLAP_KEY before the continue. Guarding it on pending_attempt >= MAX_REFRESH_ATTEMPTS is right — it fires exactly when the downstream flap guard can't, and stays quiet otherwise.

The bats case is the good part. Stubbing kubectl() to print KUBECTL:… makes the annotate observable, so it asserts the FLAP_KEY write actually happens rather than that the string appears somewhere in the script — which is the failure mode the file's own header calls out. Read rather than ran, but the shape is right and CI is green on all 52.

And client-runtime#199 is correct everywhere now (I'd checked: backend#199 resolves to a merged notification-URL PR). proxy_on_digest fixed too.

@shujaatTracebloc your change-request from 74f1a144 is the only thing left on this — I'm not approving over it, but from my side the one-liner you and @LukasWodka were holding for has landed as specified.

[LukasWodka]

Re-reviewed 79b6160 against my ea89c76 comment and Shujaat's standing change-request. I would approve this head; the only thing between it and an approval is that Shujaat's CHANGES_REQUESTED at 74f1a14 is still formally standing, and I do not approve over another reviewer's open verdict. @shujaatTracebloc — your 13:26 re-review closed blockers 1–3 and named one last item; 79b6160 is that item. If you agree it is closed, a re-review lifts the block.

The last item — a latched flap is now surfaced on the on-digest no-op tick. With the < MAX gate, a latched tick keeps restart_needed=0 and never reaches the flap guard, which was the only writer of FLAP_KEY and the MANUAL ATTENTION warning — so refresh could be dead for every control-plane image while the CronJob stayed green. The new arm at the recorded == latest / workload-on-digest branch fires only when pending_attempt >= MAX_REFRESH_ATTEMPTS, logs the same WARN text as the guard (naming the tracebloc.io/refresh-attempt clear), and annotates FLAP_KEY before the no-op. Two new bats cases cover both directions: latched → WARN + KUBECTL:annotate … refresh-flap-detected=3; pending=0 → bare no-op, no annotate. The stub now defines the four variables and a kubectl shim, so the assertion is on the shipped branch, not on text presence. CI is 52 pass / 6 skipping on this head, mergeable, no open threads.

My nits from ea89c76 — all but one landed: the wrong-repo issue-199 citation now reads client-runtime#199 in all six places (header, first-tick comment, unfinished-attempt comment, values.yaml, bats header); proxy_off_digest → proxy_on_digest; the two "hourly auto-upgrade" phrases are now "the auto-upgrade". Still open, cosmetic: the body's first line still says "DRAFT for discussion" and line 14 still says "1.9.107 → 1.9.108" while the summary below correctly says 1.9.109.

One small robustness note, not blocking: the new kubectl annotate runs under set -e without a fallback, so an API hiccup on exactly the latched tick aborts the tick before the no-op line. The other annotate writes in this script share the shape, so this is consistent rather than new; || log "…annotate failed" would make the latched arm survive the hiccup it exists to report.

Ordering unchanged: this is A, and B (#1013) must not merge before it. B is rebased onto develop with two docs commits and currently carries seven unresolved threads, so A first is also the natural order.

[saqlainsyed007]

Thanks @LukasWodka. Triaging your three points:

• Body nits — fixed. Dropped the stale "DRAFT for discussion" framing (this is approve-ready, not a draft) and corrected the version line to "bumped to 1.9.109" (line 14 had a stale 1.9.107 → 1.9.108; the summary's 1.9.109 is right). Body-only edit, no new head.
• kubectl annotate under set -e on the latched arm — leaving as-is, deliberately. As you noted it's consistent with the other annotate writes in this script rather than new, so a || log "…annotate failed" on only the latched arm would make it the odd one out. If we want the latched arm to survive an API hiccup on exactly the tick it exists to report, the right move is to give all the annotate writes the same fallback in a follow-up, not to special-case this one — happy to do that separately so this PR stays the image-refresh A-fix.
• Ordering — agreed and now stated in the body: B is fix(chart): render control-plane images from the last-refreshed digest so helm upgrade preserves the pin #1013 and lands after this.

No code change on this head, so no bugbot run. Still gated on @shujaatTracebloc's re-review to lift the standing block; his blockers 1–3 and the last item (79b6160) read as closed to you.

[shujaatTracebloc]

Re-reviewed at 79b6160. The one item I was holding for has landed exactly as specified, and my three blockers stay closed. I am not re-litigating anything from my last two passes — the substantive review is done. One new, mechanical item has appeared since, caused by develop moving rather than by anything you did, and it is the only thing between this and my approval.

The last item — CLOSED. image-refresh-cronjob.yaml:702-708 is the arm as prescribed: guarded on pending_attempt -ge MAX_REFRESH_ATTEMPTS, sitting in the api_on_digest && proxy_on_digest else branch, emitting the WARN that names the tracebloc.io/refresh-attempt clear and annotating FLAP_KEY before the no-op/continue. So the tick on which refresh is dead for every control-plane image is no longer silent-and-green — which was the #1964 rule this branch was breaking.

The bats coverage is the right kind. Defining a kubectl() shim that prints KUBECTL:… makes the annotate observable, so the latched case asserts tracebloc.io/refresh-flap-detected=3 is actually written rather than that the string appears in the script, and the pending=0 case pins the lower bound (bare no-op, no annotate, no WARN). That is the failure mode this file's own header warns about, and you avoided it.

Nits from my last pass — all closed. backend#199 → client-runtime#199: zero occurrences of the wrong ref remain in any changed file. proxy_off_digest → proxy_on_digest. Both "hourly auto-upgrade" phrases now read "the auto-upgrade", consistent with line 67 and with auto-upgrade-cronjob.yaml's chart-version gate. And the PR body is swept — no "DRAFT for discussion", and the version line is correct. Thanks for taking the whole list.

---

The one new item — the chart version now collides with develop. (Blocking, one line.)

First, a retraction. In my 74f1a14 review I wrote that "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." That is no longer true, and I am withdrawing it. #1017 (e1677f3b) merged at 15:22 on 2026-09-09 — about three hours after I wrote that — and took 1.9.109 for develop.

ref	client/Chart.yaml
this PR's merge base 402ec46b	1.9.108
this PR at 79b6160	1.9.109
develop tip	1.9.109

So relative to develop as it stands, this PR changes client/templates/** and client/values.yaml and does not move the version — the exact condition the chart guard exists to catch.

Why both gates are green anyway, so this isn't a CI failure you missed. .github/workflows/chart-version-guard.yml:33 passes BASE_SHA: ${{ github.event.pull_request.base.sha }}, and scripts/chart-version-guard.sh:50 diffs "${BASE_SHA}...HEAD". That is the frozen merge base at 1.9.108, so the guard correctly sees a bump. It cannot see a version claimed by develop after the branch forked. Green, and stale — I'm not asking you to fix the guard here.

Why it's worth a rebump rather than a shrug. v1.9.109-rc.1 was cut this morning (2026-09-10 05:29) from 9dd5e33, which does not contain this PR. release-helm-chart.yaml hard-fails unless the tag's base X.Y.Z equals client/Chart.yaml's version, then packages with helm package ./client --version "${TAG#v}". Merge at 1.9.109 and the eventual final v1.9.109 ships chart content the 1.9.109 rc never carried — one version number covering two different charts, which is what the guard's own error text ("a Helm repo only ever publishes a NEW version") is written to prevent. In this PR's favour: gh-pages publishing is gated on prerelease != 'true', so the rc did not reach the index and nothing is overwritten yet.

Ask: bump client/Chart.yaml to 1.9.110, version and appVersion in lockstep, plus the body's version line. You already ran this exact play in e24fe6d when develop moved to 1.9.108. And it re-sequences #1013, which currently sits at 1.9.110 and will need 1.9.111 — worth doing now so B isn't blocked on the same thing.

---

Non-blocking, and not conditions on this PR:

• The new arm lives inside the 3-entry for entry in "$@" loop, but pending_attempt is read once at line 493 and is loop-invariant. So on a latched tick with all three images on-digest, the WARN and the kubectl annotate fire three times, where the downstream flap guard at 800-801 fires once. Idempotent (--overwrite, same value), so this is log/API noise rather than a correctness problem — but it also triples @LukasWodka's set -eu point: an API hiccup on any of the three aborts the tick before the annotate_args write. I'd take his || log "…" on line 706, and hoisting the whole block above the loop would fix both at once. Neither is a merge condition.
• workload_image_for_repo (~338) still has no *) arm, and the two unreadable-skips still have no ceiling. Unchanged from my last pass and, as I said then, not a condition — worth carrying into the follow-up issue with the "have we ever applied a digest here?" gate.

Ordering unchanged: A before B, and per my ea89c76 note that is a correctness precondition for #1013, not a preference — B's authority is the last-refreshed-* annotation, which lags the live spec on a timed-out rollout and on helm rollback, and A's live-spec reconcile is what heals it.

Rebump to 1.9.110 and I approve on sight — there is nothing else outstanding from me, and @LukasWodka and @saadqbal are both already there.

[saqlainsyed007]

bugbot run

[cursor]

Stale Bugbot comment from a previous run.

[saqlainsyed007]

bugbot run

[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 a38563d. Configure here.}

[saqlainsyed007]

Precondition for #1013 (B): the flap-lockout annotation-ordering fix belongs here in A. (Raised by @shujaatTracebloc / @LukasWodka on #1013.)

Today annotate_args is one batched kubectl annotate at the very end, after all three set image + rollout status pairs, and the rollout status calls are unguarded — so under set -e a rollout that times out on requests-proxy or the resource-monitor DaemonSet exits before the annotate, leaving live api = @D1 with all three last-refreshed-* annotations still @D0. The flap guard then exit 0s before that annotate block and re-arms only when a human clears refresh-attempt, so the D0-annotation-vs-D1-live skew is durable, and it's reachable without any jobs-manager fault (a single cordoned/NotReady node keeps the DaemonSet's desiredNumberScheduled from settling → three ticks latch the lockout).

Why it gates B: from 1.9.110 the chart renders the last-refreshed digest, and the fleet path is Helm-3.16 client-side apply, so a re-render in that state moves a healthy, settled jobs-manager backwards D1→D0 under Recreate, every bump while the lockout holds — an enforced backward move, worse than the pre-PR bare-:tag revert. A's existing recorded == latest reconcile never runs here (recorded (D0) != latest (D1) takes the digest-changed path into the flap guard).

The fix (in this PR): write each image's last-refreshed-<image>-digest annotation immediately after that workload's own rollout status, and before the flap guard's exit 0 — so the annotation can never trail the live spec, and the reconcile runs before, not behind, the lockout check. This is the "A's reconcile must run before, not behind, the lockout check" point @LukasWodka already flagged. #1013 stays held behind this.

[saqlainsyed007]

Rebump landed: client/Chart.yaml is 1.9.110 (version + appVersion), done in eafc68c with the body's version line, then develop merged clean in a38563d. That was your one remaining item ("rebump to 1.9.110 and I approve on sight"), so re-requesting your review. And I re-sequenced #1013 to 1.9.111 so B no longer collides with A on the train, per your note. CI is green on a38563d (mutation-check finishing); Bugbot clean.

[saqlainsyed007]

Small correction to my earlier note here: since your re-review concluded A's current scope is complete ("rebump to 1.9.110 and I approve on sight"), I've split the flap-lockout annotation-ordering hardening to an internal follow-up ticket rather than holding it inside this PR — it's ordering hardening for the durable-lockout edge, not a gap in what A does, and A's recorded == latest reconcile already heals every non-locked state. Full spec + rationale are on that ticket and on the #1013 flap thread. This PR stays as reviewed; #1013 is held behind it.

[LukasWodka]

Re-review at a38563d. Everything I said at 79b6160 holds: the delta since is the develop merge (the seven PRs that landed there) plus the 1.9.110 bump Shujaat asked for, nothing in the refresh script changed, the conflict is gone and CI is fully green on this head.

Holding my approval for one reason, and it is yours, not mine. Your 07:00 note here commits A to the per-workload annotate ordering that Shujaat's blocking thread on #1013 needs: write each image's last-refreshed-* annotation right after that workload's own rollout status, and before the flap guard's exit 0. At this head the script still does one batched kubectl annotate after all three set image + rollout status pairs, and the flap guard exits before it, so the lag state is live. Approving this head would bless a version of A you have already said is incomplete. Push that change and I approve on sight, gate permitting.

@shujaatTracebloc, one housekeeping item: your 74f1a14 change-request is still the recorded verdict here. GitHub does not lift it on a later comment, so once Saqlain's annotate commit is in, it needs an explicit approve (or dismissal) from you for the PR to read green.

[shujaatTracebloc]

Approving at a38563d. The rebump landed, CI is fully green, and my change-request from 74f1a14 is cleared by this review.

The one thing I was holding for is done. client/Chart.yaml is at 1.9.110, version and appVersion in lockstep, and the sequencing is now clean end to end: develop 1.9.109 → this PR 1.9.110 → #1013 1.9.111. You also re-sequenced B in the same breath, which is why that one is not now blocked on the same thing. Thanks for turning it around quickly.

I re-verified rather than assuming. Nine commits landed since my 79b6160 review, which looks like a lot to re-check — but seven are other people's PRs arriving via the develop merge, and comparing blob hashes for this PR's own five files shows four of them byte-identical to what I reviewed:

file	@79b6160	@a38563d
templates/image-refresh-cronjob.yaml	d664eab1	d664eab1
tests/image_refresh_test.yaml	3787fbc7	3787fbc7
values.yaml	564c0962	564c0962
tests/image-refresh-repin-on-revert.bats	9ebfb874	9ebfb874
Chart.yaml	bdaf83cb	de42aa09 (the rebump)

So the substantive review stands unchanged: all three original blockers closed at ea89c76, and the latched-flap arm (WARN + FLAP_KEY on the on-digest no-op tick) landed at 79b6160 exactly as specified, with bats cases that assert the annotate actually happens via a kubectl shim rather than grepping the script text. That last part is why I am comfortable owning this — the guard can fail, which is the whole point of it.

Gate, checked at this head: 53 pass / 5 skipping / 0 failing / 0 pending. mutation-check completed success (it ran ~21 minutes — longer than usual because the develop merge pulled in the #1017/#1020/#1022 installer work, so there were more guards to mutate). Both Cursor Bugbot and bugbot / review are success. Zero unresolved threads. MERGEABLE, no conflict. The one cancelled Helm unit tests entry is a superseded duplicate — the authoritative run at 06:54:13 passed.

Carrying forward as non-blocking, unchanged from my last pass and not conditions on this merge:

• The new arm sits inside the 3-entry for entry in "$@" loop while pending_attempt is loop-invariant (read once at line 493), so a latched tick with all three images on-digest fires the WARN and kubectl annotate three times where the downstream flap guard fires once. Idempotent, so log/API noise rather than a correctness problem — but it triples @LukasWodka's set -eu exposure, and his || log "…" on line 706 plus hoisting the block above the loop would settle both.
• workload_image_for_repo (~338) still has no *) arm, and the two unreadable-skips still have no ceiling. Worth carrying into the follow-up issue alongside the "have we ever applied a digest here?" gate.

On ordering: A before B still holds as a correctness precondition, not a preference — #1013's authority is the last-refreshed-* annotation, which lags the live spec on a timed-out rollout and on helm rollback, and this PR's live-spec reconcile is what heals it. Note that B also still needs the A-side per-workload annotate (write each image's last-refreshed-* right after that workload's own rollout status, before the flap guard's exit 0) before its own blocking thread can close — that is tracked on #1013 and is not a condition on this merge.

Good work through eight rounds of findings without losing the thread on any of them.