From c11618b475036a4e7a6fff79c3dd2fef93f66ef6 Mon Sep 17 00:00:00 2001
From: Brian Gough <bjg@network-theory.co.uk>
Date: Thu, 30 Mar 2017 15:31:34 +0100
Subject: [PATCH] improve unlock error handling

---
 .../app/coffee/LockManager.coffee             | 10 +++--
 .../LockManager/ReleasingTheLock.coffee       | 45 ++++++++++++-------
 2 files changed, 37 insertions(+), 18 deletions(-)

diff --git a/services/document-updater/app/coffee/LockManager.coffee b/services/document-updater/app/coffee/LockManager.coffee
index aae60f123d..4926f2e935 100644
--- a/services/document-updater/app/coffee/LockManager.coffee
+++ b/services/document-updater/app/coffee/LockManager.coffee
@@ -70,6 +70,10 @@ module.exports = LockManager =
 
 	releaseLock: (doc_id, lockValue, callback)->
 		key = keys.blockingKey(doc_id:doc_id)
-		rclient.eval LockManager.unlockScript, 1, key, lockValue, callback
-
-	
+		rclient.eval LockManager.unlockScript, 1, key, lockValue, (err, result) ->
+			if err?
+				return callback(err)
+			if result? and result isnt 1 # successful unlock should release exactly one key
+				logger.error {doc_id:doc_id, lockValue:lockValue, redis_err:err, redis_result:result}, "unlocking error"
+				return callback(new Error("tried to release timed out lock"))
+			callback(err,result)
diff --git a/services/document-updater/test/unit/coffee/LockManager/ReleasingTheLock.coffee b/services/document-updater/test/unit/coffee/LockManager/ReleasingTheLock.coffee
index ed502fb587..5c6b6a6381 100644
--- a/services/document-updater/test/unit/coffee/LockManager/ReleasingTheLock.coffee
+++ b/services/document-updater/test/unit/coffee/LockManager/ReleasingTheLock.coffee
@@ -8,21 +8,36 @@ doc_id     = 5678
 SandboxedModule = require('sandboxed-module')
 
 describe 'LockManager - releasing the lock', ()->
+	beforeEach ->
+		@client = {
+			auth: ->
+			eval: sinon.stub()
+		}
+		mocks =
+			"logger-sharelatex":
+				log:->
+				error:->
+			"redis-sharelatex":
+				createClient : () => @client
+			"./Metrics": {inc: () ->}
+		@LockManager = SandboxedModule.require(modulePath, requires: mocks)
+		@lockValue = "lock-value-stub"
 
-	evalStub = sinon.stub().yields(1)
-	mocks =
-		"logger-sharelatex": log:->
-		"redis-sharelatex":
-			createClient : ()->
-				auth:->
-				eval: evalStub
-		"./Metrics": {inc: () ->}
-	
-	LockManager = SandboxedModule.require(modulePath, requires: mocks)
+	describe "when the lock is current", ->
+		beforeEach ->
+			@client.eval = sinon.stub().yields(null, 1)
+			@LockManager.releaseLock doc_id, @lockValue, @callback
 
-	it 'should put a all data into memory', (done)->
-		lockValue = "lock-value-stub"
-		LockManager.releaseLock doc_id, lockValue, ->
-			evalStub.calledWith(LockManager.unlockScript, 1, "Blocking:#{doc_id}", lockValue).should.equal true
-			done()
+		it 'should clear the data from redis', ->
+			@client.eval.calledWith(@LockManager.unlockScript, 1, "Blocking:#{doc_id}", @lockValue).should.equal true
 
+		it 'should call the callback', ->
+			@callback.called.should.equal true
+
+	describe "when the lock has expired", ->
+		beforeEach ->
+			@client.eval = sinon.stub().yields(null, 0)
+			@LockManager.releaseLock doc_id, @lockValue, @callback
+
+		it 'should return an error if the lock has expired', ->
+			@callback.calledWith(new Error("tried to release timed out lock")).should.equal true