Fix VcnConfig garbage collection

The garbageCollectAndWriteVcnConfigsLocked is for removing
configs whose subgroups do not have any subscriptions. This
patch fixes the issue of it in handling race condition when
subscription changes.

Previously the garbage collection checks a subGroup's
subscriptions based on SubscriptionManager. When
SubscriptionManager and TelephonySubscriptionSnapshot are
out-of-synced, the config of an active VCN instance will be
removed. It can cause NPE in getUnderlyingNetworkPolicy. This
patch fixes it by checking subscriptions against the snapshot.

This patch also fixes the safety check in
getUnderlyingNetworkPolicy so that if VCN instance exists but
its config does not, it will not attempt querying data from
the VcnConfig

Bug: 370862489
Test: FrameworksVcnTests(new tests); CtsVcnTestCases
Flag: android.net.vcn.fix_config_garbage_collection
Change-Id: Id54adabcc1910102a2ab20c432b693a8f52c5c95

4 files changed, 77 insertions, 12 deletions

diff --git a/core/java/android/net/vcn/flags.aconfig b/core/java/android/net/vcn/flags.aconfig
index 5b306243fc36..3d4401b1c71c 100644
--- a/core/java/android/net/vcn/flags.aconfig
+++ b/core/java/android/net/vcn/flags.aconfig
@@ -14,4 +14,14 @@ flag {
     namespace: "vcn"
     description: "Feature flag for adjustable safe mode timeout"
     bug: "317406085"
+}
+
+flag {
+    name: "fix_config_garbage_collection"
+    namespace: "vcn"
+    description: "Handle race condition in subscription change"
+    bug: "370862489"
+    metadata {
+      purpose: PURPOSE_BUGFIX
+    }
 }
\ No newline at end of file
diff --git a/services/core/java/com/android/server/VcnManagementService.java b/services/core/java/com/android/server/VcnManagementService.java
index 51c768b80eff..dc7749cb02ef 100644
--- a/services/core/java/com/android/server/VcnManagementService.java
+++ b/services/core/java/com/android/server/VcnManagementService.java
@@ -48,6 +48,7 @@ import android.net.LinkProperties;
 import android.net.Network;
 import android.net.NetworkCapabilities;
 import android.net.NetworkRequest;
+import android.net.vcn.Flags;
 import android.net.vcn.IVcnManagementService;
 import android.net.vcn.IVcnStatusCallback;
 import android.net.vcn.IVcnUnderlyingNetworkPolicyListener;
