From 1cf1927d1a7024e353f239fe5b5d50cbb49a8bae Mon Sep 17 00:00:00 2001
From: Eric Anderson <ejona@google.com>
Date: Fri, 3 Jan 2025 12:34:47 -0800
Subject: [PATCH] xds: Preserve nonce when unsubscribing type

This fixes a regression introduced in 19c9b998.

b/374697875
---
 .../java/io/grpc/xds/client/ControlPlaneClient.java    | 10 +++++++---
 .../java/io/grpc/xds/GrpcXdsClientImplTestBase.java    |  5 ++---
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/xds/src/main/java/io/grpc/xds/client/ControlPlaneClient.java b/xds/src/main/java/io/grpc/xds/client/ControlPlaneClient.java
index 9c7d744816a..878f1686a89 100644
--- a/xds/src/main/java/io/grpc/xds/client/ControlPlaneClient.java
+++ b/xds/src/main/java/io/grpc/xds/client/ControlPlaneClient.java
@@ -167,9 +167,10 @@ void adjustResourceSubscription(XdsResourceType<?> resourceType) {
     resourceStore.startMissingResourceTimers(resources, resourceType);
 
     if (resources.isEmpty()) {
-      // The resource type no longer has subscribing resources; clean up references to it
+      // The resource type no longer has subscribing resources; clean up references to it, except
+      // for nonces. If the resource type becomes used again the control plane can ignore requests
+      // for old/missing nonces. Old type's nonces are dropped when the ADS stream is restarted.
       versions.remove(resourceType);
-      adsStream.respNonces.remove(resourceType);
     }
   }
 
@@ -313,7 +314,10 @@ private class AdsStream implements XdsTransportFactory.EventHandler<DiscoveryRes
     // Nonce in each response is echoed back in the following ACK/NACK request. It is
     // used for management server to identify which response the client is ACKing/NACking.
     // To avoid confusion, client-initiated requests will always use the nonce in
-    // most recently received responses of each resource type.
+    // most recently received responses of each resource type. Nonces are never deleted from the
+    // map; nonces are only discarded once the stream closes because xds_protocol says "the
+    // management server should not send a DiscoveryResponse for any DiscoveryRequest that has a
+    // stale nonce."
     private final Map<XdsResourceType<?>, String> respNonces = new HashMap<>();
     private final StreamingCall<DiscoveryRequest, DiscoveryResponse> call;
     private final MethodDescriptor<DiscoveryRequest, DiscoveryResponse> methodDescriptor =
diff --git a/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplTestBase.java b/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplTestBase.java
index 1b0c363d94f..9a80e3c36b6 100644
--- a/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplTestBase.java
+++ b/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplTestBase.java
@@ -2899,10 +2899,9 @@ public void edsCleanupNonceAfterUnsubscription() {
     verifySubscribedResourcesMetadataSizes(0, 0, 0, 0);
     call.verifyRequest(EDS, Arrays.asList(), VERSION_1, "0000", NODE);
 
-    // When re-subscribing, the version and nonce were properly forgotten, so the request is the
-    // same as the initial request
+    // When re-subscribing, the version was forgotten but not the nonce
     xdsClient.watchXdsResource(XdsEndpointResource.getInstance(), "A.1", edsResourceWatcher);
-    call.verifyRequest(EDS, "A.1", "", "", NODE, Mockito.timeout(2000).times(2));
+    call.verifyRequest(EDS, "A.1", "", "0000", NODE, Mockito.timeout(2000));
   }
 
   @Test