Change skip_flush option to ignore_flush_errors in delete doc

Instead of skipping the flush, we'll still try to flush and proceed with
the doc deletion, even when the flush fails.

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

@@ -91,7 +91,7 @@ module.exports = DocumentManager =
 							return callback(error) if error?
 							callback null
 					else
-						DocumentManager.flushAndDeleteDoc project_id, doc_id, (error) ->
+						DocumentManager.flushAndDeleteDoc project_id, doc_id, {}, (error) ->
 							# There is no harm in flushing project history if the previous
 							# call failed and sometimes it is required
 							HistoryManager.flushProjectChangesAsync project_id
@@ -115,14 +115,18 @@ module.exports = DocumentManager =
 					return callback(error) if error?
 					RedisManager.clearUnflushedTime doc_id, callback
 
-	flushAndDeleteDoc: (project_id, doc_id, _callback = (error) ->) ->
+	flushAndDeleteDoc: (project_id, doc_id, options, _callback) ->
 		timer = new Metrics.Timer("docManager.flushAndDeleteDoc")
 		callback = (args...) ->
 			timer.done()
 			_callback(args...)
 
 		DocumentManager.flushDocIfLoaded project_id, doc_id, (error) ->
-			return callback(error) if error?
+			if error?
+				if options.ignoreFlushErrors
+					logger.warn {project_id: project_id, doc_id: doc_id, err: error}, "ignoring flush error while deleting document"
+				else
+					return callback(error)
 
 			# Flush in the background since it requires a http request
 			HistoryManager.flushDocChangesAsync project_id, doc_id
@@ -218,13 +222,9 @@ module.exports = DocumentManager =
 		UpdateManager = require "./UpdateManager"
 		UpdateManager.lockUpdatesAndDo DocumentManager.flushDocIfLoaded, project_id, doc_id, callback
 
-	flushAndDeleteDocWithLock: (project_id, doc_id, callback = (error) ->) ->
-		UpdateManager = require "./UpdateManager"
-		UpdateManager.lockUpdatesAndDo DocumentManager.flushAndDeleteDoc, project_id, doc_id, callback
-
-	deleteDocWithLock: (project_id, doc_id, callback) ->
+	flushAndDeleteDocWithLock: (project_id, doc_id, options, callback) ->
 		UpdateManager = require "./UpdateManager"
-		UpdateManager.lockUpdatesAndDo RedisManager.removeDocFromMemory, project_id, doc_id, callback
+		UpdateManager.lockUpdatesAndDo DocumentManager.flushAndDeleteDoc, project_id, doc_id, options, callback
 
 	acceptChangesWithLock: (project_id, doc_id, change_ids, callback = (error) ->) ->
 		UpdateManager = require "./UpdateManager"

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

@@ -106,30 +106,18 @@ module.exports = HttpController =
 	deleteDoc: (req, res, next = (error) ->) ->
 		doc_id = req.params.doc_id
 		project_id = req.params.project_id
-		skip_flush = req.query.skip_flush == 'true'
+		ignoreFlushErrors = req.query.ignore_flush_errors == 'true'
 		timer = new Metrics.Timer("http.deleteDoc")
-		if skip_flush
-			logger.log project_id: project_id, doc_id: doc_id, "deleting doc skipping flush via http (contents may be lost)"
-
-			# Warning: This action is destructive. Skipping the flush will lose
-			# contents that have not been flushed yet. Use this to fix a document in a
-			# bad state that can't be flushed anyway.
-			DocumentManager.deleteDocWithLock project_id, doc_id, (error) ->
-				timer.done()
-				return next(error) if error?
-				logger.log project_id: project_id, doc_id: doc_id, "deleted doc via http"
-				res.send 204 # No Content
-		else
-			logger.log project_id: project_id, doc_id: doc_id, "deleting doc via http"
-			DocumentManager.flushAndDeleteDocWithLock project_id, doc_id, (error) ->
-				timer.done()
-				# There is no harm in flushing project history if the previous call
-				# failed and sometimes it is required
-				HistoryManager.flushProjectChangesAsync project_id
+		logger.log project_id: project_id, doc_id: doc_id, "deleting doc via http"
+		DocumentManager.flushAndDeleteDocWithLock project_id, doc_id, { ignoreFlushErrors: ignoreFlushErrors }, (error) ->
+			timer.done()
+			# There is no harm in flushing project history if the previous call
+			# failed and sometimes it is required
+			HistoryManager.flushProjectChangesAsync project_id
 
