Merge pull request #17 from sharelatex/ja-consistency-tweaks

Retry rewind if doc and update versions don't match

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

@@ -5,12 +5,11 @@ logger = require "logger-sharelatex"
 
 module.exports = DiffManager =
 	getLatestDocAndUpdates: (project_id, doc_id, fromVersion, toVersion, callback = (error, content, version, updates) ->) ->
-		# Whichever order these are in, there is potential for updates to come in between
-		# them so that they do not return the same 'latest' versions.
-		# TODO: If these don't return consistent content, try again.
-		UpdatesManager.getDocUpdatesWithUserInfo project_id, doc_id, from: fromVersion, to: toVersion, (error, updates) ->
+		# Get updates last, since then they must be ahead and it
+		# might be possible to rewind to the same version as the doc.
+		DocumentUpdaterManager.getDocument project_id, doc_id, (error, content, version) ->
 			return callback(error) if error?
-			DocumentUpdaterManager.getDocument project_id, doc_id, (error, content, version) ->
+			UpdatesManager.getDocUpdatesWithUserInfo project_id, doc_id, from: fromVersion, to: toVersion, (error, updates) ->
 				return callback(error) if error?
 				callback(null, content, version, updates)
 	
@@ -34,7 +33,28 @@ module.exports = DiffManager =
 			
 			callback(null, diff)
 
-	getDocumentBeforeVersion: (project_id, doc_id, version, callback = (error, document, rewoundUpdates) ->) ->
+	getDocumentBeforeVersion: (project_id, doc_id, version, _callback = (error, document, rewoundUpdates) ->) ->
+		# Whichever order we get the latest document and the latest updates,
+		# there is potential for updates to be applied between them so that
+		# they do not return the same 'latest' versions.
+		# If this happens, we just retry and hopefully get them at the compatible
+		# versions.
+		retries = 3
+		callback = (error, args...) ->
+			if error?
+				if error.retry and retries > 0
+					logger.warn {error, project_id, doc_id, version, retries}, "retrying getDocumentBeforeVersion"
+					retry()
+				else
+					_callback(error)
+			else
+				_callback(null, args...)
+
+		do retry = () ->
+			retries--
+			DiffManager._tryGetDocumentBeforeVersion(project_id, doc_id, version, callback)
+
+	_tryGetDocumentBeforeVersion: (project_id, doc_id, version, callback = (error, document, rewoundUpdates) ->) ->
 		logger.log project_id: project_id, doc_id: doc_id, version: version, "getting document before version"
 		DiffManager.getLatestDocAndUpdates project_id, doc_id, version, null, (error, content, version, updates) ->
 			return callback(error) if error?
@@ -49,7 +69,9 @@ module.exports = DiffManager =
 
 			lastUpdate = updates[0]
 			if lastUpdate? and lastUpdate.v != version - 1
-				return callback new Error("latest update version, #{lastUpdate.v}, does not match doc version, #{version}")
+				error = new Error("latest update version, #{lastUpdate.v}, does not match doc version, #{version}")
+				error.retry = true
+				return callback error
 			
 			logger.log {docVersion: version, lastUpdateVersion: lastUpdate?.v, updateCount: updates.length}, "rewinding updates"
 

services/track-changes/test/unit/coffee/DiffManager/DiffManagerTests.coffee

@@ -8,7 +8,7 @@ SandboxedModule = require('sandboxed-module')
 describe "DiffManager", ->
 	beforeEach ->
 		@DiffManager = SandboxedModule.require modulePath, requires:
-			"logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub() }
+			"logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub(), warn: sinon.stub() }
 			"./UpdatesManager": @UpdatesManager = {}
 			"./DocumentUpdaterManager": @DocumentUpdaterManager = {}
 			"./DiffGenerator": @DiffGenerator = {}
@@ -90,6 +90,76 @@ describe "DiffManager", ->
 					.should.equal true
 
 	describe "getDocumentBeforeVersion", ->
