fix(app): let removeAttachment propagate file-store errors instead of swallowing them

The previous idempotency change wrapped fileUploadService.deleteFile in a
catch-all that dropped the metadata doc even on genuine failures (S3/GridFS
outage, permission errors). Because removeAttachment is shared by the
attachment delete API and profile-image deletion, this turned real failures
into silent successes and could strand unreferenceable orphan blobs.

The bulk-export cleanup race is already prevented structurally by the
per-attachment owner dedup in the cleanup cron, so the file-store catch is no
longer needed for that path. Keep the "metadata already gone" no-op (the cron
relies on it to self-heal zombie records), but remove the deleteFile try/catch
so genuine errors propagate to the caller.

Update attachment.spec.ts to lock in the propagation contract.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

apps/app/src/server/service/attachment.spec.ts

@@ -2,11 +2,12 @@ import type Crowi from '../crowi';
 import { Attachment } from '../models/attachment';
 import { AttachmentService } from './attachment';
 
-// Locks down the idempotent contract that the bulk-export cleanup cron relies
-// on (page-bulk-export-job-clean-up-cron.ts): removing an attachment whose
-// metadata doc is already gone must resolve without throwing, otherwise the
-// surrounding Promise.allSettled rejects the cleanup and leaves the parent job
-// record undeleted as a zombie.
+// Locks down two contracts of removeAttachment:
+// 1. Missing metadata doc is a no-op (the bulk-export cleanup cron relies on
+//    this to self-heal zombie job records without throwing).
+// 2. A genuine file-store failure propagates, so callers like the attachment
+//    delete API surface it instead of dropping the metadata doc and stranding
+//    an orphan blob.
 describe('AttachmentService.removeAttachment', () => {
   test('should resolve without throwing when the attachment is already gone', async () => {
     const findByIdSpy = vi
@@ -27,7 +28,7 @@ describe('AttachmentService.removeAttachment', () => {
     findByIdSpy.mockRestore();
   });
 
-  test('should still drop the metadata doc when the underlying file delete throws (race with concurrent remover)', async () => {
+  test('should propagate the error and not drop the metadata doc when the file store fails', async () => {
     const attachmentRemove = vi.fn().mockResolvedValue(undefined);
     const fakeAttachment = { _id: 'some-id', remove: attachmentRemove };
     const findByIdSpy = vi
@@ -36,19 +37,20 @@ describe('AttachmentService.removeAttachment', () => {
       .mockResolvedValue(fakeAttachment as any);
     const deleteFile = vi
       .fn()
-      .mockRejectedValue(new Error('File not found for id some-id'));
+      .mockRejectedValue(new Error('S3 is temporarily unavailable'));
     const crowi = {
       fileUploadService: { deleteFile },
-      detachHandlers: [],
     } as unknown as Crowi;
     const service = new AttachmentService(crowi);
-    // detachHandlers lives on the service instance, not crowi
     service.detachHandlers = [];
 
-    await expect(service.removeAttachment('some-id')).resolves.toBeUndefined();
+    await expect(service.removeAttachment('some-id')).rejects.toThrow(
+      'S3 is temporarily unavailable',
+    );
 
     expect(deleteFile).toHaveBeenCalledTimes(1);
-    expect(attachmentRemove).toHaveBeenCalledTimes(1);
+    // metadata doc must survive so the blob stays referenceable for retry
+    expect(attachmentRemove).not.toHaveBeenCalled();
     findByIdSpy.mockRestore();
   });
 });

apps/app/src/server/service/attachment.ts

@@ -147,11 +147,11 @@ export class AttachmentService implements IAttachmentService {
     const { fileUploadService } = this.crowi;
     const attachment = await Attachment.findById(attachmentId);
 
-    // No-op when the attachment is already gone. Two flows rely on this:
-    // (1) bulk-export cleanup may double-call this for sibling jobs that share
-    //     an attachment (the per-job loop runs concurrently), and (2) GridFS
-    //     chunks can be removed out-of-band, leaving a dangling metadata doc.
-    // Throwing here would re-surface those as cron errors on every tick.
+    // No-op when the metadata doc is already gone. The bulk-export cleanup cron
+    // relies on this to self-heal: a job whose attachment was already removed by
+    // a previous tick (or by a concurrent remover that won an unsynchronized
+    // cross-process race) resolves cleanly here and gets deleted instead of
+    // lingering as a zombie record. Throwing would re-surface it every tick.
     if (attachment == null) {
       logger.debug(
         `removeAttachment: attachment already gone, skipping: ${attachmentId}`,
@@ -159,17 +159,13 @@ export class AttachmentService implements IAttachmentService {
       return;
     }
 
-    try {
-      await fileUploadService.deleteFile(attachment);
-    } catch (err) {
-      // Same idempotency contract applies to the underlying file store: if the
-      // chunk was already deleted (e.g. a racing remove call), treat it as a
-      // no-op and continue to drop the metadata doc.
-      logger.warn(
-        `removeAttachment: deleteFile failed for ${attachmentId}, continuing to drop metadata`,
-        err,
-      );
-    }
+    // Intentionally NOT swallowing deleteFile errors. A genuine file-store
+    // failure (S3/GridFS outage, permission error) must propagate so callers
+    // such as the attachment delete API surface it instead of dropping the
+    // metadata doc and stranding an unreferenceable orphan blob. "File already
+    // gone" is not an error path here: the underlying stores already no-op it
+    // (see gridfs deleteFile, which warns and returns when the file is missing).
+    await fileUploadService.deleteFile(attachment);
     await attachment.remove();
 
     const detachedHandlerPromises = this.detachHandlers.map((handler) => {