-				return next(error) if error?
-				logger.log project_id: project_id, doc_id: doc_id, "deleted doc via http"
-				res.send 204 # No Content
+			return next(error) if error?
+			logger.log project_id: project_id, doc_id: doc_id, "deleted doc via http"
+			res.send 204 # No Content
 
 	flushProject: (req, res, next = (error) ->) ->
 		project_id = req.params.project_id

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

@@ -52,7 +52,7 @@ module.exports = ProjectManager =
 			for doc_id in (doc_ids or [])
 				do (doc_id) ->
 					jobs.push (callback) ->
-						DocumentManager.flushAndDeleteDocWithLock project_id, doc_id, (error) ->
+						DocumentManager.flushAndDeleteDocWithLock project_id, doc_id, {}, (error) ->
 							if error?
 								logger.error err: error, project_id: project_id, doc_id: doc_id, "error deleting doc"
 								errors.push(error)

services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee

@@ -16,7 +16,7 @@ describe "DocumentManager", ->
 			"./HistoryManager": @HistoryManager =
 				flushDocChangesAsync: sinon.stub()
 				flushProjectChangesAsync: sinon.stub()
-			"logger-sharelatex": @logger = {log: sinon.stub()}
+			"logger-sharelatex": @logger = {log: sinon.stub(), warn: sinon.stub()}
 			"./DocOpsManager": @DocOpsManager = {}
 			"./Metrics": @Metrics =
 				Timer: class Timer
@@ -47,7 +47,7 @@ describe "DocumentManager", ->
 			beforeEach ->
 				@RedisManager.removeDocFromMemory = sinon.stub().callsArg(2)
 				@DocumentManager.flushDocIfLoaded = sinon.stub().callsArgWith(2)
-				@DocumentManager.flushAndDeleteDoc @project_id, @doc_id, @callback
+				@DocumentManager.flushAndDeleteDoc @project_id, @doc_id, {}, @callback
 
 			it "should flush the doc", ->
 				@DocumentManager.flushDocIfLoaded
@@ -70,6 +70,25 @@ describe "DocumentManager", ->
 					.calledWithExactly(@project_id, @doc_id)
 					.should.equal true
 
+		describe "when a flush error occurs", ->
+			beforeEach ->
+				@DocumentManager.flushDocIfLoaded = sinon.stub().callsArgWith(2, new Error("boom!"))
+				@RedisManager.removeDocFromMemory = sinon.stub().callsArg(2)
+
+			it "should not remove the doc from redis", (done) ->
+				@DocumentManager.flushAndDeleteDoc @project_id, @doc_id, {}, (error) =>
+					error.should.exist
+					@RedisManager.removeDocFromMemory.called.should.equal false
+					done()
+
+			describe "when ignoring flush errors", ->
+				it "should remove the doc from redis", (done) ->
+					@DocumentManager.flushAndDeleteDoc @project_id, @doc_id, { ignoreFlushErrors: true }, (error) =>
+						if error?
+							return done(error)
+						@RedisManager.removeDocFromMemory.called.should.equal true
+						done()
+
 	describe "flushDocIfLoaded", ->
 		describe "when the doc is in Redis", ->
 			beforeEach ->
@@ -220,7 +239,7 @@ describe "DocumentManager", ->
 				@DiffCodec.diffAsShareJsOp = sinon.stub().callsArgWith(2, null, @ops)
 				@UpdateManager.applyUpdate = sinon.stub().callsArgWith(3, null)
 				@DocumentManager.flushDocIfLoaded = sinon.stub().callsArg(2)
-				@DocumentManager.flushAndDeleteDoc = sinon.stub().callsArg(2)
+				@DocumentManager.flushAndDeleteDoc = sinon.stub().callsArg(3)
 
 			describe "when already loaded", ->
 				beforeEach ->
@@ -276,7 +295,7 @@ describe "DocumentManager", ->
 
 				it "should flush and delete the doc from the doc updater", ->
 					@DocumentManager.flushAndDeleteDoc
-						.calledWith(@project_id, @doc_id)
+						.calledWith(@project_id, @doc_id, {})
 						.should.equal true
 
 				it "should not flush the project history", ->

services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee

@@ -275,12 +275,12 @@ describe "HttpController", ->
 
 		describe "successfully", ->
 			beforeEach ->
