[bborbe]

Why

2026-10-21 20:27Z a stale agent-sun helm apply deleted PVC a2a/update-go-cache on sun, deadlocking every update-go Job. Helm prunes resources a new render omits unless the release record carries helm.sh/resource-policy: keep.

Change

• helm/templates/agents.yaml: agent PVCs always render helm.sh/resource-policy: keep.
• Chart 0.15.0 → 0.16.0; CHANGELOG ## Unreleased bullet.

Verification

• make test-helm green.
• helm template t helm -f sun/agent/values.yaml --set executor.existingSecret=x → both PVCs (agent-pi, update-go-cache) carry helm.sh/resource-policy: keep.

Chart publish (manual helm package + helm push 0.16.0) follows the merge.

[ben-s-pull-request-reviewer]

{
  "verdict": "approve",
  "summary": "The chart change is minimal and does what it says for the incident case: `helm.sh/resource-policy: keep` renders on every Helm-managed agent PVC and Helm 3.16.4 honours it when an upgrade prune drops the claim. It is broader than the changelog and the new template comment admit (the PVCs now also survive `helm uninstall`, and the prune guard only engages once the annotation is live on the object), and the new annotation has no render-test coverage — all raised as non-blocking comments. Selector traceability: 5 candidate judgment rules, 1 applicable (`changelog/conventional-prefix-required` — no violation; `fix(helm):` is the repo's established scoped-prefix convention), 4 skipped (`git-commit/feature-branch-no-tag` — no tag at PR head; `git-commit/imperative-mood` — subject is imperative `keep`; `go-security/resource-ownership` and `go-security/tenant-isolation` — no Go code in the diff); mechanical funnel reported 0 findings across 79 YAMLs, and `precommit` was skipped (selector mode) — CI covers lint+test.",
  "comments": [
    {
      "file": "helm/templates/agents.yaml",
      "line": 96,
      "severity": "major",
      "blocking": false,
      "blocking_reason": "",
      "message": "The `keep` policy is not scoped to upgrade pruning: Helm 3.16.4's uninstall path filters kept resources from the release-record manifest (`filterManifestsToKeep` in `pkg/action/action.go`, called from `pkg/action/uninstall.go`), so after this lands every agent PVC also survives `helm uninstall`, and a PVC whose agent is removed from `values.yaml` is orphaned forever (a later reinstall under a different release name then fails with an ownership conflict). Neither the CHANGELOG bullet nor the template comment mentions that; the stated intent covers only the stale-apply prune. Please document the permanent-keep behaviour and the manual cleanup path (`kubectl delete pvc <volumeClaim|name> -n <namespace>`) in `helm/values.yaml` next to the `agents` schema and/or `helm/README.md`, and consider whether a `retainVolume` value (default true) is wanted for ephemeral installs. *(rule: `changelog/unreleased-entry-required` scope note — the Unreleased bullet describes only the prune case)*"
    },
    {
      "file": "helm/templates/agents.yaml",
      "line": 89,
      "severity": "minor",
      "blocking": false,
      "blocking_reason": "",
      "message": "This comment states the mechanism incorrectly, and the error matters operationally: on the upgrade-prune path the annotation is read from the LIVE object, not the release record — `kube.Client.Update` calls `info.Get()` and then checks `metadataAccessor.Annotations(info.Object)` (`pkg/kube/client.go` in Helm 3.16.4). The release-record read is the `helm uninstall` path only. Consequence: the guard only protects a PVC after a successful 0.16.0 upgrade has written the annotation onto the live object, so the first apply after this chart is deployed must not itself be a stale render that drops a claim (e.g. on sun, apply 0.16.0 with the full values first; if the incident cluster's PVC is already gone, a one-off manual patch is not needed for the annotation but the claim must be recreated). Reword to say the policy is read from the live object at prune time and state the rollout order."
    },
    {
      "file": "helm/templates/agents.yaml",
      "line": 95,
      "severity": "major",
      "blocking": false,
      "blocking_reason": "",
      "message": "The new annotation has no render assertion: `hack/test-helm-render.sh` (run by `make test-helm` from `make test`) only checks `rerunOnLostClaim` on a fixture (`rr-agent`) that declares no `volumeMountPath`, so its render contains no PVC at all and the annotation is untested end to end. Add a case with `volumeMountPath` set asserting `helm.sh/resource-policy: keep` appears on the rendered PVC, plus cases asserting no PVC is rendered for `type: service` and for an agent without `volumeMountPath` — otherwise a later template edit can silently drop the annotation and re-open the incident this PR fixes."
    }
  ],
  "concerns_addressed": [
    {
      "concern": "helm.sh/resource-policy: keep is not scoped to prune-only. Helm honours the annotation on prune AND on helm uninstall AND when an agent is dropped from values, so every agent PVC now survives release deletion forever. The new template comment and the CHANGELOG only describe the stale-apply prune case; verify the permanent orphaned-storage leak is intended and note the manual cleanup path.",
      "disposition": "addressed",
      "detail": "Confirmed against Helm 3.16.4 source (uninstall filters kept resources from the release-record manifest; upgrade prune skips them too). Raised as a non-blocking major comment on helm/templates/agents.yaml:96 asking for documentation of the permanent keep, the cleanup command, and an optional retainVolume value; the behaviour is not a correctness defect for the incident case, so it does not block."
    },
    {
      "concern": "Helm reads the policy from the live object fetched during prune (kube.Client.Update calls info.Get before checking ResourcePolicyAnno), so the guard only protects a release after 0.16.0 has been applied to it at least once. Confirm the rollout order on sun (apply 0.16.0 before any further stale apply) and whether the incident cluster already needs a one-off manual patch.",
      "disposition": "addressed",
      "detail": "Confirmed: pkg/kube/client.go Update() calls info.Get() and reads the annotation from the live object, so the premise holds and the template comment's 'release record' wording is wrong for the prune path. Raised as a non-blocking minor comment on helm/templates/agents.yaml:89 with the corrected mechanism and the rollout-order implication."
    },
    {
      "concern": "make test-helm renders agents.yaml but asserts only rerunOnLostClaim. No case covers the new annotation, nor that a type: service agent and a volumeMountPath-less agent still render no PVC. The PR's verification is a manual helm template run; add a render assertion so a later template edit cannot silently drop the annotation.",
      "disposition": "addressed",
      "detail": "Confirmed by reading hack/test-helm-render.sh: the fixture has no volumeMountPath, so no PVC is rendered and the annotation is entirely uncovered. Raised as a non-blocking major comment anchored to the added annotation (helm/templates/agents.yaml:95) asking for the annotation plus the no-PVC cases."
    },
    {
      "concern": "The keep policy is unconditional and not exposed as a values key, so installs with throwaway or ephemeral agent volumes have no opt-out and must delete PVCs by hand. Consider whether agents[].retainVolume (default true) is warranted, or at minimum document the unconditional behaviour in helm/values.yaml next to the agents schema.",
      "disposition": "addressed",
      "detail": "Confirmed: the annotation is hard-coded with no values key. Folded into the helm/templates/agents.yaml:96 comment, which asks for documentation in helm/values.yaml/helm/README.md and suggests an optional retainVolume (default true)."
    }
  ]
}

