diff --git a/services/document-updater/app.coffee b/services/document-updater/app.coffee index e3aee88bf7..96c7514f64 100644 --- a/services/document-updater/app.coffee +++ b/services/document-updater/app.coffee @@ -54,7 +54,7 @@ app.post '/project/:project_id/get_and_flush_if_old', HttpCont app.post '/project/:project_id/clearState', HttpController.clearProjectState app.post '/project/:project_id/doc/:doc_id', HttpController.setDoc app.post '/project/:project_id/doc/:doc_id/flush', HttpController.flushDocIfLoaded -app.delete '/project/:project_id/doc/:doc_id', HttpController.flushAndDeleteDoc +app.delete '/project/:project_id/doc/:doc_id', HttpController.deleteDoc app.delete '/project/:project_id', HttpController.deleteProject app.delete '/project', HttpController.deleteMultipleProjects app.post '/project/:project_id', HttpController.updateProject diff --git a/services/document-updater/app/coffee/DocumentManager.coffee b/services/document-updater/app/coffee/DocumentManager.coffee index ad0cb0eecb..b37d2e9433 100644 --- a/services/document-updater/app/coffee/DocumentManager.coffee +++ b/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,9 +222,9 @@ module.exports = DocumentManager = UpdateManager = require "./UpdateManager" UpdateManager.lockUpdatesAndDo DocumentManager.flushDocIfLoaded, project_id, doc_id, callback - flushAndDeleteDocWithLock: (project_id, doc_id, callback = (error) ->) -> + flushAndDeleteDocWithLock: (project_id, doc_id, options, callback) -> UpdateManager = require "./UpdateManager" - UpdateManager.lockUpdatesAndDo DocumentManager.flushAndDeleteDoc, 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" diff --git a/services/document-updater/app/coffee/HttpController.coffee b/services/document-updater/app/coffee/HttpController.coffee index e2e2e712bc..b7d38343d4 100644 --- a/services/document-updater/app/coffee/HttpController.coffee +++ b/services/document-updater/app/coffee/HttpController.coffee @@ -103,12 +103,13 @@ module.exports = HttpController = logger.log project_id: project_id, doc_id: doc_id, "flushed doc via http" res.send 204 # No Content - flushAndDeleteDoc: (req, res, next = (error) ->) -> + deleteDoc: (req, res, next = (error) ->) -> doc_id = req.params.doc_id project_id = req.params.project_id - logger.log project_id: project_id, doc_id: doc_id, "deleting doc via http" + ignoreFlushErrors = req.query.ignore_flush_errors == 'true' timer = new Metrics.Timer("http.deleteDoc") - DocumentManager.flushAndDeleteDocWithLock project_id, doc_id, (error) -> + 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 @@ -204,7 +205,7 @@ module.exports = HttpController = flushAllProjects: (req, res, next = (error)-> )-> res.setTimeout(5 * 60 * 1000) - options = + options = limit : req.query.limit || 1000 concurrency : req.query.concurrency || 5 dryRun : req.query.dryRun || false @@ -217,7 +218,7 @@ module.exports = HttpController = flushQueuedProjects: (req, res, next = (error) ->) -> res.setTimeout(10 * 60 * 1000) - options = + options = limit : req.query.limit || 1000 timeout: 5 * 60 * 1000 min_delete_age: req.query.min_delete_age || 5 * 60 * 1000 diff --git a/services/document-updater/app/coffee/ProjectManager.coffee b/services/document-updater/app/coffee/ProjectManager.coffee index 0d57687668..b60bb98d5e 100644 --- a/services/document-updater/app/coffee/ProjectManager.coffee +++ b/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) diff --git a/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee b/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee index 76ad7f5af5..a8520f7fc1 100644 --- a/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee +++ b/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", -> diff --git a/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee b/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee index b8ace494f5..00fd16c088 100644 --- a/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee +++ b/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee @@ -264,22 +264,23 @@ describe "HttpController", -> @next .calledWith(new Error("oops")) .should.equal true - - describe "flushAndDeleteDoc", -> + + describe "deleteDoc", -> beforeEach -> @req = params: project_id: @project_id doc_id: @doc_id + query: {} describe "successfully", -> beforeEach -> - @DocumentManager.flushAndDeleteDocWithLock = sinon.stub().callsArgWith(2) - @HttpController.flushAndDeleteDoc(@req, @res, @next) + @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", -> @@ -300,10 +301,24 @@ describe "HttpController", -> it "should time the request", -> @Metrics.Timer::done.called.should.equal true + describe "ignoring errors", -> + beforeEach -> + @req.query.ignore_flush_errors = 'true' + @DocumentManager.flushAndDeleteDocWithLock = sinon.stub().yields() + @HttpController.deleteDoc(@req, @res, @next) + + it "should delete the doc", -> + @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")) - @HttpController.flushAndDeleteDoc(@req, @res, @next) + @DocumentManager.flushAndDeleteDocWithLock = sinon.stub().callsArgWith(3, new Error("oops")) + @HttpController.deleteDoc(@req, @res, @next) it "should flush project history", -> @HistoryManager.flushProjectChangesAsync diff --git a/services/document-updater/test/unit/coffee/ProjectManager/flushAndDeleteProjectTests.coffee b/services/document-updater/test/unit/coffee/ProjectManager/flushAndDeleteProjectTests.coffee index 08fb6eab04..596d827726 100644 --- a/services/document-updater/test/unit/coffee/ProjectManager/flushAndDeleteProjectTests.coffee +++ b/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", ->