enhance LockManager to avoid accidental unlocking

services/track-changes/app/coffee/LockManager.coffee

@@ -1,17 +1,34 @@
 Settings = require "settings-sharelatex"
 redis = require("redis-sharelatex")
 rclient = redis.createClient(Settings.redis.web)
+os = require "os"
+crypto = require "crypto"
+
+HOST = os.hostname()
+PID = process.pid
+RND = crypto.randomBytes(4).toString('hex')
+COUNT = 0
 
 module.exports = LockManager =
 	LOCK_TEST_INTERVAL: 50 # 50ms between each test of the lock
 	MAX_LOCK_WAIT_TIME: 10000 # 10s maximum time to spend trying to get the lock
 	LOCK_TTL: 300 # seconds (allow 5 minutes for any operation to complete)
 
+	# Use a signed lock value as described in
+	# http://redis.io/topics/distlock#correct-implementation-with-a-single-instance
+	# to prevent accidental unlocking by multiple processes
+	randomLock : () ->
+		time = Date.now()
+		return "locked:host=#{HOST}:pid=#{PID}:random=#{RND}:time=#{time}:count=#{COUNT++}"
+
+	unlockScript: 'if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("del", KEYS[1]) else return 0 end';
+
 	tryLock : (key, callback = (err, gotLock) ->) ->
-		rclient.set key, "locked", "EX", @LOCK_TTL, "NX", (err, gotLock)->
+		lockValue = LockManager.randomLock()
+		rclient.set key, lockValue, "EX", @LOCK_TTL, "NX", (err, gotLock)->
 			return callback(err) if err?
 			if gotLock == "OK"
-				callback err, true
+				callback err, true, lockValue
 			else
 				callback err, false
 
@@ -21,10 +38,10 @@ module.exports = LockManager =
 			if Date.now() - startTime > LockManager.MAX_LOCK_WAIT_TIME
 				return callback(new Error("Timeout"))
 
-			LockManager.tryLock key, (error, gotLock) ->
+			LockManager.tryLock key, (error, gotLock, lockValue) ->
 				return callback(error) if error?
 				if gotLock
-					callback(null)
+					callback(null, lockValue)
 				else
 					setTimeout attempt, LockManager.LOCK_TEST_INTERVAL
 
@@ -37,14 +54,14 @@ module.exports = LockManager =
 			else
 				callback err, true
 
-	releaseLock: (key, callback) ->
-		rclient.del key, callback
+	releaseLock: (key, lockValue, callback) ->
+		rclient.eval LockManager.unlockScript, 1, key, lockValue, callback
 
 	runWithLock: (key, runner = ( (releaseLock = (error) ->) -> ), callback = ( (error) -> )) ->
-		LockManager.getLock key, (error) ->
+		LockManager.getLock key, (error, lockValue) ->
 			return callback(error) if error?
 			runner (error1) ->
-				LockManager.releaseLock key, (error2) ->
+				LockManager.releaseLock key, lockValue, (error2) ->
 					error = error1 or error2
 					return callback(error) if error?
 					callback()

services/track-changes/test/acceptance/coffee/LockManagerTests.coffee

@@ -0,0 +1,31 @@
+sinon = require "sinon"
+chai = require("chai")
+chai.should()
+expect = chai.expect
+mongojs = require "../../../app/js/mongojs"
+ObjectId = mongojs.ObjectId
+Settings = require "settings-sharelatex"
+LockManager = require "../../../app/js/LockManager"
+rclient = require("redis").createClient() # Only works locally for now
+
+describe "Locking document", ->
+	describe "when the lock has expired in redis", ->
+		before (done) ->
+			LockManager.LOCK_TTL = 1 # second
+			LockManager.runWithLock "doc123", (releaseA) =>
+				# we create a lock A and allow it to expire in redis
+				setTimeout () ->
+					# now we create a new lock B and try to release A
+					LockManager.runWithLock "doc123", (releaseB) =>
+						releaseA()  # try to release lock A to see if it wipes out lock B
+					, (error) ->
+						# we never release lock B so nothing should happen here
+				, 1500 # enough time to wait until the lock has expired
+			, (error) ->
+				# we get here after trying to release lock A
+				done()
+
+		it "the new lock should not be removed by the expired locker", (done) ->
+			LockManager.checkLock "doc123", (err, isFree) ->
+					expect(isFree).to.equal false
+					done()

services/track-changes/test/unit/coffee/LockManager/LockManagerTests.coffee

@@ -46,11 +46,12 @@ describe "LockManager", ->
 		describe "when the lock is taken", ->
 			beforeEach ->
 				@rclient.set = sinon.stub().callsArgWith(5, null, null)
+				@LockManager.randomLock = sinon.stub().returns "locked-random-value"
 				@LockManager.tryLock @key, @callback
 
 			it "should check the lock in redis", ->
 				@rclient.set
-					.calledWith(@key, "locked", "EX", @LockManager.LOCK_TTL, "NX")
+					.calledWith(@key, "locked-random-value", "EX", @LockManager.LOCK_TTL, "NX")
 					.should.equal true
 
 			it "should return the callback with false", ->
@@ -137,7 +138,7 @@ describe "LockManager", ->
 					releaseLock()
 				sinon.spy @, "runner"
 				@LockManager.getLock = sinon.stub().callsArg(1)
-				@LockManager.releaseLock = sinon.stub().callsArg(1)
+				@LockManager.releaseLock = sinon.stub().callsArg(2)
 				@LockManager.runWithLock @key, @runner, @callback
 
 			it "should get the lock", ->
@@ -163,7 +164,7 @@ describe "LockManager", ->
 					releaseLock(@error)
 				sinon.spy @, "runner"
 				@LockManager.getLock = sinon.stub().callsArg(1)
-				@LockManager.releaseLock = sinon.stub().callsArg(1)
+				@LockManager.releaseLock = sinon.stub().callsArg(2)
 				@LockManager.runWithLock @key, @runner, @callback
 
 			it "should release the lock", ->