Merge pull request #14377 from overleaf/tw-docupdater-flush

add check for unflushedTime

GitOrigin-RevId: e09d63b4de09e30ceb82792526224ad8b2415119

package-lock.json

@@ -44410,6 +44410,7 @@
         "mocha": "^10.2.0",
         "sandboxed-module": "^2.0.4",
         "sinon": "^9.2.4",
+        "sinon-chai": "^3.7.0",
         "timekeeper": "^2.0.0"
       }
     },
@@ -53409,6 +53410,7 @@
         "requestretry": "^7.1.0",
         "sandboxed-module": "^2.0.4",
         "sinon": "^9.2.4",
+        "sinon-chai": "^3.7.0",
         "timekeeper": "^2.0.0"
       },
       "dependencies": {

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

@@ -288,17 +288,23 @@ module.exports = DocumentManager = {
           return callback(error)
         }
         if (lines == null || version == null) {
+          Metrics.inc('flush-doc-if-loaded', 1, { status: 'not-loaded' })
           logger.debug(
             { projectId, docId },
             'doc is not loaded so not flushing'
           )
           // TODO: return a flag to bail out, as we go on to remove doc from memory?
           callback(null)
+        } else if (unflushedTime == null) {
+          Metrics.inc('flush-doc-if-loaded', 1, { status: 'unmodified' })
+          logger.debug(
+            { projectId, docId },
+            'doc is not modified so not flushing'
+          )
+          callback(null)
         } else {
           logger.debug({ projectId, docId, version }, 'flushing doc')
-          Metrics.inc('flush-doc-if-loaded', {
-            status: unflushedTime != null ? 'modified' : 'unmodified',
-          })
+          Metrics.inc('flush-doc-if-loaded', 1, { status: 'modified' })
           PersistenceManager.setDoc(
             projectId,
             docId,

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

@@ -454,11 +454,9 @@ module.exports = RedisManager = {
           multi.rpush(keys.docOps({ doc_id: docId }), ...jsonOps) // index 5
           // expire must come after rpush since before it will be a no-op if the list is empty
           multi.expire(keys.docOps({ doc_id: docId }), RedisManager.DOC_OPS_TTL) // index 6
-          // Set the unflushed timestamp to the current time if the doc
-          // hasn't been modified before (the content in mongo has been
-          // valid up to this point). Otherwise leave it alone ("NX" flag).
-          multi.set(keys.unflushedTime({ doc_id: docId }), Date.now(), 'NX')
         }
+        // Set the unflushed timestamp to the current time if not set ("NX" flag).
+        multi.set(keys.unflushedTime({ doc_id: docId }), Date.now(), 'NX')
         multi.exec((error, result) => {
           if (error) {
             return callback(error)

services/document-updater/package.json

@@ -39,6 +39,7 @@
     "mocha": "^10.2.0",
     "sandboxed-module": "^2.0.4",
     "sinon": "^9.2.4",
+    "sinon-chai": "^3.7.0",
     "timekeeper": "^2.0.0"
   }
 }

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

@@ -15,7 +15,7 @@ const DocUpdaterClient = require('./helpers/DocUpdaterClient')
 const DocUpdaterApp = require('./helpers/DocUpdaterApp')
 
 describe('Deleting a project', function () {
-  before(function (done) {
+  beforeEach(function (done) {
     let docId0, docId1
     this.project_id = DocUpdaterClient.randomId()
     this.docs = [
@@ -60,8 +60,87 @@ describe('Deleting a project', function () {
     DocUpdaterApp.ensureRunning(done)
   })
 
+  describe('without updates', function () {
+    beforeEach(function (done) {
+      sinon.spy(MockWebApi, 'setDocument')
+      sinon.spy(MockProjectHistoryApi, 'flushProject')
+
+      async.series(
+        this.docs.map(doc => {
+          return callback => {
+            DocUpdaterClient.preloadDoc(this.project_id, doc.id, error => {
+              callback(error)
+            })
+          }
+        }),
+        error => {
+          if (error != null) {
+            throw error
+          }
+          setTimeout(() => {
+            DocUpdaterClient.deleteProject(
+              this.project_id,
+              (error, res, body) => {
+                if (error) return done(error)
+                this.statusCode = res.statusCode
+                done()
+              }
+            )
+          }, 200)
+        }
+      )
+    })
+
+    afterEach(function () {
+      MockWebApi.setDocument.restore()
+      MockProjectHistoryApi.flushProject.restore()
+    })
+
+    it('should return a 204 status code', function () {
+      this.statusCode.should.equal(204)
+    })
+
+    it('should not send any document to the web api', function () {
+      MockWebApi.setDocument.should.not.have.been.called
+    })
+
+    it('should need to reload the docs if read again', function (done) {
+      sinon.spy(MockWebApi, 'getDocument')
+      async.series(
+        this.docs.map(doc => {
+          return callback => {
+            MockWebApi.getDocument
+              .calledWith(this.project_id, doc.id)
+              .should.equal(false)
+            DocUpdaterClient.getDoc(
+              this.project_id,
+              doc.id,
+              (error, res, returnedDoc) => {
+                if (error) return done(error)
+                MockWebApi.getDocument
+                  .calledWith(this.project_id, doc.id)
+                  .should.equal(true)
+                callback()
+              }
+            )
+          }
+        }),
+        () => {
+          MockWebApi.getDocument.restore()
+          done()
+        }
+      )
+    })
+
+    it('should flush each doc in project history', function () {
+      MockProjectHistoryApi.flushProject
+        .calledWith(this.project_id)
+        .should.equal(true)
+    })
+  })
+
   describe('with documents which have been updated', function () {
-    before(function (done) {
+    beforeEach(function (done) {
       sinon.spy(MockWebApi, 'setDocument')
       sinon.spy(MockProjectHistoryApi, 'flushProject')
 
@@ -101,7 +180,7 @@ describe('Deleting a project', function () {
       )
     })
 
-    after(function () {
+    afterEach(function () {
       MockWebApi.setDocument.restore()
       MockProjectHistoryApi.flushProject.restore()
     })
@@ -154,14 +233,26 @@ describe('Deleting a project', function () {
   })
 
   describe('with the background=true parameter from realtime and no request to flush the queue', function () {
-    before(function (done) {
+    beforeEach(function (done) {
       sinon.spy(MockWebApi, 'setDocument')
       sinon.spy(MockProjectHistoryApi, 'flushProject')
 
       async.series(
         this.docs.map(doc => {
           return callback => {
-            DocUpdaterClient.preloadDoc(this.project_id, doc.id, callback)
+            DocUpdaterClient.preloadDoc(this.project_id, doc.id, error => {
+              if (error != null) {
+                return callback(error)
+              }
+              DocUpdaterClient.sendUpdate(
+                this.project_id,
+                doc.id,
+                doc.update,
+                error => {
+                  callback(error)
+                }
+              )
+            })
           }
         }),
         error => {
@@ -182,7 +273,7 @@ describe('Deleting a project', function () {
       )
     })
 
-    after(function () {
+    afterEach(function () {
       MockWebApi.setDocument.restore()
       MockProjectHistoryApi.flushProject.restore()
     })
@@ -201,14 +292,26 @@ describe('Deleting a project', function () {
   })
 
   describe('with the background=true parameter from realtime and a request to flush the queue', function () {
-    before(function (done) {
+    beforeEach(function (done) {
       sinon.spy(MockWebApi, 'setDocument')
       sinon.spy(MockProjectHistoryApi, 'flushProject')
 
       async.series(
         this.docs.map(doc => {
           return callback => {
-            DocUpdaterClient.preloadDoc(this.project_id, doc.id, callback)
+            DocUpdaterClient.preloadDoc(this.project_id, doc.id, error => {
+              if (error != null) {
+                return callback(error)
+              }
+              DocUpdaterClient.sendUpdate(
+                this.project_id,
+                doc.id,
+                doc.update,
+                error => {
+                  callback(error)
+                }
+              )
+            })
           }
         }),
         error => {
@@ -230,7 +333,7 @@ describe('Deleting a project', function () {
       )
     })
 
-    after(function () {
+    afterEach(function () {
       MockWebApi.setDocument.restore()
       MockProjectHistoryApi.flushProject.restore()
     })

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

@@ -346,7 +346,8 @@ describe('Ranges', function () {
   })
 
   describe('accepting a change', function () {
-    before(function (done) {
+    beforeEach(function (done) {
+      sinon.spy(MockWebApi, 'setDocument')
       this.project_id = DocUpdaterClient.randomId()
       this.user_id = DocUpdaterClient.randomId()
       this.id_seed = '587357bd35e64f6157'
@@ -401,8 +402,11 @@ describe('Ranges', function () {
         }
       )
     })
+    afterEach(function () {
+      MockWebApi.setDocument.restore()
+    })
 
-    return it('should remove the change after accepting', function (done) {
+    it('should remove the change after accepting', function (done) {
       return DocUpdaterClient.acceptChange(
         this.project_id,
         this.doc.id,
@@ -425,6 +429,41 @@ describe('Ranges', function () {
         }
       )
     })
+
+    it('should persist the ranges after accepting', function (done) {
+      DocUpdaterClient.flushDoc(this.project_id, this.doc.id, err => {
+        if (err) return done(err)
+        DocUpdaterClient.acceptChange(
+          this.project_id,
+          this.doc.id,
+          this.id_seed + '000001',
+          error => {
+            if (error != null) {
+              throw error
+            }
+
+            DocUpdaterClient.flushDoc(this.project_id, this.doc.id, err => {
+              if (err) return done(err)
+              DocUpdaterClient.getDoc(
+                this.project_id,
+                this.doc.id,
+                (error, res, data) => {
+                  if (error != null) {
+                    throw error
+                  }
+                  expect(data.ranges.changes).to.be.undefined
+
+                  MockWebApi.setDocument
+                    .calledWith(this.project_id, this.doc.id, ['a456aa'], 1, {})
+                    .should.equal(true)
+                  done()
+                }
+              )
+            })
+          }
+        )
+      })
+    })
   })
 
   describe('deleting a comment range', function () {

services/document-updater/test/setup.js

@@ -4,6 +4,9 @@ const sinon = require('sinon')
 
 // Chai configuration
 chai.should()
+// Load sinon-chai assertions so expect(stubFn).to.have.been.calledWith('abc')
+// has a nicer failure messages
+chai.use(require('sinon-chai'))
 
 // Global stubs
 const sandbox = sinon.createSandbox()

services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js

@@ -606,6 +606,12 @@ describe('RedisManager', function () {
         )
       })
 
+      it('should set the unflushed time (potential ranges changes)', function () {
+        this.multi.set
+          .calledWith(`UnflushedTime:${this.docId}`, Date.now(), 'NX')
+          .should.equal(true)
+      })
+
       it('should not try to enqueue doc updates', function () {
         this.multi.rpush.called.should.equal(false)
       })