fix: evict cached Temporal client on bad-client failures - #329
fix: evict cached Temporal client on bad-client failures#329rupesh-parab-one-app wants to merge 2 commits into
Conversation
|
|
9b56a0d to
8bc7c67
Compare
rupesh-parab-one-app
changed the title
|
@rupesh-parab-one-app I'm super embarrassed at the long delay in getting back to this important PR. Apologies. If you wouldn't mind rebasing to main to address the merge conflicts, that would be great. I'll do a review on the revision ASAP. |
Reusing a cached SDK client after access or transport failures can keep the controller wedged on the same unhealthy client until the manager pod restarts and drops the in-memory pool. Centralize the eviction decision in shouldEvictClient and use it from both the main Reconcile path and WorkerDeployment deletion cleanup. The predicate keeps the existing PermissionDenied/Unauthenticated behavior and adds transport cases that benefit from redialing: context.DeadlineExceeded and serviceerror.Unavailable. It intentionally leaves ResourceExhausted, context.Canceled, and domain responses such as NotFound alone so ordinary server-side or lifecycle responses do not churn otherwise healthy clients. Add regressions for Reconcile and deletion cleanup so a cached client returning context.DeadlineExceeded is evicted before the next reconcile retries. Co-authored-by: Cursor <cursoragent@cursor.com>
8bc7c67 to
dfb8ce2
Compare
|
@rupesh-parab-one-app thanks for pushing a new rev! FYI, if you'd like to chat about this PR (and any other one!), feel free to find us on the Temporal community Slack #temporal-workers channel. :) |
jaypipes
left a comment
jaypipes
left a comment
There was a problem hiding this comment.
👍 good stuff, thank you @rupesh-parab-one-app :)
| var unavailable *serviceerror.Unavailable | ||
| return errors.As(err, &unavailable) |
There was a problem hiding this comment.
why evict on Unavailable? The gRPC definition of this status describes it as
most likely a transient condition, which can be corrected by retrying with a backoff
so I don't think it should be included here
There was a problem hiding this comment.
As discussed in the other comment, if we evict and it still errors the next time (because the problem was not the client), that is ultimately ok. So.. both of these comments are non-blocking
There was a problem hiding this comment.
You're right, and I don't have the evidence to defend it. I'll drop Unavailable unless you'd rather keep it once you've seen the other thread.
Two things I got wrong when I added it. First, I assumed the SDK would have retried a genuinely transient one before we saw it. That is true wherever we go through a deployment handle — all nine handle methods wrap the call in newGRPCContext(ctx, defaultGrpcRetryParameters(ctx)) (go.temporal.io/sdk@v1.41.1/internal/internal_worker_deployment_client.go, e.g. Describe at :181, SetCurrentVersion at :258) — so it covers handleDeletion and all of executePlan's writes. But GetWorkerDeploymentState calls WorkflowService().DescribeWorkerDeployment(ctx, ...) directly (internal/temporal/worker_deployment.go:82) with the plain reconcile context and no retry.ConfigKey, so NewRetryOptionsInterceptor takes its else branch and calls grpc_retry.Disable() (go.temporal.io/sdk@v1.41.1/internal/common/retry/interceptor.go:140-143). No retries at all there. So at that one call site a single transient blip evicts a healthy client, which is your point exactly.
Second, errors.As(err, &unavailable) misses *serviceerror.NamespaceUnavailable, which convert.go:80-86 returns when the status carries a NamespaceUnavailableFailure detail. So the branch does not even cover the Unavailable family cleanly.
Worth reading alongside the other thread: the deadline branch below it does not match a Temporal-origin deadline at all, so today Unavailable is the only transport-failure branch matching anything Temporal-origin. If we drop both, the predicate falls back to the auth behaviour that already exists on main, and this PR stops doing anything. That is a fine outcome if it's the right one, but I'd rather say it out loud than have it happen by accident.
| } | ||
| if errors.Is(err, context.DeadlineExceeded) { | ||
| return true | ||
| } |
There was a problem hiding this comment.
similarly, I understand that the issue you reported manifested as context.DeadlineExceeded, so including context.DeadlineExceeded as an eviction condition is the main point of this PR.
But I do have misgivings about labeling a context.DeadlineExceeded as a signifier of a "poisoned" connection. Context deadline exceeded could happen due to a very large range of things going on in the server that are unrelated to the client. And, if the SDK is doing something pathologically wrong with a "poisoned" client config, perhaps it would be more appropriate to ask the SDK to return a different error type in that case.
In #328, you call out
ResourceExhaustedmay represent throttling where client churn does not help.
as a reason for excluding ResourceExhausted, but the same exact reasoning thing can be said about context.DeadlineExceeded.
Can we at least put a comment here explaining honestly why we are including context.DeadlineExceeded error type in the "evictable" errors? It's ok with me if it says something like "saw this error repeatedly until pod restarted and client cache was cleared. not totally sure how the client became unhealthy, but prefer to err on the side of evicting in the case of this error instead of repeating it indefinitely."
I think we are protected from a perpetual and fast evict -> reload client -> context deadline error -> evict loop by the exponential backoff for erroring Reconciles (it could still be perpetual if recreating the client doesnt solve the problem, but at least if would slow over time). That is why I personally am ok with erring on the side of eviction. But I want this PR to be honest about what we do and don't know about why this error happened in the first place.
There was a problem hiding this comment.
I went looking for the explanation you asked for, this check does not match a Temporal deadline.
The SDK's errorInterceptor is its outermost unary interceptor and rewrites call errors through serviceerror.FromStatus(status.Convert(err)) (go.temporal.io/sdk@v1.41.1/internal/grpc_dialer.go:134, body at :201-208; it passes GrpcMessageTooLargeError through unchanged). codes.DeadlineExceeded becomes *serviceerror.DeadlineExceeded (go.temporal.io/api@v1.62.8/serviceerror/convert.go:77), which implements only Error() and Status() — no Unwrap, no Is. So errors.Is(err, context.DeadlineExceeded) cannot reach the sentinel. The message text survives via st.Message(), which is why the logs read context deadline exceeded and why this looked correct to me. My own tests pass only because they inject the raw sentinel through a stub and never touch gRPC.
This also answers something I had wrong in #328. The deletion path does install the SDK retry config, so I assumed a deadline there had already been retried. It hadn't: codes.DeadlineExceeded is deliberately absent from the SDK's retryable set, with the comment "they are coming from go context and 'context errors are not retriable based on user settings' by gRPC library" (go.temporal.io/sdk@v1.41.1/internal/common/retry/interceptor.go:103-108). So the SDK forwards a deadline unretried, and then the controller's check doesn't match it. Nothing in the stack reacted.
Reproduction on a branch rather than more code here, tests only, based on this PR's head: https://github.com/rupesh-parab-one-app/temporal-worker-controller/blob/5e80872729d8983673771e9808dd79b1993ab422/REPRO.md
It shows it twice: constructing the error the way the interceptor does, where the predicate returns false, and driving a real SDK client over an in-memory bufconn transport with no Temporal server. A real call whose deadline fires returns *serviceerror.DeadlineExceeded with the message context deadline exceeded; an unreachable endpoint returns *serviceerror.Unavailable. Happy to inline the relevant few lines here if you'd rather not follow a fork link.
On your ResourceExhausted parallel: your policy argument stands untouched, and I should be clear I'm not answering it. I'm saying something narrower — that today the question is moot, because the branch is unreachable for the case it was written for. The one deadline it can match is a non-Temporal one, such as a Kubernetes write in executePlan running out the 5-minute reconcile budget (worker_controller.go:113), and evicting the Temporal client there achieves nothing.
One aside on backoff, since you raised it: it isn't present on the cleanup path. Reconcile swallows the handleDeletion error and returns ctrl.Result{RequeueAfter: 10 * time.Second}, nil (worker_controller.go:157-159), so controller-runtime calls Forget and requeues at a flat 10s rather than rate-limiting — which is the retry cadence I described in #328. The main paths do return real errors, so backoff applies there. I'd rather not change those lines in this PR; I'll file it separately.
Where that leaves the comment you asked for: I don't think it's the right change on its own. My suggestion, if you agree, is to match the concrete type in addition to the sentinel — errors.As(err, &*serviceerror.DeadlineExceeded) alongside the existing errors.Is, since swapping one for the other would drop the Kubernetes case — drop Unavailable per your other comment, and then write the honest comment against what's left. The alternative is to drop the deadline case as unproven, but combined with dropping Unavailable that reduces the predicate to the auth behaviour already on main. Happy either way; tell me which and I'll push it.
5 participants
What
Cached Temporal SDK clients are now evicted consistently when the controller observes failures that indicate the cached client may no longer be usable.
This expands the original deletion-cleanup-only fix to cover both places that reuse a cached client:
Reconcilepath afterGetWorkerDeploymentState/DescribeWorkerDeploymenthandleDeletionwhile cleaning up Temporal server-side Worker Deployment dataThe shared
shouldEvictClient(err)predicate keeps the existing auth behavior and adds bounded recovery for transport/connectivity failures:serviceerror.PermissionDeniedUnauthenticatedcontext.DeadlineExceededserviceerror.UnavailableIt intentionally does not evict on broader server/application responses such as
ResourceExhausted,context.Canceled, orNotFound.Why
In #328 we observed the controller repeatedly reusing the same cached SDK client after
DescribeWorkerDeploymentreturnedcontext.DeadlineExceeded. The controller did not recover until the manager pod restarted and dropped the in-memory pool.The earlier narrow version of this PR only evicted in
handleDeletion, but reviewers correctly pointed out that the mainReconcilepath has the same shape: get a cached client, callDescribe, then requeue on transport failure without evicting. This PR now closes that recovery gap in both paths.Changes
internal/controller/worker_controller.goshouldEvictClient(err)next toisAccessDeniedErrisAccessDeniedErreviction checks withshouldEvictClienthandleDeletionto use the same predicate instead of evicting on every non-nil returninternal/controller/reconciler_events_test.goTestShouldEvictClientto lock down included/excluded error classesTestReconcile_EvictsCachedClientOnTransportFailureClose()onstubTemporalClientsoEvictClientcan close test clients safelyTest plan
go test ./internal/controller -run 'Test(ShouldEvictClient|Reconcile_EvictsCachedClientOnTransportFailure|HandleDeletion_EvictsCachedClientOnTemporalFailure|Reconcile_DescribeWorkerDeploymentNotFound)' -count=1go test ./internal/controller -count=1Closes #328.