Merge pull request #13537 from overleaf/em-promisify-clsi-manager-2

Promisify ClsiManager

GitOrigin-RevId: e785cdb1989b433f7baa1a4d974b09ba3a93fcbd

package-lock.json

@@ -37374,7 +37374,6 @@
       "version": "4.0.0",
       "resolved": "https://registry.npmjs.org/tough-cookie/-/tough-cookie-4.0.0.tgz",
       "integrity": "sha512-tHdtEpQCMrc1YLrMaqXXcj6AxhYi/xgit6mZu1+EDWUn+qhUf8wMQoFIy9NXuq23zAwtcB0t/MjACGR18pcRbg==",
-      "dev": true,
       "dependencies": {
         "psl": "^1.1.33",
         "punycode": "^2.1.1",
@@ -37388,7 +37387,6 @@
       "version": "0.1.2",
       "resolved": "https://registry.npmjs.org/universalify/-/universalify-0.1.2.tgz",
       "integrity": "sha512-rBJeI5CXAlmy1pV+617WB9J63U6XcazHHF2f2dbJix4XzpUF0RS3Zbj0FGIOCAva5P/d/GBOYaACQ1w+0azUkg==",
-      "dev": true,
       "engines": {
         "node": ">= 4.0.0"
       }
@@ -41595,6 +41593,7 @@
         "rimraf": "2.2.6",
         "sanitize-html": "^2.8.1",
         "scroll-into-view-if-needed": "^2.2.25",
+        "tough-cookie": "^4.0.0",
         "tsscmp": "^1.0.6",
         "underscore": "^1.13.1",
         "utf-8-validate": "^5.0.2",
@@ -41693,7 +41692,6 @@
         "terser-webpack-plugin": "^5.3.9",
         "timekeeper": "^2.2.0",
         "to-string-loader": "^1.2.0",
-        "tough-cookie": "^4.0.0",
         "typescript": "^5.0.4",
         "val-loader": "^5.0.1",
         "webpack": "^5.83.1",
@@ -74131,7 +74129,6 @@
       "version": "4.0.0",
       "resolved": "https://registry.npmjs.org/tough-cookie/-/tough-cookie-4.0.0.tgz",
       "integrity": "sha512-tHdtEpQCMrc1YLrMaqXXcj6AxhYi/xgit6mZu1+EDWUn+qhUf8wMQoFIy9NXuq23zAwtcB0t/MjACGR18pcRbg==",
-      "dev": true,
       "requires": {
         "psl": "^1.1.33",
         "punycode": "^2.1.1",
@@ -74141,8 +74138,7 @@
         "universalify": {
           "version": "0.1.2",
           "resolved": "https://registry.npmjs.org/universalify/-/universalify-0.1.2.tgz",
-          "integrity": "sha512-rBJeI5CXAlmy1pV+617WB9J63U6XcazHHF2f2dbJix4XzpUF0RS3Zbj0FGIOCAva5P/d/GBOYaACQ1w+0azUkg==",
-          "dev": true
+          "integrity": "sha512-rBJeI5CXAlmy1pV+617WB9J63U6XcazHHF2f2dbJix4XzpUF0RS3Zbj0FGIOCAva5P/d/GBOYaACQ1w+0azUkg=="
         }
       }
     },

services/web/app/src/Features/Compile/ClsiCookieManager.js

@@ -6,6 +6,7 @@ const RedisWrapper = require('../../infrastructure/RedisWrapper')
 const Cookie = require('cookie')
 const logger = require('@overleaf/logger')
 const Metrics = require('@overleaf/metrics')
+const { promisifyAll } = require('../../util/promises')
 
 const clsiCookiesEnabled = (Settings.clsiCookie?.key ?? '') !== ''
 
