From 6cea8580aabe0ffd62d22ce5a0bd7bf88e3d5ffe Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Wed, 8 Jul 2026 08:22:49 -0400 Subject: [PATCH] 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 --- .../src/Features/Cooldown/CooldownManager.mjs | 31 ----------- .../Features/Editor/EditorHttpController.mjs | 4 +- .../src/Features/Errors/ErrorController.mjs | 9 ++++ .../web/app/src/Features/Errors/Errors.js | 3 ++ .../LinkedFiles/LinkedFilesController.mjs | 3 +- .../ProjectEntityMongoUpdateHandler.mjs | 4 +- .../ThirdPartyDataStore/TpdsController.mjs | 12 +---- .../ThirdPartyDataStore/TpdsUpdateHandler.mjs | 16 ------ .../Uploads/ProjectUploadController.mjs | 3 +- .../CooldownManager.sequential.test.mjs | 50 ----------------- .../src/Editor/EditorHttpController.test.mjs | 4 +- .../ProjectEntityMongoUpdateHandler.test.mjs | 5 -- .../TpdsController.test.mjs | 45 +++++----------- .../TpdsUpdateHandler.test.mjs | 54 ------------------- .../Uploads/ProjectUploadController.test.mjs | 3 +- 15 files changed, 39 insertions(+), 207 deletions(-) delete mode 100644 services/web/app/src/Features/Cooldown/CooldownManager.mjs delete mode 100644 services/web/test/unit/src/Cooldown/CooldownManager.sequential.test.mjs diff --git a/services/web/app/src/Features/Cooldown/CooldownManager.mjs b/services/web/app/src/Features/Cooldown/CooldownManager.mjs deleted file mode 100644 index 7a414fe542..0000000000 --- a/services/web/app/src/Features/Cooldown/CooldownManager.mjs +++ /dev/null @@ -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 diff --git a/services/web/app/src/Features/Editor/EditorHttpController.mjs b/services/web/app/src/Features/Editor/EditorHttpController.mjs index 7dc8939186..d98968f417 100644 --- a/services/web/app/src/Features/Editor/EditorHttpController.mjs +++ b/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, diff --git a/services/web/app/src/Features/Errors/ErrorController.mjs b/services/web/app/src/Features/Errors/ErrorController.mjs index 810c44ef1c..32eec8ec52 100644 --- a/services/web/app/src/Features/Errors/ErrorController.mjs +++ b/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) diff --git a/services/web/app/src/Features/Errors/Errors.js b/services/web/app/src/Features/Errors/Errors.js index fbb5cf35b7..e968629185 100644 --- a/services/web/app/src/Features/Errors/Errors.js +++ b/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, diff --git a/services/web/app/src/Features/LinkedFiles/LinkedFilesController.mjs b/services/web/app/src/Features/LinkedFiles/LinkedFilesController.mjs index 3c88ccfca0..23c6b6cc5e 100644 --- a/services/web/app/src/Features/LinkedFiles/LinkedFilesController.mjs +++ b/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)) { diff --git a/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.mjs b/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.mjs index c236e3be45..69da79eb6b 100644 --- a/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.mjs +++ b/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({ diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.mjs b/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.mjs index 018b682655..90472c98f5 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.mjs +++ b/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) { diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.mjs b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.mjs index 6de01be624..b183bee09d 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.mjs +++ b/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 diff --git a/services/web/app/src/Features/Uploads/ProjectUploadController.mjs b/services/web/app/src/Features/Uploads/ProjectUploadController.mjs index 0e288f977f..6f7c91a72d 100644 --- a/services/web/app/src/Features/Uploads/ProjectUploadController.mjs +++ b/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', diff --git a/services/web/test/unit/src/Cooldown/CooldownManager.sequential.test.mjs b/services/web/test/unit/src/Cooldown/CooldownManager.sequential.test.mjs deleted file mode 100644 index 77b7992f62..0000000000 --- a/services/web/test/unit/src/Cooldown/CooldownManager.sequential.test.mjs +++ /dev/null @@ -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 - }) - }) - }) -}) diff --git a/services/web/test/unit/src/Editor/EditorHttpController.test.mjs b/services/web/test/unit/src/Editor/EditorHttpController.test.mjs index 33bb6874d4..46ec0bc672 100644 --- a/services/web/test/unit/src/Editor/EditorHttpController.test.mjs +++ b/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"') diff --git a/services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandler.test.mjs b/services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandler.test.mjs index 926fc29d99..6e7c278bd0 100644 --- a/services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandler.test.mjs +++ b/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, })) diff --git a/services/web/test/unit/src/ThirdPartyDataStore/TpdsController.test.mjs b/services/web/test/unit/src/ThirdPartyDataStore/TpdsController.test.mjs index b22230c0eb..a62284b3d4 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/TpdsController.test.mjs +++ b/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 + ) }) }) diff --git a/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandler.test.mjs b/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandler.test.mjs index 437126efc4..f00a91e5d7 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandler.test.mjs +++ b/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() { diff --git a/services/web/test/unit/src/Uploads/ProjectUploadController.test.mjs b/services/web/test/unit/src/Uploads/ProjectUploadController.test.mjs index 7faa007102..eff82293a5 100644 --- a/services/web/test/unit/src/Uploads/ProjectUploadController.test.mjs +++ b/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) })