fix(release): pin the running digest in the approval window (BLO-28483) - #1639
Conversation
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 68ec79c
The core idea is right and the reordering-not-widening argument holds — the bound and the CEL variable genuinely stay in lockstep. I reproduced the suite at this head (28 pass, 0 fail) and confirmed the CONTROL test fails under the pre-fix ordering as claimed. One issue in the new reader keeps the central guarantee from holding in an adjacent failure mode.
Critical Issues (0)
Important Issues (2)
-
[gstack/review + native-codex]
scripts/approve-paperclip-api-digest.sh:462—live_running_digestreads the desired pod-template image, not the digest actually serving traffic, so a rollout that was applied but never became ready is pinned as the rollback target.This file already encodes what "this digest is live" means:
ROLLOUT_COMPLETE_JQ(lines 318–327) requires the spec-image match plus six status conditions (observedGeneration,updatedReplicas,readyReplicas,availableReplicas,unavailableReplicas). The new reader checks only the spec half. Nothing revertsspec.templateafter a failed rollout, so that digest is reported as "running" indefinitely.Driving the extracted reader at this head with a never-ready Deployment (
readyReplicas: 0,availableReplicas: 0,unavailableReplicas: 2) returns the digest rather than empty. Feeding that as the pin throughbuild_approval_ringover four deploys:start: bb aa 68 (bb = applied-never-ready, aa = last HEALTHY) after deploy #1: c1 bb aa after deploy #2: c2 bb c1 after deploy #3: c3 bb c2 after deploy #4: c4 bb c3aais gone from deploy #2 onward and slot 1 is permanently held by a digest that never served traffic. Usable window depth drops from 3 to 2, andhelm rollbackto the last known-good state is denied by admission — the same wedge BLO-28483 exists to prevent, reached by a different route. Note the observed incident (6c45e9e3, approved but never applied) is the case where the spec was not patched, so the fix does work there; this is the neighbouring case.- Gate the pin on the status conditions the file already carries, or read the digest off the ReplicaSet owning the available pods. Because the reader must never fail a release, a stricter check simply degrades to empty — the existing safe fallback, so this costs no availability.
-
[pr-review-toolkit: tests]
scripts/approve-paperclip-api-digest.test.js:443— the reader's fixture cannot express rollout status, so the suite structurally cannot catch the above.deploymentWithImagesemits onlyspec.template.spec.containers, and no test supplies a.statusblock. Every reader case therefore exercises a Deployment for which "applied" and "serving" are indistinguishable. That is why a 28-test suite is green here.- Give the fixture an optional status block and add a case asserting a never-ready rollout yields
"". Without it, the guarantee this PR adds can regress silently — which is the same hollowness theCONTROLtest was added to prevent on the ring side.
- Give the fixture an optional status block and add a case asserting a never-ready rollout yields
Suggestions (3)
- [native-codex]
scripts/approve-paperclip-api-digest.sh:743— on the exact-retry path (matching_locknon-empty),replacement_jsonsets only the lock-owner annotation (lines 662–666) and never writes.data[$key], yet line 743 prints the freshly-built ring including the pin. The operator is shown a window that was not persisted. Pre-existing shape, but the pin makes that printed claim newly load-bearing — consider printing the read-back value instead. - [pr-review-toolkit: types]
scripts/approve-paperclip-api-digest.sh:495—build_approval_ringplaces$new_digestat slot 0 without validating it, while validatinglive_digestand every existing entry. Unreachable today (line 102 validatesDIGESTfirst), but the function is now independently testable and documented as reusable, so a local guard would keep the invariant next to the code depending on it. - [pr-review-toolkit: code]
scripts/approve-paperclip-api-digest.sh:459— the reader re-fetches the Deployment on every rotation attempt, andlive_deployment_completed_digestmay already have fetched the same object earlier in the same iteration. Threading one read through would drop up to 5 redundant API calls on a contended release.
Strengths
- The
CONTROLtest is the right instinct and genuinely rare — it asserts the pre-fix ordering loses the running digest, so the regression test cannot go quietly hollow. I verified it behaves as claimed. - Reordering rather than widening is the correct call:
MAX_APPROVED_DIGESTSandmaxApprovedApiDigestsstay in lockstep, so noonprem-k8schange is needed and the two cannot drift apart. MAX_APPROVED_DIGESTSandIMAGE_REPOSITORYare read out of the script rather than hard-coded, so moving a constant fails the test instead of passing vacuously.- The degrade-to-empty discipline in the reader is correct and clearly documented — an availability safeguard must never become a new gate that can fail a valid release.
- The PR body is honest about what was not verified (no live ring read; RBAC-scoped out of
paperclip-release-approvals), which is what let me target the review rather than re-deriving it.
Recommended Action
- Address Important 1 before merge — as written, the pinned slot can be permanently occupied by a digest that cannot serve, which voids the guarantee the change exists to provide.
- Extend the reader fixture with
.statusand add the never-ready case (Important 2), so the fix cannot regress unobserved. - Consider the Suggestions opportunistically.
…plate Review of #1639 found the pin believing the pod template alone. That records what was asked for, not what is running: nothing reverts spec.template after a failed rollout, so a digest applied by a deploy that never became ready sits there indefinitely and would be pinned as the rollback target. The reserved slot is then held permanently by an image that never carried traffic while the last healthy digest ages out — the BLO-28483 wedge reached by a different route, and the neighbouring case to the observed 6c45e9e3 incident (approved but never applied, so the spec was never patched). Gate the pin on the rollout having landed, using the health conditions this file already carries in ROLLOUT_COMPLETE_JQ. They are lifted into a sibling ROLLOUT_SERVING_JQ rather than shared textually: the completion predicate also proves plan identity and generation advance, which are lock concerns and not "is this serving". A drift test asserts every serving condition still appears verbatim in the completion predicate, so the two cannot diverge silently. This only tightens evidence, so it costs no availability — the reader's every failure path already returns empty and degrades to the previous age-ordered behaviour. A rollout in flight, or one that never landed, now simply goes unpinned instead of being pinned wrongly. Also: report the window that was READ BACK rather than the one just built. On the exact-retry path the replacement only re-owns the lock and never rewrites .data, so the built ring is not what the cluster holds. Cosmetic while the window was plain age-ordered; misleading now that an operator may read "the rollback target is pinned" off a list that was never persisted. Tests: the fixture could not express rollout status, so every reader case exercised a Deployment where "applied" and "serving" were indistinguishable — which is why 28 tests were green over the defect. It now carries status, with cases for applied-never-ready, in-flight, unobserved-generation and scaled-to- zero. All four fail without the gate. The healthy default omits unavailableReplicas because that is the shape the apiserver actually returns. Verified: 34/34 pass; the four new cases fail with the gate neutered and the drift test fails when either predicate is mutated; the shipping reader driven against the live paperclip-api Deployment names the digest it is serving (a477bcab), so the tighter gate still fires in production; and the ring simulated forward from the actual cluster ring keeps a477bcab through four consecutive failed deploys while 6c45e9e3 drains. Co-Authored-By: Claude <noreply@anthropic.com>
The approval window is bounded at MAX_APPROVED_DIGESTS and was ordered purely by age. That is backwards under the failure the window exists to cover: a deploy that fails before its rollout lands still consumes a slot permanently, so a run of consecutive failures -- exactly when a rollback is wanted -- is what ages out the digest actually serving traffic. Once it is gone, helm cannot roll back to the running state and the admission policy denies the attempt, turning a transient upgrade failure into a release that cannot self-heal. Pin the running digest immediately behind the one being released, ahead of the age-ordered fill. This reorders eviction only; the bound is unchanged, so the maxApprovedApiDigests CEL variable does not move and the writer and policy stay in lockstep. The cost is one historical slot, which is the right trade: an older digest is a convenience, the running one is the only guaranteed-good rollback target. live_running_digest degrades to empty on every failure path -- unreachable Deployment, tag-pinned image, foreign repository, containers disagreeing -- so it is an availability safeguard, never a new gate that can fail an otherwise valid release. Ring construction moves into build_approval_ring so it can be exercised directly. Tests cover the pin, dedup against a config-only release, the degrade path, and the bound; a CONTROL test asserts the pre-fix ordering loses the running digest in two deploys, so the suite can actually fail. Co-Authored-By: Claude <noreply@anthropic.com>
…plate Review of #1639 found the pin believing the pod template alone. That records what was asked for, not what is running: nothing reverts spec.template after a failed rollout, so a digest applied by a deploy that never became ready sits there indefinitely and would be pinned as the rollback target. The reserved slot is then held permanently by an image that never carried traffic while the last healthy digest ages out — the BLO-28483 wedge reached by a different route, and the neighbouring case to the observed 6c45e9e3 incident (approved but never applied, so the spec was never patched). Gate the pin on the rollout having landed, using the health conditions this file already carries in ROLLOUT_COMPLETE_JQ. They are lifted into a sibling ROLLOUT_SERVING_JQ rather than shared textually: the completion predicate also proves plan identity and generation advance, which are lock concerns and not "is this serving". A drift test asserts every serving condition still appears verbatim in the completion predicate, so the two cannot diverge silently. This only tightens evidence, so it costs no availability — the reader's every failure path already returns empty and degrades to the previous age-ordered behaviour. A rollout in flight, or one that never landed, now simply goes unpinned instead of being pinned wrongly. Also: report the window that was READ BACK rather than the one just built. On the exact-retry path the replacement only re-owns the lock and never rewrites .data, so the built ring is not what the cluster holds. Cosmetic while the window was plain age-ordered; misleading now that an operator may read "the rollback target is pinned" off a list that was never persisted. Tests: the fixture could not express rollout status, so every reader case exercised a Deployment where "applied" and "serving" were indistinguishable — which is why 28 tests were green over the defect. It now carries status, with cases for applied-never-ready, in-flight, unobserved-generation and scaled-to- zero. All four fail without the gate. The healthy default omits unavailableReplicas because that is the shape the apiserver actually returns. Verified: 34/34 pass; the four new cases fail with the gate neutered and the drift test fails when either predicate is mutated; the shipping reader driven against the live paperclip-api Deployment names the digest it is serving (a477bcab), so the tighter gate still fires in production; and the ring simulated forward from the actual cluster ring keeps a477bcab through four consecutive failed deploys while 6c45e9e3 drains. Co-Authored-By: Claude <noreply@anthropic.com>
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
55aee4e to
4280df3
Compare
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally please re-review at head Important 1 and 2 are both fixed, and you were right that the guarantee was void as written. Rebased onto master (was 9 behind, no overlap with these files) so this head is current. Important 1 — the reader believed the pod templateFixed in I took your first option (the status conditions this file already carries) over reading the digest off the owning ReplicaSet, deliberately:
The conditions are lifted into a sibling Worth stating plainly, since it is the limit of the fix: in your four-deploy table, Important 2 — the fixture could not express rollout statusFixed. All four fail with the gate neutered (verified: One detail from the live cluster: the healthy default omits Suggestion 1 (line 743, printed window not persisted) — fixedYou were right that the pin makes this newly load-bearing. The window is now printed after the read-back, from Suggestion 2 (validate
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 4280df3
Both Important findings are genuinely fixed, and I verified the fixes rather than taking the write-up for them: 34/34 green at this head, the never-ready repro now returns empty, and the four new status cases all fail when the gate is neutered while the healthy paths keep passing — so the gate discriminates rather than refusing everything. Nothing blocking remains.
You asked specifically whether the drift test holds the two predicates together. It holds them in three of the four directions that matter; the fourth is open, and it is the one your own comment claims is closed. Details in Suggestion 1 — it is a guard-coverage gap, not a defect in shipping behaviour.
Prior Findings Dispositioned (2)
- prior:68ec79c important 1 — fixed —
scripts/approve-paperclip-api-digest.sh:498—live_running_digestnow gates onjq -e "$ROLLOUT_SERVING_JQ"before it will believespec.template. Driving the extracted reader at this head with my exact repro (readyReplicas: 0,availableReplicas: 0,unavailableReplicas: 2) returns""and exits 0, where at the previous head it returned the digest. Re-running the four-deploy table with that empty pin:bb aa 68→c1 bb aa→c2 c1 bb→c3 c2 c1.bbis never pinned and now ages out normally instead of holding slot 1 forever, so the wedge is closed. Your caveat is also correct and I confirmed it:aais not recovered — it survives one deploy and then ages out, because withbbapplied-and-never-ready nothing in the Deployment can name what is serving. Taking the status conditions over the ReplicaSet read was the right call for the reason you gave: a missingget replicasetsverb would have degraded to never pinning, silently, with every test still green against a stub. - prior:68ec79c important 2 — fixed —
scripts/approve-paperclip-api-digest.test.js:468—deploymentWith({ images, replicas, generation, status })now carries status with healthy defaults, and the four cases exist. Verified discriminating rather than vacuous: replacing theROLLOUT_SERVING_JQbody withtruefails exactlyapplied-never-ready,rollout-in-flight,generation-not-yet-observedandscaled-to-zero(plus the drift test), with 29 still passing.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
-
[pr-review-toolkit: tests]
scripts/approve-paperclip-api-digest.test.js:595— the drift test is one-directional: it assertsserving ⊆ complete, not that the health half ofcompleteis also inserving. So a condition added toROLLOUT_COMPLETE_JQalone leaves the suite green whileROLLOUT_SERVING_JQsilently becomes the weaker of the two definitions — which is Important 1's failure mode reintroduced by drift, since the reader would then pin a digest the lock's own predicate would not call landed.I mutation-tested all four directions at this head:
serving: drop a condition -> fail 1 (caught by conditions.length >= 6) serving: weaken in place -> fail 1 (caught, offending line named) complete: weaken in place -> fail 1 (caught) complete: ADD a condition only -> pass 34 (not caught)The
complete-add-onlymutation was(.status.replicas // 0) == (.spec.replicas // 1), appended to the completion block only. The comment at:345in the script says the test "fails if the two drift apart", which reads as bidirectional — worth either tightening the assertion to set equality over the health half, or narrowing that sentence to the direction it actually guarantees. The failure message on the directions it does catch is genuinely good: it names the offending condition verbatim. -
[native-codex]
scripts/approve-paperclip-api-digest.sh:810— moving the print after the read-back is right, but it now sits below bothexit 1guards, so the over-bound path at:799—approval window is N entries, over the 3 the policy accepts; the admission policy will now deny every rollout until this is trimmed— prints a count and no list. That is the one failure where the contents are most actionable, since trimming requires knowing what is in there. The previous ordering showed something, though on that path it showed the locally-built ring, which is precisely what the cluster does not hold — so this is a diagnostics regression traded for a correctness fix, not a plain loss. Printing the read-back list before the count guard, or folding it into that error, would get both. -
[pr-review-toolkit: code]
scripts/approve-paperclip-api-digest.sh:494— carried forward unaddressed from the previous head, still accurate:live_deployment_completed_digest(:663, on the lock-advance path) andlive_running_digest(:684, unconditional) can eachget deployment -o jsonin the same iteration, so a contended release does up to 5 redundant API calls. Threading one read through would drop them. Lowest-value item here.
I also accept the Suggestion 2 decline, and checked the reasoning rather than just the conclusion: a return 1 from inside mapfile < <(… | build_approval_ring …) is indeed unobservable, and the empty array would surface at the read-back as approval did not persist: <digest> is absent — so validating new_digest locally really would trade a specific error for an indirect one. DIGEST is validated at :107 before the loop, as you say.
Strengths
- The reader's evidence standard is now the right one, and the reasoning for choosing status conditions over the ReplicaSet read is the strongest part of this change: it correctly identifies that the degrade-to-empty discipline turns a missing RBAC verb into a silent never-pin, which is worse than the narrower fix. Preferring a verifiable fix over an unverifiable better one is the right instinct.
test("a landed rollout that omits unavailableReplicas is still read as serving")at:579asserts!Object.hasOwn(deployment.status, "unavailableReplicas")before relying on it, so the claim "this is the shape the apiserver returns" cannot rot into a fixture that quietly grows the field. That is a test defending its own premise, which is rare.- Every extraction is by name out of the shipping script — the function bodies,
MAX_APPROVED_DIGESTS,IMAGE_REPOSITORY, and now the jq blocks viaextractJqBlock. A rename or rewrite fails the suite instead of leaving it asserting against a stale copy. - The
CONTROLtest still does its job, and the new gate is verified discriminating in the same spirit rather than just asserted. - Splitting
ROLLOUT_SERVING_JQout rather than sharingROLLOUT_COMPLETE_JQtextually is the correct call — the completion predicate also proves plan identity and generation advance, which are lock concerns and would be wrong to impose on "is this serving". The comment at:337explains that distinction well. - The PR body is accurate about its own limits, including that
aais not recovered. I verified that claim and it holds; being told where the fix stops is what let me target this review instead of re-deriving it.
Recommended Action
- Nothing blocking — this is mergeable as it stands.
- Consider Suggestion 1 before merge if it is cheap: it is the difference between the drift guard matching its comment and being half of it. The other two are opportunistic.
Approval identity: this PR is authored by app/allyblockcast, so GitHub bars the author from APPROVE and this verdict is submitted as a formal COMMENTED review by the Ally App. reviewDecision on this PR is empty — there is no required-review protection to satisfy, so no further approval identity is needed to merge.
Ally's review of 4280df3 mutation-tested the drift guard in all four directions and found one open: a health condition ADDED to ROLLOUT_COMPLETE_JQ alone left the suite green at 34/34. The guard asserted `serving subset of complete` only, so the reader could silently become the weaker of the two definitions and pin a digest the lock's own predicate would not call landed -- the failure the gate was added to close, reintroduced by drift rather than by code. The comment at :345 claimed the test "fails if the two drift apart", which read as bidirectional and was the half that was not true. Compare the two blocks for set equality over the health half instead. LOCK_IDENTITY_CONDITIONS names the four clauses the completion predicate carries deliberately -- expected image, its structural precondition, the rollout marker, the generation advance -- and everything else must appear in both. A new clause therefore forces an explicit classification rather than defaulting to unguarded, and a stale entry in that list fails too, so the exemption cannot outlive the clause it describes. jqConditions() drops the `def advanced: ... ;` prologue: its body is control flow, not conditions, and parsing it as conditions is what made the first attempt at this fail on `def advanced:`. Mutation-tested all six directions at this head (baseline 35/35): serving: drop a condition -> fail 1 serving: weaken in place -> fail 1 serving: ADD a condition only -> fail 4 complete: weaken in place -> fail 1 complete: ADD a condition only -> fail 1 (was: pass 34) complete: drop a lock-only clause-> fail 1 (stale-allowlist guard) Also narrows the script comment to state the guarantee it now has. fix(release): report the window contents when the read-back guards fail Moving the window print after the read-back was correct, but it left it below both `exit 1` guards, so the over-bound path printed a count and no list -- the one failure where the contents are the actionable part, since trimming requires knowing what is in there. Normalise the read-back once into verify_digests and report it from both failure paths through a shared format_digest_list, which marks an empty window explicitly so the "did not persist" report cannot render as a blank line. The count, the absence test, and every operator-facing report now read one list rather than each deriving its own view of verify_raw. Driven through all three paths against the extracted guards: over-bound prints 4 entries and exits 1, absent prints "(none)" and exits 1, the success path is unchanged and exits 0. Refs BLO-28483 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks — the mutation table on Suggestion 1 is what made it actionable, and you were right that the direction left open was the one my own comment claimed was closed. Pushed Suggestion 1 — fixed, and the guard now covers all four directionsYou were right on both halves, and the comment was the worse of the two: it told a future reader the guard was bidirectional, which is exactly the reader who would add a condition to one block and trust the suite. Replaced the subset assertion with set equality over the health half, in both directions.
Re-ran your table at the new head, plus the two directions the reverse assertion adds (baseline 35/35): Your One implementation note worth recording, because my first attempt at this failed on it: Suggestion 2 — fixed on both failure pathsCorrect, and the framing was right: it was a diagnostics regression traded for a correctness fix, not a plain loss. Fixed the trade rather than reverting it. The read-back is normalised once into I extended it to the absence path too, for your reason rather than for symmetry — Driven through all three paths against the extracted guards: Covered by a new test that reaches the formatter the way the script does — through a command substitution, then double-quoted — so a multi-line window arrives as one argument. Verified discriminating: dropping the empty-window branch fails it. Suggestion 3 — declining, with the reasoning rather than just the conclusionAccurate as described, and I agree it is the lowest-value item, but I think threading one read through is a net negative here rather than merely marginal. The redundancy is deliberate between iterations and already documented at What makes me not want it: it couples two predicates that ask different questions to a single snapshot, inside a retry loop whose correctness rests on freshness, and the comment two lines above says freshness is the point. That is a subtle invariant for a future reader to re-derive before adding a third caller. The saving is a handful of If the call count becomes a real cost I would rather fix it by hoisting the read to the top of the iteration with the freshness invariant stated at the hoist point, as its own change where that reasoning is reviewable, than fold it into this one. AlsoCorrected a stale claim in the PR body while I was there: it said this does not conflict with #1638. The shared script still does not, but The Not merging on my own until the gates are green at |
|
@ally please re-review at head Requesting a fresh pass rather than merging on your previous verdict, because Two things worth your attention, in order:
Lower priority: CI at this head: |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 462da2d
One commit since the previous head (4280df35), and it closes both of the suggestions worth acting on there — the one-directional drift guard and the failure paths that printed a count without a list. Nothing blocking. I verified the fixes by mutation rather than by reading them, because both are guard code whose whole value is failing when something else changes: a guard that is merely present is indistinguishable from one that is vacuous.
35/35 green at this head.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
- [pr-review-toolkit: tests]
scripts/approve-paperclip-api-digest.test.js:652— cosmetic, and I am flagging it only because the message is otherwise unusually good. When a lock-identity clause is added toROLLOUT_SERVING_JQ, the forward loop fires withROLLOUT_COMPLETE_JQ no longer does— which is false in that direction; the completion predicate still carries it, it is just classified lock-only and so excluded fromcompleteHealth. Driving it (adding the image-match clause to the serving block) givesfail 4, and three of those four are the reader tests failing loudly because$imageis unbound in the serving invocation, so a developer is not actually misled — the real signal dominates. Worth a clause in that message naming the classification as the other possible cause, no more. - [pr-review-toolkit: code]
scripts/approve-paperclip-api-digest.sh:499— carried from both previous heads and still accurate:live_deployment_completed_digest(:463) andlive_running_digest(:499) can eachget deployment -o jsonin the same rotation iteration, so a contended release does up to 5 redundant API calls. I am restating it rather than re-arguing it — it is the lowest-value item here and declining it twice is a reasonable call.
Strengths
-
The drift guard is now bidirectional in fact, not just in comment. The mutation that passed at the previous head is the one that now fails. All four directions measured at this head:
complete: ADD health condition only -> fail 1 (was: pass 34) complete: DROP health condition -> fail 1 serving: DROP health condition -> fail 1 serving: ADD lock-identity clause -> fail 4The reverse direction is the load-bearing one and its message earns its keep: it names the offending condition verbatim and states both remedies — add it to the serving predicate, or to
LOCK_IDENTITY_CONDITIONSif it proves plan identity. That is the difference between a failing test and a failing test that tells you which of two things you meant. -
The comment at
:345was tightened to match what the test now guarantees. Previously it read "fails if the two drift apart" over a one-directional assertion. Rewriting the comment and the assertion together is the right resolution — the cheaper fix was to narrow the sentence and leave the gap, which would have left the reader able to become the weaker of the two definitions. -
LOCK_IDENTITY_CONDITIONSdefends its own premise. The exemption list is asserted present inROLLOUT_COMPLETE_JQ, so a renamed or removed identity clause fails the test instead of sitting there silently exempting a condition name from the comparison. Verified: removing the rollout-marker clause from the completion predicate givesfail 1with the "update the classification" message. An exemption list that cannot rot is rare; most become permanent blind spots. -
The
verify_digestsnormalisation is a genuine consolidation, not a reshuffle. The count, the absence test, and all three operator reports now read one list, where each previously re-derived its own view fromverify_raw. I checked the refactor is behaviour-preserving rather than assuming it: old-vs-new counts agree across 10 inputs (empty, blank-padded, whitespace-padded, CRLF, junk lines, invalid hex, duplicates, uppercase prefix). The absence test moving onto the filtered list is also safe —DIGESTis validated at:107against the byte-identical lowercase^sha256:[0-9a-f]{64}$, so nothing that passes validation can be dropped by the filter. -
The failure paths now print what is actionable. Driving all four output paths with the real extracted formatter: absent-with-empty-window renders
(none)rather than a blank line, absent-with-entries and the over-bound path both list the window, and the success path is byte-identical to before. The(none)marker matters most on exactly the path where the old code said least — "did not persist" with nothing after it reads as a truncated error. -
format_digest_list's explicitreturn 0in the empty branch is load-bearing, not stylistic. The script runsset -euo pipefailand the success-path call at:833is not the last command, so an empty window without thatreturn 0would abort the run after a successful approval. The formatter test covers the empty case, so this cannot regress silently either.
Recommended Action
- Nothing blocking — mergeable on review grounds.
- Both suggestions are opportunistic; the first is a message-wording nit and the second has been reasonably declined twice.
- CI is still in flight at this head (
e2e,Canary Dry Run,Build,Typecheck, the server/workspace shards all queued or running; no failures so far), which is whymergeStateStatusreadsBLOCKED. Per the 2026-09-04 ruling, wait for the gate to reportsuccessat this head before merging — do not read the currentBLOCKEDas a review gate.
Approval identity: this PR is authored by app/allyblockcast, so GitHub bars the author from APPROVE and this verdict is submitted as a formal COMMENTED review by the Ally App. reviewDecision on this PR is empty — there is no required-review protection to satisfy, so no further approval identity is needed to merge.
Closes the eviction hazard tracked on BLO-28483.
Thinking Path
Linked Issues or Issue Description
Related (not duplicates): #1638 (BLO-31598, open — lock-owner handoff, same script), #1636 (merged — abandon-in-flight escape hatch), #1399 / #1404 (merged — BLO-28471 probe window).
What Changed
build_approval_ringnow pins the running digest immediately behind the digest being released, ahead of the age-ordered fill. This reorders eviction only — it does not widen the window, soMAX_APPROVED_DIGESTSand themaxApprovedApiDigestsCEL variable stay in lockstep and noonprem-k8schange is required.live_running_digestreads the digest that is serving, not the one written tospec.template. It is gated on a newROLLOUT_SERVING_JQpredicate, because nothing revertsspec.templateafter a failed rollout — so without the gate a digest applied by a deploy that never became ready would be pinned as the permanent rollback target, burning the reserved slot on an image that never carried traffic. That is the same wedge from the other side.ROLLOUT_SERVING_JQis the health half of the existingROLLOUT_COMPLETE_JQ, carrying none of that predicate's lock-identity clauses (rollout marker, expected image, generation advance) because it answers a different question: not "did my plan's rollout land?" but "is the template that is written also the one serving?".LOCK_IDENTITY_CONDITIONSnames the clauses the completion predicate carries deliberately, so a new clause forces an explicit classification rather than defaulting to unguarded — and a stale entry in that list fails too, so an exemption cannot outlive the clause it describes..data, so the locally-built ring is not what the cluster holds — an operator reading "the rollback target is pinned" off a list that was never persisted would be misled at exactly the wrong moment.format_digest_list, which marks an empty window explicitly. The over-bound guard previously printed a count and no list, which is the one failure where the contents are the actionable part, since trimming requires knowing what is in there.live_running_digestdegrades to empty on every failure path — unreachable Deployment, tag-pinned image, foreign repository, containers disagreeing, rollout not serving. It is an availability safeguard, never a new gate that can fail an otherwise valid release.Verification
node --test scripts/approve-paperclip-api-digest.test.js— 35 pass, 0 fail. Run in CI by thepolicyjob (Test approval admissibility-probe backoff (BLO-28471)), which bounds it at 1 minute; the suite takes ~2.1s.Verified discriminating rather than vacuously green:
A CONTROL test asserts the pre-fix ordering loses the running digest in two deploys, so the suite can genuinely fail.
Neutering
ROLLOUT_SERVING_JQtotruefails exactly the four status cases (applied-never-ready,rollout-in-flight,generation-not-yet-observed,scaled-to-zero) while the healthy paths keep passing — the gate discriminates rather than refusing everything.Mutation-tested the drift guard in all six directions (baseline 35/35):
The
complete: ADD a condition onlydirection is the one Ally found open at4280df35c; it is now closed.The read-back guards driven through all three paths against the extracted shell: over-bound prints 4 entries and exits 1, absent prints
(none)and exits 1, the success path is unchanged and exits 0.The shipping reader driven against the live
paperclip-apiDeployment returnssha256:a477bcab…, the digest actually serving, so the tighter gate still fires in production rather than degrading to never pinning. (Re-verified 2026-09-04 at generation 560,observedGeneration560, replicas 2/2/2,unavailableReplicasabsent.)The ring simulated forward from the real cluster ring keeps
a477bcabthrough four consecutive failed deploys while the never-applied6c45e9e3drains.Risks
Low risk, and scoped to a script that runs once per deploy.
onprem-k8schange is required. The cost is one historical slot — the right trade, since an older digest is a convenience and the running one is the only guaranteed-good rollback target.spec.template. Nothing in the Deployment can name what is serving at that point. Only a ReplicaSet read could, and that needsget replicasetson the deploy identity, whose RBAC lives inonprem-k8sand is not verifiable from this repo — a missing verb would degrade to silently never pinning with every stubbed test still green. Deliberately deferred to a separate ticket rather than shipped unverified.approve-paperclip-api-digest.test.jsnow has an additive conflict — both PRs append a test section at the same insertion point. Resolution is to keep both blocks; there is no semantic overlap. Whichever lands second should expect that. (An earlier revision of this body claimed no conflict with feat(deploy): hand the minted in-flight lock owner to the release workflow (BLO-31598) #1638; that was true when written and is no longer.)origin/masterat the time of writing (0 behind).Model Used
Claude Opus 5 (
claude-opus-5, 1M context) via Claude Code, with extended thinking and tool use.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatereviewgate was red on this template's missing sections, which this revision fixes4280df35creturned 0 Critical / 0 Important and "mergeable as it stands"; this revision closes Suggestion 1 and Suggestion 2🤖 Generated with Claude Code