fix(image-refresh): write Pass-0 stale-pin annotations before the restart block

[saqlainsyed007]

Summary

The #563 flap guard in the restart block does WARN + FLAP_KEY + exit 0 once refresh-attempt >= MAX_REFRESH_ATTEMPTS — before the digest-record annotate at the end of the tick. The stale-pin clears were batched into that final annotate, so on a tick that is both off-digest (restart_needed=1) and latched, they were dropped: a stale-pin clear that never lands leaves a false "pin is stale" finding to persist forever (a write-only annotation outliving its problem — the class client#824's clear paths fixed), and it's dropped on the exact tick refresh is dead, when the finding matters most.

Fix (as @shujaatTracebloc prescribed on #1008): move the stale-pin writes (clears and finding-sets) into their own accumulator stale_pin_args, annotated in a bounded, non-fatal call above the restart block. They touch only the stale-pin-<image> keys, never last-refreshed-<image>-digest, so writing them before the rollout can't affect the recorded == latest skip logic. The digest record deliberately stays below, after a successful rollout status — annotating it before a failed rollout would freeze the workload on the old image.

Related

Split from #1008 — item 1 (the latched-flap Pass-0 annotation drop). Item 2 (the fresh-install digest gate) merged in #1033. This is the second and final half of that split.

Type of change

• Bug fix

Test plan

• scripts/tests/image-refresh-latched-annotate.bats (new, 4 cases): extracts the shipped tail from the rendered chart and asserts the stale-pin clear lands while the digest record does not on a latched tick (restart_needed=1, attempt >= MAX), and that both land on a healthy tick — re-batching the writes reddens it.
• scripts/tests/image-refresh-stale-pin.bats: harness updated for the new accumulator.
• client/tests/image_refresh_test.yaml: render guard for the accumulator + the before-restart ordering (+1 = 40).
• 47 image-refresh bats + helm unittest ./client 730/730 + make check green. The 9 full-bats failures are pre-existing (identical on develop: footprint / embed-drift / envelope / doctor).
• Chart bumped to 1.9.113 (version + appVersion).

Checklist

• Tests added / updated and passing locally
• No secrets / credentials in the diff
• Cross-repo issues use owner-qualified form

---

Note

Medium Risk
Changes ordering of cluster annotations on the jobs-manager deployment during image-refresh ticks; behavior is narrow (stale-pin keys only) but affects operational diagnostics when refresh is latched or flapping.

Overview
Fixes a bug where stale-pin diagnostic annotations (tracebloc.io/stale-pin-* clears and sets) were batched into the same final annotate_args write as digest bookkeeping. On ticks that need a re-image and hit the flap latch (refresh-attempt >= MAX), the guard exits before that final annotate—so stale-pin clears never landed and false “pin is stale” findings could stick around.

The cron script now accumulates those writes in stale_pin_args and applies them in a separate, non-fatal kubectl annotate before the restart/flap block. last-refreshed-*-digest recording stays after a successful rollout, unchanged.

Adds image-refresh-latched-annotate.bats (latched vs healthy tick behavior), extends helm unittest regex guards for all four stale-pin write paths and ordering, and updates the stale-pin branch harness. Chart 1.9.117.

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

[cursor]

Stale Bugbot comment from a previous run.

[LukasWodka]

Verified the mechanism against the parent. On develop the four stale-pin writes (disabled-pin clear, current-pin clear, stale finding set, unpinned clear) all rode annotate_args, whose only annotate sits after the restart block, below the flap guard's exit 0, so an off-digest and latched tick dropped them. On this head they accumulate in stale_pin_args and land in a bounded (--request-timeout=15s), non-fatal (if ! sp_err="$(...)") annotate above if [ "$restart_needed" -eq 1 ], guarded by -n so an empty tick makes no call. Only stale-pin-* keys travel there; last-refreshed-* and digest-applied-* still land after rollout status, so the recorded == latest logic is unchanged. Writing a stale-pin clear before a rollout that later fails is right: pinned images continue before restart_needed is set, so the finding and the rollout concern different images.

Tests, run locally from this head: 18/18 bats (latched-annotate + stale-pin) and 40/40 helm unittest on image_refresh_test.yaml. Mutation-proved: reverting the template hunk reddens both; moving the block below the flap guard reddens the ordering assert; keeping the anchor but re-batching stale_pin_args into annotate_args reddens bats cases 2 and 4 on their own assertions.

One gap, matching the open Bugbot thread: re-batching only the disabled_stale_key clear (template line 594) leaves all three test files green. The render guard names stale_key and unpinned_stale_key but not disabled_stale_key, and the stale-pin bats "disabled-monitor exit clears" case asserts on $output, which contains both the ANNOTATE: and STALEPIN: lines, so it cannot tell the accumulators apart. Please add the third matchRegex/notMatchRegex pair, or assert STALEPIN: specifically in that case, then resolve the thread.

Chart 1.9.112 to 1.9.113 on both version and appVersion. No hunk overlap with #1013 in this template (header comments only), though #1013 will pick up another Chart.yaml conflict on rebase.

Happy to approve once the guard is extended and CI is green.

[LukasWodka]

Heads-up on the chart version: #1040 (opened a few minutes ago) also bumps client/Chart.yaml 1.9.112 to 1.9.113, to unblock the develop-to-staging promotion #1037 that the chart content => Chart.yaml version bump guard refused after #1032 and #1033 both took 1.9.112. If #1040 merges first, this PR's bump becomes a no-op again and the guard will refuse the next promotion for the same reason. Please rebase onto develop after #1040 lands and take 1.9.114 here (version and appVersion).

[saadqbal]

Not approving yet only because Bugbot's thread is open — and having checked it, Bugbot is right, so it's worth two lines rather than a dismissal. The production code is correct; this is purely a hole in the guard, so there's no logic bug to hunt.

All four stale-pin writes in the shipped script do go to the accumulator:

594  stale_pin_args=" ... ${disabled_stale_key}-"     clear, disabled image
622  stale_pin_args=" ... ${stale_key}-"              clear
633  stale_pin_args=" ... ${stale_key}=${pin_latest}" the finding SET
646  stale_pin_args=" ... ${unpinned_stale_key}-"     clear, unpinned

But the render guard names only two keys. stale_key gets a positive (stale_pin_args ... ${stale_key}-) and a negative (notMatchRegex on annotate_args ... ${stale_key}); unpinned_stale_key gets both as well. disabled_stale_key appears in neither. And the new image-refresh-latched-annotate.bats has four cases with no mention of disabled at all. So moving line 594 back into annotate_args leaves the positive assertions untouched (they name the other two keys), the negatives untouched (same), and the ordering assertion satisfied (stale_pin_args still exists for the rest) — helm-unittest and both bats stay green while the disabled-image clear is dropped on exactly the latched tick this PR exists to fix. Two assertions mirroring the stale_key pair close it.

That's worth fixing rather than waiving because it's this PR's own thesis one level up: a write that never lands leaves a false finding forever. The guard that stops that happening is itself unguarded on one of its three keys, and the comment above it says its job is to "lock the code shapes".

One smaller thing in the same area, non-blocking. The positive assertions pin the two clears but not the finding-set at 633 — deleting ${stale_key}=${pin_latest} from the accumulator entirely would keep both suites green, since the negative only forbids it riding annotate_args. The behaviour bats assert a clear lands; nothing asserts the set does. Lower stakes than the disabled path (a lost set means a real staleness goes unreported rather than a phantom persisting), but it's the same shape.

The fix itself is right and the reasoning for the split ordering is the part I'd keep: stale-pin keys above the restart block because they can't affect the recorded == latest skip, digest record below because annotating it before a failed rollout would freeze the workload on the old image. That asymmetry is the whole point and it's stated where the next person will look.

[LukasWodka]

Status check on the re-review pass: head is still 210d0b2, so the two asks from last night stand unchanged —

1. Bugbot's thread is still open and still right: disabled_stale_key (template line 594) has no matchRegex/notMatchRegex pair in image_refresh_test.yaml, so re-batching only that clear into annotate_args leaves all three suites green. Add the third pair (and, per @saadqbal, a positive assertion that the finding-set ${stale_key}=${pin_latest} rides stale_pin_args), then resolve the thread.
2. The PR is now CONFLICTING on client/Chart.yaml. Correction to my earlier comment: develop is already at 1.9.114 (chore(chart): bump to 1.9.113 so #1033's template change publishes (version collided with #1032) #1040 took 1.9.113, feat(chart): route JOB_IMAGE_HOST through tracebloc.tbRegistry so training pods follow the tracebloc registry default (ghcr.io) #1041 took 1.9.114), so please rebase and take 1.9.115 on both version and appVersion.

Production hunk is correct as reviewed; I will approve once those two land and CI is green.

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

[LukasWodka]

Re-reviewed dfa180a — both asks are in and I've mutation-proved the new guard pair locally (re-batching the disabled_stale_key clear reddens asserts[2]+[7], deleting the finding-set reddens asserts[3], restored 40/40); chart is 1.9.115 over develop's 1.9.114, conflict gone, Bugbot clean on this head. Approval is written and waiting on CI, which is red for two reasons that are not this PR:

• Prereqs / PATH persist ×4 — apt inside the ubuntu:22.04/24.04 containers can't install curl (libbrotli1 … not installable, "held broken packages"), so everything downstream fails. Same failure hit develop at 05:44Z and release-train/to-main at 06:41Z today, with green runs either side — a mirror flake, not the chart.
• Helm unit tests — two pull_request runs fired 15 s apart on the same sha (the edited type, from the body update), and cancel-in-progress killed the first; the second was cancelled at 07:06Z before any step ran, so there is no completed result on this head.

A re-run of the failed jobs (or an empty push) should turn both green; I'll approve as soon as it does.

[saqlainsyed007]

bugbot run

[saadqbal]

The disabled_stale_key pair is in, and you went one better by covering the finding-SET too — all four stale-pin writes now have a positive and every key has a negative. I checked each pattern against the actual template lines rather than taking the block on faith, and the helm unit tests are green at this head, so the assertions are live rather than decorative.

The four red Ubuntu jobs look like an apt-mirror stall, not you: exit 137 on "Installing curl" at the 12m step bound, every non-Ubuntu distro in the same matrix green, and the job's own annotation says as much. Re-run those four and mutation-check, and I'll approve.

[LukasWodka]

Re-checked a6bc300: the push is a pure merge of develop (#1045, #1016) with Chart.yaml at 1.9.117 over develop's 1.9.116 — the template hunk and all three test files are byte-identical to dfa180a, so the disabled_stale_key guard pair (asserts on stale_pin_args and annotate_args) and the finding-set positive stand, and image_refresh_test.yaml is 40/40 from this head against the merged develop.

Helm unit tests completed green on this head (run 34575772777), Bugbot found no issues on a6bc300, no open threads, and the four Ubuntu installer jobs that were red on the first attempt were the fleet-wide apt-mirror stall (same jobs red on main and release-train/to-main in the same hour); they are green on the rerun. Approving.