Use PATHs to call clear_artifacts

Use a list of PATHs Instead of using a ``Version.pk`` when calling
`clear_artifacts`. This is because in the ``Version.delete`` method,
the Version object is removed immediately after broadcasting the
``clear_artifacts`` task and that could produce a race condition.

With this change, we calculate all the PATHs needed _before_
broadcasting the task and _before_ deleting the object from the
database.

Author: humitos

readthedocs/builds/models.py

@@ -181,7 +181,7 @@ def save(self, *args, **kwargs):  # pylint: disable=arguments-differ
     def delete(self, *args, **kwargs):  # pylint: disable=arguments-differ
         from readthedocs.projects import tasks
         log.info('Removing files for version %s', self.slug)
-        broadcast(type='app', task=tasks.clear_artifacts, args=[self.pk])
+        broadcast(type='app', task=tasks.clear_artifacts, args=[self.get_artifact_paths()])
         broadcast(
             type='app', task=tasks.symlink_project, args=[self.project.pk])
         super(Version, self).delete(*args, **kwargs)
@@ -237,6 +237,24 @@ def get_build_path(self):
             return path
         return None
 
+    def get_artifact_paths(self):
+        """
+        Return a list of all production artifacts/media path for this version.
+
+        :rtype: list
+        """
+        paths = []
+
+        for type_ in ('pdf', 'epub', 'htmlzip'):
+            paths.append(
+                self.project.get_production_media_path(
+                    type_=type_,
+                    version_slug=self.slug),
+            )
+        paths.append(self.project.rtd_build_path(version=self.slug))
+
+        return paths
+
     def clean_build_path(self):
         """
         Clean build path for project version.

readthedocs/projects/tasks.py

@@ -764,10 +764,21 @@ def sync_files(project_pk, version_pk, hostname=None, html=False,
     synchronization of build artifacts on each application instance.
     """
     # Clean up unused artifacts
+    version = Version.objects.get(pk=version_pk)
     if not pdf:
-        clear_pdf_artifacts(version_pk)
+        remove_dir(
+            version.project.get_production_media_path(
+                type_='pdf',
+                version_slug=version.slug,
+            ),
+        )
     if not epub:
-        clear_epub_artifacts(version_pk)
+        remove_dir(
+            version.project.get_production_media_path(
+                type_='epub',
+                version_slug=version.slug,
+            ),
+        )
 
     # Sync files to the web servers
     move_files(
@@ -777,7 +788,7 @@ def sync_files(project_pk, version_pk, hostname=None, html=False,
         localmedia=localmedia,
         search=search,
         pdf=pdf,
-        epub=epub
+        epub=epub,
     )
 
     # Symlink project
@@ -1158,44 +1169,15 @@ def remove_dir(path):
 
 
 @app.task()
-def clear_artifacts(version_pk):
-    """Remove artifacts from the web servers."""
-    version = Version.objects.get(pk=version_pk)
-    clear_pdf_artifacts(version)
-    clear_epub_artifacts(version)
-    clear_htmlzip_artifacts(version)
-    clear_html_artifacts(version)
-
-
-@app.task()
-def clear_pdf_artifacts(version):
-    if isinstance(version, int):
-        version = Version.objects.get(pk=version)
-    remove_dir(version.project.get_production_media_path(
-        type_='pdf', version_slug=version.slug))
-
-
-@app.task()
-def clear_epub_artifacts(version):
-    if isinstance(version, int):
-        version = Version.objects.get(pk=version)
-    remove_dir(version.project.get_production_media_path(
-        type_='epub', version_slug=version.slug))
-
-
-@app.task()
-def clear_htmlzip_artifacts(version):
-    if isinstance(version, int):
-        version = Version.objects.get(pk=version)
-    remove_dir(version.project.get_production_media_path(
-        type_='htmlzip', version_slug=version.slug))
-
+def clear_artifacts(paths):
+    """
+    Remove artifacts from the web servers.
 
-@app.task()
-def clear_html_artifacts(version):
-    if isinstance(version, int):
-        version = Version.objects.get(pk=version)
-    remove_dir(version.project.rtd_build_path(version=version.slug))
+    :param paths: list containing PATHs where production media is on disk
+        (usually ``Version.get_artifact_paths``)
+    """
+    for path in paths:
+        remove_dir(path)
 
 
 @app.task(queue='web')

readthedocs/projects/views/private.py

@@ -157,7 +157,7 @@ def project_version_detail(request, project_slug, version_slug):
             if 'active' in form.changed_data and version.active is False:
                 log.info('Removing files for version %s', version.slug)
                 broadcast(
-                    type='app', task=tasks.clear_artifacts, args=[version.pk])
+                    type='app', task=tasks.clear_artifacts, args=[version.get_artifact_paths()])
                 version.built = False
                 version.save()
         url = reverse('project_version_list', args=[project.slug])
@@ -649,7 +649,7 @@ def project_version_delete_html(request, project_slug, version_slug):
     if not version.active:
         version.built = False
         version.save()
-        broadcast(type='app', task=tasks.clear_artifacts, args=[version.pk])
+        broadcast(type='app', task=tasks.clear_artifacts, args=[version.get_artifact_paths()])
     else:
         return HttpResponseBadRequest(
             "Can't delete HTML for an active version.")

readthedocs/rtd_tests/tests/test_celery.py

@@ -60,14 +60,14 @@ def test_clear_artifacts(self):
         directory = self.project.get_production_media_path(type_='pdf', version_slug=version.slug)
         os.makedirs(directory)
         self.assertTrue(exists(directory))
-        result = tasks.clear_artifacts.delay(version_pk=version.pk)
+        result = tasks.clear_artifacts.delay(paths=version.get_artifact_paths())
         self.assertTrue(result.successful())
         self.assertFalse(exists(directory))
 
         directory = version.project.rtd_build_path(version=version.slug)
         os.makedirs(directory)
         self.assertTrue(exists(directory))
-        result = tasks.clear_artifacts.delay(version_pk=version.pk)
+        result = tasks.clear_artifacts.delay(paths=version.get_artifact_paths())
         self.assertTrue(result.successful())
         self.assertFalse(exists(directory))