Uh oh!
There was an error while loading. Please reload this page.
fix(image-refresh): a bookkeeping write must not abort the tick (backend#2007) - #722
Merged
Merged
Conversation
…end#2007) The settled branch clears tracebloc.io/refresh-skip-streak under `set -eu` with no guard. The READ two lines above is deliberately non-fatal; the WRITE was left fatal, and the comment shows the asymmetry was never decided -- it reasons about the read and says nothing about the write. The cost is not cosmetic: the clear runs BEFORE Pass 1, so a transient API error on that one annotation aborts the tick and requests-proxy and resource-monitor stay frozen on the very tick that proved jobs-manager had recovered. That is backend#1964's own recovery path failing the same way #1964 did, only red instead of green. Read as a pair: #1964 is "green CronJob, no image updates", this is "red Job, no image updates". Neither refreshes. Make the clear non-fatal and report it. Nothing below reads SKIP_KEY, so a stale value costs exactly one thing: the next unsettled tick resumes counting from the old number rather than zero, tripping the ceiling EARLIER -- fail-safe in the direction #1964 wants. It is logged as a WARNING rather than swallowed, because the shared lesson of both findings is that "images did not update" must never be inferable only from the Job's colour. Scoped to this one call site on purpose. The other five annotate calls are deliberately fatal with recorded reasoning -- the streak record fails closed so the ceiling cannot be dodged (#1964), the attempt counter must persist before a restart, and the success reset is split from the digest write precisely so a failure there cannot stick a flap lockout (#626). Only this one is pure cleanup. Tests extend #1964's harness, which renders the chart, extracts the script the pod actually runs and executes it against a stubbed kubectl -- STUB_ANNOTATE_RC was already a knob. The assertion is that Pass 1 is REACHED (its per-image `checking <repo>` lines), not the exit status: a status assertion would also pass if the script died later for another reason. Mutation-proved: with the template hunk reverted, both new cases fail, case 1 at `checking tracebloc/jobs-manager` -- i.e. the tick really did abort before Pass 1 -- and the anchor was confirmed absent from the reverted file rather than assumed. 16/16 green with the fix, helm lint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 14, 2026
ContributorAuthor
bugbot run |
There was a problem hiding this comment.
✅ 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 7f9c9dd. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the second half of the
image-refresh-cronjob.yamlpair. backend#2007.The settled branch clears
tracebloc.io/refresh-skip-streakunderset -euwith no guard (client/templates/image-refresh-cronjob.yaml:360). The read two lines above is deliberately non-fatal; the write was left fatal — and the comment shows the asymmetry was never decided: it reasons about the read and says nothing about the write.That matters because the clear runs before Pass 1. A transient API error on one bookkeeping annotation aborts the tick, so
requests-proxyandresource-monitorstay frozen on the very tick that provedjobs-managerhad recovered.So #1964's own recovery path had its own way to stay frozen.
The change
Make the clear non-fatal and report it. Nothing below reads
SKIP_KEY, so a stale value costs exactly one thing: the next unsettled tick resumes counting from the old number instead of zero, tripping the ceiling earlier — fail-safe in the direction #1964 wants.Logged as a WARNING rather than swallowed, because the shared lesson of both findings is that "images did not update" must never be inferable only from the Job's colour.
Scoped to this one call site on purpose. I checked the other five
kubectl annotatecalls; all are deliberately fatal with recorded reasoning — the streak record fails closed so the ceiling cannot be dodged (#1964), the attempt counter must persist before a restart, and the success reset is split from the digest write precisely so a failure there cannot stick a flap lockout (#626). Only this one is pure cleanup.Tests
Extends #1964's harness, which renders the chart, extracts the script the pod actually runs, and executes it against a stubbed kubectl —
STUB_ANNOTATE_RCwas already a knob.The assertion is that Pass 1 is reached (its per-image "checking <repo>" lines), not the exit status: a status assertion would also pass if the script died later for an unrelated reason.
Mutation-proved. With the template hunk reverted, both new cases fail:
Case 1 failing at
checking tracebloc/jobs-manageris the bug reproduced — the tick really did abort before Pass 1. The mutation anchor was confirmed absent from the reverted file rather than assumed.16/16 green with the fix restored;
helm lintclean;Chart.yamlbumped 1.9.43 → 1.9.44 (chart-version-guard run locally against the real base).Why this is on the critical path
This is the one genuine fix among the three findings holding the
clientleg of the 2026-08-14 staging hop. The other two need no code: backend#2008 is refuted on the arithmetic (Helm'sdaemonSetReadytoleratesDesired - maxUnavailable, and maxUnavailable defaults to 1), and the AMD thread is refuted against the upstream manifest.Related: backend#2007, backend#1964, backend#2008, client#719, client#718.
Note
Low Risk
Single guarded annotate in the image-refresh script with documented fail-safe stale-annotation behavior; other annotate sites stay fatal by design.
Overview
When jobs-manager settles after a skip streak, clearing
tracebloc.io/refresh-skip-streakno longer aborts the CronJob tick underset -e. A failedkubectl annotateis logged as WARNING and execution continues into Pass 1, sorequests-proxyandresource-monitorcan refresh on the recovery tick instead of failing red with no image updates.Stale skip-streak values only shorten the next streak ceiling (fail-safe vs. #1964). Chart version 1.9.44; bats cover “reaches Pass 1 after failed clear” and “WARNING is visible.”
Reviewed by Cursor Bugbot for commit 7f9c9dd. Bugbot is set up for automated code reviews on this repo. Configure here.