WorkerDeployment deletion finalizer infinite-loops on transport-level Temporal SDK errors (cached client never evicted)

[rupesh-parab-one-app]

Summary

When a WorkerDeployment is deleted, handleDeletion (internal/controller/worker_controller.go) reuses a cached Temporal SDK client from TemporalClientPool to drive cleanup. If the cached client is broken at the transport layer (e.g. every gRPC call returns context.DeadlineExceeded), every requeue of the deletion finalizer re-uses the same poisoned client and the cleanup loop never makes progress. The only way to recover is to restart the controller pod — which drops the in-memory pool.

The main Reconcile path already evicts on serviceerror.PermissionDenied / Unauthenticated (via isAccessDeniedErr at worker_controller.go:324), but handleDeletion has no eviction at all, and transport-layer errors don't surface as access-denied.

Environment

• Controller image: ghcr.io/temporalio/temporal-worker-controller:main-… (in sync with upstream main f9d01cd)
• Temporal: Temporal Cloud (API-key auth)
• Kubernetes: EKS 1.30
• CRD revision: post-v1.7 rename (WorkerDeployment / Connection)
• Helm-rendered Secret materialised by ExternalSecrets-operator

What happened (sanitised production timeline)

Single review env, one WorkerDeployment named example-nestjs-temporal-worker in namespace review-review-dev-ylmdqj:

Time (UTC)	Pattern	Count
11:08	Connection.temporal.io "temporal-cloud" not found	6
11:08	Reconciler error (generic event-emit follow-up)	18
11:08	Helm finishes; Secret + Connection + WD all Healthy
11:09–11:15	WorkerDeployment is deleted (chart re-render) → finalizer starts
11:15–11:54	failed to clean up Temporal server-side deployment data, will retry — underlying error: unable to describe worker deployment: context deadline exceeded	~240
11:54	Operator restarts temporal-worker-controller-manager pod
11:54	Finalizer succeeds on next reconcile (fresh client dial)

So the failure spans ~40 minutes and 240+ retries of handleDeletion with the exact same error, each one re-using the cached SDK client that was poisoned during the brief window when the Connection was being created but the API-key Secret was not yet readable.

Root cause (code refs)

internal/controller/worker_controller.go#L544-L600:

temporalClient, ok := r.TemporalClientPool.GetSDKClient(clientpool.ClientPoolKey{
    HostPort:   temporalConnection.Spec.HostPort,
    Namespace:  workerDeploy.Spec.WorkerOptions.TemporalNamespace,
    SecretName: secretName,
    AuthMode:   authMode,
})
// ... fallback dial path, no eviction on subsequent failures ...

resp, err := deploymentHandler.Describe(ctx, sdkclient.WorkerDeploymentDescribeOptions{})
if err != nil {
    var notFound *serviceerror.NotFound
    if errors.As(err, &notFound) {
        l.Info("Worker Deployment not found on Temporal server, nothing to clean up")
        return nil
    }
    return fmt.Errorf("unable to describe worker deployment: %w", err)
}