-				@DocumentManager.flushAndDeleteDocWithLock = sinon.stub().callsArgWith(2)
+				@DocumentManager.flushAndDeleteDocWithLock = sinon.stub().callsArgWith(3)
 				@HttpController.deleteDoc(@req, @res, @next)
 
 			it "should flush and delete the doc", ->
 				@DocumentManager.flushAndDeleteDocWithLock
-					.calledWith(@project_id, @doc_id)
+					.calledWith(@project_id, @doc_id, { ignoreFlushErrors: false })
 					.should.equal true
 
 			it "should flush project history", ->
@@ -301,21 +301,23 @@ describe "HttpController", ->
 			it "should time the request", ->
 				@Metrics.Timer::done.called.should.equal true
 
-		describe "without flush", ->
+		describe "ignoring errors", ->
 			beforeEach ->
-				@req.query.skip_flush = 'true'
-				@DocumentManager.deleteDocWithLock = sinon.stub().yields()
+				@req.query.ignore_flush_errors = 'true'
+				@DocumentManager.flushAndDeleteDocWithLock = sinon.stub().yields()
 				@HttpController.deleteDoc(@req, @res, @next)
 
 			it "should delete the doc", ->
-				@DocumentManager.deleteDocWithLock.calledWith(@project_id, @doc_id).should.equal true
+				@DocumentManager.flushAndDeleteDocWithLock
+					.calledWith(@project_id, @doc_id, { ignoreFlushErrors: true })
+					.should.equal true
 
 			it "should return a successful No Content response", ->
 				@res.send.calledWith(204).should.equal true
 
 		describe "when an errors occurs", ->
 			beforeEach ->
-				@DocumentManager.flushAndDeleteDocWithLock = sinon.stub().callsArgWith(2, new Error("oops"))
+				@DocumentManager.flushAndDeleteDocWithLock = sinon.stub().callsArgWith(3, new Error("oops"))
 				@HttpController.deleteDoc(@req, @res, @next)
 
 			it "should flush project history", ->

services/document-updater/test/unit/coffee/ProjectManager/flushAndDeleteProjectTests.coffee

@@ -23,7 +23,7 @@ describe "ProjectManager - flushAndDeleteProject", ->
 		beforeEach (done) ->
 			@doc_ids = ["doc-id-1", "doc-id-2", "doc-id-3"]
 			@RedisManager.getDocIdsInProject = sinon.stub().callsArgWith(1, null, @doc_ids)
-			@DocumentManager.flushAndDeleteDocWithLock = sinon.stub().callsArg(2)
+			@DocumentManager.flushAndDeleteDocWithLock = sinon.stub().callsArg(3)
 			@ProjectManager.flushAndDeleteProjectWithLocks @project_id, {}, (error) =>
 				@callback(error)
 				done()
@@ -36,7 +36,7 @@ describe "ProjectManager - flushAndDeleteProject", ->
 		it "should delete each doc in the project", ->
 			for doc_id in @doc_ids
 				@DocumentManager.flushAndDeleteDocWithLock
-					.calledWith(@project_id, doc_id)
+					.calledWith(@project_id, doc_id, {})
 					.should.equal true
 
 		it "should flush project history", ->
@@ -54,7 +54,7 @@ describe "ProjectManager - flushAndDeleteProject", ->
 		beforeEach (done) ->
 			@doc_ids = ["doc-id-1", "doc-id-2", "doc-id-3"]
 			@RedisManager.getDocIdsInProject = sinon.stub().callsArgWith(1, null, @doc_ids)
-			@DocumentManager.flushAndDeleteDocWithLock = sinon.spy (project_id, doc_id, callback = (error) ->) =>
+			@DocumentManager.flushAndDeleteDocWithLock = sinon.spy (project_id, doc_id, options, callback) =>
 				if doc_id == "doc-id-1"
 					callback(@error = new Error("oops, something went wrong"))
 				else
@@ -66,7 +66,7 @@ describe "ProjectManager - flushAndDeleteProject", ->
 		it "should still flush each doc in the project", ->
 			for doc_id in @doc_ids
 				@DocumentManager.flushAndDeleteDocWithLock
-					.calledWith(@project_id, doc_id)
+					.calledWith(@project_id, doc_id, {})
 					.should.equal true
 
 		it "should still flush project history", ->