Merge pull request #31650 from overleaf/bg-handle-timeouts-in-persistence-manager

add missing handling of timeouts in PersistenceManager

GitOrigin-RevId: 7fe74068f3ea647b27103393c3f0b243b4b25fe3

services/document-updater/app/js/PersistenceManager.js

@@ -199,7 +199,11 @@ const RETRYABLE_ERRORS = new Set([
 
 function isRetryable(error) {
   // use the same retryable errors as requestretry
-  if (error instanceof Errors.WebApiServerError) {
+  // node-fetch uses AbortError:
+  // https://github.com/node-fetch/node-fetch/blob/main/docs/ERROR-HANDLING.md
+  if (error.name === 'AbortError') {
+    return true
+  } else if (error instanceof Errors.WebApiServerError) {
     const status = error.info?.status
     return (
       typeof status === 'number' &&

services/document-updater/test/acceptance/js/GettingADocumentTests.js

@@ -338,12 +338,48 @@ describe('Getting a document', function () {
     })
   })
 
-  describe('when the web api http request takes a long time', function () {
+  describe('when the web api http request times out on the first request', function () {
+    before(function (done) {
+      this.project_id = DocUpdaterClient.randomId()
+      this.doc_id = DocUpdaterClient.randomId()
+      MockWebApi.insertDoc(this.project_id, this.doc_id, {
+        lines: this.lines,
+        version: this.version,
+      })
+      sinon
+        .stub(MockWebApi, 'getDocument')
+        .onFirstCall()
+        .returns(
+          new Promise(resolve => {
+            setTimeout(() => resolve(null), 30_000)
+          })
+        )
+        .callThrough() // subsequent requests return normally
+      done()
+    })
+
+    after(function () {
+      MockWebApi.getDocument.restore()
+    })
+
+    it('should retry the request and return the document', async function () {
+      const returnedDoc = await DocUpdaterClient.getDoc(
+        this.project_id,
+        this.doc_id
+      )
+      expect(returnedDoc).to.deep.include({
+        lines: this.lines,
+        version: this.version,
+      })
+    })
+  })
+
+  describe('when the web api http request times out repeatedly', function () {
     before(function (done) {
       this.timeout = 10000
       sinon.stub(MockWebApi, 'getDocument').returns(
         new Promise(resolve => {
-          setTimeout(() => resolve(null), 30000)
+          setTimeout(() => resolve(null), 30_000)
         })
       )
       done()
@@ -353,7 +389,7 @@ describe('Getting a document', function () {
       MockWebApi.getDocument.restore()
     })
 
-    it('should return quickly(ish)', async function () {
+    it('should return an error after two attempts', async function () {
       const projectId = DocUpdaterClient.randomId()
       const docId = DocUpdaterClient.randomId()
       const start = Date.now()
@@ -361,7 +397,8 @@ describe('Getting a document', function () {
         .to.be.rejectedWith(RequestFailedError)
         .and.eventually.have.nested.property('response.status', 500)
       const delta = Date.now() - start
-      expect(delta).to.be.below(20000)
+      expect(delta).to.be.above(10_000)
+      expect(delta).to.be.below(20_000)
     })
   })
 })

services/document-updater/test/acceptance/js/SettingADocumentTests.js

@@ -916,4 +916,89 @@ describe('Setting a document', function () {
       expect(MockWebApi.setDocumentController).to.be.calledTwice
     })
   })
+
+  describe('when the web api http request times out on the first request', function () {
+    beforeEach(async function () {
+      this.project_id = DocUpdaterClient.randomId()
+      this.doc_id = DocUpdaterClient.randomId()
+      MockWebApi.insertDoc(this.project_id, this.doc_id, {
+        lines: this.lines,
+        version: this.version,
+        projectHistoryId: this.project_id,
+      })
+      await DocUpdaterClient.preloadDoc(this.project_id, this.doc_id)
+
+      const origSetDocumentController =
+        MockWebApi.setDocumentController.bind(MockWebApi)
+      const setDocumentStub = sinon
+        .stub(MockWebApi, 'setDocumentController')
+        .onFirstCall()
+        .callsFake(async (req, res, next) => {
+          await setTimeout(30_000)
+        })
+
+      setDocumentStub.onCall(1).callsFake(origSetDocumentController)
+    })
+
+    afterEach(function () {
+      MockWebApi.setDocumentController.restore()
+    })
+
+    it('should retry the request and return the document', async function () {
+      this.timeout(10000)
+      const returnedDoc = await DocUpdaterClient.setDocLines(
+        this.project_id,
+        this.doc_id,
+        this.newLines,
+        this.source,
+        this.user_id,
+        false
+      )
+      expect(returnedDoc).to.deep.include({ rev: '123' })
+      expect(MockWebApi.setDocumentController).to.be.calledTwice
+    })
+  })
+
+  describe('when the web api http request times out repeatedly', function () {
+    beforeEach(async function () {
+      this.project_id = DocUpdaterClient.randomId()
+      this.doc_id = DocUpdaterClient.randomId()
+      MockWebApi.insertDoc(this.project_id, this.doc_id, {
+        lines: this.lines,
+        version: this.version,
+        projectHistoryId: this.project_id,
+      })
+      await DocUpdaterClient.preloadDoc(this.project_id, this.doc_id)
+
+      sinon
+        .stub(MockWebApi, 'setDocumentController')
+        .callsFake(async (req, res, next) => {
+          await setTimeout(30_000)
+        })
+    })
+
+    afterEach(function () {
+      MockWebApi.setDocumentController.restore()
+    })
+
+    it('should return an error after two attempts', async function () {
+      this.timeout(15000)
+      const start = Date.now()
+      await expect(
+        DocUpdaterClient.setDocLines(
+          this.project_id,
+          this.doc_id,
+          this.newLines,
+          this.source,
+          this.user_id,
+          false
+        )
+      ).to.be.rejectedWith('request failed')
+
+      const delta = Date.now() - start
+      expect(delta).to.be.above(10_000) // 2 * 5000ms timeout
+      expect(delta).to.be.below(20_000)
+      expect(MockWebApi.setDocumentController).to.be.calledTwice
+    })
+  })
 })