From 8c0a78c7e7baaf7c718918e4ae313b96b2187c48 Mon Sep 17 00:00:00 2001
From: Jakob Ackermann <jakob.ackermann@overleaf.com>
Date: Tue, 23 Jul 2024 11:36:52 +0200
Subject: [PATCH] Merge pull request #19480 from
 overleaf/jpa-fast-path-fetch-for-transform

[document-updater] avoid fetching updates to transform when up-to-date

GitOrigin-RevId: 7962d8903a7bc9b572d7c6adfd8f33ad36f30459
---
 services/document-updater/app/js/ShareJsDB.js | 55 ++++++++++---------
 .../test/unit/js/ShareJsDB/ShareJsDBTests.js  | 24 +++++++-
 2 files changed, 53 insertions(+), 26 deletions(-)

diff --git a/services/document-updater/app/js/ShareJsDB.js b/services/document-updater/app/js/ShareJsDB.js
index 580a3d93b5..5e9c517374 100644
--- a/services/document-updater/app/js/ShareJsDB.js
+++ b/services/document-updater/app/js/ShareJsDB.js
@@ -17,6 +17,12 @@ const Keys = require('./UpdateKeys')
 const RedisManager = require('./RedisManager')
 const Errors = require('./Errors')
 
+const TRANSFORM_UPDATES_COUNT_BUCKETS = [
+  0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 15, 20, 25, 50, 75, 100,
+  // prepare buckets for full-project history/larger buffer experiments
+  150, 200, 300, 400,
+]
+
 module.exports = ShareJsDB = class ShareJsDB {
   constructor(projectId, docId, lines, version) {
     this.project_id = projectId
@@ -31,11 +37,18 @@ module.exports = ShareJsDB = class ShareJsDB {
   }
 
   getOps(docKey, start, end, callback) {
-    if (start === end) {
+    if (start === end || (start === this.version && end === null)) {
+      const status = 'is-up-to-date'
       Metrics.inc('transform-updates', 1, {
-        status: 'is-up-to-date',
+        status,
         path: 'sharejs',
       })
+      Metrics.histogram(
+        'transform-updates.count',
+        0,
+        TRANSFORM_UPDATES_COUNT_BUCKETS,
+        { path: 'sharejs', status }
+      )
       return callback(null, [])
     }
 
@@ -64,35 +77,27 @@ module.exports = ShareJsDB = class ShareJsDB {
       } else {
         if (ops.length === 0) {
           status = 'fetched-zero'
+
+          // The sharejs processing is happening under a lock.
+          // In case there are no other ops available, something bypassed the lock (or we overran it).
+          logger.warn(
+            {
+              projectId,
+              docId,
+              start,
+              end,
+              timeSinceShareJsDBInit:
+                performance.now() - this.startTimeShareJsDB,
+            },
+            'found zero docOps while transforming update'
+          )
         } else {
           status = 'fetched'
-
-          if (start === this.version && end === -1) {
-            // The sharejs processing is happening under a lock.
-            // this.version is the version as read from redis under lock.
-            // In case there are any new ops available, something bypassed the lock (or we overran it).
-            logger.warn(
-              {
-                projectId,
-                docId,
-                start,
-                nOps: ops.length,
-                timeSinceShareJsDBInit:
-                  performance.now() - this.startTimeShareJsDB,
-              },
-              'concurrent update of docOps while transforming update'
-            )
-          }
         }
         Metrics.histogram(
           'transform-updates.count',
           ops.length,
-          [
-            0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 15, 20, 25, 50, 75, 100,
-            // prepare buckets for full-project history/larger buffer experiments
-            150,
-            200, 300, 400,
-          ],
+          TRANSFORM_UPDATES_COUNT_BUCKETS,
           { path: 'sharejs', status }
         )
       }
diff --git a/services/document-updater/test/unit/js/ShareJsDB/ShareJsDBTests.js b/services/document-updater/test/unit/js/ShareJsDB/ShareJsDBTests.js
index 63748e3baa..f122a8a1ed 100644
--- a/services/document-updater/test/unit/js/ShareJsDB/ShareJsDBTests.js
+++ b/services/document-updater/test/unit/js/ShareJsDB/ShareJsDBTests.js
@@ -24,7 +24,9 @@ describe('ShareJsDB', function () {
     this.callback = sinon.stub()
     this.ShareJsDB = SandboxedModule.require(modulePath, {
       requires: {
-        './RedisManager': (this.RedisManager = {}),
+        './RedisManager': (this.RedisManager = {
+          getPreviousDocOps: sinon.stub(),
+        }),
         './Errors': Errors,
         '@overleaf/metrics': {
           inc: sinon.stub(),
@@ -87,11 +89,31 @@ describe('ShareJsDB', function () {
         return this.db.getOps(this.doc_key, this.start, this.end, this.callback)
       })
 
+      it('should not talk to redis', function () {
+        this.RedisManager.getPreviousDocOps.should.not.have.been.called
+      })
+
       return it('should return an empty array', function () {
         return this.callback.calledWith(null, []).should.equal(true)
       })
     })
 
+    describe('with start == redis-version and end unset', function () {
+      beforeEach(function () {
+        const start = this.version
+        const end = null
+        this.db.getOps(this.doc_key, start, end, this.callback)
+      })
+
+      it('should not talk to redis', function () {
+        this.RedisManager.getPreviousDocOps.should.not.have.been.called
+      })
+
+      it('should return an empty array', function () {
+        this.callback.should.have.been.calledWith(null, [])
+      })
+    })
+
     describe('with a non empty range', function () {
       beforeEach(function () {
         this.start = 35