[clsi] Forward pandoc errors to web (#33971)

* [clsi] Forward pandoc errors to web

* [clsi] Remove unused import

* [clsi] Align warning logs

* [clsi] Update HTTP response for errors

* [clsi] Update acceptance test with 422

* [clsi] Always return json body on 422

* [clsi] Include stderr in logs for non user facing errors

GitOrigin-RevId: 4284c8d4e8b7b45eac4997cd9e52ca4894b20412

services/clsi/app/js/ConversionController.js

@@ -11,6 +11,7 @@ import RequestParser from './RequestParser.js'
 import { pipeline } from 'node:stream/promises'
 import Settings from '@overleaf/settings'
 import Path from 'node:path'
+import { ConversionError } from './Errors.js'
 
 const CONVERSION_CONFIGS = {
   docx: { extension: 'docx' },
@@ -37,6 +38,23 @@ async function convertDocumentToLaTeX(req, res) {
       path,
       conversionType
     )
+  } catch (err) {
+    if (err instanceof ConversionError) {
+      if (err.isUserFacing) {
+        return res.status(422).json({
+          error: err.stderr,
+          exitCode: err.exitCode,
+        })
+      } else {
+        logger.warn(
+          { err, conversionType, stderr: err.stderr },
+          'Conversion failed with non-user-facing error'
+        )
+        return res.status(422).json({})
+      }
+    } else {
+      throw err
+    }
   } finally {
     await fs.unlink(path).catch(() => {})
   }
@@ -124,6 +142,23 @@ async function convertProjectToDocument(req, res) {
       const readStream = fsSync.createReadStream(documentPath)
       await pipeline(readStream, res)
     }
+  } catch (err) {
+    if (err instanceof ConversionError) {
+      if (err.isUserFacing) {
+        return res.status(422).json({
+          error: err.stderr,
+          exitCode: err.exitCode,
+        })
+      } else {
+        logger.warn(
+          { err, type, stderr: err.stderr },
+          'Conversion failed with non-user-facing error'
+        )
+        return res.status(422).json({})
+      }
+    } else {
+      throw err
+    }
   } finally {
     await fs.rm(conversionDir, { recursive: true, force: true }).catch(() => {})
   }

services/clsi/app/js/ConversionManager.js

@@ -5,6 +5,7 @@ import Path from 'node:path'
 import CommandRunner from './CommandRunner.js'
 import LockManager from './LockManager.js'
 import OError from '@overleaf/o-error'
