Skip to content

WPB-24076: Add meeting cleaner job in background-worker#5207

Open
blackheaven wants to merge 7 commits intodevelopfrom
gdifolco/WPB-24076-background-worker-meetings-cleaner
Open

WPB-24076: Add meeting cleaner job in background-worker#5207
blackheaven wants to merge 7 commits intodevelopfrom
gdifolco/WPB-24076-background-worker-meetings-cleaner

Conversation

@blackheaven
Copy link
Copy Markdown
Contributor

https://wearezeta.atlassian.net/browse/WPB-24076

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@blackheaven blackheaven requested review from a team as code owners April 28, 2026 10:53
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Apr 28, 2026
@blackheaven blackheaven force-pushed the gdifolco/WPB-24076-background-worker-meetings-cleaner branch from 1af1c4f to 1df93ce Compare April 28, 2026 14:32
Comment thread libs/wire-subsystems/src/Wire/MeetingsSubsystemCleaning/Interpreter.hs Outdated
Comment on lines +40 to +73
cleanupOldMeetingsImpl ::
( Member Store.MeetingsStore r,
Member ConvStore.ConversationStore r
) =>
UTCTime ->
Int ->
Sem r Int64
cleanupOldMeetingsImpl cutoffTime batchSize = do
-- 1. Fetch old meetings
oldMeetings <- Store.getOldMeetings cutoffTime batchSize

if null oldMeetings
then pure 0
else do
-- 2. Extract meeting IDs and conversation IDs
let meetingIds = map (\Store.StoredMeeting {id = mid} -> mid) oldMeetings
convIds = map (\Store.StoredMeeting {conversationId = cid} -> cid) oldMeetings

-- 3. Delete meetings from database
deletedCount <- Store.deleteMeetingBatch meetingIds

-- 4. Delete associated conversations if they are meeting conversations
-- We need to check if conversation has GroupConvType = MeetingConversation
for_ (zip oldMeetings convIds) $ \(meeting, convId) -> do
maybeConv <- ConvStore.getConversation convId
case maybeConv of
Just conv
| conv.metadata.cnvmGroupConvType == Just MeetingConversation,
conv.id_ == convId,
meeting.conversationId == convId ->
ConvStore.deleteConversation convId
_ -> pure ()

pure deletedCount
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say it would be better to use the MeetingSubsystem to delete a meeting because that will take care of proper conversation deletion as well via the ConversationSubsystem.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the reason to have it's own logic here is the batch deletion on a DB level. But we should properly remove all conversation related data, so using the MeetingSubsystem is the price we have to pay, IMO.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread libs/wire-subsystems/src/Wire/MeetingsSubsystemCleaning.hs Outdated
Comment thread libs/wire-subsystems/src/Wire/MeetingsSubsystemCleaning/Interpreter.hs Outdated
Comment thread libs/wire-subsystems/src/Wire/MeetingsStore/Postgres.hs Outdated
Comment thread libs/wire-subsystems/src/Wire/MeetingsStore/Postgres.hs Outdated
Comment thread services/background-worker/src/Wire/MeetingsCleanupWorker.hs Outdated
Comment thread services/background-worker/src/Wire/MeetingsCleanupWorker.hs
@blackheaven blackheaven force-pushed the gdifolco/WPB-24076-background-worker-meetings-cleaner branch from 1df93ce to 525b82e Compare April 28, 2026 16:29
@blackheaven blackheaven requested a review from battermann May 4, 2026 08:03
@blackheaven blackheaven force-pushed the gdifolco/WPB-24076-background-worker-meetings-cleaner branch from 692ecf7 to ce1b6b8 Compare May 4, 2026 09:03
Comment thread services/background-worker/src/Wire/MeetingsCleanupWorker.hs Outdated

backgroundWorker:
host: backgroundWorker.{{ .Release.Namespace }}.svc.cluster.local
host: background-worker.{{ .Release.Namespace }}.svc.cluster.local
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting finding 👍

Just conv
| conv.metadata.cnvmGroupConvType == Just MeetingConversation,
conv.id_ == meeting.conversationId ->
ConvStore.deleteConversation meeting.conversationId
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think that we should use the MeetingSubsystem to delete the meeting because that properly cleans up the conversation data. Here AFAICT we only delete the conversation via the ConversationStore but instead we should use the handler from the ConversationSubsystem because that takes care of deleting all the other stuff that is related to a conversation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to pair program on this tomorrow because regular meeting deletion requires a Qualified UserId and a ConnId

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants