fix: recognize an already-purged Helm release as finalizer success - #28
Conversation
helmReleaseFinalizer's errors.Is(err, driver.ErrReleaseNotFound) check
only matches when Uninstall.Run fails immediately on the initial
lookup. When the release disappears partway through instead -
resources already deleted, then the purge step finds no release
record left - Helm wraps the sentinel in a new, flattened
errors.Errorf("uninstallation completed with N error(s): ...")
(helm.sh/helm/v3/pkg/action/uninstall.go:163) that breaks errors.Is.
That specific failure mode retried the finalizer forever instead of
recognizing there was nothing left to clean up, permanently blocking
the ConditionalTTL's deletion and any finalizers still pending after
it.
Root-caused this from a production log on trx: sfj-2f40163--tupan
stuck on exactly this error while the tupan account accumulated 94
active Knative Revisions.
Fix: check release existence upfront via action.Get before calling
Uninstall, plus a text-match fallback on Uninstall's own error for the
same race in the narrow window between the two calls.
Also fixes two related NotFound gaps found while in this code path:
handleUpdateErr now swallows NotFound (the object was already deleted
by something else) instead of surfacing it as a reconcile error, and
the final r.Delete(ctx, cTTL) on the happy path now ignores NotFound
the same way the initial r.Get already did.
🛡️ SDD Check — action requiredI couldn't detect an SDD in this PR. Please check one option below (requires write access to the repo):
|
|
AdrianWR
left a comment
AdrianWR
left a comment
There was a problem hiding this comment.
Merge Readiness and Risk Assessment
Status: Ready to merge pending review approval.
Code quality: High. The solution is defensive (belt-and-suspenders approach), well-documented, and handles the exact production failure mode.
CI: All 4 checks passing (SonarCloud, Arnica Code Risks, SDD Check, ECR publish).
Test coverage: Solid unit tests with realistic fixtures; envtest suite deferred to CI (author notes no KUBEBUILDER_ASSETS in local environment).
Risk level: Low. Changes are localized to the Helm release finalizer and final deletion path. The upfront Get() is defensive and doesn't change behavior for the happy path. The text-match fallback is conservative—only triggers when errors.Is fails, which only happens with Helm's specific flattening behavior.
Production impact: Fixes a stuck finalizer in production (sfj-2f40163--tupan on trx), allowing 94+ blocked Knative Revisions to be cleaned up.
2 participants
Summary
helmReleaseFinalizer'serrors.Is(err, driver.ErrReleaseNotFound)check only matches whenUninstall.Runfails on the initial lookup. When the release disappears partway through instead, Helm wraps the sentinel in a flattenederrors.Errorf(...)(uninstall.go:163) thaterrors.Iscan't see through — that failure mode retried the finalizer forever instead of treating "nothing left to clean up" as success.action.Get, plus a text-match fallback (isReleaseNotFoundErr) onUninstall's own error for the same race in the narrow window between the two calls.NotFoundgaps found while in this code path:handleUpdateErrnow swallowsNotFound(object already deleted elsewhere) instead of surfacing it as a reconcile error, and the finalr.Delete(ctx, cTTL)on the happy path now ignoresNotFoundlike the initialr.Getalready did.Why
Root-caused from a production log on
trx:sfj-2f40163--tupanwas stuck on exactly this error, permanently blocking thatConditionalTTL's deletion (and any finalizers queued after the release one) while thetupanaccount accumulated 94 active Knative Revisions.Test plan
go build ./...,go vet ./...go test ./controllers/...— new tests use Helm's in-memory storage driver +kubefake.PrintingKubeClient(no real cluster/Helm release touched, same fixture pattern as Helm's own unit tests):TestIsReleaseNotFoundErr— sentinel, Helm's flattened wrapping, and an unrelated errorTestHelmReleaseFinalizer_AlreadyGone— reproduces the tupan/trx scenario directly: a release that was never created must not block the finalizerTestHelmReleaseFinalizer_NoopWhenHelmSpecNil/_NoopWhenDeleteFalse— existing early-return paths, unchangedTestHandleUpdateErrupdated for the new NotFound-swallowing behavior, plus a new case confirming a genuinely unrelated error still propagatesTestAPIs) not run here — noKUBEBUILDER_ASSETSin this environment; CI should cover ittrxthatsfj-2f40163--tupan(and any othersfj-*--tupanobjects stuck the same way) finish deleting after this ships