fix(image-refresh): a skip that never ends is a finding, not an exit 0 (backend#1964)

[LukasWodka]

Part 1 of tracebloc/backend#1964 (surfacing). Does not close it — see "Deliberately not here".

The problem

The settled guard skips the whole tick when the jobs-manager Deployment is not settled. That is right for a rollout in flight and wrong forever for a pod that never becomes Ready — and because the skip sits before Pass 1, requests-proxy and resource-monitor stop being reconciled too, while every tick exits 0 and the CronJob stays green.

It is reachable, and reachable by an operator

Nothing has to crash. jobs-manager's readiness probe is a TCP connect to 8080, and jobs_manager.py:3529 catches a failed run_server_in_threadon purpose so Service Bus polling and training survive. The pod then runs forever, useful, and never Ready.

run_server_in_thread → create_app() → ensure_ingestion_runs_table() (a MySQL DDL migration) and load_authz_policy(authz_path); then _ServerThread.__init__ → make_server(), which binds in the constructor:

trigger	raises in
malformed ingestion-authz.yaml — an operator-supplied ConfigMap	load_authz_policy
DB user without CREATE/ALTER on ingestion_runs (RFC-0003 grant narrowing)	ensure_ingestion_runs_table
8080 already bound	make_server

So a helm upgrade typo silently freezes image refresh for four workloads on a customer edge.

The existing flap counter cannot cover it.MAX_REFRESH_ATTEMPTS counts failed rollouts, and everything after the settled guard is unreachable when the deployment never settles.

What this does

A second counter with the same shape — MAX_SKIP_TICKS, default 8 (~2h at the 15m schedule), exposed as imageRefresh.maxSkipTicks:

situation	before	after
1st–7th unsettled tick	exit 0, silent	exit 0, annotates refresh-skip-streak=N
8th consecutive	exit 0, silent, forever	exit 1, names all four frozen workloads + the likely cause
settled again	—	clears the annotation; auto-resumes
annotation unreadable	—	exit 1 — fail-open would reset the streak every tick, so the ceiling would never be reached
deployment missing	exit 1 (#571)	unchanged, and asserted

Auto-resume is deliberate and differs from maxRefreshAttempts, which needs a human: a skip streak means "it was busy", and settling genuinely resolves that.

Deliberately not here

The ticket's decision 1 — reconcile the other three components past an unsettled jobs-manager — is unsafe as written for pods-monitor. pods-monitor-container is a sidecar in the jobs-manager Deployment (jobs-manager-deployment.yaml:458) and the CronJob re-images both containers in one patch, so refreshing it requires rolling a Deployment that cannot go Ready — incident #545's restart storm, which this guard exists to prevent. Only requests-proxy (own Deployment) and resource-monitor (own DaemonSet) can be decoupled. That is a separate PR once the decision is confirmed on the ticket.

Tests run the script, they do not grep the template

scripts/tests/image-refresh-skip-streak.bats renders the chart, extracts the script the pod actually runs, and executes it against a stubbed kubectl. Exit codes and annotation writes are the assertions. A "the ConfigMap contains MAX_SKIP_TICKS" test would pass against logic that can never fire — the inert-verification shape backend#1729 catalogued.

Hermetic: curl is stubbed to refuse, so the suite never reaches docker.io or spends the anonymous pull-rate limit the chart is careful about. Verified stable across repeated runs.

Mutation proof

Each anchor asserted, because an inert mutation and good coverage look identical in a log:

revert the ceiling to exit 0 (the original defect) 4 tests redden
fail-open the unreadable annotation read 1 test reddens
drop the other-workload names from the message 1 test reddens (the right one)
remove clear-on-settled 1 test reddens
baseline, 3 consecutive runs 14/14 ok

Two of my own assertions were vacuous on the first pass and are fixed in place with the reason recorded in the file:

• grep -qv PATTERN file does not mean "no matching line" — it returns 1 on an empty file and 0 on any file with an unrelated line. Wrong in both directions; it failed one case and passed another for the wrong reason.
• A whole-output grep for requests-proxy matches the startup banner, so that assertion passed while the script was exiting long before the guard.

Adjacent, not changed

The flap branch (#563) also exit 0s after logging "MANUAL ATTENTION NEEDED" — the same green-while-frozen shape. Left alone deliberately: it is documented behaviour and deserves its own ticket rather than a drive-by in this diff.

Test plan

• bats scripts/tests/image-refresh-skip-streak.bats — 14/14, stable over 3 runs
• bats scripts/tests/bats-hygiene.bats — 18/18 (new file satisfies the || return 1 gate)
• helm unittest client — 433/433 across 30 suites
• helm lint client -f client/ci/bm-values.yaml — clean
• 4 mutations, each reddening with its anchor asserted
• scripts/manifest.sha256 unaffected — it covers installer scripts, not chart templates

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes fleet-wide image-refresh behavior on customer edges; mis-tuned thresholds or guard bugs could false-fail jobs or miss freezes, though scope is bounded and covered by executable BATS tests.

Overview
Image-refresh now detects when the jobs-manager Deployment stays unsettled tick after tick (e.g. Running but never Ready) and stops treating that as a silent success.

When rollout status says “not settled” but the deployment exists, the script increments tracebloc.io/refresh-skip-streak on the jobs-manager Deployment. Below imageRefresh.maxSkipTicks (default 8, ~2h at the 15m schedule) it still exit 0 but logs N/MAX. At the ceiling it exit 1 with IMAGE REFRESH IS FROZEN, naming requests-proxy and resource-monitor (reconciliation never runs while the guard skips) and hints for a never-Ready ingestion HTTP server. Unreadable streak annotations fail the tick (no fail-open reset). Once settled, the streak annotation is cleared and refresh auto-resumes—unlike the flap counter.

MAX_SKIP_TICKS is wired from values into the CronJob env. Chart version 1.9.41 → 1.9.42.

scripts/tests/image-refresh-skip-streak.bats renders the chart, runs the real image-refresh.sh against stubbed kubectl/curl, and asserts exit codes and annotations (plus helm template checks for the env default/override).

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

[saqlainsyed007]

Correctness review — approving.

What it does

Adds a second, independent counter (MAX_SKIP_TICKS, default 8 ≈ 2h at the 15m schedule) to the image-refresh CronJob so an unsettled jobs-manager Deployment that will never become Ready stops being a silent exit 0. Below the ceiling it annotates refresh-skip-streak=N and skips; at the ceiling it exit 1s with a finding that names the other frozen workloads (requests-proxy, resource-monitor) and points at the reachable cause. Settling clears the annotation and auto-resumes. Wired through values.yaml → CronJob env; chart 1.9.41 → 1.9.42.

Correctness — traced, holds up

• Skip path: SKIP_KEY read is fail-closed (if ! skips="$(get_annotation …)" → exit 1), so an unreadable annotation can't collapse the streak to 0 and mask a permanent freeze. Non-numeric/empty normalized via the case guard; increment-then--ge ceiling check is right (stored 7 → 8 → exit 1; stays failed past the ceiling).
• set -e interaction: both new get_annotation calls sit in if/&& conditions, so set -e is correctly suppressed and the function's || return 1 contract is honored rather than aborting the tick.
• Clear path: deliberately non-fatal on read error (settled is already proven), only writes when there's something to clear, auto-resumes. Correct asymmetry vs. the skip path.
• Missing-deployment (#571) branch unchanged and asserted.
• Env wiring / default 8 verified by the two helm-template tests.

Tests

The suite renders the chart, extracts the real image-refresh.sh, and executes it against a stubbed kubectl/curl — exit codes and annotation writes are the assertions, not template greps. Hermetic (curl stubbed to refuse). The two previously-vacuous assertions (grep -c line counting, frozen-line-scoped grep) are genuinely fixed. All CI green (bats 14/14, helm unittest 433, shellcheck, 4 template renders) and Bugbot pass on the head commit.

Non-blocking observations (considered, not requesting changes)

• On an in-progress legitimate rollout, a transient SKIP_KEY read failure now yields a red tick where it was previously exit 0. This is the intended fail-closed trade-off and self-heals next tick.
• The clear-path kubectl annotate …- is unguarded, so under set -e a transient clear failure would abort before Pass 1 for that one tick. Only reachable after a real streak, self-heals next tick, consistent with the script's fail-closed philosophy.

Meticulous change — clear commentary, executable + mutation-verified tests, correct fail-closed reasoning. LGTM.