Merge pull request #204 from overleaf/bg-use-output-dir

use separate output dir for generated files

services/clsi/.gitignore

@@ -2,6 +2,7 @@
 node_modules
 test/acceptance/fixtures/tmp
 compiles
+output
 .DS_Store
 *~
 cache

services/clsi/Dockerfile

@@ -22,7 +22,7 @@ COPY . /app
 FROM base
 
 COPY --from=app /app /app
-RUN mkdir -p cache compiles db \
-&&  chown node:node cache compiles db
+RUN mkdir -p cache compiles db output \
+  &&  chown node:node cache compiles db output
 
 CMD ["node", "--expose-gc", "app.js"]

services/clsi/app.js

@@ -118,21 +118,45 @@ const ForbidSymlinks = require('./app/js/StaticServerForbidSymlinks')
 // create a static server which does not allow access to any symlinks
 // avoids possible mismatch of root directory between middleware check
 // and serving the files
-const staticServer = ForbidSymlinks(express.static, Settings.path.compilesDir, {
-  setHeaders(res, path, stat) {
-    if (Path.basename(path) === 'output.pdf') {
-      // Calculate an etag in the same way as nginx
-      // https://github.com/tj/send/issues/65
-      const etag = (path, stat) =>
-        `"${Math.ceil(+stat.mtime / 1000).toString(16)}` +
-        '-' +
-        Number(stat.size).toString(16) +
-        '"'
-      res.set('Etag', etag(path, stat))
+const staticCompileServer = ForbidSymlinks(
+  express.static,
+  Settings.path.compilesDir,
+  {
+    setHeaders(res, path, stat) {
+      if (Path.basename(path) === 'output.pdf') {
+        // Calculate an etag in the same way as nginx
+        // https://github.com/tj/send/issues/65
+        const etag = (path, stat) =>
+          `"${Math.ceil(+stat.mtime / 1000).toString(16)}` +
+          '-' +
+          Number(stat.size).toString(16) +
+          '"'
+        res.set('Etag', etag(path, stat))
+      }
+      return res.set('Content-Type', ContentTypeMapper.map(path))
     }
-    return res.set('Content-Type', ContentTypeMapper.map(path))
   }
-})
+)
+
+const staticOutputServer = ForbidSymlinks(
+  express.static,
+  Settings.path.outputDir,
+  {
+    setHeaders(res, path, stat) {
+      if (Path.basename(path) === 'output.pdf') {
+        // Calculate an etag in the same way as nginx
+        // https://github.com/tj/send/issues/65
+        const etag = (path, stat) =>
+          `"${Math.ceil(+stat.mtime / 1000).toString(16)}` +
+          '-' +
+          Number(stat.size).toString(16) +
+          '"'
+        res.set('Etag', etag(path, stat))
+      }
+      return res.set('Content-Type', ContentTypeMapper.map(path))
+    }
+  }
+)
 
 app.get(
   '/project/:project_id/user/:user_id/build/:build_id/output/*',
@@ -141,7 +165,7 @@ app.get(
     req.url =
       `/${req.params.project_id}-${req.params.user_id}/` +
       OutputCacheManager.path(req.params.build_id, `/${req.params[0]}`)
-    return staticServer(req, res, next)
+    return staticOutputServer(req, res, next)
   }
 )
 
@@ -154,7 +178,7 @@ app.get('/project/:project_id/build/:build_id/output/*', function (
   req.url =
     `/${req.params.project_id}/` +
     OutputCacheManager.path(req.params.build_id, `/${req.params[0]}`)
-  return staticServer(req, res, next)
+  return staticOutputServer(req, res, next)
 })
 
 app.get('/project/:project_id/user/:user_id/output/*', function (
@@ -163,11 +187,13 @@ app.get('/project/:project_id/user/:user_id/output/*', function (
   next
 ) {
   // for specific user get the path to the top level file
+  logger.warn({ url: req.url }, 'direct request for file in compile directory')
   req.url = `/${req.params.project_id}-${req.params.user_id}/${req.params[0]}`
-  return staticServer(req, res, next)
+  return staticCompileServer(req, res, next)
 })
 
 app.get('/project/:project_id/output/*', function (req, res, next) {
+  logger.warn({ url: req.url }, 'direct request for file in compile directory')
   if (
     (req.query != null ? req.query.build : undefined) != null &&
     req.query.build.match(OutputCacheManager.BUILD_REGEX)
@@ -179,7 +205,7 @@ app.get('/project/:project_id/output/*', function (req, res, next) {
   } else {
     req.url = `/${req.params.project_id}/${req.params[0]}`
   }
-  return staticServer(req, res, next)
+  return staticCompileServer(req, res, next)
 })
 
 app.get('/oops', function (req, res, next) {

services/clsi/app/js/CompileManager.js

@@ -46,6 +46,9 @@ const getCompileName = function (project_id, user_id) {
 const getCompileDir = (project_id, user_id) =>
   Path.join(Settings.path.compilesDir, getCompileName(project_id, user_id))
 
+const getOutputDir = (project_id, user_id) =>
+  Path.join(Settings.path.outputDir, getCompileName(project_id, user_id))
+
 module.exports = CompileManager = {
   doCompileWithLock(request, callback) {
     if (callback == null) {
@@ -72,6 +75,8 @@ module.exports = CompileManager = {
       callback = function (error, outputFiles) {}
     }
     const compileDir = getCompileDir(request.project_id, request.user_id)
+    const outputDir = getOutputDir(request.project_id, request.user_id)
+
     let timer = new Metrics.Timer('write-to-disk')
     logger.log(
       { project_id: request.project_id, user_id: request.user_id },
@@ -294,6 +299,7 @@ module.exports = CompileManager = {
                   return OutputCacheManager.saveOutputFiles(
                     outputFiles,
                     compileDir,
+                    outputDir,
                     (error, newOutputFiles) => callback(null, newOutputFiles)
                   )
                 }
@@ -323,6 +329,7 @@ module.exports = CompileManager = {
     }
 
     const compileDir = getCompileDir(project_id, user_id)
+    const outputDir = getOutputDir(project_id, user_id)
 
     return CompileManager._checkDirectory(compileDir, function (err, exists) {
       if (err != null) {
@@ -332,7 +339,13 @@ module.exports = CompileManager = {
         return callback()
       } // skip removal if no directory present
 
-      const proc = child_process.spawn('rm', ['-r', compileDir])
+      const proc = child_process.spawn('rm', [
+        '-r',
+        '-f',
+        '--',
+        compileDir,
+        outputDir
+      ])
 
       proc.on('error', callback)
 
@@ -343,7 +356,9 @@ module.exports = CompileManager = {
         if (code === 0) {
           return callback(null)
         } else {
-          return callback(new Error(`rm -r ${compileDir} failed: ${stderr}`))
+          return callback(
+            new Error(`rm -r ${compileDir} ${outputDir} failed: ${stderr}`)
+          )
         }
       })
     })

services/clsi/app/js/OutputCacheManager.js

@@ -26,8 +26,8 @@ const crypto = require('crypto')
 const OutputFileOptimiser = require('./OutputFileOptimiser')
 
 module.exports = OutputCacheManager = {
-  CACHE_SUBDIR: '.cache/clsi',
-  ARCHIVE_SUBDIR: '.archive/clsi',
+  CACHE_SUBDIR: 'generated-files',
+  ARCHIVE_SUBDIR: 'archived-logs',
   // build id is HEXDATE-HEXRANDOM from Date.now()and RandomBytes
   // for backwards compatibility, make the randombytes part optional
   BUILD_REGEX: /^[0-9a-f]+(-[0-9a-f]+)?$/,
@@ -59,7 +59,7 @@ module.exports = OutputCacheManager = {
     })
   },
 
-  saveOutputFiles(outputFiles, compileDir, callback) {
+  saveOutputFiles(outputFiles, compileDir, outputDir, callback) {
     if (callback == null) {
       callback = function (error) {}
     }
@@ -70,22 +70,29 @@ module.exports = OutputCacheManager = {
       return OutputCacheManager.saveOutputFilesInBuildDir(
         outputFiles,
         compileDir,
+        outputDir,
         buildId,
         callback
       )
     })
   },
 
-  saveOutputFilesInBuildDir(outputFiles, compileDir, buildId, callback) {
+  saveOutputFilesInBuildDir(
+    outputFiles,
+    compileDir,
+    outputDir,
+    buildId,
+    callback
+  ) {
     // make a compileDir/CACHE_SUBDIR/build_id directory and
     // copy all the output files into it
     if (callback == null) {
       callback = function (error) {}
     }
-    const cacheRoot = Path.join(compileDir, OutputCacheManager.CACHE_SUBDIR)
+    const cacheRoot = Path.join(outputDir, OutputCacheManager.CACHE_SUBDIR)
     // Put the files into a new cache subdirectory
     const cacheDir = Path.join(
-      compileDir,
+      outputDir,
       OutputCacheManager.CACHE_SUBDIR,
       buildId
     )
@@ -102,6 +109,7 @@ module.exports = OutputCacheManager = {
       OutputCacheManager.archiveLogs(
         outputFiles,
         compileDir,
+        outputDir,
         buildId,
         function (err) {
           if (err != null) {
@@ -198,12 +206,12 @@ module.exports = OutputCacheManager = {
     })
   },
 
-  archiveLogs(outputFiles, compileDir, buildId, callback) {
+  archiveLogs(outputFiles, compileDir, outputDir, buildId, callback) {
     if (callback == null) {
       callback = function (error) {}
     }
     const archiveDir = Path.join(
-      compileDir,
+      outputDir,
       OutputCacheManager.ARCHIVE_SUBDIR,
       buildId
     )

services/clsi/config/settings.defaults.js

@@ -27,6 +27,7 @@ module.exports = {
 
   path: {
     compilesDir: Path.resolve(__dirname, '../compiles'),
+    outputDir: Path.resolve(__dirname, '../output'),
     clsiCacheDir: Path.resolve(__dirname, '../cache'),
     synctexBaseDir(projectId) {
       return Path.join(this.compilesDir, projectId)

services/clsi/test/unit/js/CompileManagerTests.js

@@ -34,7 +34,8 @@ describe('CompileManager', function () {
         './OutputCacheManager': (this.OutputCacheManager = {}),
         'settings-sharelatex': (this.Settings = {
           path: {
-            compilesDir: '/compiles/dir'
+            compilesDir: '/compiles/dir',
+            outputDir: '/output/dir'
           },
           synctexBaseDir() {
             return '/compile'
@@ -166,6 +167,7 @@ describe('CompileManager', function () {
       this.env = {}
       this.Settings.compileDir = 'compiles'
       this.compileDir = `${this.Settings.path.compilesDir}/${this.project_id}-${this.user_id}`
+      this.outputDir = `${this.Settings.path.outputDir}/${this.project_id}-${this.user_id}`
       this.ResourceWriter.syncResourcesToDisk = sinon
         .stub()
         .callsArgWith(2, null, this.resources)
@@ -175,7 +177,7 @@ describe('CompileManager', function () {
         .callsArgWith(2, null, this.output_files)
       this.OutputCacheManager.saveOutputFiles = sinon
         .stub()
-        .callsArgWith(2, null, this.build_files)
+        .callsArgWith(3, null, this.build_files)
       this.DraftModeManager.injectDraftMode = sinon.stub().callsArg(1)
       return (this.TikzManager.checkMainFile = sinon.stub().callsArg(3, false))
     })
@@ -312,7 +314,10 @@ describe('CompileManager', function () {
         return this.child_process.spawn
           .calledWith('rm', [
             '-r',
-            `${this.Settings.path.compilesDir}/${this.project_id}-${this.user_id}`
+            '-f',
+            '--',
+            `${this.Settings.path.compilesDir}/${this.project_id}-${this.user_id}`,
+            `${this.Settings.path.outputDir}/${this.project_id}-${this.user_id}`
           ])
           .should.equal(true)
       })
@@ -348,7 +353,10 @@ describe('CompileManager', function () {
         return this.child_process.spawn
           .calledWith('rm', [
             '-r',
-            `${this.Settings.path.compilesDir}/${this.project_id}-${this.user_id}`
+            '-f',
+            '--',
+            `${this.Settings.path.compilesDir}/${this.project_id}-${this.user_id}`,
+            `${this.Settings.path.outputDir}/${this.project_id}-${this.user_id}`
           ])
           .should.equal(true)
       })
@@ -357,7 +365,7 @@ describe('CompileManager', function () {
         this.callback.calledWithExactly(sinon.match(Error)).should.equal(true)
 
         this.callback.args[0][0].message.should.equal(
-          `rm -r ${this.Settings.path.compilesDir}/${this.project_id}-${this.user_id} failed: ${this.error}`
+          `rm -r ${this.Settings.path.compilesDir}/${this.project_id}-${this.user_id} ${this.Settings.path.outputDir}/${this.project_id}-${this.user_id} failed: ${this.error}`
         )
       })
     })