Send delete request to chat when expiring deleted projects (#6997)

* Send delete request to chat when expiring deleted projects

* Add script to clean-up orphaned chat of previously expired projects

GitOrigin-RevId: 157d100bd51b6204a9e31733b5164b8e7036ef01

services/web/app/src/Features/Chat/ChatApiHandler.js

@@ -15,6 +15,20 @@ let ChatApiHandler
 const OError = require('@overleaf/o-error')
 const request = require('request')
 const settings = require('@overleaf/settings')
+const { promisify } = require('util')
+
+function destroyProject(project_id, callback) {
+  if (callback == null) {
+    callback = function () {}
+  }
+  return ChatApiHandler._apiRequest(
+    {
+      url: `${settings.apis.chat.internal_url}/project/${project_id}`,
+      method: 'DELETE',
+    },
+    callback
+  )
+}
 
 module.exports = ChatApiHandler = {
   _apiRequest(opts, callback) {
@@ -165,4 +179,10 @@ module.exports = ChatApiHandler = {
       callback
     )
   },
+
+  destroyProject,
+
+  promises: {
+    destroyProject: promisify(destroyProject),
+  },
 }

services/web/app/src/Features/Project/ProjectDeleter.js

@@ -17,6 +17,7 @@ const EditorRealTimeController = require('../Editor/EditorRealTimeController')
 const HistoryManager = require('../History/HistoryManager')
 const FilestoreHandler = require('../FileStore/FileStoreHandler')
 const TpdsUpdateSender = require('../ThirdPartyDataStore/TpdsUpdateSender')
+const ChatApiHandler = require('../Chat/ChatApiHandler')
 const moment = require('moment')
 const { promiseMapWithLimit } = require('../../util/promises')
 
@@ -386,6 +387,7 @@ async function expireDeletedProject(projectId) {
       TpdsUpdateSender.promises.deleteProject({
         project_id: deletedProject.project._id,
       }),
+      ChatApiHandler.promises.destroyProject(deletedProject.project._id),
       hardDeleteDeletedFiles(deletedProject.project._id),
     ])
 

services/web/scripts/delete_orphaned_chat_threads.js

