fix(controller): prune Temporal server-side version record on normal drain

[jaypipes]

Port of atlanhq#25.

When a Worker Deployment Version drains during a normal rollout, the controller
deletes its Kubernetes Deployment but never calls DeleteVersion on the Temporal
server. The only existing DeleteVersion call site is the CRD-deletion finalizer,
which runs only when the whole TemporalWorkerDeployment is deleted. Version
records therefore accumulate one per deploy until the per-deployment cap
(matching.maxVersionsInDeployment) is hit, after which every rollout fails to
register a new build ID and surfaces fleet-wide as KedaScaledObjectErrors (#377).

• planner.go: getDeleteDeployments' Drained case now also requires
  EligibleForDeletion. This is the only point that can prune the server-side
  record: a version's status entry only exists in status.DeprecatedVersions while
  its Deployment does (state_mapper.go), so there is no later reconcile to retry
  on once the Deployment is gone.
• execplan.go: new deleteDrainedVersions step runs in the Temporal phase of
  executePlan (executeK8sOperations is K8s-only on this branch), calling
  DeleteVersion for each sunset version with Identity=getControllerIdentity() so
  it satisfies the ManagerIdentity guard, same as the finalizer. Best-effort:
  logs and continues on failure, including NotFound.

Tests:

• planner_test.go: set EligibleForDeletion on drained-deletion fixtures; added a
  negative case (drained + scaled to zero in spec but not eligible -> not deleted).
• deletion_integration_test.go: end-to-end case that rolls v1 -> v2, drains v1,
  and asserts the controller pruned v1's Temporal version record (DescribeVersion
  -> NotFound), not just its Deployment.

Co-authored-by: @tczhao
Co-authored-by: @AshutoshM10
Signed-off-by: Jay Pipes jay.pipes@temporal.io

[GonzaloLuminary]

It would be nice if we had stronger guarantees about this. I don't think it's best practices for operators to manually fix clusters due to these leaks

[GonzaloLuminary]

I wonder if we can have a way to poll temporal state and remove drained versions?

[jaypipes]

@GonzaloLuminary that's a good idea but I think we could add that enhancement in a followup PR. I think this PR that addresses the cleanup of Temporal-side resources by calling DeleteVersion on the drained versions is still worth considering as an iterative improvement to the leaking resource bugs we have here.

[noamyehudai]

Right fix, right hook point, and using getControllerIdentity() matches what actually works: we run an external reaper doing this same delete via the same API and identity, ~6,100 successful deletes over 30 days with zero precondition rejections. Three inline comments, one worth acting on before merge.

[noamyehudai]

The Deployment is already gone by this point, so a failed DeleteVersion strands the server-side record permanently, as the doc comment acknowledges. That turns a 100% leak into a silent ~0.3% one.

Data from our external reaper (same API and identity, 3 envs, 30 days): 6,112 attempts, 6,094 succeeded, 18 failed, and all 18 were transport-level (12x RST_STREAM ... CANCEL, 6x context deadline exceeded), zero precondition rejections. So the residual failures are exactly the transient class a retry would absorb, and here they are the one class that can never be retried.

Would inverting the order work? Call DeleteVersion before deleting the Deployment:

• delete fails -> skip the Deployment delete, return the error; status entry still exists, next reconcile retries. Converges.
• delete succeeds, Deployment delete fails -> retried next reconcile, DeleteVersion returns NotFound, already skipped. Converges.

Preconditions should hold there anyway, since EligibleForDeletion asserts drained with no active pods. If the current order is preferred, a Warning event plus a metric would at least make the residue observable rather than log-only.

[noamyehudai]

eligibleForDeletion is Drained && !(Deployment.Status.Replicas > 0), so deletion now needs spec.Replicas == 0 and status.Replicas == 0 in the same reconcile.

Where that may never coincide: if the rendered per-version autoscaler has minReplicaCount >= 1 (currently the only way to keep the current version warm, #439, and to dodge the promotion deadlock, #438), it re-raises a drained version's replicas every poll while the controller re-zeroes it. We measured ~120 autoscaler raises against 122 controller re-zeroes in one 30-minute window on two drained versions. Under that oscillation the Deployment, and now the server record too, may never be deleted.

Today only spec.Replicas == 0 is required and deletes do land on schedule for us. Adding the status condition narrows the window, and narrows it hardest for the users most affected by #377. Worth documenting at minimum; ideally the gate tolerates pods still terminating.

[noamyehudai]

Two other paths leak server-side records and won't reach this function, so this reduces #377 rather than closing it:

• Never-current (INACTIVE) versions, superseded before promotion: they never take traffic, never drain, never reach Drained. We hold 106 across three envs, all counting toward the cap. Adjacent to [Bug] New version can get stuck INACTIVE and never promote when a scale-to-zero autoscaler holds its Deployment at 0 replicas #438.
• Records whose CR vanished while the version was still CURRENT: we found five, each pinning a non-drainable current version as a permanent floor.

Not asking for either here, just worth saying explicitly that external cleanup is still needed after this lands.