Some attachments being ignored in dm-log

[wookie184]

Bug:

If you DM an attachment of a certain type (seems to only happen for smaller files or non image/video files, not exactly sure) to the bot, instead of relaying it to the dm-log channel as expected, the message is seemingly ignored.

Cause:

I did some detective work and found out this error was being raised by this function when the attachment was DMed to the bot:

bot_1       | 2020-08-31 17:46:29 | bot.utils.messages | WARNING | Failed to re-upload attachment test.txt from message 750048883488325682 with status 415.
bot_1       | Traceback (most recent call last):
bot_1       |   File "/bot/bot/utils/messages.py", line 79, in send_attachments
bot_1       |     await attachment.save(file, use_cached=True)
bot_1       |   File "/usr/local/lib/python3.8/site-packages/discord/message.py", line 124, in save
bot_1       |     data = await self.read(use_cached=use_cached)
bot_1       |   File "/usr/local/lib/python3.8/site-packages/discord/message.py", line 166, in read
bot_1       |     data = await self._http.get_from_cdn(url)
bot_1       |   File "/usr/local/lib/python3.8/site-packages/discord/http.py", line 239, in get_from_cdn
bot_1       |     raise HTTPException(resp, 'failed to get asset')
bot_1       | discord.errors.HTTPException: 415 Unsupported Media Type (error code: 0): failed to get asset

Some further investigations have lead me to believe that the problem is caused by using use_cached=True on this line, and indeed, looking at the docs for attachment.save it says Note that this can still fail to download deleted attachments if too much time has passed and it does not work on some types of attachments., which seems to fit.

Solution:

The simplest option would just be to removed use_cached (as it defaults to False). I'm not sure on the reasoning for setting it to True though, in the docs it says This will allow attachments to be saved after deletion more often, compared to the regular URL which is generally deleted right after the message is deleted, although I'm not sure how necessary this is.

I would be happy to make whatever change is necessary for this, if somebody can confirm what I am observing is correct and the solution is correct. Let me know here or on discord if you are struggling to reproduce this, thanks.

[MarkKoz]

We've been getting that 415 for a long time and no one has ever figured out why. A 415 response doesn't even make sense, but that's Discord for you. I believe use_cached is enabled because some other code does try to log attachments for deleted messages. If 415 is the standard response for when a cached asset couldn't be retrieved, then that error should be ignored in send_attachments.

[wookie184]

If we require use_cached=True, would it not work to try that and fall back to use_cached=False if it fails with a 415, so we keep the ability to save deleted files, but also fix this problem?

[MarkKoz]

Yeah, that sounds okay. You could make it more clever by only using the proxy when it's beneficial (i.e. when it needs to fetch a deleted attachment). This will avoid always making two separate requests. Furthermore, it may not be worth requesting via the normal URL if the proxy fails for attachments specified as deleted by the caller (or maybe let the caller specify use_cached directly). The impression I get is that the success rate of the normal URL for deleted attachments would also be quite low.

Also, the docs says proxy_url only works for certain file types, so that 415 error is starting to make some more sense. Still, I don't know why Discord would provide a proxy URL for a resource that it will just 415 for anyway.

[Numerlor]

If this is the same issue as #909; supposedly the media server is used for thumbnails and isn't cleared immediately on deletion like the message and cdn, so we should be able to ignore the filetypes that don't embed in a way where the media server for resizing etc. is necessary

[wookie184]

If we always know when the attachment we are fetching is deleted then I think then just letting the user specify use_cached would be fine. I'm guessing the code for the attachments log is currently the only place where we try and relay deleted attachments, but I'm not 100% sure.

If you think trying to work it out from the file type would be the best solution here then that is fine too, but I'm not too sure how that would be done so somebody else might have to work on it if that is the case.