Merge pull request #16590 from overleaf/jpa-redis-lock-60s

[server-ce] increase the doc lock TTL from 30s to 60s

GitOrigin-RevId: 468f7483cc6a80e8034e3cc8071b674123985deb

libraries/redis-wrapper/RedisLocker.js

@@ -15,10 +15,11 @@ const UNLOCK_SCRIPT =
 
 module.exports = class RedisLocker {
   /**
-   * @param rclient initialized ioredis client
-   * @param getKey compose the redis key based on the passed id
-   * @param wrapTimeoutError assign the id to a designated field on the error
-   * @param metricsPrefix prefix all the metrics with the given prefix
+   * @param {import('ioredis')} rclient initialized ioredis client
+   * @param {function(string): string} getKey compose the redis key based on the passed id
+   * @param {function(Error, string): Error} wrapTimeoutError assign the id to a designated field on the error
+   * @param {string} metricsPrefix prefix all the metrics with the given prefix
+   * @param {number} lockTTLSeconds
    *
    * @example ```
    * const lock = new RedisLocker({
@@ -35,7 +36,22 @@ module.exports = class RedisLocker {
    * }
    * ```
    */
-  constructor({ rclient, getKey, wrapTimeoutError, metricsPrefix }) {
+  constructor({
+    rclient,
+    getKey,
+    wrapTimeoutError,
+    metricsPrefix,
+    lockTTLSeconds = 30,
+  }) {
+    if (
+      typeof lockTTLSeconds !== 'number' ||
+      lockTTLSeconds < 30 ||
+      lockTTLSeconds >= 1000
+    ) {
+      // set upper limit to 1000s to detect wrong units
+      throw new Error('redis lock TTL must be at least 30s and below 1000s')
+    }
+
     this.rclient = rclient
     this.getKey = getKey
     this.wrapTimeoutError = wrapTimeoutError
@@ -44,7 +60,7 @@ module.exports = class RedisLocker {
     this.LOCK_TEST_INTERVAL = 50 // 50ms between each test of the lock
     this.MAX_TEST_INTERVAL = 1000 // back off to 1s between each test of the lock
     this.MAX_LOCK_WAIT_TIME = 10000 // 10s maximum time to spend trying to get the lock
-    this.LOCK_TTL = 30 // seconds. Time until lock auto expires in redis.
+    this.LOCK_TTL = lockTTLSeconds // seconds. Time until lock auto expires in redis.
 
     // read-only copy for unit tests
     this.unlockScript = UNLOCK_SCRIPT

libraries/redis-wrapper/test/unit/src/test.js

@@ -10,6 +10,7 @@ const assert = require('assert')
 const path = require('path')
 const sinon = require('sinon')
 const modulePath = path.join(__dirname, './../../../index.js')
+const redisLockerModulePath = path.join(__dirname, './../../../RedisLocker.js')
 const { expect } = require('chai')
 
 describe('index', function () {
@@ -53,9 +54,58 @@ describe('index', function () {
       },
       globals: {
         process,
+        Buffer,
       },
     })
-    return (this.auth_pass = '1234 pass')
+    this.auth_pass = '1234 pass'
+
+    this.RedisLocker = SandboxedModule.require(redisLockerModulePath, {
+      requires: {
+        '@overleaf/metrics': {
+          inc() {},
+        },
+      },
+      globals: {
+        process,
+        Math,
+        Buffer,
+      },
+    })
+  })
+
+  describe('lock TTL', function () {
+    it('should throw an error when creating a client with wrong type', function () {
+      const createNewRedisLock = () => {
+        return new this.RedisLocker({
+          lockTTLSeconds: '60',
+        })
+      }
+      expect(createNewRedisLock).to.throw(
+        'redis lock TTL must be at least 30s and below 1000s'
+      )
+    })
+
+    it('should throw an error when creating a client with small TTL', function () {
+      const createNewRedisLock = () => {
+        return new this.RedisLocker({
+          lockTTLSeconds: 1,
+        })
+      }
+      expect(createNewRedisLock).to.throw(
+        'redis lock TTL must be at least 30s and below 1000s'
+      )
+    })
+
+    it('should throw an error when creating a client with huge TTL', function () {
+      const createNewRedisLock = () => {
+        return new this.RedisLocker({
+          lockTTLSeconds: 30_000,
+        })
+      }
+      expect(createNewRedisLock).to.throw(
+        'redis lock TTL must be at least 30s and below 1000s'
+      )
+    })
   })
 
   describe('redis-sentinel', function () {

server-ce/config/settings.js

@@ -235,6 +235,11 @@ const settings = {
     10
   ),
 
+  redisLockTTLSeconds: parseInt(
+    process.env.SHARELATEX_REDIS_LOCK_TTL_SECONDS || '60',
+    10
+  ),
+
   i18n: {
     subdomainLang: {
       www: {

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

@@ -14,4 +14,5 @@ module.exports = new RedisLocker({
     return err
   },
   metricsPrefix: 'doc',
+  lockTTLSeconds: Settings.redisLockTTLSeconds,
 })

services/document-updater/config/settings.defaults.js

@@ -150,6 +150,8 @@ module.exports = {
 
   dispatcherCount: parseInt(process.env.DISPATCHER_COUNT || 10, 10),
 
+  redisLockTTLSeconds: 30,
+
   mongo: {
     url:
       process.env.MONGO_CONNECTION_STRING ||