use a deletion tracker to revert invalid deletions

Signed-off-by: Chetan Banavikalmutt <[email protected]>
Assisted-by: Cursor

Author: chetan-rns

agent/agent.go

@@ -107,6 +107,10 @@ type Agent struct {
 
 	// sourceCache is a cache of resources from the source. We use it to revert any changes made to the local resources.
 	sourceCache *cache.SourceCache
+
+	// deletions tracks valid deletions from the source.
+	// This is used to differentiate between valid and invalid deletions
+	deletions *manager.DeletionTracker
 }
 
 const defaultQueueName = "default"
@@ -132,7 +136,9 @@ type AgentOption func(*Agent) error
 // options.
 func NewAgent(ctx context.Context, client *kube.KubernetesClient, namespace string, opts ...AgentOption) (*Agent, error) {
 	a := &Agent{
-		version: version.New("argocd-agent"),
+		version:     version.New("argocd-agent"),
+		deletions:   manager.NewDeletionTracker(),
+		sourceCache: cache.NewSourceCache(),
 	}
 	a.infStopCh = make(chan struct{})
 	a.namespace = namespace
@@ -200,6 +206,7 @@ func NewAgent(ctx context.Context, client *kube.KubernetesClient, namespace stri
 	appManagerOpts := []application.ApplicationManagerOption{
 		application.WithRole(manager.ManagerRoleAgent),
 		application.WithMode(managerMode),
+		application.WithDeletionTracker(a.deletions),
 	}
 
 	if a.options.metricsPort > 0 {
@@ -313,8 +320,6 @@ func NewAgent(ctx context.Context, client *kube.KubernetesClient, namespace stri
 	}
 	a.clusterCache = clusterCache
 
-	a.sourceCache = cache.NewSourceCache()
-
 	return a, nil
 }
 

agent/inbound.go

@@ -175,6 +175,9 @@ func (a *Agent) processIncomingApplication(ev *event.Event) error {
 			logCtx.Errorf("Error updating application: %v", err)
 		}
 	case event.Delete:
+		if a.mode == types.AgentModeManaged {
+			a.deletions.MarkExpected(incomingApp.UID)
+		}
 		err = a.deleteApplication(incomingApp)
 		if err != nil {
 			logCtx.Errorf("Error deleting application: %v", err)
@@ -255,6 +258,9 @@ func (a *Agent) processIncomingAppProject(ev *event.Event) error {
 			logCtx.Errorf("Error updating appproject: %v", err)
 		}
 	case event.Delete:
+		if a.mode == types.AgentModeManaged {
+			a.deletions.MarkExpected(incomingAppProject.UID)
+		}
 		err = a.deleteAppProject(incomingAppProject)
 		if err != nil {
 			logCtx.Errorf("Error deleting appproject: %v", err)
@@ -337,6 +343,9 @@ func (a *Agent) processIncomingRepository(ev *event.Event) error {
 		}
 
 	case event.Delete:
+		if a.mode == types.AgentModeManaged {
+			a.deletions.MarkExpected(incomingRepo.UID)
+		}
 		err = a.deleteRepository(incomingRepo)
 		if err != nil {
 			logCtx.Errorf("Error deleting repository: %v", err)
@@ -528,24 +537,23 @@ func (a *Agent) deleteApplication(app *v1alpha1.Application) error {
 
 	logCtx.Infof("Deleting application")
 
-	if a.mode == types.AgentModeManaged {
-		a.sourceCache.Application.Delete(app.UID)
-	}
-
 	deletionPropagation := backend.DeletePropagationBackground
 	err := a.appManager.Delete(a.context, a.namespace, app, &deletionPropagation)
 	if err != nil {
 		if apierrors.IsNotFound(err) {
 			logCtx.Debug("application is not found, perhaps it is already deleted")
+			if a.mode == types.AgentModeManaged {
+				a.sourceCache.Application.Delete(app.UID)
+			}
 			return nil
 		}
-		// Restore the cache if the deletion fails
-		if a.mode == types.AgentModeManaged {
-			a.sourceCache.Application.Set(app.UID, app.Spec)
-		}
 		return err
 	}
 
+	if a.mode == types.AgentModeManaged {
+		a.sourceCache.Application.Delete(app.UID)
+	}
+
 	err = a.appManager.Unmanage(app.QualifiedName())
 	if err != nil {
 		log().Warnf("Could not unmanage app %s: %v", app.QualifiedName(), err)
@@ -636,8 +644,6 @@ func (a *Agent) deleteAppProject(project *v1alpha1.AppProject) error {
 
 	logCtx.Infof("Deleting appProject")
 
-	a.sourceCache.AppProject.Delete(project.UID)
-
 	deletionPropagation := backend.DeletePropagationBackground
 	err := a.projectManager.Delete(a.context, project, &deletionPropagation)
 	if err != nil {
@@ -646,11 +652,11 @@ func (a *Agent) deleteAppProject(project *v1alpha1.AppProject) error {
 			a.sourceCache.AppProject.Delete(project.UID)
 			return nil
 		}
-		// Restore the cache if the deletion fails
-		a.sourceCache.AppProject.Set(project.UID, project.Spec)
 		return err
 	}
 
+	a.sourceCache.AppProject.Delete(project.UID)
+
 	err = a.projectManager.Unmanage(project.Name)
 	if err != nil {
 		log().Warnf("Could not unmanage appProject %s: %v", project.Name, err)
@@ -739,20 +745,19 @@ func (a *Agent) deleteRepository(repo *corev1.Secret) error {
 
 	logCtx.Infof("Deleting repository")
 
-	a.sourceCache.Repository.Delete(repo.UID)
-
 	deletionPropagation := backend.DeletePropagationBackground
 	err := a.repoManager.Delete(a.context, repo.Name, repo.Namespace, &deletionPropagation)
 	if err != nil {
 		if apierrors.IsNotFound(err) {
 			logCtx.Debug("repository is not found, perhaps it is already deleted")
+			a.sourceCache.Repository.Delete(repo.UID)
 			return nil
 		}
-		// Restore the cache if the deletion fails
-		a.sourceCache.Repository.Set(repo.UID, repo.Data)
 		return err
 	}
 
+	a.sourceCache.Repository.Delete(repo.UID)
+
 	err = a.repoManager.Unmanage(repo.Name)
 	if err != nil {
 		log().Warnf("Could not unmanage repository %s: %v", repo.Name, err)

agent/outbound.go

@@ -119,7 +119,7 @@ func (a *Agent) addAppDeletionToQueue(app *v1alpha1.Application) {
 	logCtx.Debugf("Delete app event")
 
 	if isResourceFromPrincipal(app) {
-		if manager.RevertUserInitiatedDeletion(a.context, app, a.sourceCache.Application, a.appManager, logCtx) {
+		if manager.RevertUserInitiatedDeletion(a.context, app, a.deletions, a.appManager, logCtx) {
 			logCtx.Trace("Deleted app is recreated")
 			return
 		}
@@ -258,7 +258,7 @@ func (a *Agent) addAppProjectDeletionToQueue(appProject *v1alpha1.AppProject) {
 	logCtx.Debugf("Delete appProject event")
 
 	if isResourceFromPrincipal(appProject) {
-		if manager.RevertUserInitiatedDeletion(a.context, appProject, a.sourceCache.AppProject, a.projectManager, logCtx) {
+		if manager.RevertUserInitiatedDeletion(a.context, appProject, a.deletions, a.projectManager, logCtx) {
 			logCtx.Trace("Deleted appProject is recreated")
 			return
 		}
@@ -392,7 +392,7 @@ func (a *Agent) handleRepositoryDeletion(repo *corev1.Secret) {
 
 	logCtx.Debugf("Delete repository event")
 
-	if manager.RevertUserInitiatedDeletion(a.context, repo, a.sourceCache.Repository, a.repoManager, logCtx) {
+	if manager.RevertUserInitiatedDeletion(a.context, repo, a.deletions, a.repoManager, logCtx) {
 		logCtx.Trace("Deleted repository is recreated")
 		return
 	}

internal/manager/application/application.go

@@ -58,6 +58,8 @@ type ApplicationManager struct {
 	manager.ManagedResources
 	// ObservedResources, key is qualified name of the application, value is the Application's .metadata.resourceValue field
 	manager.ObservedResources
+	// deletions tracks valid deletions from the source.
+	deletions *manager.DeletionTracker
 }
 
 // ApplicationManagerOption is a callback function to set an option to the Application
@@ -85,6 +87,13 @@ func WithMode(mode manager.ManagerMode) ApplicationManagerOption {
 	}
 }
 
+// WithDeletionTracker is used to track valid deletions from the source.
+func WithDeletionTracker(d *manager.DeletionTracker) ApplicationManagerOption {
+	return func(m *ApplicationManager) {
+		m.deletions = d
+	}
+}
+
 // NewApplicationManager initializes and returns a new Manager with the given backend and
 // options.
 func NewApplicationManager(be backend.Application, namespace string, opts ...ApplicationManagerOption) (*ApplicationManager, error) {
@@ -258,6 +267,13 @@ func (m *ApplicationManager) UpdateManagedApp(ctx context.Context, incoming *v1a
 
 	if deletionTimestampChanged {
 		logCtx.Infof("deletionTimestamp of managed agent changed from nil to non-nil, so deleting Application")
+		// Mark this as a valid deletion so the callback does not treat it as a user-initiated deletion.
+		if m.deletions != nil {
+			if v, ok := updated.Annotations[manager.SourceUIDAnnotation]; ok {
+				m.deletions.MarkExpected(ty.UID(v))
+			}
+		}
+
 		if err := m.applicationBackend.Delete(ctx, incoming.Name, incoming.Namespace, ptr.To(backend.DeletePropagationForeground)); err != nil {
 			return nil, err
 		}

internal/manager/manager.go

@@ -211,7 +211,7 @@ type resourceManager[R kubeResource] interface {
 // Returns true if the resource was recreated, false otherwise.
 func RevertUserInitiatedDeletion[R kubeResource](ctx context.Context,
 	outbound R,
-	resCache resourceCache[R],
+	deletions *DeletionTracker,
 	mgr resourceManager[R],
 	logCtx *logrus.Entry,
 ) bool {
@@ -227,9 +227,8 @@ func RevertUserInitiatedDeletion[R kubeResource](ctx context.Context,
 		return false
 	}
 
-	// If the resource is not in the cache, it means it was deleted by an incoming delete event.
-	// So no need to recreate it.
-	if !resCache.Contains(types.UID(sourceUID)) {
+	// Check if this deletion is coming from the source
+	if deletions.IsExpected(types.UID(sourceUID)) {
 		logCtx.Debugf("Expected deletion detected - allowing it to proceed")
 		return false
 	}
@@ -249,3 +248,37 @@ func RevertUserInitiatedDeletion[R kubeResource](ctx context.Context,
 
 	return true
 }
+
+// DeletionTracker tracks expected deletions from the source.
+type DeletionTracker struct {
+	mu       sync.RWMutex
+	expected map[types.UID]bool
+}
+
+func NewDeletionTracker() *DeletionTracker {
+	return &DeletionTracker{
+		expected: make(map[types.UID]bool),
+	}
+}
+
+func (d *DeletionTracker) MarkExpected(uid types.UID) {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+	d.expected[uid] = true
+}
+
+func (d *DeletionTracker) IsExpected(uid types.UID) bool {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+	_, exists := d.expected[uid]
+	if exists {
+		delete(d.expected, uid)
+	}
+	return exists
+}
+
+func (d *DeletionTracker) Unmark(uid types.UID) {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+	delete(d.expected, uid)
+}

internal/manager/manager_test.go

@@ -18,7 +18,6 @@ import (
 	"context"
 	"testing"
 
-	"github.com/argoproj-labs/argocd-agent/internal/cache"
 	argoapp "github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
@@ -79,23 +78,23 @@ func TestRevertUserInitiatedDeletion(t *testing.T) {
 			},
 		}
 		mgr := &fakeManager[*argoapp.Application]{}
-		sc := cache.NewSourceCache()
-		ok := RevertUserInitiatedDeletion(context.Background(), app, sc.Application, mgr, newLogger())
+		deletions := NewDeletionTracker()
+		ok := RevertUserInitiatedDeletion(context.Background(), app, deletions, mgr, newLogger())
 		requires.False(ok)
 		requires.Nil(mgr.created)
 
-		// With annotation but cache miss -> no recreate
+		// With annotation but valid deletion -> no recreate
 		app.Annotations = map[string]string{SourceUIDAnnotation: string(types.UID("u1"))}
 		mgr = &fakeManager[*argoapp.Application]{}
-		ok = RevertUserInitiatedDeletion(context.Background(), app, sc.Application, mgr, newLogger())
+		deletions.MarkExpected(types.UID("u1"))
+		ok = RevertUserInitiatedDeletion(context.Background(), app, deletions, mgr, newLogger())
 		requires.False(ok)
 		requires.Nil(mgr.created)
 
-		// With annotation and cache hit -> recreate
+		// With annotation and invalid deletion -> recreate
 		app.Annotations = map[string]string{SourceUIDAnnotation: string(types.UID("u2"))}
-		sc.Application.Set(types.UID("u2"), argoapp.ApplicationSpec{})
 		mgr = &fakeManager[*argoapp.Application]{}
-		ok = RevertUserInitiatedDeletion(context.Background(), app, sc.Application, mgr, newLogger())
+		ok = RevertUserInitiatedDeletion(context.Background(), app, deletions, mgr, newLogger())
 		requires.True(ok)
 		requires.NotNil(mgr.created)
 		requires.Equal(types.UID("u2"), mgr.created.GetUID())
@@ -112,22 +111,22 @@ func TestRevertUserInitiatedDeletion(t *testing.T) {
 				Namespace: "argocd",
 			},
 		}
-		sc := cache.NewSourceCache()
+		deletions := NewDeletionTracker()
 		mgr := &fakeManager[*argoapp.AppProject]{}
-		ok := RevertUserInitiatedDeletion(context.Background(), proj, sc.AppProject, mgr, newLogger())
+		ok := RevertUserInitiatedDeletion(context.Background(), proj, deletions, mgr, newLogger())
 		requires.False(ok)
 		requires.Nil(mgr.created)
 
 		proj.Annotations = map[string]string{SourceUIDAnnotation: string(types.UID("p1"))}
 		mgr = &fakeManager[*argoapp.AppProject]{}
-		ok = RevertUserInitiatedDeletion(context.Background(), proj, sc.AppProject, mgr, newLogger())
+		deletions.MarkExpected(types.UID("p1"))
+		ok = RevertUserInitiatedDeletion(context.Background(), proj, deletions, mgr, newLogger())
 		requires.False(ok)
 		requires.Nil(mgr.created)
 
 		proj.Annotations = map[string]string{SourceUIDAnnotation: string(types.UID("p2"))}
-		sc.AppProject.Set(types.UID("p2"), argoapp.AppProjectSpec{})
 		mgr = &fakeManager[*argoapp.AppProject]{}
-		ok = RevertUserInitiatedDeletion(context.Background(), proj, sc.AppProject, mgr, newLogger())
+		ok = RevertUserInitiatedDeletion(context.Background(), proj, deletions, mgr, newLogger())
 		requires.True(ok)
 		requires.NotNil(mgr.created)
 		requires.Equal(types.UID("p2"), mgr.created.GetUID())
@@ -144,22 +143,22 @@ func TestRevertUserInitiatedDeletion(t *testing.T) {
 				Namespace: "argocd",
 			},
 		}
-		sc := cache.NewSourceCache()
+		deletions := NewDeletionTracker()
 		mgr := &fakeManager[*corev1.Secret]{}
-		ok := RevertUserInitiatedDeletion(context.Background(), repo, sc.Repository, mgr, newLogger())
+		ok := RevertUserInitiatedDeletion(context.Background(), repo, deletions, mgr, newLogger())
 		requires.False(ok)
 		requires.Nil(mgr.created)
 
 		repo.Annotations = map[string]string{SourceUIDAnnotation: string(types.UID("r1"))}
 		mgr = &fakeManager[*corev1.Secret]{}
-		ok = RevertUserInitiatedDeletion(context.Background(), repo, sc.Repository, mgr, newLogger())
+		deletions.MarkExpected(types.UID("r1"))
+		ok = RevertUserInitiatedDeletion(context.Background(), repo, deletions, mgr, newLogger())
 		requires.False(ok)
 		requires.Nil(mgr.created)
 
 		repo.Annotations = map[string]string{SourceUIDAnnotation: string(types.UID("r2"))}
-		sc.Repository.Set(types.UID("r2"), map[string][]byte{"k": {}})
 		mgr = &fakeManager[*corev1.Secret]{}
-		ok = RevertUserInitiatedDeletion(context.Background(), repo, sc.Repository, mgr, newLogger())
+		ok = RevertUserInitiatedDeletion(context.Background(), repo, deletions, mgr, newLogger())
 		requires.True(ok)
 		requires.NotNil(mgr.created)
 		requires.Equal(types.UID("r2"), mgr.created.GetUID())

principal/callbacks.go

@@ -125,7 +125,7 @@ func (s *Server) deleteAppCallback(outbound *v1alpha1.Application) {
 
 	// Revert user-initiated deletion on autonomous agent applications
 	if isResourceFromAutonomousAgent(outbound) {
-		if manager.RevertUserInitiatedDeletion(s.ctx, outbound, s.sourceCache.Application, s.appManager, logCtx) {
+		if manager.RevertUserInitiatedDeletion(s.ctx, outbound, s.deletions, s.appManager, logCtx) {
 			logCtx.Trace("Deleted application is recreated")
 			return
 		}
@@ -264,7 +264,7 @@ func (s *Server) deleteAppProjectCallback(outbound *v1alpha1.AppProject) {
 
 	// Revert user-initiated deletion on autonomous agent applications
 	if isResourceFromAutonomousAgent(outbound) {
-		if manager.RevertUserInitiatedDeletion(s.ctx, outbound, s.sourceCache.AppProject, s.projectManager, logCtx) {
+		if manager.RevertUserInitiatedDeletion(s.ctx, outbound, s.deletions, s.projectManager, logCtx) {
 			logCtx.Trace("Deleted appProject is recreated")
 			return
 		}