fix: stop leaking cached envoy snapshots

projects/gateway2/proxy_syncer/proxy_syncer.go

@@ -8,6 +8,7 @@ import (
 	"maps"
 	"reflect"
 	"slices"
+	"sync"
 	"time"
 
 	"go.uber.org/zap"
@@ -25,6 +26,8 @@ import (
 	"github.com/solo-io/gloo/projects/gloo/pkg/syncer/setup"
 	"github.com/solo-io/gloo/projects/gloo/pkg/xds"
 	rlkubev1a1 "github.com/solo-io/solo-apis/pkg/api/ratelimit.solo.io/v1alpha1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	"github.com/solo-io/solo-kit/pkg/api/v1/clients/common"
@@ -103,6 +106,14 @@ type ProxySyncer struct {
 	allowedGatewayClasses sets.Set[string]
 
 	waitForSync []cache.InformerSynced
+
+	// statusGCMu guards the prev*RouteKeys sets used to garbage-collect stale
+	// route status (#7086). syncRouteStatus currently runs on a single
+	// goroutine; the mutex keeps tracking safe if that ever changes.
+	statusGCMu        sync.Mutex
+	prevHTTPRouteKeys sets.Set[types.NamespacedName]
+	prevTCPRouteKeys  sets.Set[types.NamespacedName]
+	prevTLSRouteKeys  sets.Set[types.NamespacedName]
 }
 
 type GatewayInputChannels struct {
@@ -582,7 +593,10 @@ func (s *ProxySyncer) Start(ctx context.Context) error {
 	// caches are warm, now we can do registrations
 	s.statusReport.Register(func(o krt.Event[report]) {
 		if o.Event == controllers.EventDelete {
-			// TODO: handle garbage collection (see: https://github.com/solo-io/solo-projects/issues/7086)
+			// The merged status report is a Singleton, so a delete here means the
+			// whole report collection went away (shutdown); there is no per-object
+			// report to act on. Stale per-route status GC for routes that leave the
+			// report is handled in syncRouteStatus -> gcStaleRouteStatus (#7086).
 			return
 		}
 		latestReportQueue.Enqueue(o.Latest().ReportMap)
@@ -643,10 +657,17 @@ func (s *ProxySyncer) Start(ctx context.Context) error {
 				snapWrap := e.Latest()
 				s.proxyTranslator.syncXds(ctx, snapWrap.snap, snapWrap.proxyKey)
 			} else {
-				// key := e.Latest().proxyKey
-				// if _, err := s.proxyTranslator.xdsCache.GetSnapshot(key); err == nil {
-				// 	s.proxyTranslator.xdsCache.ClearSnapshot(e.Latest().proxyKey)
-				// }
+				// Evict the snapshot for a disconnected/obsolete unique client.
+				// Without this, the xds SnapshotCache grows unbounded under client
+				// churn (each new role+labels+namespace key leaves a snapshot behind
+				// forever). The delete event only fires once the last stream for this
+				// proxyKey is gone (uniqueClients is refcounted), so no connected envoy
+				// still needs it; if a client with the same key reconnects, the
+				// snapshot is rebuilt and re-set. See solo-projects#7086.
+				key := e.Latest().proxyKey
+				if _, err := s.proxyTranslator.xdsCache.GetSnapshot(key); err == nil {
+					s.proxyTranslator.xdsCache.ClearSnapshot(key)
+				}
 			}
 		}
 	}, true)
@@ -1018,6 +1039,120 @@ func (s *ProxySyncer) syncRouteStatus(ctx context.Context, rm reports.ReportMap)
 			logger.Errorw("all attempts failed at updating TLSRoute status", "error", err, "route", rnn)
 		}
 	}
+
+	// Garbage-collect status from routes that are no longer in the report
+	// (detached or deleted) but to which we previously wrote status. See #7086.
+	s.gcStaleRouteStatus(ctx, rm)
+}
+
+// gcStaleRouteStatus clears status this controller previously wrote to routes
+// that are no longer present in the report. Without this, stale conditions
+// (e.g. Accepted/ResolvedRefs) linger on a route forever once it detaches from
+// the gateway, since the normal sync path only iterates routes still in the
+// report. See https://github.com/solo-io/solo-projects/issues/7086.
+func (s *ProxySyncer) gcStaleRouteStatus(ctx context.Context, rm reports.ReportMap) {
+	s.statusGCMu.Lock()
+	defer s.statusGCMu.Unlock()
+
+	cl := s.mgr.GetClient()
+	s.prevHTTPRouteKeys = clearStaleRouteStatus(ctx, cl, s.controllerName, s.prevHTTPRouteKeys, sets.KeySet(rm.HTTPRoutes),
+		wellknown.HTTPRouteKind, func() client.Object { return new(gwv1.HTTPRoute) })
+	s.prevTCPRouteKeys = clearStaleRouteStatus(ctx, cl, s.controllerName, s.prevTCPRouteKeys, sets.KeySet(rm.TCPRoutes),
+		wellknown.TCPRouteKind, func() client.Object { return new(gwv1a2.TCPRoute) })
+	s.prevTLSRouteKeys = clearStaleRouteStatus(ctx, cl, s.controllerName, s.prevTLSRouteKeys, sets.KeySet(rm.TLSRoutes),
+		wellknown.TLSRouteKind, func() client.Object { return new(gwv1a2.TLSRoute) })
+}
+
+// Retry settings for stale-status GC writes; vars (not consts) so tests can
+// shrink the delay.
+var (
+	statusGCRetryAttempts uint = 5
+	statusGCRetryDelay         = 100 * time.Millisecond
+)
+
+// clearStaleRouteStatus removes controllerName's parent status from every route
+// in prev that is no longer in current. It returns the tracking set for the next
+// round: routes still in current, plus any stale route whose cleanup could not
+// be completed (e.g. a conflict that outlasts the in-line retry), so the next
+// sync retries it instead of silently leaking stale status.
+//
+// Note: tracking is in-memory, so a crash between a route leaving the report and
+// its GC drops that route from tracking; such a route's stale status is not
+// revisited until it is reconciled again. Full robustness would require listing
+// routes at startup and reclaiming ones bearing our controller's status.
+func clearStaleRouteStatus(
+	ctx context.Context,
+	cl client.Client,
+	controllerName string,
+	prev, current sets.Set[types.NamespacedName],
+	routeType string,
+	newObj func() client.Object,
+) sets.Set[types.NamespacedName] {
+	logger := contextutils.LoggerFrom(ctx)
+
+	// Start from the routes still in the report; re-add any we fail to clean.
+	next := current.Clone()
+
+	for rnn := range prev.Difference(current) {
+		err := retry.Do(func() error {
+			// Re-Get inside the retry so each attempt has a fresh resourceVersion;
+			// otherwise a conflict just repeats forever.
+			route := newObj()
+			if err := cl.Get(ctx, rnn, route); err != nil {
+				if apierrors.IsNotFound(err) {
+					return nil // route deleted; status went with it
+				}
+				return err
+			}
+			if !removeControllerRouteStatus(route, controllerName) {
+				return nil // we held no status on this route
+			}
+			return cl.Status().Update(ctx, route)
+		},
+			retry.Attempts(statusGCRetryAttempts),
+			retry.Delay(statusGCRetryDelay),
+			retry.DelayType(retry.BackOffDelay),
+			// Only conflicts are worth retrying in-line (the re-Get above clears
+			// them). Other errors fail fast and are retried on the next sync.
+			retry.RetryIf(apierrors.IsConflict),
+			retry.LastErrorOnly(true),
+		)
+		if err != nil {
+			logger.Errorw("failed to clear stale route status; will retry next sync",
+				"error", err, "type", routeType, "route", rnn)
+			next.Insert(rnn)
+			continue
+		}
+		logger.Debugf("cleared stale %s status from %s", routeType, rnn)
+	}
+	return next
+}
+
+// removeControllerRouteStatus removes RouteParentStatus entries owned by
+// controllerName. It returns true if the route's status was modified.
+func removeControllerRouteStatus(route client.Object, controllerName string) bool {
+	var rs *gwv1.RouteStatus
+	switch r := route.(type) {
+	case *gwv1.HTTPRoute:
+		rs = &r.Status.RouteStatus
+	case *gwv1a2.TCPRoute:
+		rs = &r.Status.RouteStatus
+	case *gwv1a2.TLSRoute:
+		rs = &r.Status.RouteStatus
+	default:
+		return false
+	}
+	kept := make([]gwv1.RouteParentStatus, 0, len(rs.Parents))
+	for _, p := range rs.Parents {
+		if string(p.ControllerName) != controllerName {
+			kept = append(kept, p)
+		}
+	}
+	if len(kept) == len(rs.Parents) {
+		return false
+	}
+	rs.Parents = kept
+	return true
 }
 
 // syncGatewayStatus will build and update status for all Gateways in a reportMap

