fix: recover AuthReconcile visitor panics (#29436)

Project: argoproj/argo-cd | Language: Go | Concepts: concurrency, error-handling, testing-strategy | Difficulty: mid
Link: https://github.com/argoproj/argo-cd/pull/29440

The problem

The kubectl auth reconcile operation in Argo CD visits multiple RBAC manifests concurrently. If a panic occurs inside a visitor callback (for example, due to a forbidden impersonation), the panic happens in a child goroutine. The existing recovery logic at the entry point of AuthReconcile cannot catch panics from these child goroutines, causing the controller to crash.

Why it's non-trivial

A naive fix might add a recover() at the top level, but in Go, recover() only works in the same goroutine where the panic occurs. Since the visitor callbacks may run in separate goroutines, recovery must be placed directly around the callback invocation. Additionally, the fix must ensure that normal errors are preserved and only panics are wrapped as reconciliation errors. Testing must also simulate panics in child goroutines to verify correct recovery.

The approach

1. Wrap the visitor to recover panics in each callback invocation:

+	// TODO(#29484): Remove this wrapper after the upstream kubectl panic fix is available in Argo CD.
+	opts.Visitor = panicRecoveringVisitor{Visitor: opts.Visitor}

This line wraps the original visitor with a custom type (panicRecoveringVisitor) that will handle panic recovery for each callback.

2. Implement the panic-recovering visitor:

+type panicRecoveringVisitor struct {
+	resource.Visitor
+}
+
+func (v panicRecoveringVisitor) Visit(fn resource.VisitorFunc) error {
+	return v.Visitor.Visit(func(info *resource.Info, err error) (retErr error) {
+		defer func() {
+			if r := recover(); r != nil {
+				retErr = fmt.Errorf("error running kubectl auth reconcile: %v", r)
+			}
+		}()
+		return fn(info, err)
+	})
+}

This type embeds the original visitor and overrides its Visit method. It wraps the callback (fn) in a defer block that recovers from panics, converting any panic into a formatted error. Ordinary errors returned by the callback are left unchanged.

3. Add regression tests for both panic recovery and error preservation:

+type concurrentTestVisitor struct {
+	info *resource.Info
+	err  error
+}
+
+func (v concurrentTestVisitor) Visit(fn resource.VisitorFunc) error {
+	errCh := make(chan error, 1)
+	go func() {
+		errCh <- fn(v.info, v.err)
+	}()
+	return <-errCh
+}

This test visitor executes the callback in a new goroutine, simulating the concurrency of the real implementation.

+func TestRealKubectlOptionsRunner_AuthReconcile_ConcurrentVisitorPanicRecovery(t *testing.T) {
+	t.Parallel()
+	runner := &realKubectlOptionsRunner{}
+	opts := auth.NewReconcileOptions(genericclioptions.IOStreams{})
+	opts.Visitor = concurrentTestVisitor{}
+
+	err := runner.AuthReconcile(opts)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "error running kubectl auth reconcile")
+}

This test verifies that a panic in a concurrent visitor is recovered and returned as an error.

+func TestRealKubectlOptionsRunner_AuthReconcile_VisitorError(t *testing.T) {
+	t.Parallel()
+	runner := &realKubectlOptionsRunner{}
+	expectedErr := errors.New("visit failed")
+	opts := auth.NewReconcileOptions(genericclioptions.IOStreams{})
+	opts.Visitor = concurrentTestVisitor{err: expectedErr}
+
+	err := runner.AuthReconcile(opts)
+	require.ErrorIs(t, err, expectedErr)
+}

This test ensures that ordinary errors from the visitor are preserved and not masked by the panic recovery logic.

From the review thread

No notable review discussion on this PR.

Takeaways

• Recover panics as close as possible to their origin, especially in concurrent code. In Go, recover() only works in the same goroutine, so wrap callback invocations directly if they may run concurrently.
• Preserve the distinction between errors and panics. Only convert panics to errors; let ordinary errors propagate unchanged to avoid masking real error conditions.
• Test concurrency-specific error handling explicitly. Use test visitors that invoke callbacks in goroutines to simulate real-world concurrency and verify both panic recovery and error propagation.