fix(meta): RemoveFinalizer handles duplicate finalizers without panicking

[arpitjain099]

Description of your changes

RemoveFinalizer in pkg/meta deletes from the finalizer slice in place while ranging over it by index, which is the classic delete-while-ranging bug:

for i, e := range f {
    if e == finalizer {
        f = append(f[:i], f[i+1:]...)
    }
}

range fixes the length at loop start but append(f[:i], f[i+1:]...) shrinks f underneath it, so after one removal the loop keeps indexing into a now-shorter slice. When the finalizer appears more than once this either panics (["fin","fin"]) or silently leaves a copy behind (["fin","fin","fun"] returns ["fin","fun"]). It backs APIFinalizer.RemoveFinalizer on the managed-resource deletion path, so a duplicate finalizer (concurrent reconciles, patch races, version skew) can crash a controller during deletion or leave it stuck Terminating, which lines up with #1010. AddFinalizer already dedupes with slices.Contains; this side didn't.

Swapped the loop for slices.DeleteFunc, which removes all matches in one pass. slices is already imported here, so no new deps. Added two duplicate cases to TestRemoveFinalizer; they panic before the change and pass after.

go test -race ./pkg/meta/..., go vet ./pkg/meta/..., and go build ./... are all green.

I have:

• Read and followed Crossplane's contribution process.
• Run ./nix.sh flake check to ensure this PR is ready for review. (ran go test -race, go vet, and go build ./... directly; happy to run flake check if needed)
• Added or updated unit tests.
• Linked a PR or a docs tracking issue to document this change. (internal helper, no user-facing docs)
• Added backport release-x.y labels to auto-backport this PR. (deferring to maintainers on backport scope)

[coderabbitai]

📝 Walkthrough

Walkthrough

RemoveFinalizer in pkg/meta/meta.go replaces a manual index-based loop with slices.DeleteFunc to remove all matching finalizer strings. Two new table-driven test cases are added to TestRemoveFinalizer covering duplicate finalizer entries.

Changes

RemoveFinalizer duplicate handling

Layer / File(s)	Summary
slices.DeleteFunc implementation and duplicate tests pkg/meta/meta.go, pkg/meta/meta_test.go	RemoveFinalizer switches to slices.DeleteFunc to remove all occurrences of the matching finalizer; two new test cases validate removal when the finalizer appears twice (alone and alongside another finalizer).

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title is relevant, but it exceeds the 72-character limit by one character.	Shorten it to 72 characters or fewer while keeping the duplicate-finalizer fix described.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Breaking Changes	✅ Passed	Public API signatures are unchanged; RemoveFinalizer only changes duplicate-finalizer edge cases and normal cases are preserved by tests.
Description check	✅ Passed	The description directly matches the code and test changes to RemoveFinalizer and its duplicate-finalizer fix.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

pkg/meta/meta_test.go (1)

541-562: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Could we add a reason field to these new cases?

That would keep the duplicate scenarios aligned with the repo’s table-driven test convention and make failures easier to interpret. Thanks for adding the regression coverage here.

Example update

 cases := map[string]struct {
+	reason string
 	args args
 	want []string
 }{
 	"DuplicateFinalizersExist": {
+		reason: "RemoveFinalizer should remove every matching duplicate entry",
 		args: args{
 			o: &corev1.Pod{
 				ObjectMeta: metav1.ObjectMeta{
 					Finalizers: []string{finalizer, finalizer},
 				},
 			},
 			finalizer: finalizer,
 		},
 		want: []string{},
 	},
 	"DuplicateFinalizersWithOther": {
+		reason: "RemoveFinalizer should preserve non-matching finalizers while removing duplicate matches",
 		args: args{
 			o: &corev1.Pod{
 				ObjectMeta: metav1.ObjectMeta{
 					Finalizers: []string{finalizer, funalizer, finalizer},
 				},
 			},
 			finalizer: finalizer,
 		},
 		want: []string{funalizer},
 	},
 }

As per path instructions, **/*_test.go: Enforce table-driven test structure: PascalCase test names (no underscores), args/want pattern, use cmp.Diff with cmpopts.EquateErrors() for error testing. Check for proper test case naming and reason fields.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/meta/meta_test.go` around lines 541 - 562, Add a reason field to the new
table-driven cases in the meta test table so they match the repo’s test
convention and are easier to diagnose when they fail. Update the
DuplicateFinalizersExist and DuplicateFinalizersWithOther entries in the test
case list to include a concise reason string, keeping the existing args and want
structure unchanged. Use the surrounding table-driven pattern in the meta test
to keep the new cases consistent with the rest of the suite.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@pkg/meta/meta_test.go`:
- Around line 541-562: Add a reason field to the new table-driven cases in the
meta test table so they match the repo’s test convention and are easier to
diagnose when they fail. Update the DuplicateFinalizersExist and
DuplicateFinalizersWithOther entries in the test case list to include a concise
reason string, keeping the existing args and want structure unchanged. Use the
surrounding table-driven pattern in the meta test to keep the new cases
consistent with the rest of the suite.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ab4d406d-80c2-4a42-8b81-007ea282b00a

📥 Commits

Reviewing files that changed from the base of the PR and between 9b51ac3 and 3a45bed.

📒 Files selected for processing (2)

• pkg/meta/meta.go
• pkg/meta/meta_test.go