fix(controller): prune superseded inactive worker versions by mitchross · Pull Request #577 · temporalio/temporal-worker-controller

mitchross · 2026-09-09T13:05:59Z

A rollout superseded before its version becomes Current or Ramping stays Inactive forever. The controller scales its Deployment to zero, but only Drained and NotRegistered versions enter deletion, leaking both the Kubernetes Deployment and Temporal version record. This is the Inactive case discussed in #498.

Extend the existing controller deletion path:

Nominate superseded Inactive versions only after Kubernetes has observed zero desired/actual replicas and no terminating replicas; protect current and target versions.
Count running pinned workflows for the exact deployment version using both legacy dot and current colon encodings. Errors, missing responses, and nonzero counts retain the version.
Use normal DeleteVersion without skipping drainage. Temporal refusal retains the Deployment for retry; NotFound permits Kubernetes cleanup.
Retain rendered resources when an Inactive version cannot be deleted.

No CRD or new retirement clock is introduced. The existing sunset delays are defined for Drained versions. Unused Inactive versions are removed after the checks above succeed.

The documented operational limit is explicit: stop sending pinned overrides to versions being retired. Visibility is eventually consistent and the count is not atomic with deletion. Temporal's existing Drained status also does not account for newly pinned overrides. This change does not claim to provide a server-side exclusion against concurrent workflow starts or override updates.

Fixes #596

Validation:

go test ./... -count=1 with envtest assets and Helm dependencies passes.
Targeted integration tests against Kubernetes envtest and real Temporal 1.31.2 pass: superseded never-active version removal, pinned override protection followed by removal after closure, and normal Drained sunset.
The same unused-version integration test fails on the unmodified controller after 90 seconds because its Deployment remains.
Unit coverage includes observed/terminating replicas, current/target guards, visibility failures, server refusal, NotFound retry, and retained rendered resources.

The complete TestIntegration suite passes (475 seconds), covering Manual, AllAtOnce, Progressive, rollback, version limits, resource templates, and deletion. No production deployment has occurred.

CLAassistant · 2026-09-09T13:11:45Z

All committers have signed the CLA.

…ed resources

jaypipes · 2026-09-15T07:20:49Z

@mitchross

A rollout superseded before its version becomes Current or Ramping stays Inactive forever. The controller scales its Deployment to zero, but only Drained and NotRegistered versions enter deletion, leaking both the Kubernetes Deployment and Temporal version record.

A WorkerDeploymentVersion that was superseded before its version became Current was marked as Inactive by Temporal Worker Controller before this fix was committed in TWC v1.10.0/v1.7.1.

Would you mind installing TWC v1.10.0 and verifying that you no longer see the behaviour this PR addresses?

mitchross · 2026-09-15T17:18:17Z

@mitchross

A rollout superseded before its version becomes Current or Ramping stays Inactive forever. The controller scales its Deployment to zero, but only Drained and NotRegistered versions enter deletion, leaking both the Kubernetes Deployment and Temporal version record.

A WorkerDeploymentVersion that was superseded before its version became Current was marked as Inactive by Temporal Worker Controller before this fix was committed in TWC v1.10.0/v1.7.1.

Would you mind installing TWC v1.10.0 and verifying that you no longer see the behaviour this PR addresses?

Absolutely, will report back!

Replace the inactive-retirement fork with the upstream image containing #554, as requested for the upstream verification in temporalio/temporal-worker-controller#577. Keep controller/CRD charts, identity recovery, application workers, gates and rollout settings unchanged. Update the release contract and verification runbook. This switches the controller baseline; it does not claim #577's never-promoted Inactive retirement case is resolved or perform a production rollout.

mitchross · 2026-09-16T05:27:19Z

Thanks for taking a look, @jaypipes! I asked GPT Astra to run the follow-up tests and leave this reply on my behalf.

I'm completely fine with your decision on whether this needs an upstream change, and whether you want to use or adapt this patch. I just wanted to share what the retest found in case you're still curious.

The test used the official v1.10.1 controller image and Temporal 1.32.0 in a separate local Kubernetes cluster with real worker Pods. It started a worker but left it waiting for permission to receive normal jobs (rollout.strategy: Manual), then replaced it with a second version and made that second version Current. The first version was never Current or Ramping and had no workflows assigned to it.

