[document-updater] remove doc lock from per-doc operations (#35696)

GitOrigin-RevId: c45c1dbf11049c0fab8a7d788006a698ede0d510

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

@@ -1,18 +0,0 @@
-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')
-
-module.exports = new RedisLocker({
-  rclient,
-  getKey(docId) {
-    return keys.blockingKey({ doc_id: docId })
-  },
-  wrapTimeoutError(err, docId) {
-    err.doc_id = docId
-    return err
-  },
-  metricsPrefix: 'doc',
-  lockTTLSeconds: Settings.redisLockTTLSeconds,
-})

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

@@ -5,8 +5,7 @@ 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.
+// updates (the `BlockingProject:{project_id}` key).
 module.exports = new RedisLocker({
   rclient,
   getKey(projectId) {

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

@@ -1,7 +1,6 @@
 // @ts-check
 
 const { callbackifyAll } = require('@overleaf/promise-utils')
-const LockManager = require('./LockManager')
 const ProjectLockManager = require('./ProjectLockManager')
 const RedisManager = require('./RedisManager')
 const ProjectHistoryRedisManager = require('./ProjectHistoryRedisManager')
@@ -220,7 +219,7 @@ const UpdateManager = {
           .map(change => change.metadata.user_id)
 
         // Fire-and-forget without awaiting because
-        // we hold the doc lock here, and the result of the
+        // we hold the project lock here, and the result of the
         // notification doesn't affect the update
         WebApiManager.promises
           .notifyTrackChangesRejected(
@@ -252,7 +251,7 @@ const UpdateManager = {
         )
 
         // Do this last, since it's a mongo call, and so potentially longest running
-        // If it overruns the lock, it's ok, since all of our redis work is done
+        // If it overruns the project lock, it's ok, since all of our redis work is done
         await SnapshotManager.promises.recordSnapshot(
           projectId,
           docId,
@@ -277,7 +276,7 @@ const UpdateManager = {
 
   /**
    * Process the project's pending updates, then run the given method on the
-   * doc, all under the project and doc locks.
+   * doc, all under the project lock.
    *
    * @param {Function} method - called with (projectId, docId, ...args)
    * @param {string} projectId
@@ -291,9 +290,8 @@ const UpdateManager = {
       doc_id: docId,
     })
 
-    // Project lock first (always before the per-doc lock, to avoid deadlocks)
-    // so we can safely drain the shared per-project queue, then the per-doc
-    // lock for the operation itself.
+    // Take the project lock so we can safely drain the shared per-project queue
+    // and then run the operation itself.
     const projectLockValue =
       await ProjectLockManager.promises.getLock(projectId)
     profile.log('getProjectLock')
@@ -303,16 +301,11 @@ const UpdateManager = {
       await UpdateManager.fetchAndApplyProjectUpdates(projectId, profile)
       profile.log('fetchAndApplyProjectUpdates')
 
-      const lockValue = await LockManager.promises.getLock(docId)
-      profile.log('getLock')
+      await ProjectLockManager.promises.extendLock(projectId, projectLockValue)
+      profile.log('extendProjectLock')
 
-      try {
-        result = await method(projectId, docId, ...args)
-        profile.log('method')
-      } finally {
-        await LockManager.promises.releaseLock(docId, lockValue)
-        profile.log('releaseLock')
-      }
+      result = await method(projectId, docId, ...args)
+      profile.log('method')
     } finally {
       await ProjectLockManager.promises.releaseLock(projectId, projectLockValue)
       profile.log('releaseProjectLock').end()

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

@@ -77,9 +77,6 @@ module.exports = {
         process.env.REDIS_MAX_RETRIES_PER_REQUEST || '20'
       ),
       key_schema: {
-        blockingKey({ doc_id: docId }) {
-          return `Blocking:{${docId}}`
-        },
         projectBlockingKey({ project_id: projectId }) {
           return `BlockingProject:{${projectId}}`
         },
@@ -101,9 +98,6 @@ module.exports = {
         process.env.REDIS_MAX_RETRIES_PER_REQUEST || '20'
       ),
       key_schema: {
-        blockingKey({ doc_id: docId }) {
-          return `Blocking:{${docId}}`
-        },
         docLines({ doc_id: docId }) {
           return `doclines:{${docId}}`
         },

services/document-updater/scripts/check_redis_mongo_sync_state.js

@@ -4,7 +4,7 @@ const _ = require('lodash')
 const logger = require('@overleaf/logger')
 const OError = require('@overleaf/o-error')
 const Errors = require('../app/js/Errors')
-const LockManager = require('../app/js/LockManager')
+const ProjectLockManager = require('../app/js/ProjectLockManager')
 const PersistenceManager = require('../app/js/PersistenceManager')
 const ProjectFlusher = require('../app/js/ProjectFlusher')
 const ProjectManager = require('../app/js/ProjectManager')
@@ -43,13 +43,16 @@ const COMPARE_AND_SET =
 class TryAgainError extends Error {}
 
 /**
+ * @param {string} projectId
  * @param {string} docId
  * @param {Doc} redisDoc
  * @param {Doc} mongoDoc
  * @return {Promise<void>}
  */
-async function updateDocVersionInRedis(docId, redisDoc, mongoDoc) {
-  const lockValue = await LockManager.promises.getLock(docId)
+async function updateDocVersionInRedis(projectId, docId, redisDoc, mongoDoc) {
+  // Take the project lock to serialise with document-updater, which now
+  // processes all of a project's docs under the project lock.
+  const lockValue = await ProjectLockManager.promises.getLock(projectId)
   try {
     const key = Settings.redis.documentupdater.key_schema.docVersion({
       doc_id: docId,
@@ -68,7 +71,7 @@ async function updateDocVersionInRedis(docId, redisDoc, mongoDoc) {
       )
     }
   } finally {
-    await LockManager.promises.releaseLock(docId, lockValue)
+    await ProjectLockManager.promises.releaseLock(projectId, lockValue)
   }
 }
 
@@ -175,7 +178,7 @@ async function processDoc(projectId, docId) {
         console.log(
           `Fixing out of sync doc version for doc ${docId} in project ${projectId}: mongo=${mongoDoc.version} > redis=${redisDoc.version}`
         )
-        await updateDocVersionInRedis(docId, redisDoc, mongoDoc)
+        await updateDocVersionInRedis(projectId, docId, redisDoc, mongoDoc)
         return false
       } else {
         console.error(

services/document-updater/test/unit/js/LockManager/CheckingTheLock.js

@@ -1,65 +0,0 @@
-/* eslint-disable
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * DS206: Consider reworking classes to avoid initClass
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-const sinon = require('sinon')
-const assert = require('node:assert')
-const path = require('node:path')
-const modulePath = path.join(__dirname, '../../../../app/js/LockManager.js')
-const projectId = 1234
-const docId = 5678
-const blockingKey = `Blocking:${docId}`
-const SandboxedModule = require('sandboxed-module')
-
-describe('LockManager - checking the lock', function () {
-  let Profiler
-  const existsStub = sinon.stub()
-
-  const mocks = {
-    '@overleaf/redis-wrapper': {
-      createClient() {
-        return {
-          auth() {},
-          exists: existsStub,
-        }
-      },
-    },
-    '@overleaf/metrics': { inc() {} },
-    './Profiler': (Profiler = (function () {
-      Profiler = class Profiler {
-        static initClass() {
-          this.prototype.log = sinon.stub().returns({ end: sinon.stub() })
-          this.prototype.end = sinon.stub()
-        }
-      }
-      Profiler.initClass()
-      return Profiler
-    })()),
-  }
-  const LockManager = SandboxedModule.require(modulePath, { requires: mocks })
-
-  it('should return true if the key does not exists', function (done) {
-    existsStub.yields(null, '0')
-    return LockManager.checkLock(docId, (err, free) => {
-      if (err) return done(err)
-      free.should.equal(true)
-      return done()
-    })
-  })
-
-  return it('should return false if the key does exists', function (done) {
-    existsStub.yields(null, '1')
-    return LockManager.checkLock(docId, (err, free) => {
-      if (err) return done(err)
-      free.should.equal(false)
-      return done()
-    })
-  })
-})

services/document-updater/test/unit/js/LockManager/ReleasingTheLock.js

@@ -1,94 +0,0 @@
-/* eslint-disable
-    no-return-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * DS206: Consider reworking classes to avoid initClass
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-const sinon = require('sinon')
-const assert = require('node:assert')
-const path = require('node:path')
-const modulePath = path.join(__dirname, '../../../../app/js/LockManager.js')
-const projectId = 1234
-const docId = 5678
-const SandboxedModule = require('sandboxed-module')
-
-describe('LockManager - releasing the lock', function () {
-  beforeEach(function () {
-    let Profiler
-    this.client = {
-      auth() {},
-      eval: sinon.stub(),
-    }
-    const mocks = {
-      '@overleaf/redis-wrapper': {
-        createClient: () => this.client,
-      },
-      '@overleaf/settings': {
-        redis: {
-          lock: {
-            key_schema: {
-              blockingKey({ doc_id: docId }) {
-                return `Blocking:${docId}`
-              },
-            },
-          },
-        },
-      },
-      '@overleaf/metrics': { inc() {} },
-      './Profiler': (Profiler = (function () {
-        Profiler = class Profiler {
-          static initClass() {
-            this.prototype.log = sinon.stub().returns({ end: sinon.stub() })
-            this.prototype.end = sinon.stub()
-          }
-        }
-        Profiler.initClass()
-        return Profiler
-      })()),
-    }
-    this.LockManager = SandboxedModule.require(modulePath, { requires: mocks })
-    this.lockValue = 'lock-value-stub'
-    return (this.callback = sinon.stub())
-  })
-
-  describe('when the lock is current', function () {
-    beforeEach(function () {
-      this.client.eval = sinon.stub().yields(null, 1)
-      return this.LockManager.releaseLock(docId, this.lockValue, this.callback)
-    })
-
-    it('should clear the data from redis', function () {
-      return this.client.eval
-        .calledWith(
-          this.LockManager.unlockScript,
-          1,
-          `Blocking:${docId}`,
-          this.lockValue
-        )
-        .should.equal(true)
-    })
-
-    return it('should call the callback', function () {
-      return this.callback.called.should.equal(true)
-    })
-  })
-
-  return describe('when the lock has expired', function () {
-    beforeEach(function () {
-      this.client.eval = sinon.stub().yields(null, 0)
-      return this.LockManager.releaseLock(docId, this.lockValue, this.callback)
-    })
-
-    return it('should return an error if the lock has expired', function () {
-      return this.callback
-        .calledWith(sinon.match.instanceOf(Error))
-        .should.equal(true)
-    })
-  })
-})

services/document-updater/test/unit/js/LockManager/getLockTests.js

@@ -1,126 +0,0 @@
-/* eslint-disable
-    no-return-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS101: Remove unnecessary use of Array.from
- * DS102: Remove unnecessary code created because of implicit returns
- * DS206: Consider reworking classes to avoid initClass
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-const sinon = require('sinon')
-const modulePath = '../../../../app/js/LockManager.js'
-const SandboxedModule = require('sandboxed-module')
-
-describe('LockManager - getting the lock', function () {
-  beforeEach(function () {
-    let Profiler
-    this.LockManager = SandboxedModule.require(modulePath, {
-      requires: {
-        '@overleaf/redis-wrapper': {
-          createClient: () => {
-            return { auth() {} }
-          },
-        },
-        '@overleaf/metrics': { inc() {} },
-        './Profiler': (Profiler = (function () {
-          Profiler = class Profiler {
-            static initClass() {
-              this.prototype.log = sinon.stub().returns({ end: sinon.stub() })
-              this.prototype.end = sinon.stub()
-            }
-          }
-          Profiler.initClass()
-          return Profiler
-        })()),
-      },
-    })
-    this.callback = sinon.stub()
-    return (this.doc_id = 'doc-id-123')
-  })
-
-  describe('when the lock is not set', function () {
-    beforeEach(function (done) {
-      this.lockValue = 'mock-lock-value'
-      this.LockManager.tryLock = sinon
-        .stub()
-        .callsArgWith(1, null, true, this.lockValue)
-      return this.LockManager.getLock(this.doc_id, (...args) => {
-        this.callback(...Array.from(args || []))
-        return done()
-      })
-    })
-
-    it('should try to get the lock', function () {
-      return this.LockManager.tryLock.calledWith(this.doc_id).should.equal(true)
-    })
-
-    it('should only need to try once', function () {
-      return this.LockManager.tryLock.callCount.should.equal(1)
-    })
-
-    return it('should return the callback with the lock value', function () {
-      return this.callback.calledWith(null, this.lockValue).should.equal(true)
-    })
-  })
-
-  describe('when the lock is initially set', function () {
-    beforeEach(function (done) {
-      this.lockValue = 'mock-lock-value'
-      const startTime = Date.now()
-      let tries = 0
-      this.LockManager.LOCK_TEST_INTERVAL = 5
-      this.LockManager.tryLock = (docId, callback) => {
-        if (callback == null) {
-          callback = function () {}
-        }
-        if (Date.now() - startTime < 20 || tries < 2) {
-          tries = tries + 1
-          return callback(null, false)
-        } else {
-          return callback(null, true, this.lockValue)
-        }
-      }
-      sinon.spy(this.LockManager, 'tryLock')
-
-      return this.LockManager.getLock(this.doc_id, (...args) => {
-        this.callback(...Array.from(args || []))
-        return done()
-      })
-    })
-
-    it('should call tryLock multiple times until free', function () {
-      return (this.LockManager.tryLock.callCount > 1).should.equal(true)
-    })
-
-    return it('should return the callback with the lock value', function () {
-      return this.callback.calledWith(null, this.lockValue).should.equal(true)
-    })
-  })
-
-  return describe('when the lock times out', function () {
-    beforeEach(function (done) {
-      const time = Date.now()
-      this.LockManager.MAX_LOCK_WAIT_TIME = 5
-      this.LockManager.tryLock = sinon.stub().callsArgWith(1, null, false)
-      return this.LockManager.getLock(this.doc_id, (...args) => {
-        this.callback(...Array.from(args || []))
-        return done()
-      })
-    })
-
-    return it('should return the callback with an error', function () {
-      return this.callback
-        .calledWith(
-          sinon.match
-            .instanceOf(Error)
-            .and(sinon.match.has('doc_id', this.doc_id))
-        )
-        .should.equal(true)
-    })
-  })
-})

services/document-updater/test/unit/js/LockManager/tryLockTests.js

@@ -1,155 +0,0 @@
-/* eslint-disable
-    no-return-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * DS206: Consider reworking classes to avoid initClass
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-const sinon = require('sinon')
-const modulePath = '../../../../app/js/LockManager.js'
-const SandboxedModule = require('sandboxed-module')
-const tk = require('timekeeper')
-
-describe('LockManager - trying the lock', function () {
-  beforeEach(function () {
-    let Profiler
-    this.LockManager = SandboxedModule.require(modulePath, {
-      requires: {
-        '@overleaf/redis-wrapper': {
-          createClient: () => {
-            return {
-              auth() {},
-              set: (this.set = sinon.stub()),
-            }
-          },
-        },
-        '@overleaf/metrics': { inc() {} },
-        '@overleaf/settings': {
-          redis: {
-            lock: {
-              key_schema: {
-                blockingKey({ doc_id: docId }) {
-                  return `Blocking:${docId}`
-                },
-              },
-            },
-          },
-        },
-        './Profiler':
-          (this.Profiler = Profiler =
-            (function () {
-              Profiler = class Profiler {
-                static initClass() {
-                  this.prototype.log = sinon
-                    .stub()
-                    .returns({ end: sinon.stub() })
-                  this.prototype.end = sinon.stub()
-                }
-              }
-              Profiler.initClass()
-              return Profiler
-            })()),
-      },
-    })
-
-    this.callback = sinon.stub()
-    return (this.doc_id = 'doc-id-123')
-  })
-
-  describe('when the lock is not set', function () {
-    beforeEach(function () {
-      this.lockValue = 'mock-lock-value'
-      this.LockManager.randomLock = sinon.stub().returns(this.lockValue)
-      this.set.callsArgWith(5, null, 'OK')
-      return this.LockManager.tryLock(this.doc_id, this.callback)
-    })
-
-    it('should set the lock key with an expiry if it is not set', function () {
-      return this.set
-        .calledWith(`Blocking:${this.doc_id}`, this.lockValue, 'EX', 30, 'NX')
-        .should.equal(true)
-    })
-
-    return it('should return the callback with true and the lock value', function () {
-      return this.callback
-        .calledWith(null, true, this.lockValue)
-        .should.equal(true)
-    })
-  })
-
-  describe('when the lock is already set', function () {
-    beforeEach(function () {
-      this.set.callsArgWith(5, null, null)
-      return this.LockManager.tryLock(this.doc_id, this.callback)
-    })
-
-    return it('should return the callback with false', function () {
-      return this.callback.calledWith(null, false).should.equal(true)
-    })
-  })
-
-  return describe('when it takes a long time for redis to set the lock', function () {
-    beforeEach(function () {
-      tk.freeze(Date.now())
-      this.lockValue = 'mock-lock-value'
-      this.LockManager.randomLock = sinon.stub().returns(this.lockValue)
-      this.LockManager.releaseLock = sinon.stub().callsArgWith(2, null)
-      this.set.callsFake((_key, _v, _ex, _ttl, _nx, cb) => {
-        tk.freeze(Date.now() + 7000)
-        cb(null, 'OK')
-      })
-    })
-    after(function () {
-      tk.reset()
-    })
-
-    describe('in all cases', function () {
-      beforeEach(function () {
-        return this.LockManager.tryLock(this.doc_id, this.callback)
-      })
-
-      it('should set the lock key with an expiry if it is not set', function () {
-        return this.set
-          .calledWith(`Blocking:${this.doc_id}`, this.lockValue, 'EX', 30, 'NX')
-          .should.equal(true)
-      })
-
-      return it('should try to release the lock', function () {
-        return this.LockManager.releaseLock
-          .calledWith(this.doc_id, this.lockValue)
-          .should.equal(true)
-      })
-    })
-
-    describe('if the lock is released successfully', function () {
-      beforeEach(function () {
-        this.LockManager.releaseLock = sinon.stub().callsArgWith(2, null)
-        return this.LockManager.tryLock(this.doc_id, this.callback)
-      })
-
-      return it('should return the callback with false', function () {
-        return this.callback.calledWith(null, false).should.equal(true)
-      })
-    })
-
-    return describe('if the lock has already timed out', function () {
-      beforeEach(function () {
-        this.LockManager.releaseLock = sinon
-          .stub()
-          .callsArgWith(2, new Error('tried to release timed out lock'))
-        return this.LockManager.tryLock(this.doc_id, this.callback)
-      })
-
-      return it('should return the callback with an error', function () {
-        return this.callback
-          .calledWith(sinon.match.instanceOf(Error))
-          .should.equal(true)
-      })
-    })
-  })
-})

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

@@ -19,9 +19,6 @@ describe('RedisManager', function () {
           redis: {
             documentupdater: {
               key_schema: {
-                blockingKey({ doc_id: docId }) {
-                  return `Blocking:${docId}`
-                },
                 docLines({ doc_id: docId }) {
                   return `doclines:${docId}`
                 },

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

@@ -10,7 +10,6 @@ describe('UpdateManager', function () {
     this.project_id = 'project-id-123'
     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'
 
@@ -24,16 +23,10 @@ describe('UpdateManager', function () {
     this.Profiler.prototype.log = sinon.stub().returns({ end: sinon.stub() })
     this.Profiler.prototype.end = sinon.stub()
 
-    this.LockManager = {
-      promises: {
-        getLock: sinon.stub().resolves(this.lockValue),
-        releaseLock: sinon.stub().resolves(),
-      },
-    }
-
     this.ProjectLockManager = {
       promises: {
         getLock: sinon.stub().resolves(this.projectLockValue),
+        extendLock: sinon.stub().resolves(),
         releaseLock: sinon.stub().resolves(),
       },
     }
@@ -101,7 +94,6 @@ describe('UpdateManager', function () {
 
     this.UpdateManager = SandboxedModule.require(MODULE_PATH, {
       requires: {
-        './LockManager': this.LockManager,
         './ProjectLockManager': this.ProjectLockManager,
         './RedisManager': this.RedisManager,
         './RealTimeRedisManager': this.RealTimeRedisManager,
@@ -872,15 +864,18 @@ describe('UpdateManager', function () {
         )
       })
 
-      it('should lock the project then the doc', function () {
+      it('should lock the project', function () {
         this.ProjectLockManager.promises.getLock
           .calledWith(this.project_id)
           .should.equal(true)
-        this.LockManager.promises.getLock
-          .calledWith(this.doc_id)
+      })
+
+      it('should refresh the project lock before running the method', function () {
+        this.ProjectLockManager.promises.extendLock
+          .calledWith(this.project_id, this.projectLockValue)
           .should.equal(true)
-        this.LockManager.promises.getLock
-          .calledAfter(this.ProjectLockManager.promises.getLock)
+        this.method
+          .calledAfter(this.ProjectLockManager.promises.extendLock)
           .should.equal(true)
       })
 
@@ -900,10 +895,7 @@ describe('UpdateManager', function () {
         expect(this.response).to.equal(this.methodResult)
       })
 
-      it('should release both locks', function () {
-        this.LockManager.promises.releaseLock
-          .calledWith(this.doc_id, this.lockValue)
-          .should.equal(true)
+      it('should release the project lock', function () {
         this.ProjectLockManager.promises.releaseLock
           .calledWith(this.project_id, this.projectLockValue)
           .should.equal(true)
@@ -956,10 +948,7 @@ describe('UpdateManager', function () {
         ).to.be.rejectedWith(this.error)
       })
 
-      it('should free both locks', function () {
-        this.LockManager.promises.releaseLock
-          .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)