fix: clean up Temporal server-side versioning data on TWD deletion by anujagrawal380 · Pull Request #240 · temporalio/temporal-worker-controller

anujagrawal380 · 2026-03-24T18:18:04Z

Add a finalizer to TemporalWorkerDeployment to run Temporal server-side cleanup before K8s deletion
Add a finalizer to TemporalConnection to prevent it from being deleted while any TWD still references it
On TWD deletion, set current version to unversioned, clear ramping version, and delete registered versions

Problem

When a TemporalWorkerDeployment CRD is deleted (e.g., switching back to plain Deployments), the Temporal server retains the build ID routing configuration. The matching service continues routing new tasks to the deleted build ID's physical queue, while unversioned workers poll a different physical queue. Tasks sit in Scheduled state indefinitely with no errors.

A secondary race condition exists: Helm deletes both the TemporalConnection and TWD in the same upgrade. Without the connection, the controller cannot talk to Temporal to clean up. This is solved by adding a finalizer to the TemporalConnection that blocks its deletion until all referencing TWDs are gone.

Changes

internal/controller/worker_controller.go:

TWD finalizer (temporal.io/worker-deployment-cleanup):

Added to all TWD resources during normal reconciliation
On deletion, triggers handleDeletion() which:
1. Sets the current version to unversioned (BuildID: "") -- the critical step that unblocks task dispatch
2. Clears any ramping version
3. Deletes all registered versions with SkipDrainage: true
4. Attempts to delete the deployment record itself
5. Removes the connection finalizer if no other TWDs reference it
6. Removes its own finalizer, allowing K8s to complete deletion

TemporalConnection finalizer (temporal.io/connection-in-use):

Added to the TemporalConnection during normal TWD reconciliation via ensureConnectionFinalizer()
Prevents the connection from being deleted while any TWD still references it
Removed by removeConnectionFinalizerIfUnused() during TWD deletion, after checking no other TWDs in the same namespace reference the connection
Guarantees the connection is always available during TWD cleanup -- no race condition with Helm deleting both resources simultaneously

RBAC updates:

Added update;patch verbs for temporalconnections (was get;list;watch)
Added update verb for temporalconnections/finalizers

Deletion flow

Helm upgrade (TWD disabled)
  |
  v
Helm deletes TWD CRD + TemporalConnection CRD simultaneously
  |
  +--> TemporalConnection: has finalizer, K8s sets DeletionTimestamp but blocks deletion
  |
  +--> TWD: has finalizer, K8s sets DeletionTimestamp, triggers Reconcile
         |
         v
       handleDeletion() runs:
         1. Fetches TemporalConnection (guaranteed to exist via finalizer)
         2. Connects to Temporal server
         3. Sets current version to unversioned
         4. Deletes versions
         5. Removes connection finalizer (no other TWDs reference it)
         6. Removes TWD finalizer
         |
         v
       TWD deleted by K8s
         |
         v
       TemporalConnection: no more finalizers, deleted by K8s

Issue #55
Closes #166

CLAassistant · 2026-03-24T18:18:12Z

All committers have signed the CLA.

anujagrawal380 · 2026-03-24T18:20:22Z

PTAL @carlydf

jaypipes

@anujagrawal380 awesome contribution, thank you so much for this PR! I couple really minor comments below, but overall excellent work.

anujagrawal380 · 2026-03-25T15:30:44Z

@anujagrawal380 awesome contribution, thank you so much for this PR! I couple really minor comments below, but overall excellent work.

Thanks, resolved both the comments!

jaypipes

rock on :) nice work on this @anujagrawal380!

carlydf

we need integration tests for this before merging to main / including it in a release

anujagrawal380 · 2026-03-26T07:01:34Z

we need integration tests for this before merging to main / including it in a release

@carlydf Added the integration tests. PTAL

carlydf · 2026-04-22T16:59:07Z

Hi @anujagrawal380 , could you fix the linters! Would love to include this in our next release

anujagrawal380 · 2026-04-22T18:10:27Z

Hi @anujagrawal380 , could you fix the linters! Would love to include this in our next release

