search

lookit / lookit-api

Codebase for Lookit v2 and Experimenter v2. Includes an API. Docs: http://lookit.readthedocs.io/
https://lookit.mit.edu/
MIT License
10 stars 18 forks source link

Sentry: Video.DoesNotExist studies.tasks.delete_video_from_cloud #1412

Closed okaycj closed 3 weeks ago

okaycj commented 2 months ago

Summary

Sentry Issue

Django will throw DoesNoExist when we attempt to get something that isn't in the DB. This PR adds an exists check before attempting to ask S3 to remove it.

Open Question

Do we need to handle the state where the video is on S3 but not in our DB?

sonarcloud[bot] commented 2 months ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

becky-gilbert commented 2 months ago

Do we need to handle the state where the video is on S3 but not in our DB?

I'm not sure. delete_video_on_s3 is a pre-delete hook in the Video class, so this delete_video_from_cloud task should only run when a Video object is deleted from the DB. In order for a Video object to be deleted, it has to exist in the DB in the first place, right? Or is it possible for a delete request to be made for a video that doesn't exist in the DB, and for that request to get as far as this pre-delete hook without causing a 'does not exist' error? I guess we could try it if we wanted to know for sure...

becky-gilbert commented 2 months ago

Would it be hard to add a log for this case? Something like "Error deleting video from cloud: it was not found in the database." That way we could search for this message in the future, so that we can see if/when it's happening and handle it if necessary.

becky-gilbert commented 2 months ago

Do we need to handle the state where the video is on S3 but not in our DB?

After investigating this a bit more (see here: #1423) I think the answer is yes. We'll need to:

  1. Fix the code so that the S3 deletion hook can successfully delete the video (or at least get the filename) before the video object is deleted from the DB. Even if this PR fixes the Sentry errors, the videos will continue to not be deleted from S3.
  2. Fix the backlog of S3 videos that should've been deleted by going through the list of Sentry errors, or compare the S3 objects list to the DB list and remove any from S3 that do not exist in the DB.
becky-gilbert commented 3 weeks ago

Closing this as it was fixed in #1433