The controller stopped the first version's Pods, but its Kubernetes Deployment and Temporal version record remained after seven minutes. Temporal itself reported it as Inactive.
A separate version that had previously been Current cleaned up successfully.
Restarting stock didn't remove the abandoned records. Changing only the test controller to the existing v1.10.1 backport of this patch removed those same records within 29 seconds. The replacement workers continued working.

Here's the standalone reproduction, sample worker, setup commands, and test details:
https://gist.github.com/mitchross/ec514b49662c1700fb438287f75ce25d

The original cluster test was run successfully; the Gist packaging hasn't had a separate run from scratch yet. It uses public dependencies and doesn't need access to my cluster or application repositories.

Could this be a separate case from the one #554 fixes? Either way, thanks for reviewing it!

jaypipes

@mitchross thanks very much for this PR! Left a few inline suggestions for you, but nothing major :)

jaypipes · 2026-09-16T11:21:42Z

+
+Stop sending pinned version overrides to a version being retired. Visibility is eventually
+consistent, so this check cannot exclude concurrent workflow starts or override changes.
+Temporal's drained status has the [same limitation for newly pinned overrides](https://typescript.temporal.io/api/interfaces/proto.temporal.api.deployment.v1.IWorkerDeploymentVersionInfo#drainageinfo).


Remove the above last paragraph. It's too much sausagemaking I'm afraid and will likely just confuse readers.

jaypipes · 2026-09-16T11:22:43Z

 - **scaledownDelay**: How long to wait after a version has been Drained before scaling pods to zero
 - **deleteDelay**: How long to wait after a version has been Drained before deleting the Kubernetes `Deployment`

+Versions superseded before ever becoming Current or Ramping remain Inactive in Temporal;


Let's convert this into a callout...

Suggested change

Versions superseded before ever becoming Current or Ramping remain Inactive in Temporal;

> **NOTE**: Versions superseded before ever becoming Current or Ramping remain Inactive in Temporal;

jaypipes · 2026-09-16T11:23:44Z

+do not apply to these unused versions. API failures or active pollers defer deletion and are
+retried on subsequent reconciliations.


Suggested change

do not apply to these unused versions. API failures or active pollers defer deletion and are

retried on subsequent reconciliations.

do not apply to these unused versions.

(removed the last line because it's just sausagemaking and not particularly important to convey to the reader)

jaypipes · 2026-09-16T11:31:10Z

+			// Visibility can contain either the legacy dot separator or the newer colon form.
+			legacyVersion := strings.ReplaceAll(p.WorkerDeploymentName+"."+buildID, "'", "''")
+			version := strings.ReplaceAll(p.WorkerDeploymentName+":"+buildID, "'", "''")
+			count, err := temporalClient.CountWorkflow(ctx, &workflowservice.CountWorkflowExecutionsRequest{
+				Query: fmt.Sprintf("TemporalWorkerDeploymentVersion IN ('%s', '%s') AND TemporalWorkflowVersioningBehavior = 'Pinned' AND ExecutionStatus = 'Running'", legacyVersion, version),
+			})
+			if err != nil || count == nil || count.Count != 0 {


Please make a little helper function getOpenPinnedWorkflowExecutions() to encapsulate this query.

jaypipes · 2026-09-16T11:35:27Z

+// DeleteVersion does not check pinned execution visibility for Inactive versions,
+// which can receive workflows through VersioningOverride. Check visibility first.


Suggested change

// DeleteVersion does not check pinned execution visibility for Inactive versions,

// which can receive workflows through VersioningOverride. Check visibility first.

// Because DeleteVersion does not check to see if there are open workflows using

// pinned execution for Inactive versions, we query the Temporal visibility service

// for open pinned workflows before deleting Inactive versions.

jaypipes · 2026-09-16T11:38:18Z

+			legacyVersion := strings.ReplaceAll(p.WorkerDeploymentName+"."+buildID, "'", "''")
+			version := strings.ReplaceAll(p.WorkerDeploymentName+":"+buildID, "'", "''")
+			count, err := temporalClient.CountWorkflow(ctx, &workflowservice.CountWorkflowExecutionsRequest{
+				Query: fmt.Sprintf("TemporalWorkerDeploymentVersion IN ('%s', '%s') AND TemporalWorkflowVersioningBehavior = 'Pinned' AND ExecutionStatus = 'Running'", legacyVersion, version),


Might be a little easier to read if you create a qs variable first and pass that:

qs := fmt.Sprintf( "TemporalWorkerDeploymentVersion IN ('%s', '%s') "+ "AND TemporalWorkflowVersioningBehavior = 'Pinned' "+ "AND ExecutionStatus = 'Running'", legacyVersion, version, ) req := &workflowservice.CountWorkflowExecutionsRequest{Query: qs} count, err := temporalClient.CountWorkflow(ctx, req) ...

jaypipes · 2026-09-16T11:45:07Z

 ) {
 	identity := getControllerIdentity()
 	markedForDeletion := make([]*appsv1.Deployment, 0, len(p.DeleteDeployments))
+	retainedInactiveBuilds := make(map[string]bool)


Unless I'm mistaken, you don't need the above retainedInactiveBuilds variable. The continue on line 734 will cause the Deployment to not be added to the markedForDeletion slice on line 755...

Extract the pinned-workflow visibility query, clarify its formatting and safety comment, and distinguish confirmed running workflows from failed visibility checks in logs. Shorten the concepts documentation into a note. Rename retainedInactiveBuilds to retainedResourceBuilds and explain its purpose: skipping a Deployment does not filter the separate worker-resource deletion list. Preserve the existing protection for retained versions. Validation: go test ./... and go vet ./... passed. Server-backed integration tests passed for normal drained retirement and inactive retirement with and without a pinned workflow. Existing unit cases verify rendered ConfigMaps survive visibility failures, pinned workflows, and rejected deletion.

jaypipes · 2026-09-16T15:11:45Z

@mitchross hey, just FYI, the TWC maintainers all hang out on the #temporal-workers channel in the Temporal Slack community. If you'd like a higher-bandwidth conversation about any of your PRs or issues, feel free to hop over there :)

mitchross · 2026-09-17T00:27:49Z

@mitchross hey, just FYI, the TWC maintainers all hang out on the #temporal-workers channel in the Temporal Slack community. If you'd like a higher-bandwidth conversation about any of your PRs or issues, feel free to hop over there :)

Thanks for the heads up. Getting some practice in at home as we start to ramp up Temporal in a cluster at $JOB.

I think I hit on all the comments. In full transparency I leveraged AI, GPT 6. I purposely used the temporal skills , and temporal MCP docs server to ensure adherence.

jaypipes · 2026-09-17T11:45:22Z

+		if slices.ContainsFunc(workerDeploy.Status.DeprecatedVersions, func(v *temporaliov1alpha1.DeprecatedWorkerDeploymentVersion) bool {
+			return v.BuildID == buildID && v.Status == temporaliov1alpha1.VersionStatusInactive
+		}) {
+			retainedResourceBuilds[buildID] = true


Not sure if you caught my earlier comment about this, but I don't believe you need the retainedResourceBuilds variable at all. Pretty sure the existing markedForDeletion variable and behaviour is all that is needed.

jaypipes · 2026-09-17T11:46:24Z

@mitchross hey, just FYI, the TWC maintainers all hang out on the #temporal-workers channel in the Temporal Slack community. If you'd like a higher-bandwidth conversation about any of your PRs or issues, feel free to hop over there :)

Thanks for the heads up. Getting some practice in at home as we start to ramp up Temporal in a cluster at $JOB.

Nice! :)

I think I hit on all the comments. In full transparency I leveraged AI, GPT 6. I purposely used the temporal skills , and temporal MCP docs server to ensure adherence.

Totally cool. Thanks for the transparency, it is much appreciated!

Remove retainedResourceBuilds as requested in review. Use markedForDeletion to identify rejected Inactive Deployment deletions and preserve their worker resources without maintaining a separate map. Keep independent resource deletions, including orphan cleanup and drained autoscaler sunset, unchanged. Extend the existing regression cases to check orphan cleanup alongside retained Inactive versions. Report the actual observed state when the inactive-retirement integration test times out. Validation: unit tests, go vet, import formatting, and the three targeted server-backed retirement integration scenarios passed locally.

jaypipes · 2026-09-18T09:22:12Z

+	// Keep resources belonging to inactive Deployments whose deletion was refused.
+	// Other resource deletions, such as orphan cleanup, remain in the plan.
+	for _, d := range p.DeleteDeployments {
+		if slices.Contains(markedForDeletion, d) {
+			continue
+		}
+		buildID, ok := d.Labels[k8s.BuildIDLabel]
+		if !ok {
+			continue
+		}
+		if !slices.ContainsFunc(workerDeploy.Status.DeprecatedVersions, func(v *temporaliov1alpha1.DeprecatedWorkerDeploymentVersion) bool {
+			return v.BuildID == buildID && v.Status == temporaliov1alpha1.VersionStatusInactive
+		}) {
+			continue
+		}
+		p.DeleteWorkerResources = slices.DeleteFunc(p.DeleteWorkerResources, func(ref planner.WorkerResourceRef) bool {
+			return ref.BuildID == buildID
+		})
 	}


@mitchross I think maybe Claude is getting confused :) I don't believe this block of code is needed. Inactive build IDs that have open workflows are not added to the markedForDeletion slice (line 753) because of the continue statements on lines 728 and 732.

Remove the additional worker-resource filtering block as requested in review. Keep the existing markedForDeletion handling for Deployments. Validation: go vet ./... and git diff --check passed. go test ./... fails four existing TestExecutePlan_InactiveVersionDeletion cases with a missing ConfigMap: running pinned workflow, visibility failure, missing response, and rejected server deletion. Test expectations are unchanged.

Combine upstream per-version DeleteVersion retry backoff with the existing Inactive-version visibility check and its helper. Preserve the reviewer's requested removal of the worker-resource filtering block. Validation: go vet ./..., import formatting, and git diff --check pass. go test ./... reports the same four existing ConfigMap-retention failures; all other tests, including the upstream deletion-backoff tests, pass.

jaypipes

muy bueno, thank you so much @mitchross! :)

fix(controller): propose pruning superseded inactive versions

f842f0d

mitchross mentioned this pull request Sep 10, 2026

fix(temporal): retire abandoned versions in the shared worker controller mitchross/talos-argocd-proxmox#2332

Merged

test: verify inactive retirement against Temporal and preserve retain…

0d1aee7

…ed resources

mitchross changed the title ~~fix(controller): prune superseded inactive versions~~ fix(controller): prune superseded inactive worker versions Sep 10, 2026

mitchross marked this pull request as ready for review September 10, 2026 22:10

mitchross requested review from a team, eniko-dif and jlegrone as code owners September 10, 2026 22:10

mitchross mentioned this pull request Sep 14, 2026

Make Temporal releases gated and old worker versions restartable mitchross/talos-argocd-proxmox#2391

Merged

mitchross mentioned this pull request Sep 15, 2026

Return Temporal Worker Controller to official v1.10.1 mitchross/talos-argocd-proxmox#2399

Merged

jaypipes requested changes Sep 16, 2026

View reviewed changes

jaypipes added this to the vNextNext milestone Sep 16, 2026

jaypipes reviewed Sep 17, 2026

View reviewed changes

jaypipes reviewed Sep 18, 2026

View reviewed changes

mitchross added 2 commits September 18, 2026 17:18

jaypipes approved these changes Sep 19, 2026

View reviewed changes

	Versions superseded before ever becoming Current or Ramping remain Inactive in Temporal;
	> NOTE: Versions superseded before ever becoming Current or Ramping remain Inactive in Temporal;

		do not apply to these unused versions. API failures or active pollers defer deletion and are
		retried on subsequent reconciliations.

	do not apply to these unused versions. API failures or active pollers defer deletion and are
	retried on subsequent reconciliations.
	do not apply to these unused versions.

		// DeleteVersion does not check pinned execution visibility for Inactive versions,
		// which can receive workflows through VersioningOverride. Check visibility first.

-// DeleteVersion does not check pinned execution visibility for Inactive versions,
-// which can receive workflows through VersioningOverride. Check visibility first.
+// Because DeleteVersion does not check to see if there are open workflows using
+// pinned execution for Inactive versions, we query the Temporal visibility service
+// for open pinned workflows before deleting Inactive versions.

Conversation

mitchross commented Sep 9, 2026 • edited by jaypipes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaypipes commented Sep 15, 2026

Uh oh!

mitchross commented Sep 15, 2026

Uh oh!

mitchross commented Sep 16, 2026

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaypipes commented Sep 16, 2026

Uh oh!

mitchross commented Sep 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaypipes commented Sep 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mitchross commented Sep 9, 2026 •

edited by jaypipes

Loading

CLAassistant commented Sep 9, 2026 •

edited

Loading