[scottt732]

What type of PR is this?

/kind bug

What this PR does / why we need it:

The Task controller adds and removes kelos.dev/finalizer with Update, which sends the whole Task back to the API server. TaskSpec is immutable (+kubebuilder:validation:XValidation:rule="self == oldSelf"). So when a stored spec no longer round-trips byte for byte through the current Go types, the API server rejects the Update:

Task.kelos.dev "<name>" is invalid: spec: Invalid value: Task spec is immutable after creation

This happens, for example, to Tasks created before a schema change. The deletion path then returns that error on every reconcile ("unable to remove finalizer"), and the Task never finishes deleting. On one cluster this left 144 deleted Tasks stuck for up to four weeks. Each had already had its Job removed, but kept its finalizer.

This PR switches both the add and the remove to a metadata-only merge patch (client.MergeFrom(original)), so the spec is never written back.

Tests: TestReconcile_DeletionRemovesFinalizerWithoutUpdatingSpec and TestReconcile_AddsFinalizerWithoutUpdatingSpec use a fake client whose interceptor refuses every Task Update with the same Invalid error the API server returns. Without the change, both fail with that error. With it, the deleted Task is removed and the new Task gets its finalizer. go test ./internal/controller/ and go vet pass.

Which issue(s) this PR is related to:

N/A

Special notes for your reviewer:

Only the Task controller is changed. Other controllers that Update objects with an immutable spec could be affected in the same way. I kept this PR focused, but can follow up.

Does this PR introduce a user-facing change?

Fix deleted Tasks that could never finish deleting because removing the finalizer was rejected with "Task spec is immutable after creation".

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

Scott Holodak seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[cubic-dev-ai]

1 issue found across 2 files

_{You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.}

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/controller/task_controller.go">

<violation number="1" location="internal/controller/task_controller.go:308">
P1: `MergeFrom` replaces the entire `metadata.finalizers` list without checking `resourceVersion`, so a concurrent finalizer update can be silently lost here (and at the add site). Use `MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})` at both sites so stale patches conflict instead.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic</sub>

[cubic-dev-ai]

P1: MergeFrom replaces the entire metadata.finalizers list without checking resourceVersion, so a concurrent finalizer update can be silently lost here (and at the add site). Use MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{}) at both sites so stale patches conflict instead.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/controller/task_controller.go, line 308:

<comment>`MergeFrom` replaces the entire `metadata.finalizers` list without checking `resourceVersion`, so a concurrent finalizer update can be silently lost here (and at the add site). Use `MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})` at both sites so stale patches conflict instead.</comment>

<file context>
@@ -296,9 +298,14 @@ func (r *TaskReconciler) handleDeletion(ctx context.Context, task *kelos.Task) (
+		original := task.DeepCopy()
 		controllerutil.RemoveFinalizer(task, taskFinalizer)
-		if err := r.Update(ctx, task); err != nil {
+		if err := r.Patch(ctx, task, client.MergeFrom(original)); err != nil {
 			logger.Error(err, "unable to remove finalizer")
 			return ctrl.Result{}, err
</file context>

[gjkim42]

Can you set the commit author properly?