fix(apps): make the post-delete cleanup hooks report failure - #3593

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/apps/harbor/templates/hooks/cleanup.yaml`:
- Around line 141-166: Update the cleanup hook’s Challenge and solver Ingress
sweeps to first verify that the cert-manager API is available using kubectl get
--raw /apis/acme.cert-manager.io/v1. When the API is absent or unavailable, skip
both list/delete loops without calling fail; otherwise preserve the existing
listing, deletion, and failure handling in the challenge and solver cleanup
blocks.

---

Nitpick comments:
In `@hack/app-cleanup-hooks_test.bats`:
- Around line 96-111: Update the kubectl mock’s positional-argument handling so
it collects all object names rather than overwriting obj in the loop. In the
EXISTING check within the NotFound branch, iterate over the collected objs and
return success only when every requested object is listed as existing; preserve
the current behavior for ignore-not-found and failure cases.

The substance is right and the diagnosis holds. Not blocking. Two things I'd ask for before merge, two more worth deciding explicitly rather than leaving implicit.

Verified independently

Deletion scope is unchanged. Every selector is byte-identical to main — qdrant's ordinal-anchored ^persistentvolumeclaim/qdrant-storage-<release>-[0-9]+$, harbor's -l release=<release>-system, the <release>-ingress-tls by-name delete, and both host-scoped jsonpath filters. Only indentation moved. Nothing new is in range.

The uninstall does not strand the release, but the Flux layer needs saying. Checked against the sources rather than the description:

• helm/pkg/action/uninstall.go:142 appends the post-delete hook error to errs instead of returning it, :147 sets StatusUninstalled, :154-156 runs purgeReleases regardless, :163 returns uninstallation completed with N error(s). The release does leave storage, as described.
• Under helm-controller v1.4.3 the first reconcile still fails: internal/reconcile/uninstall.go reaches r.failure(...) and returns the error, reconcileReleaseDeletion tolerates only ErrNoLatest, so reconcileDelete never reaches RemoveFinalizer. The next reconcile is what clears it — Helm's storage no longer holds the release, so Releases.History returns driver.ErrReleaseNotFound, helm-controller marks UninstallSucceeded and drops the finalizer. Same outcome via the "is already deleted" branch when spec.uninstall.keepHistory is set.

So kubectl delete harbor/qdrant <name> completes after one reconcile backoff, with UninstallFailed visible on the HelmRelease in between. Worth adding to the description: "the release itself is not stranded" is true of Helm, and the layer that actually drives these releases is the part a reader will worry about.

The tests earn their place. I reverted three things independently: harbor's Certificate fail back to || echo "WARNING…" (bats red, named the object), qdrant's --ignore-not-found on the xargs delete (bats red on the already-gone case), and hook-failed added to both delete-policy annotations (both helm-unittest suites red). The mutation claims hold.

Runs: qdrant 60/60, harbor 17/17, full make helm-unit-tests green, the new bats suite 11/11, and image-pin-consistency / downstream-trigger-map / pod-label-census_test / md-no-hardwrap all pass. Both charts render in http01, dns01 and custom-host modes.

1. The new error path is defeated by the failure most likely to reach it

Neither hook bounds a single call. kubectl get carries no --request-timeout, and kubectl delete waits for the object to be finalized with no timeout — harbor/templates/hooks/cleanup.yaml:106,124,148,157 and qdrant/templates/hooks/cleanup-pvc.yaml:101,107. A throttled or unreachable apiserver, or a PVC still held by kubernetes.io/pvc-protection because a -system pod is still terminating, produces no ERROR: line at all: the hook hangs until Helm's hook timeout and the operator gets Helm's deadline instead. That is precisely the outcome the backoffLimit comment argues against, arriving by a shorter route than seven pods of backoff.

packages/apps/bucket/templates/hooks/cleanup-acme.yaml:13-16 already solved this in what is otherwise the same ACME sweep, and says why:

# Bound the whole cleanup: never let a stuck finalizer or a hung API call
# leave this post-delete hook running indefinitely and block the uninstall.
activeDeadlineSeconds: 120
backoffLimit: 0

plus --request-timeout=30s on every call and --wait=false on every delete.

--request-timeout=30s on the reads is the part I'd ask for outright: it turns a hung read into this hook's own non-zero exit with its own message, which is what the PR is for, with nothing traded away. --wait=false on the deletes and an activeDeadlineSeconds outer bound are a judgement call worth making explicitly — activeDeadlineSeconds does cost the pod log, since the Job controller terminates the pods when it fires, so it is a weaker instrument here than in bucket. Either way it should be a decision rather than an omission.

2. Three of the five cleanup hooks still have the defect this PR is about

• packages/apps/bucket/templates/hooks/cleanup-acme.yaml:66,87,98 is the same ACME sweep as harbor's. Every delete is still || echo "WARNING: …" and the script still ends on an unconditional echo "ACME cleanup complete.". It has the listing-capture fix already and none of the exit-code fix. It is not mentioned in the description.
• mariadb and clickhouse: the description diagnoses their missing retry cap correctly — a real failure spends the full default backoff and reaches the operator as a timeout rather than as the error the hook wrote — and then leaves it in place. One line per chart.

Scoping a PR is legitimate and I am not asking to grow this one indefinitely. But the title says "the post-delete cleanup hooks" and the result is five hooks in three dialects with three still wrong in ways this PR names. Either pull bucket in (same code, ~10 lines) and cap mariadb/clickhouse, or state in the description which are deliberately out of scope and where they are tracked.

3. The leftover accounting is short by three objects

helm/pkg/action/hooks.go:112-121: the hook-succeeded sweep is a second loop that runs only once every hook in the event has succeeded, and deleteHookByPolicy(h, HookFailed, …) on failure applies to the failing hook alone. So when the Job (weight 10) fails, the ServiceAccount (weight 0), Role and RoleBinding (weight 5) have already succeeded and are never collected either. The comments and the release note both say "the Job and its pods".

Worth naming the rest. A ServiceAccount plus a Role carrying delete on persistentvolumeclaims, certificates, challenges and ingresses, left in a tenant namespace until the next uninstall of the same release name, is a different thing to leave behind than a pod holding a log — and unlike the pod it is not evidence. Adding hook-failed to the RBAC objects would not help, for the reason above; this is a documentation fix, and possibly an argument for a narrower Role.

One dead end worth a line in the comment so the next reader does not go looking: helm.sh/hook-output-log-policy: hook-failed looks like the way to get the log out without retaining anything, and it does nothing here — Helm defaults HookOutputFunc to io.Discard (pkg/action/action.go:429) and helm-controller never calls SetHookOutputFunc. Retaining the Job really is the only channel under Flux.

4. backoffLimit: 1 against bucket's backoffLimit: 0

Twelve lines argue that six is wrong and zero is wrong. A chart in this repo already picked zero, for the stated reason that the Job must be bounded. Either reconcile with it in the comment or align the two; as it stands the repo will hold both values with two mutually-unaware essays attached.

5. Duplication, and where this is heading

Roughly 35 lines — the backoffLimit paragraph and the failures/fail() preamble with its rationale — are copy-pasted verbatim into two charts, and the closing block once more in each. Across the five hooks there are now three accumulator shapes (failures here, cleanup_failed in mariadb, residue_from in #3592), two severity prefixes (ERROR: here, WARNING: in #3592) and two helper names (fail / backstop).

Both charts already depend on cozy-lib, and harbor already calls cozy-lib.image from this very file. A cozy-lib.cleanupHook.preamble / .epilogue pair would hold this once, and the rationale with it. It would also get the essay out of the rendered Job spec, where it ships to every cluster on every uninstall and has no reader.

On #3592 specifically: the substance is consistent — same diagnosis method (capture the listing, keep its status, walk the whole script for the shape) and same test method. The exit 0 / exit 1 asymmetry is correct and the sources back both sides of it (uninstall.go:109-111 returns on a pre-delete hook error, :142-143 accumulates on a post-delete one). It is only the reporting surface that has diverged, and since the two land together, aligning at least the prefix and the name-the-step shape seems cheap.

Nit

qdrant's hook image is docker.io/clastix/kubectl:v1.32 — not digest-pinned and not routed through cozy-lib.image — while harbor's is both, in the same pair of files, with harbor's comment explaining why it matters for mirrored and air-gapped installs. Out of scope here, but it means the qdrant hook cannot start at all on such an install, and that fails silently in the sense this PR is about: an ImagePullBackOff reaches the operator as a Helm hook timeout, never as the hook's own message.

Overview

This fixes the actual defect described: both harbor and qdrant post-delete cleanup hooks previously swallowed every failure (|| echo "WARNING..." >&2, no set -e, unconditional final echo), so the container always exited 0 and hook-delete-policy: hook-succeeded collected the Job/pod that held the only evidence of the failure. I read the full rendered scripts (not just the diff hunks) for both charts at head and confirm the fix holds up:

• No set -e is introduced (correctly — it would abort the sweep at the first failure). Instead, failures=0 / fail() { failures=$((failures+1)); echo "ERROR: $*" >&2; } accumulates, and the exit code is decided once at the end (if [ "$failures" -ne 0 ]; then ... exit 1; fi). Every fail call happens in the main shell (never inside a pipe/subshell), so there's no variable-scoping loss.
• The listing-capture fix is portable POSIX and actually correct: if all_pvcs=$(kubectl get pvc ... ); then / if challenges=$(kubectl get challenge ...); then (packages/apps/qdrant/templates/hooks/cleanup-pvc.yaml, packages/apps/harbor/templates/hooks/cleanup.yaml) — the exit status of a bare assignment is that of the command substitution, so this correctly distinguishes "listing failed" from "listing returned nothing," which was exactly the qdrant kubectl get | grep ... || true and harbor for x in $(kubectl get ...) bug.
• --ignore-not-found is retained on every delete, so already-gone objects stay a success (idempotent re-run) — verified in the rendered templates, not just the diff.
• helm.sh/hook-delete-policy is deliberately left at before-hook-creation,hook-succeeded (no hook-failed) on the Job in both charts, so a failed Job's pod survives for postmortem. This is pinned by new helm-unittest assertions in packages/apps/harbor/tests/cleanup_hook_test.yaml and packages/apps/qdrant/tests/cleanup_pvc_test.yaml.
• hack/app-cleanup-hooks_test.bats (513 lines) extracts the real rendered script and runs it against a fake kubectl, covering success, idempotent re-run (empty listing and NotFound-on-delete shapes), delete-failure (with "sweep continues after a failure" assertions), and listing-failure for both hooks. This is the right strategy since the property under test (exit code) can't be established by matching manifest text.

A maintainer already did an independent, source-verified review (mutation-testing the bats suite, checking Helm/helm-controller behavior) and explicitly marked everything "Not blocking." I re-checked the current head against the diff and confirmed none of those points were incorporated — they're still open, still non-blocking, and not regressions against main.

Non-blocking / notes

1. Hook can still hang past Helm's timeout without producing the new error message. Neither kubectl get nor kubectl delete in either script sets --request-timeout, and no delete uses --wait=false; the Job has no activeDeadlineSeconds. A stuck finalizer (e.g. kubernetes.io/pvc-protection while a -system pod is still terminating) or a slow apiserver blocks the shell script indefinitely, so the operator sees Helm's own hook-timeout error instead of this PR's ERROR:/Cleanup FAILED message — the exact failure mode most likely to occur in production bypasses the new reporting path entirely. packages/apps/bucket/templates/hooks/cleanup-acme.yaml already guards the equivalent ACME sweep with activeDeadlineSeconds: 120, backoffLimit: 0, --request-timeout=30s, --wait=false.
2. Leftover accounting is incomplete. Verified directly in the rendered head templates: the ServiceAccount/Role/RoleBinding for both charts still carry only before-hook-creation,hook-succeeded, unchanged by this PR. On a failed cleanup they're collected as succeeded hooks while the Job/pods are retained — so a Role granting delete on PVCs/Certificates/Challenges/Ingresses (plus its binding) is also left in the tenant namespace, not just "the Job and its pods" as the in-code comments and release note say.
3. backoffLimit: 1 (packages/apps/harbor/templates/hooks/cleanup.yaml, packages/apps/qdrant/templates/hooks/cleanup-pvc.yaml) is unreconciled with bucket's backoffLimit: 0 for the same class of sweep — both carry a multi-line rationale comment arguing in a different direction.
4. Scope. packages/apps/bucket/templates/hooks/cleanup-acme.yaml has the identical swallow-every-failure shape this PR fixes in harbor/qdrant and is untouched; mariadb/clickhouse are named in the PR description as missing the same retry cap and left as-is. Reasonable to scope down, but worth stating explicitly as "out of scope, tracked" rather than leaving three of five hooks with the class of defect the title names generically.
5. Minor: CodeRabbit's nit on hack/app-cleanup-hooks_test.bats — the fake kubectl's EXISTING matcher only inspects the last positional in a multi-name xargs kubectl delete call — is still unaddressed. Harmless today (no current fixture mixes present/absent names in one call) but worth a one-line fix if the suite grows.

None of the above are regressions against main; they're gaps in an otherwise sound, well-tested fix, all previously surfaced by a maintainer's review and explicitly called non-blocking there.

Good to merge as-is; the notes above are worth a follow-up issue or a short round of fixes if the author wants to fold them in.

The fix is correct at the version this repo actually runs, the tests are load-bearing, and the question that decides whether this class of change is safe at all comes out the right way: a delete that can never succeed still completes.

That last point is the one I checked hardest, because this area has history. #3274 was closed as speculative and the real fix was #2826, and the settled principle for platform-wide hooks is that one tenant's broken state must not deadlock an operation for the whole fleet. So I traced what happens to an unsatisfiable delete. Helm appends the post-delete hook error to errs instead of returning, sets StatusUninstalled, purges, and returns uninstallation completed with N error(s). helm-controller keeps the finalizer on that failure, then the next reconcile gets ErrReleaseNotFound, returns success, and reconcileDelete removes the finalizer. kubectl delete qdrant X completes after one requeue, with nobody required to intervene for the delete itself. Same for all six harbor paths. No path leaves the app permanently undeletable.

On Andrei Kvapil (@kvaps)' leftover accounting, I read it differently and this is the substantive correction. In the helm this stack runs, a hook failure sweeps every hook that already succeeded: pkg/action/hooks.go:141 calls deleteHooksByPolicy(executingHooks[0:i], release.HookSucceeded, ...) and execHook invokes that closure on the error path too. The ServiceAccount at weight 0 and the Role/RoleBinding at weight 5 sort ahead of the Job at weight 10, so when the Job fails they are collected and only the failed Job and its pods remain, which is what the in-code comments and the release note say. The standing delete-on-PVCs Role does not outlive the uninstall. I would put a line in the comment saying this is helm v4 behaviour, since the older call graph had only the all-succeeded sweep and the accounting silently regresses if the stack ever pins back. IvanHunters' note 2 says both that the RBAC objects are collected and that the Role is left behind, and those cannot both hold.

Your point 1 on unbounded kubectl calls is right and I would sharpen the number. No call carries --request-timeout, no delete carries --timeout, neither Job carries activeDeadlineSeconds. But 30s is wrong by the repo's own arithmetic: hack/e2e-chainsaw/_lib/pod-label-census.sh:38-41 already records that against an unreachable apiserver a bounded call costs about five times the flag, since kubectl retries discovery five times, and that helper picks 15s for exactly that reason. I measured it, 2s gives 10s elapsed and 5s gives 25s. At 30s harbor's two listings alone are ~300s, which is the whole uninstall budget, since spec.uninstall is never set on app HelmReleases and the timeout falls through to the 5m default. Second refinement, --request-timeout bounds one HTTP request so it fixes the listings only, and a delete blocked on a finalizer is governed by --timeout, which the repo already distinguishes at talos-image-cache.sh:175. I would not take bucket's activeDeadlineSeconds here, you are right that it costs the log, and the log is the whole point of this PR.

The scenario that makes this the top item: harbor's <rel>-system child fails to tear down inside 5m, WaitForDelete gives up, the post-delete hook runs while a jobservice pod is still terminating, and kubectl delete pvc -l release=<rel>-system blocks on pvc-protection forever. The operator gets helm's deadline, never ERROR: failed to delete PVCs matching ..., and a Running Job pod stays in the tenant namespace after the release is gone, collected by nothing.

On backoffLimit: 1 versus bucket's 0, there are four dialects here and this PR picked the best one, so I would not reconcile downward. clickhouse carries before-hook-creation only, so it retains its Job after every uninstall including clean ones. bucket swallows all failures. mariadb exits non-zero with no cap. harbor and qdrant's before-hook-creation,hook-succeeded plus non-zero exit is the only combination that retains the Job exactly when there is something to read. Worth saying that in the comment so the next sweep converges here rather than on backoffLimit: 0. One nit inside it: on a deterministic failure, an absent ACME CRD say, the retry pays for nothing and you get two retained pods, and the twelve-line rationale reasons only about transient errors.

On CodeRabbit's discovery-probe suggestion, I would decline it. cozystack.cert-manager is emitted unconditionally in the system bundle and both paas and iaas fail without that bundle, so the window is cluster teardown only. And its premise is wrong, helm does not wait on a failed hook's pods. Adding a probe trades one silent-skip path for another, which is the thing this PR exists to stop.

Your scoping ask is fair. bucket still has the defect, cleanup-acme.yaml ends on an unconditional completion echo with every delete guarded by a warning, and mariadb and clickhouse already exit non-zero. Naming them and where they are tracked is cheap. Same for the unpinned kubectl image, which is the repo-wide norm rather than a qdrant oversight, six other packages use the same bare tag and harbor's digest is the exception, so that wants one sweep rather than a third state.

Mutations: each of the eight failure paths reverted individually reddens exactly the intended test, dropping --ignore-not-found reddens the already-gone cases, and both new manifest asserts fail on a changed backoffLimit, on removing the line entirely, and on adding hook-failed to the delete policy. Both packages define test: so neither is silently skipped, and the suite is 11 of 11 under both bats and hack/cozytest.sh.

What is not covered anywhere is the failure path end to end. Both chainsaw suites exercise a clean uninstall only, so that a failed Job survives collection, that the RBAC objects are swept anyway, and that helm-controller recovers on the second reconcile are established here by reading helm and helm-controller sources, not by a test. One dev-stand run settles all of it: install qdrant, break the hook's Role so the PVC delete 403s, delete the app, then watch the Job, the three RBAC objects, the HelmRelease conditions and how long the CR takes to disappear.