Merge pull request #34992 from overleaf/em-dropbox-rate-limits

[third-party-datastore] Handle Dropbox rate limits and per-project file-count limits during sync

GitOrigin-RevId: 0a3c4a06b6444a70394c35cb404c566ef7e25cef

services/web/app/src/Features/Cooldown/CooldownManager.mjs

@@ -1,31 +0,0 @@
-import RedisWrapper from '../../infrastructure/RedisWrapper.mjs'
-import logger from '@overleaf/logger'
-const rclient = RedisWrapper.client('cooldown')
-
-const COOLDOWN_IN_SECONDS = 60 * 10
-
-const CooldownManager = {
-  _buildKey(projectId) {
-    return `Cooldown:{${projectId}}`
-  },
-
-  async putProjectOnCooldown(projectId) {
-    logger.debug(
-      { projectId },
-      `[Cooldown] putting project on cooldown for ${COOLDOWN_IN_SECONDS} seconds`
-    )
-    await rclient.set(
-      CooldownManager._buildKey(projectId),
-      '1',
-      'EX',
-      COOLDOWN_IN_SECONDS
-    )
-  },
-
-  async isProjectOnCooldown(projectId) {
-    const result = await rclient.get(CooldownManager._buildKey(projectId))
-    return result === '1'
-  },
-}
-
-export default CooldownManager

services/web/app/src/Features/Editor/EditorHttpController.mjs

