Hitting ratelimits for getting/deleting spam messages

[CloudburstSys]

Izzy sometimes hits ratelimits for deleting messages.

Potential fix: Service which specifically queues message delete requests so that ratelimits aren't hit.

[Ixrec]

To summarize the Discord discussion, we have two relatively small fixes in mind for the spam situation:

• Today ProcessTrip calls message.DeleteAsync() for every message. Instead it should batch the messages by channel and call DeleteMessagesAsync() once per channel. In practice, almost all spam trips are for a single channel anyway.

• Don't do bulkDeletionLog.Add() until after the message was successfully deleted.

If we discover any other ways Izzy can get rate-limited in practice, those would require some other solution. For example, if fifty users spam at once, she'd probably hit limits, but that's probably not a serious concern in practice.

[Ixrec]

Some of my spam tests show rate limit warnings for getting messages, not just for deleting them. I'm not sure what we can do about that.

[Ixrec]

After circling back to this today, thinking about it some more, experimenting in Bot Testing and reading about C# thread stuff, I have arrived at the following conclusions:

• The spam logic to get/delete all recent messages is the one and only case in Izzy's codebase where:
  • A single event or command generates O(n) network requests instead of O(constant)
  • Multiple events can compete to make the same edits (internal state edits and Discord network edit requests)
• When the spam -> rate limit scenario occurs (which remains the only plausible, much less reproducible rate limit scenario we know of), not only does Discord rate limit our message getting and message deleting, but some of Izzy's threads crash due to this competition. Revealing this was an unexpected benefit of #206.
• Because this is C#, not Javascript, every event potentially leads to a new thread running in parallel with any number of other event handling threads (unless the owner of the event uses something like AutoResetEvent, which Discord.NET evidently is not and to be fair probably should not).
• I believe the fundamental solution is to synchronize SpamService's access to _users, especially the PreviousMessages field, with some lock(...) {} statements.

We should probably still do the ideas in https://github.com/Manechat/izzy-moonbot/issues/167#issuecomment-1335066966 and https://github.com/Manechat/izzy-moonbot/issues/169 as well, but locking _users ought to be necessary and sufficient to deal with the specific problem of Izzy getting herself rate limited by a relatively small number of incoming events. I don't think we need to defend ourselves against moderators or Discord itself generating too many commands or events (and Izzy already switches to batch logging ModChannel messages during a raid).

[Ixrec]

I made an attempt to either use channel.DeleteMessagesAsync() or avoid GetMessageAsync(), but concluded that these are too difficult or problematic to be worth it.

GetMessageAsync() is necessary because it will always be possible for a spammer to post N messages, have Izzy delete all N of them and post a modchat summary, then an additional few messages from the same spammer to finally reach Izzy, leading to a second round of deletions with a second modchat summary. That second round needs to know that half of what's in the RecentMessages cache has already been deleted, and actually fetching the messages is the only way to be sure they still exist. Since the only bulk fetch method I know of--channel.GetMessagesAsync()--assumes a contiguous range of messages, we can't use that here either.

DeleteMessagesAsync() has a murkier problem: It turns out to be the first use case we've had for a method interacting with Discord that takes another Discord object we need to mock as an argument. Specifically: it doesn't want a list of message ids as I hoped; it wants a list of IMessages. So even if I wrote an Izzy mock for this it would have to involve asking "is this IIzzyMessage a real Discord IMessage or the TestMessage?" at runtime, which is brittle enough I don't believe this is worth it. Not to mention it would be a pain to restructure the deletion loop to group messages by channel.

With those specific changes rejected, it looks like #523 was the last step of this issue after all. Unless we get a real incident of a spammer causing Izzy to go haywire instead of just delete the spam and ping us about it a few times, I don't think there's anything more to be done here.