Merge pull request #18 from sharelatex/bg-verify-writes

merge redis hash check

services/document-updater/app/coffee/RedisManager.coffee

@@ -11,6 +11,13 @@ crypto = require "crypto"
 # Make times easy to read
 minutes = 60 # seconds for Redis expire
 
+# LUA script to write document and return hash
+# arguments: docLinesKey docLines
+setScript = """
+	redis.call('set', KEYS[1], ARGV[1])
+	return redis.sha1hex(ARGV[1])
+"""
+
 module.exports = RedisManager =
 	rclient: rclient
 
@@ -24,7 +31,7 @@ module.exports = RedisManager =
 		logger.log project_id:project_id, doc_id:doc_id, version: version, hash:docHash, "putting doc in redis"
 		ranges = RedisManager._serializeRanges(ranges)
 		multi = rclient.multi()
-		multi.set keys.docLines(doc_id:doc_id), docLines
+		multi.eval setScript, 1, keys.docLines(doc_id:doc_id), docLines
 		multi.set keys.projectKey({doc_id:doc_id}), project_id
 		multi.set keys.docVersion(doc_id:doc_id), version
 		multi.set keys.docHash(doc_id:doc_id), docHash
@@ -32,8 +39,13 @@ module.exports = RedisManager =
 			multi.set keys.ranges(doc_id:doc_id), ranges
 		else
 			multi.del keys.ranges(doc_id:doc_id)
-		multi.exec (error) ->
+		multi.exec (error, result) ->
 			return callback(error) if error?
+			# check the hash computed on the redis server
+			writeHash = result?[0]
+			if writeHash? and writeHash isnt docHash
+				logger.error project_id: project_id, doc_id: doc_id, writeHash: writeHash, origHash: docHash, "hash mismatch on putDocInMemory"
+			# update docsInProject set
 			rclient.sadd keys.docsInProject(project_id:project_id), doc_id, callback
 
 	removeDocFromMemory : (project_id, doc_id, _callback)->
@@ -136,7 +148,7 @@ module.exports = RedisManager =
 			multi = rclient.multi()
 			newDocLines = JSON.stringify(docLines)
 			newHash = RedisManager._computeHash(newDocLines)
-			multi.set    keys.docLines(doc_id:doc_id), newDocLines
+			multi.eval setScript, 1, keys.docLines(doc_id:doc_id), newDocLines
 			multi.set    keys.docVersion(doc_id:doc_id), newVersion
 			multi.set    keys.docHash(doc_id:doc_id), newHash
 			if jsonOps.length > 0
@@ -148,8 +160,12 @@ module.exports = RedisManager =
 				multi.set keys.ranges(doc_id:doc_id), ranges
 			else
 				multi.del keys.ranges(doc_id:doc_id)
-			multi.exec (error, replys) ->
+			multi.exec (error, result) ->
 					return callback(error) if error?
+					# check the hash computed on the redis server
+					writeHash = result?[0]
+					if writeHash? and writeHash isnt newHash
+						logger.error doc_id: doc_id, writeHash: writeHash, origHash: newHash, "hash mismatch on updateDocument"
 					return callback()
 
 	getDocIdsInProject: (project_id, callback = (error, doc_ids) ->) ->

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

@@ -197,6 +197,7 @@ describe "RedisManager", ->
 			@rclient.expire = sinon.stub()
 			@rclient.ltrim = sinon.stub()
 			@rclient.del = sinon.stub()
+			@rclient.eval = sinon.stub()
 			@RedisManager.getDocVersion = sinon.stub()
 			
 			@lines = ["one", "two", "three"]
@@ -205,7 +206,7 @@ describe "RedisManager", ->
 			@hash = crypto.createHash('sha1').update(JSON.stringify(@lines)).digest('hex')
 			@ranges = { comments: "mock", entries: "mock" }
 
-			@rclient.exec = sinon.stub().callsArg(0)
+			@rclient.exec = sinon.stub().callsArg(0, null, [@hash])
 
 		describe "with a consistent version", ->
 			beforeEach ->
@@ -218,8 +219,8 @@ describe "RedisManager", ->
 					.should.equal true
 		
 			it "should set the doclines", ->
