Avoid orphaning quoted attachment thumbnails

Co-authored-by: trevor-signal <[email protected]>

Author: automated-signal

ts/jobs/AttachmentDownloadManager.preload.ts

@@ -19,6 +19,7 @@ import {
   isIncrementalMacVerificationError,
 } from '../util/downloadAttachment.preload.js';
 import {
+  deleteAttachmentData as doDeleteAttachmentData,
   deleteDownloadData as doDeleteDownloadData,
   processNewAttachment as doProcessNewAttachment,
 } from '../util/migrations.preload.js';
@@ -39,6 +40,7 @@ import {
   getUndownloadedAttachmentSignature,
   isIncremental,
   hasRequiredInformationForRemoteBackup,
+  deleteAllAttachmentFilesOnDisk,
 } from '../util/Attachment.std.js';
 import type { ReadonlyMessageAttributesType } from '../model-types.d.ts';
 import { backupsService } from '../services/backups/index.preload.js';
@@ -48,7 +50,10 @@ import {
   getMaximumIncomingAttachmentSizeInKb,
   getMaximumIncomingTextAttachmentSizeInKb,
 } from '../types/AttachmentSize.std.js';
-import { addAttachmentToMessage } from '../messageModifiers/AttachmentDownloads.preload.js';
+import {
+  addAttachmentToMessage,
+  AttachmentNotNeededForMessageError,
+} from '../messageModifiers/AttachmentDownloads.preload.js';
 import * as Errors from '../types/errors.std.js';
 import { redactGenericText } from '../util/privacy.node.js';
 import {
@@ -491,6 +496,7 @@ export class AttachmentDownloadManager extends JobManager<CoreAttachmentDownload
 }
 
 type DependenciesType = {
+  deleteAttachmentData: typeof doDeleteAttachmentData;
   deleteDownloadData: typeof doDeleteDownloadData;
   downloadAttachment: typeof downloadAttachmentUtil;
   processNewAttachment: typeof doProcessNewAttachment;
@@ -503,6 +509,7 @@ export async function runDownloadAttachmentJob({
   isLastAttempt,
   options,
   dependencies = {
+    deleteAttachmentData: doDeleteAttachmentData,
     deleteDownloadData: doDeleteDownloadData,
     downloadAttachment: downloadAttachmentUtil,
     processNewAttachment: doProcessNewAttachment,
@@ -567,6 +574,23 @@ export async function runDownloadAttachmentJob({
       return { status: 'finished' };
     }
 
+    if (error instanceof AttachmentNotNeededForMessageError) {
+      if (job.attachmentType === 'quote') {
+        log.info(
+          `${logId}: quote attachment not needed, likely the thumbnail was already copied from original message`
+        );
+      } else {
+        log.error(`${logId}: attachment not found on message`);
+      }
+
+      await deleteAllAttachmentFilesOnDisk({
+        deleteDownloadOnDisk: dependencies.deleteDownloadData,
+        deleteAttachmentOnDisk: dependencies.deleteAttachmentData,
+      })(error.attachment);
+
+      return { status: 'finished' };
+    }
+
     if (error instanceof AttachmentSizeError) {
       log.info(`${logId}: Attachment is too big.`);
       await addAttachmentToMessage(
@@ -884,6 +908,11 @@ export async function runDownloadAttachmentJobInner({
     if (isAbortError(error)) {
       throw error;
     }
+
+    if (error instanceof AttachmentNotNeededForMessageError) {
+      throw error;
+    }
+
     if (mightHaveBackupThumbnailToDownload && !attemptBackupThumbnailFirst) {
       log.error(
         `${logId}: failed to download fullsize attachment, falling back to backup thumbnail`,

ts/messageModifiers/AttachmentDownloads.preload.ts

@@ -69,6 +69,12 @@ export async function markAttachmentAsCorrupted(
   });
 }
 
+export class AttachmentNotNeededForMessageError extends Error {
+  constructor(public readonly attachment: AttachmentType) {
+    super('AttachmentNotNeededForMessageError');
+  }
+}
+
 export async function addAttachmentToMessage(
   messageId: string,
   attachment: AttachmentType,
@@ -167,14 +173,15 @@ export async function addAttachmentToMessage(
         await deleteAttachmentData(attachment.path);
       }
       if (!handledAnywhere) {
-        log.warn(
-          `${logPrefix}: Long message attachment found no matching place to apply`
-        );
+        // eslint-disable-next-line no-unsafe-finally
+        throw new AttachmentNotNeededForMessageError(attachment);
       }
     }
     return;
   }
 
+  let foundPlaceForAttachment = false;
+
   const maybeReplaceAttachment = (existing: AttachmentType): AttachmentType => {
     if (isDownloaded(existing)) {
       return existing;
@@ -184,13 +191,13 @@ export async function addAttachmentToMessage(
       return existing;
     }
 
+    foundPlaceForAttachment = true;
     return attachment;
   };
 
   if (type === 'attachment') {
     const attachments = message.get('attachments');
 
-    let handledAnywhere = false;
     let handledInEditHistory = false;
 
     const editHistory = message.get('editHistory');
@@ -207,7 +214,6 @@ export async function addAttachmentToMessage(
           attachments: edit.attachments.map(item => {
             const newItem = maybeReplaceAttachment(item);
             handledInEditHistory ||= item !== newItem;
-            handledAnywhere ||= handledInEditHistory;
             return newItem;
           }),
         };
@@ -220,18 +226,12 @@ export async function addAttachmentToMessage(
 
     if (attachments) {
       message.set({
-        attachments: attachments.map(item => {
-          const newItem = maybeReplaceAttachment(item);
-          handledAnywhere ||= item !== newItem;
-          return newItem;
-        }),
+        attachments: attachments.map(maybeReplaceAttachment),
       });
     }
 
-    if (!handledAnywhere) {
-      log.warn(
-        `${logPrefix}: 'attachment' type found no matching place to apply`
-      );
+    if (!foundPlaceForAttachment) {
+      throw new AttachmentNotNeededForMessageError(attachment);
     }
 
     return;
@@ -243,7 +243,7 @@ export async function addAttachmentToMessage(
     let handledInEditHistory = false;
 
     const editHistory = message.get('editHistory');
-    if (preview && editHistory) {
+    if (editHistory) {
       const newEditHistory = editHistory.map(edit => {
         if (!edit.preview) {
           return edit;
@@ -282,6 +282,10 @@ export async function addAttachmentToMessage(
       });
     }
 
+    if (!foundPlaceForAttachment) {
+      throw new AttachmentNotNeededForMessageError(attachment);
+    }
+
     return;
   }
 
@@ -290,7 +294,6 @@ export async function addAttachmentToMessage(
     if (!contacts?.length) {
       throw new Error(`${logPrefix}: no contacts, cannot add attachment!`);
     }
-    let handled = false;
 
     const newContacts = contacts.map(contact => {
       if (!contact.avatar?.avatar) {
@@ -301,7 +304,6 @@ export async function addAttachmentToMessage(
 
       const newAttachment = maybeReplaceAttachment(existingAttachment);
       if (existingAttachment !== newAttachment) {
-        handled = true;
         return {
           ...contact,
           avatar: { ...contact.avatar, avatar: newAttachment },
@@ -310,10 +312,8 @@ export async function addAttachmentToMessage(
       return contact;
     });
 
-    if (!handled) {
-      throw new Error(
-        `${logPrefix}: Couldn't find matching contact with avatar attachment for message`
-      );
+    if (!foundPlaceForAttachment) {
+      throw new AttachmentNotNeededForMessageError(attachment);
     }
 
     message.set({ contact: newContacts });
@@ -374,6 +374,10 @@ export async function addAttachmentToMessage(
       message.set({ quote: newQuote });
     }
 
+    if (!foundPlaceForAttachment) {
+      throw new AttachmentNotNeededForMessageError(attachment);
+    }
+
     return;
   }
 
@@ -389,6 +393,11 @@ export async function addAttachmentToMessage(
         data: sticker.data ? maybeReplaceAttachment(sticker.data) : attachment,
       },
     });
+
+    if (!foundPlaceForAttachment) {
+      throw new AttachmentNotNeededForMessageError(attachment);
+    }
+
     return;
   }