From 89f12e6b1fecebd9e259fccd6d70822db5b41652 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Tue, 14 Jul 2026 09:03:33 +0200 Subject: [PATCH] [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 | 45 ++++++++++++++ libraries/redis-wrapper/test/unit/src/test.js | 32 ++++++++++ .../app/js/ProjectLockManager.js | 21 +++++++ .../document-updater/app/js/UpdateManager.js | 57 ++++++++++++------ .../config/settings.defaults.js | 3 + .../js/UpdateManager/UpdateManagerTests.js | 60 +++++++++++++++++-- 6 files changed, 197 insertions(+), 21 deletions(-) create mode 100644 services/document-updater/app/js/ProjectLockManager.js diff --git a/libraries/redis-wrapper/RedisLocker.js b/libraries/redis-wrapper/RedisLocker.js index 17ad514246..f8b4e8cd96 100644 --- a/libraries/redis-wrapper/RedisLocker.js +++ b/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 diff --git a/libraries/redis-wrapper/test/unit/src/test.js b/libraries/redis-wrapper/test/unit/src/test.js index b8a9fbeca7..19f01ef4cd 100644 --- a/libraries/redis-wrapper/test/unit/src/test.js +++ b/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 = { diff --git a/services/document-updater/app/js/ProjectLockManager.js b/services/document-updater/app/js/ProjectLockManager.js new file mode 100644 index 0000000000..ce396fd056 --- /dev/null +++ b/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, +}) diff --git a/services/document-updater/app/js/UpdateManager.js b/services/document-updater/app/js/UpdateManager.js index ac5c313b9d..8675c5eae3 100644 --- a/services/document-updater/app/js/UpdateManager.js +++ b/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 diff --git a/services/document-updater/config/settings.defaults.js b/services/document-updater/config/settings.defaults.js index af592a27ba..fac4c11e02 100755 --- a/services/document-updater/config/settings.defaults.js +++ b/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}}` + }, }, }, diff --git a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js index 2ae14acd53..e140344c76 100644 --- a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js +++ b/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) }) }) })