MSDK 14.x dropped account-resolution caching — ~44x AccountManager IPC increase drives Android ColdStart/WarmBootstrap regression

libs/SalesforceSDK/src/com/salesforce/androidsdk/app/SalesforceSDKManager.kt

@@ -57,6 +57,7 @@ import android.view.WindowInsetsController.APPEARANCE_LIGHT_STATUS_BARS
 import android.webkit.CookieManager
 import android.webkit.URLUtil.isHttpsUrl
 import android.widget.Toast
+import androidx.annotation.CallSuper
 import androidx.annotation.VisibleForTesting
 import androidx.annotation.VisibleForTesting.Companion.PRIVATE
 import androidx.annotation.VisibleForTesting.Companion.PROTECTED
@@ -105,7 +106,6 @@ import com.salesforce.androidsdk.auth.AuthenticatorService.KEY_COOKIE_CLIENT_SRC
 import com.salesforce.androidsdk.auth.AuthenticatorService.KEY_COOKIE_SID_CLIENT
 import com.salesforce.androidsdk.auth.AuthenticatorService.KEY_CREDENTIALS_IDENTIFIER
 import com.salesforce.androidsdk.auth.AuthenticatorService.KEY_CSRF_TOKEN
-import com.salesforce.androidsdk.auth.AuthenticatorService.KEY_INSTANCE_URL
 import com.salesforce.androidsdk.auth.AuthenticatorService.KEY_LIGHTNING_SID
 import com.salesforce.androidsdk.auth.AuthenticatorService.KEY_ORG_ID
 import com.salesforce.androidsdk.auth.AuthenticatorService.KEY_PARENT_SID
