[clsi] remove locking from docker actions (#32373)

* [clsi] remove locking from docker actions

Start:
- We have an in-memory lock on the compile request

Destroy:
- as part of run: see above
- as part of cleanup: we check the last access time now, so it cannot
  happen concurrent to compiling anymore.

Co-authored-by: Anna Claire Fields <anna.fields@overleaf.com>

* [clsi] update comment

---------

Co-authored-by: Anna Claire Fields <anna.fields@overleaf.com>
GitOrigin-RevId: a58df45416ae31c0b38d5efec7f9371d747303df

services/clsi/app/js/DockerLockManager.js

@@ -1,110 +0,0 @@
-// 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
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-import logger from '@overleaf/logger'
-let LockManager
-
-const LockState = {} // locks for docker container operations, by container name
-
-export default LockManager = {
-  MAX_LOCK_HOLD_TIME: 15000, // how long we can keep a lock
-  MAX_LOCK_WAIT_TIME: 10000, // how long we wait for a lock
-  LOCK_TEST_INTERVAL: 1000, // retry time
-
-  tryLock(key, callback) {
-    let lockValue
-    if (callback == null) {
-      callback = function () {}
-    }
-    const existingLock = LockState[key]
-    if (existingLock != null) {
-      // the lock is already taken, check how old it is
-      const lockAge = Date.now() - existingLock.created
-      if (lockAge < LockManager.MAX_LOCK_HOLD_TIME) {
-        return callback(null, false) // we didn't get the lock, bail out
-      } else {
-        logger.error(
-          { key, lock: existingLock, age: lockAge },
-          'taking old lock by force'
-        )
-      }
-    }
-    // take the lock
-    LockState[key] = lockValue = { created: Date.now() }
-    return callback(null, true, lockValue)
-  },
-
-  getLock(key, callback) {
-    let attempt
-    if (callback == null) {
-      callback = function () {}
-    }
-    const startTime = Date.now()
-    return (attempt = () =>
-      LockManager.tryLock(key, function (error, gotLock, lockValue) {
-        if (error != null) {
-          return callback(error)
-        }
-        if (gotLock) {
-          return callback(null, lockValue)
-        } else if (Date.now() - startTime > LockManager.MAX_LOCK_WAIT_TIME) {
-          const e = new Error('Lock timeout')
-          e.key = key
-          return callback(e)
-        } else {
-          return setTimeout(attempt, LockManager.LOCK_TEST_INTERVAL)
-        }
-      }))()
-  },
-
-  releaseLock(key, lockValue, callback) {
-    if (callback == null) {
-      callback = function () {}
-    }
-    const existingLock = LockState[key]
-    if (existingLock === lockValue) {
-      // lockValue is an object, so we can test by reference
-      delete LockState[key] // our lock, so we can free it
-      return callback()
-    } else if (existingLock != null) {
-      // lock exists but doesn't match ours
-      logger.error(
-        { key, lock: existingLock },
-        'tried to release lock taken by force'
-      )
-      return callback()
-    } else {
-      logger.error(
-        { key, lock: existingLock },
-        'tried to release lock that has gone'
-      )
-      return callback()
-    }
-  },
-
-  runWithLock(key, runner, callback) {
-    if (callback == null) {
-      callback = function () {}
-    }
-    return LockManager.getLock(key, function (error, lockValue) {
-      if (error != null) {
-        return callback(error)
-      }
-      return runner((error1, ...args) =>
-        LockManager.releaseLock(key, lockValue, function (error2) {
-          error = error1 || error2
-          if (error != null) {
-            return callback(error)
-          }
-          return callback(null, ...Array.from(args))
-        })
-      )
-    })
-  },
-}

services/clsi/test/unit/js/DockerLockManager.test.js

@@ -1,259 +0,0 @@
-import { vi, describe, beforeEach, it } from 'vitest'
-
-import sinon from 'sinon'
-import path from 'node:path'
-const modulePath = path.join(
-  import.meta.dirname,
-  '../../../app/js/DockerLockManager'
-)
-
-describe('DockerLockManager', function () {
-  beforeEach(async function (ctx) {
-    vi.doMock('@overleaf/settings', () => ({
-      default: (ctx.Settings = { clsi: { docker: {} } }),
-    }))
-
-    return (ctx.LockManager = (await import(modulePath)).default)
-  })
-
-  return describe('runWithLock', function () {
-    describe('with a single lock', function () {
-      beforeEach(async function (ctx) {
-        await new Promise((resolve, reject) => {
-          ctx.callback = sinon.stub()
-          return ctx.LockManager.runWithLock(
-            'lock-one',
-            releaseLock =>
-              setTimeout(() => releaseLock(null, 'hello', 'world'), 100),
-
-            (err, ...args) => {
-              ctx.callback(err, ...Array.from(args))
-              return resolve()
-            }
-          )
-        })
-      })
-
-      return it('should call the callback', function (ctx) {
-        return ctx.callback
-          .calledWith(null, 'hello', 'world')
-          .should.equal(true)
-      })
-    })
-
-    describe('with two locks', function () {
-      beforeEach(async function (ctx) {
-        await new Promise((resolve, reject) => {
-          ctx.callback1 = sinon.stub()
-          ctx.callback2 = sinon.stub()
-          ctx.LockManager.runWithLock(
-            'lock-one',
-            releaseLock =>
-              setTimeout(() => releaseLock(null, 'hello', 'world', 'one'), 100),
-
-            (err, ...args) => {
-              return ctx.callback1(err, ...Array.from(args))
-            }
-          )
-          return ctx.LockManager.runWithLock(
-            'lock-two',
-            releaseLock =>
-              setTimeout(() => releaseLock(null, 'hello', 'world', 'two'), 200),
-
-            (err, ...args) => {
-              ctx.callback2(err, ...Array.from(args))
-              return resolve()
-            }
-          )
-        })
-      })
-
-      it('should call the first callback', function (ctx) {
-        return ctx.callback1
-          .calledWith(null, 'hello', 'world', 'one')
-          .should.equal(true)
-      })
-
-      return it('should call the second callback', function (ctx) {
-        return ctx.callback2
-          .calledWith(null, 'hello', 'world', 'two')
-          .should.equal(true)
-      })
-    })
-
-    return describe('with lock contention', function () {
-      describe('where the first lock is released quickly', function () {
-        beforeEach(async function (ctx) {
-          await new Promise((resolve, reject) => {
-            ctx.LockManager.MAX_LOCK_WAIT_TIME = 1000
-            ctx.LockManager.LOCK_TEST_INTERVAL = 100
-            ctx.callback1 = sinon.stub()
-            ctx.callback2 = sinon.stub()
-            ctx.LockManager.runWithLock(
-              'lock',
-              releaseLock =>
-                setTimeout(
-                  () => releaseLock(null, 'hello', 'world', 'one'),
-                  100
-                ),
-
-              (err, ...args) => {
-                return ctx.callback1(err, ...Array.from(args))
-              }
-            )
-            return ctx.LockManager.runWithLock(
-              'lock',
-              releaseLock =>
-                setTimeout(
-                  () => releaseLock(null, 'hello', 'world', 'two'),
-                  200
-                ),
-
-              (err, ...args) => {
-                ctx.callback2(err, ...Array.from(args))
-                return resolve()
-              }
-            )
-          })
-        })
-
-        it('should call the first callback', function (ctx) {
-          return ctx.callback1
-            .calledWith(null, 'hello', 'world', 'one')
-            .should.equal(true)
-        })
-
-        return it('should call the second callback', function (ctx) {
-          return ctx.callback2
-            .calledWith(null, 'hello', 'world', 'two')
-            .should.equal(true)
-        })
-      })
-
-      describe('where the first lock is held longer than the waiting time', function () {
-        beforeEach(async function (ctx) {
-          await new Promise((resolve, reject) => {
-            let doneTwo
-            ctx.LockManager.MAX_LOCK_HOLD_TIME = 10000
-            ctx.LockManager.MAX_LOCK_WAIT_TIME = 1000
-            ctx.LockManager.LOCK_TEST_INTERVAL = 100
-            ctx.callback1 = sinon.stub()
-            ctx.callback2 = sinon.stub()
-            let doneOne = (doneTwo = false)
-            const finish = function (key) {
-              if (key === 1) {
-                doneOne = true
-              }
-              if (key === 2) {
-                doneTwo = true
-              }
-              if (doneOne && doneTwo) {
-                return resolve()
-              }
-            }
-            ctx.LockManager.runWithLock(
-              'lock',
-              releaseLock =>
-                setTimeout(
-                  () => releaseLock(null, 'hello', 'world', 'one'),
-                  1100
-                ),
-
-              (err, ...args) => {
-                ctx.callback1(err, ...Array.from(args))
-                return finish(1)
-              }
-            )
-            return ctx.LockManager.runWithLock(
-              'lock',
-              releaseLock =>
-                setTimeout(
-                  () => releaseLock(null, 'hello', 'world', 'two'),
-                  100
-                ),
-
-              (err, ...args) => {
-                ctx.callback2(err, ...Array.from(args))
-                return finish(2)
-              }
-            )
-          })
-        })
-
-        it('should call the first callback', function (ctx) {
-          return ctx.callback1
-            .calledWith(null, 'hello', 'world', 'one')
-            .should.equal(true)
-        })
-
-        return it('should call the second callback with an error', function (ctx) {
-          const error = sinon.match.instanceOf(Error)
-          return ctx.callback2.calledWith(error).should.equal(true)
-        })
-      })
-
-      return describe('where the first lock is held longer than the max holding time', function () {
-        beforeEach(async function (ctx) {
-          await new Promise((resolve, reject) => {
-            let doneTwo
-            ctx.LockManager.MAX_LOCK_HOLD_TIME = 1000
-            ctx.LockManager.MAX_LOCK_WAIT_TIME = 2000
-            ctx.LockManager.LOCK_TEST_INTERVAL = 100
-            ctx.callback1 = sinon.stub()
-            ctx.callback2 = sinon.stub()
-            let doneOne = (doneTwo = false)
-            const finish = function (key) {
-              if (key === 1) {
-                doneOne = true
-              }
-              if (key === 2) {
-                doneTwo = true
-              }
-              if (doneOne && doneTwo) {
-                return resolve()
-              }
-            }
-            ctx.LockManager.runWithLock(
-              'lock',
-              releaseLock =>
-                setTimeout(
-                  () => releaseLock(null, 'hello', 'world', 'one'),
-                  1500
-                ),
-
-              (err, ...args) => {
-                ctx.callback1(err, ...Array.from(args))
-                return finish(1)
-              }
-            )
-            return ctx.LockManager.runWithLock(
-              'lock',
-              releaseLock =>
-                setTimeout(
-                  () => releaseLock(null, 'hello', 'world', 'two'),
-                  100
-                ),
-
-              (err, ...args) => {
-                ctx.callback2(err, ...Array.from(args))
-                return finish(2)
-              }
-            )
-          })
-        })
-
-        it('should call the first callback', function (ctx) {
-          return ctx.callback1
-            .calledWith(null, 'hello', 'world', 'one')
-            .should.equal(true)
-        })
-
-        return it('should call the second callback', function (ctx) {
-          return ctx.callback2
-            .calledWith(null, 'hello', 'world', 'two')
-            .should.equal(true)
-        })
-      })
-    })
-  })
-})