+import { ConversionError } from './Errors.js'
 
 const CONVERSION_CONFIGS = {
   docx: {
@@ -72,7 +73,8 @@ async function convertToLaTeX(
       null
     )
     if (exitCodePandoc !== 0) {
-      throw new OError('Non-zero exit code from pandoc', {
+      throw new ConversionError('Non-zero exit code from pandoc', {
+        type: conversionType,
         exitCode: exitCodePandoc,
         stderr: stderrPandoc,
       })
@@ -112,6 +114,9 @@ async function convertToLaTeX(
   } catch (error) {
     // Clean up the conversion directory on error to avoid leaving failed conversions around
     await fs.rm(conversionDir, { force: true, recursive: true }).catch(() => {})
+    if (error instanceof ConversionError) {
+      throw error
+    }
     throw new OError('pandoc conversion failed').withCause(error)
   }
 
@@ -204,10 +209,9 @@ async function convertLaTeXToDocumentInDir(
     )
 
     if (exitCode !== 0) {
-      throw new OError('pandoc latex-to-document conversion failed', {
+      throw new ConversionError('pandoc latex-to-document conversion failed', {
         type,
         exitCode,
-        stdout,
         stderr,
       })
     }
@@ -251,10 +255,9 @@ async function convertLaTeXToDocumentInDir(
   )
 
   if (exitCode !== 0) {
-    throw new OError('pandoc latex-to-document conversion failed', {
+    throw new ConversionError('pandoc latex-to-document conversion failed', {
       type,
       exitCode,
-      stdout,
       stderr,
     })
   }

services/clsi/app/js/Errors.js

@@ -35,6 +35,37 @@ export class TooManyCompileRequestsError extends OError {}
 export class InvalidParameter extends OError {}
 export class MissingUpdatesError extends OError {}
 
+export class ConversionError extends OError {
+  static USER_FACING_ERRORS = new Set([
+    1, // IO error
+    23, // Unsupported extension
+    24, // Citeproc error
+    25, // Other bibliography error
+    44, // Malformed XML error
+    63, // Generic error (e.g. malformed docx container)
+    64, // Parse error
+    91, // Macro loop
+    92, // UTF8 decoding error
+    94, // Unsupported char set
+    95, // Input not text
+    97, // Missing data file
+    98, // Missing metadata file
+    99, // Missing file
+  ])
+
+  isUserFacing
+  stderr
+  exitCode
+
+  constructor(message, { type, stderr, exitCode }) {
+    const isUserFacingError = ConversionError.USER_FACING_ERRORS.has(exitCode)
+    super(message, { exitCode, type })
+    this.isUserFacing = isUserFacingError
+    this.stderr = stderr
+    this.exitCode = exitCode
+  }
+}
+
 export default {
   QueueLimitReachedError,
   TimedOutError,
@@ -45,4 +76,5 @@ export default {
   TooManyCompileRequestsError,
   InvalidParameter,
   MissingUpdatesError,
+  ConversionError,
 }

services/clsi/test/acceptance/js/ConversionTests.js

@@ -28,7 +28,7 @@ describe('Conversions', function () {
       const outputStream = fs.createWriteStream(
         '/tmp/clsi_acceptance_tests_' + crypto.randomUUID() + '.zip'
       )
-      const stream = await Client.convertDocument(sourcePath, 'docx')
+      const { stream } = await Client.convertDocument(sourcePath, 'docx')
       await pipeline(stream, outputStream)
 
       await new Promise((resolve, reject) => {
@@ -75,13 +75,14 @@ describe('Conversions', function () {
       })
     })
 
-    it('should fail when file is not a docx', async function () {
+    it('should fail with 422 and surface pandoc stderr when file is not a docx', async function () {
       const sourcePath = Path.join(
         import.meta.dirname,
         '../fixtures/minimal.pdf'
       )
-      await expect(Client.convertDocument(sourcePath, 'docx')).to.eventually.be
-        .rejected
+      const { status, body } = await Client.convertDocument(sourcePath, 'docx')
+      expect(status).to.equal(422)
+      expect(body.error).to.include("couldn't unpack docx container")
     })
   })
 

services/clsi/test/acceptance/js/helpers/Client.js

@@ -33,10 +33,24 @@ function compile(projectId, data) {
 async function convertDocument(path, type) {
   const formData = new FormData()
   formData.append('qqfile', fs.createReadStream(path))
-  return await fetchStream(`${host}/convert/document-to-latex?type=${type}`, {
-    method: 'POST',
-    body: formData,
-  })
+  try {
+    const stream = await fetchStream(
+      `${host}/convert/document-to-latex?type=${type}`,
+      {
+        method: 'POST',
+        body: formData,
+      }
+    )
+    return { status: 200, stream, body: null }
+  } catch (err) {
+    if (!err.response) throw err
+    let body = err.body
+    const contentType = err.response.headers.get?.('content-type') ?? ''
+    if (contentType.includes('application/json')) {
+      body = JSON.parse(body)
+    }
+    return { status: err.response.status, stream: null, body }
+  }
 }
 
 async function convertProjectToDocument(

services/clsi/test/unit/js/ConversionController.test.js

@@ -2,6 +2,7 @@ import sinon from 'sinon'
 import { vi, describe, it, beforeEach, expect } from 'vitest'
 import Path from 'node:path'
 import { PassThrough } from 'node:stream'
+import * as Errors from '../../../app/js/Errors.js'
 
 const MODULE_PATH = Path.join(
   import.meta.dirname,
@@ -98,6 +99,8 @@ describe('ConversionController', function () {
       default: ctx.ConversionOutputCleaner,
     }))
 
+    vi.doMock('../../../app/js/Errors', () => Errors)
+
     ctx.res = new PassThrough()
     ctx.res.attachment = sinon.stub()
     ctx.res.setHeader = sinon.stub()
@@ -278,6 +281,73 @@ describe('ConversionController', function () {
           sinon.assert.calledWith(ctx.fs.rm, ctx.conversionDir)
         })
       })
+
+      describe('on a user-facing ConversionError', function () {
+        beforeEach(async function (ctx) {
+          ctx.jsonStub = sinon.stub()
+          ctx.res.status = sinon.stub().returns({ json: ctx.jsonStub })
+          ctx.req = {
+            file: { path: '/path/to/uploaded/file.docx' },
+            query: { type: 'docx' },
+          }
+          ctx.ConversionManager.promises.convertToLaTeXWithLock.rejects(
+            new Errors.ConversionError('Non-zero exit code from pandoc', {
+              type: 'docx',
+              exitCode: 64,
+              stderr: 'parse error at line 5',
+            })
+          )
+
+          await ctx.ConversionController.convertDocumentToLaTeX(
+            ctx.req,
+            ctx.res
+          )
+        })
+
+        it('should return 422 with the pandoc stderr in the response body', function (ctx) {
+          sinon.assert.calledWith(ctx.res.status, 422)
+          sinon.assert.calledWith(ctx.jsonStub, {
+            error: 'parse error at line 5',
+            exitCode: 64,
+          })
+        })
+
+        it('should remove the uploaded file', function (ctx) {
+          sinon.assert.calledWith(ctx.fs.unlink, ctx.req.file.path)
+        })
+      })
+
+      describe('on a non-user-facing ConversionError', function () {
+        beforeEach(async function (ctx) {
+          ctx.jsonStub = sinon.stub()
+          ctx.res.status = sinon.stub().returns({ json: ctx.jsonStub })
+          ctx.req = {
+            file: { path: '/path/to/uploaded/file.docx' },
+            query: { type: 'docx' },
+          }
+          ctx.ConversionManager.promises.convertToLaTeXWithLock.rejects(
+            new Errors.ConversionError('Non-zero exit code from pandoc', {
+              type: 'docx',
+              exitCode: 62,
+              stderr: 'internal pandoc bug',
+            })
+          )
+
+          await ctx.ConversionController.convertDocumentToLaTeX(
+            ctx.req,
+            ctx.res
+          )
+        })
+
+        it('should return 422 without surfacing stderr', function (ctx) {
+          sinon.assert.calledWith(ctx.res.status, 422)
+          sinon.assert.calledWith(ctx.jsonStub, {})
+        })
+
+        it('should remove the uploaded file', function (ctx) {
+          sinon.assert.calledWith(ctx.fs.unlink, ctx.req.file.path)
+        })
+      })
     })
   })
 
@@ -531,5 +601,88 @@ describe('ConversionController', function () {
         })
       })
     })
+
+    describe('when conversion fails with a user-facing ConversionError', function () {
+      beforeEach(async function (ctx) {
+        ctx.next = sinon.stub()
+        ctx.jsonStub = sinon.stub()
+        ctx.res.status = sinon.stub().returns({ json: ctx.jsonStub })
+        ctx.ConversionManager.promises.convertLaTeXToDocumentInDirWithLock.rejects(
+          new Errors.ConversionError(
+            'pandoc latex-to-document conversion failed',
+            {
+              type: 'docx',
+              exitCode: 64,
+              stderr: 'parse error at line 5',
+            }
+          )
+        )
+
+        await ctx.ConversionController.convertProjectToDocument(
+          ctx.req,
+          ctx.res,
+          ctx.next
+        )
+      })
+
+      it('should return 422 with the pandoc stderr in the response body', function (ctx) {
+        sinon.assert.calledWith(ctx.res.status, 422)
+        sinon.assert.calledWith(ctx.jsonStub, {
+          error: 'parse error at line 5',
+          exitCode: 64,
+        })
+      })
+
+      it('should not call next', function (ctx) {
+        sinon.assert.notCalled(ctx.next)
+      })
+
+      it('should still clean up the conversion directory', function (ctx) {
+        sinon.assert.calledWith(ctx.fs.rm, sinon.match(uuidDirPattern), {
+          recursive: true,
+          force: true,
+        })
+      })
+    })
+
+    describe('when conversion fails with a non-user-facing ConversionError', function () {
+      beforeEach(async function (ctx) {
+        ctx.next = sinon.stub()
+        ctx.jsonStub = sinon.stub()
+        ctx.res.status = sinon.stub().returns({ json: ctx.jsonStub })
+        ctx.ConversionManager.promises.convertLaTeXToDocumentInDirWithLock.rejects(
+          new Errors.ConversionError(
+            'pandoc latex-to-document conversion failed',
+            {
+              type: 'docx',
+              exitCode: 62,
+              stderr: 'internal pandoc bug',
+            }
+          )
+        )
+
+        await ctx.ConversionController.convertProjectToDocument(
+          ctx.req,
+          ctx.res,
+          ctx.next
+        )
+      })
+
+      it('should return 422 without surfacing stderr', function (ctx) {
+        sinon.assert.calledWith(ctx.res.status, 422)
+        sinon.assert.calledWith(ctx.jsonStub, {})
+      })
+
+      it('should not call next', function (ctx) {
+        sinon.assert.notCalled(ctx.next)
+      })
+
+      it('should still clean up the conversion directory', function (ctx) {
+        sinon.assert.calledWith(ctx.fs.rm, sinon.match(uuidDirPattern), {
+          recursive: true,
+          force: true,
+        })
+      })
+    })
   })
 })

services/clsi/test/unit/js/ConversionManager.test.js

@@ -233,7 +233,7 @@ describe('ConversionManager', function () {
               ctx.inputPath,
               'docx'
             )
-          ).to.be.rejectedWith('pandoc conversion failed')
+          ).to.be.rejectedWith('Non-zero exit code from pandoc')
         })
 
         it('should remove the entire conversion directory', function (ctx) {