Conversation
| ) | ||
| raise AnkiHubMediaDownloadError(response, media_remote_path) | ||
| # Ignore missing files (e.g. stale media entries in the database) | ||
| if response.status_code != 403: |
There was a problem hiding this comment.
I didn't get it. Did you mean that every 403 means there is no file?
There was a problem hiding this comment.
S3 returns 403 if the file is missing for unauthenticated users, unless if bucket is modified to grant the ListBucket permission (in which case it returns 404 instead). So there's no guaranteed way to tell if the file is actually missing or it's an actual access permission problem in the bucket (which will be a big problem and completely break deck installs so practically 403 only happens when the file is missing).
This is a stopgap solution until a proper solution that stops the requests from being made in the first place is implemented (TRIAGE-321).
There was a problem hiding this comment.
Thanks for the clarification. I agree this can be a reasonable stopgap.
However, as it stands, whether a download succeeds or fails (403) makes no difference. How do we notify the UI or register the download result? We should differentiate, right?
Furthermore, can we identify why it gave a 403? Even if it is done at another time? Because if the database says that the file exists_on_s3=True, can we confirm that it still exists? And if it doesn't exist, should we change the value in the database?
There was a problem hiding this comment.
However, as it stands, whether a download succeeds or fails (403) makes no difference. How do we notify the UI or register the download result? We should differentiate, right?
My thinking was that it's fine not to differentiate them in the UI for now since we know they only add noise and increase CS tickets, as no used media files should be actually missing. It's only that the add-on is keeping record of deleted files. The errors are still logged though so we can still inspect them if needed.
Furthermore, can we identify why it gave a 403?
Not from the add-on. The solution described in TRIAGE-321 (running a backend script that marks DeckMedia objects with exists_on_s3=False as modified) will cause the add-on to automatically update its local database on the next sync, which will help with most cases of the issue. But thinking of this more, having the add-on reports these errors visibly will help us identify DeckMedia objects that should be updated in the backend, so maybe it's better to hold off on hiding the errors for now - we got a few reports so far and I already updated a few hundreds of entries in the backend based on the reports so we should get less reports now.
Closing this.
2 participants
Related issues
KNW-225
Proposed changes
The media sync progress dialog implemented in #1339 uncovered an issue where the add-on keeps trying to download media files that were deleted from the backend (TRIAGE-321). As long as the backend objects are not modified, the add-on will keep reporting errors with such files on every sync.
How to reproduce
This should not be normally reproducible with fresh deck installs. It usually happens for old users as they are more likely to have media entries belonging to deleted files in their local AnkiHub database. See report: https://community.ankihub.net/t/unable-to-download-media-file-deck-assets-e77aedfe-a636-40e2-8169-2fce2673187e-tmplvqol9-png-403-forbidden/607281