MSDK 14.x dropped account-resolution caching — ~44x AccountManager IPC increase drives Android ColdStart/WarmBootstrap regression - #3040
Conversation
…ger hot paths registerUsedAppFeature(code, user) unconditionally persisted the per-user feature set to AccountManager on every call, even when the code was already registered. Guard the persist call behind the ConcurrentSkipListSet.add() result so it's a true no-op on repeat calls. clientManager constructed a fresh ClientManager (and re-resolved its backing Account) on every access. Cache the constructed manager keyed to the current user's org/user ID pair, invalidating on user switch to preserve the per-current-user binding from the ClientManager multi-user fix.
The clientManager cache only re-validates account existence on a cache-key miss (i.e. on user switch), not on every access. If the cached account is removed or becomes malformed while it remains the current-user identity (logout, corrupt-account cleanup), the cache could otherwise hand back a manager bound to an account that no longer exists. Clear the cache in cleanUp(userAccount), the existing choke point every account-removal path already funnels through, alongside the sibling clearCachedCurrentUser() call it already makes.
registerUsedAppFeature(code, user) already skips the AccountManager persistence round-trip when the code is a no-op; unregisterUsedAppFeature had the identical unconditional-persist defect. LoginActivity's per-login marker-clearing sweeps call this for markers that are already unset, so this was a guaranteed no-op AccountManager write on every login completion.
Kotlin has no language-level way to require overrides call super, so cleanUp(UserAccount?) relied entirely on convention to keep its cache-invalidating base logic (including cachedClientManager) intact in subclass overrides. @callsuper makes Android Lint's MissingSuperCall check enforce this at build time; verified by lint against SmartStoreSDKManager's existing override.
Pre-existing unused import, unrelated to any AuthenticatorService key referenced elsewhere in this file.
wmathurin
left a comment
wmathurin
left a comment
There was a problem hiding this comment.
Review feedback on the cache lifecycles and documented performance semantics.
…eature markers on account cleanup)
…currentUser resolution in clientManager cache)
…cache, not a per-user map
wmathurin
left a comment
wmathurin
left a comment
There was a problem hiding this comment.
Reviewed the follow-up commits. The per-user feature cache is now cleared on account cleanup, clientManager uses the cached identity path, and the single-entry cache semantics are documented and covered by passing focused tests. The remaining SalesforceSDK CI failures are unrelated expired test credentials.
|
Deep Review: This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce. |
|
Deep Review: This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce. |
…t-user cache reads/writes against a concurrent switch)
…identity once in getRestClient() to avoid a switch-race snapshot mismatch)
…sequential refresh token rotations
|
@brandonpage - I've got your review topics in-progress, but I'm taking a little longer this morning to really look them over. My first pass yesterday uncovered a couple other topics yours helped discover. Thanks! |
…toredCurrentUserInfo() against a concurrent cache read) clearStoredCurrentUserInfo() had the same clear-then-write race as storeCurrentUserInfo() (finding #1): it cleared the cached current user before clearing the stored user/org ID from SharedPreferences, so a concurrent getCachedCurrentUser() call landing in that window could repopulate the cache from the not-yet-cleared IDs and keep returning the logged-out user after the call returns. Found as a derivative of Brandon's finding #1 while self-reviewing the fix for that finding. Wraps the clear + SharedPreferences clear().apply() in the same currentUserLock monitor used by storeCurrentUserInfo().
…eterministic exclusion tests testStoreCurrentUserInfoIsNotStaleAfterConcurrentCacheReadDuringSwitch and testClearStoredCurrentUserInfoIsNotStaleAfterConcurrentCacheReadDuringClear raced busy-loop reader threads against thousands of writer trials, hoping the scheduler would land in the (now closed) vulnerable window. Against the fix in place this can only prove absence of failure over N trials, not presence of correct behavior, and cost 50-70+ seconds of wall-clock per run. Replaces both with a deterministic test per write path (testStoreCurrentUserInfoBlocksUntilConcurrentCacheReadCompletes, testClearStoredCurrentUserInfoBlocksUntilConcurrentCacheReadCompletes) using a same-package UserAccountManager subclass that pins a reader inside getCurrentAccount() via a CountDownLatch while it still holds currentUserLock, then asserts a concurrent writer call provably blocks until the reader releases the lock. This proves genuine mutual exclusion on the shared monitor directly, in under a second per test, instead of inferring it from thousands of racing trials.
getRestClient() built a UserAccount from the current Account, then ClientManager.peekRestClient() rebuilt an independent UserAccount from that same Account internally, decrypting the same AccountManager fields twice on every call to this Activity-resume hot path. Add a peekRestClient(UserAccount) overload that reuses the caller's already- resolved UserAccount while preserving the same liveness re-check and field validation, and route getRestClient() through it.
|
While addressing your review comment on the current-user cache race, I found the same clear-then-write race also existed on the logout path: This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce. |
|
Followed up on the This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce. |
|
One more finding from self-review: This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce. |
Generated by 🚫 Danger |
…ent(UserAccount) package-private again, bridged via a module-internal extension function, to satisfy the ClientManager API-surface guard test)
|
Follow-up: resolved a self-caught API-surface regression before merge While re-checking CI ahead of merge, I found that an earlier commit on this branch ( Fixed in commit 66f1a24: the overload is package-private again, and This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce. |
834aac5
into
forcedotcom:dev
3 participants
Summary
Restores account-resolution caching that was dropped, which was driving a large increase in
AccountManagerIPC calls on cold-start and warm-bootstrap paths.SalesforceSDKManager.clientManager, keyed by org/user ID, so repeated access for the same current user re-resolves the account once instead of on every call.getRestClient()through that cache instead of constructing aClientManagerdirectly.getRestClient()is called fromSalesforceActivityDelegate.onResume(), so it's on the Activity-resume hot path — found during review that it was bypassing the new cache entirely and still paying fullAccountManagerIPC cost on every resume.registerUsedAppFeature(code, user)andunregisterUsedAppFeature(code, user)so calls that don't actually change the per-user feature set skip theAccountManagerpersistence round-trip.LoginActivity's per-login marker-clearing sweeps call the unregister path for markers that are already unset on every login, so this was a guaranteed no-op write on every login completion before the fix.clientManagercache incleanUp(userAccount), the existing choke point every account-removal path (logout, malformed-account purge, corrupt-account cleanup) already funnels through, closing a gap where the cache could return a manager bound to an account that was removed while remaining the "current" identity.@CallSupertocleanUp(UserAccount?). Kotlin has no language-level "must call super" enforcement; this makes Android Lint'sMissingSuperCallcheck enforce it at build time for any subclass (e.g.SmartStoreSDKManager) that overrides it, since skipping the super call would silently reintroduce the invalidation gap above.AuthenticatorService.KEY_INSTANCE_URL) found while working in this file; pre-existing and unrelated to this fix.A note on scope
The
clientManagercache (this PR's core change) touches multi-user account resolution, which is a correctness-sensitive area. It has not yet been reviewed by the original author of that multi-user contract — flagging that explicitly here rather than assuming silence means no concern.Test plan
registerUsedAppFeature/unregisterUsedAppFeature(verified fail-without/pass-with the fix)clientManagercache invalidation on account removal (verified fail-without/pass-with the fix)getRestClient()reusing the cachedclientManagerinstead of bypassing it (verified fail-without/pass-with the fix)@CallSuperactually triggers Android Lint'sMissingSuperCallat build time by temporarily removing a subclass'ssuper.cleanUp()call and confirming the lint error, then restoring itSalesforceSDKManagerTests+SalesforceSDKManagerClientManagerTestrun together locally: full pass, no failuresSalesforceSDKinstrumented test suite — deferred to CIThis response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.