Merge pull request #33172 from overleaf/bg-fix-project-upload-unlinks

fix: use fsPromises ins ProjectUploadController async functions for consistency
GitOrigin-RevId: beb858d9b6cf50431fb14626dfd7cddfaf093882

services/web/app/src/Features/Uploads/ProjectUploadController.mjs

@@ -84,7 +84,9 @@ async function uploadFile(req, res, next) {
   const userId = SessionManager.getLoggedInUserId(req.session)
   let { folder_id: folderId } = req.query
   if (name == null || name.length === 0 || name.length > 150) {
-    fs.unlink(path, function () {})
+    await fsPromises.unlink(path).catch(unlinkErr => {
+      logger.warn({ err: unlinkErr, path }, 'error unlinking uploaded file')
+    })
     return res.status(422).json({
       success: false,
       error: 'invalid_filename',
@@ -109,7 +111,9 @@ async function uploadFile(req, res, next) {
       folderId = lastFolder._id
     }
   } catch (error) {
-    fs.unlink(path, function () {})
+    await fsPromises.unlink(path).catch(unlinkErr => {
+      logger.warn({ err: unlinkErr, path }, 'error unlinking uploaded file')
+    })
     throw error
   }
 
@@ -195,7 +199,12 @@ async function importDocx(req, res, next) {
       await ProjectOptionsHandler.promises.setCompiler(project._id, 'lualatex')
       res.json({ success: true, project_id: project._id })
     } finally {
-      await fsPromises.unlink(archivePath).catch(() => {})
+      await fsPromises.unlink(archivePath).catch(unlinkErr => {
+        logger.warn(
+          { err: unlinkErr, archivePath },
+          'error unlinking after docx conversion'
+        )
+      })
     }
   } catch (error) {
     logger.error({ error }, 'error importing docx file')
@@ -213,7 +222,12 @@ async function importDocx(req, res, next) {
       error: req.i18n.translate('upload_failed'),
     })
   } finally {
-    await fsPromises.unlink(path).catch(() => {})
+    await fsPromises.unlink(path).catch(unlinkErr => {
+      logger.warn(
+        { err: unlinkErr, path },
+        'error unlinking uploaded file in importDocx'
+      )
+    })
   }
 }
 

services/web/test/unit/src/Uploads/ProjectUploadController.test.mjs

@@ -112,7 +112,7 @@ describe('ProjectUploadController', function () {
       })
     )
 
-    vi.doMock('fs', () => ({
+    vi.doMock('node:fs', () => ({
       default: (ctx.fs = {}),
     }))
 
@@ -141,6 +141,7 @@ describe('ProjectUploadController', function () {
       ctx.project = { _id: (ctx.project_id = 'project-id-123') }
 
       ctx.fs.unlink = sinon.stub()
+      ctx.fsPromises.unlink = sinon.stub().resolves()
     })
 
     describe('successfully', function () {
@@ -200,6 +201,10 @@ describe('ProjectUploadController', function () {
           JSON.stringify({ success: false, error: 'upload_failed' })
         )
       })
+
+      it('should remove the uploaded file', function (ctx) {
+        ctx.fs.unlink.calledWith(ctx.path).should.equal(true)
+      })
     })
 
     describe('when ProjectUploadManager.createProjectFromZipArchive reports the file as invalid', function () {
@@ -224,6 +229,10 @@ describe('ProjectUploadController', function () {
       it("should return an 'unprocessable entity' status code", function (ctx) {
         expect(ctx.res.statusCode).to.equal(422)
       })
+
+      it('should remove the uploaded file', function (ctx) {
+        ctx.fs.unlink.calledWith(ctx.path).should.equal(true)
+      })
     })
   })
 
@@ -247,6 +256,7 @@ describe('ProjectUploadController', function () {
       ctx.req.params = { Project_id: ctx.project_id }
       ctx.req.query = { folder_id: ctx.folder_id }
       ctx.fs.unlink = sinon.stub()
+      ctx.fsPromises.unlink = sinon.stub().resolves()
     })
 
     describe('successfully', function () {
@@ -361,7 +371,7 @@ describe('ProjectUploadController', function () {
       })
 
       it('should unlink the file', function (ctx) {
-        ctx.fs.unlink.should.have.been.calledWith(ctx.path)
+        ctx.fsPromises.unlink.should.have.been.calledWith(ctx.path)
       })
 
       it('should call next with the error', function (ctx) {
@@ -384,6 +394,10 @@ describe('ProjectUploadController', function () {
           })
         )
       })
+
+      it('should remove the uploaded file', function (ctx) {
+        ctx.fs.unlink.calledWith(ctx.path).should.equal(true)
+      })
     })
 
     describe('when FileSystemImportManager.addEntity returns a too many files error', function () {
@@ -402,6 +416,10 @@ describe('ProjectUploadController', function () {
           })
         )
       })
+
+      it('should remove the uploaded file', function (ctx) {
+        ctx.fs.unlink.calledWith(ctx.path).should.equal(true)
+      })
     })
 
     describe('with an invalid filename', function () {
@@ -420,7 +438,7 @@ describe('ProjectUploadController', function () {
       })
 
       it('should remove the uploaded file', function (ctx) {
-        ctx.fs.unlink.calledWith(ctx.path).should.equal(true)
+        ctx.fsPromises.unlink.calledWith(ctx.path).should.equal(true)
       })
     })
 
@@ -440,7 +458,7 @@ describe('ProjectUploadController', function () {
       })
 
       it('should remove the uploaded file', function (ctx) {
-        ctx.fs.unlink.calledWith(ctx.path).should.equal(true)
+        ctx.fsPromises.unlink.calledWith(ctx.path).should.equal(true)
       })
     })
   })