superseded by new automated review

[ben-s-pull-request-reviewer]

{
  "verdict": "approve",
  "summary": "The change is sound: `helm.sh/resource-policy: keep` is added to the agent PVC, the prune-time claim in the comment/CHANGELOG matches Helm v3.16.4's upgrade-prune path (which fetches the live object before checking the policy), the 0.15.0→0.16.0 chart bump is consistent with repo convention, and `make precommit` → `make test` → `test-helm` runs the new cases in CI. Selector: 7 candidates, 7 applicable, 0 skipped, 0 mechanical findings; only three non-blocking test-hygiene nits, and the only docs gap is that the operator cleanup guidance lives in the CHANGELOG/template comment rather than helm/README.",
  "comments": [
    {
      "file": "hack/test-helm-render.sh",
      "line": 210,
      "severity": "nit",
      "blocking": false,
      "blocking_reason": "",
      "message": "**Nice to Have** — the annotation assertion is a whole-output exact-line grep (`grep -qx '    helm.sh/resource-policy: keep'`), not scoped to the PVC document: it would still pass if the annotation leaked into another rendered document, and it fails on a cosmetic re-indent of the annotations block. Select the PVC document first (e.g. an awk range from `kind: PersistentVolumeClaim` to the next `---`) and assert the annotation inside it."
    },
    {
      "file": "hack/test-helm-render.sh",
      "line": 191,
      "severity": "nit",
      "blocking": false,
      "blocking_reason": "",
      "message": "**Nice to Have** — the file header (lines 6-12) still describes only the rerunOnLostClaim and excludeAssignees checks. Extend it to mention the new agent-PVC render cases so the script's stated contract matches what it asserts."
    },
    {
      "file": "hack/test-helm-render.sh",
      "line": 219,
      "severity": "nit",
      "blocking": false,
      "blocking_reason": "",
      "message": "**Nice to Have** — the new cases cover volumeMountPath presence and the `type == service` exclusion, but not a `volumeClaim` that differs from the agent name nor `storageClassName`, so those branches of the same template remain unexercised. The pvc-service case is genuine: it sets `type: service`, and dropping the exclusion would render 1 PVC and fail the case."
    }
  ],
  "concerns_addressed": [
    {
      "concern": "correctness: helm.sh/resource-policy keep is read from the LIVE object at prune time, so it only protects a claim after this chart has been applied once. Verify the comment and CHANGELOG state this accurately and that the rollout ordering (apply 0.16.0 everywhere before any stale-apply risk) is acceptable.",
      "disposition": "not-an-issue",
      "detail": "Verified against Helm v3.16.4 (the version the test installs): `Client.Update` calls `info.Get()` to fetch the live object before reading `ResourcePolicyAnno` on the upgrade-prune path, so the live-object statement is accurate for prune; uninstall reads the release manifest, which carries the annotation after one 0.16.0 apply. The 'protects only after this chart has been applied once' conclusion holds for both paths, so the comment and CHANGELOG are accurate."
    },
    {
      "concern": "correctness: The keep policy is unconditional — no values toggle. PVCs now survive helm uninstall and survive an agent being removed from values, so orphaned claims accumulate and keep consuming storage quota with no cleanup path other than manual kubectl delete pvc. Check whether an opt-out (e.g. keepVolume) is warranted, and whether any alerting covers orphaned claims.",
      "disposition": "not-an-issue",
      "detail": "Deliberate trade-off: deleting the claim on uninstall/agent removal is the bug this PR fixes. Orphan cleanup is documented in the template comment and the CHANGELOG bullet (`kubectl delete pvc <volumeClaim|name> -n <namespace>`), and the ResourceQuota only counts pods, so orphaned claims do not break the concurrency cap. No alerting exists, but an opt-out value would reintroduce the data-loss path this fix closes; no change required for a bug fix."
    },
    {
      "concern": "correctness: Scope check: only agent PVCs are covered. Confirm no other standalone PVC templates exist in the chart (executor, cluster-assigner, etc.) that a stale apply could also prune. StatefulSet volumeClaimTemplates are controller-managed and out of scope, but the PR should say so explicitly.",
      "disposition": "not-an-issue",
      "detail": "Repo-wide grep under helm/: `agents.yaml` is the only standalone `PersistentVolumeClaim`; `controller-statefulset.yaml:151` uses `volumeClaimTemplates` (controller-managed) and no other template renders a PVC (executor and cluster-assigner have none). The agents.yaml comment already states the StatefulSet exclusion explicitly."
    },
    {
      "concern": "tests: The annotation assertion greps for an exact 4-space-indented line and is not scoped to the PVC document. A reindent of the annotations block breaks the test for a cosmetic reason, and a leak of the annotation into another rendered document would still pass. Prefer selecting the PVC document (e.g. awk/yq range) before asserting.",
      "disposition": "addressed",
      "detail": "Raised as a non-blocking nit on hack/test-helm-render.sh:210 — select the PVC document before asserting the annotation."
    },
    {
      "concern": "tests: Coverage gaps: no case for an agent that sets volumeClaim to a name different from the agent name, and none for storageClassName. Also confirm the pvc-service case is genuinely exercising the type==service exclusion and not silently producing zero PVCs because of a values-parse problem.",
      "disposition": "addressed",
      "detail": "pvc-service is genuine: it appends `type: service` to the same values file that renders 1 PVC in pvc-job, so a values-parse problem would fail the pvc-job case and a dropped exclusion would make the count 1 and fail pvc-service. The missing volumeClaim!=name and storageClassName branches are raised as a non-blocking nit on the new cases."
    },
    {
      "concern": "tests: Script header comment still describes only the rerunOnLostClaim and excludeAssignees checks; it should mention the new PVC cases. Also confirm the make test-helm target actually invokes this script in CI, so the new cases are not local-only.",
      "disposition": "addressed",
      "detail": "Header gap raised as a non-blocking nit on hack/test-helm-render.sh:191. CI confirmed: `.github/workflows/ci.yml:33` runs `make precommit`, and Makefile:28-33 wires `test: test-helm` → `bash hack/test-helm-render.sh`, so the new cases run in CI."
    },
    {
      "concern": "docs: Chart bump 0.15.0 to 0.16.0 — check for other places pinning the chart version (sun/agent overlay, ArgoCD Application, docs) that need the same bump, and whether a behavior change to uninstall semantics warrants MINOR per the repo chart-versioning convention.",
      "disposition": "not-an-issue",
      "detail": "No other in-repo chart-version pin: a grep for 0.15.0/chart-version outside CHANGELOG and prompts finds nothing, and helm/README does not pin a version. A MINOR bump (0.15.0→0.16.0) matches the repo's chart convention and is warranted because uninstall/prune semantics change for operators."
    },
    {
      "concern": "security: Keeping PVCs on uninstall retains whatever the volume holds (agent caches, possibly credential material) past the release lifecycle. Confirm this data-retention change is acceptable and documented for operators, and that the manual kubectl delete pvc guidance is discoverable.",
      "disposition": "not-an-issue",
      "detail": "Retention is the intended fix and the cleanup command is documented in the CHANGELOG bullet (line 13) and the template comment. The chart README's agents section does not repeat it, which is an optional docs follow-up rather than a security defect — keeping the claim exposes no credential material beyond what the running pod already has, and the orphan is operator-deletable."
    }
  ]
}