fix: Resolve ServiceAccount deletion race condition (thanks @nss10!) by lbeckman314 · Pull Request #1421 · calypr/funnel

lbeckman314 · 2026-05-20T17:40:04Z

Overview 🌀

This PR address Issue #71 Clean Orphaned Resources as Go routine + PVC Finalizers by adding improvements to how ServiceAccounts and related task resources are managed by Funnel.

Current Behavior ⚠️

In Funnel 2026-04-21, cleanOrphanedResources was moved to run at app startup. However, it was implemented as a blocking call, which causes health checks to fail when there are multiple tasks requiring cleanup. Additionally, after running for some time, the Kubernetes API becomes rate limited, causing the pod to crash.

Proposed fixes:

Convert to a goroutine: — Run cleanOrphanedResources asynchronously so it no longer blocks startup and health checks can proceed normally.
Prevent Kubernetes API rate limiting — Implement a batched deletion strategy with a sleep interval between batches to spread out API calls and stay within rate limits.

New Behavior ✔️

Added 5 second delay to avoid race conditions when cleaning up ServiceAccounts (serviceaccount.go)
Removed code cleaning up Namespace-scoped resources in favor of automatic K8s Garbage Collection via Ownership fields (backend.go)

Breaking Changes? ❌

Caution

Yes! This update currently breaks the TTL config first introduced by PR #1357.

This feature is being re-introduced as part of PR #1410 to avoid breaking existing configurations.

Signed-off-by: Liam Beckman <lbeckman314@gmail.com>

github-actions · 2026-05-20T18:02:01Z

Gen3 Integration Tests

filepath	passed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	13	5	18
TOTAL	13	5	18

- K8s fake client does not support cascading delete from Owner field Signed-off-by: Liam Beckman <lbeckman314@gmail.com>

Signed-off-by: Liam Beckman <lbeckman314@gmail.com>

[skip ci] Signed-off-by: Liam Beckman <lbeckman314@gmail.com>

Signed-off-by: Liam Beckman <lbeckman314@gmail.com>

github-actions · 2026-05-21T22:46:21Z

Gen3 Integration Tests

filepath	passed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	13	5	18
TOTAL	13	5	18

nss10 · 2026-05-22T05:14:10Z

 	}

-	// Delete PVC
-	err = resources.DeletePVC(ctx, taskId, b.conf.Kubernetes.JobsNamespace, b.client, b.log)


@lbeckman314 — Deferring the deletion of these resources to Kubernetes using ownerReferences sounds good. However, we should handle that transition in a separate PR.

It also just occurred to me: what happens to cleanOrphanedResources? If there are any pending resources left behind by older server runs or due to previous deletion errors, our logic might track them but never actually clean them up.

Kubernetes garbage collection will continuously retry deleting these resources until they are gone, so we don't need to worry about transient API errors or rate limiting.

Given this, we should do one of two things:

Make these changes in a separate PR, and mark this change as a breaking change.

Update the cleanOrphanedResources method so it no longer looks for these specific resources as "orphaned," since Kubernetes will handle the cleanup natively.

Updated in PR #1423!

Hi @lbeckman314 , I was suggesting #1423 to be a PR pointed to develop, such that it includes all changes related to resource cleanup. Keep #1421 only about handling Service Account deletion race condition. All logic regarding resource cleanup should be handled in #1423 only.

nss10 · 2026-05-22T05:15:48Z

-	if err == nil {
-		t.Error("expected ConfigMap to be deleted, but it still exists")
-	}
+	// ConfigMap deletion is handled by Kubernetes garbage collection via ownerReferences,


Instead of Configmap, look for a resource which is actually being cleaned up by cleanResources -- say PV. But as I mentioned above, it has to be in a PR of its own.

Updated in PR #1423!

