fix: make avatar download job only take conversationId

and not the profileKey+url too as we need the db entry to be in sync
with the config wrapper data (otherwise the next commit would be made
with data out of date from the wrapper side)

ts/components/registration/RegistrationStages.tsx

@@ -128,7 +128,7 @@ export async function signInWithLinking(signInDetails: { userRecoveryPhrase: str
     }, 60000);
     if (displayNameFromNetwork.length) {
       // display name, avatars, groups and contacts should already be handled when this event was triggered.
-      window?.log?.info('We got a displayName from network: ');
+      window?.log?.info(`We got a displayName from network: "${displayNameFromNetwork}"`);
     } else {
       window?.log?.info('Got a config message from network but without a displayName...');
       throw new Error('Got a config message from network but without a displayName...');

ts/receiver/configMessage.ts

@@ -19,7 +19,7 @@ import { getConversationController } from '../session/conversations';
 import { IncomingMessage } from '../session/messages/incoming/IncomingMessage';
 import { ProfileManager } from '../session/profile_manager/ProfileManager';
 import { PubKey } from '../session/types';
-import { UserUtils } from '../session/utils';
+import { StringUtils, UserUtils } from '../session/utils';
 import { toHex } from '../session/utils/String';
 import { ConfigurationSync } from '../session/utils/job_runners/jobs/ConfigurationSyncJob';
 import { IncomingConfResult, LibSessionUtil } from '../session/utils/libsession/libsession_utils';
@@ -53,6 +53,8 @@ import { queueAllCachedFromSource } from './receiver';
 import { EnvelopePlus } from './types';
 import { deleteAllMessagesByConvoIdNoConfirmation } from '../interactions/conversationInteractions';
 
+const printDumpsForDebugging = false;
+
 function groupByVariant(
   incomingConfigs: Array<IncomingMessage<SignalService.ISharedConfigMessage>>
 ) {
@@ -97,6 +99,12 @@ async function mergeConfigsWithIncomingUpdates(
         data: msg.message.data,
         hash: msg.messageHash,
       }));
+      if (printDumpsForDebugging) {
+        window.log.info(
+          `printDumpsForDebugging: before merge of ${variant}:`,
+          StringUtils.toHex(await GenericWrapperActions.dump(variant))
+        );
+      }
       const mergedCount = await GenericWrapperActions.merge(variant, toMerge);
       const needsPush = await GenericWrapperActions.needsPush(variant);
       const needsDump = await GenericWrapperActions.needsDump(variant);
@@ -106,6 +114,12 @@ async function mergeConfigsWithIncomingUpdates(
         `${variant}: "${publicKey}" needsPush:${needsPush} needsDump:${needsDump}; mergedCount:${mergedCount} `
       );
 
