Merge pull request #18583 from overleaf/em-multi-hash-key

Ensure that MULTI commands always go to a single node

GitOrigin-RevId: 223d29986766577df89c82983126dabca5d16eed

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

@@ -23,6 +23,12 @@ const ProjectHistoryRedisManager = {
     for (const op of ops) {
       metrics.summary('redis.projectHistoryOps', op.length, { status: 'push' })
     }
+
+    // Make sure that this MULTI operation only operates on project
+    // specific keys, i.e. keys that have the project id in curly braces.
+    // The curly braces identify a hash key for Redis and ensures that
+    // the MULTI's operations are all done on the same node in a
+    // cluster environment.
     const multi = rclient.multi()
     // Push the ops onto the project history queue
     multi.rpush(

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

@@ -32,6 +32,11 @@ const MAX_OPS_PER_ITERATION = 8 // process a limited number of ops for safety
 
 const RealTimeRedisManager = {
   getPendingUpdatesForDoc(docId, callback) {
+    // Make sure that this MULTI operation only operates on doc
+    // specific keys, i.e. keys that have the doc id in curly braces.
+    // The curly braces identify a hash key for Redis and ensures that
+    // the MULTI's operations are all done on the same node in a
+    // cluster environment.
     const multi = rclient.multi()
     multi.lrange(
       Keys.pendingUpdates({ doc_id: docId }),

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

@@ -77,57 +77,66 @@ const RedisManager = {
         logger.error({ err: error, docId, projectId }, error.message)
         return callback(error)
       }
-      // update docsInProject set before writing doc contents
+      setHistoryRangesSupportFlag(docId, historyRangesSupport, err => {
-      rclient.sadd(
+        if (err) {
-        keys.docsInProject({ project_id: projectId }),
+          return callback(err)
-        docId,
+        }
-        error => {
+        // update docsInProject set before writing doc contents
-          if (error) return callback(error)
+        rclient.sadd(
+          keys.docsInProject({ project_id: projectId }),
+          docId,
+          error => {
+            if (error) return callback(error)
 
-          if (!pathname) {
+            if (!pathname) {
-            metrics.inc('pathname', 1, {
+              metrics.inc('pathname', 1, {
-              path: 'RedisManager.setDoc',
+                path: 'RedisManager.setDoc',
-              status: pathname === '' ? 'zero-length' : 'undefined',
+                status: pathname === '' ? 'zero-length' : 'undefined',
+              })
+            }
+
+            // Make sure that this MULTI operation only operates on doc
+            // specific keys, i.e. keys that have the doc id in curly braces.
+            // The curly braces identify a hash key for Redis and ensures that
+            // the MULTI's operations are all done on the same node in a
+            // cluster environment.
+            const multi = rclient.multi()
+            multi.mset({
+              [keys.docLines({ doc_id: docId })]: docLines,
+              [keys.projectKey({ doc_id: docId })]: projectId,
+              [keys.docVersion({ doc_id: docId })]: version,
+              [keys.docHash({ doc_id: docId })]: docHash,
+              [keys.ranges({ doc_id: docId })]: ranges,
+              [keys.pathname({ doc_id: docId })]: pathname,
+              [keys.projectHistoryId({ doc_id: docId })]: projectHistoryId,
+            })
+            if (historyRangesSupport) {
+              multi.del(keys.resolvedCommentIds({ doc_id: docId }))
+              if (resolvedCommentIds.length > 0) {
+                multi.sadd(
+                  keys.resolvedCommentIds({ doc_id: docId }),
+                  ...resolvedCommentIds
+                )
+              }
+            }
+            multi.exec(err => {
+              if (err) {
+                callback(
+                  OError.tag(err, 'failed to write doc to Redis in MULTI', {
+                    previousErrors: err.previousErrors.map(e => ({
+                      name: e.name,
+                      message: e.message,
+                      command: e.command,
+                    })),
+                  })
+                )
+              } else {
+                callback()
+              }
             })
           }
-
+        )
-          const multi = rclient.multi()
+      })
-          multi.mset({
-            [keys.docLines({ doc_id: docId })]: docLines,
-            [keys.projectKey({ doc_id: docId })]: projectId,
-            [keys.docVersion({ doc_id: docId })]: version,
-            [keys.docHash({ doc_id: docId })]: docHash,
-            [keys.ranges({ doc_id: docId })]: ranges,
-            [keys.pathname({ doc_id: docId })]: pathname,
-            [keys.projectHistoryId({ doc_id: docId })]: projectHistoryId,
-          })
-          if (historyRangesSupport) {
-            multi.sadd(keys.historyRangesSupport(), docId)
-            multi.del(keys.resolvedCommentIds({ doc_id: docId }))
-            if (resolvedCommentIds.length > 0) {
-              multi.sadd(
-                keys.resolvedCommentIds({ doc_id: docId }),
-                ...resolvedCommentIds
-              )
-            }
-          }
-          multi.exec(err => {
-            if (err) {
-              callback(
-                OError.tag(err, 'failed to write doc to Redis in MULTI', {
-                  previousErrors: err.previousErrors.map(e => ({
-                    name: e.name,
-                    message: e.message,
-                    command: e.command,
-                  })),
-                })
-              )
-            } else {
-              callback()
-            }
-          })
-        }
-      )
     })
   },
 
@@ -143,6 +152,11 @@ const RedisManager = {
       }
     }
 
+    // Make sure that this MULTI operation only operates on doc
+    // specific keys, i.e. keys that have the doc id in curly braces.
+    // The curly braces identify a hash key for Redis and ensures that
+    // the MULTI's operations are all done on the same node in a
+    // cluster environment.
     let multi = rclient.multi()
     multi.strlen(keys.docLines({ doc_id: docId }))
     multi.del(
@@ -167,6 +181,12 @@ const RedisManager = {
         // record bytes freed in redis
         metrics.summary('redis.docLines', length, { status: 'del' })
       }
+
+      // Make sure that this MULTI operation only operates on project
+      // specific keys, i.e. keys that have the project id in curly braces.
+      // The curly braces identify a hash key for Redis and ensures that
+      // the MULTI's operations are all done on the same node in a
+      // cluster environment.
       multi = rclient.multi()
       multi.srem(keys.docsInProject({ project_id: projectId }), docId)
       multi.del(keys.projectState({ project_id: projectId }))
@@ -180,6 +200,11 @@ const RedisManager = {
   },
 
   checkOrSetProjectState(projectId, newState, callback) {
+    // Make sure that this MULTI operation only operates on project
+    // specific keys, i.e. keys that have the project id in curly braces.
+    // The curly braces identify a hash key for Redis and ensures that
+    // the MULTI's operations are all done on the same node in a
+    // cluster environment.
     const multi = rclient.multi()
     multi.getset(keys.projectState({ project_id: projectId }), newState)
     multi.expire(keys.projectState({ project_id: projectId }), 30 * minutes)
@@ -488,6 +513,12 @@ const RedisManager = {
           logger.error({ err: error, docId, ranges }, error.message)
           return callback(error)
         }
+
+        // Make sure that this MULTI operation only operates on doc
+        // specific keys, i.e. keys that have the doc id in curly braces.
+        // The curly braces identify a hash key for Redis and ensures that
+        // the MULTI's operations are all done on the same node in a
+        // cluster environment.
         const multi = rclient.multi()
         multi.mset({
           [keys.docLines({ doc_id: docId })]: newDocLines,
@@ -670,6 +701,14 @@ const RedisManager = {
   },
 }
 
+function setHistoryRangesSupportFlag(docId, historyRangesSupport, callback) {
+  if (historyRangesSupport) {
+    rclient.sadd(keys.historyRangesSupport(), docId, callback)
+  } else {
+    rclient.srem(keys.historyRangesSupport(), docId, callback)
+  }
+}
+
 module.exports = RedisManager
 module.exports.promises = promisifyAll(RedisManager, {
   without: ['_deserializeRanges', '_computeHash'],

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

@@ -441,88 +441,85 @@ describe('Ranges', function () {
         lines: ['aaa', 'bbb', 'ccc', 'ddd', 'eee'],
       }
 
-      DocUpdaterClient.enableHistoryRangesSupport(this.doc.id, (error, res) => {
+      MockWebApi.insertDoc(this.project_id, this.doc.id, {
-        if (error) {
+        lines: this.doc.lines,
+        version: 0,
+        historyRangesSupport: true,
+      })
+
+      DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => {
+        if (error != null) {
           throw error
         }
-        MockWebApi.insertDoc(this.project_id, this.doc.id, {
-          lines: this.doc.lines,
-          version: 0,
-        })
 
-        DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => {
+        this.id_seed_1 = 'tc_1'
-          if (error != null) {
+        this.id_seed_2 = 'tc_2'
-            throw error
+        this.id_seed_3 = 'tc_3'
-          }
 
-          this.id_seed_1 = 'tc_1'
+        this.updates = [
-          this.id_seed_2 = 'tc_2'
+          {
-          this.id_seed_3 = 'tc_3'
+            doc: this.doc.id,
-
+            op: [{ d: 'bbb', p: 4 }],
-          this.updates = [
+            v: 0,
-            {
+            meta: {
-              doc: this.doc.id,
+              user_id: this.user_id,
-              op: [{ d: 'bbb', p: 4 }],
+              tc: this.id_seed_1,
-              v: 0,
-              meta: {
-                user_id: this.user_id,
-                tc: this.id_seed_1,
-              },
             },
-            {
+          },
-              doc: this.doc.id,
+          {
-              op: [{ d: 'ccc', p: 5 }],
+            doc: this.doc.id,
-              v: 1,
+            op: [{ d: 'ccc', p: 5 }],
-              meta: {
+            v: 1,
-                user_id: this.user_id,
+            meta: {
-                tc: this.id_seed_2,
+              user_id: this.user_id,
-              },
+              tc: this.id_seed_2,
             },
-            {
+          },
-              doc: this.doc.id,
+          {
-              op: [{ d: 'ddd', p: 6 }],
+            doc: this.doc.id,
-              v: 2,
+            op: [{ d: 'ddd', p: 6 }],
-              meta: {
+            v: 2,
-                user_id: this.user_id,
+            meta: {
-                tc: this.id_seed_3,
+              user_id: this.user_id,
-              },
+              tc: this.id_seed_3,
             },
-          ]
+          },
+        ]
 
-          DocUpdaterClient.sendUpdates(
+        DocUpdaterClient.sendUpdates(
-            this.project_id,
+          this.project_id,
-            this.doc.id,
+          this.doc.id,
-            this.updates,
+          this.updates,
-            error => {
+          error => {
-              if (error != null) {
+            if (error != null) {
-                throw error
+              throw error
-              }
-              setTimeout(() => {
-                DocUpdaterClient.getDoc(
-                  this.project_id,
-                  this.doc.id,
-                  (error, res, data) => {
-                    if (error != null) {
-                      throw error
-                    }
-                    const { ranges } = data
-                    const changeOps = ranges.changes
-                      .map(change => change.op)
-                      .flat()
-                    changeOps.should.deep.equal([
-                      { d: 'bbb', p: 4 },
-                      { d: 'ccc', p: 5 },
-                      { d: 'ddd', p: 6 },
-                    ])
-                    done()
-                  }
-                )
-              }, 200)
             }
-          )
+            setTimeout(() => {
-        })
+              DocUpdaterClient.getDoc(
+                this.project_id,
+                this.doc.id,
+                (error, res, data) => {
+                  if (error != null) {
+                    throw error
+                  }
+                  const { ranges } = data
+                  const changeOps = ranges.changes
+                    .map(change => change.op)
+                    .flat()
+                  changeOps.should.deep.equal([
+                    { d: 'bbb', p: 4 },
+                    { d: 'ccc', p: 5 },
+                    { d: 'ddd', p: 6 },
+                  ])
+                  done()
+                }
+              )
+            }, 200)
+          }
+        )
       })
     })
+
     afterEach(function () {
       sandbox.restore()
     })

services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js

@@ -119,10 +119,6 @@ module.exports = DocUpdaterClient = {
     )
   },
 
-  enableHistoryRangesSupport(docId, cb) {
-    rclient.sadd(keys.historyRangesSupport(), docId, cb)
-  },
-
   preloadDoc(projectId, docId, callback) {
     DocUpdaterClient.getDoc(projectId, docId, callback)
   },

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

@@ -834,8 +834,11 @@ describe('RedisManager', function () {
         this.logger.error.calledWith().should.equal(false)
       })
 
-      it('should not add the document to the HistoryRangesSupport set in Redis', function () {
+      it('should remove the document from the HistoryRangesSupport set in Redis', function () {
-        this.multi.sadd.should.not.have.been.calledWith('HistoryRangesSupport')
+        this.rclient.srem.should.have.been.calledWith(
+          'HistoryRangesSupport',
+          this.docId
+        )
       })
 
       it('should not store the resolved comments in Redis', function () {
@@ -956,7 +959,7 @@ describe('RedisManager', function () {
       })
 
       it('should add the document to the HistoryRangesSupport set in Redis', function () {
-        this.multi.sadd.should.have.been.calledWith(
+        this.rclient.sadd.should.have.been.calledWith(
           'HistoryRangesSupport',
           this.docId
         )