Do not report host deletion as completed on an unknown API error - #2057

Reviewed this in depth. The diagnosis is right, the alternative you rejected is genuinely the wrong one, and the scoping is honest — I verified all four of your load-bearing claims against the code and they hold. One concrete change requested before merge.

Verified

• IsNotFound is safe here. I traced the full chain — kube.STS().Get → commonKube.GetWithRetry (common/kube/retry.go:123-146) → client-go. No fmt.Errorf/%w wrapping anywhere, so the raw *StatusError reaches your check and the common case is byte-for-byte unchanged. Worth adding to the PR description, since a wrapper here would have silently broken the NotFound path.
• PVCs really are unowned — model/common/creator/pvc.go:50,73, both commented out under the PV-retain-policy rationale. No PersistentVolumeClaimRetentionPolicy anywhere either, so STS deletion doesn't reclaim them.
• Your rejected alternative would be worse than the leak. HostCanDeletePVC (model/common/volume/deleter.go:41) seeds policy := PVCReclaimPolicyDelete and only overrides inside WalkVolumeMounts(api.CurStatefulSet, …), which returns immediately when !HasCurStatefulSet() (type_host.go:467-470). A failed Get leaves that nil — so "proceed anyway" deletes Retain PVCs. Data loss, not just a leak. Good call.
• 8 apiErrors.IsNotFound uses in this package, and worker-pdb.go:41 is indeed the canonical shape.

One thing that strengthens your case and isn't in the description: IsTransientAPIError deliberately excludes IsForbidden (retry.go:66), and the Get is already wrapped in GetWithRetry. So transients are absorbed before line 541 — the residual class reaching your new branch is terminal, which is exactly the class where "already deleted" was a lie.

Requested change — WithError introduces an uncancellable ~50s stall

WithError → writeStatus → _writeErrors returns true → StatusUpdate(context.Background(), …) (common/announcer/announcer.go:274) → statusUpdateRetry (chi/kube/cr.go:126-146): maxRetryAttempts = 50 with time.Sleep(1 * time.Second) between attempts.

Two things make this bite:

1. The ctx is context.Background(), so the IsContextDone guards at cr.go:116 and :150 can never fire. Operator shutdown cannot interrupt it.
2. Default config is status.fields.action: false but error: true (config/config.yaml). So the old WithAction path wrote no status at all — _writeActions short-circuits — while the new WithError path does. This stall is introduced by the change, not pre-existing.

Failure scenario: the apiserver is degraded, so the STS Get fails and the status write fails for the same reason. Each failing host now burns ~49s inside deleteCRProtocol's wg.Wait() (worker-deleter.go:321), where it previously returned instantly. That is precisely the failure mode this PR targets, so the change makes deletion slower and less responsive in the one scenario it exists for.

Suggested fix: keep WithEvent(...) and the Error(...) log line, drop WithError(...). The Event is the part that survives anyway — the CR is deleted seconds later by uninstallFinalizer, so the status write is lost regardless.

Nits

• worker-deleter.go:553 — "Report the failure and let the caller decide." No caller decides: the sole caller is _ = w.deleteHost(ctx, chi, h) at :640, inside a goroutine, and deleteCHI uninstalls the finalizer unconditionally. Suggest "report the failure; deletion of this host is skipped", plus a // TODO(#2056) at the discard site so the dead return value is self-documenting.
• Worth noting in the comment that the Get is already retried, so a reader knows the residual class is terminal rather than transient.
• Minor: host.Runtime.CurStatefulSet is assigned from the failed Get. client-go returns a non-nil empty struct on error, so it is left non-nil-but-empty. Harmless today since both branches return immediately, but sts, err := ...; if err != nil { … }; host.Runtime.CurStatefulSet = sts avoids the trap.

Test

There is no coverage of worker-deleter.go at all today, and this one is cheap to pin: Controller.kube is the interfaces.IKube interface, IKubeSTS.Get is on it, and the error branch returns before deleteTables/c.deleteHost, so nothing else needs faking. common/statefulset/statefulset-reconciler_test.go:48 has the fakeSTS pattern to copy, and announcer.NewAnnouncer(nil, nil) is inert because the write paths are gated on capable().

A table test over NotFound / Forbidden / InternalError / TooManyRequests / opaque, asserting deleteHost returns nil for the first and non-nil for the rest, fails on the current code and passes after. Making the fake's Delete panic also pins the real safety property: no k8s mutation when the Get could not be classified.

Given this is data-loss-adjacent, I'd like the test before merge.

Verdict

The shape is right and the deferral to #2056 is legitimate scoping, not scope-dodging — the finalizer question is a real policy decision that deserves its own review. Fix the WithError stall and add the unit test and I'm happy to see this land.