Request backfill and skip queueing for undownloadable attachments

ts/jobs/AttachmentDownloadManager.preload.ts

@@ -42,6 +42,7 @@ import {
   isIncremental,
   hasRequiredInformationForRemoteBackup,
   isDownloadableFromTransitTier,
+  isDownloadable,
 } from '../util/Attachment.std.ts';
 import type { ReadonlyMessageAttributesType } from '../model-types.d.ts';
 import { backupsService } from '../services/backups/index.preload.ts';
@@ -344,6 +345,37 @@ export class AttachmentDownloadManager extends JobManager<CoreAttachmentDownload
       }
     }
 
+    // If there's insufficient information on the attachment (e.g. it was an imported
+    // errored attachment), we may not be able to identify it via
+    // `addAttachmentToMessage`, so instead we immediately request backfill if we can and
+    // skip queueing.
+    if (
+      isManualDownload &&
+      !isDownloadable(attachment, { hasMediaBackups: true })
+    ) {
+      const message = await getMessageById(messageId);
+      if (!message) {
+        log.warn(`${logId}: message not found, skipping queueing`);
+        return attachment;
+      }
+
+      if (
+        isBackfillable({
+          attachment,
+          attachmentType,
+          isStory: message.attributes.type === 'story',
+        })
+      ) {
+        log.info(`${logId}: Attachment is undownloadable, requesting backfill`);
+        await AttachmentDownloadManager.requestBackfill(message.attributes);
+        return { ...attachment, pending: true };
+      }
+      log.warn(
+        `${logId}: Attachment is undownloadable and unbackfillable; not queueing job`
+      );
+      return attachment;
+    }
+
     const parseResult = safeParsePartial(coreAttachmentDownloadJobSchema, {
       attachment,
       attachmentType,

ts/test-electron/services/AttachmentDownloadManager_test.preload.ts

@@ -215,7 +215,7 @@ describe('AttachmentDownloadManager', () => {
         forceSave: true,
       }
     );
-    await downloadManager?.addJob({
+    return downloadManager?.addJob({
       urgency,
       ...job,
       isManualDownload: Boolean(job.isManualDownload),
@@ -825,6 +825,96 @@ describe('AttachmentDownloadManager', () => {
       assert.strictEqual(savedJobs.length, 0);
     });
   });
+
+  describe('attachments with no download information', () => {
+    // An attachment with no key/digest/cdn/localKey info (e.g. an imported errored
+    // attachment) is not downloadable from any tier and can't be reliably identified
+    // via addAttachmentToMessage, so addJob should request backfill immediately rather
+    // than queueing a job that could never succeed.
+    const noInfoOverrides: Partial<AttachmentType> = {
+      key: undefined,
+      plaintextHash: undefined,
+      digest: undefined,
+      cdnKey: undefined,
+      cdnNumber: undefined,
+      localKey: undefined,
+      path: undefined,
+    };
+
+    beforeEach(() => {
+      sandbox
+        .stub(window.ConversationController, 'areWePrimaryDevice')
+        .returns(false);
+    });
+
+    it('requests backfill and skips queueing for manual downloads', async () => {
+      const requestBackfill = sandbox
+        .stub(AttachmentDownloadManager, 'requestBackfill')
+        .resolves();
+      const job = composeJob({
+        messageId: 'messageId',
+        receivedAt: Date.now(),
+        attachmentOverrides: noInfoOverrides,
+        jobOverrides: { isManualDownload: true },
+      });
+
+      const result = await addJob(job, AttachmentDownloadUrgency.STANDARD);
+
+      assert.strictEqual(requestBackfill.callCount, 1);
+      assert.strictEqual(requestBackfill.getCall(0).args[0].id, job.messageId);
+      // Returned attachment is marked pending so the caller can reflect the
+      // in-flight backfill on the message.
+      assert.strictEqual(result?.pending, true);
+
+      const savedJobs = await DataWriter.getNextAttachmentDownloadJobs({
+        limit: 100,
+      });
+      assert.strictEqual(savedJobs.length, 0);
+    });
+
+    it('does not request backfill for automatic downloads', async () => {
+      const requestBackfill = sandbox
+        .stub(AttachmentDownloadManager, 'requestBackfill')
+        .resolves();
+      const job = composeJob({
+        messageId: 'messageId',
+        receivedAt: Date.now(),
+        attachmentOverrides: noInfoOverrides,
+        jobOverrides: { isManualDownload: false },
+      });
+
+      await addJob(job, AttachmentDownloadUrgency.STANDARD);
+
+      assert.strictEqual(requestBackfill.callCount, 0);
+
+      const savedJobs = await DataWriter.getNextAttachmentDownloadJobs({
+        limit: 100,
+      });
+      assert.strictEqual(savedJobs.length, 1);
+    });
+
+    it('does not request backfill or queue when not backfillable', async () => {
+      const requestBackfill = sandbox
+        .stub(AttachmentDownloadManager, 'requestBackfill')
+        .resolves();
+      const job = composeJob({
+        messageId: 'messageId',
+        receivedAt: Date.now(),
+        attachmentOverrides: { ...noInfoOverrides, backfillError: true },
+        jobOverrides: { isManualDownload: true },
+      });
+
+      const result = await addJob(job, AttachmentDownloadUrgency.STANDARD);
+
+      assert.strictEqual(requestBackfill.callCount, 0);
+      assert.notStrictEqual(result?.pending, true);
+
+      const savedJobs = await DataWriter.getNextAttachmentDownloadJobs({
+        limit: 100,
+      });
+      assert.strictEqual(savedJobs.length, 0);
+    });
+  });
 });
 describe('AttachmentDownloadManager.runDownloadAttachmentJob', () => {
   let sandbox: sinon.SinonSandbox;

ts/util/Attachment.std.ts

@@ -837,6 +837,7 @@ export function isDownloadable(
   { hasMediaBackups }: { hasMediaBackups: boolean }
 ): boolean {
   return (
+    hasRequiredInformationForLocalBackup(attachment) ||
     isDownloadableFromTransitTier(attachment) ||
     isDownloadableFromBackupTier(attachment, { hasMediaBackups })
   );