fix: Make file move and DB update atomic for submissions

app/src/forms/file/service.js

@@ -2,6 +2,7 @@
 const uuid = require('uuid');
 const path = require('path');
 const { FileStorage } = require('../common/models');
+const log = require('../../components/log')(module.filename);
 const storageService = require('./storage/storageService');
 
 const PERMANENT_STORAGE = config.get('files.permanent');
@@ -144,18 +145,35 @@
    * @param {string} updatedBy the user who is saving the submission.
    */
   moveSubmissionFile: async (submissionId, fileStorage, updatedBy) => {
-    // Move the file from its current directory to the "submissions" subdirectory.
-    const path = await storageService.move(fileStorage, 'submissions', submissionId);
-    if (!path) {
-      throw new Error('Error moving files for submission');
-    }
+    let trx;
+    try {
+      trx = await FileStorage.startTransaction();
+
+      // Move the file from its current directory to the "submissions" subdirectory.
+      const path = await storageService.move(fileStorage, 'submissions', submissionId);
+      if (!path) {
+        throw new Error('Error moving files for submission');
+      }
+
+      await FileStorage.query(trx).patchAndFetchById(fileStorage.id, {
+        formSubmissionId: submissionId,
+        path: path,
+        storage: PERMANENT_STORAGE,
+        updatedBy: updatedBy,
+      });
 
-    await FileStorage.query().patchAndFetchById(fileStorage.id, {
-      formSubmissionId: submissionId,
-      path: path,
-      storage: PERMANENT_STORAGE,
-      updatedBy: updatedBy,
-    });
+      await trx.commit();
+
+      // Only after successful commit, remove the original object from storage.
+      try {
+        await storageService.delete(fileStorage);
+      } catch (delErr) {
+        log.error(`Failed to delete original file after move for file ${fileStorage.id}`, delErr);
+      }
+    } catch (err) {
+      if (trx) await trx.rollback();
+      throw err;
+    }
   },
 
   moveSubmissionFiles: async (submissionId, currentUser) => {

app/src/forms/file/storage/objectStorageService.js

@@ -168,18 +168,10 @@ class ObjectStorageService {
 
   async move(fileStorage, ...subdirs) {
     try {
-      const sourcePath = fileStorage.path;
       const file = await this.copyFile(fileStorage, ...subdirs);
       if (file) {
         // this doesn't return the new key/path, but we can build it
         const newPath = this._join(this._key, ...subdirs, fileStorage.id);
-        // now delete original...
-        const params = {
-          Bucket: this._bucket,
-          Key: sourcePath,
-        };
-
-        await this._s3.send(new DeleteObjectCommand(params));
 
         return newPath;
       }

app/src/forms/submission/updateService.js

@@ -47,9 +47,14 @@ const service = {
       const fileStorage = await fileService.read(fileId);
       if (!fileStorage.formSubmissionId) {
         await FileStorage.query(trx).patchAndFetchById(fileId, { formSubmissionId: id, updatedBy: user.usernameIdp });
-        fileService.moveSubmissionFile(id, fileStorage, user.usernameIdp).catch((error) => {
-          log.error('Error moving file', error);
-        });
+
+        // move file and update storage/path atomically to ensure we don't swallow errors
+        try {
+          await fileService.moveSubmissionFile(id, fileStorage, user.usernameIdp);
+        } catch (error) {
+          log.error('Error moving file', { fileId, error: error?.message || error });
+          throw error;
+        }
       }
     }
   },

app/tests/unit/forms/file/service.spec.js

@@ -271,6 +271,120 @@ describe('create', () => {
     });
   });
 
+  describe('moveSubmissionFile', () => {
+    it('should update file path after S3 move succeeds', async () => {
+      const submissionId = 'test-submission-id';
+      const updatedBy = 'test-user';
+      const fileStorage = {
+        id: 'test-file-id',
+        path: 'chefs/prod/uploads/test-file-id',
+        formSubmissionId: null,
+        storage: 'objectStorage',
+      };
+      const newPath = `chefs/prod/submissions/${submissionId}/test-file-id`;
+
+      // Mock storageService.move to return new path
+      storageService.move = jest.fn().mockResolvedValue(newPath);
+
+      // Mock FileStorage.query().patchAndFetchById to return updated record
+      FileStorage.query = jest.fn().mockReturnValue({
+        patchAndFetchById: jest.fn().mockResolvedValue({
+          ...fileStorage,
+          path: newPath,
+          formSubmissionId: submissionId,
+        }),
+      });
+
+      // Mock storageService.delete (called after DB commit)
+      storageService.delete = jest.fn().mockResolvedValue(true);
+
+      await service.moveSubmissionFile(submissionId, fileStorage, updatedBy);
+
+      expect(storageService.move).toHaveBeenCalledWith(fileStorage, 'submissions', submissionId);
+      expect(FileStorage.query().patchAndFetchById).toHaveBeenCalledWith(
+        fileStorage.id,
+        expect.objectContaining({
+          path: newPath,
+          formSubmissionId: submissionId,
+        })
+      );
+      expect(storageService.delete).toHaveBeenCalledWith(fileStorage);
+    });
+
+    it('should not delete original file if DB update fails', async () => {
+      const submissionId = 'test-submission-id';
+      const updatedBy = 'test-user';
+      const fileStorage = {
+        id: 'test-file-id',
+        path: 'chefs/prod/uploads/test-file-id',
+      };
+      const newPath = `chefs/prod/submissions/${submissionId}/test-file-id`;
+
+      // Mock storageService.move to return new path (S3 copy succeeds)
+      storageService.move = jest.fn().mockResolvedValue(newPath);
+
+      // Mock transaction
+      const mockTrx = {
+        commit: jest.fn().mockResolvedValue(),
+        rollback: jest.fn().mockResolvedValue(),
+      };
+
+      FileStorage.startTransaction = jest.fn().mockResolvedValue(mockTrx);
+
+      // Mock DB update to fail
+      FileStorage.query = jest.fn().mockReturnValue({
+        patchAndFetchById: jest.fn().mockRejectedValue(new Error('DB update failed')),
+      });
+
+      storageService.delete = jest.fn().mockResolvedValue(true);
+
+      await expect(service.moveSubmissionFile(submissionId, fileStorage, updatedBy)).rejects.toThrow('DB update failed');
+
+      // delete should NOT have been called
+      expect(storageService.delete).not.toHaveBeenCalled();
+      // rollback should have been called
+      expect(mockTrx.rollback).toHaveBeenCalled();
+    });
+
+    it('should log error but not fail if S3 delete fails after DB commit', async () => {
+      const submissionId = 'test-submission-id';
+      const updatedBy = 'test-user';
+      const fileStorage = {
+        id: 'test-file-id',
+        path: 'chefs/prod/uploads/test-file-id',
+      };
+      const newPath = `chefs/prod/submissions/${submissionId}/test-file-id`;
+
+      storageService.move = jest.fn().mockResolvedValue(newPath);
+
+      const mockTrx = {
+        commit: jest.fn().mockResolvedValue(),
+        rollback: jest.fn().mockResolvedValue(),
+      };
+
+      FileStorage.startTransaction = jest.fn().mockResolvedValue(mockTrx);
+
+      FileStorage.query = jest.fn().mockReturnValue({
+        patchAndFetchById: jest.fn().mockResolvedValue({
+          ...fileStorage,
+          path: newPath,
+          formSubmissionId: submissionId,
+        }),
+      });
+
+      // Mock delete to fail after DB commit
+      storageService.delete = jest.fn().mockRejectedValue(new Error('S3 delete failed'));
+
+      // Should NOT throw even though delete failed
+      await expect(service.moveSubmissionFile(submissionId, fileStorage, updatedBy)).resolves.not.toThrow();
+
+      // Commit should have succeeded
+      expect(mockTrx.commit).toHaveBeenCalled();
+      // Delete was attempted
+      expect(storageService.delete).toHaveBeenCalled();
+    });
+  });
+
   describe('clone coverage', () => {
     it('clone: should create a new file record with new ID', async () => {
       FileStorage.startTransaction = jest.fn().mockResolvedValue(mockTransaction);

app/tests/unit/forms/submission/updateService.spec.js

@@ -239,6 +239,31 @@ describe('updateService', () => {
       expect(patchAndFetchById).not.toHaveBeenCalled();
       expect(fileService.moveSubmissionFile).not.toHaveBeenCalled();
     });
+
+    it('should throw error from moveSubmissionFile instead of silently catching', async () => {
+      const fileIds = ['f1'];
+      const data = {};
+      const id = 557;
+      const user = { usernameIdp: 'uploader' };
+      const trx = {};
+
+      const submissionService = {
+        _findFileIds: jest.fn().mockReturnValue(fileIds),
+      };
+
+      fileService.read.mockResolvedValue({ id: 'f1', formSubmissionId: null });
+
+      const patchAndFetchById = jest.fn().mockResolvedValue({});
+      FileStorage.query.mockReturnValue({ patchAndFetchById });
+
+      // Mock moveSubmissionFile to throw
+      fileService.moveSubmissionFile.mockRejectedValue(new Error('File move failed'));
+
+      // Should throw, not silently catch
+      await expect(updateService._handleFileUploads(submissionService, data, id, user, trx)).rejects.toThrow('File move failed');
+
+      expect(fileService.moveSubmissionFile).toHaveBeenCalledWith(id, { id: 'f1', formSubmissionId: null }, 'uploader');
+    });
   });
 
   describe('update', () => {