Race condition between anti-spam and anti-malware

[MarkKoz]

An anti-spam DeletionContext will re-upload attachments to the #attachment-log channel. The anti-malware cog will delete attachments that are blacklisted. This sometimes results in a 404 when anti-spam is trying to upload the attachments:

Sentry Issue: BOT-6D

NotFound: 404 Not Found (error code: 0): asset not found
  File "bot/utils/messages.py", line 81, in send_attachments
    await attachment.save(file, use_cached=True)
  File "discord/message.py", line 124, in save
    data = await self.read(use_cached=use_cached)
  File "discord/message.py", line 166, in read
    data = await self._http.get_from_cdn(url)
  File "discord/http.py", line 235, in get_from_cdn
    raise NotFound(resp, 'asset not found')

Failed to re-upload attachment <REDACTED> from message <REDACTED> with status 404.

[SebastiaanZ]

We seem to have multiple race conditions between our various message processing utilities and we've patched some of them already (e.g., the token removal and codeblock suggestion utilities).

There's also still the possibility that messages that get removed by one of our filtering tools (anti-spam, filtering, anti-malware) still trigger command responses from our bot, as both the on_message listeners as well as the command functions are obviously process asynchronously.

This makes me wonder if our current approach of doing these things truly asynchronously is the best way to go. One way we could prevent most of these race conditions is to switch to a different approach: Have a central on_message event processor in our Bot subclass that first processes the required filters, in an order we think is relevant, before moving on to the "process this message for commands" phase of the bot.

This would not fix all race conditions, as things that rely on multiple messages (e.g. anti-spam) could still suffer from them, but it would mean more robust processing of a single message.

The way to do this would be to turn the current on_message listeners of the several message processing utilities we have into "entry point" coroutines that get awaited in the central on_message event processor.

An obvious question is how large the impact of this change would be to perceived responsiveness of the bot, as we'd only process commands after the filtering process has completed. It does, however, both solve the "we should probably check for tokens before suggesting a codeblock"-like order issues as well as different features fighting over who gets to delete the message and process its contents.

(Alternatively and/or in addition to this, we may want to have a central feature that processes a message that should be deleted that automatically does things like attachment deletion processing. We currently only relay attachments in the anti-spam filter, but, with a central "offensive message deletion" utility, we could both uniform this approach. If we don't go with a "forced order" on filters, this could be used to solve "deletion race conditions" in a different way centrally suppressing NotFound errors.)