@carlydf @jaypipes Added few more minor improvements here: 9fd0c74 . PTAL!

jaypipes

yeah, I agree with @carlydf's concern about the deleteCleanupTimeout. I think that should be removed.

carlydf · 2026-04-24T23:17:57Z

@anujagrawal380 we hope to include this change in our release early next week, without the timeout addition. Hopefully you can respond to the feedback when you get back on Monday! To allow the tests to pass in a reasonable amount of time, you can override the pollerTTL to make pollers "die" faster on the server side.
Example

If we don't get a response by Tuesday, we will pick your commits into a separate PR (so you will still be attributed), remove the timeout, make the tests pass and merge it. Hopefully we can come to an agreement about the timeout behavior though!

…blocking unversioned workers Signed-off-by: Anuj Agrawal <anujagrawal380@gmail.com>

Signed-off-by: Anuj Agrawal <anujagrawal380@gmail.com>

…ion finalizer - Add 5-minute deletionCleanupTimeout to prevent TWD stuck in Terminating state indefinitely if Temporal server is unavailable - Return errors from version/deployment deletion to trigger requeue until versions actually clear (pollers disappear as pods terminate) - Add update/patch verbs and finalizers RBAC marker for TemporalConnections - Fix comment-spacing lint on new kubebuilder:rbac markers

…ion tests

anujagrawal380 · 2026-04-27T04:56:31Z

@anujagrawal380 we hope to include this change in our release early next week, without the timeout addition. Hopefully you can respond to the feedback when you get back on Monday! To allow the tests to pass in a reasonable amount of time, you can override the pollerTTL to make pollers "die" faster on the server side. Example

If we don't get a response by Tuesday, we will pick your commits into a separate PR (so you will still be attributed), remove the timeout, make the tests pass and merge it. Hopefully we can come to an agreement about the timeout behavior though!

Thanks @carlydf @jaypipes for the thorough review. Made the changes as suggested. Please let me know if any more changes are required!

- Drop l.Error before returning err from removeConnectionFinalizerIfUnused; controller-runtime logs returned errors automatically - Before deleting the TWD in the deletion test, update to v2.0 image, start v2.0 workers, and set v2.0 as the ramping version at 50% — exercises the clear-ramping-version path in handleDeletion and verifies the ramping version is nil after cleanup

…eletion Per Temporal server behavior, the ramping version must be cleared before setting current to unversioned to avoid a window where traffic is split between unversioned workers and the still-active ramping version.

…Token SetRampingVersion mutates the deployment, making the ConflictToken from the initial Describe stale. Re-describe after Step 1 so SetCurrentVersion in Step 2 uses a valid token instead of failing on the first reconcile.

jaypipes

excellent work on this @anujagrawal380 thank you so much! :)

carlydf

thank you!

The RBAC yaml must be generated from kubebuilder annotations, not edited manually. Running make manifests consolidates the new temporalconnections and temporalconnections/finalizers rules with existing entries.

Shivs11

ty!

When AllAtOnce strategy promotes v2.0 to current before the test sets it as ramping, revert to v1.0 as current first so SetRampingVersion succeeds. Also regenerate CRD YAML with new k8s API fields picked up by make manifests.

anujagrawal380 · 2026-04-27T18:03:49Z

@carlydf Can we please rerun tests again here?

anujagrawal380 · 2026-04-27T18:09:42Z

@carlydf Sorry, lint failed. Please again!

carlydf · 2026-04-27T18:28:16Z

@carlydf Sorry, lint failed. Please again!

@anujagrawal380, this test failed in https://github.com/temporalio/temporal-worker-controller/actions/runs/25011104275/job/73248144790. I'm re-running to see if it was just flaky and maybe will pass the second time. At the same time, ideally we would not have a flaky test since that will cause issues for future PRs. Could you take a look at the failure please?

    --- FAIL: TestIntegration/deletion-sets-current-to-unversioned (5.61s)

…tion test The AllAtOnce reconciler sets ManagerIdentity on the Temporal deployment, blocking other identities from calling SetCurrentVersion. Use a dedicated SDK client with Identity: "temporal-worker-controller" for the revert call, and wrap the entire setup in an eventually loop so the race is handled robustly rather than failing on the first conflict.