Compare to the main Reconcile path (worker_controller.go#L322-L340):

if err != nil {
    if isAccessDeniedErr(err) {
        r.TemporalClientPool.EvictClient(clientPoolKey)
    }
    // ... rate-limit handling, status update ...
}

isAccessDeniedErr only matches serviceerror.PermissionDenied and serviceerror.Unauthenticated (worker_controller.go#L923-L928), so a context.DeadlineExceeded against a broken client never triggers eviction even in the main reconcile path.

Repro (synthetic)

1. Pre-populate the TemporalClientPool with a stub SDK client whose WorkerDeploymentClient().GetHandle(name).Describe(...) returns context.DeadlineExceeded.
2. Create a WorkerDeployment + Connection referencing the same (HostPort, TemporalNamespace, SecretName, AuthMode).
3. Call handleDeletion directly.
4. Observe the error propagated and assert the cached client is still in the pool — i.e. the next reconcile will re-use the broken client and re-fail.

A passing unit test for the fix is included in the PR linked below; it inverts step 4 to assert the broken client is evicted and a healthy client is retained on NotFound.

Proposed fix (minimal)

Add a defer in handleDeletion (immediately after the SDK client is acquired) that evicts the cached client whenever handleDeletion returns a non-nil error. EvictClient is documented as safe-when-missing, so the defer is also harmless on the dial-but-don't-cache path. NotFound on Describe is intentionally treated as success and returns nil, so the defer skips eviction there and the healthy client stays in the pool.

PR:

Alternative considered (broader)

Track consecutive Temporal-server failures per ClientPoolKey in the main Reconcile path and evict after N failures (or wrap every temporal.GetWorkerDeploymentState / Describe call in an eviction-on-transport-error helper). Rejected for the initial PR because:

• It adds state and changes hot-path behaviour for every reconcile.
• The deletion-cleanup path is the one we observed wedged in production; the main reconcile has periodic requeues and the connectionFinalizerExists path already self-recovers on Connection.HostPort changes.
• The minimal fix in handleDeletion is enough to close the recovery gap; a broader change can land separately if maintainers prefer.

Happy to expand the PR scope if reviewers prefer the broader direction.

[carlydf]

Hi thanks for reporting this. I looked at your PR and came over here because I was curious what issue actually caused the Context Deadline Exceeded error. It seems to me that the "root cause" fix would be to not create a client that is "poisoned" and can only return context deadline exceeded. Such a client would need to be evicted in the normal Reconcile path too.

I want to confirm the reason for context deadline exceeded so ideally we can fix this at the root. Your fix PR mentions "stale secret" as a possible cause of context deadline exceeded, but the secret only contains auth info, so I'd expect a missing secret or secret with wrong credentials to throw an auth error, but I could totally be missing something.

Two questions:

1. In the logs before you deleted the WorkerDeployment, do you see any errors for DescribeWorkerDeployment, or were those calls succeeding?
2. Was this in a self-hosted cluster or cloud? If self-hosted, do you have access to the server logs during this time? Or if cloud, can we check the server logs of your namespace?

[rupesh-parab-one-app]

Thanks for pushing back on the "stale credentials" framing — you're right, and I owe a correction. I went back to the source logs from the affected namespace and the picture is sharper than the original write-up implied.

Controller binary: temporalio/temporal-worker-controller:v1.7.0, commit b1b7257f. Single manager pod throughout the wedge.

Answers to your two questions

Q1 — pre-deletion DescribeWorkerDeployment errors? No — those calls were never reached. Every pre-deletion reconcile bailed earlier, inside fetchAPIKeyFromSecret, with a 404 (the API-key Secret hadn't been materialized yet by our external secret operator). The very first Describe call ever attempted was inside handleDeletion, after the Secret became readable — and that's the call that hit context.DeadlineExceeded.

Q2 — self-hosted or cloud? Temporal Cloud. I'd rather not paste our tenant namespace in a public issue, but I'm happy to share it plus the exact wedge window (11:14:55Z - 11:54:32Z on 2026-05-21, 238 DescribeWorkerDeployment attempts at strict 10s cadence, all timing out) via a Temporal Cloud support ticket or DM if you want to pull the server-side logs.

Timeline (UTC, 2026-05-21)

sequenceDiagram
    autonumber
    participant ArgoCD
    participant WD as WorkerDeployment CR
    participant Ctrl as worker-controller (v1.7.0)
    participant Pool as ClientPool cache
    participant Sec as API-key Secret
    participant TC as Temporal Cloud

    Note over Ctrl,Sec: 11:08:55 - WD created
    ArgoCD->>WD: apply <WorkerDeployment>
    Ctrl->>WD: Reconcile (6 events in 1 s)
    Ctrl-->>Ctrl: error=Connection.temporal.io "<connection>" not found

    Note over Ctrl,Sec: 11:08:58 - 11:13:51 (~5 min, 7 sparse retries)
    Ctrl->>Sec: fetchAPIKeyFromSecret(...-shared-secrets)
    Sec-->>Ctrl: 404 NotFound
    Ctrl-->>Ctrl: error="failed to read API key secret ...: Secret ... not found"

    Note over Ctrl,Sec: 11:14:55 - WD deleted
    WD->>Ctrl: deletionTimestamp set
    Ctrl-->>Ctrl: msg="WorkerDeployment is being deleted, running cleanup"
    Ctrl->>Sec: fetchAPIKeyFromSecret (Secret now present)
    Sec-->>Ctrl: OK
    Ctrl->>Pool: DialAndUpsertClient (first successful dial)
    Pool-->>Ctrl: cached client
    Ctrl->>TC: DescribeWorkerDeployment (10s ctx)
    TC--xCtrl: context.DeadlineExceeded

    Note over Ctrl,Pool: 11:15:05 -> 11:54:25 (39 min, 238 retries every 10s)
    loop every 10s
        Ctrl->>Pool: GetSDKClient (cache hit, same broken client)
        Ctrl->>TC: DescribeWorkerDeployment
        TC--xCtrl: context.DeadlineExceeded
        Ctrl-->>Ctrl: msg="failed to clean up Temporal server-side deployment data, will retry"
    end

    Note over Ctrl: 11:54:32 - manager container terminated (exit 0, SIGTERM)
    Note over Ctrl: 11:54:35 - new pod starts on different node
    Note over Ctrl: 11:55:03 - Deployment Available=true

Loading

Exact log buckets

Window	Count	log message	error
11:08:55–11:08:56	6 err	unable to fetch Connection	Connection.temporal.io "<connection>" not found
11:08:56	6 err	Reconciler error	same as above (controller-runtime wrapper)
11:08:58–11:13:51	7 err (exp. backoff)	Reconciler error	unable to get worker deployment state: unable to describe worker deployment <namespace>/<WorkerDeployment>: failed to read API key secret "<secret>": Secret "<secret>" not found
11:14:55	1 info	WorkerDeployment is being deleted, running cleanup	(info)
11:15:05–11:54:25	238 err + 240 info, 10 s cadence	failed to clean up Temporal server-side deployment data, will retry	unable to describe worker deployment: context deadline exceeded
11:54:32	—	(container event_type:destroy, exit code 0)	—

Across the entire 39-minute cleanup loop, the error is verbatim unable to describe worker deployment: context deadline exceeded. No Unauthenticated, PermissionDenied, Unavailable, or any other gRPC code appears.

Where I was wrong

I framed this as "the cached client was built with stale credentials." That isn't supported by these logs. Pre-deletion, the API-key Secret was missing (404 from fetchAPIKeyFromSecret), not present-but-stale, so no successful SDK dial happened during the WorkerDeployment's normal lifecycle. You're correct that a stale-credential bug would surface as Unauthenticated/PermissionDenied, not as context.DeadlineExceeded.

What the evidence does support

• 6 s of "Connection CR not found" (sync-order race 1: WorkerDeployment applied before Connection).
• ~5 min of "Secret not found" — ExternalSecrets hadn't materialized the API-key Secret yet (sync-order race 2: WorkerDeployment/Connection applied before the Secret was synced from Vault).
• Sometime between 11:13:51 and 11:14:55 the user deleted the WorkerDeployment and the Secret became readable, so the deletion handler's first dial succeeded and the SDK client got cached.
• That client's very first RPC (DescribeWorkerDeployment, 10s deadline) timed out, and every retry against the same cached client for the next 39 minutes also timed out — until the pod was forcibly restarted at 11:54:32. There is no self-recovery in the logs; the wedge ends precisely when the manager container exits with code 0 (SIGTERM) and a new pod starts on a different node.
• Why the RPC hangs is not directly proven by these logs. Plausible candidates: Temporal Cloud throttling the source after a burst of cold-dial attempts, a lazy-gRPC connection that came up half-broken, or other transport-state issues. What is proven is that only a pod restart broke the wedge — which matches a poisoned in-memory cache entry.

On the broader-scope point

You're also right that the same eviction gap exists in the Reconcile path's Describe call (stacktrace points to worker_controller.go:235 in v1.7.0). The PR's narrow handleDeletion-only scope was justified by this incident being on the deletion path, but the code shape is identical in both places — GetSDKClient → Describe → on error, requeue without evicting. If the same hang happened in the Reconcile path, we'd get an equivalent 39-minute wedge that only a restart would clear.

And thanks again for slowing me down on the framing; the "stale credentials" story really wasn't supported by what's in the logs.

[jaypipes]

@rupesh-parab-one-app thank you (or thank Claude?) for this issue report. I believe the issue is that when a cached client is found to be bad, we are not always evicting it from the client pool. I can't think of a valid case for when we would not want to evict a bad client from the client pool, so I think the solution to this problem should be more general than just the handleDeletion() code paths. I'd support a PR that added some common code to the controller that ensured any client returned from the client pool that is bad is evicted. We could put that in the client pool code or make a little wrapper method on the controller/reconciler that would auto-evict clients that get connection reset/deadline exceeded errors when using them.

[Shivs11]

@rupesh-parab-one-app - Hi!

Thanks for the detailed timeline. One part I’m having trouble understanding is:

re-using the cached SDK client that was poisoned during the brief window when the Connection was being created but the API-key Secret was not yet readable

I don’t think that specific sequence can create/cache a stale SDK client.

• If the Connection is not found, reconcile returns before we build a ClientPoolKey, call GetSDKClient, parse auth, or dial Temporal.
• If the Connection exists but the referenced API-key Secret is not readable, ParseClientSecret returns before DialAndUpsertClient, so that should also return before a new client is cached.
• Assuming that you could have rotated your API key which could make you have a staled cached client: For API-key Secret rotation specifically, Read API key from K8s secret on every RPC call #301 changed the API-key credentials path to read the Kubernetes Secret on each outgoing RPC, rather than capturing the Secret value at client creation time. So an in-place Secret update should be picked up even when the SDK client object itself is cached.

Thus, I’m not yet seeing how the Connection/Secret materialization window could be the thing that poisoned the cached client! Is there something that I have missed here in my findings?

[rupesh-parab-one-app]

@Shivs11 you are right - my previous pre-deletion framing was wrong in an important detail. I went back through both the controller code path and the original log attributes, and the evidence points to a cached client existing earlier than I said.

The pre-deletion window actually splits into two different failure modes:

Window (UTC)	Count	Error shape	What it means
11:08:55-11:08:56	6	Connection.temporal.io "<connection>" not found	This is exactly the early return you described. The controller returns before building a ClientPoolKey, checking the pool, parsing auth, dialing, or calling Temporal.
11:08:56-11:13:51	11	unable to get worker deployment state: unable to describe worker deployment <namespace>/<WorkerDeployment>: failed to read API key secret "<secret>": Secret "<secret>" not found	This is not the ParseClientSecret early-return path. This error string is emitted from the per-RPC API-key credentials callback, so the SDK call path had already been reached with a cached client.

The key code points:

• Connection lookup happens before any pool/auth/dial work in Reconcile:
  worker_controller.go
• ParseClientSecret does a Kubernetes Get before dialing, so an initial missing Secret returns before DialAndUpsertClient:
  clientpool.go
• DialAndUpsertClient only inserts into the map after sdkclient.Dial succeeds:
  clientpool.go
• The Phase 2 string failed to read API key secret is emitted only by fetchAPIKeyFromSecret, which is the callback installed by NewAPIKeyDynamicCredentials:
  clientpool.go,
  clientpool.go

So my corrected interpretation is:

1. During the Connection-not-found phase, no client could have been cached from those reconciles.
2. By the later failed to read API key secret phase, a client was already cached. I cannot prove the exact cache-insertion timestamp from the controller logs I have, but the Phase 2 error can only happen from the dynamic credentials callback invoked during the SDK call path.
3. Because PR Read API key from K8s secret on every RPC call #301 made API-key auth read the Secret on each outgoing RPC, the Secret-not-found error can happen at RPC credentials time on an already-cached client. That explains the log shape and also means PR Read API key from K8s secret on every RPC call #301 is doing the right thing for rotation; this incident is about recovery when a cached client later behaves badly.
4. Those Phase 2 Describe attempts likely failed client-side before reaching Temporal Cloud, because the credentials callback could not supply a credential. I do not have server-side logs or gRPC clientconn state to prove the exact transport state.
5. Once the Secret became readable and deletion cleanup ran, the same cached SDK client was reused. The first credentialed Describe in cleanup returned context.DeadlineExceeded, and the controller then reused that same cached client for 238 finalizer retries until the pod restart dropped the in-memory pool.

In timeline form:

sequenceDiagram
    participant Ctrl as Controller
    participant SDK as TemporalSDK
    participant Pool as ClientPool
    participant Sec as APIKeySecret
    participant Conn as CachedClientConn
    participant TC as TemporalCloud

    Note over Ctrl,Sec: Earlier than the visible error window
    Ctrl->>Sec: ParseClientSecret Get
    Sec-->>Ctrl: Secret readable
    Ctrl->>Conn: sdkclient.Dial succeeds
    Ctrl->>Pool: cache SDK client

    Note over Ctrl,Sec: Phase 1 11:08:55 to 11:08:56
    Ctrl-->>Ctrl: Connection not found, returns before pool or dial

    Note over Ctrl,Sec: Phase 2 11:08:56 to 11:13:51
    Ctrl->>Pool: GetSDKClient cache hit
    Ctrl->>SDK: DescribeWorkerDeployment
    SDK->>Sec: fetchAPIKeyFromSecret
    Sec--xSDK: Secret not found
    Ctrl-->>Ctrl: Describe fails before a credentialed RPC reaches Temporal

    Note over Ctrl,Sec: Secret later becomes readable
    Ctrl->>Pool: GetSDKClient same cache hit during deletion cleanup
    Ctrl->>SDK: DescribeWorkerDeployment
    SDK->>Sec: fetchAPIKeyFromSecret
    Sec-->>SDK: Secret readable
    SDK->>TC: credentialed Describe RPC
    TC--xSDK: no response before deadline
    SDK--xCtrl: context deadline exceeded

    loop 238 retries
        Ctrl->>Pool: reuse same cached SDK client
        Ctrl->>SDK: DescribeWorkerDeployment
        SDK->>TC: credentialed Describe RPC
        TC--xSDK: no response before deadline
        SDK--xCtrl: context deadline exceeded
    end

    Note over Ctrl,Pool: Pod restart drops in-memory pool and next reconcile recovers

Loading

I want to be careful not to overclaim the Secret lifecycle. One plausible explanation is that the Secret object was readable at the earlier dial, then temporarily unreadable/not found to the per-RPC callback, then readable again by deletion cleanup. That could be ExternalSecrets create/update behavior, informer/cache timing, or delete/recreate behavior. I do not have Kubernetes audit evidence proving which one happened. The part I do think the logs prove is narrower: the controller had a cached SDK client by Phase 2, reused it through cleanup, and did not recover until the pool was cleared by restart.

@jaypipes I agree with the broader-scope direction. The current #329 diff is too narrow if it only fixes handleDeletion, because the main Reconcile path has the same shape: get cached client, call Describe, return/requeue on context.DeadlineExceeded without evicting. That leaves the same non-recovery class possible outside deletion.

I will expand #329 rather than try to land the deletion-only version.

The concrete shape I plan to push is:

func shouldEvictClient(err error) bool {
	if isAccessDeniedErr(err) {
		return true
	}
	if errors.Is(err, context.DeadlineExceeded) {
		return true
	}
	var unavailable *serviceerror.Unavailable
	return errors.As(err, &unavailable)
}

Then use shouldEvictClient(err) anywhere we currently evict only for isAccessDeniedErr(err), and use the same predicate in the deletion cleanup path. That keeps the policy in one place, preserves the existing auth-error behavior, and adds bounded recovery for transport/connectivity failures where redialing is the useful next step.

I am intentionally not including context.Canceled or broader server/application errors in the first pass:

• context.Canceled can be normal reconcile or shutdown cancellation.
• ResourceExhausted may represent throttling where client churn does not help.
• NotFound is a valid domain response in deletion cleanup and should not evict.

If you would prefer this policy to live behind a pool-level wrapper instead of a reconciler-level predicate, I am happy to shape it that way. The reason I am starting at the call sites is just that ClientPool currently hands out clients and is not itself on the RPC path, while the controller already has explicit eviction call sites for auth failures.

I will update #329 with the broader Reconcile + deletion behavior and add a Reconcile-path regression test alongside the deletion-path test.