Repository: readthedocs/readthedocs.org
Issue: Deleting a project does not delete media in storage

As we write more build output to cloud storage we need to make sure that those remote files are removed when versions or projects are deleted. Currently this does not appear to be happening.


Changed files:
readthedocs/projects/tasks.py, readthedocs/projects/models.py, readthedocs/projects/views/public.py, readthedocs/rtd_tests/tests/test_project_views.py, readthedocs/builds/syncers.py, readthedocs/builds/models.py, readthedocs/projects/views/private.py, readthedocs/builds/storage.py, readthedocs/projects/constants.py

diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py
index 493c4997f..63da874a3 100644
--- a/readthedocs/builds/models.py
+++ b/readthedocs/builds/models.py
@@ -24,6 +24,7 @@ from readthedocs.projects.constants import (
     GITLAB_URL,
     PRIVACY_CHOICES,
     PRIVATE,
+    MEDIA_TYPES,
 )
 from readthedocs.projects.models import APIProject, Project
 from readthedocs.projects.version_handling import determine_stable_version
@@ -259,6 +260,11 @@ class Version(models.Model):
             task=tasks.remove_dirs,
             args=[self.get_artifact_paths()],
         )
+
+        # Remove build artifacts from storage
+        storage_paths = self.get_storage_paths()
+        tasks.remove_build_storage_paths.delay(storage_paths)
+
         project_pk = self.project.pk
         super().delete(*args, **kwargs)
         broadcast(
@@ -340,6 +346,25 @@ class Version(models.Model):
 
         return paths
 
+    def get_storage_paths(self):
+        """
+        Return a list of all build artifact storage paths for this version.
+
+        :rtype: list
+        """
+        paths = []
+
+        for type_ in MEDIA_TYPES:
+            paths.append(
+                self.project.get_storage_path(
+                    type_=type_,
+                    version_slug=self.slug,
+                    include_file=False,
+                )
+            )
+
+        return paths
+
     def clean_build_path(self):
         """
         Clean build path for project version.
diff --git a/readthedocs/builds/storage.py b/readthedocs/builds/storage.py
index f79406428..06cc795e2 100644
--- a/readthedocs/builds/storage.py
+++ b/readthedocs/builds/storage.py
@@ -1,6 +1,7 @@
 import logging
 from pathlib import Path
 
+from django.core.exceptions import SuspiciousFileOperation
 from django.core.files.storage import FileSystemStorage
 from storages.utils import safe_join, get_available_overwrite_name
 
@@ -51,6 +52,9 @@ class BuildMediaStorageMixin:
 
         :param path: the path to the directory to remove
         """
+        if path in ('', '/'):
+            raise SuspiciousFileOperation('Deleting all storage cannot be right')
+
         log.debug('Deleting directory %s from media storage', path)
         folders, files = self.listdir(self._dirpath(path))
         for folder_name in folders:
diff --git a/readthedocs/builds/syncers.py b/readthedocs/builds/syncers.py
index bee327e88..7241288b9 100644
--- a/readthedocs/builds/syncers.py
+++ b/readthedocs/builds/syncers.py
@@ -10,14 +10,12 @@ import os
 import shutil
 
 from django.conf import settings
-from django.core.files.storage import get_storage_class
 
 from readthedocs.core.utils import safe_makedirs
 from readthedocs.core.utils.extend import SettingsOverrideObject
 
 
 log = logging.getLogger(__name__)
-storage = get_storage_class()()
 
 
 class BaseSyncer:
diff --git a/readthedocs/projects/constants.py b/readthedocs/projects/constants.py
index a80e22f19..7c83cffd7 100644
--- a/readthedocs/projects/constants.py
+++ b/readthedocs/projects/constants.py
@@ -19,6 +19,19 @@ DOCUMENTATION_CHOICES = (
     ('sphinx_singlehtml', _('Sphinx Single Page HTML')),
 )
 
+MEDIA_TYPE_HTML = 'html'
+MEDIA_TYPE_PDF = 'pdf'
+MEDIA_TYPE_EPUB = 'epub'
+MEDIA_TYPE_HTMLZIP = 'htmlzip'
+MEDIA_TYPE_JSON = 'json'
+MEDIA_TYPES = (
+    MEDIA_TYPE_HTML,
+    MEDIA_TYPE_PDF,
+    MEDIA_TYPE_EPUB,
+    MEDIA_TYPE_HTMLZIP,
+    MEDIA_TYPE_JSON,
+)
+
 SAMPLE_FILES = (
     ('Installation', 'projects/samples/installation.rst.html'),
     ('Getting started', 'projects/samples/getting_started.rst.html'),
diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py
index 04e092e7f..a5bbb9a13 100644
--- a/readthedocs/projects/models.py
+++ b/readthedocs/projects/models.py
@@ -42,9 +42,15 @@ from readthedocs.search.parse_json import process_file
 from readthedocs.vcs_support.backends import backend_cls
 from readthedocs.vcs_support.utils import Lock, NonBlockingLock
 
+from .constants import (
+    MEDIA_TYPES,
+    MEDIA_TYPE_PDF,
+    MEDIA_TYPE_EPUB,
+    MEDIA_TYPE_HTMLZIP,
+)
+
 
 log = logging.getLogger(__name__)
-storage = get_storage_class()()
 
 
 class ProjectRelationship(models.Model):
@@ -463,6 +469,29 @@ class Project(models.Model):
         except Exception:
             log.exception('Error creating default branches')
 
+    def delete(self, *args, **kwargs):  # pylint: disable=arguments-differ
+        from readthedocs.projects import tasks
+
+        # Remove local FS build artifacts on the web servers
+        broadcast(
+            type='app',
+            task=tasks.remove_dirs,
+            args=[(self.doc_path,)],
+        )
+
+        # Remove build artifacts from storage
+        storage_paths = []
+        for type_ in MEDIA_TYPES:
+            storage_paths.append(
+                '{}/{}'.format(
+                    type_,
+                    self.slug,
+                )
+            )
+        tasks.remove_build_storage_paths.delay(storage_paths)
+
+        super().delete(*args, **kwargs)
+
     def get_absolute_url(self):
         return reverse('projects_detail', args=[self.slug])
 
@@ -746,32 +775,30 @@ class Project(models.Model):
     def has_aliases(self):
         return self.aliases.exists()
 
-    def has_pdf(self, version_slug=LATEST):
+    def has_media(self, type_, version_slug=LATEST):
         path = self.get_production_media_path(
-            type_='pdf', version_slug=version_slug
-        )
-        storage_path = self.get_storage_path(
-            type_='pdf', version_slug=version_slug
+            type_=type_, version_slug=version_slug
         )
-        return os.path.exists(path) or storage.exists(storage_path)
+        if os.path.exists(path):
+            return True
+
+        if settings.RTD_BUILD_MEDIA_STORAGE:
+            storage = get_storage_class(settings.RTD_BUILD_MEDIA_STORAGE)()
+            storage_path = self.get_storage_path(
+                type_=type_, version_slug=version_slug
+            )
+            return storage.exists(storage_path)
+
+        return False
+
+    def has_pdf(self, version_slug=LATEST):
+        return self.has_media(MEDIA_TYPE_PDF, version_slug=version_slug)
 
     def has_epub(self, version_slug=LATEST):
-        path = self.get_production_media_path(
-            type_='epub', version_slug=version_slug
-        )
-        storage_path = self.get_storage_path(
-            type_='epub', version_slug=version_slug
-        )
-        return os.path.exists(path) or storage.exists(storage_path)
+        return self.has_media(MEDIA_TYPE_EPUB, version_slug=version_slug)
 
     def has_htmlzip(self, version_slug=LATEST):
-        path = self.get_production_media_path(
-            type_='htmlzip', version_slug=version_slug
-        )
-        storage_path = self.get_storage_path(
-            type_='htmlzip', version_slug=version_slug
-        )
-        return os.path.exists(path) or storage.exists(storage_path)
+        return self.has_media(MEDIA_TYPE_HTMLZIP, version_slug=version_slug)
 
     @property
     def sponsored(self):
diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py
index 4291f1dbc..38182c095 100644
--- a/readthedocs/projects/tasks.py
+++ b/readthedocs/projects/tasks.py
@@ -1680,6 +1680,20 @@ def remove_dirs(paths):
         shutil.rmtree(path, ignore_errors=True)
 
 
+@app.task(queue='web')
+def remove_build_storage_paths(paths):
+    """
+    Remove artifacts from build media storage (cloud or local storage)
+
+    :param paths: list of paths in build media storage to delete
+    """
+    if settings.RTD_BUILD_MEDIA_STORAGE:
+        storage = get_storage_class(settings.RTD_BUILD_MEDIA_STORAGE)()
+        for storage_path in paths:
+            log.info('Removing %s from media storage', storage_path)
+            storage.delete_directory(storage_path)
+
+
 @app.task(queue='web')
 def sync_callback(_, version_pk, commit, *args, **kwargs):
     """
diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py
index d360fecd4..c43eec68f 100644
--- a/readthedocs/projects/views/private.py
+++ b/readthedocs/projects/views/private.py
@@ -210,11 +210,7 @@ def project_delete(request, project_slug):
     )
 
     if request.method == 'POST':
-        broadcast(
-            type='app',
-            task=tasks.remove_dirs,
-            args=[(project.doc_path,)],
-        )
+        # Delete the project and all related files
         project.delete()
         messages.success(request, _('Project deleted'))
         project_dashboard = reverse('projects_dashboard')
diff --git a/readthedocs/projects/views/public.py b/readthedocs/projects/views/public.py
index 8003778da..64d822727 100644
--- a/readthedocs/projects/views/public.py
+++ b/readthedocs/projects/views/public.py
@@ -33,7 +33,6 @@ from .base import ProjectOnboardMixin
 log = logging.getLogger(__name__)
 search_log = logging.getLogger(__name__ + '.search')
 mimetypes.add_type('application/epub+zip', '.epub')
-storage = get_storage_class()()
 
 
 class ProjectIndex(ListView):
@@ -217,11 +216,14 @@ def project_download_media(request, project_slug, type_, version_slug):
     )
 
     if settings.DEFAULT_PRIVACY_LEVEL == 'public' or settings.DEBUG:
-        storage_path = version.project.get_storage_path(
-            type_=type_, version_slug=version_slug
-        )
-        if storage.exists(storage_path):
-            return HttpResponseRedirect(storage.url(storage_path))
+
+        if settings.RTD_BUILD_MEDIA_STORAGE:
+            storage = get_storage_class(settings.RTD_BUILD_MEDIA_STORAGE)()
+            storage_path = version.project.get_storage_path(
+                type_=type_, version_slug=version_slug
+            )
+            if storage.exists(storage_path):
+                return HttpResponseRedirect(storage.url(storage_path))
 
         media_path = os.path.join(
             settings.MEDIA_URL,
diff --git a/readthedocs/rtd_tests/tests/test_project_views.py b/readthedocs/rtd_tests/tests/test_project_views.py
index 1396df1ad..e5d151ead 100644
--- a/readthedocs/rtd_tests/tests/test_project_views.py
+++ b/readthedocs/rtd_tests/tests/test_project_views.py
@@ -404,7 +404,7 @@ class TestPrivateViews(MockBuildTestCase):
         response = self.client.get('/dashboard/pip/delete/')
         self.assertEqual(response.status_code, 200)
 
-        with patch('readthedocs.projects.views.private.broadcast') as broadcast:
+        with patch('readthedocs.projects.models.broadcast') as broadcast:
             response = self.client.post('/dashboard/pip/delete/')
             self.assertEqual(response.status_code, 302)
             self.assertFalse(Project.objects.filter(slug='pip').exists())