@@ -0,0 +1,191 @@
+const READ_CONCURRENCY_SECONDARY =
+  parseInt(process.env.READ_CONCURRENCY_SECONDARY, 10) || 1000
+const READ_CONCURRENCY_PRIMARY =
+  parseInt(process.env.READ_CONCURRENCY_PRIMARY, 10) || 500
+const WRITE_CONCURRENCY = parseInt(process.env.WRITE_CONCURRENCY, 10) || 10
+const BATCH_SIZE = parseInt(process.env.BATCH_SIZE, 10) || 100
+const DRY_RUN = process.env.DRY_RUN !== 'false'
+const MAX_CHATS_TO_DESTROY =
+  parseInt(process.env.MAX_CHATS_TO_DESTROY, 10) || false
+// persist fallback in order to keep batchedUpdate in-sync
+process.env.BATCH_SIZE = BATCH_SIZE
+// raise mongo timeout to 10mins if otherwise unspecified
+process.env.MONGO_SOCKET_TIMEOUT =
+  parseInt(process.env.MONGO_SOCKET_TIMEOUT, 10) || 600000
+
+const { ObjectId, ReadPreference } = require('mongodb')
+const { db } = require('../app/src/infrastructure/mongodb')
+const { promiseMapWithLimit } = require('../app/src/util/promises')
+const { batchedUpdate } = require('./helpers/batchedUpdate')
+const ChatApiHandler = require('../app/src/Features/Chat/ChatApiHandler')
+
+console.log({
+  DRY_RUN,
+  WRITE_CONCURRENCY,
+  BATCH_SIZE,
+  MAX_CHATS_TO_DESTROY,
+})
+
+const RESULT = {
+  DRY_RUN,
+  projectChatsDestroyed: 0,
+  continueFrom: null,
+}
+
+async function processBatch(_, rooms) {
+  if (rooms.length && rooms[0]._id) {
+    RESULT.continueFrom = rooms[0]._id
+  }
+
+  // Logic taken from delete_orphaned_docs_online_check.js
+  // gets projectIds from rooms,
+  // then checks 'expired' status of project
+
+  const projectIds = Array.from(
+    new Set(rooms.map(room => room.project_id.toString()))
+  ).map(ObjectId)
+  console.log(
+    `Checking projects (${projectIds.length})`,
+    JSON.stringify(projectIds)
+  )
+
+  const doubleCheckProjectIdsOnPrimary = []
+  async function checkProjectOnSecondary(projectId) {
+    if (await checkProjectExistsOnSecondary(projectId)) {
+      // Finding a project with secondary confidence is sufficient.
+      return
+    }
+    // At this point, the secondaries deem this project as having orphaned chat.
+    doubleCheckProjectIdsOnPrimary.push(projectId)
+  }
+
+  const projectsWithOrphanedChat = []
+  async function checkProjectOnPrimary(projectId) {
+    if (await checkProjectExistsOnPrimary(projectId)) {
+      // The project is actually live.
+      return
+    }
+    projectsWithOrphanedChat.push(projectId)
+  }
+
+  await promiseMapWithLimit(
+    READ_CONCURRENCY_SECONDARY,
+    projectIds,
+    checkProjectOnSecondary
+  )
+  await promiseMapWithLimit(
+    READ_CONCURRENCY_PRIMARY,
+    doubleCheckProjectIdsOnPrimary,
+    checkProjectOnPrimary
+  )
+
+  console.log(
+    `Destroying chat for projects (${projectsWithOrphanedChat.length})`,
+    JSON.stringify(projectsWithOrphanedChat)
+  )
+  if (!DRY_RUN) {
+    await promiseMapWithLimit(
+      WRITE_CONCURRENCY,
+      projectsWithOrphanedChat,
+      ChatApiHandler.promises.destroyProject
+    )
+  }
+  RESULT.projectChatsDestroyed += projectsWithOrphanedChat.length
+
+  console.log(RESULT)
+  if (
+    MAX_CHATS_TO_DESTROY &&
+    RESULT.projectChatsDestroyed >= MAX_CHATS_TO_DESTROY
+  ) {
+    console.log(
+      `MAX_CHATS_TO_DELETE limit (${MAX_CHATS_TO_DESTROY}) reached. Stopping.`
+    )
+    process.exit(0)
+  }
+}
+
+async function getDeletedProject(projectId, readPreference) {
+  return await db.deletedProjects.findOne(
+    { 'deleterData.deletedProjectId': projectId },
+    {
+      // There is no index on .project. Pull down something small.
+      projection: { 'project._id': 1 },
+      readPreference,
+    }
+  )
+}
+
+async function getProject(projectId, readPreference) {
+  return await db.projects.findOne(
+    { _id: projectId },
+    {
+      // Pulling down an empty object is fine for differentiating with null.
+      projection: { _id: 0 },
+      readPreference,
+    }
+  )
+}
+
+async function checkProjectExistsWithReadPreference(projectId, readPreference) {
+  // NOTE: Possible race conditions!
+  // There are two processes which are racing with our queries:
+  //  1. project deletion
+  //  2. project restoring
+  // For 1. we check the projects collection before deletedProjects.
+  // If a project were to be delete in this very moment, we should see the
+  //  soft-deleted entry which is created before deleting the projects entry.
+  // For 2. we check the projects collection after deletedProjects again.
+  // If a project were to be restored in this very moment, it is very likely
+  //  to see the projects entry again.
+  // Unlikely edge case: Restore+Deletion in rapid succession.
+  // We could add locking to the ProjectDeleter for ruling ^ out.
+  if (await getProject(projectId, readPreference)) {
+    // The project is live.
+    return true
+  }
+  const deletedProject = await getDeletedProject(projectId, readPreference)
+  if (deletedProject && deletedProject.project) {
+    // The project is registered for hard-deletion.
+    return true
+  }
+  if (await getProject(projectId, readPreference)) {
+    // The project was just restored.
+    return true
+  }
+  // The project does not exist.
+  return false
+}
+
+async function checkProjectExistsOnPrimary(projectId) {
+  return await checkProjectExistsWithReadPreference(
+    projectId,
+    ReadPreference.PRIMARY
+  )
+}
+
+async function checkProjectExistsOnSecondary(projectId) {
+  return await checkProjectExistsWithReadPreference(
+    projectId,
+    ReadPreference.SECONDARY
+  )
+}
+
+async function main() {
+  const projection = {
+    _id: 1,
+    project_id: 1,
+  }
+  await batchedUpdate('rooms', {}, processBatch, projection)
+  console.log('Final')
+  console.log(RESULT)
+}
+
+main()
+  .then(() => {
+    console.log('Done.')
+    process.exit(0)
+  })
+  .catch(error => {
+    console.error({ error })
+    process.exit(1)
+  })

services/web/test/acceptance/src/DeletionTests.js

