Add a scheduled task to clean up stale builds (#3312)

Fixes #2795
Fixes #3308

readthedocs/projects/tasks.py

@@ -6,6 +6,7 @@
 
 from __future__ import absolute_import
 
+import datetime
 import hashlib
 import json
 import logging
@@ -20,6 +21,7 @@
 from celery.exceptions import SoftTimeLimitExceeded
 from django.conf import settings
 from django.core.urlresolvers import reverse
+from django.db.models import Q
 from django.utils.translation import ugettext_lazy as _
 from readthedocs_build.config import ConfigError
 from slumber.exceptions import HttpClientError
@@ -41,6 +43,7 @@
 from readthedocs.core.symlink import PublicSymlink, PrivateSymlink
 from readthedocs.core.utils import send_email, broadcast
 from readthedocs.doc_builder.config import load_yaml_config
+from readthedocs.doc_builder.constants import DOCKER_LIMITS
 from readthedocs.doc_builder.environments import (LocalEnvironment,
                                                   DockerEnvironment)
 from readthedocs.doc_builder.exceptions import BuildEnvironmentError
@@ -1033,3 +1036,48 @@ def sync_callback(_, version_pk, commit, *args, **kwargs):
     """
     fileify(version_pk, commit=commit)
     update_search(version_pk, commit=commit)
+
+
+@app.task()
+def finish_inactive_builds():
+    """
+    Finish inactive builds.
+
+    A build is consider inactive if it's not in ``FINISHED`` state and it has been
+    "running" for more time that the allowed one (``Project.container_time_limit``
+    or ``DOCKER_LIMITS['time']`` plus a 20% of it).
+
+    These inactive builds will be marked as ``success`` and ``FINISHED`` with an
+    ``error`` to be communicated to the user.
+    """
+    time_limit = int(DOCKER_LIMITS['time'] * 1.2)
+    delta = datetime.timedelta(seconds=time_limit)
+    query = (~Q(state=BUILD_STATE_FINISHED) &
+             Q(date__lte=datetime.datetime.now() - delta))
+
+    builds_finished = 0
+    builds = Build.objects.filter(query)[:50]
+    for build in builds:
+
+        if build.project.container_time_limit:
+            custom_delta = datetime.timedelta(
+                seconds=int(build.project.container_time_limit))
+            if build.date + custom_delta > datetime.datetime.now():
+                # Do not mark as FINISHED builds with a custom time limit that wasn't
+                # expired yet (they are still building the project version)
+                continue
+
+        build.success = False
+        build.state = BUILD_STATE_FINISHED
+        build.error = _(
+            'This build was terminated due to inactivity. If you '
+            'continue to encounter this error, file a support '
+            'request with and reference this build id ({0}).'.format(build.pk)
+        )
+        build.save()
+        builds_finished += 1
+
+    log.info(
+        'Builds marked as "Terminated due inactivity": %s',
+        builds_finished,
+    )

readthedocs/rtd_tests/tests/test_project.py

@@ -1,16 +1,24 @@
-from __future__ import absolute_import
+# -*- coding: utf-8 -*-
+from __future__ import (
+    absolute_import, division, print_function, unicode_literals)
+
+import datetime
 import json
+
 from django.test import TestCase
-from readthedocs.builds.constants import LATEST
-from readthedocs.projects.models import Project
-from rest_framework.reverse import reverse
 from django_dynamic_fixture import get
-from readthedocs.restapi.serializers import ProjectSerializer
+from rest_framework.reverse import reverse
+
+from readthedocs.builds.constants import (
+    BUILD_STATE_CLONING, BUILD_STATE_FINISHED, BUILD_STATE_TRIGGERED, LATEST)
+from readthedocs.builds.models import Build
+from readthedocs.projects.models import Project
+from readthedocs.projects.tasks import finish_inactive_builds
 from readthedocs.rtd_tests.mocks.paths import fake_paths_by_regex
 
 
 class TestProject(TestCase):
-    fixtures = ["eric", "test_data"]
+    fixtures = ['eric', 'test_data']
 
     def setUp(self):
         self.client.login(username='eric', password='test')
@@ -40,17 +48,19 @@ def test_translations(self):
         self.assertEqual(response.status_code, 200)
 
         translation_ids_from_api = [
-            t['id'] for t in response.data['translations']]
+            t['id'] for t in response.data['translations']
+        ]
         translation_ids_from_orm = [
-            t[0] for t in main_project.translations.values_list('id')]
+            t[0] for t in main_project.translations.values_list('id')
+        ]
 
         self.assertEqual(
             set(translation_ids_from_api),
-            set(translation_ids_from_orm)
+            set(translation_ids_from_orm),
         )
 
     def test_translation_delete(self):
-        """Ensure translation deletion doesn't cascade up to main project"""
+        """Ensure translation deletion doesn't cascade up to main project."""
         # In this scenario, a user has created a project and set the translation
         # to another project. If the user deletes this new project, the delete
         # operation shouldn't cascade up to the main project, and should instead
@@ -62,12 +72,13 @@ def test_translation_delete(self):
         self.assertTrue(Project.objects.filter(pk=project_delete.pk).exists())
         self.assertEqual(
             Project.objects.get(pk=project_keep.pk).main_language_project,
-            project_delete
+            project_delete,
         )
         project_delete.delete()
         self.assertFalse(Project.objects.filter(pk=project_delete.pk).exists())
         self.assertTrue(Project.objects.filter(pk=project_keep.pk).exists())
-        self.assertIsNone(Project.objects.get(pk=project_keep.pk).main_language_project)
+        self.assertIsNone(
+            Project.objects.get(pk=project_keep.pk).main_language_project)
 
     def test_token(self):
         r = self.client.get('/api/v2/project/6/token/', {})
@@ -104,3 +115,63 @@ def test_has_epub_with_epub_build_disabled(self):
         self.pip.enable_epub_build = False
         with fake_paths_by_regex('\.epub$'):
             self.assertFalse(self.pip.has_epub(LATEST))
+
+
+class TestFinishInactiveBuildsTask(TestCase):
+    fixtures = ['eric', 'test_data']
+
+    def setUp(self):
+        self.client.login(username='eric', password='test')
+        self.pip = Project.objects.get(slug='pip')
+
+        self.taggit = Project.objects.get(slug='taggit')
+        self.taggit.container_time_limit = 7200  # 2 hours
+        self.taggit.save()
+
+        # Build just started with the default time
+        self.build_1 = Build.objects.create(
+            project=self.pip,
+            version=self.pip.get_stable_version(),
+            state=BUILD_STATE_CLONING,
+        )
+
+        # Build started an hour ago with default time
+        self.build_2 = Build.objects.create(
+            project=self.pip,
+            version=self.pip.get_stable_version(),
+            state=BUILD_STATE_TRIGGERED,
+        )
+        self.build_2.date = (
+            datetime.datetime.now() - datetime.timedelta(hours=1))
+        self.build_2.save()
+
+        # Build started an hour ago with custom time (2 hours)
+        self.build_3 = Build.objects.create(
+            project=self.taggit,
+            version=self.taggit.get_stable_version(),
+            state=BUILD_STATE_TRIGGERED,
+        )
+        self.build_3.date = (
+            datetime.datetime.now() - datetime.timedelta(hours=1))
+        self.build_3.save()
+
+    def test_finish_inactive_builds_task(self):
+        finish_inactive_builds()
+
+        # Legitimate build (just started) not finished
+        self.build_1.refresh_from_db()
+        self.assertTrue(self.build_1.success)
+        self.assertEqual(self.build_1.error, '')
+        self.assertEqual(self.build_1.state, BUILD_STATE_CLONING)
+
+        # Build with default time finished
+        self.build_2.refresh_from_db()
+        self.assertFalse(self.build_2.success)
+        self.assertNotEqual(self.build_2.error, '')
+        self.assertEqual(self.build_2.state, BUILD_STATE_FINISHED)
+
+        # Build with custom time not finished
+        self.build_3.refresh_from_db()
+        self.assertTrue(self.build_3.success)
+        self.assertEqual(self.build_3.error, '')
+        self.assertEqual(self.build_3.state, BUILD_STATE_TRIGGERED)