xds/resolver: drop field references on Close to prevent retention across ClientConn recycle

[sushanb]

Summary

When callers repeatedly Close() a ClientConn dialed with an xds:/// target and immediately redial a fresh one (as Bigtable's ConnectionRecycler does every few minutes), the closed *xdsResolver instances are pinned in memory across recycles. Over hours this accumulates significant heap. On grpc-go v1.81.1 baseline, an 8-hour run of the recycle reproduction (50 DirectPath channels, 10s recycle interval) grew retained heap from ~90 MiB → 6.3 GiB.

Full reproduction: https://github.com/sushanb/bigtable-recycle-repro

Root cause

Retention is via multiple field references off *xdsResolver that survive Close:

• r.cc → the ClientConn, whose ServiceConfig still transitively references the resolver
• r.curConfigSelector → captured by the ClientConn's ServiceConfig via UpdateState
• r.dm / r.xdsClient → watcher metadata held by the shared xdsclient.DefaultPool singleton, which survives every ClientConn lifetime

Individually each field is a benign back-reference; together they form multiple independent retention chains through externally-anchored state. Go's tracing GC handles cycles fine, but these aren't cycles — they're chains rooted at package-global state (the shared xdsClient pool) and other ClientConns that stay alive.

Explicitly dropping the field references at the end of Close leaves the resolver struct with no outbound edges. The entire per-channel resolver + CDS balancer subtree (roughly 40 CallbackSerializers, JSON-parsed service configs, attributes.Attributes chains, cloned []resolver.Address slices per channel) becomes GC-collectible.

Impact (measured)

Same repro, same workload, 8-hour A/B, 50 channels, 10s recycle interval, DirectPath xDS engaged:

Metric	Baseline (v1.81.1)	With fix	Reduction
HeapAlloc growth	+5487 MiB	+475 MiB	91%
HeapSys growth	+6243 MiB	+891 MiB	86%
HeapObjects growth	+61.5M	+5.9M	90%

Hourly heap snapshots and pprof -base composition diffs are in the linked repro.

Related but distinct

This is orthogonal to #9140 (activeClusters refcount on early stream failure). Both bugs exist in v1.83; both should ship. Applying #9140 alone does NOT fix this leak — verified with an 8-hour run against grpc-go master @ the #9140 merge commit 89d4d61e.

Test

Included internal/xds/balancer/cdsbalancer/e2e_test/close_redial_leak_test.go — reproduces the "shared xdsclient.Pool + close-and-redial one of many channels" pattern in-process:

• Uses NewXDSResolverWithPoolForTesting with a shared pool so every dial sub/unsubscribes against the same refcounted xdsClient (matches production wiring; NewXDSResolverWithConfigForTesting would build a fresh pool per iteration and hide the leak surface).
• One persistent ClientConn pins the pool refcount above zero across the run, mirroring the "N-1 other channels stay open" invariant from Bigtable.
• 1000 dial-and-close iterations after a warm-up window.
• Asserts per-iteration HeapInuse growth < 20 KiB.

Caveat: the synthetic in-process xDS server is much smaller than production DirectPath (a handful of clusters vs many, minimal endpoint set), so per-iter numbers are within GC noise regardless of whether the fix is applied. The test's primary purpose is a future-regression guard at 20 KiB/iter — any change that reintroduces a retention chain of the observed magnitude (~60 KB/iter in the standalone repro) will exceed that threshold. The primary evidence for the fix's magnitude is the standalone-repro A/B linked above.

Test plan

• go test -run "Test/CloseRedialDoesNotRetainXDSState" ./internal/xds/balancer/cdsbalancer/e2e_test/ passes with the fix
• Full ./internal/xds/resolver/... build clean
• 8h A/B on the standalone recycle reproduction — 86-91% heap-growth reduction
• Existing xds e2e tests (please run in CI)

RELEASE NOTES: None

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: sushanb / name: sushantsusan (92b62bc)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.57%. Comparing base (6d697e4) to head (163464c).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9301      +/-   ##
==========================================
+ Coverage   87.54%   87.57%   +0.03%     
==========================================
  Files         429      429              
  Lines       30622    30636      +14     
==========================================
+ Hits        26807    26830      +23     
+ Misses       3814     3806       -8     
+ Partials        1        0       -1     