@@ -878,6 +879,7 @@ public class VcnManagementService extends IVcnManagementService.Stub {
 
     private void garbageCollectAndWriteVcnConfigsLocked() {
         final SubscriptionManager subMgr = mContext.getSystemService(SubscriptionManager.class);
+        final Set<ParcelUuid> subGroups = mLastSnapshot.getAllSubscriptionGroups();
 
         boolean shouldWrite = false;
 
@@ -885,11 +887,20 @@ public class VcnManagementService extends IVcnManagementService.Stub {
         while (configsIterator.hasNext()) {
             final ParcelUuid subGrp = configsIterator.next();
 
-            final List<SubscriptionInfo> subscriptions = subMgr.getSubscriptionsInGroup(subGrp);
-            if (subscriptions == null || subscriptions.isEmpty()) {
-                // Trim subGrps with no more subscriptions; must have moved to another subGrp
-                configsIterator.remove();
-                shouldWrite = true;
+            if (Flags.fixConfigGarbageCollection()) {
+                if (!subGroups.contains(subGrp)) {
+                    // Trim subGrps with no more subscriptions; must have moved to another subGrp
+                    logDbg("Garbage collect VcnConfig for group=" + subGrp);
+                    configsIterator.remove();
+                    shouldWrite = true;
+                }
+            } else {
+                final List<SubscriptionInfo> subscriptions = subMgr.getSubscriptionsInGroup(subGrp);
+                if (subscriptions == null || subscriptions.isEmpty()) {
+                    // Trim subGrps with no more subscriptions; must have moved to another subGrp
+                    configsIterator.remove();
+                    shouldWrite = true;
+                }
             }
         }
 
@@ -1094,13 +1105,7 @@ public class VcnManagementService extends IVcnManagementService.Stub {
             synchronized (mLock) {
                 final Vcn vcn = mVcns.get(subGrp);
                 final VcnConfig vcnConfig = mConfigs.get(subGrp);
-                if (vcn != null) {
-                    if (vcnConfig == null) {
-                        // TODO: b/284381334 Investigate for the root cause of this issue
-                        // and handle it properly
-                        logWtf("Vcn instance exists but VcnConfig does not for " + subGrp);
-                    }
-
+                if (vcn != null && vcnConfig != null) {
                     if (vcn.getStatus() == VCN_STATUS_CODE_ACTIVE) {
                         isVcnManagedNetwork = true;
                     }
@@ -1120,6 +1125,8 @@ public class VcnManagementService extends IVcnManagementService.Stub {
                             }
                         }
                     }
+                } else if (vcn != null && vcnConfig == null) {
+                    logWtf("Vcn instance exists but VcnConfig does not for " + subGrp);
                 }
             }
 
diff --git a/services/core/java/com/android/server/vcn/TelephonySubscriptionTracker.java b/services/core/java/com/android/server/vcn/TelephonySubscriptionTracker.java
index baf84cf4aa8b..3392d039109a 100644
--- a/services/core/java/com/android/server/vcn/TelephonySubscriptionTracker.java
+++ b/services/core/java/com/android/server/vcn/TelephonySubscriptionTracker.java
@@ -436,6 +436,17 @@ public class TelephonySubscriptionTracker extends BroadcastReceiver {
             return mPrivilegedPackages.keySet();
         }
 
+        /** Returns all subscription groups */
+        @NonNull
+        public Set<ParcelUuid> getAllSubscriptionGroups() {
+            final Set<ParcelUuid> subGroups = new ArraySet<>();
+            for (SubscriptionInfo subInfo : mSubIdToInfoMap.values()) {
+                subGroups.add(subInfo.getGroupUuid());
+            }
+
+            return subGroups;
+        }
+
         /** Checks if the provided package is carrier privileged for the specified sub group. */
         public boolean packageHasPermissionsForSubscriptionGroup(
                 @NonNull ParcelUuid subGrp, @NonNull String packageName) {
diff --git a/tests/vcn/java/com/android/server/VcnManagementServiceTest.java b/tests/vcn/java/com/android/server/VcnManagementServiceTest.java
index 7e0bbc4b3e50..3828a71d7b28 100644
--- a/tests/vcn/java/com/android/server/VcnManagementServiceTest.java
+++ b/tests/vcn/java/com/android/server/VcnManagementServiceTest.java
@@ -70,6 +70,7 @@ import android.net.Network;
 import android.net.NetworkCapabilities;
 import android.net.NetworkRequest;
 import android.net.Uri;
+import android.net.vcn.Flags;
 import android.net.vcn.IVcnStatusCallback;
 import android.net.vcn.IVcnUnderlyingNetworkPolicyListener;
 import android.net.vcn.VcnConfig;
@@ -84,6 +85,7 @@ import android.os.Process;
 import android.os.UserHandle;
 import android.os.UserManager;
 import android.os.test.TestLooper;
+import android.platform.test.flag.junit.SetFlagsRule;
 import android.telephony.SubscriptionInfo;
 import android.telephony.SubscriptionManager;
 import android.telephony.TelephonyManager;
@@ -102,6 +104,7 @@ import com.android.server.vcn.util.PersistableBundleUtils;
 import com.android.server.vcn.util.PersistableBundleUtils.PersistableBundleWrapper;
 
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.ArgumentCaptor;
@@ -119,6 +122,8 @@ import java.util.UUID;
 @RunWith(AndroidJUnit4.class)
 @SmallTest
 public class VcnManagementServiceTest {
+    @Rule public final SetFlagsRule mSetFlagsRule = new SetFlagsRule();
+
     private static final String CONTEXT_ATTRIBUTION_TAG = "VCN";
     private static final String TEST_PACKAGE_NAME =
             VcnManagementServiceTest.class.getPackage().getName();
@@ -288,6 +293,8 @@ public class VcnManagementServiceTest {
         doReturn(Collections.singleton(TRANSPORT_WIFI))
                 .when(mMockDeps)
                 .getRestrictedTransports(any(), any(), any());
+
+        mSetFlagsRule.enableFlags(Flags.FLAG_FIX_CONFIG_GARBAGE_COLLECTION);
     }
 
 
@@ -438,6 +445,14 @@ public class VcnManagementServiceTest {
             return subIds;
         }).when(snapshot).getAllSubIdsInGroup(any());
 
+        doAnswer(invocation -> {
+            final Set<ParcelUuid> subGroups = new ArraySet<>();
+            for (Entry<Integer, ParcelUuid> entry : subIdToGroupMap.entrySet()) {
+                subGroups.add(entry.getValue());
+            }
+            return subGroups;
+        }).when(snapshot).getAllSubscriptionGroups();
+
         return snapshot;
     }
 
@@ -1483,6 +1498,28 @@ public class VcnManagementServiceTest {
     }
 
     @Test
+    public void testGarbageCollectionKeepConfigUntilNewSnapshot() throws Exception {
+        setupActiveSubscription(TEST_UUID_2);
+        startAndGetVcnInstance(TEST_UUID_2);
+
+        // Report loss of subscription from mSubMgr
+        doReturn(Collections.emptyList()).when(mSubMgr).getSubscriptionsInGroup(any());
+        triggerSubscriptionTrackerCbAndGetSnapshot(
+                TEST_UUID_2,
+                Collections.singleton(TEST_UUID_2),
+                Collections.singletonMap(TEST_SUBSCRIPTION_ID, TEST_UUID_2));
+
+        assertTrue(mVcnMgmtSvc.getConfigs().containsKey(TEST_UUID_2));
+
+        // Report loss of subscription from snapshot
+        triggerSubscriptionTrackerCbAndGetSnapshot(null, Collections.emptySet());
+
+        mTestLooper.moveTimeForward(VcnManagementService.CARRIER_PRIVILEGES_LOST_TEARDOWN_DELAY_MS);
+        mTestLooper.dispatchAll();
+        assertFalse(mVcnMgmtSvc.getConfigs().containsKey(TEST_UUID_2));
+    }
+
+    @Test
     public void testVcnCarrierConfigChangeUpdatesPolicyListener() throws Exception {
         setupActiveSubscription(TEST_UUID_2);