[document-updater] wrap doc locks in project lock (#35416)

* [document-updater] wrap doc locks in project lock

* [document-updater] use BlockingProject as redis key prefix

* [redis-wrapper] update comment

GitOrigin-RevId: 0ce50e76386871e6c29481494febc8d6b37e7df5

libraries/redis-wrapper/RedisLocker.js

@@ -13,6 +13,10 @@ const MAX_REDIS_REQUEST_LENGTH = 5000 // 5 seconds
 
 const UNLOCK_SCRIPT =
   'if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("del", KEYS[1]) else return 0 end'
+// Extend (bump) the TTL of a lock we still own, so a long-running critical
+// section does not let the lock expire from under us.
+const EXTEND_SCRIPT =
+  'if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("expire", KEYS[1], ARGV[2]) else return 0 end'
 
 module.exports = class RedisLocker {
   /**
@@ -69,6 +73,7 @@ module.exports = class RedisLocker {
     this.promises = {
       checkLock: promisify(this.checkLock.bind(this)),
       getLock: promisify(this.getLock.bind(this)),
+      extendLock: promisify(this.extendLock.bind(this)),
       releaseLock: promisify(this.releaseLock.bind(this)),
 
       // tryLock returns two values: gotLock and lockValue. We need to merge
@@ -198,6 +203,46 @@ module.exports = class RedisLocker {
     })
   }
 
+  /**
+   * Extend (bump) the TTL of a lock we still hold back to the full lock TTL.
+   * Useful for long critical sections (e.g. draining a whole project) that
+   * would otherwise outlive the lock and let a second worker in.
+   *
+   * @param {string} id
+   * @param {string} lockValue
+   * @param {function(Error): void} callback
+   */
+  extendLock(id, lockValue, callback) {
+    if (callback == null) {
+      callback = function () {}
+    }
+    const key = this.getKey(id)
+    this.rclient.eval(
+      EXTEND_SCRIPT,
+      1,
+      key,
+      lockValue,
+      this.LOCK_TTL,
+      (err, result) => {
+        if (err != null) {
+          return callback(err)
+        } else if (result != null && result !== 1) {
+          // A successful extension updates the expiration time on exactly one key.
+          // Anything else means we no longer hold the lock (it expired or was taken).
+          logger.error(
+            { id, key, lockValue, redis_err: err, redis_result: result },
+            'extending lock error'
+          )
+          metrics.inc(this.metricsPrefix + '-extend-error')
+          return callback(new Error('tried to extend a lock we no longer hold'))
+        } else {
+          metrics.inc(this.metricsPrefix + '-extend-success')
+          return callback(null)
+        }
+      }
+    )
+  }
+
   /**
    * @param {string} id
    * @param {string} lockValue

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

@@ -116,6 +116,38 @@ describe('index', function () {
     })
   })
 
+  describe('extendLock', function () {
+    beforeEach(function () {
+      this.rclient = { eval: sinon.stub() }
+      this.locker = new this.RedisLocker({
+        rclient: this.rclient,
+        getKey: id => `lock:${id}`,
+        wrapTimeoutError: (err, id) => err,
+        metricsPrefix: 'test',
+        lockTTLSeconds: 30,
+      })
+    })
+
+    it('should extend the lock TTL when we still own it', function (done) {
+      this.rclient.eval.yields(null, 1)
+      this.locker.extendLock('id-1', 'lock-value', err => {
+        expect(err).to.not.exist
+        this.rclient.eval
+          .calledWith(sinon.match.string, 1, 'lock:id-1', 'lock-value', 30)
+          .should.equal(true)
+        done()
+      })
+    })
+
+    it('should error when we no longer hold the lock', function (done) {
+      this.rclient.eval.yields(null, 0)
+      this.locker.extendLock('id-1', 'lock-value', err => {
+        expect(err).to.exist
+        done()
+      })
+    })
+  })
+
   describe('redis-sentinel', function () {
     it('should throw an error when creating a client', function () {
       const redisSentinelOptions = {

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

@@ -0,0 +1,21 @@
+const Settings = require('@overleaf/settings')
+const redis = require('@overleaf/redis-wrapper')
+const rclient = redis.createClient(Settings.redis.lock)
+const keys = Settings.redis.lock.key_schema
+const RedisLocker = require('@overleaf/redis-wrapper/RedisLocker')
+
+// Project level lock used to serialise processing of a project's pending
+// updates (the `BlockingProject:{project_id}` key). Acquired before the per-doc
+// `Blocking:{doc_id}` lock - never the other way around - to avoid deadlocks.
+module.exports = new RedisLocker({
+  rclient,
+  getKey(projectId) {
+    return keys.projectBlockingKey({ project_id: projectId })
+  },
+  wrapTimeoutError(err, projectId) {
+    err.project_id = projectId
+    return err
+  },
+  metricsPrefix: 'project',
+  lockTTLSeconds: Settings.redisLockTTLSeconds,
+})

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

@@ -2,6 +2,7 @@
 
 const { callbackifyAll } = require('@overleaf/promise-utils')
 const LockManager = require('./LockManager')
+const ProjectLockManager = require('./ProjectLockManager')
 const RedisManager = require('./RedisManager')
 const ProjectHistoryRedisManager = require('./ProjectHistoryRedisManager')
 const RealTimeRedisManager = require('./RealTimeRedisManager')
@@ -40,18 +41,30 @@ const UpdateManager = {
       doc_id: docId,
     })
 
-    const lockValue = await LockManager.promises.tryLock(docId)
-    if (lockValue == null) {
-      return
-    }
-    profile.log('tryLock')
+    // Always take the project lock before the per-doc lock to avoid deadlocks.
+    // Wait for the project lock rather than bailing out: another action
+    // holding it (e.g. for a different doc) will not process this doc's queue.
+    const projectLockValue =
+      await ProjectLockManager.promises.getLock(projectId)
+    profile.log('getProjectLock')
 
     try {
-      await UpdateManager.processOutstandingUpdates(projectId, docId)
-      profile.log('processOutstandingUpdates')
+      const lockValue = await LockManager.promises.tryLock(docId)
+      if (lockValue == null) {
+        return
+      }
+      profile.log('tryLock')
+
+      try {
+        await UpdateManager.processOutstandingUpdates(projectId, docId)
+        profile.log('processOutstandingUpdates')
+      } finally {
+        await LockManager.promises.releaseLock(docId, lockValue)
+        profile.log('releaseLock')
+      }
     } finally {
-      await LockManager.promises.releaseLock(docId, lockValue)
-      profile.log('releaseLock').end()
+      await ProjectLockManager.promises.releaseLock(projectId, projectLockValue)
+      profile.log('releaseProjectLock').end()
     }
 
     await UpdateManager.continueProcessingUpdatesWithLock(projectId, docId)
@@ -279,19 +292,29 @@ const UpdateManager = {
       doc_id: docId,
     })
 
-    const lockValue = await LockManager.promises.getLock(docId)
-    profile.log('getLock')
+    // Always take the project lock before the per-doc lock to avoid deadlocks.
+    const projectLockValue =
+      await ProjectLockManager.promises.getLock(projectId)
+    profile.log('getProjectLock')
 
     let result
     try {
-      await UpdateManager.processOutstandingUpdates(projectId, docId)
-      profile.log('processOutstandingUpdates')
+      const lockValue = await LockManager.promises.getLock(docId)
+      profile.log('getLock')
 
-      result = await method(projectId, docId, ...args)
-      profile.log('method')
+      try {
+        await UpdateManager.processOutstandingUpdates(projectId, docId)
+        profile.log('processOutstandingUpdates')
+
+        result = await method(projectId, docId, ...args)
+        profile.log('method')
+      } finally {
+        await LockManager.promises.releaseLock(docId, lockValue)
+        profile.log('releaseLock')
+      }
     } finally {
-      await LockManager.promises.releaseLock(docId, lockValue)
-      profile.log('releaseLock').end()
+      await ProjectLockManager.promises.releaseLock(projectId, projectLockValue)
+      profile.log('releaseProjectLock').end()
     }
 
     // We held the lock for a while so updates might have queued up

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

@@ -80,6 +80,9 @@ module.exports = {
         blockingKey({ doc_id: docId }) {
           return `Blocking:{${docId}}`
         },
+        projectBlockingKey({ project_id: projectId }) {
+          return `BlockingProject:{${projectId}}`
+        },
       },
     },
 

services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js

@@ -11,6 +11,7 @@ describe('UpdateManager', function () {
     this.projectHistoryId = 'history-id-123'
     this.doc_id = 'document-id-123'
     this.lockValue = 'mock-lock-value'
+    this.projectLockValue = 'mock-project-lock-value'
     this.pathname = '/a/b/c.tex'
 
     this.Metrics = {
@@ -31,6 +32,14 @@ describe('UpdateManager', function () {
       },
     }
 
+    this.ProjectLockManager = {
+      promises: {
+        tryLock: sinon.stub().resolves(this.projectLockValue),
+        getLock: sinon.stub().resolves(this.projectLockValue),
+        releaseLock: sinon.stub().resolves(),
+      },
+    }
+
     this.RedisManager = {
       promises: {
         setDocument: sinon.stub().resolves(),
@@ -92,6 +101,7 @@ describe('UpdateManager', function () {
     this.UpdateManager = SandboxedModule.require(MODULE_PATH, {
       requires: {
         './LockManager': this.LockManager,
+        './ProjectLockManager': this.ProjectLockManager,
         './RedisManager': this.RedisManager,
         './RealTimeRedisManager': this.RealTimeRedisManager,
         './ShareJsUpdateManager': this.ShareJsUpdateManager,
@@ -147,6 +157,15 @@ describe('UpdateManager', function () {
           )
         })
 
+        it('should acquire the project lock before the doc lock', function () {
+          this.ProjectLockManager.promises.getLock
+            .calledWith(this.project_id)
+            .should.equal(true)
+          this.LockManager.promises.tryLock
+            .calledAfter(this.ProjectLockManager.promises.getLock)
+            .should.equal(true)
+        })
+
         it('should acquire the lock', function () {
           this.LockManager.promises.tryLock
             .calledWith(this.doc_id)
@@ -159,6 +178,12 @@ describe('UpdateManager', function () {
             .should.equal(true)
         })
 
+        it('should free the project lock', function () {
+          this.ProjectLockManager.promises.releaseLock
+            .calledWith(this.project_id, this.projectLockValue)
+            .should.equal(true)
+        })
+
         it('should process the outstanding updates', function () {
           this.UpdateManager.promises.processOutstandingUpdates
             .calledWith(this.project_id, this.doc_id)
@@ -200,6 +225,12 @@ describe('UpdateManager', function () {
             .calledWith(this.doc_id, this.lockValue)
             .should.equal(true)
         })
+
+        it('should free the project lock', function () {
+          this.ProjectLockManager.promises.releaseLock
+            .calledWith(this.project_id, this.projectLockValue)
+            .should.equal(true)
+        })
       })
     })
 
@@ -220,6 +251,12 @@ describe('UpdateManager', function () {
           false
         )
       })
+
+      it('should free the project lock', function () {
+        this.ProjectLockManager.promises.releaseLock
+          .calledWith(this.project_id, this.projectLockValue)
+          .should.equal(true)
+      })
     })
   })
 
@@ -876,10 +913,16 @@ describe('UpdateManager', function () {
         )
       })
 
-      it('should lock the doc', function () {
+      it('should lock the project then the doc', function () {
+        this.ProjectLockManager.promises.getLock
+          .calledWith(this.project_id)
+          .should.equal(true)
         this.LockManager.promises.getLock
           .calledWith(this.doc_id)
           .should.equal(true)
+        this.LockManager.promises.getLock
+          .calledAfter(this.ProjectLockManager.promises.getLock)
+          .should.equal(true)
       })
 
       it('should process any outstanding updates', function () {
@@ -899,10 +942,13 @@ describe('UpdateManager', function () {
         expect(this.response).to.equal(this.methodResult)
       })
 
-      it('should release the lock', function () {
+      it('should release both locks', function () {
         this.LockManager.promises.releaseLock
           .calledWith(this.doc_id, this.lockValue)
           .should.equal(true)
+        this.ProjectLockManager.promises.releaseLock
+          .calledWith(this.project_id, this.projectLockValue)
+          .should.equal(true)
       })
 
       it('should continue processing updates', function () {
@@ -928,10 +974,13 @@ describe('UpdateManager', function () {
         ).to.be.rejectedWith(this.error)
       })
 
-      it('should free the lock', function () {
+      it('should free both locks', function () {
         this.LockManager.promises.releaseLock
           .calledWith(this.doc_id, this.lockValue)
           .should.equal(true)
+        this.ProjectLockManager.promises.releaseLock
+          .calledWith(this.project_id, this.projectLockValue)
+          .should.equal(true)
       })
     })
 
@@ -952,10 +1001,13 @@ describe('UpdateManager', function () {
         ).to.be.rejectedWith(this.error)
       })
 
-      it('should free the lock', function () {
+      it('should free both locks', function () {
         this.LockManager.promises.releaseLock
           .calledWith(this.doc_id, this.lockValue)
           .should.equal(true)
+        this.ProjectLockManager.promises.releaseLock
+          .calledWith(this.project_id, this.projectLockValue)
+          .should.equal(true)
       })
     })
   })