From f66696f6cc881aa549ea76083e552845b8d842b2 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 13 Jan 2025 20:28:33 +0100 Subject: [PATCH 1/2] feat: add sentry context to erroneous situation callbacks --- .../UserStorageController.test.ts | 7 +- .../user-storage/UserStorageController.ts | 4 +- .../controller-integration.test.ts | 102 +++++++++++++++--- .../account-syncing/controller-integration.ts | 72 ++++++++++++- 4 files changed, 163 insertions(+), 22 deletions(-) diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts index 5aa1f3b8a1..a9c18980a0 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts @@ -173,9 +173,8 @@ describe('user-storage/user-storage-controller - performGetStorageAllFeatureEntr getMetaMetricsState: () => true, }); - const result = await controller.performGetStorageAllFeatureEntries( - 'notifications', - ); + const result = + await controller.performGetStorageAllFeatureEntries('notifications'); mockAPI.done(); expect(result).toStrictEqual([MOCK_STORAGE_DATA]); }); @@ -893,7 +892,7 @@ describe('user-storage/user-storage-controller - syncInternalAccountsWithUserSto ) => { onAccountAdded?.(); onAccountNameUpdated?.(); - onAccountSyncErroneousSituation?.('error message'); + onAccountSyncErroneousSituation?.('error message', {}); getMessenger(); getUserStorageControllerInstance(); return undefined; diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts index 99c5606bdb..619336ccae 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts @@ -161,6 +161,7 @@ type ControllerConfig = { onAccountSyncErroneousSituation?: ( profileId: string, situationMessage: string, + sentryContext?: Record, ) => void; }; @@ -864,10 +865,11 @@ export default class UserStorageController extends BaseController< this.#config?.accountSyncing?.onAccountAdded?.(profileId), onAccountNameUpdated: () => this.#config?.accountSyncing?.onAccountNameUpdated?.(profileId), - onAccountSyncErroneousSituation: (situationMessage) => + onAccountSyncErroneousSituation: (situationMessage, sentryContext) => this.#config?.accountSyncing?.onAccountSyncErroneousSituation?.( profileId, situationMessage, + sentryContext, ), }, { diff --git a/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.test.ts index d0a9bf109e..f1638d67f3 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.test.ts @@ -298,28 +298,32 @@ describe('user-storage/account-syncing/controller-integration - syncInternalAcco describe('handles corrupted user storage gracefully', () => { const arrangeMocksForBogusAccounts = async () => { + const accountsList = + MOCK_INTERNAL_ACCOUNTS.ONE_DEFAULT_NAME as InternalAccount[]; const { messengerMocks, config, options } = await arrangeMocks({ messengerMockOptions: { accounts: { - accountsList: - MOCK_INTERNAL_ACCOUNTS.ONE_DEFAULT_NAME as InternalAccount[], + accountsList, }, }, }); + const userStorageList = + MOCK_USER_STORAGE_ACCOUNTS.TWO_DEFAULT_NAMES_WITH_ONE_BOGUS; + return { config, options, messengerMocks, + accountsList, + userStorageList, mockAPI: { mockEndpointGetUserStorage: await mockEndpointGetUserStorageAllFeatureEntries( USER_STORAGE_FEATURE_NAMES.accounts, { status: 200, - body: await createMockUserStorageEntries( - MOCK_USER_STORAGE_ACCOUNTS.TWO_DEFAULT_NAMES_WITH_ONE_BOGUS, - ), + body: await createMockUserStorageEntries(userStorageList), }, ), mockEndpointBatchDeleteUserStorage: @@ -363,20 +367,86 @@ describe('user-storage/account-syncing/controller-integration - syncInternalAcco expect(mockAPI.mockEndpointBatchDeleteUserStorage.isDone()).toBe(true); }); - it('fires the onAccountSyncErroneousSituation callback in erroneous situations', async () => { - const onAccountSyncErroneousSituation = jest.fn(); + describe('Fires the onAccountSyncErroneousSituation callback on erroneous situations', () => { + it('And logs if the final state is incorrect', async () => { + const onAccountSyncErroneousSituation = jest.fn(); - const { config, options } = await arrangeMocksForBogusAccounts(); + const { config, options, userStorageList, accountsList } = + await arrangeMocksForBogusAccounts(); - await AccountSyncingControllerIntegrationModule.syncInternalAccountsWithUserStorage( - { - ...config, - onAccountSyncErroneousSituation, - }, - options, - ); + await AccountSyncingControllerIntegrationModule.syncInternalAccountsWithUserStorage( + { + ...config, + onAccountSyncErroneousSituation, + }, + options, + ); - expect(onAccountSyncErroneousSituation).toHaveBeenCalledTimes(1); + expect(onAccountSyncErroneousSituation).toHaveBeenCalledTimes(2); + expect(onAccountSyncErroneousSituation.mock.calls).toEqual([ + [ + 'An account was present in the user storage accounts list but was not found in the internal accounts list after the sync', + { + internalAccountsList: accountsList, + internalAccountsToBeSavedToUserStorage: [], + refreshedInternalAccountsList: accountsList, + userStorageAccountsList: userStorageList, + userStorageAccountsToBeDeleted: [userStorageList[1]], + }, + ], + [ + 'Erroneous situations were found during the sync, and final state does not match the expected state', + { + finalInternalAccountsList: accountsList, + finalUserStorageAccountsList: null, + }, + ], + ]); + }); + + it('And logs if the final state is correct', async () => { + const onAccountSyncErroneousSituation = jest.fn(); + + const { config, options, userStorageList, accountsList } = + await arrangeMocksForBogusAccounts(); + + await mockEndpointGetUserStorageAllFeatureEntries( + USER_STORAGE_FEATURE_NAMES.accounts, + { + status: 200, + body: await createMockUserStorageEntries([userStorageList[0]]), + }, + ); + + await AccountSyncingControllerIntegrationModule.syncInternalAccountsWithUserStorage( + { + ...config, + onAccountSyncErroneousSituation, + }, + options, + ); + + expect(onAccountSyncErroneousSituation).toHaveBeenCalledTimes(2); + expect(onAccountSyncErroneousSituation.mock.calls).toEqual([ + [ + 'An account was present in the user storage accounts list but was not found in the internal accounts list after the sync', + { + internalAccountsList: accountsList, + internalAccountsToBeSavedToUserStorage: [], + refreshedInternalAccountsList: accountsList, + userStorageAccountsList: userStorageList, + userStorageAccountsToBeDeleted: [userStorageList[1]], + }, + ], + [ + 'Erroneous situations were found during the sync, but final state matches the expected state', + { + finalInternalAccountsList: accountsList, + finalUserStorageAccountsList: [userStorageList[0]], + }, + ], + ]); + }); }); }); diff --git a/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.ts b/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.ts index e39498d58b..43211387df 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.ts @@ -96,7 +96,10 @@ type SyncInternalAccountsWithUserStorageConfig = AccountSyncingConfig & { maxNumberOfAccountsToAdd?: number; onAccountAdded?: () => void; onAccountNameUpdated?: () => void; - onAccountSyncErroneousSituation?: (errorMessage: string) => void; + onAccountSyncErroneousSituation?: ( + errorMessage: string, + sentryContext?: Record, + ) => void; }; /** @@ -141,6 +144,9 @@ export async function syncInternalAccountsWithUserStorage( ); return; } + // Keep a record if erroneous situations are found during the sync + // This is done so we can send the context to Sentry in case of an erroneous situation + let erroneousSituationsFound = false; // Prepare an array of internal accounts to be saved to the user storage const internalAccountsToBeSavedToUserStorage: InternalAccount[] = []; @@ -191,8 +197,18 @@ export async function syncInternalAccountsWithUserStorage( if (!userStorageAccount) { // If the account was just added in the previous step, skip saving it, it's likely to be a bogus account if (newlyAddedAccounts.includes(internalAccount)) { + erroneousSituationsFound = true; onAccountSyncErroneousSituation?.( 'An account was added to the internal accounts list but was not present in the user storage accounts list', + { + internalAccount, + userStorageAccount, + newlyAddedAccounts, + userStorageAccountsList, + internalAccountsList, + refreshedInternalAccountsList, + internalAccountsToBeSavedToUserStorage, + }, ); continue; } @@ -295,11 +311,65 @@ export async function syncInternalAccountsWithUserStorage( USER_STORAGE_FEATURE_NAMES.accounts, userStorageAccountsToBeDeleted.map((account) => account.a), ); + erroneousSituationsFound = true; onAccountSyncErroneousSituation?.( 'An account was present in the user storage accounts list but was not found in the internal accounts list after the sync', + { + userStorageAccountsToBeDeleted, + internalAccountsList, + refreshedInternalAccountsList, + internalAccountsToBeSavedToUserStorage, + userStorageAccountsList, + }, ); } + if (erroneousSituationsFound) { + const [finalUserStorageAccountsList, finalInternalAccountsList] = + await Promise.all([ + getUserStorageAccountsList(options), + getInternalAccountsList(options), + ]); + + const doesEveryAccountInInternalAccountsListExistInUserStorageAccountsList = + finalInternalAccountsList.every((account) => + finalUserStorageAccountsList?.some( + (userStorageAccount) => userStorageAccount.a === account.address, + ), + ); + + // istanbul ignore next + const doesEveryAccountInUserStorageAccountsListExistInInternalAccountsList = + (finalUserStorageAccountsList?.length || 0) > maxNumberOfAccountsToAdd + ? true + : finalUserStorageAccountsList?.every((account) => + finalInternalAccountsList.some( + (internalAccount) => internalAccount.address === account.a, + ), + ); + + const doFinalListsMatch = + doesEveryAccountInInternalAccountsListExistInUserStorageAccountsList && + doesEveryAccountInUserStorageAccountsListExistInInternalAccountsList; + + const context = { + finalUserStorageAccountsList, + finalInternalAccountsList, + }; + if (doFinalListsMatch) { + onAccountSyncErroneousSituation?.( + 'Erroneous situations were found during the sync, but final state matches the expected state', + context, + ); + } else { + erroneousSituationsFound = true; + onAccountSyncErroneousSituation?.( + 'Erroneous situations were found during the sync, and final state does not match the expected state', + context, + ); + } + } + // We do this here and not in the finally statement because we want to make sure that // the accounts are saved / updated / deleted at least once before we set this flag await getUserStorageControllerInstance().setHasAccountSyncingSyncedAtLeastOnce( From ccd875eb2ba97f51cdae28fe93660b7f5b29dc0f Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 13 Jan 2025 20:53:11 +0100 Subject: [PATCH 2/2] fix: remove unused statement --- .../user-storage/account-syncing/controller-integration.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.ts b/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.ts index 43211387df..027f119879 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/account-syncing/controller-integration.ts @@ -362,7 +362,6 @@ export async function syncInternalAccountsWithUserStorage( context, ); } else { - erroneousSituationsFound = true; onAccountSyncErroneousSituation?.( 'Erroneous situations were found during the sync, and final state does not match the expected state', context,