From 7adf52694c7e5e534717edfc32b5badeca07c65e Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Tue, 18 Mar 2025 09:26:43 +0000 Subject: [PATCH] [web] generate clsi buildId ahead of fetching project content (#24337) * [web] generate clsi buildId ahead of fetching project content The buildIds timestamp component will be used for cache invalidation. * [clsi] strict validation for buildId * [clsi] validate buildId parameter GitOrigin-RevId: 88e8b2d48e78fa137b6dca7f2e6b93bbcf88a777 --- services/clsi/app/js/OutputCacheManager.js | 13 +++++--- services/clsi/app/js/RequestParser.js | 13 ++++++++ .../test/unit/js/DockerLockManagerTests.js | 2 +- .../clsi/test/unit/js/LockManagerTests.js | 1 + .../clsi/test/unit/js/RequestParserTests.js | 32 +++++++++++++++++++ .../app/src/Features/Compile/ClsiManager.js | 1 + .../src/Features/Compile/CompileManager.js | 8 +++++ .../unit/src/Compile/CompileManagerTests.js | 1 + 8 files changed, 65 insertions(+), 6 deletions(-) diff --git a/services/clsi/app/js/OutputCacheManager.js b/services/clsi/app/js/OutputCacheManager.js index ceda308d8d..1e9a10c921 100644 --- a/services/clsi/app/js/OutputCacheManager.js +++ b/services/clsi/app/js/OutputCacheManager.js @@ -98,10 +98,9 @@ module.exports = OutputCacheManager = { CONTENT_SUBDIR: 'content', 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]+)?$/, - CONTENT_REGEX: /^[0-9a-f]+(-[0-9a-f]+)?$/, + // build id is HEXDATE-HEXRANDOM from Date.now() and RandomBytes + BUILD_REGEX: /^[0-9a-f]+-[0-9a-f]+$/, + CONTENT_REGEX: /^[0-9a-f]+-[0-9a-f]+$/, CACHE_LIMIT: 2, // maximum number of cache directories CACHE_AGE: 90 * 60 * 1000, // up to 90 minutes old @@ -137,7 +136,11 @@ module.exports = OutputCacheManager = { outputDir, callback ) { - OutputCacheManager.generateBuildId(function (err, buildId) { + const getBuildId = cb => { + if (request.buildId) return cb(null, request.buildId) + OutputCacheManager.generateBuildId(cb) + } + getBuildId(function (err, buildId) { if (err) { return callback(err) } diff --git a/services/clsi/app/js/RequestParser.js b/services/clsi/app/js/RequestParser.js index 28e182ea44..f5c07d3bcf 100644 --- a/services/clsi/app/js/RequestParser.js +++ b/services/clsi/app/js/RequestParser.js @@ -1,4 +1,5 @@ const settings = require('@overleaf/settings') +const OutputCacheManager = require('./OutputCacheManager') const VALID_COMPILERS = ['pdflatex', 'latex', 'xelatex', 'lualatex'] const MAX_TIMEOUT = 600 @@ -135,6 +136,11 @@ function parse(body, callback) { } ) response.rootResourcePath = _checkPath(rootResourcePath) + + response.buildId = _parseAttribute('buildId', compile.options.buildId, { + type: 'string', + regex: OutputCacheManager.BUILD_REGEX, + }) } catch (error1) { const error = error1 return callback(error) @@ -199,6 +205,13 @@ function _parseAttribute(name, attribute, options) { throw new Error(`${name} attribute should be a ${options.type}`) } } + if (options.type === 'string' && options.regex instanceof RegExp) { + if (!options.regex.test(attribute)) { + throw new Error( + `${name} attribute does not match regex ${options.regex}` + ) + } + } } else { if (options.default != null) { return options.default diff --git a/services/clsi/test/unit/js/DockerLockManagerTests.js b/services/clsi/test/unit/js/DockerLockManagerTests.js index c494a85c66..f69179443c 100644 --- a/services/clsi/test/unit/js/DockerLockManagerTests.js +++ b/services/clsi/test/unit/js/DockerLockManagerTests.js @@ -16,7 +16,7 @@ const modulePath = require('node:path').join( '../../../app/js/DockerLockManager' ) -describe('LockManager', function () { +describe('DockerLockManager', function () { beforeEach(function () { return (this.LockManager = SandboxedModule.require(modulePath, { requires: { diff --git a/services/clsi/test/unit/js/LockManagerTests.js b/services/clsi/test/unit/js/LockManagerTests.js index 64238ea0c6..7005b3e5a3 100644 --- a/services/clsi/test/unit/js/LockManagerTests.js +++ b/services/clsi/test/unit/js/LockManagerTests.js @@ -21,6 +21,7 @@ describe('LockManager', function () { compileConcurrencyLimit: 5, }), './Errors': (this.Erros = Errors), + './RequestParser': { MAX_TIMEOUT: 600 }, }, }) }) diff --git a/services/clsi/test/unit/js/RequestParserTests.js b/services/clsi/test/unit/js/RequestParserTests.js index 5fdae7ed3d..437c3c4fbe 100644 --- a/services/clsi/test/unit/js/RequestParserTests.js +++ b/services/clsi/test/unit/js/RequestParserTests.js @@ -30,6 +30,7 @@ describe('RequestParser', function () { this.RequestParser = SandboxedModule.require(modulePath, { requires: { '@overleaf/settings': (this.settings = {}), + './OutputCacheManager': { BUILD_REGEX: /^[0-9a-f]+-[0-9a-f]+$/ }, }, }) }) @@ -274,6 +275,37 @@ describe('RequestParser', function () { }) }) + describe('with a valid buildId', function () { + beforeEach(function (done) { + this.validRequest.compile.options.buildId = '195a4869176-a4ad60bee7bf35e4' + this.RequestParser.parse(this.validRequest, (error, data) => { + if (error) return done(error) + this.data = data + done() + }) + }) + + it('should return an error', function () { + this.data.buildId.should.equal('195a4869176-a4ad60bee7bf35e4') + }) + }) + + describe('with a bad buildId', function () { + beforeEach(function () { + this.validRequest.compile.options.buildId = 'foo/bar' + this.RequestParser.parse(this.validRequest, this.callback) + }) + + it('should return an error', function () { + this.callback + .calledWithMatch({ + message: + 'buildId attribute does not match regex /^[0-9a-f]+-[0-9a-f]+$/', + }) + .should.equal(true) + }) + }) + describe('with a resource with a valid date', function () { beforeEach(function () { this.date = '12:00 01/02/03' diff --git a/services/web/app/src/Features/Compile/ClsiManager.js b/services/web/app/src/Features/Compile/ClsiManager.js index a211f82408..68c8a9c0de 100644 --- a/services/web/app/src/Features/Compile/ClsiManager.js +++ b/services/web/app/src/Features/Compile/ClsiManager.js @@ -765,6 +765,7 @@ function _finaliseRequest(projectId, options, project, docs, files) { return { compile: { options: { + buildId: options.buildId, compiler: project.compiler, timeout: options.timeout, imageName: project.imageName, diff --git a/services/web/app/src/Features/Compile/CompileManager.js b/services/web/app/src/Features/Compile/CompileManager.js index b4d34f1507..9b5404865a 100644 --- a/services/web/app/src/Features/Compile/CompileManager.js +++ b/services/web/app/src/Features/Compile/CompileManager.js @@ -1,4 +1,5 @@ let CompileManager +const Crypto = require('crypto') const Settings = require('@overleaf/settings') const RedisWrapper = require('../../infrastructure/RedisWrapper') const rclient = RedisWrapper.client('clsi_recently_compiled') @@ -25,6 +26,10 @@ function instrumentWithTimer(fn, key) { } } +function generateBuildId() { + return `${Date.now().toString(16)}-${Crypto.randomBytes(8).toString('hex')}` +} + async function compile(projectId, userId, options = {}) { const recentlyCompiled = await CompileManager._checkIfRecentlyCompiled( projectId, @@ -67,6 +72,9 @@ async function compile(projectId, userId, options = {}) { return { message: 'autocompile-backoff', outputFiles: [] } } + // Generate the buildId ahead of fetching the project content from redis/mongo so that the buildId's timestamp is before any lastUpdated date. + options.buildId = generateBuildId() + // only pass userId down to clsi if this is a per-user compile const compileAsUser = Settings.disablePerUserCompiles ? undefined : userId const { diff --git a/services/web/test/unit/src/Compile/CompileManagerTests.js b/services/web/test/unit/src/Compile/CompileManagerTests.js index 7fcbcee335..41b27cbd69 100644 --- a/services/web/test/unit/src/Compile/CompileManagerTests.js +++ b/services/web/test/unit/src/Compile/CompileManagerTests.js @@ -122,6 +122,7 @@ describe('CompileManager', function () { .calledWith(this.project_id, this.user_id, { timeout: this.limits.timeout, compileGroup: 'standard', + buildId: sinon.match(/[a-f0-9]+-[a-f0-9]+/), }) .should.equal(true) })