Merge pull request #14358 from overleaf/jpa-abort-check

[web] skip streaming clsi response when the request was aborted

GitOrigin-RevId: 8152399f18328eafd3e93143190a5eb8d1fdff1d

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

@@ -328,7 +328,15 @@ module.exports = CompileController = {
               req.params.build_id,
               'output.pdf'
             )
-            CompileController.proxyToClsi(projectId, url, {}, req, res, next)
+            CompileController.proxyToClsi(
+              projectId,
+              'output-file',
+              url,
+              {},
+              req,
+              res,
+              next
+            )
           })
         }
       })
@@ -374,7 +382,15 @@ module.exports = CompileController = {
         return
       }
       const url = `/project/${projectId}/output/output.pdf`
-      CompileController.proxyToClsi(projectId, url, {}, req, res, next)
+      CompileController.proxyToClsi(
+        projectId,
+        'output-file',
+        url,
+        {},
+        req,
+        res,
+        next
+      )
     })
   },
 
@@ -390,7 +406,15 @@ module.exports = CompileController = {
         req.params.build_id,
         req.params.file
       )
-      CompileController.proxyToClsi(projectId, url, {}, req, res, next)
+      CompileController.proxyToClsi(
+        projectId,
+        'output-file',
+        url,
+        {},
+        req,
+        res,
+        next
+      )
     })
   },
 
