Update build status even on exception (#2573)

* Never call update_build from a BuildEnvironment

* Fix broken celery test

This is really a design decision thing. Either projects should be marked
as having valid clones even if record is off, or they shouldn't.

Personally, I believe the former behavior is better, since we will have
actually cloned the repo at that point (in the setup_env), but this does
mean that record will record *some* data in the database, which might
not be desirable.

* Catch all exceptions when exiting BuildEnvironment

This removes the special handling of BuildEnvironmentError, and
logs and catches all exceptions which aren't subclasses of
BuildEnvironmentWarning.

This simplifies exception handling for users of BuildEnvironments,
but may have some unexpected effects if, for instance,
SystemExit, StopIteration or KeyboardInterrupt is raised while
inside a BuildEnvironment.

* Don't surface the exception message to the end user

* Raise BuildEnvironmentError on error parsing YAML

readthedocs/doc_builder/backends/mkdocs.py

@@ -7,6 +7,7 @@
 from django.template import Context, loader as template_loader
 
 from readthedocs.doc_builder.base import BaseBuilder
+from readthedocs.doc_builder.exceptions import BuildEnvironmentError
 
 log = logging.getLogger(__name__)
 
@@ -42,6 +43,15 @@ def append_conf(self, **kwargs):
             user_config = {
                 'site_name': self.version.project.name,
             }
+        except yaml.YAMLError as exc:
+            note = ''
+            if hasattr(exc, 'problem_mark'):
+                mark = exc.problem_mark
+                note = ' (line %d, column %d)' % (mark.line+1, mark.column+1)
+            raise BuildEnvironmentError(
+                "Your mkdocs.yml could not be loaded, "
+                "possibly due to a syntax error%s" % (
+                    note,))
 
         # Handle custom docs dirs
 

readthedocs/doc_builder/environments.py

@@ -12,7 +12,7 @@
 from datetime import datetime
 
 from django.utils.text import slugify
-from django.utils.translation import ugettext_lazy as _
+from django.utils.translation import ugettext_lazy as _, ugettext_noop
 from docker import Client
 from docker.utils import create_host_config
 from docker.errors import APIError as DockerAPIError, DockerException
@@ -281,7 +281,7 @@ def __enter__(self):
 
     def __exit__(self, exc_type, exc_value, tb):
         ret = self.handle_exception(exc_type, exc_value, tb)
-        self.update_build(state=BUILD_STATE_FINISHED)
+        self.build['state'] = BUILD_STATE_FINISHED
         log.info(LOG_TEMPLATE
                  .format(project=self.project.slug,
                          version=self.version.slug,
@@ -294,24 +294,19 @@ def handle_exception(self, exc_type, exc_value, _):
         This reports on the exception we're handling and special cases
         subclasses of BuildEnvironmentException.  For
         :py:class:`BuildEnvironmentWarning`, exit this context gracefully, but
-        don't mark the build as a failure.  For :py:class:`BuildEnvironmentError`,
-        exit gracefully, but mark the build as a failure.  For all other
-        exception classes, the build will be marked as a failure and an
-        exception will bubble up.
+        don't mark the build as a failure.  For all other exception classes,
+        including :py:class:`BuildEnvironmentError`, the build will be marked as
+        a failure and the context will be gracefully exited.
         """
         if exc_type is not None:
             log.error(LOG_TEMPLATE
                       .format(project=self.project.slug,
                               version=self.version.slug,
                               msg=exc_value),
                       exc_info=True)
-            if issubclass(exc_type, BuildEnvironmentWarning):
-                return True
-            else:
+            if not issubclass(exc_type, BuildEnvironmentWarning):
                 self.failure = exc_value
-                if issubclass(exc_type, BuildEnvironmentError):
-                    return True
-                return False
+            return True
 
     def run(self, *cmd, **kwargs):
         '''Shortcut to run command from environment'''
@@ -409,7 +404,14 @@ def update_build(self, state=None):
             self.build['length'] = build_length.total_seconds()
 
         if self.failure is not None:
-            self.build['error'] = str(self.failure)
+            # Only surface the error message if it was a
+            # BuildEnvironmentException or BuildEnvironmentWarning
+            if isinstance(self.failure,
+                          (BuildEnvironmentException, BuildEnvironmentWarning)):
+                self.build['error'] = str(self.failure)
+            else:
+                self.build['error'] = ugettext_noop(
+                    "An unexpected error occurred")
 
         # Attempt to stop unicode errors on build reporting
         for key, val in self.build.items():
@@ -480,7 +482,7 @@ def __enter__(self):
                         _('A build environment is currently '
                           'running for this version'))
                     self.failure = exc
-                    self.update_build(state=BUILD_STATE_FINISHED)
+                    self.build['state'] = BUILD_STATE_FINISHED
                     raise exc
                 else:
                     log.warn(LOG_TEMPLATE

readthedocs/rtd_tests/mocks/mock_api.py

@@ -9,6 +9,9 @@ class ProjectData(object):
     def get(self):
         return dict()
 
+    def put(self, x=None):
+        return x
+
 
 def mock_version(repo):
     class MockVersion(object):
@@ -77,7 +80,7 @@ def project(self, x):
         return ProjectData()
 
     def build(self, x):
-        return mock.Mock(**{'get.return_value': {'state': 'triggered'}})
+        return mock.Mock(**{'get.return_value': {'id': 123, 'state': 'triggered'}})
 
     def command(self, x):
         return mock.Mock(**{'get.return_value': {}})

readthedocs/rtd_tests/tests/test_celery.py

@@ -8,6 +8,7 @@
 from django_dynamic_fixture import get
 from mock import patch, MagicMock
 
+from readthedocs.builds.constants import BUILD_STATE_INSTALLING, BUILD_STATE_FINISHED
 from readthedocs.builds.models import Build
 from readthedocs.projects.models import Project
 from readthedocs.projects import tasks
@@ -78,6 +79,42 @@ def test_update_docs(self):
                 intersphinx=False)
         self.assertTrue(result.successful())
 
+    @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs',
+           new=MagicMock)
+    @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs')
+    @patch('readthedocs.doc_builder.environments.BuildEnvironment.update_build')
+    def test_update_docs_unexpected_setup_exception(self, mock_update_build, mock_setup_vcs):
+        exc = Exception()
+        mock_setup_vcs.side_effect = exc
+        build = get(Build, project=self.project,
+                    version=self.project.versions.first())
+        with mock_api(self.repo) as mapi:
+            result = tasks.update_docs.delay(
+                self.project.pk,
+                build_pk=build.pk,
+                record=False,
+                intersphinx=False)
+        self.assertTrue(result.successful())
+        mock_update_build.assert_called_once_with(state=BUILD_STATE_FINISHED)
+
+    @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs')
+    @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs',
+           new=MagicMock)
+    @patch('readthedocs.doc_builder.environments.BuildEnvironment.update_build')
+    def test_update_docs_unexpected_build_exception(self, mock_update_build, mock_build_docs):
+        exc = Exception()
+        mock_build_docs.side_effect = exc
+        build = get(Build, project=self.project,
+                    version=self.project.versions.first())
+        with mock_api(self.repo) as mapi:
+            result = tasks.update_docs.delay(
+                self.project.pk,
+                build_pk=build.pk,
+                record=False,
+                intersphinx=False)
+        self.assertTrue(result.successful())
+        mock_update_build.assert_called_with(state=BUILD_STATE_FINISHED)
+
     def test_update_imported_doc(self):
         with mock_api(self.repo):
             result = tasks.update_imported_docs.delay(self.project.pk)

readthedocs/rtd_tests/tests/test_doc_building.py

@@ -22,6 +22,8 @@
 from readthedocs.rtd_tests.base import RTDTestCase
 from readthedocs.rtd_tests.mocks.environment import EnvironmentMockGroup
 
+DUMMY_BUILD_ID = 123
+
 
 class TestLocalEnvironment(TestCase):
     '''Test execution and exception handling in environment'''
@@ -44,14 +46,15 @@ def test_normal_execution(self):
         type(self.mocks.process).returncode = PropertyMock(return_value=0)
 
         build_env = LocalEnvironment(version=self.version, project=self.project,
-                                     build={})
+                                     build={'id': DUMMY_BUILD_ID})
         with build_env:
             build_env.run('echo', 'test')
         self.assertTrue(self.mocks.process.communicate.called)
         self.assertTrue(build_env.done)
         self.assertTrue(build_env.successful)
         self.assertEqual(len(build_env.commands), 1)
         self.assertEqual(build_env.commands[0].output, u'This is okay')
+        self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
 
     def test_failing_execution(self):
         '''Build in failing state'''
@@ -60,7 +63,7 @@ def test_failing_execution(self):
         type(self.mocks.process).returncode = PropertyMock(return_value=1)
 
         build_env = LocalEnvironment(version=self.version, project=self.project,
-                                     build={})
+                                     build={'id': DUMMY_BUILD_ID})
         with build_env:
             build_env.run('echo', 'test')
             self.fail('This should be unreachable')
@@ -69,11 +72,12 @@ def test_failing_execution(self):
         self.assertTrue(build_env.failed)
         self.assertEqual(len(build_env.commands), 1)
         self.assertEqual(build_env.commands[0].output, u'This is not okay')
+        self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
 
     def test_failing_execution_with_caught_exception(self):
         '''Build in failing state with BuildEnvironmentError exception'''
         build_env = LocalEnvironment(version=self.version, project=self.project,
-                                     build={})
+                                     build={'id': DUMMY_BUILD_ID})
 
         with build_env:
             raise BuildEnvironmentError('Foobar')
@@ -82,20 +86,20 @@ def test_failing_execution_with_caught_exception(self):
         self.assertEqual(len(build_env.commands), 0)
         self.assertTrue(build_env.done)
         self.assertTrue(build_env.failed)
+        self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
 
-    def test_failing_execution_with_uncaught_exception(self):
+    def test_failing_execution_with_unexpected_exception(self):
         '''Build in failing state with exception from code'''
         build_env = LocalEnvironment(version=self.version, project=self.project,
-                                     build={})
+                                     build={'id': DUMMY_BUILD_ID})
 
-        def _inner():
-            with build_env:
-                raise Exception()
+        with build_env:
+            raise ValueError('uncaught')
 
-        self.assertRaises(Exception, _inner)
         self.assertFalse(self.mocks.process.communicate.called)
         self.assertTrue(build_env.done)
         self.assertTrue(build_env.failed)
+        self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
 
 
 class TestDockerEnvironment(TestCase):
@@ -116,7 +120,7 @@ def tearDown(self):
     def test_container_id(self):
         '''Test docker build command'''
         docker = DockerEnvironment(version=self.version, project=self.project,
-                                   build={'id': 123})
+                                   build={'id': DUMMY_BUILD_ID})
         self.assertEqual(docker.container_id,
                          'build-123-project-6-pip')
 
@@ -126,13 +130,14 @@ def test_connection_failure(self):
             'side_effect': DockerException
         })
         build_env = DockerEnvironment(version=self.version, project=self.project,
-                                      build={})
+                                      build={'id': DUMMY_BUILD_ID})
 
         def _inner():
             with build_env:
                 self.fail('Should not hit this')
 
         self.assertRaises(BuildEnvironmentError, _inner)
+        self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
 
     def test_api_failure(self):
         '''Failing API error response from docker should raise exception'''
@@ -146,13 +151,14 @@ def test_api_failure(self):
         })
 
         build_env = DockerEnvironment(version=self.version, project=self.project,
-                                      build={})
+                                      build={'id': DUMMY_BUILD_ID})
 
         def _inner():
             with build_env:
                 self.fail('Should not hit this')
 
         self.assertRaises(BuildEnvironmentError, _inner)
+        self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
 
     def test_command_execution(self):
         '''Command execution through Docker'''
@@ -163,7 +169,7 @@ def test_command_execution(self):
         })
 
         build_env = DockerEnvironment(version=self.version, project=self.project,
-                                      build={'id': 123})
+                                      build={'id': DUMMY_BUILD_ID})
         with build_env:
             build_env.run('echo test', cwd='/tmp')
 
@@ -177,6 +183,7 @@ def test_command_execution(self):
         self.assertEqual(build_env.commands[0].output, 'This is the return')
         self.assertEqual(build_env.commands[0].error, None)
         self.assertTrue(build_env.failed)
+        self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
 
     def test_command_execution_cleanup_exception(self):
         '''Command execution through Docker, catch exception during cleanup'''
@@ -193,13 +200,14 @@ def test_command_execution_cleanup_exception(self):
         })
 
         build_env = DockerEnvironment(version=self.version, project=self.project,
-                                      build={'id': 123})
+                                      build={'id': DUMMY_BUILD_ID})
         with build_env:
             build_env.run('echo', 'test', cwd='/tmp')
 
         self.mocks.docker_client.kill.assert_called_with(
             'build-123-project-6-pip')
         self.assertTrue(build_env.successful)
+        self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
 
     def test_container_already_exists(self):
         '''Docker container already exists'''
@@ -211,7 +219,7 @@ def test_container_already_exists(self):
         })
 
         build_env = DockerEnvironment(version=self.version, project=self.project,
-                                      build={})
+                                      build={'id': DUMMY_BUILD_ID})
         def _inner():
             with build_env:
                 build_env.run('echo', 'test', cwd='/tmp')
@@ -222,6 +230,7 @@ def _inner():
             'A build environment is currently running for this version')
         self.assertEqual(self.mocks.docker_client.exec_create.call_count, 0)
         self.assertTrue(build_env.failed)
+        self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
 
     def test_container_timeout(self):
         '''Docker container timeout and command failure'''
@@ -241,7 +250,7 @@ def test_container_timeout(self):
         })
 
         build_env = DockerEnvironment(version=self.version, project=self.project,
-                                      build={})
+                                      build={'id': DUMMY_BUILD_ID})
         with build_env:
             build_env.run('echo', 'test', cwd='/tmp')
 
@@ -250,6 +259,7 @@ def test_container_timeout(self):
             'Build exited due to time out')
         self.assertEqual(self.mocks.docker_client.exec_create.call_count, 1)
         self.assertTrue(build_env.failed)
+        self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
 
 
 class TestBuildCommand(TestCase):