Optimize token cache remove path and add filter-first-clone flight for filtered retrieval, Fixes AB#3570409 (#3074)

Fixes
[AB#3570409](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3570409)
`removeAccount()`/`removeCredential()` were triggering decrypt-all via
`keySet()` → `getAll()` just to check key existence. Adds a flight-gated
optimization for filtered retrieval and elapsed-time telemetry for
`deleteAccessTokensWithIntersectingScopes`.

## Changes

- **`removeAccount()` / `removeCredential()`**: Replace
`mSharedPreferencesFileManager.keySet().contains(cacheKey)` with O(1)
in-memory `containsKey()` on `mCachedAccountRecordsWithKeys` /
`mCachedCredentialsWithKeys`

- **Flight `ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE`**: When enabled,
`getAccountsFilteredBy()` / `getCredentialsFilteredBy()` filter directly
on uncloned in-memory references, then clone only matching items —
avoiding cloning records that will be filtered out. `getAccounts()` /
`getCredentials()` are unaffected and always return cloned lists.

- **Telemetry**: Add
`elapsed_time_cache_delete_access_tokens_with_intersecting_scopes` span
attribute in `MsalOAuth2TokenCache`; remove unused
`elapsed_time_save_account_shared_preferences`; add
`is_filter_then_clone_enabled` attribute in `getCredentials()`

- **Tests**: `KeySetTrackingStorage` wrapper asserts
`keySet()`/`getAll()` are never called during remove; flight on/off
tests verify clone-only-matching behavior for filtered methods

- **Changelog**: vNext `[PATCH]` entry `#3074`

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: siddhijain <30181294+siddhijain@users.noreply.github.com>

Author: 3 people

changelog.txt

@@ -1,5 +1,6 @@
 vNext
 ----------
+- [PATCH] Optimize AcquireTokenSilent save path: replace keySet() decrypt-all with in-memory map lookup in removeAccount()/removeCredential(), add telemetry for deleteAccessTokensWithIntersectingScopes, and remove unused elapsed_time_save_account_shared_preferences attribute (#3074)
 - [MINOR] Add DeviceRegistrationClientApplication as public API for OneAuth device registration with mandatory correlationId, DeviceState and DrsDiscoveryEndpoint enums (#3073)
 - [MINOR] Move device registration protocol types, domain types, controller, and packer from broker to common to enable OneAuth device registration support (#3066)
 - [MINOR] Upgrade compileSdkVersion to 36 and buildToolsVersion to 36.0.0 (#3065)

common/src/test/java/com/microsoft/identity/common/SharedPreferencesAccountCredentialCacheWithMemoryCacheTest.java

@@ -30,6 +30,7 @@
 import com.google.gson.JsonPrimitive;
 import com.microsoft.identity.common.components.AndroidPlatformComponentsFactory;
 import com.microsoft.identity.common.components.InMemoryStorageSupplier;
+import com.microsoft.identity.common.internal.mocks.MockCommonFlightsManager;
 import com.microsoft.identity.common.java.authscheme.BearerAuthenticationSchemeInternal;
 import com.microsoft.identity.common.java.cache.CacheKeyValueDelegate;
 import com.microsoft.identity.common.java.cache.SharedPreferencesAccountCredentialCacheWithMemoryCache;
@@ -40,20 +41,28 @@
 import com.microsoft.identity.common.java.dto.IdTokenRecord;
 import com.microsoft.identity.common.java.dto.PrimaryRefreshTokenRecord;
 import com.microsoft.identity.common.java.dto.RefreshTokenRecord;
+import com.microsoft.identity.common.java.flighting.CommonFlight;
+import com.microsoft.identity.common.java.flighting.CommonFlightsManager;
+import com.microsoft.identity.common.java.flighting.IFlightsProvider;
 import com.microsoft.identity.common.java.interfaces.INameValueStorage;
+import com.microsoft.identity.common.java.util.ported.Predicate;
 import com.microsoft.identity.common.shadows.ShadowAndroidSdkStorageEncryptionManager;
 
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.Mockito;
 import org.robolectric.RobolectricTestRunner;
 import org.robolectric.annotation.Config;
 
 import java.util.HashMap;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import static com.microsoft.identity.common.java.cache.CacheKeyValueDelegate.CACHE_VALUE_SEPARATOR;
 import static org.junit.Assert.assertEquals;
@@ -62,6 +71,7 @@
 import static org.junit.Assert.assertNotSame;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.when;
 
 @RunWith(RobolectricTestRunner.class)
 @Config(shadows = {ShadowAndroidSdkStorageEncryptionManager.class})
@@ -2515,4 +2525,322 @@ public void testReturnedAllCredentialsAreCloned() {
         creds1.get(0).setCachedAt("banana");
         assertNotEquals(creds1.get(0), creds2.get(0));
     }
+
+    // =====================================================================
+    // Flight-gated behavior tests for ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE
+    // =====================================================================
+
+    private void enableFilterThenCloneFlight() {
+        CommonFlightsManager.INSTANCE.resetFlightsManager();
+        final IFlightsProvider mockFlightsProvider = Mockito.mock(IFlightsProvider.class);
+        when(mockFlightsProvider.isFlightEnabled(CommonFlight.ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE))
+                .thenReturn(true);
+        final MockCommonFlightsManager mockFlightsManager = new MockCommonFlightsManager();
+        mockFlightsManager.setMockCommonFlightsProvider(mockFlightsProvider);
+        CommonFlightsManager.INSTANCE.initializeCommonFlightsManager(mockFlightsManager);
+    }
+
+    private void resetFlight() {
+        CommonFlightsManager.INSTANCE.resetFlightsManager();
+    }
+
+    @Test
+    public void getAccounts_flightDisabled_returnsMutableListOfClones() {
+        final AccountRecord account = buildDefaultAccountRecord();
+        mSharedPreferencesAccountCredentialCache.saveAccount(account);
+
+        final List<AccountRecord> accounts1 = mSharedPreferencesAccountCredentialCache.getAccounts();
+        assertEquals(1, accounts1.size());
+        final List<AccountRecord> accounts2 = mSharedPreferencesAccountCredentialCache.getAccounts();
+        assertEquals(1, accounts2.size());
+
+        // The list itself should be a new mutable list each time
+        assertNotSame(accounts1, accounts2);
+
+        // The returned objects should be clones, not the same references
+        assertNotSame(accounts1.get(0), accounts2.get(0));
+        assertEquals(accounts1.get(0), accounts2.get(0));
+
+        // Mutating a returned object should not affect subsequent retrievals
+        accounts1.get(0).setLocalAccountId("mutated");
+        final List<AccountRecord> accounts3 = mSharedPreferencesAccountCredentialCache.getAccounts();
+        assertNotEquals("mutated", accounts3.get(0).getLocalAccountId());
+
+        // The returned list should be mutable (no exception thrown)
+        accounts1.add(new AccountRecord());
+    }
+
+    @Test
+    public void getAccounts_flightEnabled_stillReturnsMutableListOfClones() {
+        enableFilterThenCloneFlight();
+        try {
+            final AccountRecord account = buildDefaultAccountRecord();
+            mSharedPreferencesAccountCredentialCache.saveAccount(account);
+
+            final List<AccountRecord> accounts1 = mSharedPreferencesAccountCredentialCache.getAccounts();
+            assertEquals(1, accounts1.size());
+            final List<AccountRecord> accounts2 = mSharedPreferencesAccountCredentialCache.getAccounts();
+            assertEquals(1, accounts2.size());
+
+            // Even with flight enabled, getAccounts() still returns clones
+            assertNotSame(accounts1.get(0), accounts2.get(0));
+            assertEquals(accounts1.get(0), accounts2.get(0));
+
+            // Mutating a returned object should not affect subsequent retrievals
+            accounts1.get(0).setLocalAccountId("mutated");
+            final List<AccountRecord> accounts3 = mSharedPreferencesAccountCredentialCache.getAccounts();
+            assertNotEquals("mutated", accounts3.get(0).getLocalAccountId());
+
+            // The returned list should be mutable (no exception thrown)
+            accounts1.add(new AccountRecord());
+        } finally {
+            resetFlight();
+        }
+    }
+
+    @Test
+    public void getCredentials_flightDisabled_returnsMutableListOfClones() {
+        final RefreshTokenRecord rt = buildDefaultRefreshToken();
+        mSharedPreferencesAccountCredentialCache.saveCredential(rt);
+
+        final List<Credential> creds1 = mSharedPreferencesAccountCredentialCache.getCredentials();
+        assertEquals(1, creds1.size());
+        final List<Credential> creds2 = mSharedPreferencesAccountCredentialCache.getCredentials();
+        assertEquals(1, creds2.size());
+
+        // The list itself should be a new mutable list each time
+        assertNotSame(creds1, creds2);
+
+        // The returned objects should be clones, not the same references
+        assertNotSame(creds1.get(0), creds2.get(0));
+        assertEquals(creds1.get(0), creds2.get(0));
+
+        // Mutating a returned object should not affect subsequent retrievals
+        creds1.get(0).setCachedAt("mutated");
+        final List<Credential> creds3 = mSharedPreferencesAccountCredentialCache.getCredentials();
+        assertNotEquals("mutated", creds3.get(0).getCachedAt());
+
+        // The returned list should be mutable (no exception thrown)
+        creds1.add(new RefreshTokenRecord());
+    }
+
+    @Test
+    public void getCredentials_flightEnabled_stillReturnsMutableListOfClones() {
+        enableFilterThenCloneFlight();
+        try {
+            final RefreshTokenRecord rt = buildDefaultRefreshToken();
+            mSharedPreferencesAccountCredentialCache.saveCredential(rt);
+
+            final List<Credential> creds1 = mSharedPreferencesAccountCredentialCache.getCredentials();
+            assertEquals(1, creds1.size());
+            final List<Credential> creds2 = mSharedPreferencesAccountCredentialCache.getCredentials();
+            assertEquals(1, creds2.size());
+
+            // Even with flight enabled, getCredentials() still returns clones
+            assertNotSame(creds1.get(0), creds2.get(0));
+            assertEquals(creds1.get(0), creds2.get(0));
+
+            // Mutating a returned object should not affect subsequent retrievals
+            creds1.get(0).setCachedAt("mutated");
+            final List<Credential> creds3 = mSharedPreferencesAccountCredentialCache.getCredentials();
+            assertNotEquals("mutated", creds3.get(0).getCachedAt());
+
+            // The returned list should be mutable (no exception thrown)
+            creds1.add(new RefreshTokenRecord());
+        } finally {
+            resetFlight();
+        }
+    }
+
+    // =====================================================================
+    // Tests verifying filter-then-clone optimization in FilteredBy methods
+    // =====================================================================
+
+    @Test
+    public void getAccountsFilteredBy_flightEnabled_returnsClonedMatches() {
+        enableFilterThenCloneFlight();
+        try {
+            final AccountRecord account = buildDefaultAccountRecord();
+            mSharedPreferencesAccountCredentialCache.saveAccount(account);
+
+            // Also save a non-matching account
+            final AccountRecord otherAccount = new AccountRecord();
+            otherAccount.setHomeAccountId("other-home-id");
+            otherAccount.setEnvironment("other.environment.com");
+            otherAccount.setRealm("other-realm");
+            otherAccount.setLocalAccountId("other-local-id");
+            otherAccount.setUsername("other-user");
+            otherAccount.setAuthorityType(AUTHORITY_TYPE);
+            mSharedPreferencesAccountCredentialCache.saveAccount(otherAccount);
+
+            // Filter by the default account's realm — should return only one match
+            final List<AccountRecord> filtered = mSharedPreferencesAccountCredentialCache
+                    .getAccountsFilteredBy(HOME_ACCOUNT_ID, ENVIRONMENT, REALM);
+            assertEquals(1, filtered.size());
+            assertEquals(HOME_ACCOUNT_ID, filtered.get(0).getHomeAccountId());
+
+            // Returned objects should be clones — mutating should not affect cache
+            filtered.get(0).setLocalAccountId("mutated");
+            final List<AccountRecord> filtered2 = mSharedPreferencesAccountCredentialCache
+                    .getAccountsFilteredBy(HOME_ACCOUNT_ID, ENVIRONMENT, REALM);
+            assertNotEquals("mutated", filtered2.get(0).getLocalAccountId());
+        } finally {
+            resetFlight();
+        }
+    }
+
+    @Test
+    public void getCredentialsFilteredBy_flightEnabled_returnsClonedMatches() {
+        enableFilterThenCloneFlight();
+        try {
+            final RefreshTokenRecord rt = buildDefaultRefreshToken();
+            mSharedPreferencesAccountCredentialCache.saveCredential(rt);
+
+            // Also save a non-matching credential
+            final RefreshTokenRecord otherRt = new RefreshTokenRecord();
+            otherRt.setCredentialType(CredentialType.RefreshToken.name());
+            otherRt.setHomeAccountId("other-home-id");
+            otherRt.setEnvironment("other.environment.com");
+            otherRt.setClientId("other-client-id");
+            otherRt.setCachedAt(CACHED_AT);
+            otherRt.setSecret(SECRET);
+            mSharedPreferencesAccountCredentialCache.saveCredential(otherRt);
+
+            // Filter by the default credential's attributes — should return only one match
+            final List<Credential> filtered = mSharedPreferencesAccountCredentialCache
+                    .getCredentialsFilteredBy(
+                            HOME_ACCOUNT_ID, ENVIRONMENT, CredentialType.RefreshToken,
+                            CLIENT_ID, null, null, null, null, null);
+            assertEquals(1, filtered.size());
+            assertEquals(CLIENT_ID, filtered.get(0).getClientId());
+
+            // Returned objects should be clones — mutating should not affect cache
+            filtered.get(0).setCachedAt("mutated");
+            final List<Credential> filtered2 = mSharedPreferencesAccountCredentialCache
+                    .getCredentialsFilteredBy(
+                            HOME_ACCOUNT_ID, ENVIRONMENT, CredentialType.RefreshToken,
+                            CLIENT_ID, null, null, null, null, null);
+            assertNotEquals("mutated", filtered2.get(0).getCachedAt());
+        } finally {
+            resetFlight();
+        }
+    }
+
+    // =====================================================================
+    // Tests verifying removeAccount/removeCredential do not call keySet()
+    // =====================================================================
+
+    /**
+     * An {@link INameValueStorage} wrapper that counts calls to {@link #keySet()} and {@link #getAll()}.
+     * Used to verify that the {@code removeAccount()} and {@code removeCredential()} optimization
+     * avoids the expensive decrypt-all path by not calling these methods on the backing storage.
+     */
+    private static class KeySetTrackingStorage implements INameValueStorage<String> {
+        private final INameValueStorage<String> mDelegate;
+        // AtomicInteger used because the cache performs its initial load on a background thread.
+        private final AtomicInteger mKeySetCallCount = new AtomicInteger(0);
+        private final AtomicInteger mGetAllCallCount = new AtomicInteger(0);
+
+        KeySetTrackingStorage(final INameValueStorage<String> delegate) {
+            mDelegate = delegate;
+        }
+
+        @Override
+        public String get(final String name) {
+            return mDelegate.get(name);
+        }
+
+        @Override
+        public Map<String, String> getAll() {
+            mGetAllCallCount.incrementAndGet();
+            return mDelegate.getAll();
+        }
+
+        @Override
+        public void put(final String name, final String value) {
+            mDelegate.put(name, value);
+        }
+
+        @Override
+        public void remove(final String name) {
+            mDelegate.remove(name);
+        }
+
+        @Override
+        public void clear() {
+            mDelegate.clear();
+        }
+
+        @Override
+        public Set<String> keySet() {
+            mKeySetCallCount.incrementAndGet();
+            return mDelegate.keySet();
+        }
+
+        @Override
+        public Iterator<Map.Entry<String, String>> getAllFilteredByKey(final Predicate<String> keyFilter) {
+            return mDelegate.getAllFilteredByKey(keyFilter);
+        }
+
+        int getKeySetCallCount() {
+            return mKeySetCallCount.get();
+        }
+
+        int getAllCallCount() {
+            return mGetAllCallCount.get();
+        }
+
+        void resetCallCounts() {
+            mKeySetCallCount.set(0);
+            mGetAllCallCount.set(0);
+        }
+    }
+
+    @Test
+    public void removeAccount_doesNotCallKeySetOrGetAll() throws Exception {
+        final INameValueStorage<String> baseStorage = new InMemoryStorageSupplier()
+                .getEncryptedNameValueStore(
+                        sAccountCredentialSharedPreferences + "_removeAccountTest",
+                        String.class);
+        final KeySetTrackingStorage trackingStorage = new KeySetTrackingStorage(baseStorage);
+        final SharedPreferencesAccountCredentialCacheWithMemoryCache cache =
+                new SharedPreferencesAccountCredentialCacheWithMemoryCache(mDelegate, trackingStorage);
+
+        final AccountRecord account = buildDefaultAccountRecord();
+        cache.saveAccount(account);
+
+        // Force initial load to complete (getAccounts blocks on waitForInitialLoad),
+        // then reset counters so only removeAccount() calls are measured.
+        cache.getAccounts();
+        trackingStorage.resetCallCounts();
+
+        cache.removeAccount(account);
+
+        assertEquals("removeAccount() must not call keySet()", 0, trackingStorage.getKeySetCallCount());
+        assertEquals("removeAccount() must not call getAll()", 0, trackingStorage.getAllCallCount());
+    }
+
+    @Test
+    public void removeCredential_doesNotCallKeySetOrGetAll() throws Exception {
+        final INameValueStorage<String> baseStorage = new InMemoryStorageSupplier()
+                .getEncryptedNameValueStore(
+                        sAccountCredentialSharedPreferences + "_removeCredentialTest",
+                        String.class);
+        final KeySetTrackingStorage trackingStorage = new KeySetTrackingStorage(baseStorage);
+        final SharedPreferencesAccountCredentialCacheWithMemoryCache cache =
+                new SharedPreferencesAccountCredentialCacheWithMemoryCache(mDelegate, trackingStorage);
+
+        final RefreshTokenRecord rt = buildDefaultRefreshToken();
+        cache.saveCredential(rt);
+
+        // Force initial load to complete (getCredentials blocks on waitForInitialLoad),
+        // then reset counters so only removeCredential() calls are measured.
+        cache.getCredentials();
+        trackingStorage.resetCallCounts();
+
+        cache.removeCredential(rt);
+
+        assertEquals("removeCredential() must not call keySet()", 0, trackingStorage.getKeySetCallCount());
+        assertEquals("removeCredential() must not call getAll()", 0, trackingStorage.getAllCallCount());
+    }
 }