From ee3d3b09ed72f1f78d4aaea29b3448d080b30a3f Mon Sep 17 00:00:00 2001
From: Jakob Ackermann <jakob.ackermann@overleaf.com>
Date: Fri, 21 Aug 2020 12:15:35 +0100
Subject: [PATCH] [misc] wrap redis errors as tagging does not work with them

ioredis may reuse the error instance for multiple callbacks. E.g. when
 the connection to redis fails, the queue is flushed with the same
 MaxRetriesPerRequestError instance.
---
 services/real-time/app/js/ChannelManager.js    | 18 ++++++++++--------
 .../real-time/app/js/ConnectedUsersManager.js  | 10 +++++-----
 .../real-time/app/js/DocumentUpdaterManager.js |  4 ++--
 .../test/unit/js/ChannelManagerTests.js        |  3 ++-
 4 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/services/real-time/app/js/ChannelManager.js b/services/real-time/app/js/ChannelManager.js
index da0490ff9a..8e2bb57863 100644
--- a/services/real-time/app/js/ChannelManager.js
+++ b/services/real-time/app/js/ChannelManager.js
@@ -23,12 +23,13 @@ module.exports = {
     const channel = `${baseChannel}:${id}`
     const actualSubscribe = function () {
       // subscribe is happening in the foreground and it should reject
-      const p = rclient.subscribe(channel)
-      p.finally(function () {
-        if (clientChannelMap.get(channel) === subscribePromise) {
-          clientChannelMap.delete(channel)
-        }
-      })
+      return rclient
+        .subscribe(channel)
+        .finally(function () {
+          if (clientChannelMap.get(channel) === subscribePromise) {
+            clientChannelMap.delete(channel)
+          }
+        })
         .then(function () {
           logger.log({ channel }, 'subscribed to channel')
           metrics.inc(`subscribe.${baseChannel}`)
@@ -37,9 +38,10 @@ module.exports = {
           logger.error({ channel, err }, 'failed to subscribe to channel')
           metrics.inc(`subscribe.failed.${baseChannel}`)
           // add context for the stack-trace at the call-site
-          OError.tag(err, 'failed to subscribe to channel', { channel })
+          throw new OError('failed to subscribe to channel', {
+            channel
+          }).withCause(err)
         })
-      return p
     }
 
     const pendingActions = clientChannelMap.get(channel) || Promise.resolve()
diff --git a/services/real-time/app/js/ConnectedUsersManager.js b/services/real-time/app/js/ConnectedUsersManager.js
index 340d0706fd..ed37db1ef6 100644
--- a/services/real-time/app/js/ConnectedUsersManager.js
+++ b/services/real-time/app/js/ConnectedUsersManager.js
@@ -68,7 +68,7 @@ module.exports = {
 
     multi.exec(function (err) {
       if (err) {
-        OError.tag(err, 'problem marking user as connected')
+        err = new OError('problem marking user as connected').withCause(err)
       }
       callback(err)
     })
@@ -104,7 +104,7 @@ module.exports = {
     multi.del(Keys.connectedUser({ project_id, client_id }))
     multi.exec(function (err) {
       if (err) {
-        OError.tag(err, 'problem marking user as disconnected')
+        err = new OError('problem marking user as disconnected').withCause(err)
       }
       callback(err)
     })
@@ -116,9 +116,9 @@ module.exports = {
       result
     ) {
       if (err) {
-        OError.tag(err, 'problem fetching connected user details', {
+        err = new OError('problem fetching connected user details', {
           other_client_id: client_id
-        })
+        }).withCause(err)
         return callback(err)
       }
       if (!(result && result.user_id)) {
@@ -154,7 +154,7 @@ module.exports = {
       results
     ) {
       if (err) {
-        OError.tag(err, 'problem getting clients in project')
+        err = new OError('problem getting clients in project').withCause(err)
         return callback(err)
       }
       const jobs = results.map((client_id) => (cb) =>
diff --git a/services/real-time/app/js/DocumentUpdaterManager.js b/services/real-time/app/js/DocumentUpdaterManager.js
index 587b6f959d..2d7f2890c6 100644
--- a/services/real-time/app/js/DocumentUpdaterManager.js
+++ b/services/real-time/app/js/DocumentUpdaterManager.js
@@ -136,12 +136,12 @@ const DocumentUpdaterManager = {
       error
     ) {
       if (error) {
-        OError.tag(error, 'error pushing update into redis')
+        error = new OError('error pushing update into redis').withCause(error)
         return callback(error)
       }
       rclient.rpush('pending-updates-list', doc_key, function (error) {
         if (error) {
-          OError.tag(error, 'error pushing doc_id into redis')
+          error = new OError('error pushing doc_id into redis').withCause(error)
         }
         callback(error)
       })
diff --git a/services/real-time/test/unit/js/ChannelManagerTests.js b/services/real-time/test/unit/js/ChannelManagerTests.js
index 6026f6ab5c..95e9000a49 100644
--- a/services/real-time/test/unit/js/ChannelManagerTests.js
+++ b/services/real-time/test/unit/js/ChannelManagerTests.js
@@ -91,7 +91,8 @@ describe('ChannelManager', function () {
         )
         p.then(() => done(new Error('should not subscribe but fail'))).catch(
           (err) => {
-            err.message.should.equal('some redis error')
+            err.message.should.equal('failed to subscribe to channel')
+            err.cause.message.should.equal('some redis error')
             this.ChannelManager.getClientMapEntry(this.rclient)
               .has('applied-ops:1234567890abcdef')
               .should.equal(false)