+		beforeEach ->
+			@DiffManager._tryGetDocumentBeforeVersion = sinon.stub()
+			@document = "mock-documents"
+			@rewound_updates = "mock-rewound-updates"
+
+		describe "succesfully", ->
+			beforeEach ->
+				@DiffManager._tryGetDocumentBeforeVersion.yields(null, @document, @rewound_updates)
+				@DiffManager.getDocumentBeforeVersion @project_id, @doc_id, @version, @callback
+			
+			it "should call _tryGetDocumentBeforeVersion", ->
+				@DiffManager._tryGetDocumentBeforeVersion
+					.calledWith(@project_id, @doc_id, @version)
+					.should.equal true
+			
+			it "should call the callback with the response", ->
+				@callback.calledWith(null, @document, @rewound_updates).should.equal true
+		
+		describe "with a retry needed", ->
+			beforeEach ->
+				retried = false
+				@DiffManager._tryGetDocumentBeforeVersion = (project_id, doc_id, version, callback) =>
+					if !retried
+						retried = true
+						error = new Error()
+						error.retry = true
+						callback error
+					else
+						callback(null, @document, @rewound_updates)
+				sinon.spy @DiffManager, "_tryGetDocumentBeforeVersion"
+				@DiffManager.getDocumentBeforeVersion @project_id, @doc_id, @version, @callback
+			
+			it "should call _tryGetDocumentBeforeVersion twice", ->
+				@DiffManager._tryGetDocumentBeforeVersion
+					.calledTwice
+					.should.equal true
+			
+			it "should call the callback with the response", ->
+				@callback.calledWith(null, @document, @rewound_updates).should.equal true
+		
+		describe "with a non-retriable error", ->
+			beforeEach ->
+				@error = new Error("oops")
+				@DiffManager._tryGetDocumentBeforeVersion.yields(@error)
+				@DiffManager.getDocumentBeforeVersion @project_id, @doc_id, @version, @callback
+			
+			it "should call _tryGetDocumentBeforeVersion once", ->
+				@DiffManager._tryGetDocumentBeforeVersion
+					.calledOnce
+					.should.equal true
+			
+			it "should call the callback with the error", ->
+				@callback.calledWith(@error).should.equal true
+		
+		describe "when retry limit is matched", ->
+			beforeEach ->
+				@error = new Error("oops")
+				@error.retry = true
+				@DiffManager._tryGetDocumentBeforeVersion.yields(@error)
+				@DiffManager.getDocumentBeforeVersion @project_id, @doc_id, @version, @callback
+			
+			it "should call _tryGetDocumentBeforeVersion three times (max retries)", ->
+				@DiffManager._tryGetDocumentBeforeVersion
+					.calledThrice
+					.should.equal true
+			
+			it "should call the callback with the error", ->
+				@callback.calledWith(@error).should.equal true
+
+	describe "_tryGetDocumentBeforeVersion", ->
 		beforeEach ->
 			@content = "hello world"
 			# Op versions are the version they were applied to, so doc is always one version
@@ -113,7 +183,7 @@ describe "DiffManager", ->
 					updates.reverse()
 					return @rewound_content
 				@rewindUpdatesWithArgs = @DiffGenerator.rewindUpdates.withArgs(@content, @updates.slice().reverse())
-				@DiffManager.getDocumentBeforeVersion @project_id, @doc_id, @fromVersion, @callback
+				@DiffManager._tryGetDocumentBeforeVersion @project_id, @doc_id, @fromVersion, @callback
 
 			it "should get the latest doc and version with all recent updates", ->
 				@DiffManager.getLatestDocAndUpdates
@@ -131,12 +201,12 @@ describe "DiffManager", ->
 				@version = 50
 				@updates = [ { op: "mock-1", v: 40 }, { op: "mock-1", v: 39 } ]
 				@DiffManager.getLatestDocAndUpdates = sinon.stub().callsArgWith(4, null, @content, @version, @updates)
-				@DiffManager.getDocumentBeforeVersion @project_id, @doc_id, @fromVersion, @callback
+				@DiffManager._tryGetDocumentBeforeVersion @project_id, @doc_id, @fromVersion, @callback
 
-			it "should call the callback with an error", ->
-				@callback
-					.calledWith(new Error("latest update version, 40, does not match doc version, 42"))
-					.should.equal true
+			it "should call the callback with an error with retry = true set", ->
+				@callback.calledOnce.should.equal true
+				error = @callback.args[0][0]
+				expect(error.retry).to.equal true
 
 		describe "when the updates are inconsistent", ->
 			beforeEach ->