Implementation proposal: TemporalNamespace deletion stuck on finalizer patch

[Azerothian]

Source

Upstream issue: alexandrevilain#943 — "Cannot delete namespaces"

When trying to delete a TemporalNamespace I often encounter issues where the operator gets stuck and fails to reconcile Temporal namespaces … ERROR Reconciler error … unable to patch object: TemporalNamespace.temporal.io "debugging" is invalid: metadata.finalizers: Forbidden: no new finalizers can be added if the object is being deleted, found new finalizers []string{"foregroundDeletion"} … I usually have to manually remove the finalizer from the TemporalNamespace object and also restart the operator itself as it gets stuck.

Why it fits this operator

Deleting a TemporalNamespace is a first-class operator lifecycle operation. A namespace that cannot be deleted (operator stuck in a loop, forced to hand-remove Kubernetes finalizers and restart the controller) leaves garbage workflows behind and blocks cluster teardown. This is core reconciler/finalizer logic — the operator's responsibility.

Proposed implementation

1. Finalizer guard — in the TemporalNamespace finalizer/deletion path, never re-add a finalizer once Kubernetes has marked the object as being deleted (foregroundDeletion present → abort cleanly instead of re-patching).
2. Idempotent deletion — when the namespace is already absent on the Temporal server (try to delete but not found), treat "not found" as success and finish removing CRD-side resources instead of erroring/looping.
3. Reconcile self-heal — on a failed delete reconcile, surface a clear reconcile-blocked state instead of repeatedly re-patching the same invalid object (the cause of the stuck loop).

Affected areas

• TemporalNamespace controller / reconciler deletion path
• Finalizer logic (controllers/finalizer)
• Error handling for the namespace-delete RPC (not found)

Risks / migration concerns

• Deletion is destructive — keep the delete-vs-patch distinction; never silently skip when the namespace exists on the server.
• Needs e2e coverage (create → delete → re-create) so the fix doesn't reintroduce stuck/finalizer states.
• Tracks the still-open upstream defect.

[Azerothian]

Implementation proposal (code-verified)