anujagrawal380 · 2026-04-27T18:34:51Z

@carlydf Sorry, lint failed. Please again!

@anujagrawal380, this test failed in https://github.com/temporalio/temporal-worker-controller/actions/runs/25011104275/job/73248144790. I'm re-running to see if it was just flaky and maybe will pass the second time. At the same time, ideally we would not have a flaky test since that will cause issues for future PRs. Could you take a look at the failure please?
    --- FAIL: TestIntegration/deletion-sets-current-to-unversioned (5.61s)

@carlydf Added a commit, please rerun once more!

With AllAtOnce the controller continuously re-promotes v2.0 to current, fighting any attempt by the test to set v2.0 as ramping. Switch to Manual strategy and use the existing setCurrentVersion / setRampingVersion helpers (which use defaults.ControllerIdentity) so the test drives versioning state explicitly and handleDeletion's later cleanup calls match the ManagerIdentity.

carlydf · 2026-04-27T19:10:46Z

@carlydf Sorry, lint failed. Please again!

@anujagrawal380, this test failed in https://github.com/temporalio/temporal-worker-controller/actions/runs/25011104275/job/73248144790. I'm re-running to see if it was just flaky and maybe will pass the second time. At the same time, ideally we would not have a flaky test since that will cause issues for future PRs. Could you take a look at the failure please?
    --- FAIL: TestIntegration/deletion-sets-current-to-unversioned (5.61s)
@carlydf Added a commit, please rerun once more!

@anujagrawal380 it failed again. Have not looked at why. Is this passing locally? To run locally you can do:

KUBEBUILDER_ASSETS=/Users/cdf/Desktop/temporal-worker-controller/bin/k8s/1.27.1-darwin-arm64 go test -tags test_dep ./internal/tests/internal
   -run "TestIntegration/deletion-sets-current-to-unversioned" -timeout 600s

but replace the KUBEBUILDER_ASSETS env var with the correct path for your system. To generate those assets and find out the right path, you can run make test-integration and then Ctrl+C the test after the KUBEBUILDER_ASSETS line is printed.

 % make test-integration
test -s /Users/cdf/Desktop/temporal-worker-controller/bin/controller-gen && /Users/cdf/Desktop/temporal-worker-controller/bin/controller-gen --version | grep -q v0.19.0 || \
        GOBIN=/Users/cdf/Desktop/temporal-worker-controller/bin go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.19.0
GOWORK=off GO111MODULE=on /Users/cdf/Desktop/temporal-worker-controller/bin/controller-gen rbac:roleName=manager-role crd:allowDangerousTypes=true,maxDescLen=0,generateEmbeddedObjectMeta=true paths=./api/... paths=./internal/... paths=./cmd/... \
    output:crd:artifacts:config=helm/temporal-worker-controller-crds/templates
python3 hack/sync-rbac-rules.py
Synced RBAC rules from config/rbac/role.yaml → helm/temporal-worker-controller/templates/rbac.yaml
GOWORK=off GO111MODULE=on /Users/cdf/Desktop/temporal-worker-controller/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths=./api/... paths=./internal/... paths=./cmd/...
test -s /Users/cdf/Desktop/temporal-worker-controller/bin/setup-envtest || GOBIN=/Users/cdf/Desktop/temporal-worker-controller/bin go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest
Running integration tests...
KUBEBUILDER_ASSETS="/Users/cdf/Desktop/temporal-worker-controller/bin/k8s/1.27.1-darwin-arm64" go test -v -tags test_dep ./internal/tests/internal -run TestIntegration
^Cmake: *** [test-integration] Interrupt: 2

anujagrawal380 requested review from a team and jlegrone as code owners March 24, 2026 18:18

jaypipes reviewed Mar 25, 2026

View reviewed changes

Comment thread internal/controller/worker_controller.go Outdated

Comment thread internal/controller/worker_controller.go Outdated

jaypipes approved these changes Mar 25, 2026

View reviewed changes

carlydf requested changes Mar 25, 2026

View reviewed changes