-				@rclient.set
-					.calledWith("doclines:#{@doc_id}", JSON.stringify(@lines))
+				@rclient.eval
+					.calledWith(sinon.match(/redis.call/), 1, "doclines:#{@doc_id}", JSON.stringify(@lines))
 					.should.equal true
 				
 			it "should set the version", ->
@@ -254,6 +255,10 @@ describe "RedisManager", ->
 
 			it "should call the callback", ->
 				@callback.called.should.equal true
+
+			it 'should not log any errors', ->
+				@logger.error.calledWith()
+					.should.equal false
 		
 		describe "with an inconsistent version", ->
 			beforeEach ->
@@ -279,8 +284,8 @@ describe "RedisManager", ->
 					.should.equal false
 		
 			it "should still set the doclines", ->
-				@rclient.set
-					.calledWith("doclines:#{@doc_id}", JSON.stringify(@lines))
+				@rclient.eval
+					.calledWith(sinon.match(/redis.call/), 1, "doclines:#{@doc_id}", JSON.stringify(@lines))
 					.should.equal true
 		
 		describe "with empty ranges", ->
@@ -298,15 +303,30 @@ describe "RedisManager", ->
 					.calledWith("Ranges:#{@doc_id}")
 					.should.equal true
 
+		describe "with a corrupted write", ->
+			beforeEach ->
+				@badHash = "INVALID-HASH-VALUE"
+				@rclient.exec = sinon.stub().callsArgWith(0, null, [@badHash])
+				@RedisManager.getDocVersion.withArgs(@doc_id).yields(null, @version - @ops.length)
+				@RedisManager.updateDocument @doc_id, @lines, @version, @ops, @ranges, @callback
+
+			it 'should log a hash error', ->
+				@logger.error.calledWith()
+					.should.equal true
+
+			it "should call the callback", ->
+				@callback.called.should.equal true
+
 	describe "putDocInMemory", ->
 		beforeEach ->
 			@rclient.set = sinon.stub()
 			@rclient.sadd = sinon.stub().yields()
 			@rclient.del = sinon.stub()
-			@rclient.exec.yields()
+			@rclient.eval = sinon.stub()
 			@lines = ["one", "two", "three"]
 			@version = 42
 			@hash = crypto.createHash('sha1').update(JSON.stringify(@lines)).digest('hex')
+			@rclient.exec = sinon.stub().callsArgWith(0, null, [@hash])
 			@ranges = { comments: "mock", entries: "mock" }
 		
 		describe "with non-empty ranges", ->
@@ -314,8 +334,8 @@ describe "RedisManager", ->
 				@RedisManager.putDocInMemory @project_id, @doc_id, @lines, @version, @ranges, done
 			
 			it "should set the lines", ->
-				@rclient.set
-					.calledWith("doclines:#{@doc_id}", JSON.stringify @lines)
+				@rclient.eval
+					.calledWith(sinon.match(/redis.call/), 1, "doclines:#{@doc_id}", JSON.stringify(@lines))
 					.should.equal true
 			
 			it "should set the version", ->
@@ -342,6 +362,10 @@ describe "RedisManager", ->
 				@rclient.sadd
 					.calledWith("DocsIn:#{@project_id}", @doc_id)
 					.should.equal true
+
+			it 'should not log any errors', ->
+				@logger.error.calledWith()
+					.should.equal false
 	
 		describe "with empty ranges", ->
 			beforeEach (done) ->
@@ -357,6 +381,15 @@ describe "RedisManager", ->
 					.calledWith("Ranges:#{@doc_id}", JSON.stringify(@ranges))
 					.should.equal false
 
+		describe "with a corrupted write", ->
+			beforeEach (done) ->
+				@rclient.exec = sinon.stub().callsArgWith(0, null, ["INVALID-HASH-VALUE"])
+				@RedisManager.putDocInMemory @project_id, @doc_id, @lines, @version, @ranges, done
+
+			it 'should log a hash error', ->
+				@logger.error.calledWith()
+					.should.equal true
+
 	describe "removeDocFromMemory", ->
 		beforeEach (done) ->
 			@rclient.del = sinon.stub()