@@ -6,12 +6,14 @@ const settings = require('@overleaf/settings')
 const { db, ObjectId } = require('../../../app/src/infrastructure/mongodb')
 const MockDocstoreApiClass = require('./mocks/MockDocstoreApi')
 const MockFilestoreApiClass = require('./mocks/MockFilestoreApi')
+const MockChatApiClass = require('./mocks/MockChatApi')
 
-let MockDocstoreApi, MockFilestoreApi
+let MockDocstoreApi, MockFilestoreApi, MockChatApi
 
 before(function () {
   MockDocstoreApi = MockDocstoreApiClass.instance()
   MockFilestoreApi = MockFilestoreApiClass.instance()
+  MockChatApi = MockChatApiClass.instance()
 })
 
 describe('Deleting a user', function () {
@@ -310,6 +312,7 @@ describe('Deleting a project', function () {
         MockFilestoreApi.files[this.projectId.toString()] = {
           dummyFile: 'wombat',
         }
+        MockChatApi.projects[this.projectId.toString()] = ['message']
       })
     })
 
@@ -370,6 +373,28 @@ describe('Deleting a project', function () {
         )
       })
 
+      it('Should destroy the chat', function (done) {
+        expect(MockChatApi.projects[this.projectId.toString()]).to.exist
+
+        request.post(
+          `/internal/project/${this.projectId}/expire-deleted-project`,
+          {
+            auth: {
+              user: settings.apis.web.user,
+              pass: settings.apis.web.pass,
+              sendImmediately: true,
+            },
+          },
+          (error, res) => {
+            expect(error).not.to.exist
+            expect(res.statusCode).to.equal(200)
+
+            expect(MockChatApi.projects[this.projectId.toString()]).not.to.exist
+            done()
+          }
+        )
+      })
+
       it('Should remove the project data from mongo', function (done) {
         db.deletedProjects.findOne(
           { 'deleterData.deletedProjectId': ObjectId(this.projectId) },

services/web/test/acceptance/src/mocks/MockChatApi.js

@@ -22,6 +22,12 @@ class MockChatApi extends AbstractMockApi {
     res.json(Object.assign({ room_id: projectId }, message))
   }
 
+  destroyProject(req, res) {
+    const projectId = req.params.project_id
+    delete this.projects[projectId]
+    res.sendStatus(204)
+  }
+
   applyRoutes() {
     this.app.get('/project/:project_id/messages', (req, res) =>
       this.getGlobalMessages(req, res)
@@ -29,6 +35,9 @@ class MockChatApi extends AbstractMockApi {
     this.app.post('/project/:project_id/messages', (req, res) =>
       this.sendGlobalMessage(req, res)
     )
+    this.app.delete('/project/:project_id', (req, res) =>
+      this.destroyProject(req, res)
+    )
   }
 }
 

services/web/test/unit/src/Project/ProjectDeleterTests.js

@@ -135,6 +135,11 @@ describe('ProjectDeleter', function () {
     this.Features = {
       hasFeature: sinon.stub().returns(true),
     }
+    this.ChatApiHandler = {
+      promises: {
+        destroyProject: sinon.stub().resolves(),
+      },
+    }
 
     this.ProjectDeleter = SandboxedModule.require(modulePath, {
       requires: {
@@ -148,6 +153,7 @@ describe('ProjectDeleter', function () {
         '../Tags/TagsHandler': this.TagsHandler,
         '../FileStore/FileStoreHandler': this.FileStoreHandler,
         '../ThirdPartyDataStore/TpdsUpdateSender': this.TpdsUpdateSender,
+        '../Chat/ChatApiHandler': this.ChatApiHandler,
         '../Collaborators/CollaboratorsHandler': this.CollaboratorsHandler,
         '../Collaborators/CollaboratorsGetter': this.CollaboratorsGetter,
         '../Docstore/DocstoreManager': this.DocstoreManager,
@@ -488,6 +494,12 @@ describe('ProjectDeleter', function () {
           project_id: this.deletedProjects[0].project._id,
         })
       })
+
+      it('should destroy the chat threads and messages', function () {
+        expect(
+          this.ChatApiHandler.promises.destroyProject
+        ).to.have.been.calledWith(this.deletedProjects[0].project._id)
+      })
     })
 
     describe('when history-v1 is not available', function () {
@@ -577,6 +589,11 @@ describe('ProjectDeleter', function () {
         expect(this.TpdsUpdateSender.promises.deleteProject).to.not.have.been
           .called
       })
+
+      it('should not destroy the chat threads and messages', function () {
+        expect(this.ChatApiHandler.promises.destroyProject).to.not.have.been
+          .called
+      })
     })
   })