carlydf approved these changes Apr 16, 2026

View reviewed changes

anujagrawal380 force-pushed the fix/twd-leaves-stale-versioning-data branch 3 times, most recently from d6a305c to 9fd0c74 Compare April 22, 2026 18:07

carlydf requested changes Apr 23, 2026

View reviewed changes

Comment thread internal/controller/worker_controller.go Outdated

jaypipes requested changes Apr 24, 2026

View reviewed changes

anujagrawal380 added 6 commits April 26, 2026 19:42

fix: deleting a TWD leaves stale versioning data on Temporal server, …

2f52229

…blocking unversioned workers Signed-off-by: Anuj Agrawal <anujagrawal380@gmail.com>

fix: persist connection until twd deletes for cleanup

c0ebbaf

Signed-off-by: Anuj Agrawal <anujagrawal380@gmail.com>

fix: add else log for no ramping version

ba857b9

Signed-off-by: Anuj Agrawal <anujagrawal380@gmail.com>

fix: use single temporal.io/delete-protection finalizer

cd83d2c

Signed-off-by: Anuj Agrawal <anujagrawal380@gmail.com>

fix: integration tests

05fb227

Signed-off-by: Anuj Agrawal <anujagrawal380@gmail.com>

anujagrawal380 force-pushed the fix/twd-leaves-stale-versioning-data branch from 9fd0c74 to 04b61a2 Compare April 26, 2026 14:18

fix: remove unused mgr parameter and dead env construction from delet…

0ab31a3

…ion tests

jaypipes requested changes Apr 27, 2026

View reviewed changes

Comment thread internal/controller/worker_controller.go Outdated

Comment thread internal/controller/worker_controller.go Outdated

Comment thread internal/tests/internal/deletion_integration_test.go

jaypipes reviewed Apr 27, 2026

View reviewed changes

Comment thread internal/tests/internal/deletion_integration_test.go Outdated

anujagrawal380 added 2 commits April 27, 2026 19:56

jaypipes reviewed Apr 27, 2026

View reviewed changes

Comment thread internal/controller/worker_controller.go Outdated

fix: update handleDeletion docstring to reflect actual cleanup sequence

2e4808b

jaypipes approved these changes Apr 27, 2026

View reviewed changes

carlydf approved these changes Apr 27, 2026

View reviewed changes

Comment thread helm/temporal-worker-controller/templates/rbac.yaml

fix: regenerate rbac.yaml via make manifests

5d1c04c

The RBAC yaml must be generated from kubebuilder annotations, not edited manually. Running make manifests consolidates the new temporalconnections and temporalconnections/finalizers rules with existing entries.

Shivs11 approved these changes Apr 27, 2026

View reviewed changes

fix: apply gofmt alignment to deletion integration test

f8e49d9

carlydf merged commit b8d9428 into temporalio:main Apr 27, 2026
18 checks passed

Conversation

anujagrawal380 commented Mar 24, 2026 • edited by carlydf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Changes

Deletion flow

Uh oh!

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

Uh oh!

Uh oh!

anujagrawal380 commented Mar 24, 2026

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

anujagrawal380 commented Mar 25, 2026

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

carlydf left a comment

Choose a reason for hiding this comment

Uh oh!

anujagrawal380 commented Mar 26, 2026

Uh oh!

carlydf commented Apr 22, 2026

Uh oh!

anujagrawal380 commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

carlydf commented Apr 24, 2026

Uh oh!

anujagrawal380 commented Apr 27, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

carlydf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Shivs11 left a comment

Choose a reason for hiding this comment

Uh oh!

anujagrawal380 commented Apr 27, 2026

Uh oh!

anujagrawal380 commented Apr 27, 2026

Uh oh!

carlydf commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anujagrawal380 commented Apr 27, 2026

Uh oh!

carlydf commented Apr 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

anujagrawal380 commented Mar 24, 2026 •

edited by carlydf

Loading

CLAassistant commented Mar 24, 2026 •

edited

Loading

anujagrawal380 commented Apr 22, 2026 •

edited

Loading

carlydf commented Apr 27, 2026 •

edited

Loading