@@ -411,6 +435,7 @@ module.exports = CompileController = {
     }
     CompileController.proxyToClsiWithLimits(
       submissionId,
+      'output-file',
       url,
       {},
       limits,
@@ -467,6 +492,7 @@ module.exports = CompileController = {
         const url = CompileController._getUrl(projectId, userId, 'sync/pdf')
         CompileController.proxyToClsi(
           projectId,
+          'sync-to-pdf',
           url,
           { page, h, v, imageName },
           req,
@@ -508,6 +534,7 @@ module.exports = CompileController = {
         const url = CompileController._getUrl(projectId, userId, 'sync/code')
         CompileController.proxyToClsi(
           projectId,
+          'sync-to-code',
           url,
           { file, line, column, imageName },
           req,
@@ -518,13 +545,14 @@ module.exports = CompileController = {
     })
   },
 
-  proxyToClsi(projectId, url, qs, req, res, next) {
+  proxyToClsi(projectId, action, url, qs, req, res, next) {
     CompileManager.getProjectCompileLimits(projectId, function (error, limits) {
       if (error) {
         return next(error)
       }
       CompileController.proxyToClsiWithLimits(
         projectId,
+        action,
         url,
         qs,
         limits,
@@ -535,7 +563,7 @@ module.exports = CompileController = {
     })
   },
 
-  proxyToClsiWithLimits(projectId, url, qs, limits, req, res, next) {
+  proxyToClsiWithLimits(projectId, action, url, qs, limits, req, res, next) {
     _getPersistenceOptions(
       req,
       projectId,
@@ -551,19 +579,43 @@ module.exports = CompileController = {
           ...persistenceOptions.qs,
           ...qs,
         }).toString()
+        const timer = new Metrics.Timer(
+          'proxy_to_clsi',
+          1,
+          { path: action },
+          [0, 100, 1000, 2000, 5000, 10000, 15000, 20000, 30000, 45000, 60000]
+        )
+        Metrics.inc('proxy_to_clsi', 1, { path: action, status: 'start' })
         fetchStreamWithResponse(url.href, {
           method: req.method,
           signal: AbortSignal.timeout(60 * 1000),
           headers: persistenceOptions.headers,
         })
           .then(({ stream, response }) => {
+            if (req.destroyed) {
+              // The client has disconnected already, avoid trying to write into the broken connection.
+              Metrics.inc('proxy_to_clsi', 1, {
+                path: action,
+                status: 'req-aborted',
+              })
+              return
+            }
+            Metrics.inc('proxy_to_clsi', 1, {
+              path: action,
+              status: response.status,
+            })
+
             for (const key of ['Content-Length', 'Content-Type']) {
               res.setHeader(key, response.headers.get(key))
             }
             res.writeHead(response.status)
             return pipeline(stream, res)
           })
+          .then(() => {
+            timer.done()
+          })
           .catch(err => {
+            const duration = timer.done()
             if (!res.headersSent) {
               if (err instanceof RequestFailedError) {
                 res.sendStatus(err.response.status)
@@ -571,7 +623,19 @@ module.exports = CompileController = {
                 res.sendStatus(500)
               }
             }
-            logger.warn({ err, projectId, url }, 'CLSI proxy error')
+            const reqAborted = Boolean(req.destroyed)
+            if (reqAborted) {
+              Metrics.inc('proxy_to_clsi', 1, {
+                path: action,
+                status: 'req-aborted-late',
+              })
+            } else {
+              Metrics.inc('proxy_to_clsi', 1, { path: action, status: 'error' })
+            }
+            logger.warn(
+              { err, projectId, url, action, reqAborted, duration },
+              'CLSI proxy error'
+            )
           })
       }
     )

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

@@ -78,7 +78,12 @@ describe('CompileController', function () {
         '@overleaf/fetch-utils': this.fetchUtils,
         request: (this.request = sinon.stub()),
         '../Project/ProjectGetter': (this.ProjectGetter = {}),
-        '@overleaf/metrics': (this.Metrics = { inc: sinon.stub() }),
+        '@overleaf/metrics': (this.Metrics = {
+          inc: sinon.stub(),
+          Timer: class {
+            done() {}
+          },
+        }),
         './CompileManager': this.CompileManager,
         '../User/UserGetter': this.UserGetter,
         './ClsiManager': this.ClsiManager,
@@ -409,6 +414,7 @@ describe('CompileController', function () {
         this.CompileController.proxyToClsi
           .calledWith(
             this.projectId,
+            'output-file',
             `/project/${this.projectId}/user/${this.user_id}/output/output.pdf`,
             {},
             this.req,
@@ -432,6 +438,7 @@ describe('CompileController', function () {
         this.CompileController.proxyToClsi
           .calledWith(
             this.projectId,
+            'output-file',
             `/project/${this.projectId}/user/${this.user_id}/build/${this.buildId}/output/output.pdf`,
             {},
             this.req,
@@ -470,6 +477,7 @@ describe('CompileController', function () {
       it('should proxy to CLSI with correct URL and default limits', function () {
         this.CompileController.proxyToClsiWithLimits.should.have.been.calledWith(
           this.submission_id,
+          'output-file',
           this.expected_url,
           {},
           {
@@ -493,6 +501,7 @@ describe('CompileController', function () {
       it('should proxy to CLSI with correct URL and specified limits', function () {
         this.CompileController.proxyToClsiWithLimits.should.have.been.calledWith(
           this.submission_id,
+          'output-file',
           this.expected_url,
           {},
           {
@@ -526,6 +535,7 @@ describe('CompileController', function () {
     it('should proxy the request with an imageName', function () {
       expect(this.CompileController.proxyToClsi).to.have.been.calledWith(
         this.projectId,
+        'sync-to-code',
         `/project/${this.projectId}/user/${this.user_id}/sync/code`,
         { file, line, column, imageName },
         this.req,
@@ -558,6 +568,7 @@ describe('CompileController', function () {
     it('should proxy the request with an imageName', function () {
       expect(this.CompileController.proxyToClsi).to.have.been.calledWith(
         this.projectId,
+        'sync-to-pdf',
         `/project/${this.projectId}/user/${this.user_id}/sync/pdf`,
         { page, h, v, imageName },
         this.req,
@@ -590,6 +601,7 @@ describe('CompileController', function () {
             })
           this.CompileController.proxyToClsi(
             this.projectId,
+            'output-file',
             (this.url = '/test'),
             { query: 'foo' },
             this.req,
@@ -620,6 +632,7 @@ describe('CompileController', function () {
             })
           this.CompileController.proxyToClsi(
             this.projectId,
+            'output-file',
             (this.url = '/test'),
             {},
             this.req,
@@ -647,6 +660,7 @@ describe('CompileController', function () {
             })
           this.CompileController.proxyToClsi(
             this.projectId,
+            'output-file',
             (this.url = '/test'),
             {},
             this.req,
@@ -678,6 +692,7 @@ describe('CompileController', function () {
             })
           this.CompileController.proxyToClsi(
             this.projectId,
+            'output-file',
             (this.url = '/test'),
             {},
             this.req,
@@ -705,6 +720,7 @@ describe('CompileController', function () {
           this.req.query = { build: 1234 }
           this.CompileController.proxyToClsi(
             this.projectId,
+            'output-file',
             (this.url = '/test'),
             {},
             this.req,
@@ -765,6 +781,7 @@ describe('CompileController', function () {
       sinon.assert.calledWith(
         this.CompileController.proxyToClsi,
         this.projectId,
+        'output-file',
         `/project/${this.projectId}/output/output.pdf`,
         {},
         this.req,
@@ -774,6 +791,7 @@ describe('CompileController', function () {
       this.CompileController.proxyToClsi
         .calledWith(
           this.projectId,
+          'output-file',
           `/project/${this.projectId}/output/output.pdf`,
           {},
           this.req,