Verified against main @ 424555b. Items 1–3 of the proposal have already landed on this fork (PR #6, commits 1ccdd6a and 9deb4b9). What is left is a different, still-real reason a TemporalNamespace gets stuck on its finalizer — and it is the one that matches the "operator gets stuck and I have to remove the finalizer by hand" half of the upstream report.

What is already fixed

Item 1 — never re-add a finalizer on a deleting object. ensureFinalizer (controllers/temporalnamespace_controller.go:197-215) returns early when DeletionTimestamp is non-zero.

Item 3 — stop the deferred whole-object patch from clobbering foregroundDeletion. This was the literal cause of the error in the report. patch.Helper.Patch issues client.Patch(ctx, obj, client.MergeFrom(before)) — a JSON merge patch, and metadata.finalizers is a list, so it is replaced wholesale. Once the reconcile mutated the finalizers on the same object the helper snapshotted, the deferred patch re-sent a stale list and the API server answered exactly:

metadata.finalizers: Forbidden: no new finalizers can be added if the object is
being deleted, found new finalizers []string{"foregroundDeletion"}

controllers/finalizer.go:49-57 now keeps finalizers out of that patch entirely — patchFinalizers deep-copies, mutates the copy, and writes it with client.MergeFromWithOptimisticLock{}. Because the reconcile's own namespace object is never touched, reflect.DeepEqual(before, after) holds for the metadata half and the helper issues no object patch at all. controllers/finalizer_test.go:139-158 pins the old failure as a regression test against envtest.

Item 2 — idempotent deletion. ensureNamespaceDeleted:232-239 already treats *serviceerror.NamespaceNotFound as success and goes on to remove the finalizer.

So this issue should be re-scoped to what is still broken.

What is still broken: the readiness gate sits in front of the deletion path

controllers/temporalnamespace_controller.go:114-129:

if !cluster.IsReady() {
    logger.Info("Skipping namespace reconciliation until referenced cluster is ready")
    return reconcile.Result{RequeueAfter: 10 * time.Second}, nil     // <- line 117
}

// Check if the resource has been marked for deletion
if !namespace.ObjectMeta.DeletionTimestamp.IsZero() {               // <- line 121
    err := r.ensureNamespaceDeleted(ctx, namespace, cluster, target)

A TemporalNamespace with allowDeletion: true whose cluster exists but is not ready never reaches line 121. It requeues every 10s, forever, holding deletionFinalizer. This is the ordinary teardown order — delete the namespaces and the cluster together, or delete a namespace while the cluster is degraded — and the only way out is hand-editing the finalizer, exactly as reported.

Note the cluster not-found case is handled correctly at :91-104 (drops the finalizer). It is only the exists-but-unhealthy case that hangs.

The same ordering bug is in controllers/temporalschedule_controller.go: !namespace.IsReady() at :109 and !cluster.IsReady() at :133 both precede the deletion check at :157. controllers/temporalclusterclient_controller.go gets it right — deletion at :83, readiness at :107.

Second gap: no escape hatch when the frontend is unreachable

Even with the ordering fixed, ensureNamespaceDeleted has three ways to block indefinitely:

• temporal.GetClusterClient fails (:225-228) — frontend gone, certs gone.
• DeleteNamespace returns anything other than NamespaceNotFound (:236-238) — including permanent failures like Unimplemented/PermissionDenied, which no retry clears.
• removeFinalizer conflicts (:241) — recoverable, retried next cycle.

All of these route through handleError → handleErrorWithRequeue (:255-262), which returns the error so controller-runtime backs off. The backoff caps around 16m40s, so it is not a hot loop — but the namespace is permanently undeletable and the user is back to editing finalizers.

Refined implementation

1. Move the deletion check above the readiness gate.

In Reconcile, hoist the if !namespace.ObjectMeta.DeletionTimestamp.IsZero() block (:120-129) to sit immediately after target, err := r.Resolver.For(...) (:109-112) and before if !cluster.IsReady() (:114). ensureNamespaceDeleted needs cluster and target, both of which are already resolved by that point; it does not need readiness. Apply the same move in temporalschedule_controller.go (deletion block :157-166 above the two gates at :109 and :133).

Gotcha: target resolution (:109) must stay ahead of the deletion block, since ensureNamespaceDeleted takes it. If Resolver.For itself fails the namespace is still stuck — see (3).

2. Classify permanent RPC failures as terminal.

In ensureNamespaceDeleted:231-239, extend the errors.As chain past NamespaceNotFound. *serviceerror.PermissionDenied and *serviceerror.InvalidArgument from DeleteNamespace cannot be retried away; route them through the existing handleTerminalError (:271-274) so they surface as SpecValidationFailedReason without eternal backoff. This is the pattern acd9f5b already established for search attributes.

3. Add an explicit escape hatch.

An annotation checked at the top of ensureNamespaceDeleted, before the client is built:

// temporal.io/force-delete: "true" drops the finalizer without contacting the server.
// The temporal-side namespace is left behind; it is the operator of last resort for a
// cluster that is already gone.
if namespace.Annotations[forceDeleteAnnotation] == "true" {
    logger.Info("Force-deleting namespace, skipping temporal-side deletion", "namespace", namespace.GetName())
    return removeFinalizer(ctx, r.Client, namespace, deletionFinalizer)
}

Explicit and per-object, so it keeps the delete-vs-patch distinction the issue asks for: nothing is silently skipped, and a user who has to reach for it has said so on the object. Prefer this to a failure-count timeout — a timeout would silently abandon a namespace during a transient frontend outage.

Tests

• controllers/finalizer_test.go already has the envtest harness and createTestCluster helper. Add: cluster present but not ready + namespace deleting → reconcile removes the finalizer and the object goes away. That test fails today.
• Same shape for the schedule controller.
• Force-delete annotation: object deleting, no reachable frontend, annotation set → finalizer dropped, no client dialled.
• e2e tests/e2e/namespace_test.go: create → delete → re-create, plus delete-namespace-while-cluster-unhealthy.

Risk

Moving the deletion check ahead of the readiness gate means deletion now runs against a cluster the operator has not confirmed healthy. That is the intent — ensureNamespaceDeleted already handles an unreachable frontend by returning an error and requeueing, so a genuinely transient outage still retries rather than skipping the server-side delete.