feat: fix message attachment cleanup and handle group attach+msg delete

ts/data/data.ts

@@ -292,11 +292,13 @@ async function removeAllMessagesInConversationSentBefore(args: {
   return channels.removeAllMessagesInConversationSentBefore(args);
 }
 
-async function removeAllAttachmentsInConversationSentBefore(args: {
+async function getAllMessagesWithAttachmentsInConversationSentBefore(args: {
   deleteAttachBeforeSeconds: number;
   conversationId: GroupPubkeyType;
-}): Promise<Array<string>> {
-  return channels.removeAllAttachmentsInConversationSentBefore(args);
+}): Promise<Array<MessageModel>> {
+  const msgAttrs = await channels.getAllMessagesWithAttachmentsInConversationSentBefore(args);
+
+  return msgAttrs.map((msg: any) => new MessageModel(msg));
 }
 
 async function getMessageIdsFromServerIds(
@@ -839,7 +841,7 @@ export const Data = {
   removeMessage,
   removeMessagesByIds,
   removeAllMessagesInConversationSentBefore,
-  removeAllAttachmentsInConversationSentBefore,
+  getAllMessagesWithAttachmentsInConversationSentBefore,
   cleanUpExpirationTimerUpdateHistory,
   getMessageIdsFromServerIds,
   getMessageById,

ts/data/dataInit.ts

@@ -41,7 +41,7 @@ const channelsToMake = new Set([
   'saveMessages',
   'removeMessage',
   'removeMessagesByIds',
-  'removeAllAttachmentsInConversationSentBefore',
+  'getAllMessagesWithAttachmentsInConversationSentBefore',
   'removeAllMessagesInConversationSentBefore',
   'cleanUpExpirationTimerUpdateHistory',
   'getUnreadByConversation',

ts/models/message.ts

@@ -301,12 +301,11 @@ export class MessageModel extends Backbone.Model<MessageAttributes> {
     return '';
   }
 
-  public onDestroy() {
-    void this.cleanup();
-  }
-
   public async cleanup() {
-    await deleteExternalMessageFiles(this.attributes);
+    const changed = await deleteExternalMessageFiles(this.attributes);
+    if (changed) {
+      await this.commit();
+    }
   }
 
   public getPropsForExpiringMessage(): PropsForExpiringMessage {

ts/node/sql.ts

@@ -1003,7 +1003,7 @@ function removeAllMessagesInConversationSentBefore(
     .prepare(
       `SELECT id FROM ${MESSAGES_TABLE} WHERE conversationId = $conversationId AND sent_at <= $beforeMs;`
     )
-    .all({ conversationId, beforeMs: deleteBeforeSeconds * 1000 }) as Array<string>;
+    .all({ conversationId, beforeMs: deleteBeforeSeconds * 1000 });
 
   assertGlobalInstanceOrInstance(instance)
     .prepare(
@@ -1011,10 +1011,10 @@ function removeAllMessagesInConversationSentBefore(
     )
     .run({ conversationId, beforeMs: deleteBeforeSeconds * 1000 });
   console.info('removeAllMessagesInConversationSentBefore deleted msgIds:', JSON.stringify(msgIds));
-  return msgIds;
+  return msgIds.map(m => m.id);
 }
 
-async function removeAllAttachmentsInConversationSentBefore(
+async function getAllMessagesWithAttachmentsInConversationSentBefore(
   {
     deleteAttachBeforeSeconds,
     conversationId,
@@ -1023,29 +1023,14 @@ async function removeAllAttachmentsInConversationSentBefore(
 ) {
   const rows = assertGlobalInstanceOrInstance(instance)
     .prepare(
-      `SELECT json FROM ${MESSAGES_TABLE} WHERE conversationId = $conversationId AND sent_at <= $beforeMs`
+      `SELECT json FROM ${MESSAGES_TABLE} WHERE conversationId = $conversationId AND sent_at <= $beforeMs;`
     )
     .all({ conversationId, beforeMs: deleteAttachBeforeSeconds * 1000 });
   const messages = map(rows, row => jsonToObject(row.json));
-
-  const externalFiles: Array<string> = [];
-  const msgIdsWithChanges: Array<string> = [];
-  forEach(messages, message => {
-    const externalFilesMsg = getExternalFilesForMessage(message);
-    if (externalFilesMsg.length) {
-      externalFiles.push(...externalFilesMsg);
-      msgIdsWithChanges.push();
-    }
+  const messagesWithAttachments = messages.filter(m => {
+    return getExternalFilesForMessage(m).some(a => !isEmpty(a) && isString(a)); // when we remove an attachment, we set the path to '' so it should be excluded here
   });
-  const uniqPathsToRemove = uniq(externalFiles);
-  console.info('removeAllAttachmentsInConversationSentBefore removing attachments:', externalFiles);
-  console.info(
-    'removeAllAttachmentsInConversationSentBefore impacted msgIds:',
-    JSON.stringify(msgIdsWithChanges)
-  );
-
-  const userDataPath = app.getPath('userData');
-  await deleteAll({ userDataPath, attachments: uniqPathsToRemove });
+  return messagesWithAttachments;
 }
 
 function removeAllMessagesInConversation(
@@ -2553,7 +2538,7 @@ export const sqlNode = {
   removeMessage,
   removeMessagesByIds,
   removeAllMessagesInConversationSentBefore,
-  removeAllAttachmentsInConversationSentBefore,
+  getAllMessagesWithAttachmentsInConversationSentBefore,
   cleanUpExpirationTimerUpdateHistory,
   removeAllMessagesInConversation,
   getUnreadByConversation,

ts/session/apis/snode_api/swarm_polling_config/SwarmPollingGroupConfig.ts

@@ -1,6 +1,7 @@
 import { GroupPubkeyType } from 'libsession_util_nodejs';
 import { isFinite, isNumber } from 'lodash';
 import { Data } from '../../../../data/data';
+import { messagesExpired } from '../../../../state/ducks/conversations';
 import { groupInfoActions } from '../../../../state/ducks/metaGroups';
 import { MetaGroupWrapperActions } from '../../../../webworker/workers/browser/libsession_worker_interface';
 import { ed25519Str } from '../../../onions/onionPath';
@@ -22,7 +23,13 @@ async function handleMetaMergeResults(groupPk: GroupPubkeyType) {
       deleteBeforeSeconds: infos.deleteBeforeSeconds,
       conversationId: groupPk,
     });
-    console.warn('deletedMsgIds', deletedMsgIds);
+    window.log.info(
+      `removeAllMessagesInConversationSentBefore of ${ed25519Str(groupPk)} before ${infos.deleteBeforeSeconds}: `,
+      deletedMsgIds
+    );
+    await window.inboxStore.dispatch(
+      messagesExpired(deletedMsgIds.map(messageId => ({ conversationKey: groupPk, messageId })))
+    );
   }
 
   if (
@@ -32,11 +39,22 @@ async function handleMetaMergeResults(groupPk: GroupPubkeyType) {
     infos.deleteAttachBeforeSeconds > 0
   ) {
     // delete any attachments in this conversation sent before that timestamp (in seconds)
-    const impactedMsgids = await Data.removeAllAttachmentsInConversationSentBefore({
+    const impactedMsgModels = await Data.getAllMessagesWithAttachmentsInConversationSentBefore({
       deleteAttachBeforeSeconds: infos.deleteAttachBeforeSeconds,
       conversationId: groupPk,
     });
-    console.warn('impactedMsgids', impactedMsgids);
+    window.log.info(
+      `getAllMessagesWithAttachmentsInConversationSentBefore of ${ed25519Str(groupPk)} before ${infos.deleteAttachBeforeSeconds}: impactedMsgModelsIds `,
+      impactedMsgModels.map(m => m.id)
+    );
+
+    for (let index = 0; index < impactedMsgModels.length; index++) {
+      const msg = impactedMsgModels[index];
+
+      // eslint-disable-next-line no-await-in-loop
+      // eslint-disable-next-line no-await-in-loop
+      await msg?.cleanup();
+    }
   }
 }
 

ts/types/MessageAttachment.ts

@@ -11,20 +11,39 @@ import {
   autoOrientJPEGAttachment,
   captureDimensionsAndScreenshot,
   deleteData,
+  deleteDataSuccessful,
   loadData,
   replaceUnicodeV2,
 } from './attachments/migrations';
 
 // NOTE I think this is only used on the renderer side, but how?!
-export const deleteExternalMessageFiles = async (message: {
-  attachments: any;
-  quote: any;
-  preview: any;
+export const deleteExternalMessageFiles = async (messageAttributes: {
+  attachments: Array<any> | undefined;
+  quote: { attachments: Array<any> | undefined };
+  preview: Array<any> | undefined;
 }) => {
-  const { attachments, quote, preview } = message;
+  let anyChanges = false;
+  const { attachments, quote, preview } = messageAttributes;
 
   if (attachments && attachments.length) {
     await Promise.all(attachments.map(deleteData));
+    anyChanges = true;
+
+    // test that the files were deleted successfully
+    try {
+      let results = await Promise.allSettled(attachments.map(deleteDataSuccessful));
+      results = results.filter(result => result.status === 'rejected');
+
+      if (results.length) {
+        throw Error;
+      }
+    } catch (err) {
+      // eslint-disable-next-line no-console
+      console.warn(
+        '[deleteExternalMessageFiles]: Failed to delete attachments for',
+        messageAttributes
+      );
+    }
   }
 
   if (quote && quote.attachments && quote.attachments.length) {
@@ -41,6 +60,8 @@ export const deleteExternalMessageFiles = async (message: {
         }
 
         attachment.thumbnail = undefined;
+        anyChanges = true;
+
         return attachment;
       })
     );
@@ -57,10 +78,13 @@ export const deleteExternalMessageFiles = async (message: {
         }
 
         item.image = undefined;
+        anyChanges = true;
+
         return image;
       })
     );
   }
+  return anyChanges;
 };
 
 let attachmentsPath: string | undefined;

ts/types/attachments/migrations.ts

@@ -1,8 +1,8 @@
+/* eslint-disable no-param-reassign */
 import { arrayBufferToBlob, blobToArrayBuffer } from 'blob-util';
-import { pathExists } from 'fs-extra';
 
+import fse from 'fs-extra';
 import { isString } from 'lodash';
-
 import * as GoogleChrome from '../../util/GoogleChrome';
 import * as MIME from '../MIME';
 import { toLogFormat } from './Errors';
@@ -145,26 +145,6 @@ export const loadData = async (attachment: any) => {
   return { ...attachment, data };
 };
 
-const handleDiskDeletion = async (path: string) => {
-  await deleteOnDisk(path);
-  try {
-    const exists = await pathExists(path);
-
-    // NOTE we want to confirm the path no longer exists
-    if (exists) {
-      throw Error('Error: File path still exists.');
-    }
-
-    window.log.debug(`deleteDataSuccessful: Deletion succeeded for attachment ${path}`);
-    return undefined;
-  } catch (err) {
-    window.log.warn(
-      `deleteDataSuccessful: Deletion failed for attachment ${path} ${err.message || err}`
-    );
-    return path;
-  }
-};
-
 //      deleteData :: (RelativePath -> IO Unit)
 //                    Attachment ->
 //                    IO Unit
@@ -177,24 +157,43 @@ export const deleteData = async (attachment: {
     throw new TypeError('deleteData: attachment is not valid');
   }
 
-  let { path, thumbnail, screenshot } = attachment;
-
-  if (path && isString(path)) {
-    const pathAfterDelete = await handleDiskDeletion(path);
-    path = isString(pathAfterDelete) ? pathAfterDelete : undefined;
+  const { path, thumbnail, screenshot } = attachment;
+  if (isString(path)) {
+    await deleteOnDisk(path);
+    attachment.path = '';
   }
-
   if (thumbnail && isString(thumbnail.path)) {
-    const pathAfterDelete = await handleDiskDeletion(thumbnail.path);
-    thumbnail = isString(pathAfterDelete) ? pathAfterDelete : undefined;
+    await deleteOnDisk(thumbnail.path);
+    attachment.thumbnail = undefined;
   }
-
   if (screenshot && isString(screenshot.path)) {
-    const pathAfterDelete = await handleDiskDeletion(screenshot.path);
-    screenshot = isString(pathAfterDelete) ? pathAfterDelete : undefined;
+    await deleteOnDisk(screenshot.path);
+    attachment.screenshot = undefined;
   }
 
-  return { path, thumbnail, screenshot };
+  return attachment;
+};
+
+export const deleteDataSuccessful = async (attachment: {
+  path: string;
+  thumbnail: any;
+  screenshot: any;
+}) => {
+  const errorMessage = `deleteDataSuccessful: Deletion failed for attachment ${attachment.path}`;
+  // eslint-disable-next-line @typescript-eslint/no-misused-promises
+  return fse.pathExists(attachment.path, (err, exists) => {
+    if (err) {
+      return Promise.reject(new Error(`${errorMessage} ${err}`));
+    }
+
+    // Note we want to confirm the path no longer exists
+    if (exists) {
+      return Promise.reject(errorMessage);
+    }
+
+    window.log.debug(`deleteDataSuccessful: Deletion succeeded for attachment ${attachment.path}`);
+    return true;
+  });
 };
 
 type CaptureDimensionType = { contentType: string; path: string };