@@ -16,7 +17,7 @@ if (Settings.redis.clsi_cookie_secondary != null) {
 }
 
 module.exports = function (backendGroup) {
-  return {
+  const cookieManager = {
     buildKey(projectId, userId) {
       if (backendGroup != null) {
         return `clsiserver:${backendGroup}:${projectId}:${userId}`
@@ -25,7 +26,7 @@ module.exports = function (backendGroup) {
       }
     },
 
-    _getServerId(
+    getServerId(
       projectId,
       userId,
       compileGroup,
@@ -69,14 +70,18 @@ module.exports = function (backendGroup) {
           })
           return callback(err)
         }
+        if (!clsiCookiesEnabled) {
+          return callback()
+        }
+        const serverId = this._parseServerIdFromResponse(res)
         this.setServerId(
           projectId,
           userId,
           compileGroup,
           compileBackendClass,
-          res,
+          serverId,
           null,
-          function (err, serverId) {
+          function (err) {
             if (err) {
               logger.warn(
                 { err, projectId },
@@ -128,20 +133,19 @@ module.exports = function (backendGroup) {
       userId,
       compileGroup,
       compileBackendClass,
-      response,
+      serverId,
       previous,
       callback
     ) {
       if (!clsiCookiesEnabled) {
         return callback()
       }
-      const serverId = this._parseServerIdFromResponse(response)
       if (serverId == null) {
         // We don't get a cookie back if it hasn't changed
         return rclient.expire(
           this.buildKey(projectId, userId),
           this._getTTLInSeconds(previous),
-          err => callback(err, undefined)
+          err => callback(err)
         )
       }
       if (!previous) {
@@ -164,7 +168,7 @@ module.exports = function (backendGroup) {
         )
       }
       this._setServerIdInRedis(rclient, projectId, userId, serverId, err =>
-        callback(err, serverId)
+        callback(err)
       )
     },
 
@@ -181,7 +185,20 @@ module.exports = function (backendGroup) {
       if (!clsiCookiesEnabled) {
         return callback()
       }
-      rclient.del(this.buildKey(projectId, userId), callback)
+      rclient.del(this.buildKey(projectId, userId), err => {
+        if (err) {
+          // redis errors need wrapping as the instance may be shared
+          return callback(
+            new OError(
+              'Failed to clear clsi persistence',
+              { projectId, userId },
+              err
+            )
+          )
+        } else {
+          return callback()
+        }
+      })
     },
 
     getCookieJar(
@@ -194,7 +211,7 @@ module.exports = function (backendGroup) {
       if (!clsiCookiesEnabled) {
         return callback(null, request.jar(), undefined)
       }
-      this._getServerId(
+      this.getServerId(
         projectId,
         userId,
         compileGroup,
@@ -216,4 +233,15 @@ module.exports = function (backendGroup) {
       )
     },
   }
+  cookieManager.promises = promisifyAll(cookieManager, {
+    without: [
+      '_parseServerIdFromResponse',
+      'checkIsLoadSheddingEvent',
+      '_getTTLInSeconds',
+    ],
+    multiResult: {
+      getCookieJar: ['jar', 'clsiServerId'],
+    },
+  })
+  return cookieManager
 }

services/web/app/src/Features/Compile/ClsiFormatChecker.js

@@ -13,6 +13,7 @@ let ClsiFormatChecker
 const _ = require('lodash')
 const async = require('async')
 const settings = require('@overleaf/settings')
+const { promisifyAll } = require('../../util/promises')
 
 module.exports = ClsiFormatChecker = {
   checkRecoursesForProblems(resources, callback) {
@@ -84,3 +85,5 @@ module.exports = ClsiFormatChecker = {
     }
   },
 }
+
+module.exports.promises = promisifyAll(module.exports)

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

@@ -142,6 +142,8 @@ class ThirdPartyUserNotFoundError extends BackwardCompatibleError {
   }
 }
 
+class OutputFileFetchFailedError extends OError {}
+
 class SubscriptionAdminDeletionError extends OErrorV2CompatibleError {
   constructor(options) {
     super('subscription admins cannot be deleted', options)
@@ -210,6 +212,7 @@ module.exports = {
   EmailExistsError,
   InvalidError,
   NotInV2Error,
+  OutputFileFetchFailedError,
   SAMLIdentityExistsError,
   SAMLAlreadyLinkedError,
   SAMLEmailNotAffiliatedError,

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

@@ -23,7 +23,6 @@ const {
   CompileFailedError,
   UrlFetchFailedError,
   InvalidUrlError,
-  OutputFileFetchFailedError,
   AccessDeniedError,
   BadEntityTypeError,
   BadDataError,
@@ -35,6 +34,7 @@ const {
   RemoteServiceError,
   FileCannotRefreshError,
 } = require('./LinkedFilesErrors')
+const { OutputFileFetchFailedError } = require('../Errors/Errors')
 const Modules = require('../../infrastructure/Modules')
 const { plainTextResponse } = require('../../infrastructure/Response')
 

services/web/app/src/Features/LinkedFiles/LinkedFilesErrors.js

@@ -5,7 +5,6 @@ class UrlFetchFailedError extends BackwardCompatibleError {}
 class InvalidUrlError extends BackwardCompatibleError {}
 
 class CompileFailedError extends BackwardCompatibleError {}
-class OutputFileFetchFailedError extends BackwardCompatibleError {}
 
 class AccessDeniedError extends BackwardCompatibleError {}
 
@@ -31,7 +30,6 @@ module.exports = {
   CompileFailedError,
   UrlFetchFailedError,
   InvalidUrlError,
-  OutputFileFetchFailedError,
   AccessDeniedError,
   BadEntityTypeError,
   BadDataError,

services/web/app/src/Features/LinkedFiles/ProjectOutputFileAgent.js

@@ -7,8 +7,8 @@ const {
   CompileFailedError,
   BadDataError,
   AccessDeniedError,
-  OutputFileFetchFailedError,
 } = require('./LinkedFilesErrors')
+const { OutputFileFetchFailedError } = require('../Errors/Errors')
 const LinkedFilesHandler = require('./LinkedFilesHandler')
 
 function _prepare(projectId, linkedFileData, userId, callback) {
@@ -46,31 +46,20 @@ function createLinkedFile(
       if (err) {
         return callback(err)
       }
-      readStream.on('error', callback)
-      readStream.on('response', response => {
-        if (response.statusCode >= 200 && response.statusCode < 300) {
-          LinkedFilesHandler.importFromStream(
-            projectId,
-            readStream,
-            linkedFileData,
-            name,
-            parentFolderId,
-            userId,
-            (err, file) => {
-              if (err) {
-                return callback(err)
-              }
-              callback(null, file._id)
-            }
-          ) // Created
-        } else {
-          err = new OutputFileFetchFailedError(
-            `Output file fetch failed: ${linkedFileData.build_id}, ${linkedFileData.source_output_file_path}`
-          )
-          err.statusCode = response.statusCode
-          callback(err)
+      LinkedFilesHandler.importFromStream(
+        projectId,
+        readStream,
+        linkedFileData,
+        name,
+        parentFolderId,
+        userId,
+        (err, file) => {
+          if (err) {
+            return callback(err)
+          }
+          callback(null, file._id)
         }
-      })
+      )
     })
   })
 }
@@ -94,32 +83,21 @@ function refreshLinkedFile(
         if (err) {
           return callback(err)
         }
-        readStream.on('error', callback)
-        readStream.on('response', response => {
-          if (response.statusCode >= 200 && response.statusCode < 300) {
-            linkedFileData.build_id = newBuildId
-            LinkedFilesHandler.importFromStream(
-              projectId,
-              readStream,
-              linkedFileData,
-              name,
-              parentFolderId,
-              userId,
-              (err, file) => {
-                if (err) {
-                  return callback(err)
-                }
-                callback(null, file._id)
-              }
-            ) // Created
-          } else {
-            err = new OutputFileFetchFailedError(
-              `Output file fetch failed: ${linkedFileData.build_id}, ${linkedFileData.source_output_file_path}`
-            )
-            err.statusCode = response.statusCode
-            callback(err)
+        linkedFileData.build_id = newBuildId
+        LinkedFilesHandler.importFromStream(
+          projectId,
+          readStream,
+          linkedFileData,
+          name,
+          parentFolderId,
+          userId,
+          (err, file) => {
+            if (err) {
+              return callback(err)
+            }
+            callback(null, file._id)
           }
-        })
+        )
       }
     )
   })
@@ -193,7 +171,6 @@ function _getFileStream(linkedFileData, userId, callback) {
           if (err) {
             return callback(err)
           }
-          readStream.pause()
           callback(null, readStream)
         }
       )
@@ -239,7 +216,6 @@ function _compileAndGetFileStream(linkedFileData, userId, callback) {
             if (err) {
               return callback(err)
             }
-            readStream.pause()
             callback(null, readStream, buildId)
           }
         )

services/web/app/src/router.js

@@ -1253,18 +1253,18 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) {
     AuthorizationMiddleware.ensureUserCanReadProject,
     function (req, res) {
       const projectId = req.params.Project_id
+      // use a valid user id for testing
+      const testUserId = '123456789012345678901234'
       const sendRes = _.once(function (statusCode, message) {
         res.status(statusCode)
         plainTextResponse(res, message)
-        ClsiCookieManager.clearServerId(projectId, () => {})
+        ClsiCookieManager.clearServerId(projectId, testUserId, () => {})
       }) // force every compile to a new server
       // set a timeout
       let handler = setTimeout(function () {
         sendRes(500, 'Compiler timed out')
         handler = null
       }, 10000)
-      // use a valid user id for testing
-      const testUserId = '123456789012345678901234'
       // run the compile
       CompileManager.compile(
         projectId,

services/web/package.json

@@ -242,6 +242,7 @@
     "rimraf": "2.2.6",
     "sanitize-html": "^2.8.1",
     "scroll-into-view-if-needed": "^2.2.25",
+    "tough-cookie": "^4.0.0",
     "tsscmp": "^1.0.6",
     "underscore": "^1.13.1",
     "utf-8-validate": "^5.0.2",
@@ -340,7 +341,6 @@
     "terser-webpack-plugin": "^5.3.9",
     "timekeeper": "^2.2.0",
     "to-string-loader": "^1.2.0",
-    "tough-cookie": "^4.0.0",
     "typescript": "^5.0.4",
     "val-loader": "^5.0.1",
     "webpack": "^5.83.1",

services/web/test/unit/src/Compile/ClsiCookieManagerTests.js

@@ -51,7 +51,7 @@ describe('ClsiCookieManager', function () {
   describe('getServerId', function () {
     it('should call get for the key', function (done) {
       this.redis.get.callsArgWith(1, null, 'clsi-7')
-      this.ClsiCookieManager._getServerId(
+      this.ClsiCookieManager.getServerId(
         this.project_id,
         this.user_id,
         '',
@@ -74,7 +74,7 @@ describe('ClsiCookieManager', function () {
         .stub()
         .yields(null)
       this.redis.get.callsArgWith(1, null)
-      this.ClsiCookieManager._getServerId(
+      this.ClsiCookieManager.getServerId(
         this.project_id,
         this.user_id,
         '',
@@ -95,7 +95,7 @@ describe('ClsiCookieManager', function () {
         .stub()
         .yields(null)
       this.redis.get.callsArgWith(1, null, '')
-      this.ClsiCookieManager._getServerId(
+      this.ClsiCookieManager.getServerId(
         this.project_id,
         this.user_id,
         '',
@@ -115,52 +115,91 @@ describe('ClsiCookieManager', function () {
 
   describe('_populateServerIdViaRequest', function () {
     beforeEach(function () {
-      this.response = 'some data'
-      this.request.post.callsArgWith(1, null, this.response)
-      this.ClsiCookieManager.setServerId = sinon.stub().yields(null, 'clsi-9')
+      this.clsiServerId = 'server-id'
+      this.ClsiCookieManager.setServerId = sinon.stub().yields()
     })
 
-    it('should make a request to the clsi', function (done) {
-      this.ClsiCookieManager._populateServerIdViaRequest(
-        this.project_id,
-        this.user_id,
-        'standard',
-        'e2',
-        (err, serverId) => {
-          if (err) {
-            return done(err)
-          }
-          const args = this.ClsiCookieManager.setServerId.args[0]
-          args[0].should.equal(this.project_id)
-          args[1].should.equal(this.user_id)
-          args[2].should.equal('standard')
-          args[3].should.equal('e2')
-          args[4].should.deep.equal(this.response)
-          done()
+    describe('with a server id in the response', function () {
+      beforeEach(function () {
+        this.response = {
+          headers: {
+            'set-cookie': [
+              `${this.settings.clsiCookie.key}=${this.clsiServerId}`,
+            ],
+          },
         }
-      )
+        this.request.post.callsArgWith(1, null, this.response)
+      })
+
+      it('should make a request to the clsi', function (done) {
+        this.ClsiCookieManager._populateServerIdViaRequest(
+          this.project_id,
+          this.user_id,
+          'standard',
+          'e2',
+          (err, serverId) => {
+            if (err) {
+              return done(err)
+            }
+            const args = this.ClsiCookieManager.setServerId.args[0]
+            args[0].should.equal(this.project_id)
+            args[1].should.equal(this.user_id)
+            args[2].should.equal('standard')
+            args[3].should.equal('e2')
+            args[4].should.deep.equal(this.clsiServerId)
+            done()
+          }
+        )
+      })
+
+      it('should return the server id', function (done) {
+        this.ClsiCookieManager._populateServerIdViaRequest(
+          this.project_id,
+          this.user_id,
+          '',
+          'e2',
+          (err, serverId) => {
+            if (err) {
+              return done(err)
+            }
+            serverId.should.equal(this.clsiServerId)
+            done()
+          }
+        )
+      })
     })
 
-    it('should return the server id', function (done) {
-      this.ClsiCookieManager._populateServerIdViaRequest(
-        this.project_id,
-        this.user_id,
-        '',
-        'e2',
-        (err, serverId) => {
-          if (err) {
-            return done(err)
+    describe('without a server id in the response', function () {
+      beforeEach(function () {
+        this.response = { headers: {} }
+        this.request.post.yields(null, this.response)
+      })
+      it('should not set the server id there is no server id in the response', function (done) {
+        this.ClsiCookieManager._parseServerIdFromResponse = sinon
+          .stub()
+          .returns(null)
+        this.ClsiCookieManager.setServerId(
+          this.project_id,
+          this.user_id,
+          'standard',
+          'e2',
+          this.clsiServerId,
+          null,
+          err => {
+            if (err) {
+              return done(err)
+            }
+            this.redis.setex.called.should.equal(false)
+            done()
           }
-          serverId.should.equal('clsi-9')
-          done()
-        }
-      )
+        )
+      })
     })
   })
 
   describe('setServerId', function () {
     beforeEach(function () {
-      this.response = 'dsadsakj'
+      this.clsiServerId = 'server-id'
       this.ClsiCookieManager._parseServerIdFromResponse = sinon
         .stub()
         .returns('clsi-8')
@@ -172,34 +211,30 @@ describe('ClsiCookieManager', function () {
         this.user_id,
         'standard',
         'e2',
-        this.response,
+        this.clsiServerId,
         null,
         err => {
           if (err) {
             return done(err)
           }
-          this.redis.setex
-            .calledWith(
-              `clsiserver:${this.project_id}:${this.user_id}`,
-              this.settings.clsiCookie.ttlInSeconds,
-              'clsi-8'
-            )
-            .should.equal(true)
+          this.redis.setex.should.have.been.calledWith(
+            `clsiserver:${this.project_id}:${this.user_id}`,
+            this.settings.clsiCookie.ttlInSeconds,
+            this.clsiServerId
+          )
           done()
         }
       )
     })
 
     it('should set the server id with the regular ttl for reg instance', function (done) {
-      this.ClsiCookieManager._parseServerIdFromResponse = sinon
-        .stub()
-        .returns('clsi-reg-8')
+      this.clsiServerId = 'clsi-reg-8'
       this.ClsiCookieManager.setServerId(
         this.project_id,
         this.user_id,
         'standard',
         'e2',
-        this.response,
+        this.clsiServerId,
         null,
         err => {
           if (err) {
@@ -208,31 +243,13 @@ describe('ClsiCookieManager', function () {
           expect(this.redis.setex).to.have.been.calledWith(
             `clsiserver:${this.project_id}:${this.user_id}`,
             this.settings.clsiCookie.ttlInSecondsRegular,
-            'clsi-reg-8'
+            this.clsiServerId
           )
           done()
         }
       )
     })
 
-    it('should return the server id', function (done) {
-      this.ClsiCookieManager.setServerId(
-        this.project_id,
-        this.user_id,
-        'standard',
-        'e2',
-        this.response,
-        null,
-        (err, serverId) => {
-          if (err) {
-            return done(err)
-          }
-          serverId.should.equal('clsi-8')
-          done()
-        }
-      )
-    })
-
     it('should not set the server id if clsiCookies are not enabled', function (done) {
       delete this.settings.clsiCookie.key
       this.ClsiCookieManager = SandboxedModule.require(modulePath, {
@@ -246,30 +263,9 @@ describe('ClsiCookieManager', function () {
         this.user_id,
         'standard',
         'e2',
-        this.response,
-        null,
-        (err, serverId) => {
-          if (err) {
-            return done(err)
-          }
-          this.redis.setex.called.should.equal(false)
-          done()
-        }
-      )
-    })
-
-    it('should not set the server id there is no server id in the response', function (done) {
-      this.ClsiCookieManager._parseServerIdFromResponse = sinon
-        .stub()
-        .returns(null)
-      this.ClsiCookieManager.setServerId(
-        this.project_id,
-        this.user_id,
-        'standard',
-        'e2',
-        this.response,
+        this.clsiServerId,
         null,
-        (err, serverId) => {
+        err => {
           if (err) {
             return done(err)
           }
@@ -301,19 +297,17 @@ describe('ClsiCookieManager', function () {
         this.user_id,
         'standard',
         'e2',
-        this.response,
+        this.clsiServerId,
         null,
-        (err, serverId) => {
+        err => {
           if (err) {
             return done(err)
           }
-          this.redis_secondary.setex
-            .calledWith(
-              `clsiserver:${this.project_id}:${this.user_id}`,
-              this.settings.clsiCookie.ttlInSeconds,
-              'clsi-8'
-            )
-            .should.equal(true)
+          this.redis_secondary.setex.should.have.been.calledWith(
+            `clsiserver:${this.project_id}:${this.user_id}`,
+            this.settings.clsiCookie.ttlInSeconds,
+            this.clsiServerId
+          )
           done()
         }
       )
@@ -322,7 +316,7 @@ describe('ClsiCookieManager', function () {
 
   describe('getCookieJar', function () {
     beforeEach(function () {
-      this.ClsiCookieManager._getServerId = sinon.stub().yields(null, 'clsi-11')
+      this.ClsiCookieManager.getServerId = sinon.stub().yields(null, 'clsi-11')
     })
 
     it('should return a jar with the cookie set populated from redis', function (done) {