projects/gateway2/proxy_syncer/status_gc_client_test.go

@@ -0,0 +1,145 @@
+package proxy_syncer
+
+import (
+	"context"
+	"errors"
+	"testing"
+	"time"
+
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/sets"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
+	gwv1 "sigs.k8s.io/gateway-api/apis/v1"
+
+	"github.com/solo-io/gloo/pkg/schemes"
+)
+
+const testController = "solo.io/gloo-gateway"
+
+func staleHTTPRoute() *gwv1.HTTPRoute {
+	return &gwv1.HTTPRoute{
+		ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "r"},
+		Status: gwv1.HTTPRouteStatus{RouteStatus: gwv1.RouteStatus{
+			Parents: []gwv1.RouteParentStatus{{
+				ControllerName: gwv1.GatewayController(testController),
+				ParentRef:      gwv1.ParentReference{Name: "gw"},
+			}},
+		}},
+	}
+}
+
+func httpRouteKey() types.NamespacedName {
+	return types.NamespacedName{Namespace: "ns", Name: "r"}
+}
+
+func newHTTPRouteObj() client.Object { return new(gwv1.HTTPRoute) }
+
+func conflictErr() error {
+	return apierrors.NewConflict(
+		schema.GroupResource{Group: gwv1.GroupName, Resource: "httproutes"},
+		"r", errors.New("the object has been modified"))
+}
+
+func init() {
+	// keep retries fast in tests
+	statusGCRetryDelay = time.Millisecond
+}
+
+// A transient conflict on the status write should be retried in-line (with a
+// fresh Get) and ultimately succeed, so the route is cleaned and dropped from
+// tracking.
+func TestClearStaleRouteStatus_TransientConflictThenSuccess(t *testing.T) {
+	var updateCalls int
+	cl := fake.NewClientBuilder().
+		WithScheme(schemes.GatewayScheme()).
+		WithObjects(staleHTTPRoute()).
+		WithStatusSubresource(&gwv1.HTTPRoute{}).
+		WithInterceptorFuncs(interceptor.Funcs{
+			SubResourceUpdate: func(ctx context.Context, c client.Client, sr string, obj client.Object, opts ...client.SubResourceUpdateOption) error {
+				updateCalls++
+				if updateCalls == 1 {
+					return conflictErr()
+				}
+				return c.SubResource(sr).Update(ctx, obj, opts...)
+			},
+		}).
+		Build()
+
+	prev := sets.New(httpRouteKey())
+	current := sets.New[types.NamespacedName]() // route left the report
+
+	next := clearStaleRouteStatus(context.Background(), cl, testController, prev, current,
+		"HTTPRoute", newHTTPRouteObj)
+
+	if next.Has(httpRouteKey()) {
+		t.Fatalf("route should have been cleaned and dropped from tracking, but it was retained")
+	}
+	if updateCalls < 2 {
+		t.Fatalf("expected the write to be retried after the conflict, got %d update calls", updateCalls)
+	}
+
+	got := &gwv1.HTTPRoute{}
+	if err := cl.Get(context.Background(), httpRouteKey(), got); err != nil {
+		t.Fatalf("get: %v", err)
+	}
+	if len(got.Status.Parents) != 0 {
+		t.Fatalf("expected our parent status to be cleared, got %d parents", len(got.Status.Parents))
+	}
+}
+
+// A persistent non-conflict error should fail fast (not retried in-line) and the
+// route should be RETAINED in tracking so the next sync retries it; the next
+// pass (no error) clears it.
+func TestClearStaleRouteStatus_PersistentErrorRetainsThenClears(t *testing.T) {
+	failWrites := true
+	var updateCalls int
+	cl := fake.NewClientBuilder().
+		WithScheme(schemes.GatewayScheme()).
+		WithObjects(staleHTTPRoute()).
+		WithStatusSubresource(&gwv1.HTTPRoute{}).
+		WithInterceptorFuncs(interceptor.Funcs{
+			SubResourceUpdate: func(ctx context.Context, c client.Client, sr string, obj client.Object, opts ...client.SubResourceUpdateOption) error {
+				updateCalls++
+				if failWrites {
+					return errors.New("status update boom") // non-conflict => not retried in-line
+				}
+				return c.SubResource(sr).Update(ctx, obj, opts...)
+			},
+		}).
+		Build()
+
+	prev := sets.New(httpRouteKey())
+	current := sets.New[types.NamespacedName]()
+
+	// First pass: write fails, route must be retained for retry next cycle.
+	next := clearStaleRouteStatus(context.Background(), cl, testController, prev, current,
+		"HTTPRoute", newHTTPRouteObj)
+	if !next.Has(httpRouteKey()) {
+		t.Fatalf("route should be retained in tracking after a failed cleanup")
+	}
+	if updateCalls != 1 {
+		t.Fatalf("non-conflict error must not be retried in-line; got %d update calls", updateCalls)
+	}
+
+	// Next sync cycle: writes succeed; the retained route should now be cleared
+	// and dropped from tracking.
+	failWrites = false
+	next2 := clearStaleRouteStatus(context.Background(), cl, testController, next, current,
+		"HTTPRoute", newHTTPRouteObj)
+	if next2.Has(httpRouteKey()) {
+		t.Fatalf("route should be cleaned and dropped from tracking on the retry cycle")
+	}
+
+	got := &gwv1.HTTPRoute{}
+	if err := cl.Get(context.Background(), httpRouteKey(), got); err != nil {
+		t.Fatalf("get: %v", err)
+	}
+	if len(got.Status.Parents) != 0 {
+		t.Fatalf("expected our parent status cleared on retry cycle, got %d parents", len(got.Status.Parents))
+	}
+}

