Merge pull request #8423 from overleaf/briangough-issue8388

increase lock and timeout for history resync to 6 minutes

GitOrigin-RevId: 2aea0cbc26c92ed0aad8f815ccd41a2abc0b752e

services/document-updater/app.js

@@ -149,6 +149,7 @@ app.delete('/project', HttpController.deleteMultipleProjects)
 app.post('/project/:project_id', HttpController.updateProject)
 app.post(
   '/project/:project_id/history/resync',
+  longerTimeout,
   HttpController.resyncProjectHistory
 )
 app.post('/project/:project_id/flush', HttpController.flushProject)
@@ -258,3 +259,8 @@ for (const signal of [
 ]) {
   process.on(signal, shutdownCleanly(signal))
 }
+
+function longerTimeout(req, res, next) {
+  res.setTimeout(6 * 60 * 1000)
+  next()
+}

services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js

@@ -221,6 +221,7 @@ function resyncProjectHistory(
       path: `/project/${projectId}/history/resync`,
       json: { docs, files, projectHistoryId },
       method: 'POST',
+      timeout: 6 * 60 * 1000, // allow 6 minutes for resync
     },
     projectId,
     'resync-project-history',
@@ -299,6 +300,7 @@ function _makeRequest(options, projectId, metricsKey, callback) {
       url: `${settings.apis.documentupdater.url}${options.path}`,
       json: options.json,
       method: options.method || 'GET',
+      timeout: options.timeout || 30 * 1000,
     },
     function (error, res, body) {
       timer.done()

services/web/app/src/Features/History/HistoryController.js

@@ -97,6 +97,8 @@ module.exports = HistoryController = {
   },
 
   resyncProjectHistory(req, res, next) {
+    // increase timeout to 6 minutes
+    res.setTimeout(6 * 60 * 1000)
     const projectId = req.params.Project_id
     ProjectEntityUpdateHandler.resyncProjectHistory(projectId, function (err) {
       if (err instanceof Errors.ProjectHistoryDisabledError) {

services/web/app/src/Features/History/HistoryManager.js

@@ -99,6 +99,7 @@ async function resyncProject(projectId, options = {}) {
     await request.post({
       url: `${settings.apis.project_history.url}/project/${projectId}/resync`,
       json: body,
+      timeout: 6 * 60 * 1000,
     })
   } catch (err) {
     throw OError.tag(err, 'failed to resync project history', { projectId })

services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js

@@ -30,7 +30,7 @@ const VALID_ROOT_DOC_REGEXP = new RegExp(
   'i'
 )
 
-function wrapWithLock(methodWithoutLock) {
+function wrapWithLock(methodWithoutLock, lockManager = LockManager) {
   // This lock is used to make sure that the project structure updates are made
   // sequentially. In particular the updates must be made in mongo and sent to
   // the doc-updater in the same order.
@@ -39,7 +39,7 @@ function wrapWithLock(methodWithoutLock) {
       const adjustedLength = Math.max(rest.length, 1)
       const args = rest.slice(0, adjustedLength - 1)
       const callback = rest[adjustedLength - 1]
-      LockManager.runWithLock(
+      lockManager.runWithLock(
         LOCK_NAMESPACE,
         projectId,
         cb => methodWithoutLock(projectId, ...args, cb),
@@ -56,7 +56,7 @@ function wrapWithLock(methodWithoutLock) {
       const adjustedLength = Math.max(rest.length, 1)
       const args = rest.slice(0, adjustedLength - 1)
       const callback = rest[adjustedLength - 1]
-      LockManager.runWithLock(
+      lockManager.runWithLock(
         LOCK_NAMESPACE,
         projectId,
         cb => mainTask(projectId, ...args, cb),
@@ -1331,62 +1331,64 @@ const ProjectEntityUpdateHandler = {
 
   // This doesn't directly update project structure but we need to take the lock
   // to prevent anything else being queued before the resync update
-  resyncProjectHistory: wrapWithLock((projectId, callback) =>
-    ProjectGetter.getProject(
-      projectId,
-      { rootFolder: true, overleaf: true },
-      (error, project) => {
-        if (error != null) {
-          return callback(error)
-        }
+  resyncProjectHistory: wrapWithLock(
+    (projectId, callback) =>
+      ProjectGetter.getProject(
+        projectId,
+        { rootFolder: true, overleaf: true },
+        (error, project) => {
+          if (error != null) {
+            return callback(error)
+          }
 
-        const projectHistoryId =
-          project &&
-          project.overleaf &&
-          project.overleaf.history &&
-          project.overleaf.history.id
-        if (projectHistoryId == null) {
-          error = new Errors.ProjectHistoryDisabledError(
-            `project history not enabled for ${projectId}`
-          )
-          return callback(error)
-        }
+          const projectHistoryId =
+            project &&
+            project.overleaf &&
+            project.overleaf.history &&
+            project.overleaf.history.id
+          if (projectHistoryId == null) {
+            error = new Errors.ProjectHistoryDisabledError(
+              `project history not enabled for ${projectId}`
+            )
+            return callback(error)
+          }
 
-        let { docs, files, folders } =
-          ProjectEntityHandler.getAllEntitiesFromProject(project)
-        // _checkFileTree() must be passed the folders before docs and
-        // files
-        ProjectEntityUpdateHandler._checkFiletree(
-          projectId,
-          projectHistoryId,
-          [...folders, ...docs, ...files],
-          error => {
-            if (error) {
-              return callback(error)
-            }
-            docs = _.map(docs, doc => ({
-              doc: doc.doc._id,
-              path: doc.path,
-            }))
+          let { docs, files, folders } =
+            ProjectEntityHandler.getAllEntitiesFromProject(project)
+          // _checkFileTree() must be passed the folders before docs and
+          // files
+          ProjectEntityUpdateHandler._checkFiletree(
+            projectId,
+            projectHistoryId,
+            [...folders, ...docs, ...files],
+            error => {
+              if (error) {
+                return callback(error)
+              }
+              docs = _.map(docs, doc => ({
+                doc: doc.doc._id,
+                path: doc.path,
+              }))
 
-            files = _.map(files, file => ({
-              file: file.file._id,
-              path: file.path,
-              url: FileStoreHandler._buildUrl(projectId, file.file._id),
-              _hash: file.file.hash,
-            }))
+              files = _.map(files, file => ({
+                file: file.file._id,
+                path: file.path,
+                url: FileStoreHandler._buildUrl(projectId, file.file._id),
+                _hash: file.file.hash,
+              }))
 
-            DocumentUpdaterHandler.resyncProjectHistory(
-              projectId,
-              projectHistoryId,
-              docs,
-              files,
-              callback
-            )
-          }
-        )
-      }
-    )
+              DocumentUpdaterHandler.resyncProjectHistory(
+                projectId,
+                projectHistoryId,
+                docs,
+                files,
+                callback
+              )
+            }
+          )
+        }
+      ),
+    LockManager.withTimeout(6 * 60) // use an extended lock for the resync operations
   ),
 
   _checkFiletree(projectId, projectHistoryId, entities, callback) {

services/web/app/src/infrastructure/LockManager.js

@@ -5,13 +5,34 @@ const { callbackify, promisify } = require('util')
 
 const RedisWebLocker = require('@overleaf/redis-wrapper/RedisWebLocker')
 
-const LockManager = new RedisWebLocker({
-  rclient,
-  getKey(namespace, id) {
-    return `lock:web:${namespace}:${id}`
-  },
-  options: settings.lockManager,
-})
+// this method creates a lock manager with the provided timeout options
+function createLockManager(options) {
+  return new RedisWebLocker({
+    rclient,
+    getKey(namespace, id) {
+      return `lock:web:${namespace}:${id}`
+    },
+    options,
+  })
+}
+
+// this is the default lock manager for web
+const LockManager = createLockManager(settings.lockManager)
+
+// this method creates a lock manager with a configurable timeout
+// it shares the lock keys with the default lock manager
+LockManager.withTimeout = function (timeout) {
+  const overrides = {
+    redisLockExpiry: timeout, // in seconds
+    slowExecutionThreshold: 0.5 * timeout * 1000, // in ms
+  }
+  const lockManagerSettingsWithTimeout = Object.assign(
+    {},
+    settings.lockManager,
+    overrides
+  )
+  return createLockManager(lockManagerSettingsWithTimeout)
+}
 
 // need to bind the promisified function when it is part of a class
 // see https://nodejs.org/dist/latest-v16.x/docs/api/util.html#utilpromisifyoriginal

services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js

@@ -350,6 +350,7 @@ describe('DocumentUpdaterHandler', function () {
               user_id: this.user_id,
             },
             method: 'POST',
+            timeout: 30 * 1000,
           })
           .should.equal(true)
       })
@@ -439,6 +440,7 @@ describe('DocumentUpdaterHandler', function () {
             url: `${this.settings.apis.documentupdater.url}/project/${this.project_id}/doc/${this.doc_id}?fromVersion=${this.fromVersion}`,
             method: 'GET',
             json: true,
+            timeout: 30 * 1000,
           })
           .should.equal(true)
       })
@@ -658,6 +660,7 @@ describe('DocumentUpdaterHandler', function () {
               change_ids: [this.change_id],
             },
             method: 'POST',
+            timeout: 30 * 1000,
           })
           .should.equal(true)
       })
@@ -881,6 +884,7 @@ describe('DocumentUpdaterHandler', function () {
                     version: this.version,
                     projectHistoryId: this.projectHistoryId,
                   },
+                  timeout: 30 * 1000,
                 })
                 .should.equal(true)
               done()
@@ -926,6 +930,7 @@ describe('DocumentUpdaterHandler', function () {
                     version: this.version,
                     projectHistoryId: this.projectHistoryId,
                   },
+                  timeout: 30 * 1000,
                 })
                 .should.equal(true)
               done()
@@ -975,6 +980,7 @@ describe('DocumentUpdaterHandler', function () {
                     version: this.version,
                     projectHistoryId: this.projectHistoryId,
                   },
+                  timeout: 30 * 1000,
                 })
                 .should.equal(true)
               done()
@@ -1018,6 +1024,7 @@ describe('DocumentUpdaterHandler', function () {
                     version: this.version,
                     projectHistoryId: this.projectHistoryId,
                   },
+                  timeout: 30 * 1000,
                 })
                 .should.equal(true)
               done()
@@ -1080,6 +1087,7 @@ describe('DocumentUpdaterHandler', function () {
                   version: this.version,
                   projectHistoryId: this.projectHistoryId,
                 },
+                timeout: 30 * 1000,
               })
               done()
             }

services/web/test/unit/src/History/HistoryControllerTests.js

@@ -311,7 +311,7 @@ describe('HistoryController', function () {
       beforeEach(function () {
         this.project_id = 'mock-project-id'
         this.req = { params: { Project_id: this.project_id } }
-        this.res = { sendStatus: sinon.stub() }
+        this.res = { setTimeout: sinon.stub(), sendStatus: sinon.stub() }
         this.next = sinon.stub()
 
         this.error = new Errors.ProjectHistoryDisabledError()
@@ -335,7 +335,7 @@ describe('HistoryController', function () {
       beforeEach(function () {
         this.project_id = 'mock-project-id'
         this.req = { params: { Project_id: this.project_id } }
-        this.res = { sendStatus: sinon.stub() }
+        this.res = { setTimeout: sinon.stub(), sendStatus: sinon.stub() }
         this.next = sinon.stub()
 
         this.ProjectEntityUpdateHandler.resyncProjectHistory = sinon
@@ -349,6 +349,10 @@ describe('HistoryController', function () {
         )
       })
 
+      it('sets an extended response timeout', function () {
+        this.res.setTimeout.should.have.been.calledWith(6 * 60 * 1000)
+      })
+
       it('resyncs the project', function () {
         return this.ProjectEntityUpdateHandler.resyncProjectHistory
           .calledWith(this.project_id)

services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js

@@ -88,6 +88,7 @@ describe('ProjectEntityUpdateHandler', function () {
       runWithLock: sinon.spy((namespace, id, runner, callback) =>
         runner(callback)
       ),
+      withTimeout: sinon.stub().returns(this.LockManager),
     }
     this.ProjectModel = {
       updateOne: sinon.stub(),
@@ -1954,6 +1955,10 @@ describe('ProjectEntityUpdateHandler', function () {
           .should.equal(true)
       })
 
+      it('uses an extended timeout', function () {
+        this.LockManager.withTimeout.calledWith(6 * 60).should.equal(true)
+      })
+
       it('tells the doc updater to sync the project', function () {
         const docs = [
           {