Files with missing lines	Coverage Δ
clientconn.go	93.00% <100.00%> (+0.14%)	⬆️
internal/xds/resolver/xds_resolver.go	91.66% <100.00%> (+0.29%)	⬆️
internal/xds/xdsdepmgr/xds_dependency_manager.go	91.87% <100.00%> (-0.42%)	⬇️

... and 19 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[easwars]

@mbissa : Could you please prioritize this review. GCS is affected by this memory leak.

[eshitachandwani]

Hi @sushanb , thank you for your contribution and for opening this PR! Before we can move forward, could you please sign the CLA?

[sushanb]

I signed the cla. @eshitachandwani

[eshitachandwani]

For

r.cc → the ClientConn, whose ServiceConfig still transitively references the resolver

Can we instead try to free up the service config , maybe by doing something like cc.safeConfigSelector.UpdateConfigSelector(&defaultConfigSelector{nil}) in cc.Close() , This would work for other resolvers too, not just xds resolver. We will also need to free fields of xds resolver too.

[sushanb]

Thanks @eshitachandwani — good call, pushed as 79cc56c2. Added cc.safeConfigSelector.UpdateConfigSelector(&defaultConfigSelector{nil}) right after cc.resolverWrapper.close() in ClientConn.Close. Placing it after the resolver has stopped avoids racing with an UpdateState from the resolver; SafeConfigSelector already handles concurrent SelectConfig calls from in-flight RPCs, and defaultConfigSelector{nil} is the same idiom NewClient and the parse-error path (clientconn.go:191, :886) already use — so no NPE risk on the RPC path.

Kept the xdsResolver.Close field-nil'ing as you noted — the resolver struct itself is still pinned by the shared xdsclient.DefaultPool singleton (via watcher metadata that outlives any single ClientConn), so its outbound edges (r.cc, r.dm, r.xdsClient, r.activeClusters, r.activePlugins, etc.) still need to be dropped to break the retention chain into the CDS balancer subtree. The two changes together are what the 8-hour A/B measured.

Build + ./internal/xds/resolver/..., ./internal/xds/balancer/cdsbalancer/e2e_test/, and root ./ tests all pass locally.

[eshitachandwani]

LGTM modulo 2 comments. Adding @easwars as a second reviewer.

[eshitachandwani]

We dont need this cc.Connect. The client will connect itself on the first RPC.

[sushanb]

Dropped in efecd06 — you are right, the following EmptyCall with WaitForReady handles it.

[eshitachandwani]

I dont think we should include the issue in logs.

[sushanb]

Dropped the suffix in efecd06 — failure message is self-contained now.

[easwars]

Can we also nil out the fields in the dependency manager's Close method:

       m.watcher = nil
       m.xdsClient = nil

[easwars]

Our CI pipeline does not set the -test.short flag and I don't think we want this test to run for every CI run, which happens for every push on every PR. I can see two options here:

• Change the testing.yml file to set the -test.short flag
• Remove this test from OSS and move it to g3, and ensure this is a g3 only file

I prefer the latter. Let me know your thoughts.

[easwars]

Thoughts on this one? @sushanb

[sushanb]

Done in 80dac1c — added m.watcher = nil and m.xdsClient = nil at the end of DependencyManager.Close. Safe because every callback that touches those fields (onListenerUpdate, onRouteConfigUpdate, all cluster/endpoint/dns callbacks) already re-checks m.stopped under m.mu first, so no in-flight caller can dereference a nil after Close returns.

[easwars]

@sushanb : The tests are failing with a panic now.

[sushanb]

Fixed in 8b015fa.

Root cause: after 80dac1c nils out m.watcher, the unsubscribe closure returned by SubscribeToCluster — which cdsBalancer.Close invokes via sync.OnceFunc — still reached maybeSendUpdateLocked and dereferenced the nil watcher. That closure runs from balancer-tree teardown, which happens after resolverWrapper.close (and therefore after DependencyManager.Close).

Fix adds if m.stopped { return } at the top of unsubscribeFromCluster, matching the pattern every other post-Close callback in the file already follows.

Verified locally with -race: go test ./test/xds/... ./internal/xds/... -count=1 -race all pass.

[easwars]

LGTM, modulo where to place the test and how to run it.

[easwars]

Why do we need two calls to this?

[easwars]

Nit: Single line. See: go/go-style/guide#line-length

[easwars]

Nit: Single line for every call to t.Logf

[easwars]

Nit: Single line here too.