projects/gateway2/proxy_syncer/status_gc_test.go

@@ -0,0 +1,59 @@
+package proxy_syncer
+
+import (
+	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	gwv1 "sigs.k8s.io/gateway-api/apis/v1"
+)
+
+func httpRouteWithParents(controllers ...string) *gwv1.HTTPRoute {
+	r := &gwv1.HTTPRoute{
+		ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "r"},
+	}
+	for _, c := range controllers {
+		r.Status.Parents = append(r.Status.Parents, gwv1.RouteParentStatus{
+			ControllerName: gwv1.GatewayController(c),
+			ParentRef:      gwv1.ParentReference{Name: "gw"},
+		})
+	}
+	return r
+}
+
+func TestRemoveControllerRouteStatus(t *testing.T) {
+	const me = "solo.io/gloo-gateway"
+	const other = "example.com/other-controller"
+
+	t.Run("removes only our parent status", func(t *testing.T) {
+		r := httpRouteWithParents(me, other)
+		if !removeControllerRouteStatus(r, me) {
+			t.Fatalf("expected status to be modified")
+		}
+		if len(r.Status.Parents) != 1 {
+			t.Fatalf("expected 1 parent left, got %d", len(r.Status.Parents))
+		}
+		if string(r.Status.Parents[0].ControllerName) != other {
+			t.Fatalf("expected the other controller's status to remain, got %q", r.Status.Parents[0].ControllerName)
+		}
+	})
+
+	t.Run("no-op when we own no status", func(t *testing.T) {
+		r := httpRouteWithParents(other)
+		if removeControllerRouteStatus(r, me) {
+			t.Fatalf("expected no modification when controller owns no parent status")
+		}
+		if len(r.Status.Parents) != 1 {
+			t.Fatalf("expected the other controller's status untouched")
+		}
+	})
+
+	t.Run("clears all when we own everything", func(t *testing.T) {
+		r := httpRouteWithParents(me, me)
+		if !removeControllerRouteStatus(r, me) {
+			t.Fatalf("expected status to be modified")
+		}
+		if len(r.Status.Parents) != 0 {
+			t.Fatalf("expected all parents removed, got %d", len(r.Status.Parents))
+		}
+	})
+}