Move backend.go and backend_test.go resource cleanup changes to review/service-account-deletion (PR #1423) per reviewer request. PR #1421 should only contain the ServiceAccount deletion race condition fix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Assisted-by: Claude Code:claude [Claude Code]

github-actions · 2026-05-29T03:27:12Z

Gen3 Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	12	1	5	18
TOTAL	12	1	5	18

Test summary after rerunning failed integration tests

filepath	failed	SUBTOTAL
tests/test_gen3_workflow.py	1	1
TOTAL	1	1

github-actions · 2026-05-29T18:34:15Z

Gen3 Integration Tests

filepath	passed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	13	5	18
TOTAL	13	5	18

github-actions · 2026-06-03T20:57:37Z

Gen3 Integration Tests

filepath	passed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	25	4	29
TOTAL	25	4	29

@nss10

* fix: Resolve ServiceAccount deletion race condition (thanks @nss10!) Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * fix: K8s Unit Tests - K8s fake client does not support cascading delete from Owner field Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * chore: Revert function name in serviceaccount.go Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * chore: Update comment [skip ci] Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * chore: Remove unused parameter Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * chore: Restrict fix/service-account-deletion to SA race condition only Move backend.go and backend_test.go resource cleanup changes to review/service-account-deletion (PR #1423) per reviewer request. PR #1421 should only contain the ServiceAccount deletion race condition fix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Assisted-by: Claude Code:claude [Claude Code] * fix: Add support for cleaning orphaned Executor jobs Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * fix: Test + lint errors * fix: Linting errors Signed-off-by: Liam Beckman <lbeckman314@gmail.com> --------- Signed-off-by: Liam Beckman <lbeckman314@gmail.com>

@nss10

…ackoffLimit`) (#1413) * fix: Ensure CreateContainerConfigError results in SYSTEM_ERROR Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * fix: Minor linting error in kubernetes backend test Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * feat: Update ContainerConfigError status check in K8s Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * fix: Correctly mark tasks as SYSTEM_ERROR when K8s Job creation fails Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * fix: enrich Kubernetes error diagnostics with pod warning events and exit info Surfaces human-readable detail from pod Warning events and container termination status into TES system logs, so users can see messages like 'secret "foo" not found' without needing kubectl access. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Assisted-by: Claude Code:claude [Claude Code] * fix: Resolve ServiceAccount deletion race condition (thanks @nss10!) Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * fix: K8s Unit Tests - K8s fake client does not support cascading delete from Owner field Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * chore: Revert function name in serviceaccount.go Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * chore: Update comment [skip ci] Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * chore: Remove unused parameter Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * feat: Add state filter in ListTasks (Credit: @nss10) - Original source: 7f60650 * feat: Add support for graceful FailedCreateEvents Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * chore: Combine `if` statements in kubernetes/backend.go Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * fix: Add Executor Pod status check Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * fix: Correctly set K8sExecutorErr reason fallback * feat: Add Executor-pod error events to SYSTEM_LOG * chore: Restrict fix/service-account-deletion to SA race condition only Move backend.go and backend_test.go resource cleanup changes to review/service-account-deletion (PR #1423) per reviewer request. PR #1421 should only contain the ServiceAccount deletion race condition fix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Assisted-by: Claude Code:claude [Claude Code] * fix: Add ImagePullBackOff to terminal pod states (SYSTEM_ERROR) * fix: Add remaining K8s terminal pod states (SYSTEM_ERROR) Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * chore: Remove debug fmt.Println statements from reconciler Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Assisted-by: Claude Code:claude [Claude Code] * refactor: Unify FetchPodWarningEvents as a shared exported function Rename fetchExecutorPodWarningEvents (worker) and the backend method fetchPodWarningEvents to a single exported FetchPodWarningEvents in the compute/kubernetes package. worker/kubernetes.go now imports and calls the shared function, eliminating duplicate logic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Assisted-by: Claude Code:claude [Claude Code] * fix: Replace maxErrEventWrites with failedCreateThreshold + minFailureSpan hasJobFailedCreateEvent now fetches all job events in one API call and requires both a minimum occurrence count (failedCreateThreshold=5) and a minimum time span between first and last failure (minFailureSpan=20s) before treating FailedCreate events as a persistent error. This avoids false-positives from rapid transient bursts early in a job's lifecycle. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Assisted-by: Claude Code:claude [Claude Code] * fix: Update ImagePullBackOff test to match current terminal waiting list ImagePullBackOff was added to terminalWaitingReasons but the corresponding test case was not updated. Correct the assertion. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Assisted-by: Claude Code:claude [Claude Code] * fix: Minor linting fix * fix: Add support for cleaning orphaned Executor jobs Signed-off-by: Liam Beckman <lbeckman314@gmail.com> * refactor: Address remaining PR review comments - Move terminalWaitingReasons inside hasTerminalContainerWaitingError (no longer needed at package scope) - Remove InitContainers check (YAGNI — worker/executor pods don't use init containers) - Use slices.Contains in hasTerminalContainerWaitingError - Consolidate pod list fetch: fetch pods once per job and pass to both hasTerminalContainerWaitingError and FetchPodWarningEvents to avoid redundant K8s API calls - Update FetchPodWarningEvents signature to accept *corev1.PodList instead of jobName - Update all callers (reconciler, worker/kubernetes.go, tests) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Assisted-by: Claude Code:claude [Claude Code] * Update backend.go --------- Signed-off-by: Liam Beckman <lbeckman314@gmail.com> Co-authored-by: Sai Shanmukha <nss10@outlook.com>

fix: Resolve ServiceAccount deletion race condition (thanks @nss10!)

d2e910b

Signed-off-by: Liam Beckman <lbeckman314@gmail.com>

github-code-quality Bot found potential problems May 20, 2026

View reviewed changes

Comment thread worker/kubernetes.go Fixed

lbeckman314 force-pushed the fix/service-account-deletion branch from 589dab4 to d2e910b Compare May 21, 2026 22:13

lbeckman314 added 3 commits May 21, 2026 15:15

fix: K8s Unit Tests

0a8117a

- K8s fake client does not support cascading delete from Owner field Signed-off-by: Liam Beckman <lbeckman314@gmail.com>

chore: Revert function name in serviceaccount.go

7a5616f

Signed-off-by: Liam Beckman <lbeckman314@gmail.com>

chore: Update comment

fdc9b71

[skip ci] Signed-off-by: Liam Beckman <lbeckman314@gmail.com>

lbeckman314 self-assigned this May 21, 2026

lbeckman314 added the bug label May 21, 2026

lbeckman314 added this to the Gen3Workflow Production-Ready Support milestone May 21, 2026

chore: Remove unused parameter

7c134e7

Signed-off-by: Liam Beckman <lbeckman314@gmail.com>

nss10 suggested changes May 22, 2026

View reviewed changes

lbeckman314 mentioned this pull request May 27, 2026

fix: Update Funnel resource cleanup all K8s resources (including Worker + Executor jobs) #1423

Merged

nss10 approved these changes May 29, 2026

View reviewed changes

Merge branch 'develop' into fix/service-account-deletion

d496921

lbeckman314 merged commit 89af0db into develop Jun 3, 2026
18 checks passed

lbeckman314 deleted the fix/service-account-deletion branch June 3, 2026 22:39

lbeckman314 mentioned this pull request Jun 9, 2026

feat: Re-add reconcile support for GCP Batch #1430

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Resolve ServiceAccount deletion race condition (thanks @nss10!)#1421

fix: Resolve ServiceAccount deletion race condition (thanks @nss10!)#1421
lbeckman314 merged 7 commits into
developfrom
fix/service-account-deletion

lbeckman314 commented May 20, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

nss10 May 22, 2026

Uh oh!

lbeckman314 May 27, 2026

Uh oh!

nss10 May 27, 2026

Uh oh!

nss10 May 22, 2026

Uh oh!

lbeckman314 May 27, 2026

Uh oh!

github-actions Bot commented May 29, 2026

Uh oh!

github-actions Bot commented May 29, 2026

Uh oh!

github-actions Bot commented Jun 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

lbeckman314 commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview 🌀

Current Behavior ⚠️

New Behavior ✔️

Breaking Changes? ❌

Uh oh!

github-actions Bot commented May 20, 2026

Gen3 Integration Tests

Uh oh!

Uh oh!

github-actions Bot commented May 21, 2026

Gen3 Integration Tests

Uh oh!

nss10 May 22, 2026

Choose a reason for hiding this comment

Uh oh!

lbeckman314 May 27, 2026

Choose a reason for hiding this comment

Uh oh!

nss10 May 27, 2026

Choose a reason for hiding this comment

Uh oh!

nss10 May 22, 2026

Choose a reason for hiding this comment

Uh oh!

lbeckman314 May 27, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 29, 2026

Gen3 Integration Tests

Uh oh!

github-actions Bot commented May 29, 2026

Gen3 Integration Tests

Uh oh!

github-actions Bot commented Jun 3, 2026

Gen3 Integration Tests

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lbeckman314 commented May 20, 2026 •

edited

Loading