@@ -1017,14 +1017,49 @@ open class SalesforceSDKManager protected constructor(
     /**
      * Clean up cached data.
      *
+     * Overrides must call `super.cleanUp(userAccount)` — this base
+     * implementation invalidates [cachedClientManager] and other
+     * account-scoped caches. Skipping the super call reintroduces stale
+     * cache reads for the removed/malformed account. [CallSuper] makes
+     * Android Lint enforce this at build time.
+     *
      * @param userAccount The user account
      */
+    @CallSuper
     protected open fun cleanUp(userAccount: UserAccount?) {
         SalesforceAnalyticsManager.reset(userAccount)
         RestClient.clearCaches(userAccount)
         UserAccountManager.getInstance().clearCachedCurrentUser()
 
+        /*
+         * The removed/malformed account may be the one clientManager has
+         * cached; drop it so the next access re-resolves rather than
+         * returning a manager bound to a now-invalid account.
+         *
+         * This is event-driven invalidation, not per-access re-validation:
+         * the cache is only checked for validity here, when an account is
+         * actually removed, not on every clientManager read. Re-validating
+         * on every access would reintroduce the AccountManager IPC cost
+         * this whole cache exists to avoid. Every account-removal path
+         * (logout, purgeMalformedPersistedAccount, and the corrupt-account
+         * path in getRestClient) funnels through this method, so it is a
+         * complete invalidation point for the cache's lifetime.
+         */
+        cachedClientManager = null
+
         userAccount?.let { userAccountResolved ->
+            /*
+             * Drops this user's persisted feature markers so a later login
+             * as the same identity starts from an empty set rather than
+             * inheriting the previous session's markers. Without this, the
+             * no-op guards in registerUsedAppFeature/unregisterUsedAppFeature
+             * (which only persist on an actual set change) can skip writing
+             * the new session's flags if they happen to match what's left
+             * over from the old one, leaving in-memory and persisted state
+             * inconsistent.
+             */
+            perUserFeatures.remove("${userAccountResolved.orgId}/${userAccountResolved.userId}")
+
             (screenLockManager as ScreenLockManager?)?.cleanUp(userAccountResolved)
             (biometricAuthenticationManager as BiometricAuthenticationManager)
                 .cleanUp(userAccountResolved)
@@ -1531,8 +1566,14 @@ open class SalesforceSDKManager protected constructor(
         if (user == null) { registerUsedAppFeature(appFeatureCode); return }
         val key = "${user.orgId}/${user.userId}"
         val set = perUserFeatures.getOrPut(key) { ConcurrentSkipListSet(CASE_INSENSITIVE_ORDER) }
-        set.add(appFeatureCode)
-        persistUserFeatureFlags(user, set)
+        /*
+         * add() returns false when the code is already present, so this skips
+         * the AccountManager round-trip in persistUserFeatureFlags on repeat
+         * calls.
+         */
+        if (set.add(appFeatureCode)) {
+            persistUserFeatureFlags(user, set)
+        }
     }
 
     /**
@@ -1544,8 +1585,15 @@ open class SalesforceSDKManager protected constructor(
     fun unregisterUsedAppFeature(appFeatureCode: String, user: UserAccount?) {
         if (user == null) { unregisterUsedAppFeature(appFeatureCode); return }
         val key = "${user.orgId}/${user.userId}"
-        perUserFeatures[key]?.remove(appFeatureCode)
-        persistUserFeatureFlags(user, perUserFeatures[key] ?: emptySet())
+        val set = perUserFeatures[key] ?: return
+        /*
+         * remove() returns false when the code was already absent, so this
+         * skips the AccountManager round-trip in persistUserFeatureFlags on
+         * repeat/no-op calls.
+         */
+        if (set.remove(appFeatureCode)) {
+            persistUserFeatureFlags(user, set)
+        }
     }
 
     private fun persistUserFeatureFlags(user: UserAccount, flags: Set<String>) {
@@ -1593,32 +1641,83 @@ open class SalesforceSDKManager protected constructor(
             }
         """.trimIndent()
 
+    /**
+     * Single-entry current-user cache for [clientManager], keyed by the
+     * current user's org/user ID pair.
+     *
+     * Holds at most one `(key, manager)` pair, not a per-account map: an A
+     * -> B -> A user-switch sequence does not retain A's manager across the
+     * trip through B, it replaces A with B and then constructs a new
+     * manager when A becomes current again. The tradeoff: an access
+     * pattern that rapidly alternates the current user gets no caching
+     * benefit (every access is a miss). Not a regression, since that
+     * pattern paid the full IPC cost before this cache existed too.
+     */
+    @Volatile
+    private var cachedClientManager: Pair<String, ClientManager>? = null
+
     /**
      * Returns a manager bound to the user who is current at the time of access,
      * or null when there is no current user. Retaining the returned manager
      * retains that user's identity even if the application later switches
      * users.
+     *
+     * The manager is cached per current-user identity to avoid re-resolving the
+     * AccountManager-backed account on every access; the cache is invalidated
+     * whenever the current user's identity changes.
+     *
+     * Uses [UserAccountManager.getCachedCurrentUser] rather than
+     * `getCurrentUser()` for the identity lookup: `getCurrentUser()`
+     * unconditionally re-resolves the current account via `AccountManager`
+     * on every call, which would reintroduce exactly the IPC this cache
+     * exists to avoid. `cachedCurrentUser` only cares about identity
+     * (org/user ID) to compute the cache key, so its possibly-stale OAuth
+     * fields are fine here; `storeCurrentUserInfo` invalidates this
+     * underlying cache on every user switch, so the identity itself is
+     * never stale across a switch.
      */
     val clientManager: ClientManager?
-        get() = userAccountManager.currentUser?.let { user ->
-            ClientManager(appContext, user)
-        }?.takeIf { manager -> manager.account != null }
+        get() {
+            val user = userAccountManager.cachedCurrentUser ?: return null
+            val key = "${user.orgId}/${user.userId}"
+            cachedClientManager?.let { (cachedKey, manager) ->
+                if (cachedKey == key) return manager
+            }
+            /*
+             * Check-then-act, not atomic: concurrent callers can both miss
+             * here and each construct their own ClientManager, with the
+             * last write to cachedClientManager winning. @Volatile only
+             * guarantees the write is visible to other threads, not that
+             * this read-then-write is exclusive. Accepted as benign — the
+             * losing manager is simply discarded, not left in an
+             * inconsistent state — rather than paying for a lock on this
+             * hot path.
+             */
+            val manager = ClientManager(appContext, user).takeIf { it.account != null } ?: return null
+            cachedClientManager = key to manager
+            return manager
+        }
 
     /**
      * Returns an authenticated client for the current user or starts login when
      * no persisted account is current. If the current account cannot produce a
      * valid user and client, removes that exact corrupt account and completes
      * the normal logout, account-switching, or login flow without invoking
      * [restClientCallback].
+     *
+     * Called from `SalesforceActivityDelegate.onResume()`, so this is on the
+     * Activity-resume hot path; it goes through [clientManager] rather than
+     * constructing a `ClientManager` directly so repeated resumes for the
+     * same current user reuse the cached instance instead of re-resolving
+     * the account via `AccountManager` on every resume.
      */
     fun getRestClient(
         activityContext: Activity,
         restClientCallback: RestClientCallback,
     ) {
         val account = userAccountManager.currentAccount
         if (account != null) {
-            val user = userAccountManager.buildUserAccount(account)
-            val client = user?.let { ClientManager(appContext, it).peekRestClient() }
+            val client = clientManager?.peekRestClient()
             if (client == null) {
                 w(TAG, "Removing a corrupt current account that cannot create a REST client")
                 logout(
@@ -1789,6 +1888,13 @@ open class SalesforceSDKManager protected constructor(
         user: UserAccount,
         restClient: RestClient? = null
     ): String = try {
+        /*
+         * Deliberately does not go through the clientManager cache: that
+         * cache is bound to userAccountManager.currentUser, but the debug
+         * action this backs lets a developer force-refresh a non-current
+         * user's token, so this must resolve fresh for the exact [user]
+         * passed in rather than reusing whichever account is cached.
+         */
         val resolvedClient = restClient ?: ClientManager(appContext, user).peekRestClient()
         if (resolvedClient == null) {
             "Token refresh failed: user is unavailable"

libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/app/SalesforceSDKManagerClientManagerTest.kt

@@ -111,6 +111,130 @@ class SalesforceSDKManagerClientManagerTest {
         assertManagerBoundTo(retainedManagerA, userA)
     }
 
+    @Test
+    fun clientManager_repeatedAccessForSameCurrentUser_returnsSameCachedInstance() {
+        /*
+         * Regression guard for the account-resolution caching fix: repeated
+         * access for an unchanged current user must not reconstruct
+         * ClientManager (and re-resolve its backing Account) on every call.
+         */
+        persistUser("cached")
+        val first = requireNotNull(sdkManager.clientManager)
+        val second = requireNotNull(sdkManager.clientManager)
+        val third = requireNotNull(sdkManager.clientManager)
+
+        assertTrue("Repeated access for the same current user must return the cached instance", first === second)
+        assertTrue("Repeated access for the same current user must return the cached instance", second === third)
+    }
+
+    @Test
+    fun clientManager_repeatedAccess_doesNotForceFreshCurrentUserResolution() {
+        /*
+         * Regression guard: clientManager's identity lookup must not
+         * unconditionally re-resolve the current user via AccountManager on
+         * every access. UserAccountManager.getCurrentUser() always
+         * allocates a new UserAccount by decrypting AccountManager-backed
+         * fields, so its cached reference changes identity every time it
+         * runs. Seeding that cache once and then confirming the reference
+         * is unchanged after repeated clientManager access proves those
+         * accesses used the cached identity lookup instead of re-invoking
+         * getCurrentUser().
+         */
+        persistUser("no-fresh-lookup")
+        val seededCurrentUser = requireNotNull(userAccountManager.currentUser)
+
+        requireNotNull(sdkManager.clientManager)
+        requireNotNull(sdkManager.clientManager)
+        requireNotNull(sdkManager.clientManager)
+
+        assertTrue(
+            "Repeated clientManager access must not force a fresh currentUser resolution",
+            seededCurrentUser === userAccountManager.cachedCurrentUser,
+        )
+    }
+
+    @Test
+    fun clientManager_afterCurrentUserSwitch_freshGetterReturnsNewInstanceBoundToNewUser() {
+        /*
+         * Regression guard: switching the current user must invalidate the
+         * cache so the next access resolves a client bound to the new user,
+         * not a stale cached instance from the old user.
+         */
+        val userA = persistUser("switch-a")
+        val managerForA = requireNotNull(sdkManager.clientManager)
+
+        val userB = persistUser("switch-b")
+        val managerForB = requireNotNull(sdkManager.clientManager)
+
+        assertTrue(
+            "Switching the current user must produce a differently-bound ClientManager instance",
+            managerForA !== managerForB,
+        )
+        assertManagerBoundTo(managerForA, userA)
+        assertManagerBoundTo(managerForB, userB)
+    }
+
+    @Test
+    fun clientManager_afterCachedAccountIsRemovedAndReAdded_returnsFreshlyBoundInstance() {
+        /*
+         * Regression guard: the cache must not survive removal of the
+         * account it is bound to. If the same identity is re-added
+         * afterward, the next access must resolve a fresh ClientManager
+         * bound to the new persisted Account, not the stale cached instance
+         * from before the removal.
+         */
+        val user = persistUser("removed-and-readded")
+        val staleManager = requireNotNull(sdkManager.clientManager)
+        val staleAccount = requireNotNull(staleManager.account)
+
+        sdkManager.logout(staleAccount, null, false)
+
+        userAccountManager.createAccount(user)
+        val freshManager = requireNotNull(sdkManager.clientManager)
+
+        assertTrue(
+            "A cache entry must not survive removal of the account it is bound to",
+            staleManager !== freshManager,
+        )
+        assertManagerBoundTo(freshManager, user)
+    }
+
+    @Test
+    fun clientManager_afterLogoutAndReloginAsSameIdentity_startsWithClearedFeatureMarkers() {
+        /*
+         * Regression guard: logging out and back in as the same identity
+         * must not carry the previous session's per-user feature markers
+         * forward. cleanUp() must drop this identity's perUserFeatures
+         * entry so a fresh login starts from an empty set and can select a
+         * different login type without the old session's markers lingering.
+         */
+        val user = persistUser("relogin-same-identity")
+        sdkManager.registerUsedAppFeature(Features.FEATURE_AUTH_TYPE_NATIVE, user)
+        assertTrue(
+            "Marker must be registered before logout",
+            sdkManager.isUserFeatureRegistered(Features.FEATURE_AUTH_TYPE_NATIVE, user),
+        )
+
+        sdkManager.logout(requireNotNull(userAccountManager.buildAccount(user)), null, false)
+
+        userAccountManager.createAccount(user)
+        assertFalse(
+            "A fresh login as the same identity must not inherit the previous " +
+                "session's feature markers",
+            sdkManager.isUserFeatureRegistered(Features.FEATURE_AUTH_TYPE_NATIVE, user),
+        )
+
+        sdkManager.registerUsedAppFeature(Features.FEATURE_AUTH_TYPE_WEB_SERVER_HYBRID, user)
+        assertTrue(
+            "The new session must be able to select a different login type",
+            sdkManager.isUserFeatureRegistered(Features.FEATURE_AUTH_TYPE_WEB_SERVER_HYBRID, user),
+        )
+        assertFalse(
+            "The old session's login-type marker must not carry forward",
+            sdkManager.isUserFeatureRegistered(Features.FEATURE_AUTH_TYPE_NATIVE, user),
+        )
+    }
+
     @Test
     fun retainedClient_refreshesPersistedAWhileBRemainsCurrent() {
         val userA = persistUser("refresh-a")
@@ -304,6 +428,33 @@ class SalesforceSDKManagerClientManagerTest {
         }
     }
 
+    @Test
+    fun getRestClient_withCurrentUser_populatesClientManagerCacheWithDeliveredInstance() {
+        /*
+         * Regression guard for the account-resolution caching fix:
+         * getRestClient() must resolve through the cached clientManager
+         * property rather than constructing a new ClientManager per call,
+         * since it is invoked from SalesforceActivityDelegate's onResume()
+         * on every Activity resume. Before the fix, getRestClient() built
+         * its own ClientManager directly and never touched the cache field,
+         * so this assertion fails against the pre-fix implementation.
+         */
+        val user = persistUser("resume")
+        val activity = mockk<Activity>(relaxed = true)
+        val deliveredManagers = mutableListOf<RestClient>()
+        clearCachedClientManagerField()
+
+        sdkManager.getRestClient(activity) { client -> deliveredManagers += client }
+
+        assertEquals(1, deliveredManagers.size)
+        assertClientFor(deliveredManagers.single(), user)
+        val cachedEntry = readCachedClientManagerField()
+        assertTrue(
+            "getRestClient() must populate the clientManager cache, not bypass it",
+            cachedEntry != null,
+        )
+    }
+
     @Test
     fun getRestClient_withUnusableCurrentUser_removesExactAccountWithoutCallback() {
         val user = persistUser("unusable")
@@ -378,6 +529,18 @@ class SalesforceSDKManagerClientManagerTest {
         assertEquals(user.orgId, client.clientInfo.orgId)
     }
 
+    private fun cachedClientManagerField() =
+        SalesforceSDKManager::class.java.getDeclaredField("cachedClientManager").apply {
+            isAccessible = true
+        }
+
+    private fun clearCachedClientManagerField() {
+        cachedClientManagerField().set(sdkManager, null)
+    }
+
+    private fun readCachedClientManagerField(): Any? =
+        cachedClientManagerField().get(sdkManager)
+
     private fun mockRefreshHttpClient(
         request: CapturingSlot<Request>,
         response: Response,