From 80ede301faa9d7a8b9434e661dbf6f3c84c6f224 Mon Sep 17 00:00:00 2001 From: Andrew Rumble Date: Fri, 24 May 2024 10:38:12 +0100 Subject: [PATCH] Merge pull request #18474 from overleaf/ar-return-build-id-from-clsi-after-compile [clsi] Return buildId after compiles GitOrigin-RevId: 872048f4fea8f5a00b817e29bd26a444d179a45f --- services/clsi/app/js/CompileController.js | 4 +++- services/clsi/app/js/CompileManager.js | 16 ++++++++++------ services/clsi/app/js/OutputCacheManager.js | 8 ++++---- .../clsi/test/unit/js/CompileControllerTests.js | 16 +++++++++++++++- .../clsi/test/unit/js/CompileManagerTests.js | 5 ++++- 5 files changed, 36 insertions(+), 13 deletions(-) diff --git a/services/clsi/app/js/CompileController.js b/services/clsi/app/js/CompileController.js index 2c893226d8..9e12cf28b7 100644 --- a/services/clsi/app/js/CompileController.js +++ b/services/clsi/app/js/CompileController.js @@ -30,7 +30,7 @@ function compile(req, res, next) { return next(error) } CompileManager.doCompileWithLock(request, (error, result) => { - let { outputFiles, stats, timings } = result || {} + let { buildId, outputFiles, stats, timings } = result || {} let code, status if (outputFiles == null) { outputFiles = [] @@ -98,6 +98,7 @@ function compile(req, res, next) { if (error) { outputFiles = error.outputFiles || [] + buildId = error.buildId } timer.done() @@ -107,6 +108,7 @@ function compile(req, res, next) { error: error?.message || error, stats, timings, + buildId, outputUrlPrefix: Settings.apis.clsi.outputUrlPrefix, outputFiles: outputFiles.map(file => ({ url: diff --git a/services/clsi/app/js/CompileManager.js b/services/clsi/app/js/CompileManager.js index 2a685d3e86..9d1981263d 100644 --- a/services/clsi/app/js/CompileManager.js +++ b/services/clsi/app/js/CompileManager.js @@ -208,7 +208,7 @@ async function doCompile(request) { Metrics.inc('compiles-timeout', 1, request.metricsOpts) } - const { outputFiles, allEntries } = await _saveOutputFiles({ + const { outputFiles, allEntries, buildId } = await _saveOutputFiles({ request, compileDir, resourceList, @@ -216,7 +216,7 @@ async function doCompile(request) { timings, }) error.outputFiles = outputFiles // return output files so user can check logs - + error.buildId = buildId // Clear project if this compile was abruptly terminated if (error.terminated || error.timedout) { await clearProjectWithListing( @@ -280,7 +280,7 @@ async function doCompile(request) { // Emit compile time. timings.compile = ts - const { outputFiles } = await _saveOutputFiles({ + const { outputFiles, buildId } = await _saveOutputFiles({ request, compileDir, resourceList, @@ -296,7 +296,7 @@ async function doCompile(request) { emitPdfStats(stats, timings, request) } - return { outputFiles, stats, timings } + return { outputFiles, stats, timings, buildId } } async function _saveOutputFiles({ @@ -316,20 +316,24 @@ async function _saveOutputFiles({ let { outputFiles, allEntries } = await OutputFileFinder.promises.findOutputFiles(resourceList, compileDir) + let buildId + try { - outputFiles = await OutputCacheManager.promises.saveOutputFiles( + const saveResult = await OutputCacheManager.promises.saveOutputFiles( { request, stats, timings }, outputFiles, compileDir, outputDir ) + buildId = saveResult.buildId + outputFiles = saveResult.outputFiles } catch (err) { const { project_id: projectId, user_id: userId } = request logger.err({ projectId, userId, err }, 'failed to save output files') } timings.output = timer.done() - return { outputFiles, allEntries } + return { outputFiles, allEntries, buildId } } async function stopCompile(projectId, userId) { diff --git a/services/clsi/app/js/OutputCacheManager.js b/services/clsi/app/js/OutputCacheManager.js index 5e7af34834..13aad1bc74 100644 --- a/services/clsi/app/js/OutputCacheManager.js +++ b/services/clsi/app/js/OutputCacheManager.js @@ -164,7 +164,7 @@ module.exports = OutputCacheManager = { outputDir, stats, (err, outputFiles) => { - if (err) return callback(err, outputFiles) + if (err) return callback(err, { outputFiles, buildId }) const enablePdfCaching = request.enablePdfCaching const enablePdfCachingDark = @@ -173,7 +173,7 @@ module.exports = OutputCacheManager = { !Settings.enablePdfCaching || (!enablePdfCaching && !enablePdfCachingDark) ) { - return callback(null, outputFiles) + return callback(null, { outputFiles, buildId }) } OutputCacheManager.saveStreamsInContentDir( @@ -191,9 +191,9 @@ module.exports = OutputCacheManager = { { err, outputDir, stats, timings }, 'pdf caching failed' ) - return callback(null, outputFiles) + return callback(null, { outputFiles, buildId }) } - callback(err, outputFiles) + callback(err, { outputFiles, buildId }) } ) } diff --git a/services/clsi/test/unit/js/CompileControllerTests.js b/services/clsi/test/unit/js/CompileControllerTests.js index 041e03bbaf..6a8793d1cf 100644 --- a/services/clsi/test/unit/js/CompileControllerTests.js +++ b/services/clsi/test/unit/js/CompileControllerTests.js @@ -50,6 +50,7 @@ function tryImageNameValidation(method, imageNameField) { describe('CompileController', function () { beforeEach(function () { + this.buildId = 'build-id-123' this.CompileController = SandboxedModule.require(modulePath, { requires: { './CompileManager': (this.CompileManager = {}), @@ -118,6 +119,7 @@ describe('CompileController', function () { outputFiles: this.output_files, stats: this.stats, timings: this.timings, + buildId: this.buildId, }) this.CompileController.compile(this.req, this.res) }) @@ -147,6 +149,7 @@ describe('CompileController', function () { error: null, stats: this.stats, timings: this.timings, + buildId: this.buildId, outputUrlPrefix: '/zone/b', outputFiles: this.output_files.map(file => ({ url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${file.build}/output/${file.path}`, @@ -165,6 +168,7 @@ describe('CompileController', function () { outputFiles: this.output_files, stats: this.stats, timings: this.timings, + buildId: this.buildId, }) this.CompileController.compile(this.req, this.res) }) @@ -178,6 +182,7 @@ describe('CompileController', function () { error: null, stats: this.stats, timings: this.timings, + buildId: this.buildId, outputUrlPrefix: '', outputFiles: this.output_files.map(file => ({ url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${file.build}/output/${file.path}`, @@ -207,6 +212,7 @@ describe('CompileController', function () { outputFiles: this.output_files, stats: this.stats, timings: this.timings, + buildId: this.buildId, }) this.CompileController.compile(this.req, this.res) }) @@ -221,6 +227,7 @@ describe('CompileController', function () { stats: this.stats, timings: this.timings, outputUrlPrefix: '/zone/b', + buildId: this.buildId, outputFiles: this.output_files.map(file => ({ url: `${this.Settings.apis.clsi.url}/project/${this.project_id}/build/${file.build}/output/${file.path}`, ...file, @@ -250,6 +257,7 @@ describe('CompileController', function () { outputFiles: this.output_files, stats: this.stats, timings: this.timings, + buildId: this.buildId, }) this.CompileController.compile(this.req, this.res) }) @@ -262,6 +270,7 @@ describe('CompileController', function () { status: 'failure', error: null, stats: this.stats, + buildId: this.buildId, timings: this.timings, outputUrlPrefix: '/zone/b', outputFiles: this.output_files.map(file => ({ @@ -276,9 +285,11 @@ describe('CompileController', function () { describe('with an error', function () { beforeEach(function () { + const error = new Error((this.message = 'error message')) + error.buildId = this.buildId this.CompileManager.doCompileWithLock = sinon .stub() - .callsArgWith(1, new Error((this.message = 'error message')), null) + .callsArgWith(1, error, null) this.CompileController.compile(this.req, this.res) }) @@ -291,6 +302,7 @@ describe('CompileController', function () { error: this.message, outputUrlPrefix: '/zone/b', outputFiles: [], + buildId: this.buildId, // JSON.stringify will omit these stats: undefined, timings: undefined, @@ -320,6 +332,7 @@ describe('CompileController', function () { outputUrlPrefix: '/zone/b', outputFiles: [], // JSON.stringify will omit these + buildId: undefined, stats: undefined, timings: undefined, }, @@ -346,6 +359,7 @@ describe('CompileController', function () { outputUrlPrefix: '/zone/b', outputFiles: [], // JSON.stringify will omit these + buildId: undefined, stats: undefined, timings: undefined, }, diff --git a/services/clsi/test/unit/js/CompileManagerTests.js b/services/clsi/test/unit/js/CompileManagerTests.js index 6aa416afae..6f5b5baf64 100644 --- a/services/clsi/test/unit/js/CompileManagerTests.js +++ b/services/clsi/test/unit/js/CompileManagerTests.js @@ -35,6 +35,7 @@ describe('CompileManager', function () { build: 1234, }, ] + this.buildId = 'build-id-123' this.commandOutput = 'Dummy output' this.compileBaseDir = '/compile/dir' this.outputBaseDir = '/output/dir' @@ -61,7 +62,9 @@ describe('CompileManager', function () { } this.OutputCacheManager = { promises: { - saveOutputFiles: sinon.stub().resolves(this.buildFiles), + saveOutputFiles: sinon + .stub() + .resolves({ outputFiles: this.buildFiles, buildId: this.buildId }), }, } this.Settings = {