+      if (printDumpsForDebugging) {
+        window.log.info(
+          `printDumpsForDebugging: after merge of ${variant}:`,
+          StringUtils.toHex(await GenericWrapperActions.dump(variant))
+        );
+      }
       const incomingConfResult: IncomingConfResult = {
         needsDump,
         needsPush,

ts/session/profile_manager/ProfileManager.ts

@@ -1,8 +1,8 @@
 import { isEmpty } from 'lodash';
 import { getConversationController } from '../conversations';
 import { UserUtils } from '../utils';
-import { AvatarDownload } from '../utils/job_runners/jobs/AvatarDownloadJob';
 import { toHex } from '../utils/String';
+import { AvatarDownload } from '../utils/job_runners/jobs/AvatarDownloadJob';
 
 /**
  * This can be used to update our conversation display name with the given name right away, and plan an AvatarDownloadJob to retrieve the new avatar if needed to download it
@@ -42,21 +42,45 @@ async function updateProfileOfContact(
     window.log.warn('updateProfileOfContact can only be used for existing and private convos');
     return;
   }
-
+  let changes = false;
   const existingDisplayName = conversation.get('displayNameInProfile');
 
   // avoid setting the display name to an invalid value
   if (existingDisplayName !== displayName && !isEmpty(displayName)) {
     conversation.set('displayNameInProfile', displayName || undefined);
-    await conversation.commit();
+    changes = true;
   }
-  // add an avatar download job only if needed
+
   const profileKeyHex = !profileKey || isEmpty(profileKey) ? null : toHex(profileKey);
-  await AvatarDownload.addAvatarDownloadJobIfNeeded({
-    profileKeyHex,
-    profileUrl,
-    pubkey,
-  });
+
+  let avatarChanged = false;
+  // trust whatever we get as an update. It either comes from a shared config wrapper or one of that user's message. But in any case we should trust it, even if it gets resetted.
+  const prevPointer = conversation.get('avatarPointer');
+  const prevProfileKey = conversation.get('profileKey');
+
+  // we have to set it right away and not in the async download job, as the next .commit will save it to the
+  // database and wrapper (and we do not want to override anything in the wrapper's content
+  // with what we have locally, so we need the commit to have already the right values in pointer and profileKey)
+  if (prevPointer !== profileUrl || prevProfileKey !== profileKeyHex) {
+    conversation.set({
+      avatarPointer: profileUrl || undefined,
+      profileKey: profileKeyHex || undefined,
+    });
+
+    // if the avatar data we had before is not the same of what we received, we need to schedule a new avatar download job.
+    avatarChanged = true; // allow changes from strings to null/undefined to trigger a AvatarDownloadJob. If that happens, we want to remove the local attachment file.
+  }
+
+  if (changes) {
+    await conversation.commit();
+  }
+
+  if (avatarChanged) {
+    // this call will download the new avatar or reset the local filepath if needed
+    await AvatarDownload.addAvatarDownloadJob({
+      conversationId: pubkey,
+    });
+  }
 }
 
 export const ProfileManager = {

ts/session/utils/job_runners/JobRunner.ts

@@ -98,16 +98,6 @@ export class PersistedJobRunner<T extends TypeOfPersistedData> {
       return 'type_exists';
     }
 
-    // if addJobCheck returned 'removeJobsFromQueue it means that job logic estimates some jobs have to remove before adding that one.
-    // so let's grab the jobs to remove, remove them, and then add that new job nevertheless
-    if (addJobChecks === 'removeJobsFromQueue') {
-      // fetch all the jobs which we should remove and remove them
-      const toRemove = job.nonRunningJobsToRemove(serializedNonRunningJobs);
-      this.deleteJobsByIdentifier(toRemove.map(m => m.identifier));
-      this.sortJobsList();
-      await this.writeJobsToDB();
-    }
-
     // make sure there is no job with that same identifier already .
 
     window.log.info(`job runner adding type :"${job.persistedData.jobType}" `);

ts/session/utils/job_runners/PersistedJob.ts

@@ -29,8 +29,6 @@ export interface FakeSleepForMultiJobData extends PersistedJobData {
 export interface AvatarDownloadPersistedData extends PersistedJobData {
   jobType: 'AvatarDownloadJobType';
   conversationId: string;
-  profileKeyHex: string | null;
-  profilePictureUrl: string | null;
 }
 
 export interface ConfigurationSyncPersistedData extends PersistedJobData {
@@ -43,11 +41,7 @@ export type TypeOfPersistedData =
   | FakeSleepJobData
   | FakeSleepForMultiJobData;
 
-export type AddJobCheckReturn =
-  | 'skipAddSameJobPresent'
-  | 'removeJobsFromQueue'
-  | 'sameJobDataAlreadyInQueue'
-  | null;
+export type AddJobCheckReturn = 'skipAddSameJobPresent' | 'sameJobDataAlreadyInQueue' | null;
 
 export enum RunJobResult {
   Success = 1,

ts/session/utils/job_runners/jobs/AvatarDownloadJob.ts

@@ -1,4 +1,4 @@
-import { isEmpty, isEqual, isNumber, isString } from 'lodash';
+import { isEmpty, isNumber, isString } from 'lodash';
 import { v4 } from 'uuid';
 import { UserUtils } from '../..';
 import { downloadAttachment } from '../../../../receiver/attachments';
@@ -20,18 +20,11 @@ const defaultMsBetweenRetries = 10000;
 const defaultMaxAttemps = 3;
 
 /**
- * Returns true if given those details we should add an Avatar Download Job to the list of jobs to run
+ * Returns true if the provided conversationId is a private chat and that we should add an Avatar Download Job to the list of jobs to run.
+ * Before calling this function, you have to update the related conversation profileKey and avatarPointer fields with the urls which should be downloaded, or reset them if you wanted them reset.
  */
-function shouldAddAvatarDownloadJob({
-  profileKeyHex,
-  profileUrl,
-  pubkey,
-}: {
-  pubkey: string;
-  profileUrl: string | null | undefined;
-  profileKeyHex: string | null | undefined;
-}) {
-  const conversation = getConversationController().get(pubkey);
+export function shouldAddAvatarDownloadJob({ conversationId }: { conversationId: string }) {
+  const conversation = getConversationController().get(conversationId);
   if (!conversation) {
     // return true so we do not retry this task.
     window.log.warn('shouldAddAvatarDownloadJob did not corresponding conversation');
@@ -43,37 +36,24 @@ function shouldAddAvatarDownloadJob({
     return false;
   }
   const prevPointer = conversation.get('avatarPointer');
+  const profileKey = conversation.get('profileKey');
+  const hasNoAvatar = isEmpty(prevPointer) || isEmpty(profileKey);
 
-  if (!isEmpty(profileUrl) && !isEmpty(profileKeyHex) && !isEqual(prevPointer, profileUrl)) {
-    return true;
+  if (hasNoAvatar) {
+    return false;
   }
-  return false;
+
+  return true;
 }
 
-async function addAvatarDownloadJobIfNeeded({
-  profileKeyHex,
-  profileUrl,
-  pubkey,
-}: {
-  pubkey: string;
-  profileUrl: string | null | undefined;
-  profileKeyHex: string | null | undefined;
-}) {
-  if (profileKeyHex && shouldAddAvatarDownloadJob({ pubkey, profileUrl, profileKeyHex })) {
+async function addAvatarDownloadJob({ conversationId }: { conversationId: string }) {
+  if (shouldAddAvatarDownloadJob({ conversationId })) {
     const avatarDownloadJob = new AvatarDownloadJob({
-      conversationId: pubkey,
-      profileKeyHex,
-      profilePictureUrl: profileUrl || null,
+      conversationId,
       nextAttemptTimestamp: Date.now(),
     });
-    window.log.debug(
-      `addAvatarDownloadJobIfNeeded: adding job download for ${pubkey}:${profileUrl}:${profileKeyHex} `
-    );
+    window.log.debug(`addAvatarDownloadJobIfNeeded: adding job download for ${conversationId} `);
     await runners.avatarDownloadRunner.addJob(avatarDownloadJob);
-  } else {
-    // window.log.debug(
-    //   `addAvatarDownloadJobIfNeeded: no download required for ${pubkey}:${profileUrl}:${profileKeyHex} `
-    // );
   }
 }
 
@@ -89,12 +69,9 @@ class AvatarDownloadJob extends PersistedJob<AvatarDownloadPersistedData> {
     nextAttemptTimestamp,
     maxAttempts,
     currentRetry,
-    profileKeyHex,
-    profilePictureUrl,
     identifier,
-  }: Pick<AvatarDownloadPersistedData, 'profileKeyHex' | 'profilePictureUrl'> & {
-    conversationId: string;
-  } & Partial<
+  }: Pick<AvatarDownloadPersistedData, 'conversationId'> &
+    Partial<
       Pick<
         AvatarDownloadPersistedData,
         | 'nextAttemptTimestamp'
@@ -112,8 +89,6 @@ class AvatarDownloadJob extends PersistedJob<AvatarDownloadPersistedData> {
       maxAttempts: isNumber(maxAttempts) ? maxAttempts : defaultMaxAttemps,
       nextAttemptTimestamp: nextAttemptTimestamp || Date.now() + defaultMsBetweenRetries,
       currentRetry: isNumber(currentRetry) ? currentRetry : 0,
-      profileKeyHex,
-      profilePictureUrl,
     });
   }
 
@@ -142,103 +117,81 @@ class AvatarDownloadJob extends PersistedJob<AvatarDownloadPersistedData> {
       return RunJobResult.PermanentFailure;
     }
     let changes = false;
+    const toDownloadPointer = conversation.get('avatarPointer');
+    const toDownloadProfileKey = conversation.get('profileKey');
+
+    // if there is an avatar and profileKey for that user ('', null and undefined excluded), download, decrypt and save the avatar locally.
+    if (toDownloadPointer && toDownloadProfileKey) {
+      try {
+        window.log.debug(`[profileupdate] starting downloading task for  ${conversation.id}`);
+        const downloaded = await downloadAttachment({
+          url: toDownloadPointer,
+          isRaw: true,
+        });
+        conversation = getConversationController().getOrThrow(convoId);
 
-    const shouldRunJob = shouldAddAvatarDownloadJob({
-      pubkey: convoId,
-      profileKeyHex: this.persistedData.profileKeyHex,
-      profileUrl: this.persistedData.profilePictureUrl,
-    });
-    if (!shouldRunJob) {
-      // return true so we do not retry this task.
-      window.log.warn('AvatarDownloadJob shouldAddAvatarDownloadJob said no');
-
-      return RunJobResult.PermanentFailure;
-    }
+        if (!downloaded.data.byteLength) {
+          window.log.debug(`[profileupdate] downloaded data is empty for  ${conversation.id}`);
+          return RunJobResult.RetryJobIfPossible; // so we retry this job
+        }
 
-    if (this.persistedData.profilePictureUrl && this.persistedData.profileKeyHex) {
-      const prevPointer = conversation.get('avatarPointer');
-      const needsUpdate =
-        !prevPointer || !isEqual(prevPointer, this.persistedData.profilePictureUrl);
+        // null => use placeholder with color and first letter
+        let path = null;
 
-      if (needsUpdate) {
         try {
-          window.log.debug(`[profileupdate] starting downloading task for  ${conversation.id}`);
-          const downloaded = await downloadAttachment({
-            url: this.persistedData.profilePictureUrl,
-            isRaw: true,
-          });
-          conversation = getConversationController().getOrThrow(convoId);
-
-          if (!downloaded.data.byteLength) {
-            window.log.debug(`[profileupdate] downloaded data is empty for  ${conversation.id}`);
-            return RunJobResult.RetryJobIfPossible; // so we retry this job
-          }
-
-          // null => use placeholder with color and first letter
-          let path = null;
-
+          const profileKeyArrayBuffer = fromHexToArray(toDownloadProfileKey);
+          let decryptedData: ArrayBuffer;
           try {
-            const profileKeyArrayBuffer = fromHexToArray(this.persistedData.profileKeyHex);
-            let decryptedData: ArrayBuffer;
-            try {
-              decryptedData = await decryptProfile(downloaded.data, profileKeyArrayBuffer);
-            } catch (decryptError) {
-              window.log.info(
-                `[profileupdate] failed to decrypt downloaded data ${conversation.id} with provided profileKey`
-              );
-              // if we cannot decrypt the content, there is no need to keep retrying.
-              return RunJobResult.PermanentFailure;
-            }
-
+            decryptedData = await decryptProfile(downloaded.data, profileKeyArrayBuffer);
+          } catch (decryptError) {
             window.log.info(
-              `[profileupdate] about to auto scale avatar for convo ${conversation.id}`
+              `[profileupdate] failed to decrypt downloaded data ${conversation.id} with provided profileKey`
             );
+            // if we got content, but cannot decrypt it with the provided profileKey, there is no need to keep retrying.
+            return RunJobResult.PermanentFailure;
+          }
 
-            const scaledData = await autoScaleForIncomingAvatar(decryptedData);
+          window.log.info(
+            `[profileupdate] about to auto scale avatar for convo ${conversation.id}`
+          );
 
-            const upgraded = await processNewAttachment({
-              data: await scaledData.blob.arrayBuffer(),
-              contentType: MIME.IMAGE_UNKNOWN, // contentType is mostly used to generate previews and screenshot. We do not care for those in this case.
-            });
-            conversation = getConversationController().getOrThrow(convoId);
+          // we autoscale incoming avatars because our app keeps decrypted avatars in memory and some platforms allows large avatars to be uploaded.
+          const scaledData = await autoScaleForIncomingAvatar(decryptedData);
 
-            // Only update the convo if the download and decrypt is a success
-            conversation.set('avatarPointer', this.persistedData.profilePictureUrl);
-            conversation.set('profileKey', this.persistedData.profileKeyHex || undefined);
-            ({ path } = upgraded);
-          } catch (e) {
-            window?.log?.error(`[profileupdate] Could not decrypt profile image: ${e}`);
-            return RunJobResult.RetryJobIfPossible; // so we retry this job
-          }
+          const upgraded = await processNewAttachment({
+            data: await scaledData.blob.arrayBuffer(),
+            contentType: MIME.IMAGE_UNKNOWN, // contentType is mostly used to generate previews and screenshot. We do not care for those in this case.
+          });
+          conversation = getConversationController().getOrThrow(convoId);
+          ({ path } = upgraded);
+        } catch (e) {
+          window?.log?.error(`[profileupdate] Could not decrypt profile image: ${e}`);
+          return RunJobResult.RetryJobIfPossible; // so we retry this job
+        }
 
-          conversation.set({ avatarInProfile: path || undefined });
+        conversation.set({ avatarInProfile: path || undefined });
 
-          changes = true;
-        } catch (e) {
-          if (isString(e.message) && (e.message as string).includes('404')) {
-            window.log.warn(
-              `[profileupdate] Failed to download attachment at ${this.persistedData.profilePictureUrl}. We got 404 error: "${e.message}"`
-            );
-            return RunJobResult.PermanentFailure;
-          }
+        changes = true;
+      } catch (e) {
+        // TODO would be nice to throw a specific exception here instead of relying on the error string.
+        if (isString(e.message) && (e.message as string).includes('404')) {
           window.log.warn(
-            `[profileupdate] Failed to download attachment at ${this.persistedData.profilePictureUrl}. Maybe it expired? ${e.message}`
+            `[profileupdate] Failed to download attachment at ${toDownloadPointer}. We got 404 error: "${e.message}"`
           );
-          return RunJobResult.RetryJobIfPossible;
+          return RunJobResult.PermanentFailure;
         }
+        window.log.warn(
+          `[profileupdate] Failed to download attachment at ${toDownloadPointer}. Maybe it expired? ${e.message}`
+        );
+        return RunJobResult.RetryJobIfPossible;
       }
     } else {
-      if (
-        conversation.get('avatarInProfile') ||
-        conversation.get('avatarPointer') ||
-        conversation.get('profileKey')
-      ) {
-        changes = true;
+      // there is no valid avatar to download, make sure the local file of the avatar of that user is removed
+      if (conversation.get('avatarInProfile')) {
         conversation.set({
           avatarInProfile: undefined,
-          avatarPointer: undefined,
-          profileKey: undefined,
         });
+        changes = true;
       }
     }
 
@@ -270,27 +223,20 @@ class AvatarDownloadJob extends PersistedJob<AvatarDownloadPersistedData> {
     return super.serializeBase();
   }
 
-  public nonRunningJobsToRemove(jobs: Array<AvatarDownloadPersistedData>) {
-    // for an avatar download job, we want to remove any job matching the same conversationID.
-    return jobs.filter(j => j.conversationId === this.persistedData.conversationId);
+  public nonRunningJobsToRemove(_jobs: Array<AvatarDownloadPersistedData>) {
+    return [];
   }
 
   public addJobCheck(jobs: Array<AvatarDownloadPersistedData>): AddJobCheckReturn {
     // avoid adding the same job if the exact same one is already planned
     const hasSameJob = jobs.some(j => {
-      return (
-        j.conversationId === this.persistedData.conversationId &&
-        j.profileKeyHex === this.persistedData.profileKeyHex &&
-        j.profilePictureUrl === this.persistedData.profilePictureUrl
-      );
+      return j.conversationId === this.persistedData.conversationId;
     });
 
     if (hasSameJob) {
       return 'skipAddSameJobPresent';
     }
-    if (this.nonRunningJobsToRemove(jobs).length) {
-      return 'removeJobsFromQueue';
-    }
+
     return null;
   }
 
@@ -301,5 +247,5 @@ class AvatarDownloadJob extends PersistedJob<AvatarDownloadPersistedData> {
 
 export const AvatarDownload = {
   AvatarDownloadJob,
-  addAvatarDownloadJobIfNeeded,
+  addAvatarDownloadJob,
 };