@@ -150,7 +150,7 @@ async function addDoc(req, res, next) {
     )
     res.json(doc)
   } catch (err) {
-    if (err.message === 'project_has_too_many_files') {
+    if (err instanceof Errors.TooManyFilesError) {
       res.status(400).json(
         req.i18n.translate('project_has_too_many_files_limit', {
           limit: Settings.maxEntitiesPerProject,
@@ -180,7 +180,7 @@ async function addFolder(req, res, next) {
     )
     res.json(doc)
   } catch (err) {
-    if (err.message === 'project_has_too_many_files') {
+    if (err instanceof Errors.TooManyFilesError) {
       res.status(400).json(
         req.i18n.translate('project_has_too_many_files_limit', {
           limit: Settings.maxEntitiesPerProject,

services/web/app/src/Features/Errors/ErrorController.mjs

@@ -10,6 +10,11 @@ import HttpErrorHandler from './HttpErrorHandler.mjs'
 import { plainTextResponse } from '../../infrastructure/Response.mjs'
 import { expressifyErrorHandler } from '@overleaf/promise-utils'
 
+// Keep in sync with the third-party-datastore service, which matches on this
+// code: services/third-party-datastore/app/js/Dropbox/DropboxPoller.js
+// (_isWebOverLimitError).
+const TOO_MANY_FILES_ERROR_CODE = 'project_has_too_many_files'
+
 function notFound(req, res) {
   res.status(404)
   res.render('general/404', { title: 'page_not_found' })
@@ -151,6 +156,10 @@ function handleApiError(err, req, res, next) {
   } else if (err instanceof InvalidRequestError) {
     req.logger.setLevel('warn')
     if (shouldSendErrorResponse) res.sendStatus(400)
+  } else if (err instanceof Errors.TooManyFilesError) {
+    req.logger.setLevel('warn')
+    if (shouldSendErrorResponse)
+      res.status(422).json({ code: TOO_MANY_FILES_ERROR_CODE })
   } else {
     req.logger.setLevel('error')
     if (shouldSendErrorResponse) res.sendStatus(500)

services/web/app/src/Features/Errors/Errors.js

@@ -50,6 +50,8 @@ class ServiceNotConfiguredError extends BackwardCompatibleError {}
 
 class TooManyRequestsError extends BackwardCompatibleError {}
 
+class TooManyFilesError extends BackwardCompatibleError {}
+
 class ResourceGoneError extends BackwardCompatibleError {}
 
 class DuplicateNameError extends OError {}
@@ -398,6 +400,7 @@ module.exports = {
   ForbiddenError,
   ServiceNotConfiguredError,
   TooManyRequestsError,
+  TooManyFilesError,
   ResourceGoneError,
   DuplicateNameError,
   InvalidNameError,

services/web/app/src/Features/LinkedFiles/LinkedFilesController.mjs

@@ -19,6 +19,7 @@ import LinkedFilesErrors from './LinkedFilesErrors.mjs'
 import {
   OutputFileFetchFailedError,
   FileTooLargeError,
+  TooManyFilesError,
 } from '../Errors/Errors.js'
 import Modules from '../../infrastructure/Modules.mjs'
 import { plainTextResponse } from '../../infrastructure/Response.mjs'
@@ -257,7 +258,7 @@ export default LinkedFilesController = {
     } else if (error instanceof FileCannotRefreshError) {
       res.status(400)
       plainTextResponse(res, 'This file cannot be refreshed')
-    } else if (error.message === 'project_has_too_many_files') {
+    } else if (error instanceof TooManyFilesError) {
       res.status(400)
       plainTextResponse(res, 'too many files')
     } else if (/\bECONNREFUSED\b/.test(error.message)) {

services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.mjs

@@ -5,7 +5,6 @@ import path from 'node:path'
 import mongodb from 'mongodb-legacy'
 import Settings from '@overleaf/settings'
 import OError from '@overleaf/o-error'
-import CooldownManager from '../Cooldown/CooldownManager.mjs'
 import Errors from '../Errors/Errors.js'
 import { Folder } from '../../models/Folder.mjs'
 import LockManager from '../../infrastructure/LockManager.mjs'
@@ -556,8 +555,7 @@ async function _putElement(project, folderId, element, type, userId) {
       { projectId: project._id },
       'project too big, stopping insertions'
     )
-    await CooldownManager.putProjectOnCooldown(project._id)
-    throw new Error('project_has_too_many_files')
+    throw new Errors.TooManyFilesError('project has too many files')
   }
 
   const { element: folder, path } = await ProjectLocator.promises.findElement({

services/web/app/src/Features/ThirdPartyDataStore/TpdsController.mjs

@@ -51,13 +51,7 @@ async function mergeUpdate(req, res) {
       source
     )
   } catch (err) {
-    if (err.name === 'TooManyRequestsError') {
-      logger.warn(
-        { err, userId, filePath },
-        'tpds update failed to be processed, too many requests'
-      )
-      return res.sendStatus(429)
-    } else if (err.message === 'project_has_too_many_files') {
+    if (err instanceof Errors.TooManyFilesError) {
       logger.warn(
         { err, userId, filePath },
         'tpds trying to append to project over file limit'
@@ -65,10 +59,8 @@ async function mergeUpdate(req, res) {
       await NotificationsBuilder.promises
         .tpdsFileLimit(userId)
         .create(projectName, projectId)
-      return res.sendStatus(400)
-    } else {
-      throw err
     }
+    throw err
   }
 
   if (metadata == null) {

services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.mjs

@@ -8,8 +8,6 @@ import ProjectGetter from '../Project/ProjectGetter.mjs'
 import ProjectHelper from '../Project/ProjectHelper.mjs'
 import ProjectRootDocManager from '../Project/ProjectRootDocManager.mjs'
 import FileTypeManager from '../Uploads/FileTypeManager.mjs'
-import CooldownManager from '../Cooldown/CooldownManager.mjs'
-import Errors from '../Errors/Errors.js'
 import Modules from '../../infrastructure/Modules.mjs'
 
 async function newUpdate(
@@ -25,13 +23,6 @@ async function newUpdate(
     return null
   }
 
-  const projectIsOnCooldown = await CooldownManager.isProjectOnCooldown(
-    project._id
-  )
-  if (projectIsOnCooldown) {
-    throw new Errors.TooManyRequestsError('project on cooldown')
-  }
-
   const shouldIgnore = FileTypeManager.shouldIgnore(path)
   if (shouldIgnore) {
     return null
@@ -174,13 +165,6 @@ async function createFolder(userId, projectId, projectName, path) {
     return null
   }
 
-  const projectIsOnCooldown = await CooldownManager.isProjectOnCooldown(
-    project._id
-  )
-  if (projectIsOnCooldown) {
-    throw new Errors.TooManyRequestsError('project on cooldown')
-  }
-
   const shouldIgnore = FileTypeManager.shouldIgnore(path)
   if (shouldIgnore) {
     return null

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

@@ -17,6 +17,7 @@ import {
   DuplicateNameError,
   FileTooLargeError,
   DocumentConversionError,
+  TooManyFilesError,
 } from '../Errors/Errors.js'
 import DocumentConversionManager from './DocumentConversionManager.mjs'
 import ProjectOptionsHandler from '../Project/ProjectOptionsHandler.mjs'
@@ -143,7 +144,7 @@ async function uploadFile(req, res, next) {
             success: false,
             error: 'duplicate_file_name',
           })
-        } else if (error.message === 'project_has_too_many_files') {
+        } else if (error instanceof TooManyFilesError) {
           return res.status(422).json({
             success: false,
             error: 'project_has_too_many_files',

services/web/test/unit/src/Cooldown/CooldownManager.sequential.test.mjs

@@ -1,50 +0,0 @@
-import { beforeEach, describe, expect, it } from 'vitest'
-import mongodb from 'mongodb-legacy'
-import CooldownManager from '../../../../app/src/Features/Cooldown/CooldownManager.mjs'
-import RedisWrapper from '../../../../app/src/infrastructure/RedisWrapper.mjs'
-
-const { ObjectId } = mongodb
-
-describe('CooldownManager', function () {
-  beforeEach(RedisWrapper.cleanupTestRedis)
-
-  beforeEach(function (ctx) {
-    ctx.project1Id = new ObjectId().toString()
-    ctx.project2Id = new ObjectId().toString()
-  })
-
-  describe('_buildKey', function () {
-    it('should build a properly formatted redis key', function () {
-      expect(CooldownManager._buildKey('ABC')).to.equal('Cooldown:{ABC}')
-    })
-  })
-
-  describe('isProjectOnCooldown', function () {
-    describe('when no project is on cooldown', function () {
-      it('returns false for project 1', async function (ctx) {
-        const result = await CooldownManager.isProjectOnCooldown(ctx.project1Id)
-        expect(result).to.be.false
-      })
-
-      it('returns false for project 2', async function (ctx) {
-        const result = await CooldownManager.isProjectOnCooldown(ctx.project2Id)
-        expect(result).to.be.false
-      })
-    })
-    describe('when project 1 is on cooldown', function () {
-      beforeEach(async function (ctx) {
-        await CooldownManager.putProjectOnCooldown(ctx.project1Id)
-      })
-
-      it('returns true for project 1', async function (ctx) {
-        const result = await CooldownManager.isProjectOnCooldown(ctx.project1Id)
-        expect(result).to.be.true
-      })
-
-      it('returns false for project 2', async function (ctx) {
-        const result = await CooldownManager.isProjectOnCooldown(ctx.project2Id)
-        expect(result).to.be.false
-      })
-    })
-  })
-})

services/web/test/unit/src/Editor/EditorHttpController.test.mjs

@@ -519,7 +519,7 @@ describe('EditorHttpController', function () {
 
       it('handle too many files', async function (ctx) {
         ctx.EditorController.promises.addDoc.rejects(
-          new Error('project_has_too_many_files')
+          new Errors.TooManyFilesError('project_has_too_many_files')
         )
         await new Promise(resolve => {
           ctx.res.callback = () => {
@@ -580,7 +580,7 @@ describe('EditorHttpController', function () {
       it('handle too many files', async function (ctx) {
         await new Promise(resolve => {
           ctx.EditorController.promises.addFolder.rejects(
-            new Error('project_has_too_many_files')
+            new Errors.TooManyFilesError('project_has_too_many_files')
           )
           ctx.res.callback = () => {
             expect(ctx.res.body).to.equal('"project_has_too_many_files_limit"')

services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandler.test.mjs

@@ -73,7 +73,6 @@ describe('ProjectEntityMongoUpdateHandler', function () {
     }
 
     ctx.Settings = { maxEntitiesPerProject: 100 }
-    ctx.CooldownManager = {}
     ctx.LockManager = {
       promises: {
         runWithLock: sinon.spy((namespace, id, runner) => runner()),
@@ -201,10 +200,6 @@ describe('ProjectEntityMongoUpdateHandler', function () {
       default: ctx.Settings,
     }))
 
-    vi.doMock('../../../../app/src/Features/Cooldown/CooldownManager', () => ({
-      default: ctx.CooldownManager,
-    }))
-
     vi.doMock('../../../../app/src/models/Folder', () => ({
       Folder: ctx.FolderModel,
     }))

services/web/test/unit/src/ThirdPartyDataStore/TpdsController.test.mjs

@@ -59,6 +59,10 @@ describe('TpdsController', function () {
       },
     }
 
+    vi.doMock('../../../../app/src/Features/Errors/Errors.js', () => ({
+      default: Errors,
+    }))
+
     vi.doMock(
       '../../../../app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler',
       () => ({
@@ -249,37 +253,16 @@ describe('TpdsController', function () {
       })
     })
 
-    it('should return a 400 error when the project is too big', async function (ctx) {
-      await new Promise(resolve => {
-        ctx.TpdsUpdateHandler.promises.newUpdate.rejects({
-          message: 'project_has_too_many_files',
-        })
-        const res = {
-          sendStatus: status => {
-            expect(status).to.equal(400)
-            ctx.NotificationsBuilder.promises.tpdsFileLimit.should.have.been.calledWith(
-              ctx.user_id
-            )
-            resolve()
-          },
-        }
-        ctx.TpdsController.mergeUpdate(ctx.req, res)
-      })
-    })
-
-    it('should return a 429 error when the update receiver fails due to too many requests error', async function (ctx) {
-      await new Promise(resolve => {
-        ctx.TpdsUpdateHandler.promises.newUpdate.rejects(
-          new Errors.TooManyRequestsError('project on cooldown')
-        )
-        const res = {
-          sendStatus: status => {
-            expect(status).to.equal(429)
-            resolve()
-          },
-        }
-        ctx.TpdsController.mergeUpdate(ctx.req, res)
-      })
+    it('should notify and rethrow when the project is too big', async function (ctx) {
+      const err = new Errors.TooManyFilesError('project has too many files')
+      ctx.TpdsUpdateHandler.promises.newUpdate.rejects(err)
+      const res = { json: sinon.stub() }
+      const next = sinon.stub()
+      await ctx.TpdsController.mergeUpdate(ctx.req, res, next)
+      expect(next).to.have.been.calledWith(err)
+      ctx.NotificationsBuilder.promises.tpdsFileLimit.should.have.been.calledWith(
+        ctx.user_id
+      )
     })
   })
 

services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandler.test.mjs

@@ -1,17 +1,12 @@
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 import sinon from 'sinon'
 import mongodb from 'mongodb-legacy'
-import Errors from '../../../../app/src/Features/Errors/Errors.js'
 
 const ObjectId = mongodb.ObjectId
 
 const MODULE_PATH =
   '../../../../app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.mjs'
 
-vi.mock('../../../../app/src/Features/Errors/Errors.js', () =>
-  vi.importActual('../../../../app/src/Features/Errors/Errors.js')
-)
-
 describe('TpdsUpdateHandler', function () {
   beforeEach(async function (ctx) {
     ctx.projectName = 'My recipes'
@@ -44,9 +39,6 @@ describe('TpdsUpdateHandler', function () {
       parentFolder_id: new ObjectId(),
     }
 
-    ctx.CooldownManager = {
-      isProjectOnCooldown: sinon.stub().resolves(false),
-    }
     ctx.FileTypeManager = {
       shouldIgnore: sinon.stub().returns(false),
     }
@@ -105,10 +97,6 @@ describe('TpdsUpdateHandler', function () {
       },
     }
 
-    vi.doMock('../../../../app/src/Features/Cooldown/CooldownManager', () => ({
-      default: ctx.CooldownManager,
-    }))
-
     vi.doMock('../../../../app/src/Features/Uploads/FileTypeManager', () => ({
       default: ctx.FileTypeManager,
     }))
@@ -255,24 +243,6 @@ describe('TpdsUpdateHandler', function () {
       expectUpdateNotProcessed()
       expectDropboxNotUnlinked()
     })
-
-    describe('update to a project on cooldown', async function () {
-      setupMatchingProjects(['active1'])
-      setupProjectOnCooldown()
-      beforeEach(async function (ctx) {
-        await expect(
-          ctx.TpdsUpdateHandler.promises.newUpdate(
-            ctx.userId,
-            '', // projectId
-            ctx.projectName,
-            ctx.path,
-            ctx.update,
-            ctx.source
-          )
-        ).to.be.rejectedWith(Errors.TooManyRequestsError)
-      })
-      expectUpdateNotProcessed()
-    })
   })
 
   describe('getting a file delete', function () {
@@ -453,22 +423,6 @@ describe('TpdsUpdateHandler', function () {
       expectFolderUpdateNotProcessed()
       expectDropboxUnlinked()
     })
-
-    describe('update to a project on cooldown', async function () {
-      setupMatchingProjects(['active1'])
-      setupProjectOnCooldown()
-      beforeEach(async function (ctx) {
-        await expect(
-          ctx.TpdsUpdateHandler.promises.createFolder(
-            ctx.userId,
-            ctx.projectId,
-            ctx.projectName,
-            ctx.path
-          )
-        ).to.be.rejectedWith(Errors.TooManyRequestsError)
-      })
-      expectFolderUpdateNotProcessed()
-    })
   })
 })
 
@@ -483,14 +437,6 @@ function setupMatchingProjects(projectKeys) {
   })
 }
 
-function setupProjectOnCooldown() {
-  beforeEach(function (ctx) {
-    ctx.CooldownManager.isProjectOnCooldown
-      .withArgs(ctx.projects.active1._id)
-      .resolves(true)
-  })
-}
-
 /* Test helpers */
 
 function receiveUpdate() {

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

@@ -13,6 +13,7 @@ import ArchiveErrors from '../../../../app/src/Features/Uploads/ArchiveErrors.mj
 import {
   FileTooLargeError,
   DocumentConversionError,
+  TooManyFilesError,
 } from '../../../../app/src/Features/Errors/Errors.js'
 
 vi.mock('../../../../app/src/Features/Errors/Errors.js', () =>
@@ -422,7 +423,7 @@ describe('ProjectUploadController', function () {
       beforeEach(function (ctx) {
         ctx.FileSystemImportManager.addEntity = sinon
           .stub()
-          .callsArgWith(6, new Error('project_has_too_many_files'))
+          .callsArgWith(6, new TooManyFilesError('project_has_too_many_files'))
         ctx.ProjectUploadController.uploadFile(ctx.req, ctx.res)
       })